fix: verify sandbox git remote matches expected repo#82
Open
sweetmantech wants to merge 11 commits intomainfrom
Open
fix: verify sandbox git remote matches expected repo#82sweetmantech wants to merge 11 commits intomainfrom
sweetmantech wants to merge 11 commits intomainfrom
Conversation
…shot Snapshots can carry a stale git remote from a different account's sandbox. When .git already exists, check that origin matches the expected githubRepo and update it if not. Also adds remoteUrl to push logs for easier debugging. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds remote URL verification to the GitHub repository setup process within sandboxes and enhances logging in the push operation. When a repository is already cloned, the remote origin URL is fetched, normalized, and compared against the expected GitHub repository. If mismatched, the origin is automatically rewritten to the authenticated repository URL. Push operations now log the actual remote URL being used. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/sandboxes/pushSandboxToGithub.ts (1)
22-26: Consider checkingexitCodebefore usingstdoutfor robustness.The remote URL is read without verifying that the command succeeded. If
git remote get-url originfails (e.g., no remote configured),remoteUrlwill be empty or misleading. WhileensureGithubReporuns before this function (per context snippet 4), adding defensive error handling would be consistent with the pattern inrunGitCommand.ts.🔧 Proposed fix
const remoteCheck = await sandbox.runCommand({ cmd: "git", args: ["remote", "get-url", "origin"], }); - const remoteUrl = ((await remoteCheck.stdout()) || "").trim(); + const remoteUrl = + remoteCheck.exitCode === 0 + ? ((await remoteCheck.stdout()) || "").trim() + : "<unknown>"; logger.log("Pushing sandbox files to GitHub", { remoteUrl });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sandboxes/pushSandboxToGithub.ts` around lines 22 - 26, Check the git command's exit code before using stdout: after calling sandbox.runCommand (remoteCheck) verify remoteCheck.exitCode (or equivalent) indicates success and only then use remoteCheck.stdout() to set remoteUrl; if the command failed, handle it consistently (e.g., log or throw a descriptive error and treat remoteUrl as missing) similar to patterns in runGitCommand.ts and in callers like ensureGithubRepo so you don't proceed with an empty/misleading remoteUrl.src/sandboxes/ensureGithubRepo.ts (2)
72-76: Consider checkingexitCodebefore usingstdout.Similar to the pattern in
pushSandboxToGithub.ts, the remote URL is read without verifying command success. While this is inside the.gitdirectory check block, if the origin remote somehow doesn't exist,currentRemotewould be empty. The current behavior would still work (empty string won't matchgithubRepo, triggering the update), but explicit error handling would be cleaner.🔧 Proposed fix
const remoteResult = await sandbox.runCommand({ cmd: "git", args: ["remote", "get-url", "origin"], }); - const currentRemote = ((await remoteResult.stdout()) || "").trim(); + const currentRemote = + remoteResult.exitCode === 0 + ? ((await remoteResult.stdout()) || "").trim() + : "";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sandboxes/ensureGithubRepo.ts` around lines 72 - 76, In ensureGithubRepo.ts, before reading remoteResult.stdout() from the sandbox.runCommand call that fetches "git remote get-url origin", check remoteResult.exitCode (or the equivalent success indicator) and handle non-zero exit by logging or treating currentRemote as empty; update the code around the remoteResult variable to mirror the pattern used in pushSandboxToGithub.ts (verify exitCode, capture stderr for diagnostics, and only use stdout when exitCode === 0) so you don't rely on stdout when the git command failed or the origin remote is missing.
89-93: The return value ofrunGitCommandis not checked.If the remote update fails, the function continues and returns
githubRepoas if successful. The subsequent push would fail, but with a less specific error. Consider checking the result and either logging a warning or returningundefinedon failure.🔧 Proposed fix to handle failure
- await runGitCommand( + const updated = await runGitCommand( sandbox, ["remote", "set-url", "origin", repoUrl], "update remote to correct repo" ); + if (!updated) { + logger.error("Failed to update sandbox remote", { expected: githubRepo }); + return undefined; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sandboxes/ensureGithubRepo.ts` around lines 89 - 93, In ensureGithubRepo, the call to runGitCommand([... "remote", "set-url", "origin", repoUrl]) isn't checked so failures are ignored; change ensureGithubRepo to capture the return value of runGitCommand, and if it indicates failure (falsy/false), log a warning via the existing logger or processLogger and return undefined (or throw) instead of proceeding to push; ensure any subsequent code that uses githubRepo only runs when the remote update succeeded so the function doesn't erroneously return githubRepo after a failed runGitCommand.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/sandboxes/ensureGithubRepo.ts`:
- Around line 72-76: In ensureGithubRepo.ts, before reading
remoteResult.stdout() from the sandbox.runCommand call that fetches "git remote
get-url origin", check remoteResult.exitCode (or the equivalent success
indicator) and handle non-zero exit by logging or treating currentRemote as
empty; update the code around the remoteResult variable to mirror the pattern
used in pushSandboxToGithub.ts (verify exitCode, capture stderr for diagnostics,
and only use stdout when exitCode === 0) so you don't rely on stdout when the
git command failed or the origin remote is missing.
- Around line 89-93: In ensureGithubRepo, the call to runGitCommand([...
"remote", "set-url", "origin", repoUrl]) isn't checked so failures are ignored;
change ensureGithubRepo to capture the return value of runGitCommand, and if it
indicates failure (falsy/false), log a warning via the existing logger or
processLogger and return undefined (or throw) instead of proceeding to push;
ensure any subsequent code that uses githubRepo only runs when the remote update
succeeded so the function doesn't erroneously return githubRepo after a failed
runGitCommand.
In `@src/sandboxes/pushSandboxToGithub.ts`:
- Around line 22-26: Check the git command's exit code before using stdout:
after calling sandbox.runCommand (remoteCheck) verify remoteCheck.exitCode (or
equivalent) indicates success and only then use remoteCheck.stdout() to set
remoteUrl; if the command failed, handle it consistently (e.g., log or throw a
descriptive error and treat remoteUrl as missing) similar to patterns in
runGitCommand.ts and in callers like ensureGithubRepo so you don't proceed with
an empty/misleading remoteUrl.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f38f1e50-a8f0-4ff6-ba7c-4e620c253f65
📒 Files selected for processing (2)
src/sandboxes/ensureGithubRepo.tssrc/sandboxes/pushSandboxToGithub.ts
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.
Summary
originmatches the expectedgithubRepoURLremoteUrlto push logs for easier debuggingRoot cause
Snapshots carry
.git/configwith the remote URL from when the snapshot was taken. If an account's sandbox was created from a snapshot belonging to a different account (e.g., the org root account), the remote points to the wrong repo.ensureGithubRepopreviously trusted the existing.gitdirectory without verifying the remote, causing pushes to land in the wrong repo.Test plan
remoteUrlmatching the expected repo🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements