fix(security): make the push argument boundary shell-faithful

[iamwhatever]

Problem / Motivation

Main is red: nine tests fail on the git-publish push gate in src/kiro_crew/security.py, from opposite directions with one root cause.

• SET A (main is red: #7808 broke test_push_branch_gate.py (6 failures), cascading to every PR #8695) — test/test_push_branch_gate.py: 6 failed in TestUnrecognisedOptionsReadProtectively. These are fix(security): model publish option arity so the floor tag holds #7808's own assertions failing on arrival: shapes like an extglob refspec (@(main)), a heredoc strip operator (<<- EOF), a glued redirection (origin> log), a glued all-branches flag behind a redirect, and a background operator LOSE their protective floor tag — under-tagging, a security hole.
• SET B (main red: test_security.py::TestGitPublishSubshellGluing (3 tests) fails on every merge ref since #7808 #8700) — test/test_security.py -k TestGitPublishSubshellGluing: 3 failed. These date from fix(security): close four enforcement bypasses in the bash command gates #7356 and were green before: a redirect-target process substitution (> >(tee log.txt)), path-qualified feature pushes, and quoted-operator ref names ('feature|x') are read as denials — over-blocking legitimate feature publishes.

Why it matters

Every open PR inherits the failure through refs/pull/N/merge (Backend Tests shards, Backend Lint & Type Check, Coverage Gate cascade), so no PR can reach a green rollup until this lands. The under-tagging half is additionally a per-rule-opt-out hazard on the push gate itself.

What changed (motivation → approach → change)

Root cause: the positional argument list is derived in one place (_git_push_args, reworked by #7356) and modelled in another (_push_segment_targets_protected, rewritten by #7808 on a pre-#7356 base). The cross-merge of 166ff5a (#8491) followed by eaa8a45 (#7808) left the two disagreeing about where the argument list stops when a segment carries a shell metacharacter: the extractor returned operator-CUT words and stripped constructs the walk was written to model on RAW spellings.

The fix makes the boundary derived once, faithfully — the commit body carries the full per-metacharacter answer to "where does the list stop, and where must it stop". Summary:

1. _git_push_args returns RAW words; the walk dequotes/cuts them itself. The extractor's redirect grammar folds <<-'s dash into the operator, and keeps a process-substitution WORD (<(cmd) at word start) as one argument for the walk to land on the ungated branch, while a redirect-TARGET substitution is stepped over (the shell consumes it before git runs).
2. Process substitution left the segment-wide _AMBIGUOUS_EXPANSION_RE (that segment-wide match is what over-blocked > >(tee log.txt)): it is position-dependent and now handled positionally.
3. Segments are split quote-aware (_split_push_segments): a quoted ;/| in a legal ref name is data. An unquoted newline still separates, so a newline remaining in a segment is a bash splice — ungated, unless a nested payload is itself a publish (a multi-line quoted script), which defers to the payload walk.
4. A git token that begins inside a quoted span is a wrapper's payload text and never anchors the outer line (_push_token_start_states); every deferral is gated on top-level residue (_push_top_level_residue_hazard), so bash -c '…feat…' & $x push origin main stays ungated-denied while bash -c '(cd /tmp && git push origin my-feature)' stays allowed.
5. A glued-anchor segment ((git push origin main)) escalates any finding to the ungated sentinel via one shared anchor resolver (_resolve_push_anchor); a pure subshell-closer tail (my-feature)) is punctuation, not word fusion.
6. Process-substitution spans are measured with a quote-aware paren walk (_paren_construct_end) — a quoted ( in the body is data; counting it swallowed a trailing protected refspec.

Items 4–6 and the quote-aware $-residue reading (_push_token_expands) came out of two pre-push review rounds (gpt-5.6-sol + opus-5) plus a verifier round; both reviewer-found allow-direction holes are fixed and regression-tested. No protective reading was weakened: every change widens what is denied or removes a reading bash itself contradicts.

The spec's stated ordering risk held: both failure directions were re-run after every change. FIX FORWARD as directed — #7808 is not reverted; its 749 test lines are the arity model this implements.

Tests

• The 9 existing failing tests are the specification and are untouched: 6+3 → 0 failed (test_push_branch_gate.py 115 passed, TestGitPublishSubshellGluing 15 passed).
• New TestArgumentBoundaryHelpers (12 tests): raw-word extractor contract, <<- delimiter consumption, process-substitution word retention, payload-quoted anchor gating, quote-aware segmentation, quoted-newline splice vs multi-line payload, glued-verb escalation vs clean subshell feature push, subshell-closer punctuation, quoted-paren process-substitution boundary (review round), wrapper-deferral residue gate (review round), ANSI-C $'…' span integrity.
• Full backend suite: FAILED list identical to pristine main on the same host (103 environmental failures both sides; one extra xdist-load flake passes in isolation). Before/after per the spec: 6 failed/97 passed and 3 failed/12 passed → 0 failed in both.

Manual verification

N/A — unit coverage sufficient: the gate is a pure function of the command string, and every reported shape (plus the review-round exploits) is pinned as a direct unit test.

Related Issues

Note: PR #8672 touches the same area of src/kiro_crew/security.py on a base predating #7808 — it does not carry the arity model and will inherit this red until it rebases onto this fix; a textual conflict there is expected and deliberately not absorbed here. Issue #7832 (bash-faithful word lexer) is the architectural successor and is intentionally out of scope.

Closes #8695
Closes #8700

Pattern harvest

Rule candidate: review-prompt
Pattern: "two PRs each green on their own base, red together — a parser split across an extractor and a modeller must derive its boundary once; flag any PR that changes one of _git_push_args / _push_segment_targets_protected without running both directions' test files".

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of a4782554fb8d3f23977d8ea46ddbd8c6fe034fea — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Sound fix-forward for a real red-main security regression, but it grows the hand-rolled bash emulation into six must-agree quote-state automata inside one fail-closed gate.

Watch

• After this PR, security.py carries six independent single/double/ANSI-C quote walkers (_push_token_shell_read, _split_push_segments, _paren_construct_end, _push_token_start_states, _push_top_level_view, _push_token_expands). The PR's own comments name walk-disagreement as the bug class ("without it the walks disagree about one grammar"), and its harvest rule says a boundary "must be derived once" — yet the quote grammar is now derived six times. Cause → mechanism → consequence: a future grammar tweak (e.g. $"...", backticks) lands in one walk and not the others → the walks diverge exactly as the fix(security): model publish option arity so the floor tag holds #7808/fix(security): close four enforcement bypasses in the bash command gates #7356 cross-merge did → the same over-block (main red: test_security.py::TestGitPublishSubshellGluing (3 tests) fails on every merge ref since #7808 #8700) or under-tag (main is red: #7808 broke test_push_branch_gate.py (6 failures), cascading to every PR #8695) harm resurfaces, undetected until adversarial testing. security: consolidate the git-publish scan onto a bash-faithful word lexer #7832 is the tracked cure; until it lands this file is a drift hazard every touch must re-verify in both directions.

Suggestions

• Extract one shared advance-quote-state(text, i, state) step the six walks call, so the grammar physically cannot diverge — a small, in-scope consolidation that doesn't wait for the security: consolidate the git-publish scan onto a bash-faithful word lexer #7832 lexer.

[DESIGN-REVIEWED] a478255

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of a4782554fb8d3f23977d8ea46ddbd8c6fe034fea — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

First-Principles-Verdict: PASS

Nine red tests on main are the specification; every behavior change traces to one of them or to a review-found allow hole, and the deferral to #7832 is declared.

What this change ships

Intent: turn main green by making the push gate's extractor and modeller derive the argument boundary once — a FIX.

1. Nine failing tests on main go green, both directions — justified (unwedges a red build)
2. Legal quoted-operator refs ('feature|x', 'a;b') no longer denied — justified (main red: test_security.py::TestGitPublishSubshellGluing (3 tests) fails on every merge ref since #7808 #8700)
3. Feature push with > >(tee log.txt) redirect-target no longer denied — justified (main red: test_security.py::TestGitPublishSubshellGluing (3 tests) fails on every merge ref since #7808 #8700)
4. Subshell feature push (cd /tmp; git push origin my-feature) now allowed — justified (main red: test_security.py::TestGitPublishSubshellGluing (3 tests) fails on every merge ref since #7808 #8700)
5. @(main), <<- EOF, glued origin>, glued --all regain protective tags — justified (main is red: #7808 broke test_push_branch_gate.py (6 failures), cascading to every PR #8695)
6. Glued-verb (git push origin main) escalates past single-row opt-out — justified, declared (per-rule opt-out hazard)
7. Wrapper deferral now denies on top-level residue (… & $x push origin main) — justified, declared (allow-direction hole)
8. Quoted-newline splices denied; multi-line quoted payloads defer to payload walk — justified, declared
9. Spec section in docs/system-specs/modules/security.md — mandated (AGENTS.md same-commit rule)
10. _CMD_SEPARATOR_RE kept alive with zero consumers — undeclared rider

Subtractions

• Delete _CMD_SEPARATOR_RE (src/kiro_crew/security.py:6377). After the switch to _split_push_segments it has zero consumers — grepped _CMD_SEPARATOR_RE across src/ (1 definition + 1 docstring mention) and test/ (1 comment). The diff keeps it "for the documentation of what separates segments", but that separator set is already spelled out in _split_push_segments's own docstring; a stranded regex kept as prose will silently diverge from the walker that replaced it.

[FIRST-PRINCIPLES-REVIEWED] a478255

[github-actions]

GPT 5.6 Review — 🔴 changes requested (blocking)

GPT 5.6 found at least one blocking issue that must be resolved before merging a4782554fb8d3f23977d8ea46ddbd8c6fe034fea. Adjudication emitted 0 well-formed verdict line(s) for total=1, so GPT's verdict stands.

This comment is updated in place on each push.

BLOCKING -- src/kiro_crew/security.py:6475 -- Case-pattern parentheses prematurely close process substitutions

if ch == ")" and depth > 0:
Valid case ... pattern) -> splitter cuts at ;; -> trailing main is uninspected -> protected push is allowed.
Anchor: backend-security-controls
Fix: Distinguish case-pattern terminators from construct-closing parentheses in both process-substitution walks.
[BLOCK-MERGE] a478255
[GPT-REVIEWED] a478255

Adjudication (Opus 4.8) — is blocking on each finding proportionate?

F1 opens at security.py:6475 and the splitter it belongs to feeds _push_segment_targets_protected (security.py:7523), the fail-closed gate that blocks agent pushes to protected branches. A case pat) terminator inside >(...) decrements depth at line 6475 without the process substitution actually closing, so a trailing main refspec splits into an uninspected segment and a protected-branch push slips the gate.

Harm rung: UNBOUNDED — this is a bypass of the git-push protected-branch security control (governance/security ceiling), which the ladder answers without weighing. The remedy cost (distinguishing case-pattern terminators from construct-closing parens in the two process-substitution walks) is bounded, but for a security-class bypass I hold no authority to downgrade regardless.

Conditions confirmed: splitter decrements on any ) at depth>0 (security.py:6475); no case-statement grammar awareness anywhere in the walk (lines 6455-6479); result flows to the protected-push tagger (security.py:7523). Recovery path: none in-band — a mis-split segment silently drops the protected refspec from inspection.

[ADJUDICATION] a4782554fb8d3f23977d8ea46ddbd8c6fe034fea total=1 uphold=1 downgrade=0
UPHOLD F1 src/kiro_crew/security.py:6475 reason=security-class
[GPT-ADJUDICATED] a4782554fb8d3f23977d8ea46ddbd8c6fe034fea

False positive or not applicable? A repository writer can comment:
/ai-review override gpt a4782554fb8d3f23977d8ea46ddbd8c6fe034fea: <one-sentence reason>

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed a4782554fb8d3f23977d8ea46ddbd8c6fe034fea — this comment is updated in place on each push.

Review details

The discovery pass produced no candidates, and my independent trace confirms the compensating controls:

• Removing [<>]\( from _AMBIGUOUS_EXPANSION_RE is compensated word-side by the if "<(" in raw or ">(" in raw: ungating in _push_segment_targets_protected, and redirect-target process-sub bodies (> >(git push origin main)) remain scanned via _substitution_bodies/_shell_payload_sources (verified at security.py:4669) so the protected push in the body is still denied.
• _split_push_segments, _paren_construct_end, the <<- regex addition, the raw-word return, the subshell-closer tail, the residue-hazard deferral, and the glued-anchor escalation each resolve to fail-closed or more-protective behavior, corroborated by the dedicated helper unit tests.

I could ground no concrete false-negative, crash, or data-loss defect on the changed lines to the (a)/(b)/(c) bar at 80+.

No findings.

[OPUS-REVIEWED] a478255

Verdict parsed from the review's SHA-scoped output markers for commit a4782554fb8d3f23977d8ea46ddbd8c6fe034fea.

False positive or not applicable? A repository writer can comment:
/ai-review override fable a4782554fb8d3f23977d8ea46ddbd8c6fe034fea: <one-sentence reason>

[iamwhatever]

• Process-substitution separators hide trailing protected refs (span=3c5b15a2d41e) — disposition: fixed in b821e4d.

_split_push_segments now tracks process-substitution nesting: an unquoted <(/>( opens a depth counter and separators split only at depth 0, so the ; inside the construct stays part of its command list and the trailing protected ref remains in the judged segment.

Verified: the reported command is now a single segment, the extractor steps over the whole construct via the quote-aware paren walk, and the trailing ref reports git-publish-push-protected-branch-name. Regression test test_a_separator_inside_a_process_substitution_is_not_a_boundary pins both the segmentation and the tag.

[iamwhatever]

• Multiline payload deferral skips a sibling all-branches push (span=3c5b15a2d41e) — disposition: fixed in b821e4d.

_push_top_level_residue_hazard no longer relies on a token blocklist alone: it blanks every quoted span and hands the remaining TOP-LEVEL view to _is_git_publish, so any publish material outside the quotes — an all-branches flag, a second git word, a bare push — makes the deferral unsafe and the segment keeps the ungated deny. Deny-by-default over the publish detector rather than an enumerated hazard list.

Verified: the reported command now returns the ungated sentinel; test_a_deferral_cannot_vouch_for_a_sibling_top_level_push pins --all, ambiguous-ref, and bare-push residue shapes plus the still-allowed clean multi-line wrapper.

[iamwhatever]

• The newline-deferral branch lets a top-level protected/ambiguous/bare/mirror push ride a benign quoted-newline wrapper (span=c9886dc1579f) — disposition: fixed in b821e4d.

Both deferral branches (the quoted-newline branch and the args-None branch) share _push_top_level_residue_hazard, which now blanks every quoted span and runs _is_git_publish over the remaining top-level view — the full deny vocabulary the detector already carries (ambiguous refs, wildcards, all-branches flags, the bare-push shape, a second git word), rather than the $/~/protected-name blocklist the review correctly called incomplete. Any publish material at top level keeps the segment on the ungated deny.

Verified: bash -c '<feature push with quoted newline>' & git push origin head returns the ungated sentinel, as do the --all and bare-push variants; the clean multi-line wrapper still defers and its payload is judged by the nested-payload walk. Pinned in test_a_deferral_cannot_vouch_for_a_sibling_top_level_push.

[iamwhatever]

• Nested parentheses prematurely end process substitutions (span=3c5b15a2d41e) — disposition: fixed in be63963.

The splitter's depth counter now increments on a plain unquoted ( while inside a process substitution, so a nested subshell in the construct's command list no longer lets the first ) close it early. This matches _paren_construct_end, which already counts all unquoted parens.

Verified: the reported command stays one segment and its trailing protected ref reports git-publish-push-protected-branch-name; pinned in test_a_separator_inside_a_process_substitution_is_not_a_boundary.

[iamwhatever]

• Empty quote glue hides sibling pushes (span=3c5b15a2d41e) — disposition: fixed in be63963.

Character-blind blanking is gone. The new _push_top_level_view resolves quoted spans the way bash builds argv words: a span without whitespace joins the surrounding word (g""it → git), so quote glue can no longer break the program word apart before the _is_git_publish shape check; a span with whitespace (a -c payload) blanks to one space, preserving the wrapper deferral.

Verified: the reported glued spelling now returns the ungated sentinel; pinned in test_a_deferral_cannot_vouch_for_a_sibling_top_level_push.

[iamwhatever]

• Quoting the program word ('git') evades the new top-level residue check (span=c9886dc1579f) — disposition: fixed in be63963.

Exactly the fix the review proposed: _push_top_level_view de-quotes a whitespace-free quoted span into the surrounding word ('git' → git, "git" → git, and the glued g""it from the sibling GPT finding) before the _is_git_publish shape check, while a quoted span containing whitespace — the multi-word -c payload whose reading the deferral hands to the payload walk — still blanks. Both deferral branches share the hazard, so the --all, bare, and head sibling spellings are all covered.

Verified: bash -c '<feature push with quoted newline>' & 'git' push --all origin and the 'git' push origin head variant return the ungated sentinel; the clean multi-line wrapper still defers. Pinned in test_a_deferral_cannot_vouch_for_a_sibling_top_level_push.

[iamwhatever]

• $'git' $'push' --all survives multiline-wrapper deferral undetected because the top-level view retains $ (span=3c5b15a2d41e) — disposition: fixed in 023b7ff.

Advisory, but a real allow-direction gap in the deferral seam, so fixed rather than deferred: _push_top_level_view now drops the $ that introduces an ANSI-C span (bash resolves $'git' to git, the $ is part of the quoting construct), so the resolved program word reaches the _is_git_publish shape check. An ANSI span carrying a backslash escape ($'g\x69t') is not decoded — it is statically unresolvable, so _push_token_expands now reports it as a hazard and the deferral fails closed instead.

Verified: the reported spelling returns the ungated sentinel; pinned in test_a_deferral_cannot_vouch_for_a_sibling_top_level_push plus direct unit pins on _push_top_level_view and _push_token_expands.

[iamwhatever]

• Wildcard executable bypasses sibling-push residue guard (span=3c5b15a2d41e) — disposition: fixed in a478255.

_push_token_expands now flags unquoted glob characters (* ? [) and extglob openers (@( +( !( — ?(/*( are covered by their leading glob char), closing the class rather than the instance: the hazard set now covers the same expansion inventory the positional walk itself models ($, leading ~, ANSI-C escapes, globs, extglob), so the residue guard and the walk can no longer disagree about what bash can expand into a program word. A quoted glob stays data, matching bash.

Verified: bash -c '<feature push with quoted newline>' & /usr/bin/g?t push --all origin returns the ungated sentinel; direct unit pins on /usr/bin/g?t, @(git), and the quoted 'g?t' negative are in test_a_deferral_cannot_vouch_for_a_sibling_top_level_push.

[iamwhatever]

🤖 Kiro Crew Auto-Pipeline [operator: iamwhatever]

Closing this PR: superseded by #8712 (merged 2026-09-05T12:27Z, commit c791f0f1d, authored by @pepmach7 — thank you, Stan, for landing the root-cause fix).

Verified on pristine main @ 369bb6d0b:

• test/test_push_branch_gate.py — 127 passed (was 6 failed)
• test/test_security.py -k TestGitPublishSubshellGluing — 15 passed (was 3 failed)

All 9 originally-failing tests are green on main, so this branch's reason to exist (unblocking the red main) is resolved. Rather than rebase a now-conflicting parallel rewrite of the same tokenizer onto the landed fix, standing down is the lower-risk path.

One pointer for the record: GPT 5.6's final blocking finding here (span=3c5b15a2d41e, case-pattern ) prematurely closing a process-substitution walk) was against this branch's rewritten splitter, and this PR's review rounds surfaced several bash-grammar productions (process substitution nesting, quoted parens, ANSI-C quoting, case patterns) that any segment walk must model. That corpus is relevant input to #7832 (consolidating the git-publish scan onto a bash-faithful word lexer), which remains the architectural successor for this area.