feat: add rtcamp app to ci and fix ci perms - #1979
Conversation
There was a problem hiding this comment.
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_threadto theversion-16-hotfixbranch in CI. - Add
rtcampapp fetch (authenticated) and include it ininstall-appduring unit tests.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
iamimmanuelraj
left a comment
There was a problem hiding this comment.
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-16 → version-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).
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