fix(security): stop re-asking a command-level guard per payload

[chenmingwei23]

Problem / Motivation

is_denied is quadratic in the number of nested shell payloads a command
carries, so a large-but-ordinary-looking command makes the deny gate run for
minutes.

Measured on current main, spaced repro from #8595 (bash -c a0pay -c a1pay ...,
18,000 pairs, a 222,894-byte command):

payloads	command bytes	is_denied on main
4,000	46,894	10.63s
18,000	222,894	209.19s

Doubling the payload count multiplies the time by ~3.9, not ~2: ratios
1.99 / 3.41 / 3.79 / 3.87 across 250 -> 500 -> 1k -> 2k -> 4k payloads, a
fitted growth exponent of 1.95. The reporter measured ~293s at 18k on their
own workstation; this is the same curve on a different machine.

The extractor is not the bottleneck and never was: _nested_shell_payloads is
linear on the same inputs (ratios 1.92 / 2.02 / 2.03 / 2.02, 0.166s at 18k).

Why it matters

The gate is what decides whether a command may run, and a command string is
attacker-influenced input. Extrapolating the measured fit, ~6,300 payloads
(about 74KB of command text) is enough to cross a 25s-class watchdog on this
host. When the watchdog fires first, the outcome is not a slow decision but an
interrupted one, so this is availability of the deny path rather than mere
latency.

It is reachable on main today through the spaced spelling, and the glued
spelling reaches the same code path since #8491 merged (glued: 10.22s at 18k).

What changed (motivation -> approach -> change)

Symptom -> curve. Timed the real is_denied over 250 to 32,000 payloads
rather than trusting the word "super-linear". It is quadratic, not exponential,
which decides what kind of fix is needed.

Curve -> root cause, and this is where the issue's own diagnosis is wrong.
#8595 attributes the cost to "N payloads x M rules x payload length". That
product is linear in N, because each payload is about ten bytes, so it cannot
produce an exponent of 1.95. Profiling at 2,000 payloads (6.18s total) locates
the real term:

• _deny_pattern_matches, the per-payload x per-rule scan that all three
  suggested mitigations aim at: 282,282 calls, 0.477s, 7.7% of the time.
• _data_consumer_exempt, called once per payload: 4.912s, 79%. Inside it,
  any(_SCRIPT_EXECUTES_RE.search(tok) for tok in tokens) runs 8,004,000
  iterations, which is exactly 2000 x 4002 = payloads x len(tokens).

_data_consumer_exempt has three guards that read only tokens, and tokens
is bound once by the caller, outside the payload loop, as is programs. So a
command-level question was being re-answered once per payload. Recovering each
payload's token positions with [i for i, tok in enumerate(tokens) if tok == payload] walked the same argv a second time, once per payload.

Root cause -> change. Both values are now computed once per command and
passed into the loop:

• the three command-level guards move into _data_consumer_command_disqualified,
  and _data_consumer_exempt takes an optional command_disqualified;
• the payload's token positions come from a token_positions index built once.

Neither can change a verdict. Each hoisted guard is a pure function of tokens
and each one refuses the exemption, so hoisting alters how often the same
answer is computed, never what it is. token_positions.get(payload, []) returns
the same ascending index list the comprehension produced. Callers that ask about
a single token pass nothing and keep the old self-computing path, so the three
call sites this PR does not touch are unchanged. A command carrying no nested
payload skips both, so ordinary commands pay nothing new.

Deviation from the issue's suggested directions, stated up front. #8595
proposes dedup, a cap with fail-closed, or precompiled/aggregated rules, and two
operators routed the issue to needs-investigation on the grounds that choosing
between them is a contract decision. That reading of those three options is
correct, and it is why none of them is implemented here: they all target the
7.7% term. Specifically, dedup cannot help this shape at all, since a0pay to
a17999pay are already distinct; and a cap or a rule-scan rewrite would each
pick a policy where _AltWorkBudget's own docstring records three revisions
failing open. Hoisting a loop-invariant needs no cap, no dedup, no budget
threaded through the extraction sites and no contract choice. If maintainers
still want a cap as defence-in-depth for other amplification axes, that is
separable from this fix and unaffected by it.

Both halves are load-bearing, measured rather than assumed. Reverting only
the position index, keeping the guard hoist, leaves the walk quadratic: 1.37s /
4.30s / 15.56s at 4k / 8k / 16k payloads, ratios 3.15 and 3.62. So this is one
defect with two expressions at one call site, not a fix plus a tidy-up.

Tests

Cost is pinned structurally, never on elapsed time. A wall-clock bound would
claim a performance budget for every other pass in the gate and would flake on a
slower runner, so what the tests assert is the bounded quantity.

• test_guard_is_charged_once_however_many_payloads -- the command-level guard
  is charged the same number of times at 30, 60 and 120 payloads.
• test_argv_sweep_is_linear_in_the_argv_not_quadratic_in_payloads -- the
  _SCRIPT_EXECUTES_RE argv sweep count stays within 3x the argv length.
  Measured before: 1,830 / 7,260 / 28,920 sweeps at 30 / 60 / 120 payloads
  (ratio ~3.98). After: 61 / 121 / 241 (ratio ~1.99).
• test_argv_is_walked_per_command_not_per_payload -- argv elements consumed
  stay within 40x the argv length. Before: 2,984 / 9,554 / 33,494 / 124,574 at
  30 / 60 / 120 / 240 payloads (ratio ~3.72). After: 1,154 / 2,294 / 4,574 /
  9,134 (ratio ~2.00).
• test_every_way_the_exemption_is_refused_still_refuses and
  test_the_ordinary_data_consumer_is_still_exempt -- every documented route by
  which the exemption is refused, and the ordinary exempt case, in both
  directions, so the change can neither widen nor narrow the exemption.
• test_precomputed_and_self_computed_guards_agree -- the None branch used by
  the untouched callers gives the same answer as the hoisted value, token by
  token.

Before and after, is_denied, measured not extrapolated:

payloads	spelling	before	after
18,000	spaced	209.19s	2.55s
18,000	glued	10.22s	2.33s

Growth is now linear. Doubling ratios over 4k -> 8k -> 16k -> 32k payloads are
1.89 / 2.04 / 2.06, a fitted exponent of 1.04, against 1.95 before. At
32,000 payloads (a 404,894-byte command) the gate takes 4.51s.

Suite: pytest -n0 test/test_denied_commands_security.py -> 695 passed.
flake8, isort --check-only, mypy src/kiro_crew/ (1302 files) and the black
gate all clean.

Five mutations, each hand-applied and asserted to have applied, each reddening a
different observable with a different assertion:

1. remove the hoisted guard argument -> guard counts become 31 / 61 / 121, and
   sweeps become 1,891 against a bound of 183.
2. restore the enumerate position scan -> 2,984 elements against a bound of
   2,440.
3. guard always reports "not disqualified" -> echo <name> <verb> | sh is
   allowed when it must be denied.
4. guard always reports "disqualified" -> echo <name> <verb> is denied when it
   must be allowed.
5. None branch stops self-computing -> the agreement test reports
   self-computed=True passed-in=False, and a refusal case flips to allowed.

Mutations 3 and 4 fail in opposite directions on the same guard, which is what
rules out a test that reddens indiscriminately.

Manual verification

Deny-set identity was proven by running the same matrix under main and under
this branch and diffing the results, rather than arguing it from the diff: 79
commands x 3 rule tiers, byte-identical verdicts, 50 of the 79 denials. The
matrix covers destructive and credential and publish shapes, nested payloads
behind six different launchers, every documented refusal route for the
data-consumer exemption, ordinary allowed commands, and the #8595 repro shape
with a real denial embedded at several positions. Controls: the comparison
detects a deliberate one-character change, and the two outputs are non-empty
(22,528 bytes each), so an empty-vs-empty comparison cannot pass for agreement.

All measurements were taken in a detached worktree at a pinned main with none of
this branch's code present, with the imported module's path asserted, so the
before numbers are attributable to main alone.

Related Issues

Closes #8595

Context, not changed here: #8197 and #8491 are the glued-payload extraction work
this issue was found alongside; #8491 is merged, and the 2x2 in #8595 correctly
attributes the cost to main rather than to that PR. #8338 and #8282 concern cost
in command length, a different axis that this change does not address.

Pattern harvest

Rule candidate: review-prompt

Pattern: a predicate that reads only a loop-invariant collection, evaluated
inside a loop over that same collection, turns an O(n) helper into O(n^2) at the
call site. The tell here was a guard taking both an index and the whole argv,
where two of its three checks never used the index.

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) — ✅ PASS

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

Design-Verdict: PASS

A measured quadratic on attacker-influenced input in the deny gate, fixed at its actual root by a semantics-preserving loop-invariant hoist, with verdict identity and cost shape both pinned structurally.

[DESIGN-REVIEWED] e56fd3e

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

The refactor is behavior-preserving. The three command-level guards moved into _data_consumer_command_disqualified return True exactly where the originals returned False, and the composed logic in _data_consumer_exempt (early per-token guards index <= 0 and _CONTROL_OPERATOR_RE, then the disqualification check, then the _DATA_CONSUMER_PROGRAMS membership) is unchanged. tokens is bound once per while pending iteration and never reassigned inside the for payload loop, so hoisting command_disqualified and token_positions before that loop is sound. token_positions.get(payload, []) reproduces the enumerate comprehension exactly (ascending positions, string keys), and both hoists are skipped when payloads is empty so the False fallback is never consumed. No divergence, no weakened denial.

No findings.

[OPUS-REVIEWED] e56fd3e

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

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

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of e56fd3edcce34e5ca776a6e6c7c1e9e4a0fbf92e and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] e56fd3e

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of e56fd3edcce34e5ca776a6e6c7c1e9e4a0fbf92e — 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.

All verification is done. The fix targets the payload loop in _deny_segment_views; I confirmed the three untouched _data_consumer_exempt call sites (security.py:5553, 5990, 6403) each sit inside their own per-token loop over the same argv and keep the self-computing branch, which is the one depth finding worth surfacing. Everything else in the diff is declared, measured, and cause-level.

First-Principles-Verdict: CONCERNS

The reported quadratic is fixed at its cause, but the same recompute-per-iteration pattern survives, counted, at three sibling call sites the description frames as untouched by design.

What this change ships

Intent: keep the deny gate able to decide a payload-stuffed command in seconds so the security decision completes instead of being watchdog-killed. FIX.

1. Deny gate answers an 18k-payload command in ~2.5s, not ~209s — justified, cause-level (security: is_denied is super-linear in nested payload count — ~293s on main for an 18k-payload spaced multi -c command #8595)
2. Command-level exemption guards computed once per command (new internal helper) — justified; 2 consumers counted (security.py:4027, 18754)
3. _data_consumer_exempt accepts a precomputed command_disqualified; absent means self-compute — justified; 1 passer, 3 callers on the default
4. Payload positions read from a once-built index, not a per-payload argv scan — justified, measured load-bearing
5. Complexity pinned by structural counts, not wall-clock — justified; mandated by testing-conventions § Determinism

Watch

• Grep _data_consumer_exempt( in src/: 4 call sites; the 3 unfixed ones (security.py:5553, 5990, 6403) each call it inside a for i, token in enumerate(tokens) loop, and the default branch re-runs the O(argv) _SCRIPT_EXECUTES_RE sweep per triggering token — so N trigger tokens (e.g. pkill kirocrew repeated) rebuilds the quadratic those floors were already once de-quadratified for (see the per-frame scan hoist at security.py:6397). The description's "callers that ask about a single token" understates this: each call is single-token, each caller loops. Deferring is defensible — an unconditional per-frame hoist would charge ordinary commands a sweep they currently skip — but it is the same root cause behind a narrower gate, left unfixed at 3 counted sites.

[FIRST-PRINCIPLES-REVIEWED] e56fd3e

[chenmingwei23]

Conceding the First Principles finding on the wording, and filing the substance rather than widening this PR. Putting it in a comment because the description is otherwise accurate and editing it would re-roll three clean review verdicts on this head.

The wording was understated and the reviewer is right. The description says "callers that ask about a single token pass nothing and keep the old self-computing path". Each call is single-token; each caller loops. Verified by reading all three enclosing loops at main d4c2cbf:

site (main-relative)	enclosing loop	trigger predicate
security.py:5520	:5495 frames, :5497 for i, token in enumerate(tokens)	_is_self_program / _is_self_module_invocation
security.py:5957	:5951 frames, :5953 for i, token in enumerate(tokens)	_is_kill_by_name_program
security.py:6370	:6360 frames, :6365 for i in range(len(tokens))	_self_program_index not None

It is real and reachable, and it is not this PR's. Measured both columns, because a claim that something is or is not this change's effect is a differential one:

trigger tokens	argv sweeps on main	argv sweeps on this branch	seconds on main
200	481,200	481,200	0.54
400	1,922,400	1,922,400	1.04
800	7,684,800	7,684,800	4.20
1,600	30,729,600	30,729,600	13.78

Sweeps multiply by exactly 4.00 per doubling, and are identical on both trees -- this diff moves that axis by zero, in either direction. Roughly 2,200 trigger tokens (~33KB) crosses a 25s watchdog. Measured on a clean-main worktree with this branch's changes absent.

Filed as #8972 with that evidence, the in-module precedent at security.py:6362 ("Once per FRAME, not once per token: this is the loop whose per-token scan made the floor quadratic"), and the tradeoff the reviewer named: the guards are reached only after a narrow trigger predicate matches, so an unconditional per-frame hoist would charge every command reaching those floors one argv sweep it currently skips. That is a judgement about the common case and wants its own measurement, which is why it is a separate change rather than three more edited security-critical call sites here.

The optional command_disqualified parameter this PR adds is the seam that fix will use, so nothing here needs to change for it to land.

[bolichen97]

Approved after an independent verdict-identity check rather than from the description.

Not a cache, and nothing payload-specific is skipped. The three guards moved into _data_consumer_command_disqualified read only tokens — I read each body and none touches index or token. The genuinely per-payload guards (index <= 0, _CONTROL_OPERATOR_RE.search(token)) stay inside the loop and are still charged per payload. tokens is bound once at _deny_segment_views (tokens = _shell_tokens(source)) and is never rebound or mutated anywhere in the for payload body, so the hoisted boolean and token_positions are computed from the same object every call then reads. token_positions.get(payload, []) reproduces the enumerate comprehension exactly, including the empty result for a synthesized payload that is not a token — which still fails closed by descending.

Failure direction is safe. A hoisted True refuses the exemption and the payload is descended into (over-block). The bypass direction needs the hoisted value to be False where a per-payload evaluation would be True, which a pure function of an unchanged input cannot produce. The three untouched call sites pass no keyword and take the None self-compute branch, so they are byte-identical to main.

Verdict identity, measured not argued. 970 commands — every launcher x body pair, all six data-consumer programs with and without a piped evaluator, every documented refusal route, the #8595 repro at 3/12/40 payloads with a real denial planted at three positions each, nested -c, glued herestring, env -S, sed e-flag, line continuations, and the quote/empty-element re-spellings — run under the merge base 8aef8fe3f and under e56fd3edc in separate detached worktrees with each module's own path asserted. Byte-identical output, 326 denials / 644 allows so it is not a degenerate comparison, and the diff provably reddens when a single verdict is flipped in either direction.

The fix is real. Same harness, spaced repro: main 0.437 / 1.264 / 4.671s at 500 / 1k / 2k payloads (ratios 2.89, 3.70); this branch 0.171 / 0.226 / 0.465s (ratios 1.32, 2.06). New memory is one dict bounded by len(tokens), so it adds no amplification axis of its own.

AGENTS.md security invariants untouched: no rule records, no restated rule count, no sensitive-path matcher change, no gate relocation. First Principles CONCERNS is advisory and about the pattern surviving at three sibling call sites, which is correctly filed separately rather than widening this diff.