fix(sandbox): classify git push as network access

[PierrunoYT]

Summary

• classify git push and remote git archive invocations as network-sensitive with shared, positional Git option parsing
• fail closed for reviewed shell function, CMD, PowerShell, env -S, shell-launcher, BusyBox, and strace forms while avoiding false prompts for non-executing text and local operands
• exercise the existing turn-scoped network grant path with an integration regression proving an approved git push receives the temporary network overlay, including the gitlawb:// transport

Fixes #703.

Validation

• make fmt-check
• go vet ./...
• go test ./...
• go test -race ./internal/sandbox ./internal/agent
• go test ./internal/agent -run '^TestRunApprovedGitPushPromptAppliesTurnNetworkGrant$' -count=1
• go run ./cmd/zero-release build
• go run ./cmd/zero-release smoke
• make lint-static (0 issues)
• make vulncheck (No vulnerabilities found.)
• git diff HEAD --check

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Git 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 approved git push execution.

Changes

Sandbox network classification

Layer / File(s)	Summary
Git network detection and analyzer coverage internal/agent/command_prefix.go, internal/agent/command_prefix_test.go, internal/sandbox/analyzer.go, internal/sandbox/analyzer_test.go, internal/sandbox/safe_command.go	Git option parsing, subcommand detection, remote archive handling, and executable normalization cover inline and separate values, local commands, and Windows executable variants.
Risk classification for parsed and unparseable Git commands internal/sandbox/risk.go, internal/sandbox/risk_hardening_test.go, internal/sandbox/engine_test.go	Fallback tokenization and Git-specific matching classify network activity through wrappers and shell payloads while preserving non-network results for local commands and option-like tokens.
Approved git push execution coverage internal/agent/loop_test.go	The regression test verifies that an approved git push receives the network grant and executes with the expected fake Git output.

Estimated code review effort: 4 (Complex) | ~45 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

Loading

Suggested reviewers: gnanam1990, anandh8x

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.61% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes address issue #703 by detecting git push network access and applying the approved turn-level network grant.
Out of Scope Changes check	✅ Passed	The production and test changes support Git network detection, sandbox approval behavior, and related parsing hardening for issue #703.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the primary change: classifying git push as network access in the sandbox.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

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 commandUsesNetwork to classify git push as network access.
• Add analyzer coverage for git push (and a non-network git commit) in AnalyzeCommand tests.
• Add a risk-classifier hardening test asserting git push is 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.

[coderabbitai]

🧹 Nitpick comments (1)

internal/sandbox/risk_hardening_test.go (1)

297-309: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer t.Errorf over t.Fatalf in loops.

Using t.Fatalf inside a loop will immediately abort the test on the first failure, which prevents the remaining test cases from executing. Replacing it with t.Errorf allows 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

📥 Commits

Reviewing files that changed from the base of the PR and between 7a22945 and d72a192.

📒 Files selected for processing (5)

• internal/agent/loop_test.go
• internal/sandbox/analyzer.go
• internal/sandbox/analyzer_test.go
• internal/sandbox/risk.go
• internal/sandbox/risk_hardening_test.go

[Vasanthdev2004]

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]

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]

I found issues that need to be addressed before this is ready.

Findings

• Resolve the merge conflict with main
  GitHub currently reports this PR as CONFLICTING / 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 generic firstSubcommand skips -C/-c/--git-dir themselves but treats their following values as the subcommand. Consequently ordinary, parseable commands such as git -C repo push origin main, git -c http.proxy=x fetch origin, and git --git-dir /repo/.git pull produce no network risk. 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 (as internal/agent/command_prefix.go already does) and add parseable regression coverage.

• [P2] Recognize the Windows git.exe command spelling
  internal/sandbox/analyzer.go:152
effectiveProgram normalizes git.exe to git.exe, not git, so git.exe push origin main never 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 handle git.exe) and cover that spelling in the analyzer and risk tests.

[PierrunoYT]

Merged current main (595c292) — the PR is MERGEABLE again — and closed the AST gap everyone converged on.

Resolve the merge conflict with main — done. main meanwhile grew gitUsesNetwork, already covering clone|fetch|pull|push|ls-remote|archive, so the conflict resolves to main's version and this branch now carries only what is still missing on top of it.

[P2] Classify Git commands after consuming global-option values (@jatmn, @gnanam1990, @Vasanthdev2004 — all three findings share this root cause) — fixed with a git-aware gitSubcommand that skips the values of git's space-separated globals (-C, -c, --config-env, --exec-path, --git-dir, --namespace, --super-prefix, --work-tree), mirroring gitOptionConsumesValue in internal/agent/command_prefix.go as suggested. Joined forms (--git-dir=/x, -C/x) were already fine as single dash-prefixed tokens. Verified against the exact matrix from the reviews:

command	before	after
git -C repo push origin main	Network=false, High	Network=true, Critical + network
git -c http.sslVerify=false push	Network=false, High	Network=true, Critical + network
git --git-dir /repo/.git fetch origin	Network=false, High	Network=true, Critical + network
git --work-tree /repo pull origin main	Network=false, High	Network=true, Critical + network
git -C repo commit -m x	no network	no network (unchanged)

[P2] Recognize the Windows git.exe spelling — effectiveProgram now trims a trailing .exe after normalizeProgramToken, so git.exe push classifies like git push; the same fix covers curl.exe, wget.exe, and the other program tables. Deliberately .exe only — a .bat/.cmd of the same stem is a separate script, not the program it is named after, so treating it as that program would be a guess. The command-prefix tables that list powershell.exe explicitly are a different code path and are untouched.

Parseable regression coverage — the key point from @Vasanthdev2004's review was that the only -C test forced the unparseable path and masked the AST gap. New TestClassifyParseableGitNetworkCommandsUseASTPath asserts AnalyzeCommand(...).TooComplex == false for each case before checking Critical + network, so the hardened fallback regex cannot mask a future AST regression, and it also pins the local-work cases (git -C repo commit, git -C repo status, git.exe commit) as non-network. The analyzer table gained the same forms.

@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. TestRunApprovedGitPushPromptAppliesTurnNetworkGrant (this branch's rewrite of the existing curl test) exercises it end to end — prompt → approve → turn network grant applied → git push gitlawb://… actually runs — and it passes.

@copilot: use a gitlawb:// URL rather than a bare token — applied earlier in d72a192; the analyzer, risk, and end-to-end tests all use gitlawb://example.com/repo.git. @coderabbitai: t.Errorf over t.Fatalf in loops — applied; the unparseable table runs as subtests with t.Errorf, and the new test does the same.

Validation (Windows host, Go 1.26.5): go build ./..., go vet ./..., go test ./... -count=1 all pass; GOOS=linux|darwin|windows builds clean; gofmt clean on every changed file.

@jatmn @Vasanthdev2004 @gnanam1990 — ready for another look; I can't use the reviewer-request button on this repo, hence the mention.

@coderabbitai review

[coderabbitai]

@PierrunoYT I’ll review the updated PR, with particular attention to option parsing, Windows executable normalization, and ensuring the parseable AST path—not only the fail-closed fallback—enforces the network classification.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

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 win

Cover git.exe in the unparseable network fallback.

unparseableNetworkPattern matches git only, so an unparseable git.exe push ... misses the critical network category even though parseable git.exe is classified correctly. Accept an optional .exe suffix 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.go requires 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

📥 Commits

Reviewing files that changed from the base of the PR and between d72a192 and 595c292.

📒 Files selected for processing (5)

• internal/agent/loop_test.go
• internal/sandbox/analyzer.go
• internal/sandbox/analyzer_test.go
• internal/sandbox/risk.go
• internal/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]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Recognize --attr-source as 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. gitSubcommand therefore returns head instead of push; because this command parses successfully, the fallback is not consulted and the shell call never receives the critical network classification 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 normalizes git.exe, but parser-failing Windows commands never reach that code. For example, git.exe push origin main & rem ' runs under cmd.exe but is rejected by the POSIX parser; this pattern requires whitespace immediately after git, so classification adds only unparseable_command, not network, 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 .exe suffix and scan Git tokens up to a command separator without the arbitrary cap, with regressions for both forms.

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between 595c292 and d54e971.

📒 Files selected for processing (5)

• internal/agent/command_prefix.go
• internal/sandbox/analyzer.go
• internal/sandbox/analyzer_test.go
• internal/sandbox/risk.go
• internal/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

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P2] Do not treat arbitrary Git arguments as global options in the fallback
  internal/sandbox/risk.go:44
  The unbounded (?:\s+[^\s;&|]+)* portion consumes any token before a network verb. For example, git status push & rem ' is valid cmd.exe input but fails the POSIX parser; push is only a git status pathspec, yet the regex marks the command as critical network. That produces a network prompt and, on approval, a network-enabled turn profile for a local command. Restrict this span to the supported Git global options and their values, with a negative regression test.

[PierrunoYT]

Pushed 708c2c2 to address the remaining outstanding finding (from @coderabbitai and @jatmn): the unparseable-command regex fallback in internal/sandbox/risk.go skipped any run of non-separator tokens before the git subcommand verb, so a bare non-option token (e.g. status in git status push, where push is only a pathspec argument) was mistaken for a git global option and the command was misclassified as critical network.

Fix: the skipped span before the subcommand verb is now restricted to git's actual global options, mirroring gitGlobalOptionConsumesValue in analyzer.go — a recognized value-taking option (-C, -c, --attr-source, --config-env, --exec-path, --git-dir, --namespace, --super-prefix, --work-tree) consumes one following token as its value, and any other dash-prefixed token (including a joined --git-dir=/x form) is skipped on its own. A bare non-option token now correctly halts the scan, so git status push no longer trips the network category. The span stays unbounded (no arbitrary cap), so the existing multi-option regression (git -c a=1 -c b=2 ... push) still passes.

Added TestClassifyUnparseableNonGitOptionTokenStaysNonNetwork covering git status push & rem ' (forced onto the fallback path via the unmatched trailing quote) — asserts it stays unparseable_command/High with no network category.

Validation:

• go test ./internal/sandbox/...
• go vet ./internal/sandbox/... ./internal/agent/...

All previously reported findings (the -C/-c/--git-dir AST gap, git.exe normalization, --attr-source, and the merge conflict) were already resolved in earlier commits on this branch — this addresses the last open item.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- changed files in 708c2c2 ---'
git show --stat --oneline --find-renames 708c2c2

printf '%s\n' '--- Go declarations in affected files ---'
ast-grep outline internal/sandbox/risk.go --items all --type function --match 'Classify|Network|Git'
ast-grep outline internal/sandbox/analyzer.go --items all --type function --match 'git|Git|commandUsesNetwork'
ast-grep outline internal/sandbox/risk_hardening_test.go --items all --type function --match 'TestClassifyUnparseable'

printf '%s\n' '--- exact patch ---'
git show --format=fuller --find-renames --unified=80 708c2c2 -- \
  internal/sandbox/risk.go \
  internal/sandbox/risk_hardening_test.go \
  internal/sandbox/analyzer.go

printf '%s\n' '--- related test cases and option helpers ---'
rg -n -C 12 \
  'TestClassifyUnparseableNonGitOptionTokenStaysNonNetwork|TestClassifyUnparseableNetworkCommandFailsClosed|unparseableNetworkPattern|gitGlobalOptionConsumesValue|gitSubcommand' \
  internal/sandbox/risk.go \
  internal/sandbox/risk_hardening_test.go \
  internal/sandbox/analyzer.go

Length of output: 33304

---

@PierrunoYT Verified. The fallback now stops at a bare non-option token, so git status push is no longer mistaken for a network push, while recognized global-option forms—including repeated -c options and git.exe—remain covered by the fail-closed path.

The new regression test correctly asserts unparseable_command/High without a network category. This addresses the outstanding finding.

---

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

[PierrunoYT]

Rebased this branch onto current upstream/main (cabfeef) and force-updated it with 10cbacd.

Addressed the outstanding review blockers:

• shared Git global-option parsing across agent/sandbox paths and corrected git archive's positional/two-pass option handling, including remote positives and local output/pathspec negatives
• restored fail-closed network classification for function/subshell bodies, CMD (CALL/IF/FOR /F/START/caret and nested launcher forms), PowerShell/pwsh, GNU env -S, shell -c clusters, BusyBox, and strace
• added correct handling for modern pwsh -CommandWithArgs, GNU env --argv0, and strace clustered/optional arguments without prompting for inert or invalid forms
• bounded attacker-controlled recursion and removed quadratic rescans in nested CMD and repeated FOR /F input
• retained and specialized the integration regression proving an approved git push receives the existing turn-scoped network overlay, including gitlawb://
• preserved rebased upstream Windows/security behavior

Validation completed:

• make fmt-check
• go vet ./...
• go test ./...
• go test -race ./internal/sandbox ./internal/agent
• go test ./internal/agent -run '^TestRunApprovedGitPushPromptAppliesTurnNetworkGrant$' -count=1
• go run ./cmd/zero-release build
• go run ./cmd/zero-release smoke
• make lint-static — 0 issues
• make vulncheck — No vulnerabilities found. (the prior vuln.go.dev 403 did not reproduce locally)
• git diff HEAD --check

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Fail closed for dynamic env -S source
  internal/sandbox/analyzer.go:168
  The new split-string path is attempted only when every shell word is literal. Consequently PAYLOAD='curl https://...'; env -S "$PAYLOAD" falls through to ordinary wrapper handling: effectiveProgram consumes -S and its operand, then reports no executable. GNU env -S receives the shell-expanded value and executes its argv, so a network command reaches Engine.Evaluate without the network category or the turn-scoped network grant. The fallback has the same failure mode: an opaque split string is returned as recognized-but-clean.

  The root cause is using literal reconstruction as both an optimization and a proof of safety. Keep an explicit opaque/environment-dependent result for the split-string operand and propagate it through both AST and fallback paths. If the resulting executable or any command-defining token cannot be resolved statically, classify it as network-sensitive; add engine-level coverage for a shell-expanded -S operand and for a child environment expansion such as CMD=curl env -S '${CMD} https://...'.

• [P1] Treat opaque PowerShell command source as network-sensitive
  internal/sandbox/analyzer.go:201
fallbackPowerShellPayload marks -EncodedCommand as opaque, and this branch deliberately ignores opaque payloads. A valid UTF-16LE/Base64 payload for Invoke-WebRequest https://... therefore parses successfully but gets no network risk or approval prompt. The same loss occurs for powershell -Command "$PAYLOAD": wordTexts drops the expansion and the payload becomes empty. Neither case reaches the unparseable fallback because the outer POSIX command parses successfully. This defeats the Windows classifier-based network boundary.

  Do not use “cannot inspect” as “does not use network” for an interpreter argument that is executable source. Model PowerShell payload extraction as (payload, opaque), inspect bounded valid encoded input if practical, and otherwise add network when opaque is true or the selected Command/CommandWithArgs operand is nonliteral. Cover valid encoded network source and shell-expanded Command source through Engine.Evaluate, not only classifier helpers.

• [P1] Continue parsing START switches after its title
  internal/sandbox/risk.go:383
  CMD permits switches after the optional title, e.g. start "" /b curl https://.... cmdStartPayloadTokenInfo first calls cmdStartOperandsTokenInfo, which stops at the quoted title; it then drops that title and returns /b curl .... The CMD resolver treats /b as the executable, while the POSIX resolver sees only start, so neither contributes a network classification. Adding & rem ' makes this valid CMD form use the intended unparseable fallback and it receives no prompt under NetworkDeny.

  Parse START's grammar as one invocation rather than as a one-pass prefix strip: consume its supported switches, consume an optional quoted title, then consume supported switches again before selecting the command. Preserve value-taking switch handling (/d, /node, /affinity) in both passes and add engine-level regressions for title-before-/b, title-before-/wait, and title-before-/d <path>.

• [P2] Do not consume an argument after bare --exec-path
internal/sandbox/analyzer.go:465
  Git's bare --exec-path is terminal (--exec-path[=<path>]): git --exec-path /tmp push prints the local exec path and exits; it neither treats /tmp as an option value nor invokes push. The shared helper instead says bare --exec-path consumes the following token, so parseGitInvocation skips /tmp, reaches push, and marks the local informational command as network-sensitive. This is a regression from the previous first-subcommand scan and causes an unnecessary prompt and temporary network grant.

  Separate Git globals into terminal options, exact separated-value options, and inline-value-only options. Put bare --exec-path in the terminal class and retain --exec-path=<path> as a nonterminal inline form. Exercise both AST and fallback classification with git --exec-path, git --exec-path /tmp push, and git --exec-path=/tmp push so the shared parser stays aligned with Git's actual option grammar.

[PierrunoYT]

Addressed all four findings from the review on 10cbacd2 and squashed the branch to a single commit (ba5771d).

Findings

[P1] Fail closed for dynamic env -S source

An unresolvable split string was returned as recognized-but-clean. splitEnvString failing now marks the result environment-dependent, and a new envSplitSourceDynamic covers the AST path, where effectiveProgram previously consumed -S with its operand and reported no executable at all. Separated (env -S "$PAYLOAD"), joined (env -S"$PAYLOAD"), and --split-string="$PAYLOAD" forms all classify as network-sensitive; the child-expansion case (CMD=curl env -S '${CMD} https://...') stays covered by the existing dependent-token path.

[P1] Treat opaque PowerShell command source as network-sensitive

fallbackPowerShellPayload conflated "source exists but cannot be read" with "no source at all". It now returns powerShellPayload{payload, opaque, sourceIndex}, so the three outcomes stay distinct:

• -EncodedCommand is base64/UTF-16LE decoded within a 64 KiB bound and analyzed when it decodes; anything that does not decode is opaque and fails closed.
• sourceIndex lets the AST path detect a Command operand built from an expansion (powershell -Command "$PAYLOAD"), which wordText otherwise drops to an empty payload.
• -File, -Version, colon-joined values, and invalid switches remain inert, exactly as before.

[P1] Continue parsing START switches after its title

cmdStartPayloadTokenInfo runs the switch scan again after dropping the quoted title, preserving value-taking /d, /node, and /affinity handling in both passes. This surfaced an adjacent issue in the same form: the shared tokenizer reads \ as a POSIX escape, so start "" /d C:\ curl ... arrived as the single token C:\ curl and the switch consumed the executable. normalizeCMDCommandTokens now splits those back apart for the CMD resolver only.

[P2] Do not consume an argument after bare --exec-path

Moved to gitTerminalGlobalOptions and removed from GitGlobalOptionConsumesValue, matching git's documented --exec-path[=<path>] grammar: bare, it prints the local exec path and exits, so git --exec-path /tmp push neither reads /tmp as a value nor reaches push. The inline --exec-path=<path> form stays nonterminal. Both spellings are pinned in the shared table that resolves the AST and fallback paths together, so internal/agent's prefix parser cannot drift from it.

Two test expectations changed

powershell -EncodedCommand curl & rem ' moved from the non-network lists to the network lists in engine_test.go and risk_hardening_test.go. curl is not decodable as base64 UTF-16LE, so under the new rule it is opaque and fails closed. powershell -e ZQB2AGkAbAA= stays non-network because it now genuinely decodes, to the local program name evil, rather than being ignored as uninspectable.

New regressions run through Engine.Evaluate for each bucket: undecodable and valid encoded payloads, expanded Command and env -S operands, and START's title-before-/b, /wait, and /d <path>.

Validation

• make fmt-check (clean)
• go vet ./...
• go test ./...
• go run ./cmd/zero-release build
• go run ./cmd/zero-release smoke
• git diff HEAD --check

go test -race was not run on this pass: the local toolchain has no C compiler, and -race requires cgo. The changes are pure classification logic with no concurrency, and CI covers the race jobs.

Open item

The Fixes #703 scoping question from the earlier pass is unchanged: the integration regression proving an approved git push receives the turn-scoped network overlay is still in the branch. Happy to rescope the description to link #703 without closing it if that is the preference.

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Recognize abbreviated GNU env --split-string options
  internal/sandbox/safe_command.go:522
  GNU env accepts unambiguous long-option abbreviations: env --split 'curl https://evil.test' executes curl just as env --split-string … does. envSplitOption recognizes only the exact long spelling (and its = form), so envSplitCommandFields returns unrecognized for this valid invocation. The normal AST path then skips --split as an ordinary wrapper option and treats the whole quoted payload (curl https://evil.test) as one executable name; it never matches curl, adds no network category, and the engine therefore does not apply the NetworkDeny prompt.

  The root problem is modelling only selected spellings instead of the runtime option grammar. Centralize recognition of the supported GNU env -S aliases—including accepted unambiguous long prefixes and joined values—or conservatively mark an unresolved split-string form as network-sensitive. Please add parsed-command engine regressions for --split <payload> and --split=<payload>, plus negative cases that prove non-split options and literal payload text do not receive a network grant.

• [P1] Fail closed when BusyBox or strace receives a dynamic child executable
  internal/sandbox/analyzer.go:292
  The new wrapper handling calls wordTexts(args) before passing arguments to busyboxCommandArgs and straceCommandArgs; wordText deliberately removes shell expansions. After APPLET=curl; busybox "$APPLET" https://evil.test, the second command parses successfully, but the wrapper resolver receives an empty child-program token and returns no network use. The command therefore never reaches the unparseable fallback, despite BusyBox executing curl. strace "$APPLET" https://evil.test has the same failure mode. An approved shell command can consequently egress without the network prompt or temporary network overlay.

  The root cause is losing the distinction between a literal child argv and an unknown one before interpreting a launcher. Preserve literalness alongside token text for delegated BusyBox/strace argv, and fail closed when the executable position or option parsing depends on an expansion rather than treating it as a clean unknown program. Please add engine-level regressions for dynamic BusyBox and strace child executables, together with literal non-network controls, so both the AST and fallback paths retain the same network-consent contract.

[PierrunoYT]

Pushed 58f6fcdd, addressing the four items still open after the last review round.

Fixed

• [P1] Recognize abbreviated GNU env --split-string options (safe_command.go:522, was envSplitOption)
  Added matchesEnvSplitLongOption, which matches any unambiguous prefix of split-string the way GNU getopt_long does — --split, --sp, --split-string=..., etc. — instead of only the exact spelling. Verified env --split 'curl https://…' and env --split='curl https://…' now classify as network on both the AST path and the unparseable fallback.

• [P1] Fail closed when BusyBox or strace receives a dynamic child executable (analyzer.go:230-238)
  Added busyboxSourceDynamic/straceSourceDynamic, mirroring the existing powerShellSourceDynamic/envSplitSourceDynamic idiom: check isLiteralWord at the position the child program resolves to, rather than trusting wordTexts's silently-expansion-dropping reconstruction. straceCommandArgs was refactored to share its option-walk (straceChildIndex) with the new check instead of duplicating strace's grammar a second time. Verified APPLET=curl; busybox "$APPLET" https://… and the strace equivalent now classify as network.

Also found and fixed

• Same BusyBox/strace gap on the unparseable-command fallback path (risk.go, fallbackBodyUsesNetwork)
  Not explicitly named in the last review, but the same class of bug reaches the fallback tokenizer too: it never expands $APPLET, so the literal token $APPLET was matched against known program names and read as "unknown, not network." Added fallbackTokenLooksDynamic (fails closed on any $/` in the resolved child-program token) so the AST and fallback paths give the same answer, per the ask in the prior review to keep both aligned.

Validation

• go build ./..., go vet ./..., gofmt -l on all changed files — clean
• go test ./... (whole module) — pass
• go test ./internal/agent -run '^TestRunApprovedGitPushPromptAppliesTurnNetworkGrant$' — pass
• git diff HEAD --check — clean
• Added regression cases to analyzer_test.go (AST path) and risk_hardening_test.go (fallback path), including negative controls for non-split env long options and literal (non-network) BusyBox/strace children, matching the style of the existing tests in each file.

Re-verified everything from earlier rounds (function-definition regression, env -S dynamic source, PowerShell opaque/encoded payload, git --exec-path, CMD START switches-after-title) is still holding — no regressions there.

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Classify CMD launcher payloads on the normal parseable path
  internal/sandbox/analyzer.go:191
  This change adds CMD-specific network classification, but the CMD resolver is only reached from matchesUnparseableNetwork, which classifyWithScope calls only after the POSIX parser reports TooComplex. That leaves the normal AST path with no equivalent branch for cmd, call, or start.

  On Windows, Zero passes bash-tool command text verbatim to cmd.exe /d /c. An ordinary cmd /c git push origin main (and likewise cmd /c curl ...) is valid POSIX syntax, so AnalyzeCommand successfully produces a call whose executable is only cmd; it neither inspects /c's payload nor sets AnalysisResult.Network. Since the fallback is skipped, the engine can accept the command under NetworkDeny or an already-approved shell permission without the network prompt this PR is intended to provide.

  Please make command-text interpreter handling a shared classification step rather than a fallback-only feature: extract the CMD payload from the AST arguments, recurse into it with the same bounded/fail-closed rules used for shell and PowerShell source, and reuse that extraction in the fallback. Add engine-level regressions for normal parseable cmd /c git push, cmd /c curl, call git push, and start forms, not only examples made unparseable with rem '.

• [P1] Fail closed for CMD-expanded command names in fallback resolution
  internal/sandbox/risk.go:154
  The new fallback resolver resolves the command name literally, while CMD expands it before executing it. For example, set "N=curl" & call %N% https://evil.test & rem ' reaches the fallback because of the CMD rem tail; CMD expands %N% to curl, but fallbackBodyUsesNetwork sees %N% as an unknown executable and emits no network category. The same bypass works with N=git followed by %N% push ...; delayed expansion (!N!) creates the equivalent problem when enabled.

  The root cause is that dynamic-token detection is both incomplete (fallbackTokenLooksDynamic only recognizes $ and backticks) and applied only after BusyBox/strace have selected a child command. CMD resolution can produce a dynamic executable directly through call, cmd /c, or start, so it reaches fallbackBodyUsesNetwork before that special-case check.

  Please centralize executable-token uncertainty handling at the point fallbackBodyUsesNetwork selects its program. Recognize CMD %VAR%, %VAR:...%, and delayed !VAR! forms as unresolved executable text, and conservatively classify them as network-sensitive before testing program names. Apply the same rule after CMD's call, /c, and start payload extraction. Add positive tests for each launcher plus negative tests proving literal percent/bang characters that cannot be expansions are not unnecessarily gated.

• [P1] Keep the fallback aligned with the AST check for dynamic sh -c payloads
  internal/sandbox/risk.go:212
  The new fallback recursion is missing the AST path's existing unresolved-payload check. For sh -c "$PAYLOAD" & rem ', CMD executes the first segment while the unmatched quote routes classification through the fallback. sh expands PAYLOAD to a command such as curl ... or git push ..., but the fallback calls matchesUnparseableNetworkAt on the literal $PAYLOAD, which has no recognized network program, and returns no network risk.

  This is a parity bug introduced by adding the new fallback shell-payload recursion without carrying over analyzeInto's !isLiteralWord(...) => Network rule. The fallback cannot evaluate shell expansions, so recursively treating their spelling as shell source is not a safe substitute for recognizing that the executed command is unknown.

  Please use one common “command source is statically inspectable” check for AST and fallback interpreter payloads. When a selected -c payload contains an unresolved variable, command substitution, or equivalent dynamic source, classify it as network-sensitive before recursion; only recursively parse known literal payloads. Include fallback regressions for $VAR, ${VAR}, command substitution, and a local literal sh -c 'printf ok' control.

• [P1] Preserve or fail closed on env -S variables used by a command-bearing argument
  internal/sandbox/safe_command.go:686
splitEnvString expands ${NAME} using Zero's own process environment and only marks the invocation unsafe when the final executable token is dependent. It does not receive shell assignment prefixes (CallExpr.Assigns) and therefore cannot model the environment that GNU env actually receives. It also drops dependency state once the resolved executable is known, even when a later argument becomes interpreter source.

  For example, PAYLOAD='curl https://evil.test' env -S 'sh -c "${PAYLOAD}"' exports PAYLOAD to GNU env; GNU env -S expands it, then invokes sh -c 'curl https://evil.test'. The classifier instead reads Zero's ambient PAYLOAD (typically empty), resolves sh -c "", and skips the network prompt. The same envSplitCommand path is used by the newly expanded fallback handling.

  Please avoid evaluating target-command variables from the reviewer/agent process environment. Carry the command's assignment environment and an explicit taint/unknown bit through split-string parsing, including nested env -S rewrites. Any dynamic or assignment-derived field that controls an executable or interpreter payload should fail closed; a dependency in an ordinary inert argument can remain precise. Add AST and unparseable-fallback regressions for prefix assignments feeding direct executables and sh -c payloads, plus controls for known local literals.

[PierrunoYT]

Addressed jatmn's latest review in f6435a66 and pushed it to this PR branch.

• CMD launchers now use the same bounded command-body resolver on the normal AST path, covering parseable cmd /c, call, and start invocations instead of relying on the unparseable fallback.
• Fallback executable uncertainty is centralized: CMD %VAR%, %VAR:...%, and delayed !VAR! command names fail closed after direct, call, cmd /c, and start resolution. Literal unmatched percent/bang text remains non-network.
• Dynamic sh -c payloads now fail closed on the fallback path; literal local payloads remain local.
• GNU env -S no longer resolves ${NAME} from Zero's ambient process environment. Dependency/taint is retained through argv reconstruction and fails closed when it controls the executable or interpreter source, including nested sh -c; inert dependent arguments remain precise.

Regression coverage includes AST and fallback parity plus Engine.Evaluate assertions for ReasonNetworkBlocked on the reviewed command forms.

Validation:

• focused requested regressions: passed
• go test ./...: passed (84 packages, 5 without tests)
• go vet ./...: passed
• release build and smoke: passed
• govulncheck: no vulnerabilities
• git diff HEAD --check: passed before commit
• changed files are gofmt-clean

Environment limitations: make is unavailable on this Windows workstation. Advisory static lint still reports only the unrelated pre-existing ST1005 finding at internal/peermsg/private_dir_windows.go:107. Race validation could not run because this environment has CGO disabled (-race requires cgo).

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Rebase this branch onto the declared base before merging
  internal/providers/oauth.go:1
2d2450e (the current main base) is not an ancestor of f6435a6; their merge base is cabfeef. The submitted base-to-head diff therefore includes apparent reversions of #890's provider, model-discovery, agent, and TUI work across 37 unrelated files, including the OAuth helper and /fast/service-tier support, even though the two branch commits only concern sandbox classification. The repository explicitly treats this stale-base state as a hard blocker; whether GitHub's three-way merge retains a particular base-side hunk is not a substitute for reviewing an up-to-date resolved diff. Rebase onto 2d2450e and preserve the base-side behavior during conflict resolution, rather than carrying these unrelated paths forward from the old merge base. Then request review of the resolved complete diff, because the classifier changes overlap a security-sensitive area and cannot be safely reviewed as two old commits in isolation.

• [P1] Classify quoted CMD command source before resolving its executable
  internal/sandbox/analyzer.go:690
  A parseable cmd /c "git push origin main" executes git push, but cmdLauncherUsesNetwork passes the entire quoted /c payload to fallbackBodyUsesNetwork as one token. fallbackBodyUsesNetwork consequently treats git push origin main as the executable name rather than parsing it as command source, so it never reaches gitUsesNetwork; the request has only the normal shell risk and is allowed under NetworkDeny (or an already-approved shell permission) without a network prompt or temporary grant. The unquoted AST test does not cover this because its arguments are already separate words.

  The root cause is using an argv-oriented CMD resolver for a field that CMD treats as interpreter source. Make command-source extraction a shared operation: once /c or /k selects a payload, reclassify the payload as bounded command text (and fail closed if it cannot be read), rather than passing a quoted payload through the executable-token path. Reuse that operation in both the normal AST and unparseable fallback paths so their handling cannot diverge. Add Engine.Evaluate regressions for quoted cmd /c "git push …", quoted remote git archive, and a non-network quoted local command.

• [P1] Keep a literal env -S payload classified when an unrelated trailing argument is dynamic
  internal/sandbox/analyzer.go:168
env -S 'git push origin main' "$EXTRA" is parseable and GNU env executes the static split-string command while appending the expanded trailing argument. Because literalCallFields requires every call argument to be literal, the unrelated trailing expansion bypasses envSplitCommandFields; envSplitSourceDynamic only considers whether the split-string operand itself is dynamic, and effectiveProgram then consumes -S and finds no program. The result has no network category, so the push bypasses the approval boundary. The same shape hides curl and other static split-string commands.

  The root cause is conflating literalness of the whole shell call with inspectability of the command-defining -S field. Preserve literalness/taint per argument while locating and expanding env -S: resolve a literal split-string payload even when later inert argv is dynamic, and fail closed only when the executable position or an interpreter-source position depends on an unresolved value. Keep the AST and fallback paths on the same env -S resolver, then add engine tests for static Git/curl payloads plus dynamic trailing argv, dynamic executable/source fields, and local controls whose dynamic arguments are not command-defining.

• [P1] Fail closed for dynamic executable positions inside PowerShell command source
  internal/sandbox/analyzer.go:216
  The new outer-source check only rejects a nonliteral -Command argument. A literal PowerShell program such as powershell -Command '& $env:APP https://evil.test' executes curl when APP=curl, but textualPayloadUsesNetwork parses the PowerShell text as POSIX shell. That parser treats $env:APP as a dynamic command word and effectiveProgram silently drops it, leaving neither Network nor TooComplex set; the outer command is therefore allowed without the network prompt. Assigning $APP='curl' inside the PowerShell source and invoking & $APP … has the same problem.

  The root cause is treating a successfully parsed-but-not-semantically-understood PowerShell payload as evidence that it is local. Model PowerShell command source separately from POSIX shell source: either parse enough PowerShell invocation forms to identify literal command positions, or conservatively mark an unresolved invocation/operator position as network-sensitive. Do not use the POSIX parser's clean result to clear a PowerShell payload it cannot interpret. Add Engine.Evaluate coverage for & $env:APP, a variable assigned in the source, valid literal network commands, and literal local commands so the fail-closed rule remains narrowly scoped.

[jatmn]

I found issues that need to be addressed before this is ready.

This PR has been through many review rounds because each pass fixes a specific spelling (git -C repo push, git.exe, caret escapes, abbreviated --split) while the underlying classifier architecture still treats parseable and unparseable input as two separate worlds, and treats interpreter source (CMD /c, eval, PowerShell -Command) as argv tokens. The sections below name the three merge blockers, then explain why the drip pattern keeps happening and what to change so the next round is not another spelling hunt.

---

Why this keeps producing follow-up findings

1. Two classification paths that do not share outcomes

Production classification flows through Classify → AnalyzeCommand (AST/analyzeInto) plus, only when TooComplex, matchesUnparseableNetwork (risk.go:1226–1227). The PR invested heavily in fallbackBodyUsesNetwork, CMD resolvers, and git token parsers on the fallback side, but Classify never consults fallback when the POSIX parser succeeds. That means any launcher the AST path does not recurse into (eval, quoted CMD payloads) stays clean even when matchesUnparseableNetwork would return true. Fallback-only tests (&& "unterminated", & rem ') pass; parseable bypasses remain.

Root-cause fix: Pick one authoritative classifier for network (and destructive) on shell commands. Practical options:

• Fold fallback into Classify: e.g. network := analysis.Network || matchesUnparseableNetwork(command) with fallback tuned to avoid the old whole-string false positives, or
• Make the AST path complete: every launcher that runs command text (eval, cmd/call/start, PowerShell -Command, env -S, shell -c/--command) must recurse into payload as bounded command text, not pass payload through fallbackBodyUsesNetwork as body[0].

Do not add another parallel skip list without updating both paths and Classify.

2. Interpreter source vs executable argv

CMD /c "git push origin main" is not “run the program named git push origin main”. It is “run the command line git push origin main”. The new cmdLauncherUsesNetwork (analyzer.go:673–695) extracts the /c operand and passes it to fallbackBodyUsesNetwork, which calls executableTokenBase(body[0]) (risk.go:155) — basename only, no whitespace split, no re-tokenize. The same mistake class hits quoted call "…" payloads.

Root-cause fix: Introduce a small shared operation, e.g. classifyCommandText(payload string, depth int) bool, used by:

• AST launcher branches after extracting /c, /k, CALL, eval, shell -c, PowerShell -Command
• Fallback CMD/PowerShell payload extraction (fallbackCommandInterpreterPayload, etc.)

That function should tokenize payload as command text (with CMD/POSIX rules as appropriate), recurse with depth limits, and fail closed when text cannot be read. Do not route interpreter payloads through executableTokenBase on the first token.

3. “Literal path succeeded” mistaken for “invocation is safe”

envSplitSourceDynamic (analyzer.go:574–577) returns false after a literal -S operand (“the ordinary literal path already reads it”), without scanning remaining argv. GNU env still appends trailing arguments (env -S 'printf ok' $PAYLOAD). The same pattern appears elsewhere: proving the split-string operand is literal is not proof the whole env invocation is statically known.

Root-cause fix: After recognizing -S/--split-string, scan the rest of the env argv. Any nonliteral token after the split → fail closed (Network=true). Apply the same rule on the fallback env split path. Negative tests: trailing literals only stay non-network.

4. Duplicated git grammar

parseGitInvocation, matchesUnparseableGitNetwork, gitTargetsRemoteArchive, and command_prefix.gitSubcommand each carry slices of git’s option grammar. This PR unified GitGlobalOptionConsumesValue but not terminal globals (--help, bare --exec-path, etc.), so sandbox and prefix parsers still disagree. Each round fixes one option (--attr-source, --remote after --) without a single git module.

Root-cause fix: One shared git scanner (value-taking globals, terminal globals, subcommand position, archive operand layout) feeding both gitUsesNetwork and matchesUnparseableGitNetwork, and export terminal-global behavior for command_prefix.go if prefix safety must stay aligned.

5. Test strategy that hides parseable gaps

Many new tests only exercise the unparseable suffix (& rem ', && "unterminated") or AnalyzeCommand helpers, not Engine.Evaluate with NetworkDeny + PermissionGranted: true. The integration test uses BackendUnavailable and never asserts NetworkAllow on a backend stub. So each round proves fallback spellings while parseable Windows forms (cmd /c "git push …") and grant application stay unverified.

Root-cause fix: For every launcher you add or touch, require a parseable Engine.Evaluate case: NetworkDeny, shell already granted, expect ActionPrompt + ReasonNetworkBlocked. For grant claims, assert plan policy NetworkAllow like TestGrantRequestPermissionsNetworkOverlaysPolicyForTurn. Use a parity table: old regex / known bypass → must be covered on parseable path, not only behind rem '.

6. Stale branch noise

Fork point dc15e822 vs main 0eab63c makes GitHub’s two-dot diff show ~50 files of apparent reverts. That wastes review cycles on credstore/dictation/daemon paths that never land on merge. Rebase first; review the three-dot diff only.

---

Findings (must fix before merge)

• [P1] Rebase this branch onto current main before merge
  Where: branch topology (dc15e822 merge base vs 0eab63c declared base)
  What happens: GitHub’s two-dot diff (main..head) spans 50 files and shows apparent reversions of path-jail (#891), credential-store locking (#891), daemon cleanup (#774), dictation fixes (#852), worktree git hardening (#891), and diff-viewer work (#902). The three-dot merge surface is ten sandbox/agent files. Those “reverts” are diff artifacts — e.g. worktrees.go drops hardenWorktreeGit in the two-dot view but not in a merge onto 0eab63c. CI is green only on stale head 8a7a7ddf; there is no check run on a rebased artifact.
  Author guidance: Rebase onto 0eab63c, resolve conflicts preserving main behavior, push, and request review of the three-dot diff. Do not interpret two-dot “reverts” as work this PR needs to undo. After rebase, re-run the validation block from the PR description on the new head.

• [P1] Classify quoted CMD and CALL payloads as command source (PR regression)
  Where: internal/sandbox/analyzer.go:673 (cmdLauncherUsesNetwork); internal/sandbox/risk.go:155 (executableTokenBase in fallbackBodyUsesNetwork)
  Repro (verified on head): cmd /c "git push origin main", cmd /c "curl https://evil.test", call "git push origin main" → AnalyzeCommand: Network=false, TooComplex=false. Engine.Evaluate with NetworkDeny + PermissionGranted: true → ActionAllow, reason tool requires approval before execution, risk categories ["shell"] only — no network prompt, no turn overlay. Unquoted cmd /c git push origin main correctly gets network.
  Why this PR: cmdLauncherUsesNetwork does not exist on main. It was added to cover CMD on the AST path but passes quoted /c operands as a single token to fallbackBodyUsesNetwork, which treats git push origin main as the program name.
  Failure impact: On Windows, Zero forwards bash-tool text to cmd.exe /d /c. Quoted one-liners are common. After one bash approval, push/curl egress without network prompt — the approval model Windows unelevated relies on when WFP is unavailable.
  Root-cause fix: After /c, /k, or CALL selects a payload, call shared classifyCommandText(payload) (re-tokenize as command source; fail closed if unreadable). Do not pass quoted payload through executableTokenBase(body[0]). Reuse the same helper in fallback CMD extraction so paths cannot diverge.
  Tests: Engine.Evaluate — quoted cmd /c "git push …" (network prompt), quoted call "curl …" (network prompt), quoted cmd /c "git status" (no network).

• [P1] Keep env -S classified when trailing argv is dynamic (PR regression)
  Where: internal/sandbox/analyzer.go:168–175 (analyzeInto env split branch); envSplitSourceDynamic (analyzer.go:544–577)
  Repro (verified on head): env -S 'git push origin main' "$EXTRA", env -S 'printf ok' $PAYLOAD, env -S 'printf ok' "${PAYLOAD}" https://evil.test → Network=false, no network category. Contrast: env -S 'printf ok' curl "$URL" flags network (literal curl in split string). Dynamic inside split string (env -S "$PAYLOAD") is covered; trailing argv after a literal split is not.
  Why this PR: envSplitSourceDynamic is new. At lines 574–577, after a literal -S operand it returns false immediately without scanning further tokens. literalCallFields fails on trailing $PAYLOAD, so the fast path at 168–175 never runs; analysis falls through to effectiveProgram("env") with no network flag.
  Failure impact: Attacker or agent can hide network binary in trailing expansion after a benign-looking split string once shell is approved.
  Root-cause fix: In envSplitSourceDynamic and the literal envSplitCommandFields success path: after consuming -S operand, continue scanning remaining argv; any !isLiteralWord → treat as network-dependent / fail closed. Do not use “literal split operand” as early exit for the whole invocation. Mirror on fallback env split handling.
  Tests: Engine.Evaluate for trailing "$EXTRA", $PAYLOAD, and negative control with trailing literals only.

---

Follow-up (optional for #703; pre-existing or adjacent)

I would not block merge on these alone after the three items above, but they explain remaining review noise if left unaddressed.

• [P2] Parseable eval bypass (analyzeInto + classifyWithScope:1226–1227)
  Repro: eval git push origin main → matchesUnparseableNetwork=true but Classify has no network (TooComplex gate exists on main). PR added eval in fallbackBodyUsesNetwork but not AST analyzeInto.
  Guidance: Add eval launcher branch calling classifyCommandText(strings.Join(args," ")), or fold fallback into Classify as described above.

• [P2] Double-quoted nested sh -c (analyzeInto:194–203)
  Depth 3+ with fmt.Sprintf("sh -c %q", …) loses network without fail-closed; single-quote nesting is covered. Fail closed when launcher present but payload not resolved.

• [P2] Parseable @curl (commandWordsUseNetworkAt + trimCMDEchoPrefix only on fallback)
  @curl https://evil.test clean on AST; unparseable @curl … & rem ' flagged. Apply echo-prefix normalization before program match on AST path.

• [P2] gitSubcommand vs gitTerminalGlobalOptions (command_prefix.go:395 vs analyzer.go:449)
  Sandbox stops at terminal globals; prefix parser does not. Share terminal-global set or export from shared git module.

• [P3] git send-pack not in whitelist (gitUsesNetwork:363) — pre-existing on main.

• [P3] Integration test (loop_test.go:2444) uses BackendUnavailable, does not assert NetworkAllow on backend stub — tighten test or PR description.

---

Suggested author checklist before next review request

1. Rebase onto 0eab63c; confirm three-dot diff is sandbox/agent only.
2. Implement shared command-text classification for interpreter payloads (CMD/call/eval/shell/PowerShell); stop routing payloads through executableTokenBase.
3. Fix env trailing argv scan; add trailing-dynamic Engine.Evaluate tests.
4. Decide Classify vs AST completeness for fallback parity (eval, @curl); do not add fallback-only fixes without parseable coverage.
5. Run parseable Engine.Evaluate matrix for every launcher touched; not only & rem ' cases.
6. (Optional) Single git grammar module + prefix alignment; optional follow-up PR if scope is tight.

This should collapse the next review to “did you rebase and fix the two PR regressions plus the architectural split?” rather than another round of individual spellings.

[PierrunoYT]

Rebased onto d065467c and took the architectural fix rather than another spelling pass. Head is f0bca25d; the three-dot diff is now 7 files (sandbox/agent only).

The split, closed

The root cause you named — interpreter source treated as executable argv, and two classification paths that don't share outcomes — is addressed with one shared operation.

classifyCommandText(payload, depth) is now the single entry point for "a string another program will run as a command line". It re-tokenizes rather than taking a basename, runs both the AST scan and the unparseable matcher, and fails closed when the text can't be read. CMD /c//k, CALL, start, eval, shell -c, and PowerShell -Command route through it on both paths, so a launcher cannot be command text on one and a program name on the other. textualPayloadUsesNetwork is gone rather than kept as an alias — there is one function.

Running both scans is deliberate: a CMD one-liner is legitimately not POSIX, so the parser rejecting it is expected, not evidence of safety. That is the "fold fallback in" option applied where it actually matters (payloads) without the whole-string false positives the old regex had.

Findings

[P1] Rebase — done, onto d065467c (main moved past the 0eab63c you cited). Clean, no conflicts. Three-dot surface: analyzer.go, risk.go, command_prefix.go + their tests and loop_test.go. The credstore/daemon/dictation/worktree "reverts" are gone from the view.

[P1] Quoted CMD and CALL payloads — fixed. cmdBodyUsesNetwork reads a single quoted token both ways, because the ambiguity is genuine: cmd /c "git push origin main" is a command line, cmd /c "C:\Program Files\curl\curl.exe" is one program path containing a space. CMD itself resolves that by trying the path then parsing as a command line, so the classifier does the same and fails closed if either reading reaches the network. Shared with the fallback's candidate loop. Verified: all six quoted forms now reach ActionPrompt/ReasonNetworkBlocked, and cmd /c "git status" / cmd /c "C:\Program Files\git\bin\git.exe status" stay local.

[P1] env -S trailing argv — fixed. envSplitSourceDynamic no longer early-exits on a literal -S operand; it scans the rest of the argv and fails closed on any non-literal token. Your framing was the useful part: the literal operand proves something about the split string, not the invocation. env -S 'sh -c' "$PAYLOAD" is the case that makes it concrete. Trailing literals only stay non-network.

Also fixed (your P2/P3 list)

Parseable eval — added the AST branch; source assembled from an expansion fails closed. Both paths now agree.

Nested double-quoted sh -c — this turned out to be a real bug rather than a depth issue, and it's worth flagging because it affected more than sh -c. wordText returned raw source for DblQuoted parts: mvdan/sh preserves \" as two characters because escape removal is the shell's job at expansion time. For an argv token that's harmless; for a -c operand the text is the next command. One unwrap of sh -c "sh -c \"curl …\"" handed the recursion the fragment \"sh and silently dropped the rest — which is why it died at nesting level 3 regardless of the depth limit. unescapeDoubleQuoted applies POSIX escape removal for the five characters where a backslash is special inside double quotes, so a quoted Windows path ("C:\Users\me\file.txt") survives intact. Nesting now classifies correctly at every level, and the existing depth guard handles exhaustion.

Parseable @curl — echo-prefix normalization applied before the program match on the AST path, via a shared trimCMDEchoPrefixToken.

gitSubcommand vs terminal globals — GitTerminalGlobalOption exported from sandbox and consumed by command_prefix.go, same pattern as GitGlobalOptionConsumesValue. This one had a real consequence: git --help status resolved to the read-only prefix git status and was auto-approved as a command it isn't.

git send-pack — added to the network subcommand list. Since matchesUnparseableGitNetwork already defers to gitUsesNetwork, both paths pick it up from one edit — which is the single-git-scanner property you asked for on item 4, at least for the subcommand and terminal-global grammar. A full git module extraction I left alone as the follow-up you marked optional.

Integration test — tightened rather than re-scoped. It now asserts BuildCommandPlan(...).Policy.Network == NetworkAllow during the approved tool call (via a probe tool that delegates to the real scoped bash tool), and that it is not NetworkAllow after the turn. I checked the assertion isn't vacuous by inverting it — it fails with network = "allow". BackendUnavailable is retained because the whole suite uses it for cross-platform reasons, and the overlay is a policy fact rather than a backend fact, so the assertion holds regardless.

Test strategy

TestEvaluatePromptsForParseableNetworkLaunchers is the parity table you asked for: 21 launcher forms, each asserted parseable (the guard fails the test if TooComplex), through Engine.Evaluate with NetworkDeny + PermissionGranted: true, expecting ActionPrompt/ReasonNetworkBlocked. TestEvaluateAllowsParseableLocalLaunchers is its negative counterpart — 14 local forms that must not cost a network prompt, because failing closed everywhere would just train users to approve egress reflexively. Both tables are the place a new launcher gets added.

Validation

gofmt -l clean (only the pre-existing unformatted perfbench/testdata fixtures, unchanged by this branch), go vet ./..., go test ./..., go run ./cmd/zero-release build, go run ./cmd/zero-release smoke, git diff HEAD --check.

Two I could not run locally and am not claiming: go test -race needs cgo and there's no gcc in this environment, and make lint-static fails before linting because the pinned golangci-lint v2.12.2 is built with Go 1.25 while the module now targets 1.26.6 (the Go language version used to build golangci-lint is lower than the targeted Go version) — that's from main's Go bump in dc15e822, not this branch, but it may be worth a separate version bump.

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P2] Do not turn an over-depth shell launcher into a clean command
  internal/sandbox/safe_command.go:154
detectInteractiveCommandAt now returns an empty InteractiveCommandResult once depth > maxAnalyzerDepth. Its callers interpret that result as a confirmed non-interactive payload, so five nested sh -c/bash -c launchers around vim file bypass the no-TTY preflight and run until the agent times out. The new test deliberately pins that false negative, but the detector's contract is to prevent terminal-bound commands from hanging the agent; an inspection budget must not turn “could not inspect” into “safe.” Preserve the recursion bound, but give the depth-exhausted case a conservative result (for example, a dedicated unsupported-nested-launcher reason marked interactive) and add an execution-path regression proving an over-depth wrapper chain is blocked rather than launched.

• [P2] Remove unrelated test seams from the issue-scoped PR
  internal/providers/providerio/export_test.go:1
  This PR adds four unreferenced test-only exports across providerio, agenteval, tools, and tui (ScanSSEData, TraceEventKeys, ValidateToolCapabilities, and the TUI PR helpers). None is used by the sandbox/network implementation or its regression tests, and none belongs to approved issue #703. This is scope drift rather than support for the Git network grant: it adds test-visible API surface in unrelated packages, makes the security fix harder to audit, and leaves future maintainers to preserve helpers with no stated owner or use case. Remove these seams from this PR; if an independent test suite genuinely needs one, submit it separately with the consuming test and the relevant approved issue.

Guidance for the next revision

The recurring review churn is coming from a structural problem, not from any one command spelling. This PR now contains several partially overlapping readers for command execution semantics: the AST walker, the unparseable-command fallback, command-text recursion for launchers, env -S rewriting, CMD/PowerShell helpers, and separate interactive-command inspection. A narrow fix in one reader can leave another reader with a different answer, especially when a launcher changes whether an argument is argv or executable source. The depth-limit regression is an example of the same pattern: bounding a recursive reader was treated as an implementation detail, but its caller interprets the resulting empty value as a positive safety conclusion.

Before asking for another review, please make the safety contract explicit and drive every reader from it:

1. Define three distinct outcomes for command inspection: known local, known network/interactive, and unresolved. Do not represent unresolved input as the same zero value as known-local input. At a recursion, parsing, decoding, or token-budget limit, the caller must choose a conservative prompt/block path rather than silently continuing.
2. Keep one authoritative command-text classifier for launchers. Shell -c, eval, CMD /c/CALL, PowerShell command operands, and any wrapper that executes source should pass their payload through that classifier with a shared depth budget. Keep argv parsing separate from source parsing; a quoted command line is not a single executable filename.
3. For wrappers that rewrite argv, such as env -S, model the complete resulting argv—including appended trailing arguments and option terminators—before deciding whether the executable or a command-source operand is statically known. Use the same resolved representation for both the AST and fallback paths where possible.
4. Make the focused tests prove the enforcement boundary, not just helper return values. For every launcher family touched, include parseable Engine.Evaluate coverage under NetworkDeny that asserts a network command prompts and a local command does not. Include the over-depth case and verify that it is blocked/prompted rather than merely observing the detector helper.
5. Keep the final diff limited to #703. Move unrelated test seams and any follow-on parser generalization that cannot be covered by the Git network-grant behavior into focused, independently approved work.

That approach should make the next review about whether the single enforcement contract is satisfied, rather than about another parser-specific edge case.

[PierrunoYT]

PierrunoYT updated the branch through 2cbf268f on fresh upstream main.

• Over-depth shell-launcher inspection now returns a conservative blocking result instead of the clean zero value. Nested quoted payloads reach the parse-aware reader without destructive re-trimming, and a real bash-tool regression proves unresolved launcher chains are blocked before execution (exit_code=-1).
• The unrelated, unreferenced providerio/agenteval/tools/TUI test seams are absent after incorporating upstream d66ad715; the fresh diff remains scoped to the sandbox network grants don't persist after approving — network access stays denied, fails with context canceled #703 agent/sandbox work plus the enforcement-path test.
• Validation passed: focused tests, race-enabled sandbox/agent tests, make fmt-check, go vet ./..., go test ./..., release build and smoke, make lint-static (0 issues), and diff hygiene.
• make vulncheck ran and reports the known unrelated GO-2026-6115 denial-of-service finding in github.com/ledongthuc/pdf (fixed version: N/A), reachable from internal/imageinput/pdf.go; no other vulnerabilities were found.