fix(security): stop a long command from taking the gateway down, and name the job

[bolichen97]

Problem / Motivation

An hourly cron whose agent emitted a ~9 KB bash command full of https:// URLs took a user's gateway down every hour. The crash dump's main thread sat in security.py inside is_sensitive_bash_command for the full 25 s watchdog budget (dashboard.loop_stall_exit_after_secs=25), the loop-stall watchdog hard-exited the process, and the run in flight left no trace in the cron store — so the job was due again on the next boot and re-ran the crash. kirocrew doctor showed the stack but could not name the job, so the user paused the wrong one (a script cron that never touches the model). The user's own benchmark of the installed gate: 1.2 KB 0.6 s, 2.3 KB 2.3 s, 4.6 KB 9.1 s, 9.1 KB 35.8 s.

Why it matters

Every surface that streams a permission request (cron, Slack, dashboard side panel, workflows) and every caller of hooks.on_tool_call runs this gate inline on the event loop, so one long command from the model kills the gateway and every in-flight turn with it; for a cron it does so again on the next boot. #7941 removed the eleven .* token anchors and cut the constant ~20x, but the growth was still quadratic on every shape measured (10 KB 0.3 s, 40 KB 5 s, pattern tier alone), the title tier still ran inline, and nothing on disk said which job was running.

Supersedes #8277, #8278 and #8279 (the same three changes as one PR; each was independently mergeable and CI-clean on its own).

What changed (motivation → approach → change)

1. The gate is linear and bounded (security.py)

Three constructs were each quadratic on their own and their costs multiply, so each was measured and fixed separately (harness in a subprocess under timeout; pattern tier alone, 10 KB / 40 KB, before → after):

construct	shape	before	after	fix
redirect alternative `.*[<>	]\s*`	any input	0.30 s / 4.8 s	5 ms / 19 ms
UNC anchor \\\\[^\s'"]+ + generalized separator	one UNC token with \X\.. hops	0.06 s / 0.8 s	11 ms / 43 ms	UNC takes a plain separator: its greedy run already absorbs every character a no-op chain contains, so the languages are equal and each backtrack step is a constant-time literal check
verb-anchored verb.*<path>, interp … open( … <path>	verb-dense line	12 ms / 130 ms	4 ms / 18 ms	no regex spelling of "a verb earlier on this line" is linear; . never crosses a newline, so per line it decomposes into "earliest verb end" + "path search from there" (_verb_anchored_sensitive_hit, two linear searches, same language). _sensitive_pattern_hit runs both halves and is the only entry to the pattern tier

Fixing the first alone leaves the UNC shape quadratic (0.8 s at 40 KB); fixing the second alone changes nothing visible because the first dominates; fixing both leaves the verb term (×3.5 per doubling). All three are needed.

Whole gate at the crash size (10–12 KB) on every adversarial shape tried — doubled-separator paths, URL-dense JSON, UNC chains, verb-dense lines, ~-runs, 24 000-backslash runs: 20–80 ms, against 15–36 s on the shipped build. With the ceiling lifted for measurement, 20 → 40 → 80 KB scales ×2 per doubling on every shape.

MAX_SCANNABLE_COMMAND_CHARS (20 KiB) is a hard ceiling in the gate itself: a longer command is refused with a reason, never scanned partially and never let through unscanned. llm_helpers._MAX_SCANNABLE_TOOL_INPUT_CHARS already refused at that size and now aliases the same constant. Two later passes (_ENV_CRED_PATTERNS, the normalizer) remain O(k·n) in verb tokens and are bounded by the ceiling (≤ 60 ms at 20 KB); they were not on the crash path.

Zero verdict change. A 381 474-command generated differential corpus (verbs × preceding characters × paths × terminators, script-open shapes, multi-line, UNC and %VAR% spellings, plus every golden from the existing tests) produced no difference against origin/main and against the tree before #7941.

2. The title tier scans off the loop (llm_helpers.py)

#7941 moved the tool_input scan onto a worker but the title checks — is_sensitive_path, is_sensitive_bash_command, is_denied on event.title; for a shell tool the title is the command — still ran inline, and that is the frame in the crash dump. Title and tool_input now go through one asyncio.to_thread hop, title first, keeping every reason string and mechanism label (always_deny / always_deny_input). The empty-title refusal stays on the loop.

CPython's re holds the GIL for the whole of one match call — confirmed with a tick-counting probe whose clock starts before the worker does (a 5–8 s re.search on a worker leaves the main thread a single tick on 3.10 and 3.12, the same shape as sorted() on a large list, while zlib.compress, which does release, leaves it ticking). An earlier revision of this description claimed the opposite from a probe that started its clock after the worker already held the GIL; that claim is withdrawn and the comments and spec now state the measured behaviour. So the hop does not keep the loop live inside one scan — the linear patterns and the 20 KiB ceiling are the liveness guarantee — and what it buys is the realpath I/O inside is_sensitive_path (which releases the GIL) plus a yield between tool_input strings. hooks.on_tool_call (HOOK_BASED policy and the channel dispatchers that call it synchronously) still runs inline and relies on the gate's own bound, stated in the spec.

3. A run leaves an in-flight marker, and the breaker names the job (cron.py, cron_inflight.py, stall_attribution.py, cli_doctor.py, dashboard/server.py)

• cron_inflight — a run writes <data home>/cron-running/<job id>.json (job_id, name, started_at, pid) when it starts executing and clears it on every finally path. A marker whose PID is dead is exactly "this job was in flight when that gateway died": no schedule inference. Writes are off-loop and best-effort; reads are size-bounded; job ids that would leave the directory are refused.
• stall_attribution — reads the newest stack-bearing dump, names the surface from the outermost recognised frame of the wedged thread (a cron turn passes through slack/gateway.py, so a top-down match would call it Slack), names the permission-gate frame, and joins abandoned markers to the dump by the writing process's identity — PID plus the PID domain and start id the dump header already records (RunningMarker.same_process), so a replacement container's PID 1 is not the one that died and a recycled PID reads as dead. One match names the job; several name candidates and no job; none says so; a chat/channel surface is named and implicates no job. It never guesses. crash_dump_store gains dump_owner_identity, current_process_identity, pid_identity_alive and dump_wedged_frames.
• Breaker in CronService.start() — runs on a worker before _arm_timer(). Only when the surface is cron and exactly one abandoned marker carries the dump's PID does it park that job auto_paused (enabled=False, last_status="error", a last_error naming the dump and kirocrew cron resume <id>), persisted under the store lock and SEL-audited as cron_auto_pause / auto_paused_loop_stall. The dump name is claimed in cron-running/.loop-stall-breaker so one crash pauses its job once. Abandoned markers are swept after being read.
• kirocrew doctor — prints an attribution: block under the dump's stack: stuck-in frame, surface, job, recommended: kirocrew cron pause <id>, and the job's current pause state from crons.json (no gateway needed). The boot notification carries the same lines.

Sample doctor output on a synthesized data home shaped like the field host:

  attribution:
    stuck in the tool permission gate (security.py:7969 is_sensitive_bash_command)
    the gateway (PID 4200001) was executing cron job 'twb-refresh' (59c321a9) when the watchdog terminated it
    recommended: kirocrew cron pause 59c321a9
    job is currently enabled

Trust boundary: the markers are fenced evidence. The breaker's pause RESTS on a marker, so cron-running is on security._CREW_SECRET_LEAVES beside crons.json and cron-history (and masked by the OS sandbox alongside cron-history): a marker the agent could write would be an unauthorized "pause this job" that routes around the MCP cron tools, and one it could delete would disable the breaker for a crash loop about to recur. Under that fence cron_inflight still treats its own directory as hostile, for a leaf planted before the fence existed — reads open O_NOFOLLOW and take only single-linked regular files under _MARKER_MAX_BYTES; writes go through atomic_write(restrict_to_owner=True), whose mkstemp name cannot be pre-planted and whose linked-parent refusal is what stops a redirected marker write from landing on a keystone file. Nothing in the sandbox reads a marker: they are written by the run task in the gateway process and the breaker runs in CronService.start(), which only the gateway calls.

The verdict outlives the evidence, and evidence is consumed only once the verdict survives a restart. Before the markers are swept the breaker writes what they said to cron-running/.loop-stall-attribution (dump name plus the candidate and unrelated markers, no .json suffix so read_markers cannot mistake it for a marker) and attribute_dump merges it back for that dump — so kirocrew doctor and the restart notification, which both run after an automatic restart has already swept, still name the candidates the breaker declined to choose between. And a pause the store refused to persist is NOT claimed and its evidence is NOT swept, so the boot whose store is readable again reaches the same verdict instead of skipping a job that is still enabled and still due. No failure inside the breaker can fail start(): it is a safety net, and losing the scheduler would be worse than losing the net.

Specs: docs/system-specs/modules/security.md, learn-cron-dashboard.md, cli.md.

Tests

• test/test_security_gate_liveness.py — verdict pins for the cases where each rewritten construct was the only matching branch, plus negatives (path before the verb, verb on another line, open( before the interpreter); the pattern tier is the union of both halves; source guards; ceiling refused-above / scanned-at / tiers agree; both trigger paths at the crash size (Pass 1b double-separator 10 KB, URL payload 12 KB) and doubling-ratio linearity for the backslash run (Pass 1) and one shape per construct. Seven mutants each turn a test red (drop the ceiling; drop the verb half; accept a path before the verb; .* back on the redirect; generalized separator back on UNC; no line split; open( before the interpreter); the two spelling mutants are also caught by the timing pins alone. The per-construct linearity pin takes the BEST of three samples (a contended runner can only make a scan look slower) and its absolute backstop is 12 s, not 3 s: 80–100 KB is above MAX_SCANNABLE_COMMAND_CHARS so the gate refuses a command that long and nothing waits for that scan — the cost the loop actually pays is pinned at the crash size, under the ceiling, by the two tests beside it. A four-core runner with three sibling xdist workers read 4.0 s (Linux) and 5.2 s (Windows) for ~0.3 s of dev-box work, so 3 s false-red there; a reintroduced quadratic term still costs minutes at that size, so the ratio and the backstop both catch it.
• test/test_llm_helpers_tool_input_offload.py::TestTitleTierOffLoop — title bash/path/regex denials keep reason and always_deny; title decides before tool_input; title predicates run on a non-loop thread in the same hop as the tool_input predicates; the empty-title refusal never enters the scan.
• test/test_cron_loop_stall_breaker.py — the breaker test seeds an overdue strict every job and asserts it did not fire after start(); with the breaker call removed the job fires on the first tick (['<id>'] == []), which is the crash loop. A resumed job is not re-paused on a second boot; chat-surface and two-candidate dumps pause nothing; the pause is SEL-audited once; markers are present during a run and gone after normal and raising completion.
• test/test_cron_loop_stall_breaker.py::TestMarkerIoRefusesWhatItDidNotWrite — a symlinked marker and a symlinked claim are not followed; a FIFO marker is refused instead of blocking the worker start() awaits (asserted under wait_for, so a regression times out rather than hanging the shard); a redirecting parent link leaves the target directory empty; an oversized marker is refused. Plus: an unpersisted pause writes no claim, keeps its evidence, and is retried and paused on the next boot; the non-cron and two-candidate cases sweep their markers but the recorded verdict still names the candidates to a later attribute_dump; a marker no dump explains is swept; a breaker that raises leaves the scheduler running; the rule table's module and function names are pinned against the package, so a rename cannot silently degrade classify_surface to "unknown". test_security.py::TestCronStoreProtection pins the new fence in both home prefixes, on the read gate, the write gate and the shell forms (forge, read and rm), and test_sandbox_governance_mask.py's reconciliation ratchet pins the OS mask.
• test/test_stall_attribution.py + test/stall_dump_helpers.py — marker round trip and validation; surface classification (cron through the Slack module, dashboard chat, Slack, unknown, Windows paths); attribution for single/multiple/no marker, PID mismatch, marker newer than the dump, live owner; describe() lines; newest-dump selection past a header-only file.
• test_security_regex_linearity.py::test_long_nonshell_line_does_not_blow_up resized under the ceiling (it fed 22.5 KB and asserted "allowed"; that is now refused by design).

test_cron.py::TestComputeNextRunTs::test_cron_schedule fails on untouched origin/main (timezone-dependent) and is unrelated.

Manual verification

Reproduced the field curve on the shipped build (10.7 KB double-separator command: 14.8 s). GIL probe (tick-counting, clock started before the worker) on 3.10.20 / 3.12.13. Ran kirocrew doctor against a temp data home with a crons.json, a cron-running marker and a dump in the store's format (output above).

Related Issues

Supersedes #8277, #8278, #8279. Follows #7941 (anchor rewrite + tool_input offload) and #8022 (bounded realpath in the same gate).

Pattern harvest

Rule candidate: review-prompt
Pattern: a leading .* (or a greedy run followed by a starred group) inside an alternation evaluated with re.search on agent-supplied text — redundant for existence, quadratic in the subject; measure each .*-bearing branch alone, because their costs multiply and fixing one hides the rest. And: an offload that moves a loop over N inputs to a worker while the same predicates still run inline on the request's primary field.

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

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

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

Design-Verdict: PASS

A measured, root-cause fix — linear patterns plus a hard ceiling carry the liveness guarantee, and the breaker acts only on unambiguous fenced evidence, never inference.

[DESIGN-REVIEWED] a57f462

[github-actions]

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

Premise-level review of a57f4620799082e4378dfc637bcd8d3e3b8232d2 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 is read: the contract, the full 3,729-line patch, the intent file, and the base tree checks (no pre-existing in-flight-run record; newest_dump_with_stacks, _owner_alive, process_start_time all reused from base, not duplicated). Final review follows.

First-Principles-Verdict: CONCERNS

Nearly every item is derived from one reported field crash; the one heavy piece is the attribution-record file, which re-persists the very markers it deletes.

What this change ships

Intent: stop a long agent command from crashing the gateway hourly, and let the operator see which cron job did it — a FIX (three previously separate PRs merged).

1. Commands over 20 KiB are refused with a reason, on every caller — justified (25 s watchdog, measured costs)
2. The sensitive-command scan is linear: ~10 KB costs ms, not 15–36 s — the fix, cause-level
3. Permission-request title scan moved off the event loop — the fix (the crash frame), mechanism-level
4. A running cron job leaves a cron-running/<id>.json marker — justified (os._exit skips every finally)
5. Boot auto-pauses the one job a cron-surface dump names, once — justified (the hourly re-crash)
6. doctor and the restart notification name the surface and job — justified (user paused the wrong job)
7. cron-running agent-fenced and sandbox-hidden — justified (a forged marker is an unauthorized pause)
8. Oversized cron script refused instead of vetted on its 256 KiB prefix — fix rides along, real bypass closed
9. Dump-owner liveness falls back to ps start time on macOS/BSD — rides along, not in the visible description
10. Crash-dump reads bounded at 4 MiB — rides along, boundary-derived

Watch

• Item 9 silently changes existing dump-sweep behavior on macOS (a recycled-PID dump becomes sweepable where the guard previously never confirmed). Safe direction, but it predates no marker need — it could ship alone.
• The .loop-stall-attribution record exists only because markers are swept before doctor runs; the sweep is a choice, not a constraint — see Subtractions.

Subtractions

• Delete record_attribution / read_recorded_attribution / ATTRIBUTION_RECORD_NAME and the trimming loop (~100 lines in cron_inflight.py plus the merge block in stall_attribution.attribute_dump, each with exactly 1 consumer): instead, sweep an abandoned marker only when no stack-bearing dump explains it (that sweep already exists — test_a_marker_no_dump_explains_is_swept). Retained markers give doctor the same candidates the record reconstructs; the claim file alone already prevents re-pause.
• remove_markers has 0 consumers outside its own module (grepped remove_markers across src/: 1 hit, sweep_abandoned_markers); fold it in.

[FIRST-PRINCIPLES-REVIEWED] a57f462

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

I've traced the single candidate against the actual PR-head code. The candidate is a self-rated "low confidence" double-cancel race. Falsifying it:

• (a) concrete input: Every cancellation path (stop() line 1457, cancel() line 1723, _kill_job_session line 1552) calls task.cancel() exactly once. The scenario requires a second cancel() landing precisely inside the finally's await asyncio.shield(marker_write) window — which itself only exists if the first cancel landed inside the sub-millisecond try-body shield window before the write completed. No caller produces this; it is a stacked "could."
• The single-cancel path is safe by design: the finally's await asyncio.shield(marker_write) waits for the write to publish, then clear_marker removes it.
• (c) observable wrong outcome collapses: For a stale marker to cause a wrong auto-pause, the gateway must later loop-stall-crash with a cron-surface stack. But the actually-stalling cron run leaves its own in-flight marker with the same PID identity → ≥2 abandoned markers → the breaker declines ("one cannot be named", pauses nothing). If instead the stale marker names the same job that is stalling, pausing it is correct. There is no clean path to pausing the wrong job.

The candidate fails (a) and (c). Nothing else in the diff grounds a new finding to the 80+ bar — this is careful defensive hardening with fail-closed reads, fenced evidence, and idempotent claims.

No findings.

[OPUS-REVIEWED] a57f462

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

FINDING -- src/kiro_crew/dashboard/crash_dump_store.py:691 -- "a reader then falls back to the number alone" contradicts RunningMarker.same_process, which rejects missing PID domains -> Fix: document that missing domains prevent attribution.

[GPT-REVIEWED] a57f462

[bolichen97]

Addressed the review findings in 8953ed82e (still one commit):

• Opus — doctor's job is not None gate ignored the surface. Now if _attribution.is_cron and _attribution.job is not None, the same predicate the breaker and describe() use, so a bystander marker under a chat/Slack stack no longer prints job is currently … or files an "attributed to cron job" issue that contradicts the line above it.
• Design — the breaker's sweep consumed the ambiguous-case evidence before the doctor could read it. The breaker now records what the abandoned markers said (cron-running/.loop-stall-attribution: dump name + candidate/unrelated markers) before sweeping, and attribute_dump merges that record back for the same dump. kirocrew doctor and the boot notification therefore reach the breaker's verdict — including the two-candidates "inspect each before pausing" branch and the unrelated-abandoned report — on a later run. Tested in test_recorded_verdict_survives_the_marker_sweep and the extended test_two_candidates_pause_nothing (markers gone, doctor still names both).
• Design — _SURFACE_RULES could drift silently. test_rules_name_modules_and_functions_that_exist resolves every path fragment against the installed package and every function name against a def in kiro_crew; it caught one loose fragment on the spot (/kiro_crew/heartbeat → /kiro_crew/heartbeat.py).

First Principles "watch" (12 label-only surfaces) left as is: each row only changes wording, and a recognised non-cron label is exactly what prevents the misattribution this PR fixes.

[bolichen97]

Both GPT blocks on 2020a4e64 fixed in 9e5d7e3ac (one commit, rebased onto main):

• Windows follows a marker symlink (O_NOFOLLOW opens as 0 there). _read_own_file now refuses is_link_or_junction(path) before the open on every platform; on POSIX the flag still closes the check-to-open window. Test: test_a_leaf_link_is_refused_even_without_o_nofollow (flag zeroed, symlinked marker → not read).
• A lost claim could re-pause a resumed job. Two changes: write_claim reports whether it landed and the markers are swept only behind a written claim, so a boot that could not claim retains the evidence; and the pause is idempotent per dump — _pause_for_loop_stall treats a job whose last_error already names this dump as settled (resume keeps last_error), so a re-derived verdict on the next boot pauses nothing and just claims and sweeps. Test: test_a_lost_claim_cannot_re_pause_a_resumed_job (claim write failing → pause persisted, markers kept; operator resumes; next boot leaves the job enabled, writes the claim, sweeps).

Also unblocked the Fork workflow-change guard: the black-baseline prune that touched .github/black-baseline.txt is reverted, so the PR touches no .github/**.

[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. Both PRs independently rewrite the verb-anchored branch of _build_sensitive_regex into a chained-search helper and both drop the redirect arm's leading .*, and they conflict textually. Neither subsumes the other: 5336 owns the Issue #5265 spelling fence, 8282 owns the 20 KiB ceiling, the UNC anchor and the cron attribution. A maintainer should pick which PR owns the verb extraction and rebase the other onto it. 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. Same function, same liveness premise, non-overlapping bounds; worth coordinating landing order and confirming neither ceiling is presented as covering the other's case. Files: src/kiro_crew/security.py.
• PR #7913 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 #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.
• This PR is PARTIALLY_COVERED with PR #7941. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #8282: KEEP. The merged predecessor covers only the constant-factor part of the same problem; the growth, the ceiling, the title tier and the whole cron attribution/breaker are absent from current main. 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]

Pushed 5a1836220 (one commit, rebased):

• First Principles BLOCK — dump_owner_pid has zero consumers. Correct; deleted. stall_attribution uses dump_owner_identity, and the description now says so.
• GPT — test holds a process-lifetime dump descriptor. Correct; test_latest_picks_the_newest_dump_with_stacks now writes the header-only dump as bytes instead of calling open_dump_file, so no fd or module global outlives the test.
• GPT finding — _save() failure left the job disabled in memory. Fixed: the pre-pause fields are restored on CronStoreUnreadable, so this session schedules the job as the disk says and the next boot retries the pause; the audit event now fires only once the pause persisted.

GPT BLOCK — "auto-pause trusts agent-writable crash dumps": not fixing, with reasons. A forged dump alone can pause nothing: the breaker acts only on a cron-surface dump plus exactly one abandoned marker whose (pid, pid_domain, pid_start) equals the dump header's, and the markers are fenced (cron-running is on _CREW_SECRET_LEAVES and sandbox-masked). So the forgery needs a marker the agent cannot write — which exists only when a real gateway hard-exited while that job was genuinely running — and its whole effect is to pause the one job a real crash actually interrupted: audited (auto_paused_loop_stall), named in last_error, reversible with one command, once per dump. That is strictly less than what the same actor can already do to that job through the crons.json edits the gate already refuses. Fencing logs/crash-dumps read+write would cost the one thing the dumps exist for — an agent diagnosing its own gateway's stall (this PR's own investigation read them) and kirocrew doctor run through an agent shell — to remove a capability the forger does not gain. The trust boundary is stated in the description and in security.md.

First Principles' subtraction (fold remove_markers into sweep_abandoned_markers) is advisory and left as is.

[bolichen97]

Both GPT blocks on 5a1836220 fixed in b44d35246 (one commit, rebased):

• Raw cron names reach the terminal. describe() now renders every string read off disk — marker names and ids, and the dump's frame path/function — through _inert, which escapes non-printable characters and leaves printable text (non-ASCII names included) as written; the doctor's issues-summary line goes through the existing _safe_display. Test: test_describe_renders_disk_strings_inert (a name carrying \x1b[2K…\r… and a frame path with \x07 come out escaped, the CJK part intact).
• clear_marker unlinks through a linked parent. Refused when cron-running is a link, the same _readable_running_dir rule the readers use. Test: test_clear_does_not_delete_through_a_linked_directory.

The forged-dump block from the previous round is no longer raised; the trust-boundary reasoning above stands as the record.

[bolichen97]

Both GPT blocks on b44d35246 addressed in d1c59177f (one commit, rebased):

• Record can exceed its reader's limit. record_attribution now bounds names to 128 chars, trims the unrelated_abandoned tail until the serialized record fits the reader's cap (raised to 256 KiB; candidates are never dropped), and reports whether a readable record landed — the breaker sweeps the markers only behind both a written claim and a readable record. Test: test_record_is_kept_within_what_its_reader_accepts (120 candidates + 3000 unrelated with 2 KB names → all candidates survive the sweep).
• PID reuse undetected outside Linux. _pid_start_id falls back to platform_compat.process_start_time where procfs is absent, so macOS gets a start identity too (Windows still degrades to PID liveness, which is what that helper returns there — stated in the docstring). Dump headers and markers pick it up through the same function.

[bolichen97]

On d1c59177f, pushed 5508de97f (one commit, rebased):

• Unbounded dump read on the startup path — fixed. All three dump readers (dump_first_stack_lines, dump_wedged_frames, the boot replay) now go through _read_dump_bytes, the same O_NOFOLLOW + regular-file reader the header scan already used, capped at 4 MiB (_DUMP_READ_MAX_BYTES; faulthandler writes tens of KB at most).
• Docstring said Windows has no start identity — corrected; process_start_time does return one there.

"Incomplete marker evidence can pause the wrong job" — not changing, with reasons. The scenario needs two jobs in flight during one crash and the marker write for exactly one of them failing. Both markers are written by the same process into the same directory with the same atomic_write, so a failure that hits one hits both (unwritable directory, disk full), and then nothing is attributed. When one marker does survive, the job it names was running in the wedged process — it is a legitimate suspect, not an unrelated job — and the pause is audited, names the dump, and is reversible with one command. An "atomic record of the complete active-job set" would mean serializing every run start through a single file on the hot path to defend against a failure mode that already fails safe; that is the over-engineering this PR is trying not to do. The breaker's contract ("exactly one abandoned marker, else pause nothing") is stated in the spec and is where this residual lives.

[bolichen97]

Both GPT blocks on 5508de97f fixed in 81eb3fc57 (one commit, rebased):

• PID-only (pre-fence) markers accepted. There is no legitimate legacy marker — this module is what introduces them — so the fallback is gone: pid_domain is a required field (_marker_from refuses a file without it, so it is never read) and same_process requires PID and domain to match, with the start id compared when both sides recorded it. A dump header without a domain joins nothing. Test: test_a_marker_without_identity_names_nothing.
• Cancellation mid-write could leave a marker behind. The marker write is kept as a task and the finally awaits it (shielded) before clear_marker, so the unlink always follows the publish.

[bolichen97]

Pushed 7f6f7932f (one commit, rebased). Two things in it:

Correction, and a retraction. The Design lane's "watch" item was right and I was wrong: CPython's re holds the GIL for the whole of one match call. My earlier probe started its clock after Thread.start(), by which time the worker already held the GIL, so the first recorded gap was small and the answer came out inverted. A probe that starts the clock first and counts the main thread's 20 ms ticks (gil_probe_v2.py, below) settles it:

Python	worker call	worker time	main-thread ticks (expected if released)
3.12.13	sorted(2e7 floats) — known GIL holder	6.11 s	2 (~305)
3.12.13	zlib.compress ×40 — known releaser	0.08 s	4 (~4)
3.12.13	re.search on 47 KB	5.54 s	1 (~277)
3.10.20	re.search on 47 KB	7.67 s	1 (~383)

Consequences, now stated everywhere the old claim was: the to_thread hop does not keep the loop live inside one scan; the liveness guarantee within a scan is the linear patterns plus the 20 KiB ceiling (both kept, both tested); what the hop buys is the realpath I/O inside is_sensitive_path (which does release) and a yield between tool_input strings. llm_helpers.py comments, security.md, the description and the commit message are corrected. @chenmingwei23 — your GIL observation in #8349 was right and my reply to it was not; apologies for the noise. #7941's original comment was also correct.

gil_probe_v2.py

import random, re, sys, threading, time, zlib

def probe(label, fn):
    done = threading.Event(); out = {}
    def worker():
        t = time.perf_counter(); fn(); out["s"] = round(time.perf_counter() - t, 2); done.set()
    th = threading.Thread(target=worker, daemon=True)
    t0 = time.perf_counter(); ticks = 0          # clock starts BEFORE the worker
    th.start()
    while not done.is_set() and time.perf_counter() - t0 < 120:
        time.sleep(0.02); ticks += 1
    print(sys.version.split()[0], label, "worker", out.get("s"), "s  main ticks", ticks,
          f"(~{int(out.get('s', 0) / 0.02)} if released)")

data = [random.random() for _ in range(20_000_000)]
probe("sorted", lambda: sorted(data))
blob = ("x" * 1_000_000).encode()
probe("zlib x40", lambda: [zlib.compress(blob, 9) for _ in range(40)])
pat = re.compile(r"(?:cat\s.*|.*[<>|]\s*)/home/[^/\s]+/\.aws", re.I)
subject = "ls " + " ".join("/opt//" + "a" * 60 for _ in range(700))
probe("re.search", lambda: pat.search(subject))

GPT block on 81eb3fc57 — fixed: in pid_identity_alive the start-id mismatch is checked before the own-PID shortcut, so a restarted gateway handed the crashed one's PID does not read the dead marker as its own.

Design's suggestion (a source guard that every _READ_CMDS/_WRITE_CMDS alternative ends in \s) is a good one; left for a follow-up to keep this change from growing further.

[bolichen97]

GPT block on 7f6f7932f fixed in 95ff62cf8: recorded is now checked before write_claim, so an attribution whose record could not be written lands no claim and sweeps nothing — the markers stay for the boot that can record. Test: test_an_unrecorded_attribution_is_not_claimed.

[bolichen97]

GPT block on 95ff62cf8 — rebutted, no change.

The finding asks for an HMAC "gateway-only key" over the marker and dump files. That is the same class as the forged-dump finding on the earlier head (dropped after the fence went in), now moved to a pre-upgrade window. Three facts make it a non-issue:

1. The forgery buys the attacker nothing it does not already have. The only effect of forged evidence is that the breaker auto-pauses the named cron job. The agent already has cron_pause as an MCP tool (mcp_cron.py), and it could also just edit crons.json's enabled field on the old build. A pause the operator can see — with a last_error naming the dump and kirocrew doctor showing exactly which files drove it — is strictly less than what the agent can do by simply calling the tool.
2. Post-upgrade, the write path is already closed. cron-running is in _CREW_SECRET_LEAVES (agent cannot read or write it), readers refuse anything that is not an owner-written regular file, and a marker is only ever joined to a dump by PID + PID-domain + start id. To pause a job the attacker needs a marker and a dump with a consistent fabricated identity, both planted before the build that introduces the format existed.
3. A key stored where the files are protects nothing more than the fence. Any secret the gateway can read to verify a signature lives in the same data home; keeping it from the agent relies on exactly the same sensitive-path fence that already protects cron-running. Signing therefore adds a key-management surface (rotation, first-boot bootstrap, "unsigned = reject" breaking every legitimate marker across an upgrade) and no security.

Given the shipped invariants (fenced marker dir, identity-joined attribution, operator-visible and reversible outcome), a signing scheme here is over-engineering for a threat whose payoff is a capability the agent already has by design.

[bolichen97]

Design Review CONCERNS — accepted-and-deferred

The Watch item is correct: _verb_anchored_sensitive_hit's linear decomposition of the old verb.*path branch holds only while every _READ_CMDS/_WRITE_CMDS alternative has the "leftmost-match ⇒ earliest-end" shape, and a future verb addition that violates this would fail-open silently on the keystone deny path.

This is not disputed. The suggestion — keep the old compiled (?:verb).*path form as a test-only reference and differentially compare against _verb_anchored_sensitive_hit on generated inputs — is sound and will be implemented in a follow-up.

Deferred to: #8485

Add a CI-tested differential gate: keep the old verb.*path spelling as a test-only reference and compare it against _verb_anchored_sensitive_hit on generated inputs, so a verb-list edit that breaks the decomposition goes red automatically.

Issue #8485 carries deferred-finding label, assignee bolichen97, and Due: 2026-10-04.

[bolichen97]

First Principles Review CONCERNS — needs-a-decision (maintainer)

The Watch item: .loop-stall-attribution record is a copy of data the breaker just swept, patching the symptom (sweep before last reader) rather than removing the cause.

The suggested subtraction (~150 lines: delete record_attribution, read_recorded_attribution, ATTRIBUTION_RECORD_NAME, _RECORD_MAX_BYTES, _RECORD_NAME_MAX_CHARS, and the merge block in stall_attribution.attribute_dump) would instead sweep a marker only once its claimed dump is gone or aged out — retained markers already yield the identical verdict to every later reader.

The observation is architecturally sound. However:

• doctor and the restart notification run after the sweep; if markers are retained until the dump ages out (typically 7 days), the sweep ordering changes materially and the claim/sweep/record protocol needs a corresponding rewrite
• This is a non-trivial refactor outside the stated PR scope (fix a stall loop + name the job)
• The current shape is safe: the record is written like a marker, sized for its reader, and the _RECORD_MAX_BYTES bound prevents unbounded growth

Maintainer question: should this be addressed in-PR (requires extending scope to the sweep-ordering protocol) or deferred to a follow-up? If deferred, is the current shape acceptable for merging?

[operator: bolichen]

[bolichen97]

Correcting my own rebuttal of the GPT block — argument 1 does not hold, and this needs a maintainer decision.

I re-checked the marker-forgery finding against the tree instead of re-asserting the earlier reply, and two of the three legs I posted in the rebuttal above are wrong. Recording that rather than leaving it to stand.

What is wrong with the rebuttal.

1. "The agent already has cron_pause as an MCP tool." — It has it only for jobs it owns. mcp_cron.py's cron_pause handler calls _check_cron_job_ownership(svc, jid) and returns its refusal before touching the store, and _authz_session_key() resolves through the strict resolver. A forged marker is not equivalent: attribute_dump builds candidates from whatever job_id the marker file names (stall_attribution.py, the read_markers loop — the join is on the dump owner's (pid, pid_domain, start_id), never on the job's identity or its owner), and the breaker then pauses that id. So the forgery grants a pause of a job the session does not own — an alerting or backup cron belonging to another session, or an _UNOWNED row. That is a capability the agent does not otherwise have, which is exactly what I claimed it already had.

2. "It could also just edit crons.json's enabled field on the old build." — It could not. crons.json and cron-history are already in _CREW_SECRET_LEAVES on origin/main, i.e. before this PR; I confirmed with is_sensitive_path against a temp KIROCREW_HOME that both resolve True on the pre-upgrade tree. This PR adds cron-running to that list; it does not add crons.json, which was never writable.

3. Argument 3 ("a key stored where the files are protects nothing more than the fence") is also weaker than I put it. The value of a signature here is temporal, not spatial: an agent that planted files before the key existed cannot produce a signature under a key minted at or after the upgrade, so "reject unsigned" would in fact close the window the finding names. sel_hmac.key is already a fenced gateway-only key used this way, so the pattern has precedent in-tree.

What is actually left of the finding. The window is narrow and post-upgrade is closed, which is the part of the rebuttal that does hold:

• Post-upgrade an agent cannot write a marker at all (cron-running is fenced, readers take only owner-written unlinked regular files under O_NOFOLLOW).
• logs/crash-dumps/ is not fenced, so a dump is forgeable at any time — but a forged dump alone names no job and pauses nothing, because the breaker needs a marker too. Pairing a forged dump with a legitimate surviving marker only re-targets the pause onto the job that genuinely was in flight, which is the intended outcome.
• So the arbitrary-victim case really is pre-upgrade-only: both files must predate the build that introduces the format. The outcome stays bounded and operator-visible — one job, last_error naming the dump, kirocrew doctor printing the evidence, reversible with kirocrew cron resume <id> — and the claim file means it happens at most once per dump.

Why I am not just pushing a fix. There are two credible remedies and choosing between them is a design call on the security surface, not a cleanup:

• What GPT asked for: an HMAC over marker and dump, unsigned rejected. Closes the window; adds a key bootstrap, a rotation story, and an "unsigned = reject" rule that has to be right across the upgrade that introduces it.
• A key-free alternative: treat the directory's contents as untrusted until a sentinel stored under an already-fenced leaf (run/, trust/) says the fence was in place when they were written. A pre-upgrade agent cannot write that sentinel, so the first boot after the upgrade sweeps whatever it finds unread instead of trusting it. No key, no rotation; it does add a mechanism and a spec change.

I do not think I should pick one unilaterally on a security-labelled PR, so I am leaving the GPT lane red and flagging it. Happy to implement either promptly once a maintainer says which.

For the record on the rest of the PR: rebased onto f59b865bb, three conflicts resolved (security.py pass 1b now routes _fence_hit_in_collapsed through _sensitive_pattern_hit, since the verb-anchored branch no longer lives in the compiled alternation and searching it alone would drop that branch on the collapsed copies; test_security_regex_linearity.py takes main's same-run ratio guard from #8630 wholesale and keeps only the resize under the new ceiling). The rebase also surfaced a real defect of mine, now fixed: the 20 KiB command-line ceiling was refusing ordinary cron script bodies, re-introducing through pass 0 the length-keyed refusal _source_command_subjects exists to remove. The ceiling is now a parameter of the subject class — MAX_SCANNABLE_SOURCE_BODY_CHARS (256 KiB, aliased by mcp_cron._MAX_SCRIPT_SCAN_BYTES so the reader admits exactly what the gate scans) — checked before ast.parse, because the parse is itself unbounded work on an unbounded body (0.3 s at the ceiling, 12 s at 1.5 MB).

[bolichen97]

GPT blocks on 21b31d5ae — one fixed in d123a4feb, one disclosed as a bounded residual (needs a maintainer call).

1. mcp_cron.py:242 — oversized scripts bypass tail scanning. Correct, and it is a real fence bypass. Fixed.

Reproduced before fixing, on a temp KIROCREW_HOME: a body of 433 long lines (few enough to stay under _ALT_MAX_STAGES and _SOURCE_COMMAND_SUBJECT_CAP, so nothing else refuses it) totalling 262 831 chars, then open("/home/user/.aws/credentials").read() at the end.

what the vetter SEES  : None
what the FULL body is : Error: cron script blocked: references a credential path …

_vet_script_file read exactly _MAX_SCRIPT_SCAN_BYTES, so the body handed to the gate was at the limit rather than over it, every pass scanned the benign prefix clean, and the sandbox then executed the whole file.

Two notes on attribution and on my own comment. The truncation predates this PR — origin/main has the identical f.read(_MAX_SCRIPT_SCAN_BYTES) with 256 * 1024 and no ceiling in the gate at all, so the tail was already unscanned there. But my alias is what made the two numbers exactly equal, and the comment I wrote on that line ("the reader admits exactly what the gate will scan") is precisely the claim that made the truncation invisible. So it is mine to fix.

The fix is GPT's: read one character past the cap (_SCRIPT_READ_PROBE_BYTES = _MAX_SCRIPT_SCAN_BYTES + 1). An oversized body then exceeds MAX_SCANNABLE_SOURCE_BODY_CHARS and is refused, which is the direction every other budget in this gate takes; a file of exactly the cap reads short of the probe and is still scanned in full, so no legitimate script is refused at the boundary. Post-fix:

oversized + payload past the cap -> Error: … Blocked: input is too large to security-scan (262145 chars > 262144 limit)
exactly at the cap (262 143)     -> None
ordinary 120 KB script           -> None

Tests: test/test_mcp_cron_security.py::TestOversizedScriptIsRefusedNotTruncated — the probe arithmetic, the credential read past the cap (the regression, which returned None before), no false refusal at the cap, and a payload inside the cap still denied on its merits rather than on its size (so the refusal is not standing in for a scan). Spec updated in the same commit.

2. cron.py:4030 — sweeping markers loses older crash evidence. Real, but narrower and less costly than stated; I am not redesigning the claim file for it here.

The mechanism is as described: attribute_latest_stall attributes only the newest stack-bearing dump, and the sweep behind the claim removes unrelated_abandoned markers too. So if a cron stall (dump1 + marker X) is followed by a newer non-cron stall (dump2) before the breaker has run, the breaker attributes dump2, pauses nothing, and sweeps marker X.

What I checked, and where I end up differing on severity:

• It is not unbounded, and it is not a lost pause. The sweep does not disable the breaker for job X. X is still enabled and still due, so it runs again, writes a fresh marker, and stalls again — and that crash's dump is the newest, cron-surface one, so the next boot pauses X. The cost is one extra crash cycle, not a crash loop that never breaks.
• The evidence is not lost to the operator. record_attribution runs before the sweep and records the unrelated_abandoned markers keyed to dump2, so kirocrew doctor and the restart notification still name marker X. That is what the "verdict outlives the evidence" half of this PR is for.
• It cannot mis-target. unrelated_abandoned markers are never candidates, so no wrong job is paused.
• Reachability is narrow. The breaker runs inside CronService.start() at boot, so for dump2 to be newer and dump1 unclaimed, the second stall has to land in a boot where the breaker did not settle dump1 — e.g. the store was busy/unreadable, which deliberately returns without claiming or sweeping. It is reachable, but it is not the ordinary path.

GPT's remedy — process every unclaimed dump by process identity, with claims retained per dump — is a redesign of the breaker's central mechanism: multi-dump attribution plus a per-dump claim file, where the claim is currently one dump name. On a change this size, at the end of a rebase round, I do not think I should swap that in unilaterally against the documented choice already in the spec ("a marker no dump explains is swept"). The alternative small fixes I considered all collapse back into needing multi-dump knowledge, so there is no cheap version of this.

So I am recording it as a named residual with the bound above rather than either silently carrying it or rebuilding the claim file. Happy to implement the per-dump claim promptly if you would rather have it in this PR than as a follow-up.

Where the rest of CI stands. On 21b31d5ae every backend shard was green on both Linux (3.12 × 4) and Windows (× 4), plus E2E, Opus 4.8, Design, First Principles, UX and the whole gate set — 57 success / 6 skipped. The only non-GPT red was Backend Lint & Type Check, an isort ordering slip on the import I added (MAX_SCANNABLE_SOURCE_BODY_CHARS before _SENSITIVE_HOME_DIRS); that is fixed in d123a4feb and I re-ran the lane's full step list locally — isort, flake8, mypy --platform linux, black gate, subprocess-encoding, agent-SDK boundary, sync-IO-in-async, lockdown-before-publish — all clean.