fix(sandbox): classify git push as network access - #726
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughGit network detection now handles value-taking global options, remote archives, wrappers, and platform-specific Git executables. Parsed and unparseable network commands receive network classification, with regression coverage for network grants and approved ChangesSandbox network classification
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant SandboxClassifier
participant GitAnalyzer
participant GitProcess
Agent->>SandboxClassifier: request git push
SandboxClassifier->>GitAnalyzer: classify Git subcommand
GitAnalyzer-->>SandboxClassifier: critical network risk
SandboxClassifier-->>Agent: request network approval
Agent->>GitProcess: execute approved git push
GitProcess-->>Agent: return command output
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR updates the sandbox command analyzer/risk classifier to treat git push as network-sensitive, with added tests to ensure the AST-based analyzer flags it even when the command doesn’t contain an obvious URL.
Changes:
- Extend
commandUsesNetworkto classifygit pushas network access. - Add analyzer coverage for
git push(and a non-networkgit commit) inAnalyzeCommandtests. - Add a risk-classifier hardening test asserting
git pushis flagged as critical+network when regex-based detection would miss it.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| internal/sandbox/analyzer.go | Expands git subcommand network detection to include push. |
| internal/sandbox/analyzer_test.go | Adds AnalyzeCommand test cases for git push and a local-only git commit. |
| internal/sandbox/risk_hardening_test.go | Adds a hardening test to ensure AST-based classification flags git push as network. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/sandbox/risk_hardening_test.go (1)
297-309: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
t.Errorfovert.Fatalfin loops.Using
t.Fatalfinside a loop will immediately abort the test on the first failure, which prevents the remaining test cases from executing. Replacing it witht.Errorfallows all cases to be evaluated even if one fails.♻️ Proposed refactor
for _, command := range []string{ `curl https://example.com && "unterminated`, `git fetch origin && "unterminated`, `git pull origin main && "unterminated`, `git push gitlawb://example.com/repo.git main && "unterminated`, } { risk := classifyCommand(command) if !HasRiskCategory(risk, "unparseable_command") { - t.Fatalf("Classify(%q) = categories %v; want unparseable_command", command, risk.Categories) + t.Errorf("Classify(%q) = categories %v; want unparseable_command", command, risk.Categories) } if risk.Level != RiskCritical || !HasRiskCategory(risk, "network") { - t.Fatalf("Classify(%q) = level %s, categories %v; want critical network", command, risk.Level, risk.Categories) + t.Errorf("Classify(%q) = level %s, categories %v; want critical network", command, risk.Level, risk.Categories) } }🤖 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 `@internal/sandbox/risk_hardening_test.go` around lines 297 - 309, In the table-driven loop testing classifyCommand, replace both t.Fatalf calls with t.Errorf so each command case is evaluated even when an earlier assertion fails.
🤖 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 `@internal/sandbox/risk_hardening_test.go`:
- Around line 297-309: In the table-driven loop testing classifyCommand, replace
both t.Fatalf calls with t.Errorf so each command case is evaluated even when an
earlier assertion fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 45f38035-3958-4d77-b84f-f163855e8c49
📒 Files selected for processing (5)
internal/agent/loop_test.gointernal/sandbox/analyzer.gointernal/sandbox/analyzer_test.gointernal/sandbox/risk.gointernal/sandbox/risk_hardening_test.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Thanks for this, and the direction is right (git push should be network-gated). But the AST classifier still misses the most common git form, so I would like a fix before it lands.
The git branch of commandUsesNetwork calls firstSubcommand, which skips only dash-prefixed and numeric tokens. git's value-consuming global options put their value in the NEXT token, so firstSubcommand returns that value as the "subcommand." I ran the classifier against the current head:
git push origin main Network=true Risk=critical network <- correct
git -C repo push origin main Network=false Risk=high (none) <- missed
git -c http.sslVerify=false push Network=false Risk=high (none) <- missed
git --git-dir /x/.git push Network=false Risk=high (none) <- missed
git.exe push origin main Network=false Risk=high (none) <- missed
These all parse cleanly, so TooComplex stays false and the unparseable-pattern fallback never runs. So git -C <dir> push (the canonical form for operating on a repo without cd) classifies as plain shell, not network, and its risk drops from Critical to High.
To be fair on severity: this is not an always-open egress hole. When the sandbox backend is provisioned, the runtime deny-by-default still blocks the socket and raises the network prompt via ReasonNetworkBlocked, so the classifier is defense-in-depth there. But it becomes a real unprompted-egress path when the backend is unavailable or degraded, and the Critical-to-High mis-level can flip auto-allow in the more permissive autonomy modes regardless. Since the whole point of the PR is to classify these, I would rather close the gap than ship a gate that misses the most common invocation.
The fix looks small:
- In the git case, skip the values of git's space-separated value-consuming globals (-C, -c, --git-dir, --work-tree, --namespace, --exec-path, --super-prefix) before taking the subcommand. The joined --git-dir=/x form is already fine since it is one dash-prefixed token.
- Normalize a .exe program token so git.exe is treated as git.
- Add a PARSEABLE regression test: classifyCommand("git -C repo push origin main") should be RiskCritical with the network category. Right now the only -C test is the one with the trailing
&& "unterminated, which forces the unparseable path and masks this AST gap.
Otherwise the wiring is fine, and build/vet/gofmt are clean locally. Happy to re-review quickly once the AST path handles the option forms.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
APPROVE — the core fix is correct and well-covered: git clone|fetch|pull|push now classify as network access on the primary AST path, verified end-to-end (git push origin main → Network=true, git commit -m x stays Network=false), and the hardened regex fallback fails closed on unparseable variants. The one remaining gap is a minor consistency issue, not merge-blocking.
Nice work: the AST change (analyzer.go:153) and the fallback hardening (risk.go:36, now git(?:\s+[^\s;&|]+){0,8}\s+(clone|fetch|pull|push)) are both landed, and the new tests are real — TestAnalyzeCommand (git fetch/pull/push-custom-transport → network), TestClassifyASTCatchesNetworkProgramsRegexMisses, and TestClassifyUnparseableNetworkCommandFailsClosed including the git -C repo push … && "unterminated fail-closed case. All PR-relevant classification tests pass locally.
[Minor] AST path doesn't skip git global options, so git -C <dir> push isn't classified as network — inconsistent with the fallback you just hardened
internal/sandbox/analyzer.go:153 (root cause: firstSubcommand, analyzer.go:243) — pre-existing blind spot, PR-introduced inconsistency
The three reported findings all collapse to this single root cause. firstSubcommand skips dash-prefixed tokens but treats the next bare token as the subcommand, so for git -C /repo push origin main (words [-C, /repo, push, …]) it returns /repo, not push. The git case then returns Network=false. Runtime probe on the PR HEAD:
git push origin main Network=true TooComplex=false
git -C /repo push origin main Network=false TooComplex=false <- gap
git -c http.proxy=x push origin main Network=false TooComplex=false <- gap
git -C /repo fetch origin Network=false TooComplex=false <- gap
git -C /repo pull origin main Network=false TooComplex=false <- gap
git --git-dir=/repo/.git push origin main Network=true TooComplex=false (caught: --foo starts with '-')
The gap is specifically the space-separated value-taking global options (-C <path>, -c <name=value>, --git-dir <path>, --work-tree <path>, --namespace <ns>, --exec-path <path>). Because these commands parse cleanly (TooComplex=false), the hardened unparseableNetworkPattern at risk.go:36 is never consulted — that branch is gated on analysis.TooComplex at risk.go:132. So the fallback now tolerates git -C repo push but the primary AST path does not: the two paths disagree, and the network category the PR exists to add is silently omitted for the very common git -C <dir> push/fetch/pull form.
Impact is bounded — this is not a network-exfiltration bypass. Network enforcement mode is derived from policy.Network via NormalizeNetworkMode at profile.go:109, decoupled from the analyzer, and the auto-allow branch at engine.go:410 is gated on shellSandboxActive → NativeIsolation. So a misclassified git -C . push still runs wrapped by the platform sandbox with NetworkDeny enforced at the syscall level (the connect() is blocked and the agent reactively prompts), and where no native sandbox is active it prompts anyway via the general path rather than auto-allowing. The only real-world effect is degrading a proactive ReasonNetworkBlocked prompt (engine.go:355/357) into a reactive/generic one, plus the AST↔regex inconsistency.
Provenance: the underlying firstSubcommand blind spot is pre-existing — base analyzer.go:153 was firstSubcommand(words, nil) == "clone" and had the same hole for git -C <dir> clone. What this PR introduces is the inconsistency: it extended classification to push/fetch/pull and explicitly closed the -C gap in the regex fallback (and tests it), but left the primary AST path unfixed.
Suggested fix: give the git case a dedicated subcommand resolver that consumes git's global value-taking options before reading the subcommand (-C <path>, -c <name=value>, --git-dir, --work-tree, --namespace, --exec-path in their separate-token form), mirroring the tolerance already in unparseableNetworkPattern, then test the resolved token against {clone,fetch,pull,push}. Add git -C repo push origin main (plus -c / fetch / pull variants) to TestClassifyASTCatchesNetworkProgramsRegexMisses and TestAnalyzeCommand — those assertions fail today and would pin the fix.
Tests: go build ./..., go vet on the touched packages, and gofmt -l are clean; all PR-relevant classification/agent tests pass. The failing tests in internal/sandbox and internal/agent (path/symlink/out-of-workspace under /private/tmp) are pre-existing environmental failures that reproduce identically on base — not PR-attributable.
Merge is kevin's call per the program gate.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
Resolve the merge conflict with
main
GitHub currently reports this PR asCONFLICTING/DIRTY, so it cannot be merged or tested in its target-branch composition. Rebase or merge the current target branch and resolve the conflict before requesting another review. -
[P2] Classify Git commands after consuming global-option values
internal/sandbox/analyzer.go:153
The genericfirstSubcommandskips-C/-c/--git-dirthemselves but treats their following values as the subcommand. Consequently ordinary, parseable commands such asgit -C repo push origin main,git -c http.proxy=x fetch origin, andgit --git-dir /repo/.git pullproduce nonetworkrisk. Because they parse successfully, the new regex fallback is not consulted, and the engine skips its required proactive network-approval path. Use a Git-aware resolver that consumes value-taking global options (asinternal/agent/command_prefix.goalready does) and add parseable regression coverage. -
[P2] Recognize the Windows
git.execommand spelling
internal/sandbox/analyzer.go:152
effectiveProgramnormalizesgit.exetogit.exe, notgit, sogit.exe push origin mainnever reaches this new Git network classifier. It is parseable, so the fallback cannot repair the miss and the command does not receive the intended proactive network prompt. Normalize executable suffixes (or explicitly handlegit.exe) and cover that spelling in the analyzer and risk tests.
…ush-network main's #7xx already classifies git clone/fetch/pull/push/ls-remote/archive as network in gitUsesNetwork, so the conflict resolves to main's version and this branch keeps only what is still missing: - gitSubcommand resolves the subcommand past git's value-taking global options (-C, -c, --config-env, --exec-path, --git-dir, --namespace, --super-prefix, --work-tree). firstSubcommand treated their VALUE as the subcommand, so `git -C repo push origin main` — the canonical form for operating on a repo without cd — classified as plain shell with no network category. Those commands parse cleanly, so the unparseable-command regex fallback never ran. - effectiveProgram trims a trailing .exe, so `git.exe push` (and curl.exe, wget.exe, ...) reach the same classification as the bare name. Only .exe is trimmed; a .bat/.cmd of the same stem is a different script. - Parseable regression coverage for both, in the analyzer table and a new risk test that asserts TooComplex is false so the fallback cannot mask a future AST gap, plus the local-work cases that must stay off the network path. The hardened fallback regex, the t.Errorf-in-subtests conversion, and the end-to-end approved-git-push network-grant test come from this branch unchanged.
|
Merged current Resolve the merge conflict with [P2] Classify Git commands after consuming global-option values (@jatmn, @gnanam1990, @Vasanthdev2004 — all three findings share this root cause) — fixed with a git-aware
[P2] Recognize the Windows Parseable regression coverage — the key point from @Vasanthdev2004's review was that the only @copilot: "the approved-prompt behavior may need an execution-path change, not just classification" — checked, and no execution change is needed. The turn network grant already applies on approval; the missing piece was purely classification, so the command never reached that path. @copilot: use a Validation (Windows host, Go 1.26.5): @jatmn @Vasanthdev2004 @gnanam1990 — ready for another look; I can't use the reviewer-request button on this repo, hence the mention. @coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/sandbox/risk_hardening_test.go (1)
337-342: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCover
git.exein the unparseable network fallback.
unparseableNetworkPatternmatchesgitonly, so an unparseablegit.exe push ...misses the criticalnetworkcategory even though parseablegit.exeis classified correctly. Accept an optional.exesuffix and add that regression case.Proposed fix
-|\bgit(?:\s+[^\s;&|]+){0,8}\s+(clone|fetch|pull|push)\b +|\bgit(?:\.exe)?(?:\s+[^\s;&|]+){0,8}\s+(clone|fetch|pull|push)\b+ `git.exe push gitlawb://example.com/repo.git main && "unterminated`,As per coding guidelines,
**/*_test.gorequires regression tests for behavior changes.🤖 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 `@internal/sandbox/risk_hardening_test.go` around lines 337 - 342, Update the unparseableNetworkPattern to match both git and git.exe command names while preserving the existing network-command requirements. In the risk-hardening regression table in the relevant test, add an unparseable git.exe push case so it is classified under the network category.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@internal/sandbox/risk_hardening_test.go`:
- Around line 337-342: Update the unparseableNetworkPattern to match both git
and git.exe command names while preserving the existing network-command
requirements. In the risk-hardening regression table in the relevant test, add
an unparseable git.exe push case so it is classified under the network category.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b9a21d6-40d7-4443-869c-0714562d2150
📒 Files selected for processing (5)
internal/agent/loop_test.gointernal/sandbox/analyzer.gointernal/sandbox/analyzer_test.gointernal/sandbox/risk.gointernal/sandbox/risk_hardening_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/sandbox/risk.go
- internal/agent/loop_test.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Recognize
--attr-sourceas a value-taking Git global option
internal/sandbox/analyzer.go:308
Git accepts--attr-source <tree-ish>before the subcommand (for example,git --attr-source HEAD push origin main), but it is absent from this new skip list.gitSubcommandtherefore returnsheadinstead ofpush; because this command parses successfully, the fallback is not consulted and the shell call never receives the criticalnetworkclassification or its network-enabled approval profile. Add this global option and a regression case; update the paired command-prefix parser too if the documented mirroring is intentional. -
[P2] Make the unparseable Git fallback cover the supported Windows form and option count
internal/sandbox/risk.go:36
The new AST path normalizesgit.exe, but parser-failing Windows commands never reach that code. For example,git.exe push origin main & rem 'runs undercmd.exebut is rejected by the POSIX parser; this pattern requires whitespace immediately aftergit, so classification adds onlyunparseable_command, notnetwork, and skips the network approval/turn-grant path. The{0,8}cap has the same failure once a Git invocation has more than four value-taking global options. Match an optional.exesuffix and scan Git tokens up to a command separator without the arbitrary cap, with regressions for both forms.
…allback gitSubcommand (and its mirror in internal/agent/command_prefix.go) was missing --attr-source from git's value-taking global options, so `git --attr-source HEAD push origin main` resolved to the wrong subcommand and never got classified as network access. The unparseable-command regex fallback used when the shell parser fails now also matches an optional .exe suffix, so a Windows form like `git.exe push origin main & rem '` — valid under cmd.exe but rejected by the POSIX parser AnalyzeCommand uses — still classifies as network. The generic-token scan before the subcommand no longer caps at 8 tokens; Go's regexp package is RE2-backed (linear time, no backtracking blowup), so the cap only served to silently drop coverage once a git invocation had more than a handful of value-taking global options. Addresses review feedback from jatmn on PR Gitlawb#726. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/sandbox/risk.go`:
- Around line 36-44: The unparseable Git fallback in unparseableNetworkPattern
must not treat arbitrary tokens before push, fetch, or pull as global options.
Restrict matching to recognized Git global options and their values, or reuse
the shared token-aware resolver from analyzer.go, while preserving support for
git.exe and complex valid invocations. Add a regression test covering a local
command such as git status push so it is not classified as network.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 59e3c8be-fb6a-46bd-8c23-15d475e08ebd
📒 Files selected for processing (5)
internal/agent/command_prefix.gointernal/sandbox/analyzer.gointernal/sandbox/analyzer_test.gointernal/sandbox/risk.gointernal/sandbox/risk_hardening_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/sandbox/risk_hardening_test.go
- internal/sandbox/analyzer.go
f5ad5de
|
Final follow-up on current head
The PR is mergeable. PierrunoYT requests a fresh review to supersede the previous |
…ush-network Amp-Thread-ID: https://ampcode.com/threads/T-019fa55a-70fb-71ad-816d-7bf849ceb6c4 # Conflicts: # internal/agent/loop_test.go
Amp-Thread-ID: https://ampcode.com/threads/T-019fa55a-70fb-71ad-816d-7bf849ceb6c4 Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
Current upstream Fresh review-history and thread audit:
Validation on Go 1.26.5:
All current GitHub checks pass, including Ubuntu/macOS/Windows Smoke, Security & code health, Performance Smoke, Zero Review, and CodeRabbit status: CI run, review run. The only remaining gate is the stale |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Do not treat
-c/-Coption values as git subcommands in the unparseable fallback
internal/sandbox/risk.go:54
The unparseable git branch can match-cor-Cthrough the generic dash-option alternative without consuming the following token as the option value. When that next token is a network verb (push,fetch, etc.), the regex treats it as the subcommand even though it is only a config key or work-tree path. For example,git -c push origin main & rem 'andgit -C push origin main & rem 'classify as criticalnetworkon the fallback path, and quoted forms such asgit -c "push" origin main & rem 'andgit -C "push" status & rem 'do the same. Basemaindid not classify these as network. This is the same failure mode you fixed for bare tokens likestatusingit status push, but it still applies when the misleading token is a-c/-Cvalue. Please ensure value-taking globals always consume their value token before subcommand matching (or exclude-c/-Cfrom the generic dash-option arm), and add regressions besideTestClassifyUnparseableNonGitOptionTokenStaysNonNetwork. -
[P2] Do not match
.gitURL/path fragments as agitexecutable in the unparseable fallback
internal/sandbox/risk.go:54
The new\bgit(?:\.(?:exe|cmd|bat|com))?program matcher also matches thegitsuffix inside.gitURLs and hostnames. On the unparseable fallback path, strings such asbar.git push origin & rem 'andhttps://github.com/foo/bar.git push origin && "unterminatedclassify as criticalnetworkeven though nogitcommand is present. Basemaindid not have this behavior. Anchor the matcher to an actual git invocation (for example require whitespace or a path separator beforegit, not a.from.git) and add negative regressions for.gitURLs/pathspecs that happen to containpush/fetchtokens later in the string. -
[P3] Unparseable fallback now false-positives on
git --help <topic>/git --version <topic>forms
internal/sandbox/risk.go:54
Commands such asgit --help push & rem 'andgit --version push & rem 'now classify as criticalnetworkon the unparseable fallback path because the generic dash-option skip advances to the topic token and treats it as the subcommand. Basemaindid not classify these on the fallback path. Please extend the negative coverage to these dash-flag forms, or halt the scan when a valueless global is followed by a bare token that is not a real git subcommand. Note: parseablegit --help pushwas already classified as network on basemainvia the AST path (firstSubcommandskipping--help); that pre-existing AST behavior is outside this diff and not introduced by the regex change.
Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
PierrunoYT addressed all fallback-classification findings in
Validation: focused risk-classification tests pass with |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/sandbox/analyzer.go (1)
281-283: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClassify
git archiveas networked only with--remote.Local
git archive HEADreads local objects and does not require network access, but both network classification paths makearchivenetwork-only.
internal/sandbox/analyzer.go#L281-L283: require--remote/--remote=…before markingarchiveas network.internal/sandbox/analyzer_test.go#L89-L90: add a localgit archive HEADregression withnetwork: false.internal/sandbox/risk.go#L100-L281: apply the same--remotecondition in fallback token parsing.🤖 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 `@internal/sandbox/analyzer.go` around lines 281 - 283, Classify git archive as networked only when --remote or --remote=… is present. Update the gitSubcommand network classification in internal/sandbox/analyzer.go and the fallback token parsing in internal/sandbox/risk.go; add a regression case in internal/sandbox/analyzer_test.go for local git archive HEAD expecting network: false.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@internal/sandbox/analyzer.go`:
- Around line 281-283: Classify git archive as networked only when --remote or
--remote=… is present. Update the gitSubcommand network classification in
internal/sandbox/analyzer.go and the fallback token parsing in
internal/sandbox/risk.go; add a regression case in
internal/sandbox/analyzer_test.go for local git archive HEAD expecting network:
false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d5e72f2c-787b-4dba-9838-0d095cca0ac3
📒 Files selected for processing (6)
internal/agent/command_prefix_test.gointernal/agent/loop_test.gointernal/sandbox/analyzer.gointernal/sandbox/analyzer_test.gointernal/sandbox/risk.gointernal/sandbox/risk_hardening_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/agent/loop_test.go
- internal/sandbox/risk_hardening_test.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Restore network classification for wrapper-prefixed unparseable commands
internal/sandbox/risk.go:71
This is a confirmed regression from8a7ffe6, not pre-existing drift. The refactor ofmatchesUnparseableNetworktokenizes each;/&/|segment and only elevates tonetworkwhentokens[0]is a git executable or when the rebuilt segment matches the new^-anchoredunparseableNetworkPattern. That drops basemain's whole-string\b(curl|wget|…)\b/\bgit\s+clone\bmatching, so obfuscated commands where the network binary is not argv[0] lose thenetworkcategory. Verified on base vs head:classifyCommand(sudo curl https://example.com && "unterminated)goes from Critical +networkto High +unparseable_commandonly; the same miss hitsenv curl …,env git fetch …,sudo git push …,sudo npm install …,timeout 5 curl …,PATH=.:$PATH git push …, andsh -c 'curl …' && "unterminated. Engine impact is also verified: withNetworkDenyandPermissionGranted: true, base returnsActionPrompt/ReasonNetworkBlocked, while head returnsActionAllowwith"tool requires approval before execution". The parseable AST fixes forgit -C repo push,git.exe, and quoted Windows forms are working; the gap is only on the fail-closed fallback when the executable is behind a wrapper or env assignment. Please keep the git-specific token-aware matcher, but also peelwrapperPrograms/ env assignments (aseffectiveProgramalready does on the parseable path) or otherwise restore fail-closed detection on this path, and add regressions besideTestClassifyUnparseableNetworkCommandFailsClosed. -
[P3] Strip Windows executable suffixes in the unparseable fallback
internal/sandbox/risk.go:118
Companion regression from the samematchesUnparseableNetworkrefactor. Parseable classification already normalizescurl.exethroughnormalizeProgramToken, butexecutableTokenBaseonly lowercases the basename and leaves.exeattached. Verified: unparseablecurl.exe https://example.com && "unterminatedlosesnetworkon head (network=false) while basemainstill matched it via\bcurl\b(network=true). Please reuse the same suffix normalization here (or callnormalizeProgramToken) and add an unparseablecurl.exe/wget.exeregression. -
[P3] Split fallback segments on newlines
internal/sandbox/risk.go:129
Companion regression from the same refactor.fallbackCommandTokensflushes commands only on;,&, and|, not on\n. Verified:true\ncurl https://example.com && "unterminatedstays one segment whose first token istrue, so the embeddedcurlis invisible to the new matcher (network=falseon head,network=trueon base). Please treat newline as a command separator (or otherwise scan later tokens in the segment) and add a regression. -
[P3] Gate local
git archiveon--remotewhen touching this verb list
internal/sandbox/analyzer.go:281
This one is partly drift, not a merge blocker for the coregit pushfix. Treating everyarchivesubcommand as network is already true on basemainforgit archive HEAD; this PR'sgitSubcommandfix extends that pre-existing false positive togit -C repo archive HEAD, which base misclassified as offline because it readrepoas the subcommand.git archiveonly needs network with--remote. If you are already editing the verb list in this PR, please gate both the AST and fallback matchers on a remote flag and addgit archive HEAD/git -C repo archive HEADnegative cases withnetwork: false. If not, this can be a follow-up; it should not block the wrapper-regression fix above.
Reworking the fallback to be token-aware made it read the program from tokens[0], which is only the program when nothing precedes it. A wrapper prefix, an environment assignment, or a shell launcher put it elsewhere, so `sudo curl …`, `env git fetch …`, `timeout 5 curl …`, `PATH=.:$PATH git push …`, and `sh -c 'curl …'` all lost the network category that the previous whole-string match caught. Under NetworkDeny with permission already granted that turned a network prompt into a plain allow — egress granted to a command too obfuscated to parse. The fallback now resolves each segment's program exactly as the parseable path does, through commandBodyFields (extracted from commandBody), and recurses into a shell launcher's -c payload the way analyzeInto does. Resolving the program rather than matching a network name anywhere in the string is what still keeps `git status push` and `echo https://example.com/repo.git push` out. Two companion gaps from the same refactor: executableTokenBase now strips Windows executable suffixes through the same helper normalizeProgramToken uses, so `curl.exe` matches here as it does there, and the fallback tokenizer treats a newline as a command separator rather than as whitespace, so a program on a later line is resolved instead of being read as arguments of the first. Separately, `git archive` is now gated on --remote on both paths. A local archive streams the object store and never leaves the machine; treating every archive as network cost a proactive prompt on an offline command, and this PR's gitSubcommand fix had extended that false positive to `git -C repo archive`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Addressed all four findings in [P2] Wrapper-prefixed unparseable commands are fail-closed again. You called it: reading the program from Resolving the program instead of reverting to whole-string matching is what still keeps [P3] [P3] Newlines. [P3] Regressions (each verified to fail with the source changes stashed and the tests kept):
Validation
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/sandbox/analyzer.go`:
- Around line 300-303: Update the argument scan in the analyzer logic to stop
processing words once the standalone `--` separator is encountered, so
subsequent pathspecs such as `--remote` are not classified as network options.
Preserve detection of `--remote` and `--remote=...` before the separator, and
add a regression case covering `git archive HEAD -- --remote`.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c0ce84d3-2a63-4901-b4ef-1978ea27905e
📒 Files selected for processing (6)
internal/sandbox/analyzer.gointernal/sandbox/analyzer_test.gointernal/sandbox/engine_test.gointernal/sandbox/risk.gointernal/sandbox/risk_hardening_test.gointernal/sandbox/safe_command.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/sandbox/risk.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Respect
--when scanninggit archivefor--remote
internal/sandbox/analyzer.go:299
gitTargetsRemoteArchivewalks every argument token and treats any--remote/--remote=…as a network archive, with no stop at the standalone--end-of-options marker. After--, operands are pathspecs, not options, sogit archive HEAD -- --remote(and the same shape with-C/-o) is a local archive of a tree entry named--remote, not a remote fetch. Both the parseable AST path (gitUsesNetwork) and the unparseable fallback (matchesUnparseableGitNetwork) call this helper, so the false positive hits proactive network prompts and turn network grants for a purely local command. This gap was introduced when this PR gatedarchiveon--remote; it is not pre-existing drift. The same package already handles--correctly forrminhasRecursiveForce; mirror that pattern here and add a regression forgit archive HEAD -- --remoteexpectingnetwork: false. CodeRabbit'sCHANGES_REQUESTEDreview on head6225826is still open on this point. -
[P3] Align unparseable shell recursion depth with the AST path
internal/sandbox/risk.go:74
maxUnparseableShellDepthis3whilemaxAnalyzerDepthinanalyzer.gois4, despite the comment that the fallback "mirrors" the parseable limit. On a fourth nestedsh -c/bash -clayer that is also unparseable, the AST path can still reach an innercurl/git push, butmatchesUnparseableNetworkAtstops recursing one level earlier and drops thenetworkcategory. UnderNetworkDenywith shell permission already granted, that is the same fail-open shape the wrapper-prefix regression fixed in6225826, but it requires heavy obfuscation and is unlikely in normal use. This mismatch is new in this PR's fallback code, not drift from basemain. Either bump the fallback cap to4or document and test the intentional difference.
Follow-up (optional for #703; pre-existing or edge-case parity)
These are real classification gaps, but they are mostly outside the core git push fix or require obscure spellings. I would not block merge on them alone once the git archive -- issue above is fixed.
-
[P3] Recurse into
--commandpayloads on launcher paths
internal/sandbox/risk.go:152andinternal/sandbox/analyzer.go:488
fallbackDashCPayloadanddashCPayloadonly recognize-c, whileshellDashCPayloadinsafe_command.goalready accepts both-cand--command. An unparseablebash --command "git push origin main" && "unterminatednever recurses into the payload, so classification can stay atunparseable_commandwithoutnetworkafter a prior shell approval. The-c-only limitation on the parseable AST path predates this PR; this branch copied it into the new fallback instead of sharingshellDashCPayload. Worth fixing if you touch launcher recursion, but it is parity drift rather than a new regression from the6225826wrapper fix. -
[P3] Normalize drive-relative Windows executables on the fallback path
internal/sandbox/risk.go:166
executableTokenBasestrips basename and suffixes but does not callwindowsExecutablePathBasename, so drive-relative spellings such asC:git.exenormalize toc:gitinstead ofgit. The parseable path already classifies'C:curl.exe' https://example.comas network vianormalizeProgramToken. An unparseable Windows form likeC:git.exe push origin main & rem 'can miss thenetworkcategory on the fail-closed path while the AST path would catch it. This is an obscure Windows spelling gap on the fallback path introduced when this PR addedexecutableTokenBaseparity claims; the commongit.exe/ quoted-path forms you already test are fine.
Rechecked — prior review items look addressed on current head
The wrapper-prefix, curl.exe, newline-segment, and local-git archive regressions from the 8a7ffe6 review appear fixed in 6225826, with matching coverage in TestClassifyUnparseableNetworkBehindWrapperFailsClosed, TestEvaluatePromptsForUnparseableNetworkBehindWrapper, and TestClassifyUnparseableLocalGitArchiveStaysNonNetwork. Parseable -C / -c / --git-dir / git.exe / --attr-source forms and the git status push negatives also still look covered. Windows Smoke failed on internal/tools TestExecCommandForegroundServerReturnsSessionAndServesHTTP; this PR does not touch internal/tools, and internal/sandbox and internal/agent passed on that CI run.
Gating `git archive` on --remote scanned every token, with no stop at the standalone `--`. After that separator the operands are pathspecs, so `git archive HEAD -- --remote` archives a tree entry NAMED --remote out of the local object store — and was classified as network, costing a proactive prompt and a network grant on a purely local command. Both the AST path and the unparseable fallback call the same helper, so both were wrong; the end-of-options rule is the one hasRecursiveForce already applies to `rm -- -rf`. Three parity gaps in the same code, each one a claim my own comments made: The fallback's launcher recursion cap said it mirrored the parseable path while being one level shallower, so a fourth nested layer that the analyzer still reaches would drop the network category. It now IS maxAnalyzerDepth rather than a copy of its value. Both payload scanners knew only `-c`, though bash and zsh accept `--command` and shellDashCPayload already handled both — so `bash --command 'git push …'` was never recursed into on either path. executableTokenBase claimed parity with normalizeProgramToken but skipped windowsExecutablePathBasename, leaving a drive-relative `C:git.exe` as "c:git" and unmatched on the fail-closed path while the AST path classified it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All four addressed in [P2] [P3] Recursion depth. Bumped, but by making it On testing it: I could not honestly drive a four-layer [P3, optional] [P3, optional] Drive-relative Windows executables. Taken. Tests: 11 new cases across Validation: Agreed the Windows Smoke failure is unrelated — |
Summary
git pushas network-sensitive before sandbox executiongitlawb://in analyzer and risk-classifier testsFixes #703.
Validation
go test ./...go vet ./...GOTOOLCHAIN=go1.26.5 go run golang.org/x/vuln/cmd/govulncheck@v1.3.0 ./...The repository-wide lint command still reports 35 pre-existing findings unrelated to this change.
Summary by CodeRabbit
-C,--git-dir), includinggit.exe/git.cmd.git archiveas network only when a--remoteis targeted, while keeping local archive non-network.sh -cpayload parsing) and aligned Windows command token normalization for executable suffixes.