feat(sandbox): broker SCM credentials for git operations#679
Open
hreiten wants to merge 2 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughRefactors git auth to a credential-helper flow: control plane exposes POST /sessions/:id/scm-credentials; providers implement generateCredentialHelperAuth(); sandboxes obtain short-lived per-request credentials via a system git credential helper and a GH CLI wrapper; clone-token embedding is removed (legacy fallbacks preserved). ChangesGit Credential Helper Brokering
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.
Inline comments:
In `@packages/sandbox-runtime/src/sandbox_runtime/entrypoint.py`:
- Around line 62-65: The current entrypoint wrapper condition gates all token
checks behind OI_GITHUB_TOKEN_IS_FALLBACK != "1", which prevents
GITHUB_APP_TOKEN from being treated as a real override when fallback is set;
update the shell conditional around the exec of REAL_GH so that GITHUB_APP_TOKEN
is accepted regardless of OI_GITHUB_TOKEN_IS_FALLBACK (i.e., execute REAL_GH if
GITHUB_APP_TOKEN is set OR if OI_GITHUB_TOKEN_IS_FALLBACK != "1" and either
GH_TOKEN or GITHUB_TOKEN is set), leaving the exec "$REAL_GH" "$@" call intact.
- Around line 230-245: In _ensure_credential_helper_configured, the code sets
git's "credential.helper" even when writing the shim (shim_path.write_text with
shim_body) fails; change the flow so the credential.helper is only configured
when the shim actually exists/wrote successfully: after attempting to write and
chmod shim_path (inside the try/except around shim_path.write_text/chmod) set a
local success flag (e.g., shim_written) or re-check shim_path.exists() and only
proceed to set ("credential.helper", str(shim_path)) when that flag/check is
true; ensure the except block does not fall through to configure
credential.helper pointing at a non-existent shim and keep logging the original
warning (self.log.warn("credential_helper.shim_write_failed", error=str(e))).
In `@packages/sandbox-runtime/tests/test_git_credential_helper.py`:
- Around line 85-590: Tests repeat raw timing literals (3600, 60, 0.1); add
module-level named constants (e.g. ONE_HOUR_SECONDS = 3600, ONE_MINUTE_SECONDS =
60, SHORT_SLEEP_SECONDS = 0.1) and replace the numeric literals used in tests
such as test_get_returns_credentials_on_success,
test_refreshes_when_cache_within_expiry_buffer, test_uses_cache_within_buffer,
test_concurrent_invocations_share_one_refresh (slow_handler sleep),
test_control_plane_response_invalid_expiry_is_fatal,
test_token_action_prints_bare_token, and any other occurrences so all
timeouts/TTLs use the new constants to encode units consistently.
- Around line 551-568: The problem is concurrent per-thread patching of
module-global stdio in run_one(), which can clobber patches when two threads
run; remove the patch.object(helper.sys, "stdin"/"stdout"/"stderr") calls from
inside run_one() and instead apply a single patch around the threaded section
(wrap the t1/t2 start/join block) so the global sys streams are patched once, or
alternatively provide thread-local I/O to helper.main without modifying
helper.sys; update the test_concurrent_invocations_share_one_refresh test to
call run_one() without patching helper.sys and perform any required
sys.stdin/stdout/stderr setup outside the threads (or via thread-local
redirect_context) using the unique symbols run_one(), helper.sys.stdin,
helper.sys.stdout, helper.sys.stderr, and helper.main.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 054d978c-199b-42a9-ae48-0d5e505331e1
📒 Files selected for processing (44)
README.mddocs/HOW_IT_WORKS.mddocs/adr/0001-single-provider-scm-boundaries.mddocs/provider-contribution-checklist.mdpackages/control-plane/README.mdpackages/control-plane/src/auth/github-app.test.tspackages/control-plane/src/auth/github-app.tspackages/control-plane/src/router.scm-credentials.test.tspackages/control-plane/src/router.tspackages/control-plane/src/routes/shared.tspackages/control-plane/src/sandbox/providers/daytona-provider.test.tspackages/control-plane/src/sandbox/providers/daytona-provider.tspackages/control-plane/src/session/contracts.tspackages/control-plane/src/session/durable-object.tspackages/control-plane/src/session/http/handlers/sandbox.handler.test.tspackages/control-plane/src/session/http/handlers/sandbox.handler.tspackages/control-plane/src/session/http/routes.test.tspackages/control-plane/src/session/http/routes.tspackages/control-plane/src/session/scm-credentials-service.test.tspackages/control-plane/src/session/scm-credentials-service.tspackages/control-plane/src/source-control/errors.tspackages/control-plane/src/source-control/index.tspackages/control-plane/src/source-control/provider-from-env.test.tspackages/control-plane/src/source-control/provider-from-env.tspackages/control-plane/src/source-control/providers/github-provider.test.tspackages/control-plane/src/source-control/providers/github-provider.tspackages/control-plane/src/source-control/providers/gitlab-provider.test.tspackages/control-plane/src/source-control/providers/gitlab-provider.tspackages/control-plane/src/source-control/types.tspackages/control-plane/test/integration/scm-credentials.test.tspackages/daytona-infra/src/toolchain.pypackages/github-bot/README.mdpackages/modal-infra/src/images/base.pypackages/modal-infra/src/sandbox/manager.pypackages/modal-infra/src/web_api.pypackages/modal-infra/tests/test_sandbox_env_vars.pypackages/modal-infra/tests/test_web_api_create_sandbox.pypackages/sandbox-runtime/src/sandbox_runtime/credentials/__init__.pypackages/sandbox-runtime/src/sandbox_runtime/credentials/git_credential_helper.pypackages/sandbox-runtime/src/sandbox_runtime/entrypoint.pypackages/sandbox-runtime/tests/test_entrypoint_build_mode.pypackages/sandbox-runtime/tests/test_entrypoint_urls.pypackages/sandbox-runtime/tests/test_gh_wrapper.pypackages/sandbox-runtime/tests/test_git_credential_helper.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
We hit this in our own Open-Inspect deployment: long-running or revisited sessions could no longer run git commands after the GitHub App installation token expired. The common workflow was to kick off an agent, come back more than an hour later, and continue the same session. At that point follow-up fetch/push/ls-remote operations were still using the token captured when the sandbox was created, so git failed even though the session itself was otherwise healthy.
What changed
POST /sessions/:id/scm-credentialsbroker endpoint that mints fresh SCM credentials through the configured provider.originremotes to plain HTTPS URLs before fetch so older snapshots route through the helper.ghwrapper for GitHub CLI calls, sinceghreads env tokens rather than git's credential protocol.Security and compatibility
The helper fails closed by scoping credential responses to HTTPS, the configured SCM host, and the current session repository path, with Git LFS path support. It caches credentials locally with a short refresh buffer and
0600permissions, and it refuses stale-cache fallback on broker failures. Live sessions also refuse to fall back to staticVCS_CLONE_TOKENwhen control-plane context is present but incomplete.Validation
npm test -w @open-inspect/control-planenpm run test:integration -w @open-inspect/control-plane -- test/integration/scm-credentials.test.tsnpm run typecheck -w @open-inspect/control-planenpm run lint -w @open-inspect/control-planenpm run build -w @open-inspect/control-planecd packages/sandbox-runtime && uv run --extra dev pytest tests/test_git_credential_helper.py tests/test_gh_wrapper.py tests/test_entrypoint_build_mode.py tests/test_entrypoint_urls.py -qcd packages/sandbox-runtime && uv run --extra dev ruff check src tests && uv run --extra dev ruff format --check src testscd packages/modal-infra && uv run --extra dev pytest tests/test_sandbox_env_vars.py tests/test_web_api_create_sandbox.py -qcd packages/modal-infra && uv run --extra dev ruff check src tests && uv run --extra dev ruff format --check src testspython3 -m compileall packages/daytona-infra/srcSummary by CodeRabbit
New Features
Documentation
Infrastructure / Runtime
Tests