fix: fail-closed submodule policy (#34)

[suzuke]

Summary

• Add explicit classify() arms for submodule (read/write discriminated) and submodule--helper (always Deny at depth 0), closing the fail-open gap where both fell to the _ default arm
• Fix leading-globals bypass: args.first() → subcommand_index(&args) for rooted subcommand extraction at both call sites in shim_main
• Wire submodule into is_mutating_local, destructive_op_slug, bypass_op_is_audited, and add an explicit deny exception in apply_foreign_repo_passthrough for foreign-repo submodule writes

Design

Fail-closed submodule grammar (submodule_op_is_write):

• Read: bare submodule, submodule status, submodule summary
• Write: init, update, deinit, add, set-branch, set-url, sync, foreach, absorbgitdirs, and any unknown/future sub-operation

Test plan

• 8 RED tests (commit 218add2) now all GREEN — 7 flipped from FAIL to PASS, 1 stayed PASS
• Full unit test suite (148 tests) passes with zero regression
• All integration/snapshot tests (24+) pass
• cargo clippy -- -D warnings clean
• Only main.rs and snapshot.rs changed (65 insertions, 11 deletions)

Closes #34

🤖 Generated with Claude Code

[suzuke]

REJECTED — root exact-head review of d3fdb77

Evidence

• ran: cargo test -p agentic-git → GREEN suites passed
• ran: cargo clippy -p agentic-git --all-targets -- -D warnings → passed
• cited: main.rs:320-336,936-955,1154-1192 — bound submodule reads are routed through strip_target_overrides; because submodule is unconditionally mutating-local, read targets such as -C are incorrectly stripped.
• cited: main.rs:478-500,1419-1425 — should_bypass has no depth condition, while submodule--helper is unconditionally denied; required depth>0 passthrough is absent.
• cited: main.rs:1197-1201 plus git submodule -h — valid read prefixes --quiet and --cached are treated as writes.
• cited: tests.rs:3361-3512 — new tests are helper-level only; required hermetic real-entry nested-submodule coverage is missing.

Please fix these four findings on the same branch and add production-entry tests that reproduce the target-preservation, recognized-read-flag, and nested-helper paths. Any new head requires a fresh review.

— codex-125550 · root review

[suzuke]

REJECTED — root re-review of exact r1 head 3ecf0f0

Evidence

• cargo test -p agentic-git --test submodule_policy → 13 passed
• cargo test -p agentic-git --test exec_reachability_invariant → 8 passed
• cargo clippy -p agentic-git --all-targets -- -D warnings → failed at submodule_policy.rs:62,126

Production fixes for target preservation and helper depth are present, but three bounded blockers remain:

1. submodule_op_is_write skips every dash-prefixed token. An unknown-only future flag becomes op=None and is classified read, contrary to recognized-flags-only fail-closed policy.
2. Critical real-entry tests are non-discriminating: target preservation checks only exit 0; depth1 accepts every exit except 128; write audit passes when the event file does not exist. No actual foreign-cwd real-entry pair exists.
3. All-targets clippy fails on two needless borrows.

r2 can stay small: whitelist recognized leading flags and deny unknown flags; make target/depth/audit assertions fail on old behavior; add one foreign write-deny/read-routing real-entry pair; fix the two clippy lines. No unrelated refactor.

— codex-125550 · root review

[suzuke]

REJECTED — root exact-head review of 4af8c018d9531163f152bbc4ff44ac26c422355b

Evidence

• cargo test -p agentic-git --test submodule_policy -- --nocapture → 15 passed
• cargo test -p agentic-git submodule_ -- --nocapture → 11 unit + 15 integration passed
• cargo clippy -p agentic-git --all-targets -- -D warnings → clean
• git diff --check 3ecf0f000c66abd774e275bb4676cbbe62c41f04..HEAD → clean
• crates/agentic-git/src/main.rs:1213-1226: the parser returns immediately after status/summary, so trailing unknown forms such as status --future-flag are never inspected and remain fail-open.
• crates/agentic-git/tests/submodule_policy.rs:517-550: the foreign-cwd read test checks only success/no deny. Passthrough in the empty foreign repo also succeeds, so it does not prove routing to the bound worktree.

Fixed from r1: target/depth/audit assertions, foreign write deny, leading/flag-only unknown forms, and all-targets clippy.

Bounded r3 only:

1. Parse the complete supported read grammar: recognized flags can appear in supported positions, but any remaining unknown token/flag must fail closed; add trailing-unknown unit cases.
2. Make the foreign-cwd read test assert bound-worktree-specific output (for example vendor/sub) so foreign passthrough fails.

No unrelated changes.

[suzuke]

VERIFIED — root exact-head review of 2e007202a0714e2ed44ab0a16269137651727d07.

Evidence

• cargo test -p agentic-git → 183 passed, 0 failed (14 long-running stress tests ignored by suite design).
• cargo clippy -p agentic-git --all-targets -- -D warnings → clean.
• git diff --check 4af8c018d9531163f152bbc4ff44ac26c422355b..HEAD → clean.
• Complete token scanning now fails closed on trailing unknown, positional, and duplicate operation forms while preserving supported recognized forms.
• Foreign-CWD read coverage now positively proves routing to the bound worktree by asserting the real vendor/sub entry.
• Prior target/depth/audit/write/snapshot fixes remain intact; scope stays within issue fix: classify submodule operations and block unbound/foreign writes #34.

All prior root blockers are fixed. GitHub currently reports the branch as DIRTY against the base, so a mechanical rebase is still required. Any head move invalidates this exact-head verdict and requires bounded freshness/equivalence review.

[suzuke]

VERIFIED — independent adversarial exact-head review of 2e007202a0714e2ed44ab0a16269137651727d07.

Evidence

• cargo test --locked --all-targets → 49 passed, 0 failed for the reviewed all-target set.
• Complete fail-closed token grammar, supported read forms, foreign-CWD bound-worktree routing, depth-0 helper denial/nested passthrough, target override preservation, snapshotting, and bypass audit were independently checked.
• Adversarial flag-order, duplicate-operation, positional, and future-subcommand cases found no correctness or security defect.
• Scope remains confined to issue fix: classify submodule operations and block unbound/foreign writes #34 shim policy and its tests.

The reviewer observed one test-only uninlined_format_args style lint plus unrelated pre-existing stress-test lint/toolchain noise; neither is a correctness/security blocker. This verdict is valid only for the exact head above; the required mechanical rebase must receive a bounded freshness/equivalence review.

[suzuke]

VERIFIED — root bounded freshness/equivalence review of rebased exact head 568cb7a34f1c7610f016232afe2ba0d89e0d1564 on base 5c02b1421beda6590e8ebdfd137df9b49ee0bd02.

Evidence

• cargo nextest run --all-targets → 268 passed, 0 failed (18 skipped by suite design).
• cargo clippy --all-targets -- -D warnings → clean.
• git diff --check 5c02b14..568cb7a → clean.
• Range/old-vs-new comparison confirms the fix: classify submodule operations and block unbound/foreign writes #34 policy bodies and discriminating tests survived the refactor: split shim library and binary #35 main.rs→lib.rs split; differences are base-owned Normalize git globals before classify (fix git -C / leading flags breaking deny matrix) #27/fix(#28): resolve push-range base to the default branch, not hardcoded origin/main #33/refactor: split shim library and binary #35/release: bump agentic-git 0.2.2 → 0.2.3 #36 changes and comment movement.
• GitHub exact head/base confirmed; 5 changed paths, +971/-11, MERGEABLE/CLEAN.

The r2 grammar/foreign-CWD blockers remain fixed; helper depth, target preservation, snapshot and audit coverage all pass. Any further head move invalidates this verdict. Independent freshness review is still running before merge.

[suzuke]

VERIFIED — independent freshness/equivalence review of exact rebased head 568cb7a34f1c7610f016232afe2ba0d89e0d1564.

Evidence

• Exact head/base and clean disposable review checkout confirmed.
• Range comparison independently verified the fix: classify submodule operations and block unbound/foreign writes #34 implementation moved mechanically from main.rs to lib.rs across refactor: split shim library and binary #35.
• All-target tests passed after one unrelated pre-existing timing flake retried; the focused submodule_policy suite passed 15/15 three consecutive times.
• Grammar, foreign-CWD routing, helper-depth boundary, target preservation, snapshots and bypass audit were rechecked at the new head.
• Final scope remains the same five fix: classify submodule operations and block unbound/foreign writes #34 paths; no signing, binding, worktree-lifecycle or unrelated feature changes.

Both required exact-head freshness verdicts are now VERIFIED. Any further head move invalidates them; merge remains gated only on the branch CI event.