fix: model parameter expansion and comments in shell char walk

[aniruddhaadak80]

Problem / Motivation

_iter_shell_chars (src/kiro_crew/security/shell_normalizer.py) models shell quote and
escape handling, but did not handle two additional bash syntax categories:

• ${...} parameter expansion
• # comments

A literal ) inside $(echo ${v:-}) or after a # comment arrived with
active=True, so _matching_close_paren counted it as the substitution
closer and truncated the extracted body. The nested-payload extractor then
handed every downstream scan a fragment while bash runs the whole body, so a
nested publish hides below the truncation point -- is_denied allows a command
bash executes (same shape as the documented quoted-paren truncation behind
_matching_close_paren).

Why it matters

The _iter_shell_chars generator is the single source of truth for shell
quote/escape state walked by every consumer: the boundary walk, the git-publish
border walk, the nested-payload extractor, and is_denied/is_sensitive_bash_command.
When the machine did not know about ${...} or #, those constructs were
invisible to it, and a ) inside them was treated as a bare ) -- a
false-allow of the class security reviews have caught before.

What changed (motivation → approach → change)

Extended _iter_shell_chars with two new syntax branches:

1. ${...} parameter expansion: when $ is followed by { in the
   unquoted state (state == 0, unpaired $), the generator enters an
   expansion closed by the matching }. Interior steps yield active=False
   so _matching_close_paren ignores them, while quote state runs through
   the span per POSIX 2.6.2 -- only an unquoted, unescaped } decrements
   brace depth, so a quoted } (${v:-"}"}, ${v:-'}X)Y'}) cannot close
   early and desync the outer walk. Tracked quotes stay local to the span.
   Nested $(...) inside still resolves via the raw-text descent in
   _substitution_bodies.

2. # comments: a # at word start (bash: echo a#b is one word) starts
   a comment running to the newline; interior steps yield active=False, and a
   backslash-newline continues the comment as in bash.

Tests

• test/test_security.py: test_substitution_span_survives_expansion_and_comment_parens
  and the new test_expansion_close_ignores_quoted_braces (pins the two
  reviewer repros) pass
• test/test_push_branch_gate.py::test_every_bash_metacharacter_is_accounted_for
  passes -- ${ and # keep their existing accounting rows, so no spec move
• black --target-version py310, flake8, isort: clean on both touched files;
  mypy on shell_normalizer.py: clean

Pattern harvest

Not generalizable: one-off bash-fidelity gap in a single state machine plus its
regression pins; the review lesson (never freeze quote state across a span you
close by delimiter) is already stated in the code comment.

Related Issues

Fixes #9181. Note: #8830 (case-pattern terminators) is intentionally not cited --
that is #9164's scope, already merged.

Checklist

• At most 2 commits with Conventional Commits titles
• Regression tests added and passing; spec accounting verified unchanged
• Self-review completed; code follows project style guidelines
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of 7173f920e16e743b53d3b3cec72e1abe00185811 via the fork AI-review pipeline — 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.

I've now verified the base state machine (_iter_shell_chars at shell_normalizer.py:1482 has no ${ or # branch on base, so the defect is real), the accounting-inventory test the description cites (test_push_branch_gate.py:1265, with existing ${ and # rows at lines 1287 and 1302), checked for existing mechanisms (${VAR:-literal} handling at line 429 does a different job — literal classification, not span protection), and counted siblings of the root cause (walker constructs whose interior is data: quotes and ANSI-C handled on base, ${...} and # fixed here, heredoc bodies not modeled anywhere in the char walk — only token-level _heredoc_marker at line 1211).

First-Principles-Verdict: CONCERNS

Harvest claims "not generalizable," but heredoc bodies are a third data-interior construct the walker still truncates on — same root cause, left uncounted.

What this change ships

Intent: stop is_denied allowing commands bash runs because a literal ) inside ${...} or a # comment truncated the extracted substitution body — a FIX (linked issue #9181, repros pinned by tests added here).

Inventory (4 items)

1. A ) inside ${...} no longer truncates the substitution body handed to deny scans — justified
2. A ) inside a # comment no longer truncates; comments now run to the newline for every consumer — justified
3. A quoted } inside ${...} reads as literal, so the expansion cannot close early and desync the walk — justified
4. Two regression tests pin the reviewer repros — justified

Watch

• The root cause is "_iter_shell_chars doesn't model constructs whose interior is data"; this fixes two instances, but heredoc bodies are an unfixed sibling (count: 1 — no << branch exists in the walker, only token-level _heredoc_marker at shell_normalizer.py:1211). A ) on its own line inside $(cat <<E … E) closes the span "proven" and hides everything below it — the same allow-direction shape as security: _iter_shell_chars does not model parameter expansion or comments, so a literal ) inside them truncates every substitution span #9181, while the description's harvest section says nothing is left.
  Clears when: the heredoc-body case is shown covered by an existing layer (a test on base) or filed as a linked issue.

[FIRST-PRINCIPLES-REVIEWED] 7173f92

[github-actions]

GPT 5.6 Review (fork) — ⚠️ review incomplete

Reviewed accef9afaf9706b156602a38e3e1050bc92df940 via the fork AI-review pipeline; updated in place on each push.

No completed GPT verdict for this commit; see the Fork GPT 5.6 Review job logs.

[github-actions]

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of 7173f920e16e743b53d3b3cec72e1abe00185811 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I have what I need. I traced the new ${...} interior loop against the POSIX rule the PR itself cites, and found a concrete gap worth flagging.

Design-Verdict: CONCERNS

The ${...} interior tracker skips quoted strings but not command substitutions, so ${v:-$(echo })} re-opens the truncation class — and regresses an input base handled.

Watch

• POSIX 2.6.2, quoted in the diff, says the matching } is found "skipping over enclosed quoted strings, and command substitutions"; the interior loop implements only the first half. In kill $(echo ${v:-$(echo })}; git push origin main), the loop closes the expansion at the } inside $(echo }), the following ) comes back active, _matching_close_paren truncates the outer body before the ;, and is_denied never scans the nested publish bash executes. Pre-PR this input extracted correctly (the embedded $() self-balanced under plain paren counting), so the hunk's claim "errs toward scanning MORE, never less" is false for exactly this shape — it's a new fail-open, not fail-closed. Backticks inside ${} miss the same way.
  Clears when: the interior loop skips embedded command substitutions ($( … ) and backticks) when matching }, with a regression pin for ${v:-$(echo })} showing the full body extracted.

Suggestions

• The interior loop is a second, simplified quote tracker embedded inside "THE shell quote/escape state machine" whose own docstring names two-scanners drift as the defect it was cured of twice — the $() miss is that drift; prefer routing the }-match through the shared machinery (recursive descent, as _matching_close_paren does for parens) over growing the local tracker.

[DESIGN-REVIEWED] 7173f92

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 7173f920e16e743b53d3b3cec72e1abe00185811 via the fork AI-review pipeline; updated in place on each push.

Review details

I've completed my falsification analysis. Let me summarize the key verification I did before finalizing.

The candidate's core premise — that bash closes ${v:-a{b} at the first } because a bare { doesn't nest (only ${ does, per extract_dollar_brace_string) — is correct, and the diff's elif qstate == 0 and c == "{": depth += 1 does over-extend the span past bash's real close point. That divergence is real.

But the candidate's claimed observable wrong outcome (an allow-direction miss) does not hold up. I traced every consumer of _iter_shell_chars:

• git-publish (_git_publish_floor_tags → _split_push_command_segments): the over-extended segment still contains ${, so _AMBIGUOUS_EXPANSION_RE.search(command) matches → _GIT_PUBLISH_UNGATED deny; and if no clean push segment survives, if not saw_push also denies. Over-blocks — the safe direction. (Candidate conceded this.)
• self-kill, both families: detection runs through _self_tokens/_shell_payload_walk, which use shlex.split(posix=True) — independent of _iter_shell_chars. echo ${v:-a{b}; kill $(pgrep -f kirocrew) tokenizes to a clean kill token whose own argv _substitution_bodies scans and matches kirocrew; pkill is caught via _argv_programs/_ends_argv on the trailing ;. The candidate's assertion that "the self-kill path has no backstop" is false.
• is_denied: Pass 1 is a whole-string substring/regex match; Pass 2 splitting is purely textual (_CMD_SEPARATOR_RE), not _iter_shell_chars. Unaffected.
• _substitution_bodies/_matching_close_paren: an over-extended span never balances → returns unproven → yields the whole remainder → scans more. Fail-closed.
• The "cd-tracking" consumer the candidate cites (_split_shell_segments) does not exist as a live function — only referenced in a docstring.

So the divergence at most causes over-blocking (the diff's stated, safe "scan MORE" direction) and is backstopped everywhere against an actual allow. I could not re-derive a concrete allow-direction failure at 80+ without a contrived, unverifiable combination — which the falsification bar forbids. No new grounded defect surfaced in the diff either.

No findings.

[OPUS-REVIEWED] 7173f92

[bolichen97]

@aniruddhaadak80 Thanks for staying on this. Notes from a repository-wide audit of open PRs, audited at 68fd3a5. Your branch has since moved to accef9a, so please treat anything already fixed as resolved.

Already on main: merged #9183 deleted src/kiro_crew/security.py and split it into the src/kiro_crew/security/ package, moving _iter_shell_chars to src/kiro_crew/security/shell_normalizer.py. Your current head already targets that file and adds test/test_security.py, which clears the conflict and the missing-test gap we recorded.

Still missing on main: _iter_shell_chars there has only the backslash, single-quote, ANSI-C and double-quote branches, with no ${...} and no comment handling. The gap you describe is real and open, and that remaining scope is the value of this PR.

Two defects we found in 68fd3a5, please confirm they are addressed in the new revision:

1. The ${...} branch passed True as the seventh _ShellChar field, trailing_escape, and left active true. A ) inside ${v:-)} was therefore still counted by _matching_close_paren, so the stated fix did not take effect.
2. The comment branch tested only state == 0 and ch == '#', not word start. An unquoted mid-word #, which bash prints literally in echo a#b, ended the generator and hid the rest of the command. That is an allow-direction truncation.

One description fix: #8830 is the case-pattern terminator, which #9164 addresses and this diff does not. #9164 edits the same walk, so please coordinate with it before rebasing.

Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong.

[aniruddhaadak80]

All blocking findings addressed in the latest push (rebased onto current main, 2 commits total):

• Design + Opus (quoted brace closes early): the ${...} interior loop now tracks single/double quotes and backslash escapes through the span and decrements brace depth only on an unquoted, unescaped }. Tracked quotes stay local to the span. New test_expansion_close_ignores_quoted_braces pins both repros, and both tests pass.
• Audit defect 1 (7th-field True): already resolved in accef9a -- interior yields active=False.
• Audit defect 2 (mid-word #): already resolved in accef9a via at_word_start (pinned by test).
• security: _matching_close_paren treats a case-pattern ) as the substitution closer, truncating every consumer's body #8830 citation: removed from the body; Related is now security: _iter_shell_chars does not model parameter expansion or comments, so a literal ) inside them truncates every substitution span #9181. fix(security): teach the span walk bash case-pattern paren terminators (#8830) #9164 is merged, no coordination outstanding.
• Spec accounting: test_every_bash_metacharacter_is_accounted_for already rows ${ and # at their layers and passes unchanged -- no layer move, no spec edit.
• Hygiene/lint: body now carries ## Pattern harvest; black/flake8/isort/mypy clean on both touched files.