Skip to content

feat: add rtcamp app to ci and fix ci perms - #1979

Merged
robinroy03 merged 9 commits into
version-16-hotfixfrom
feat/improve-testing
Aug 10, 2026
Merged

feat: add rtcamp app to ci and fix ci perms#1979
robinroy03 merged 9 commits into
version-16-hotfixfrom
feat/improve-testing

Conversation

@robinroy03

@robinroy03 robinroy03 commented Aug 5, 2026

Copy link
Copy Markdown
Member

add the rtcamp and depended apps to ci to avoid test skips
if a fork pr, skip those tests to prevent secret sharing
fix the permission issues
gmail thread branch flipped to dev

fixes https://github.com/rtCamp/next-pms/security/code-scanning/24 and #1901

@robinroy03
robinroy03 requested review from iamimmanuelraj and a balanced review from Copilot August 5, 2026 12:06
@robinroy03 robinroy03 linked an issue Aug 5, 2026 that may be closed by this pull request
1 task

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Adds the rtcamp app to the unit-test CI setup so the test environment matches production more closely and avoids conditional/skip behavior due to missing apps.

Changes:

  • Switch frappe_gmail_thread to the version-16-hotfix branch in CI.
  • Add rtcamp app fetch (authenticated) and include it in install-app during unit tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .github/workflows/unit-tests.yml Outdated
Comment thread .github/workflows/unit-tests.yml Outdated
@robinroy03 robinroy03 changed the title feat: add rtcamp app to ci feat: add rtcamp app to ci and fix ci perms Aug 5, 2026
@robinroy03
robinroy03 removed the request for review from iamimmanuelraj August 6, 2026 06:53

@iamimmanuelraj iamimmanuelraj left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direction looks good — the permissions hardening now covers all 9 workflows (code-scanning/24 fully closed), and the fork path degrades instead of failing. One real bug, one security tightening, rest are nits.

1. The uv cache addition is currently a no-op (bug)

unit-tests.yml adds ~/.cache/uv to the cache path, but the key is unchanged (${{ runner.os }}-pip-${{ hashFiles('**/pyproject.toml') }}). actions/cache skips the save step on an exact key hit, so ~/.cache/uv will never actually be written until pyproject.toml changes. Bump the key, e.g. ${{ runner.os }}-pip-uv-${{ hashFiles('**/pyproject.toml') }} (and the restore-keys prefix to match).

2. The token stays in ~/.gitconfig through the test step (security)

git config --global url...insteadOf writes RTBOT_TOKEN in plaintext to $HOME/.gitconfig, and it's still there when bench run-tests executes repo code. The earlier Copilot comment was about URL-embedding in the remote — insteadOf fixes that, but not this. Suggest unsetting at the end of the Install step:

git config --global --unset "url.https://rtbot:${RTBOT_TOKEN}@github.com/.insteadOf"

Ephemeral runner plus secret masking limit the blast radius, so this is medium-low, not blocking.

3. The insteadOf rewrite is org-wide

It rewrites every https://github.com/ fetch, including resilient-tech/india-compliance (third-party, public) and any transitive git dependencies pulled during bench get-app. Same host, so no cross-host leak, but worth scoping:

git config --global "url.https://rtbot:${RTBOT_TOKEN}@github.com/rtCamp/.insteadOf" "https://github.com/rtCamp/"

Worth double-checking that the RTCAMP_APP host matches that prefix.

4. Moving branches aren't pinned

telephony --branch develop, helpdesk --branch main, and crm --branch main, while everything else pins version-16 / version-16-hotfix. That makes CI non-deterministic, and telephony develop tracks frappe develop, so it will drift and break against version-16 eventually. Use version-16 branches if they exist.

5. The frappe_gmail_thread branch flip isn't in the PR description

version-16version-16-hotfix. Plausible given the base branch, but it's unmentioned, and the pin rides along on a forward-merge to version-16. Can you confirm it's intentional?

6. The fork skip is silent

Fork PRs go green with materially less coverage, and the only signal is an echo buried in the log. Consider writing a line to $GITHUB_STEP_SUMMARY so reviewers see it on the PR itself.

7. "rtcamp-gated tests will skip" isn't enforced by this diff

Nothing here gates tests. It's safe today, since the fork path is identical to pre-PR behavior and tests can't regress. But if tests requiring these apps land later without frappe.get_installed_apps() guards, forks will error rather than skip. Is that gating in place on the test side?


Checked and fine: $PRIVATE_APPS unquoted is correct word-splitting; the fork secrets-empty guard behaves correctly; all checks pass (Tests 9m8s).

@robinroy03
robinroy03 merged commit 0da4ee0 into version-16-hotfix Aug 10, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add private rtCamp apps to the test suite

3 participants