fix: scope the separator-run collapse to shell-grammar subjects

[rnoack1]

Problem / Motivation

mcp_cron._vet_script_contents scans a cron script's body — Python source — with
security.is_sensitive_bash_command. Pass 1b of that function collapses separator
runs, which is right for a Win32 shell string and wrong for source code: in source
a backslash run is an escape. \\ is one backslash, \. a literal dot, so the
collapse strips the escape and manufactures a path the body never contained.

A body that only redacts or names a fenced store is refused as though it read
one. Three bodies that reproduce it, each with zero hits on the unmodified text —
only a collapsed copy matches, and none of them reads anything:

import re
SCRUB = re.compile(r"%LOCALAPPDATA%\\kiro-cli")                              # redaction pattern
SCRUB = re.compile(r"/home/\S*/\.kiro/crew/security_policy.json")            # regex -> literal path
def run(ctx):
    """Never touch %LOCALAPPDATA%\\kiro-cli -- it is the keystone."""   # docstring: STILL refused

Fixes #7912

Why it matters

The denial is permanent in practice. The fire-time gate deliberately keeps the job
and does not feed the auto-pause counter, so an affected script is refused on every
tick until someone finds and edits the line. The message names a credential path,
pointing the reader at an access that does not exist. The third body is a docstring, and it is the one case this PR does NOT clear: see the accepted over-block below.

It also fires hardest on the code most likely to mention a credential path on purpose:
a redaction helper written to keep a jar path out of its own log output is the most
likely thing to be refused.

What changed (motivation → approach → change)

Motivation. A shell-grammar heuristic is being applied to a subject that is not
shell grammar.

Approach. Scope the heuristic to its subject, then replace it for that subject
rather than simply dropping it. Scoping alone would reopen #6350 inside a script body:
the separator run still exists in the DECODED literal, so open(r"…\\kiro-cli\\c.json")
hands the OS two backslashes that Win32 collapses at open time, while the raw source
text matches no fence pattern. Measured on this branch, four attack bodies blocked on
base were missed with the skip alone.

Scoping is still right for the shell matcher — this is the posture the function
already takes one layer out, and says why:

We deliberately do NOT run is_denied over a script body: it encodes shell
tool-name semantics (e.g. *git*push*) that false-positive on ordinary Python source

But a source body needs an escape-aware counterpart, and a transform of the literal
VALUE cannot supply it: a regex escape and a path separator are the same character once
decoded, so re.compile(r"%LOCALAPPDATA%\\kiro-cli") and
open(r"%LOCALAPPDATA%\\kiro-cli\\c.json") are indistinguishable by any value
transform. Only the sink differs — which is why the replacement is sink-aware.

Change.

• is_sensitive_bash_command takes subject_is_shell_grammar: bool = True
  (keyword-only). The default preserves today's behaviour exactly, so every existing
  caller is unaffected. _vet_script_contents — the one caller whose subject is source
  code — passes False.
• The skip is keyed on the subject, never on one check. A caller either has shell
  grammar and gets all three pass-1b checks, or does not and gets none — the property
  the extraction control depends on, per the comment at the top of that block.
• New security surface: sensitive_run_in_source_literals in security.py. For a
  source subject the collapse is replaced, not removed. The body is parsed once and
  the same three pass-1b checks run over separator-collapsed copies of each decoded
  literal, so a run that survives into a real path is still refused.
  • str and bytes both, bytes decoded latin-1 (total over a byte range, one
    code point per byte, so a run survives unchanged) — open/os.open accept a bytes
    path, so an rb"…" literal reaches the same sinks.
  • Sink exoneration, argument-position aware. A literal is exonerated only in the
    pattern operand (args[0] / pattern=) of an allowlisted re.* call. The
    allowlisted calls are not uniformly safe: re.sub(pattern, repl, string) returns
    its subject verbatim and its replacement substantially so, so a fenced path in
    either slot would flow on through a call that merely looks harmless.
  • The re binding must be authentic. The allowlist keys on the spelling
    re.<func>, so it is withdrawn for a body that rebinds the name at all —
    import evil as re, re = …, class re, a parameter or loop variable named re.
  • Deny is the default. An unknown call, a name bound first, or no enclosing call
    keeps the deny verdict, so an unenumerated sink over-blocks rather than opening the
    fence — the direction _TRUST_ROOT_READ_LISTERS argues for. shutil.copy is
    covered without shutil appearing in the checker.
  • Non-docstring statement-position constants are skipped (a bare string
    expression, which Python evaluates and discards, so it reaches no sink). A
    docstring is NOT skipped: Python RETAINS it as __doc__, where a body can
    read it back and hand it to a sink (open(f.__doc__)), so docstrings are
    scanned. The cost is an accepted over-block — a prose-only docstring naming the
    store is refused even though it reads nothing, pinned by
    test_a_docstring_naming_a_fenced_store_is_an_accepted_over_block. Exempting
    docstrings would reopen a single-separator open(f.__doc__) path, so the value
    check stays uniform. Comments never reach the check at all.
  • Fail-closed fallbacks. A body that does not parse yields no literals and the
    caller runs the raw scan with the collapse; a body too deeply nested to
    traverse is reported the same way rather than raising, so a legitimate deep
    expression degrades to the textual scan instead of failing the gate.
• Owning specs updated in the same commit: docs/system-specs/modules/security.md
  (pass 1b, its shell-grammar scope, and the escape-aware replacement) and
  docs/system-specs/modules/learn-cron-dashboard.md (the script-body scan).

Tests

test/test_mcp_cron_security.py:

• ATTACK_SCRIPTS_WITH_A_SEPARATOR_RUN — 16 bodies that must be BLOCKED, each
  verified blocked on the pre-PR base so every one is a true regression guard: raw and
  non-raw open(), pathlib, an f-string segment, a name-bound literal, an
  unenumerated sink (shutil.copy), the two shapes _separator_collapsed_variants'
  own docstring records as prior review-found regressions (a mixed-separator run and
  a UNC leading pair with an interior run), the bytes twins in both the
  drive-letter and relative-traversal spellings, a fenced literal laundered through
  re.sub's subject and replacement slots, and two re-rebinding bodies.
• BENIGN_SCRIPTS_WITH_A_SEPARATOR_RUN (3) — the false positives this PR clears,
  including the motivating redactor line.
• test_vet_script_contents_still_exonerates_a_pattern_slot_literal — the motivating
  real-world case must survive the argument-position narrowing.
• test_vet_script_contents_keeps_the_collapse_when_the_body_does_not_parse — the
  unparseable body is scanned as text, never exonerated.
• test_vet_script_contents_survives_a_deeply_nested_expression — a valid deep body
  must not raise out of the gate.
• COMMANDS_WITH_A_SEPARATOR_RUN (4) — the leak control: the carve-out must not reach
  the command path. Every payload is reachable only through pass 1b, verified
  missed when the flag is flipped, so this control can actually fail.

A fourth leak-control candidate, cat $HOME//.aws/credentials, was dropped on
measurement: a later pass catches it regardless, so it would have passed even if the
carve-out leaked.

test/test_security.py::TestWindowsSeparatorRuns — all 14 command-path tests for
#6350 still pass, unmodified. Full run: 2284 passed, mypy clean, flake8==7.1.0 clean.

Manual verification

• pytest test/test_mcp_cron_security.py test/test_security.py test/test_cron_gateway_integration.py test/test_cron_sdk.py test/test_app_bridges.py test/test_slack_gateway_cron_exec_coverage.py test/test_hooks.py test/test_denied_commands_security.py -n0 → 2478 passed, 3 skipped
• Blocking gates run locally, all pass: scrub-lint.sh --no-history,
  verify_vendor_manifest.py, check_loop_bound_locks.py (+--test),
  check_harness_parity.py (+--test), docs_lint.py --test, docs-lint.sh,
  check_black_formatting.py, check_subprocess_encoding.py (+--test),
  isort --check-only, flake8 (pinned 7.1.0 → 0 findings), mypy src/kiro_crew/
• Rebased onto 53847dccc mid-review; git patch-id --stable identical before and
  after, so the patch is unchanged by the rebase.

Screenshots / video

N/A — no user-visible UI change.

Related Issues

Fixes #7912

Docs updated in this commit, per the same-commit rule:

• docs/system-specs/modules/security.md — pass 1b was undocumented (docs/ had no
  reference to _separator_collapsed_variants or the collapse, and the function's own
  docstring advertises "a two-pass approach"). Adds the pass and its shell-grammar
  scope.
• docs/system-specs/modules/learn-cron-dashboard.md — script-cron safety was
  documented as path-gating plus SEL audit, with no mention of the body scan. Adds the
  scan, its two shell-grammar exclusions, and the permanence of a fire-time denial.

Pattern harvest

Rule candidate: semgrep
Pattern: a matcher whose heuristics assume one input grammar, invoked on a subject in
a different grammar. Here a shell-command matcher was called on a Python source body,
so an escape sequence was read as a redundant path separator. The tell is a
transformation that is only meaning-preserving under the assumed grammar —
collapsing a separator run is a no-op for a Win32 shell and destructive for source
code — and the failure lands on benign inputs while the malicious ones stay caught, so
it reads as a false positive rather than as a scoping error.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title
• 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

[jeeshofone]

Cross-check from reading the scanner while triaging #7912 — one constraint worth verifying against this fix's shape:

If "scope the collapse to shell-grammar subjects" means the script-body path skips _separator_collapsed_variants entirely, that reopens #6350 inside scripts on Windows: a Python body doing open("%LOCALAPPDATA%\\\\kiro-cli\\\\...") carries two literal backslashes in the string VALUE, Win32 collapses the run at open time and the fenced store is read — while the raw source (uncollapsed) matches no fence pattern. So the source path can't just drop pass 1b; it needs an escape-aware equivalent (e.g. apply the collapse to decoded string-literal values, or scan a literal-decoded rendering alongside the raw text, erring toward denial on ambiguity).

Also noting _separator_collapsed_variants' docstring records two prior review-found regressions on this exact mechanism (mixed-separator runs; UNC leading pairs) — worth carrying those cases into any new test coverage here. If the PR already handles the decoded-literal case, please disregard.

[github-actions]

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

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

All evidence gathered and verified against the base tree. Final review:

First-Principles-Verdict: CONCERNS

One exonerating branch (_MATCH_RETURNING_METHODS discarded-result allowance) permits only a shape the diff itself calls dead code at module level — inherited by symmetry, zero counted users.

What this change ships

Intent: stop a cron script that merely names or redacts a fenced path from being permanently refused at every fire — a FIX.

1. A script whose re pattern redacts/names a protected store is now accepted — justified, cause-level (heuristic scoped to its subject).
2. A script reaching a store through a separator run inside a decoded literal (open/pathlib/shutil/bytes/f-string) stays refused — justified (keystone fence; skip-alone measured to miss 4 attack bodies).
3. New security.is_sensitive_source_body() owns both halves; the skip flag stays private — justified, 1 consumer (mcp_cron.py:777).
4. Shell gate's pass 1b skips one duplicate scan of the unmodified subject — rides along; justified by measured 2× cost on a linearity-guarded path, detection pinned both directions.
5. A prose-only docstring naming a store is still refused — declared accepted over-block, pinned by test.
6. Test-only/chained uses (if re.search(F,…), re.compile(F).search(s)) stay refused — declared over-block; base refused them too.
7. ~770 lines of deny-first AST guards so the one permitted slot cannot be laundered — justified by the agent-untrusted-to-its-ceiling boundary.
8. Both owning specs updated in the same commit — mandated by AGENTS.md.

Watch

• Two counted sibling gates left unfixed, both declared in the description (grep is_sensitive_bash_command over src/): llm_helpers.py:958 runs the same collapse over every tool-input string, so the agent is still refused writing the redactor cron can now run — the user-level job stays half-blocked; skills_script_validator.py scans raw text with no collapse, so the doubled spelling closed here still passes there.

Subtractions

• Delete _MATCH_RETURNING_METHODS and drop match/search/fullmatch/finditer from _SAFE_COMPILED_PATTERN_METHODS (security.py): the only shape their branch in _compiled_name_escapes exonerates is a discarded statement-position p.search(s) — the diff's own spec text removed the identical module-level rule as dead code because "no real script writes" it; benign-corpus uses of these four methods: 0 of 5 bodies. Deny-by-default already covers them.

[FIRST-PRINCIPLES-REVIEWED] bde516d

[github-actions]

Design Review (Fable 5, fork) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Right scoping fix, but the exoneration half builds an adversarial-grade AST analyzer to defend a layer a two-token string concat steps around entirely.

Watch

• Proportionality of the hardening arms race. The ~780-line escape analysis (module authenticity, Match recovery, walrus, wildcard imports, sys.modules mutation, namespace writes…) exists to let one idiom — a fenced spelling in an re pattern slot — survive. But the literal scan it guards is defeated by open("%LOCALAPPDATA%\\kiro" + "-cli\\c.json"): no single ast.Constant matches the fence, so a hostile author never needs an exonerated literal at all. The deny-first shape means future gaps land as over-blocks, not fence openings — the real cost is a permanent bespoke static analyzer in security.py whose regression risk is re-breaking the exact false positive this PR clears (each new Python binding/recovery spelling must be triaged against ~10 interacting whole-body forfeit rules).
• The stated user harm is only half-cleared end-to-end. As the description itself notes, llm_helpers.py still refuses an agent writing the redactor body cron may now run, and skills_script_validator.py retains the doubled-separator gap closed here for cron. Until the first lands, the motivating scenario (agent-authored redaction cron) still fails one gate earlier with the same misleading message.

Suggestions

• Record the string-concatenation bypass in security.md as this layer's ceiling, so future review rounds stop deepening the exoneration analyzer past the value the layer can deliver.
• The new analyzer (~780 lines, self-contained) belongs in its own module with security.py re-exporting is_sensitive_source_body; security.py is already ~9,000 lines and this block shares no state with it.

[DESIGN-REVIEWED] bde516d

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

I've verified the complete production change: _vet_script_contents swaps is_sensitive_bash_command(text) → is_sensitive_source_body(text), with every other detector (_CRON_CRED_PATH_RE, _CRON_SECRET_ENV_RE, _CRON_SECRET_NAME_RE, scan_exfiltration_urls) unchanged around it.

I independently traced the load-bearing properties:

• Deny-first is intact. In visit, a fence-hitting literal is exonerated only when key is not None AND key in _SOURCE_PATTERN_SINKS AND in_pattern_slot AND re_authentic — every other branch of the if (... or ... or ...) returns the denial. No call, unknown call, non-pattern slot, or a rebound re all deny.
• No under-block regression from skipping pass 1b. When the body parses, is_sensitive_bash_command(text, _subject_is_shell_grammar=False) still runs pass 1 (raw regex), the normalizer, IMDS and env-cred checks over the raw source; only the raw-text separator collapse is skipped, and the decoded-literal scan replaces it over str + latin-1-decoded bytes, value first then collapsed copies. An unparseable body keeps the raw collapse.
• Every re-extraction route is closed or fails closed — .pattern/.re/.__dict__ dotted and getattr/attrgetter-spelled reads, Match-via-callable-repl (positions 1 and 0), Match-returning module sinks absent from the set, untrackable/unbound compile results, the walrus-in-pattern-slot case, the outermost-expression requirement, and name/attr/subscript/call/wildcard-import/dynamic-exec/namespace-mapping rebinding of re and its aliases.
• No crash path. ast.parse errors and RecursionError (both at parse and during visit) route to (False, None) → raw scan with the collapse; the guarded index accesses in _replacement_is_provably_non_callable and _enclosing_call_slot cannot go out of range.

I could not ground a reachable behavioral defect or rule violation on any changed line, and I originate none.

No findings.

[OPUS-REVIEWED] bde516d

[github-actions]

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

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

2 of 2 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

BLOCKING -- src/kiro_crew/security.py:9055 -- Executable pattern expressions bypass the credential fence
and parent.args[0] is inner
re.compile(FENCED + Reader()) -> _vet_script_contents exonerates the nested literal -> Reader.__radd__ reads the expanded credential path before compilation.
Anchor: backend-security-controls
Fix: Exonerate only a literal directly occupying the pattern operand.

BLOCKING -- src/kiro_crew/security.py:8886 -- Stored re aliases bypass authenticity validation
and isinstance(value, ast.Name)
holder=[re]; m=holder[0]; m.compile=reader; P=re.compile(FENCED) -> alias mutation is missed -> the cron executes reader against the credential path.
Anchor: backend-security-controls
Fix: Forfeit authenticity when an alias is stored through a container or derived expression.

[BLOCK-MERGE] bde516d
[GPT-REVIEWED] bde516d

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #5336 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #5336: MERGE_DISCUSSION. Complementary changes to the same Pass 1b loop, conflicting only textually. Both should land; the second one merged must rebase. Files: src/kiro_crew/security.py.
• PR #7298 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7298: CONTINUE_DEVELOPMENT. Independent behaviour, shared insertion point in the same function and shared cost machinery; coordinate ordering rather than treating either as redundant. Files: src/kiro_crew/security.py.
• PR #7414 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7414: KEEP. File and keyword proximity only; no shared behaviour and no conflict. Files: src/kiro_crew/mcp_cron.py. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.
• This PR is OVERLAPPING with PR #6993. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7913: MERGE_DISCUSSION. The merged PR is the origin of the defect, not coverage of the fix. It only establishes that pass 1b on main is unconditional and must not be dropped outright. Files: src/kiro_crew/security.py.
• This PR is OVERLAPPING with PR #8282. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7913: MERGE_DISCUSSION. Guaranteed conflict in the same pass-1b block plus interacting cost arguments. Land order should be agreed: whichever lands second must re-express its change against the other's shape (8282's _sensitive_pattern_hit needs to be inside _fence_hit_in_collapsed, or 7913's helper call re-applied on top of Pass 0). Files: src/kiro_crew/security.py, src/kiro_crew/llm_helpers.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

[bolichen97]

Evidence for the convergence, not a review request and not a claim on these functions — I am not
opening a competing PR while this is live.

Measured this branch at d5be722cc against the 13 script crons of one real install, calling
mcp_cron._vet_script_contents directly: 8 are still refused, none of them by pass 1b. They
are refused by two structural budget passes that landed after this PR was opened and that
subject_is_shell_grammar=False correctly leaves in force, since it is documented as skipping
"this pass and only this pass":

• _ALT_MAX_STAGES = 512 — #7441 6644b18ec,
  2026-09-03 23:19 UTC — 6 of the 8
• _FIND_SUBSTITUTION_BUDGET = 64 — #7298
  d03617614, 2026-09-04 16:18 UTC — the other 2

Minimal repro on this branch, a body with no shell content, no fenced path and nothing to redact:

from kiro_crew.mcp_cron import _vet_script_contents
_vet_script_contents("x = 1\n" * 600)
# Error: cron script blocked by security policy: Blocked: command has more pipeline
# stages than this gate inspects (512), so a traversal in it cannot be ruled out

_alt_collect_stages splits the subject on newline / ; / |, so in a source body every line is
a stage and a few hundred lines of ordinary Python exhaust the cap. Stage count is not line count,
so it bites earlier than expected: one of the refused scripts is 331 lines.

Both budgets are right for a command line, and their fail-closed exhaustion is the thing #7441
deliberately fixed — so the fix is not to exempt a source subject from them, which would make
exhaustion mean "inspect less" again. The premise to attack is the staging: a Python body is not a
pipeline, the strings it hands to a shell are, and each of those is small enough that no budget is
ever reached.

That is why this is posted here rather than as a separate PR — your literal walk already
enumerates every decoded str/bytes constant, which is exactly the vehicle. Running the
shell-grammar passes per decoded literal instead of over the raw whole file would remove the size
ceiling while keeping coverage of the shell a script actually invokes, and your existing "a body
that does not parse keeps the raw-text scan" rule already supplies the fallback so nothing is
quietly exonerated. Whether that belongs in this PR or a follow-up is your call.

Context on why this is not hypothetical: on the install above, the six _ALT_MAX_STAGES jobs are a
live issue-triage pipeline that stopped dead at a gateway restart which picked up #7441 — scripts
untouched since 2026-09-01, intake/dispatch/cleanup/audit all refused on every tick. Full numbers
and the per-script table are on
#7912.