refactor(sandbox): move gh token precedence into the credential helper#683
refactor(sandbox): move gh token precedence into the credential helper#683ColeMurray wants to merge 1 commit into
Conversation
The gh CLI wrapper carried a 4-branch /bin/sh token-precedence decision tree, including a brittle `GITHUB_TOKEN != GITHUB_APP_TOKEN` value-equality heuristic. Move that decision into the Python credential helper as a pure, unit-testable `_gh_wrapper_should_mint()` behind a new `gh-token` action, and reduce the wrapper to a thin delegator. - Drop the value-equality heuristic; rely on the authoritative OI_GITHUB_TOKEN_IS_FALLBACK marker (a marked fallback always refreshes). - Export GH_TOKEN before exec instead of `env GH_TOKEN=… exec`, so the token never lands in process argv. - Port the shell decision-tree tests to Python (decision matrix + action tests); slim the shell test to delegator wiring + argv-cleanliness. Also converts two pre-existing `isinstance(x, (int, float))` calls to union syntax (UP038) so the file is lint-clean under current ruff.
📝 WalkthroughWalkthroughThis PR refactors GitHub token minting by moving precedence logic from a complex shell wrapper into Python credential helper functions. The new ChangesGitHub Token Minting Refactoring
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
![open-inspect[bot]](https://avatars.githubusercontent.com/u/2492022?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
PR #683, refactor(sandbox): move gh token precedence into the credential helper, by @ColeMurray. Reviewed 4 changed files (+206/-176); the refactor moves the gh token precedence decision into the Python helper, keeps the shell wrapper thin, and adds targeted tests for the decision matrix and wrapper wiring.
Critical Issues
None found.
Suggestions
None blocking.
Nitpicks
None.
Positive Feedback
- The token precedence rules are now centralized in a pure helper function with a clear decision matrix, which is much easier to test and maintain than shell branching.
- Exporting
GH_TOKENbeforeexecavoids putting the minted token in process argv. - The tests cover the important fallback-marker and user-token precedence cases, plus shell behavior for empty and failed helper output.
Questions
None.
Verdict
Approve.
Verification: attempted pytest tests/test_gh_wrapper.py tests/test_git_credential_helper.py, but pytest is not installed in this environment. Ran manual checks against _gh_wrapper_should_mint with PYTHONPATH=src successfully.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/sandbox-runtime/tests/test_gh_wrapper.py (1)
93-106: ⚡ Quick winAdd the existing-
GH_TOKENfallthrough case.These tests only prove the wrapper preserves
GITHUB_TOKENwhen the helper prints nothing or exits nonzero. The delegated contract inpackages/sandbox-runtime/tests/test_git_credential_helper.py:559-621also treats a user-providedGH_TOKENas authoritative, so it's worth locking that down at the shell layer too.🧪 Suggested test
+def test_preserves_existing_gh_token_when_helper_prints_nothing(tmp_path: Path) -> None: + wrapper = _build_wrapper(tmp_path, token_cmd_body=PRINTS_NOTHING) + out = _run(wrapper, {"VCS_HOST": "github.com", "GH_TOKEN": "user_token"}) + assert "GH_TOKEN=user_token" in out🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sandbox-runtime/tests/test_gh_wrapper.py` around lines 93 - 106, Add a new test to cover the fallthrough when an existing GH_TOKEN is present in the environment: mirror the patterns in test_no_export_when_helper_prints_nothing and test_falls_through_when_helper_fails but call _build_wrapper with PRINTS_NOTHING and EXITS_NONZERO (or one of them) and invoke _run with an environment that includes both GITHUB_TOKEN and GH_TOKEN (e.g., "GITHUB_TOKEN":"user_token", "GH_TOKEN":"preexisting_token"); assert the output contains "GH_TOKEN=preexisting_token" and that "GITHUB_TOKEN=user_token" is still present to ensure the wrapper preserves a user-provided GH_TOKEN.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/sandbox-runtime/tests/test_gh_wrapper.py`:
- Around line 93-106: Add a new test to cover the fallthrough when an existing
GH_TOKEN is present in the environment: mirror the patterns in
test_no_export_when_helper_prints_nothing and
test_falls_through_when_helper_fails but call _build_wrapper with PRINTS_NOTHING
and EXITS_NONZERO (or one of them) and invoke _run with an environment that
includes both GITHUB_TOKEN and GH_TOKEN (e.g., "GITHUB_TOKEN":"user_token",
"GH_TOKEN":"preexisting_token"); assert the output contains
"GH_TOKEN=preexisting_token" and that "GITHUB_TOKEN=user_token" is still present
to ensure the wrapper preserves a user-provided GH_TOKEN.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5d29383-90bb-447c-a2e0-fad3a57cf7e4
📒 Files selected for processing (4)
packages/sandbox-runtime/src/sandbox_runtime/credentials/git_credential_helper.pypackages/sandbox-runtime/src/sandbox_runtime/entrypoint.pypackages/sandbox-runtime/tests/test_gh_wrapper.pypackages/sandbox-runtime/tests/test_git_credential_helper.py
1 participant
Summary
/bin/shtoken-precedence decision tree into the Python credential helper as a pure, unit-testable_gh_wrapper_should_mint()behind a newgh-tokenaction; the shell wrapper becomes a thin delegator.GITHUB_TOKEN != GITHUB_APP_TOKENvalue-equality heuristic in favor of the authoritativeOI_GITHUB_TOKEN_IS_FALLBACKmarker.GH_TOKENbeforeexec(instead ofenv GH_TOKEN=… exec), so the token never appears in process argv.Follow-up to #679 — items P1-1 and P1-2 from the post-merge maintainability review.
Behavior change (intentional)
With
OI_GITHUB_TOKEN_IS_FALLBACK=1, gh now always refreshes. Previously, ifGITHUB_TOKENandGITHUB_APP_TOKENdiffered while the marker was set, the wrapper passed through with the (stale)GITHUB_TOKEN— which gh reads, while it ignoresGITHUB_APP_TOKEN. The only case where old ≠ new requires manually re-exporting an internal env var inside the running sandbox; the supported override path (repo secrets) never sets the marker, so user-provided tokens are still respected. Net: strictly better in every supported case.Out of scope / unchanged
get/store/eraseprotocol actions.base.py/toolchain.py/ entrypoint runtime) — action-agnostic, unaffected by thetoken→gh-tokenrename.CACHE_BUSTERbump needed: the wrapper is rewritten on every boot, and the baked shim is unchanged.Incidental
Two pre-existing
isinstance(x, (int, float))calls converted to union syntax (UP038) so the file is lint-clean under current ruff (valid on every ruff version).Test plan
ruff check+ruff formatclean on the changed filespytest tests/test_gh_wrapper.py tests/test_git_credential_helper.py— 44 passed locallyGH_TOKEN, userGITHUB_TOKEN/GITHUB_APP_TOKEN, marked fallback (incl. differing values), andGH_TOKENwinning over a markerGH_TOKENand never in argv; empty/failed helper falls through to the existing envSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests