chore(subsystems): strip history narration from comments by iamwhatever · Pull Request #9338 · kirodotdev/KiroCrew

iamwhatever · 2026-09-08T01:40:05Z

Problem / Motivation

Comments in security/, slack/, acp/ and mcp_gateway/ had turned into a
task log. They carried PR and issue numbers, review-round markers such as
GPT 5.6 review round 6, and sentences about what the code used to do.
docs/system-specs/common/code-style.md already bans this in the section
"Comments explain the WHY": history lives in git, and a comment states CURRENT
behaviour in present tense.

This is the companion to #9334, which did the same for the top-level modules.
Split because the two together exceed a reviewable diff; the paths are disjoint.

Why it matters

A comment that narrates a change goes stale the moment the next change lands. A
reader cannot tell whether it describes the code in front of them or an earlier
version of it. That is worse than no comment, because it looks authoritative.

410 comment and docstring lines in these packages matched history-narration
patterns before this change. Every reference to this repo's own PRs, issues and
review rounds is now gone. What remains matches only on words the scanner cannot
tell apart from ordinary prose, such as a pid that "no longer exists".

What changed (motivation → approach → change)

security/argv_floor.py alone carried more than fifty of these markers, and
slack/gateway.py about seventy. A reader working on the deny floor had to guess
which parentheses were reasoning and which were just provenance.

The approach was to read every match in its surrounding code, then pick one of
three moves. Strip the reference and keep the sentence. Or rewrite the narration
as a present-tense statement of what the code does now. Or, if the comment said
nothing once the history was gone, delete it whole.

The change removes PR, issue and ticket numbers, review-round and finding
markers, and change-log sentences. Nothing else moves.

The diff also lowers comment-history-baseline.json. That file is the ratchet the
check_comment_history.py gate reads, and it records a per-file count of remaining
history markers. Cleaning a file drops its count, so the gate requires the entry to
be lowered or pruned in the same change. 21 entries are pruned and 22 lowered here,
all for files this PR cleaned; no entry goes up, which the gate enforces in one
direction only.

Tests

Comments and docstrings only, so no test outcome should move.

The executable token stream of all 43 files is identical to the base commit
af7f3e7fe, with comments dropped and docstrings normalised to a placeholder.
Verified mechanically per file.
All 43 files compile under Python 3.12.
133 test files covering these packages: 7423 passed, 6 failed, 14 skipped. The
same 6 test ids fail identically on a clean af7f3e7fe worktree, so they are
pre-existing local environment artifacts, not this diff.

Gates run locally, all passing: black (baselined), subprocess-encoding, isort,
flake8, brand name, feature-map, changelog-history.

Manual verification

The security packages needed the most care, because their parentheticals mix
provenance with a verified attack spelling. Those were separated by hand rather
than pattern-matched. For example

(GPT 5.6 round 3 on #7808, verified real): ``V='ci.skip main'; git push``

keeps the attack spelling and loses the round reference. Every measured number
("bash-measured", "~3.5s against 2,000 characters", "~512 lines") is kept as a
fact.

Two comments recorded why a rule is deliberately ABSENT — the AWS variable-name
denials in denied_rules.py, and the ambiguous-bin fallback in
resolve_once.py. Those arguments are the reason nobody re-adds the rule, so
they are kept in full and restated in present tense rather than trimmed.

Screenshots / video

Not applicable — no user-visible change.

Related Issues

Companion to #9334.

Pattern harvest

The check that made this safe was mechanical rather than careful reading:
tokenise each file before and after, drop comments, normalise docstrings to a
placeholder, and compare the remaining token stream. On the companion PR it
caught three files where an agent had reflowed real code.

It also caught the reason that happens: running black on a file listed in
.github/black-baseline.txt reformats pre-existing code, so "format what you
touched" and "change no code" conflict in exactly those files. The baselined ones
must be left unformatted.

Rewriting a comment to present tense often shortens it, which leaves a
half-empty continuation line that reads as a truncated sentence. Two such breaks
happened here and both were caught by re-reading the patched region rather than
by any gate. A prose edit needs the paragraph re-read, not just the line.

Checklist

Follows the code style in docs/system-specs/common/code-style.md
No executable code changed (token stream verified identical)
Local gates pass
No changelog entry needed (comment-only chore; the changelog gate is
append-only and has no draft section to add to)
No feature-map entry needed (gate confirms the change is accounted for)

Contribution License Agreement

By submitting this pull request I confirm that my contribution is made under the
terms of the project's license.

github-actions · 2026-09-08T01:43:25Z

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 98832d0

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

github-actions · 2026-09-08T01:43:51Z

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 98832d0

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

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

github-actions · 2026-09-08T01:44:39Z

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Spec-backed cleanup executed with real rigor: baseline ratchet only shrinks, executable lines verifiably untouched, and the two deliberate-absence rationales survive intact in present tense.

I independently checked the full range: every changed line under src/kiro_crew/ is comment or docstring prose (the sole code-adjacent hunk, return # binary missing/unspawnable…, changes only its trailing comment); comment-history-baseline.json only lowers or prunes entries, matching the gate's one-way contract; and the load-bearing "why this rule is ABSENT" arguments in denied_rules.py and resolve_once.py — the ones that stop a future re-add — are kept whole, with verified attack spellings (bash-measured, the k''ill splice, the heredoc bypasses) preserved as facts rather than provenance. This is the shape docs/system-specs/common/code-style.md mandates, done at the right layer (the ratchet, not a one-off sweep), with no gate, test, or spec weakened.

[DESIGN-REVIEWED] 98832d0

github-actions · 2026-09-08T01:45:11Z

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 98832d0af35b0ef8e0bfdd3e45295700b5ed0f2f — 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 checks pass: the baseline only shrinks, every code-looking line in the diff turned out to be a comment rewrite, the attack spellings and why-absent rationales are preserved, and the whole change is mandated by the documented code-style.md invariant. Final review:

First-Principles-Verdict: PASS

Token-stream identity across 43 files is author-asserted, not CI-enforced — spot-checks found no code drift, but a human should skim argv_floor.py and shell_normalizer.py hunks.

What this change ships

Inventory (5 items) — 5 justified

Intent: make comments in security/, slack/, acp/, mcp_gateway/ state current behaviour instead of narrating change history — a FIX against the documented code-style.md invariant ("Comments explain the WHY").

PR/issue numbers, review-round markers and change-log sentences removed or rewritten to present tense in 43 files — justified
Comments that said nothing once history was gone deleted whole — justified
comment-history-baseline.json ratchet: 21 entries pruned, 22 lowered, none raised (verified in the hunk) — justified
Why-a-rule-is-absent rationales (AWS denials, resolve_once bin fallback, pool.py user_identity) kept, restated in present tense — justified
Verified attack spellings and measured numbers (kill$(:) $(pgrep -f <name>), "bash-measured") retained while their review provenance drops — justified

Checks run: grepped all removed/added lines matching code shapes (statements, assignments, keywords) — every hit was prose or a trailing-comment trim (j += 1, return at patch lines 2247, 3831); confirmed scripts/check_comment_history.py exists on base.

[FIRST-PRINCIPLES-REVIEWED] 98832d0

… mapping `cleanup_orphaned_sessions()` prunes `session_pid_<pid>.txt` only when `platform_compat.pid_exists()` is false, and on POSIX that is literally `os.kill(pid, 0)`. Linux draws thread ids from the pid space, exposes `/proc/<tid>`, and lets you signal a tid -- so once a dead session's pid is recycled as a THREAD of an unrelated live process, the mapping passes the check and survives indefinitely. What that costs depends on the mapping. One that records a start token is already safe to resolve: `session_pid_sig._pid_recycled()` compares the live start token and refuses on a mismatch on both the strict and the lenient path, and a tid's live token cannot match the dead process's. A LEGACY token-less mapping has no recorded token for that guard to compare, so it keeps resolving for as long as the file lingers -- same-uid only, so robustness rather than a privilege boundary. Independently of either, nothing retires the files until the next gateway restart. Adds `platform_compat.live_thread_group_leaders()`: a single `os.listdir("/proc")` for the whole sweep. That listing enumerates ONLY thread-group leaders -- a non-leader tid is absent from it even though `/proc/<tid>` stays directly openable, which is exactly why the cheaper per-pid probes cannot tell the two apart. One directory read replaces a synchronous `/proc/<pid>/status` read per mapping, so the sweep performs no per-entry file I/O at all; measured on Linux that is 1.5 ms once against 7.8 ms across 233 mappings. It is also cheaper than `process_matches()`, which shells out to `ps` on macOS and so cannot be used per entry. The helper returns `None` -- never an empty set -- on non-Linux, on an unreadable `/proc`, and on a listing carrying no pids, and the caller treats `None` as retain-everything. An inconclusive answer can therefore never be the thing that decides a pid is stale; it only ever narrows an existing liveness check. The mapping files are materialized before the snapshot is taken, and that ordering is load-bearing rather than incidental: a mapping file is written at spawn, so every path in the list belongs to a process that already existed when the snapshot was read, and is therefore present in it and retained. Iterating the glob lazily instead would let an entry yielded AFTER the snapshot belong to a pid absent from it -- pruning a live session's mapping. The list is bounded by the mapping count the sweep was already reading per-entry (233 on the host measured above). The narrowed pass is NOT on the gateway boot path. The stale-mapping pass is now `prune_stale_session_pid_files(narrow_with_leaders=)`, and the boot sweep passes `False`, so boot does exactly what it did before this branch -- `no-new-work-on-gateway-boot-path` names orphan sweeps specifically, and the leaders snapshot is a `/proc` read that path may not carry. The narrowed pass runs instead on a tracked task after the `KIROCREW_READY` print, beside the governance refresh that cites the same rule. Deferring it is sound rather than merely convenient, for two reasons that are both pinned by tests. The pass retains any pid that is signalable AND a thread-group leader, which every session this process spawns is, so a session started while it runs cannot be pruned by it. And it is the one pass that does not rewrite the shared `kiro_session_pids.txt`, so it cannot race a spawn appending to that file -- which is what makes it, specifically, the pass that is safe to move while passes 1 and 4 stay on boot. Running beside live sessions does expose one race, and closing it is what makes the placement defensible. The leaders set is read ONCE for the whole pass, so a pid recycled after that read -- whose new owner has already republished a mapping at the same path -- is absent from the snapshot while naming a LIVE session. Deciding on the snapshot alone would unlink that mapping, and losing a live session's identity breaks session-keyed MCP resolution rather than merely tidying a file. So absence from the snapshot now selects a CANDIDATE only, and the decision takes a reading for that one pid: new `platform_compat.is_thread_group_leader()` reads `Tgid` from `/proc/<pid>/status` and the mapping is unlinked only on a definite "not a process", retained on True and on every inconclusive answer. The per-pid read is paid only for candidates, so the common path still does no per-entry file I/O, and the host-wide listing keeps doing the cheap filtering it was added for. A re-read alone still leaves a window, because the pid can be recycled between that reading and the unlink, so the sweep additionally holds a lock SHARED with `session_pid_sig.publish_session_pid` -- new `session_map_lock()` -- across the decision and both unlinks. A publish therefore either lands before the sweep judges that path or waits until the sweep is finished with it, so an unlink can only ever remove a mapping the sweep actually judged. Publication is the only legitimate writer of these files, which is what makes one lock sufficient. It is taken once for the loop rather than per entry, and the body is a probe plus an unlink per entry, so the hold is short. The force-exit path does NOT take that lock. It runs inline in a signal handler and then calls `os._exit`, so a lock it could contend on is a handler that may never exit -- keeping the process alive exactly when a second Ctrl-C asked it to die. `cleanup_orphaned_sessions(prune_mappings=False)` skips the mapping pass there; killing leftover processes is what that path is for, and tidying mapping files is not urgent at force exit because the next sweep does it. (Pass 1's pre-existing pid-file lock is not a licence for a second one on that path: one blocking acquire being tolerated is not an argument for adding another.) The sweep is synchronous filesystem work end to end -- a glob to drive it, a `pid_exists` per entry, one `/proc` listing, and an unlink per pruned entry -- so the two call sites that run inside the orchestrator coroutine (startup and post-shutdown) now `await asyncio.to_thread(...)`. That also takes the pre-existing glob, probe and unlink work off the loop, so those paths are strictly less blocking than before this branch. The force-exit signal handler keeps the synchronous call, minus the mapping pass as above: a handler cannot await, and the process calls `os._exit` immediately afterwards, so loop latency is not meaningful there. Behaviour on macOS and Windows is unchanged. Tests: `live_thread_group_leaders()` unit coverage including every fail-open branch, and a behaviour test that recycles a real thread tid and asserts the mapping is pruned. Two pin the boot-path contract: the boot setting takes no `/proc` read (the leaders helper is replaced with one that raises, so a regression to the narrowed call fails rather than silently costing a scan on every launch), and the deferred pass leaves `kiro_session_pids.txt` byte-equal while pruning a mapping. Six pin the race fix: `is_thread_group_leader()` on this process, on a real live thread's tid (which `pid_exists` calls alive -- the trap), on non-Linux, on a vanished pid and on a malformed `status`; plus a regression test that hands the pass a snapshot taken before this process existed and asserts the live mapping survives. Three pin the lock: the sweep's unlink lands between acquire and release, publication's write lands inside the same lock, and both sides resolve to the SAME lock object -- asserted on ORDER, since taking a lock and then acting after releasing it would still be the bug. Every new gate was checked by inversion -- four of them: removing the revalidation fails the race test, releasing the lock before the loop fails the ordering test, flipping the boot call to the narrowed form fails the boot test, and letting the force-exit path keep the mapping pass fails the force-exit test, which replaces the lock with one that raises so a regression cannot pass quietly. 463 passed / 23 skipped across the two touched test files, against 447 / 23 on `main`, plus 126 passed across the five suites that exercise the publish path and 426 across the gateway / spawn-offload / sweep-helper suites. `mypy src/kiro_crew/` clean over 1344 source files. Deliberately deferred: `_skip_tagged` (session_pid.py) still asks only `pid_exists` of the owning gateway pid, so a gateway pid recycled as a tid keeps its orphans from being reaped. It is the same class this change fixes one pass later and the leaders set is already in hand, but narrowing a kill-safety predicate changes what gets SIGKILLed rather than what gets unlinked, and that belongs in its own change with its own tests. The comment-history gate added in kirodotdev#9328 judges every file a diff touches. Three comments this branch adds narrated change history, which docs/system-specs/common/code-style.md already forbids; they are reworded to give the same reason without the historical framing. `slack/gateway.py` needed two beyond those: its baseline entry records 67 while the file scans 69 on pristine `main`, and has since the baseline was introduced, so any diff touching that file inherits the overage. Both spans retired there are ones kirodotdev#9338 also deletes, and both sit within 25 lines of a hunk this branch already changes.

… mapping `cleanup_orphaned_sessions()` prunes `session_pid_<pid>.txt` only when `platform_compat.pid_exists()` is false, and on POSIX that is literally `os.kill(pid, 0)`. Linux draws thread ids from the pid space, exposes `/proc/<tid>`, and lets you signal a tid -- so once a dead session's pid is recycled as a THREAD of an unrelated live process, the mapping passes the check and survives indefinitely. What that costs depends on the mapping. One that records a start token is already safe to resolve: `session_pid_sig._pid_recycled()` compares the live start token and refuses on a mismatch on both the strict and the lenient path, and a tid's live token cannot match the dead process's. A LEGACY token-less mapping has no recorded token for that guard to compare, so it keeps resolving for as long as the file lingers -- same-uid only, so robustness rather than a privilege boundary. Independently of either, nothing retires the files until the next gateway restart. Adds `platform_compat.live_thread_group_leaders()`: a single `os.listdir("/proc")` for the whole sweep. That listing enumerates ONLY thread-group leaders -- a non-leader tid is absent from it even though `/proc/<tid>` stays directly openable, which is exactly why the cheaper per-pid probes cannot tell the two apart. One directory read replaces a synchronous `/proc/<pid>/status` read per mapping, so the sweep performs no per-entry file I/O at all; measured on Linux that is 1.5 ms once against 7.8 ms across 233 mappings. It is also cheaper than `process_matches()`, which shells out to `ps` on macOS and so cannot be used per entry. The helper returns `None` -- never an empty set -- on non-Linux, on an unreadable `/proc`, and on a listing carrying no pids, and the caller treats `None` as retain-everything. An inconclusive answer can therefore never be the thing that decides a pid is stale; it only ever narrows an existing liveness check. The mapping files are materialized before the snapshot is taken, and that ordering is load-bearing rather than incidental: a mapping file is written at spawn, so every path in the list belongs to a process that already existed when the snapshot was read, and is therefore present in it and retained. Iterating the glob lazily instead would let an entry yielded AFTER the snapshot belong to a pid absent from it -- pruning a live session's mapping. The list is bounded by the mapping count the sweep was already reading per-entry (233 on the host measured above). The narrowing is asked for on ONE call site, and the sweep runs only where it already ran. The stale-mapping pass is now `prune_stale_session_pid_files(narrow_with_leaders=)`. The gateway's boot path and its force-exit handler both pass `False`, so each does exactly the work it did before this branch: `no-new-work-on-gateway-boot-path` names orphan sweeps specifically and the leaders snapshot is a `/proc` read that path may not carry, and a signal handler that reaches for extra work before its `os._exit` is a handler that may not get there. The graceful-shutdown sweep asks for the narrowing. That placement is the point rather than a compromise. Nothing is spawning a session by then, so the pass is not racing a mapping publisher, and it holds exactly the position the sweep already held -- so this change adds no concurrency that `main` did not already have, and needs no new lock, no new lock file in an agent-writable directory, and no work on a path that must stay short. The cost is that a recycled-pid mapping is retired at shutdown rather than mid-run; bounding accumulation across restarts is what the sweep is for, and resolution of a token-bearing mapping is already guarded by the recorded start token independently of it. Snapshot staleness is still handled, because the guarantee should not rest on the call site alone. The leaders set is read ONCE for the pass, so a pid recycled after that read would be absent from it while naming a live process. Absence from the snapshot therefore selects a CANDIDATE only, and the decision takes a reading for that one pid: new `platform_compat.is_thread_group_leader()` reads `Tgid` from `/proc/<pid>/status`, and a mapping is unlinked only on a definite "not a process" -- retained on True and on every inconclusive answer. That read is paid only for candidates, so the common path still does no per-entry file I/O and the host-wide listing keeps doing the cheap filtering it was added for. The sweep is synchronous filesystem work end to end -- a glob to drive it, a `pid_exists` per entry, one `/proc` listing, and an unlink per pruned entry -- so the two call sites that run inside the orchestrator coroutine (startup and post-shutdown) now `await asyncio.to_thread(...)`. That also takes the pre-existing glob, probe and unlink work off the loop, so those paths are strictly less blocking than before this branch. The force-exit signal handler keeps the synchronous call: a handler cannot await, and the process calls `os._exit` immediately afterwards, so loop latency is not meaningful there. Behaviour on macOS and Windows is unchanged. Tests: `live_thread_group_leaders()` unit coverage including every fail-open branch, and a behaviour test that recycles a real thread tid and asserts the mapping is pruned. Five cover the per-pid re-read: `is_thread_group_leader()` on this process, on a real live thread's tid (which `pid_exists` reports alive -- the trap), on non-Linux, on a vanished pid and on a malformed `status`. Three pin the contracts the call sites rest on: the `False` setting takes no `/proc` read at all, the pass leaves the shared `kiro_session_pids.txt` byte-equal while pruning a mapping, and a snapshot taken before this process existed does not cost the live mapping. Each of those was checked by INVERSION rather than only by passing -- removing the revalidation fails the snapshot test, and flipping the setting to the narrowed form fails the no-`/proc` test. 461 passed / 23 skipped across the two touched test files, against 447 / 23 on `main`; 500 passed across the publish-path, gateway, spawn-offload and sweep-helper suites; `mypy src/kiro_crew/` clean over 1344 source files. Deliberately deferred: `_skip_tagged` (session_pid.py) still asks only `pid_exists` of the owning gateway pid, so a gateway pid recycled as a tid keeps its orphans from being reaped. It is the same class this change fixes one pass later and the leaders set is already in hand, but narrowing a kill-safety predicate changes what gets SIGKILLed rather than what gets unlinked, and that belongs in its own change with its own tests. The comment-history gate added in kirodotdev#9328 judges every file a diff touches. Three comments this branch adds narrated change history, which docs/system-specs/common/code-style.md already forbids; they are reworded to give the same reason without the historical framing. `slack/gateway.py` needed two beyond those: its baseline entry records 67 while the file scans 69 on pristine `main`, and has since the baseline was introduced, so any diff touching that file inherits the overage. Both spans retired there are ones kirodotdev#9338 also deletes, and both sit within 25 lines of a hunk this branch already changes.

… mapping `cleanup_orphaned_sessions()` prunes `session_pid_<pid>.txt` only when `platform_compat.pid_exists()` is false, and on POSIX that is literally `os.kill(pid, 0)`. Linux draws thread ids from the pid space, exposes `/proc/<tid>`, and lets you signal a tid -- so once a dead session's pid is recycled as a THREAD of an unrelated live process, the mapping passes the check and survives indefinitely. What that costs depends on the mapping. One that records a start token is already safe to resolve: `session_pid_sig._pid_recycled()` compares the live start token and refuses on a mismatch on both the strict and the lenient path, and a tid's live token cannot match the dead process's. A LEGACY token-less mapping has no recorded token for that guard to compare, so it keeps resolving for as long as the file lingers -- same-uid only, so robustness rather than a privilege boundary. Independently of either, nothing retires the files until the next gateway restart. Adds `platform_compat.live_thread_group_leaders()`: a single `os.listdir("/proc")` for the whole sweep. That listing enumerates ONLY thread-group leaders -- a non-leader tid is absent from it even though `/proc/<tid>` stays directly openable, which is exactly why the cheaper per-pid probes cannot tell the two apart. One directory read replaces a synchronous `/proc/<pid>/status` read per mapping, so the sweep performs no per-entry file I/O at all; measured on Linux that is 1.5 ms once against 7.8 ms across 233 mappings. It is also cheaper than `process_matches()`, which shells out to `ps` on macOS and so cannot be used per entry. The helper returns `None` -- never an empty set -- on non-Linux, on an unreadable `/proc`, and on a listing carrying no pids, and the caller treats `None` as retain-everything. An inconclusive answer can therefore never be the thing that decides a pid is stale; it only ever narrows an existing liveness check. The mapping files are materialized before the snapshot is taken, and that ordering is load-bearing rather than incidental: a mapping file is written at spawn, so every path in the list belongs to a process that already existed when the snapshot was read, and is therefore present in it and retained. Iterating the glob lazily instead would let an entry yielded AFTER the snapshot belong to a pid absent from it -- pruning a live session's mapping. The list is bounded by the mapping count the sweep was already reading per-entry (233 on the host measured above). The narrowing is asked for on ONE call site, and the sweep runs only where it already ran. The stale-mapping pass is now `_prune_stale_session_pid_files(narrow_with_leaders=)` -- private, since it has one production consumer. The gateway's boot path and its force-exit handler both pass `False`, so each does exactly the work it did before this branch: `no-new-work-on-gateway-boot-path` names orphan sweeps specifically and the leaders snapshot is a `/proc` read that path may not carry, and a signal handler that reaches for extra work before its `os._exit` is a handler that may not get there. The graceful-shutdown sweep asks for the narrowing. That placement is the point rather than a compromise. Nothing is spawning a session by then, so the pass is not racing a mapping publisher, and it holds exactly the position the sweep already held -- so this change adds no concurrency that `main` did not already have, and needs no new lock, no new lock file in an agent-writable directory, and no work on a path that must stay short. The cost is that a recycled-pid mapping is retired at shutdown rather than mid-run; bounding accumulation across restarts is what the sweep is for, and resolution of a token-bearing mapping is already guarded by the recorded start token independently of it. Owned plainly: a gateway that is hard-killed never reaches the graceful path, so on that host its recycled-pid mappings wait for a later clean exit. Snapshot staleness is still handled, because the guarantee should not rest on the call site alone. The leaders set is read ONCE for the pass, so a pid recycled after that read would be absent from it while naming a live process. Absence from the snapshot therefore selects a CANDIDATE only, and the decision takes a reading for that one pid: new `platform_compat.is_thread_group_leader()` reads `Tgid` from `/proc/<pid>/status`, and a mapping is unlinked only on a definite "not a process" -- retained on True and on every inconclusive answer. That read is paid only for candidates, so the common path still does no per-entry file I/O and the host-wide listing keeps doing the cheap filtering it was added for. The sweep is synchronous filesystem work end to end -- a glob to drive it, a `pid_exists` per entry, one `/proc` listing, and an unlink per pruned entry -- so the two call sites that run inside the orchestrator coroutine (startup and post-shutdown) now `await asyncio.to_thread(...)`. That also takes the pre-existing glob, probe and unlink work off the loop, so those paths are strictly less blocking than before this branch. The force-exit signal handler keeps the synchronous call: a handler cannot await, and the process calls `os._exit` immediately afterwards, so loop latency is not meaningful there. Behaviour on macOS and Windows is unchanged. Tests: `live_thread_group_leaders()` unit coverage including every fail-open branch, and a behaviour test that recycles a real thread tid and asserts the mapping is pruned. Five cover the per-pid re-read: `is_thread_group_leader()` on this process, on a real live thread's tid (which `pid_exists` reports alive -- the trap), on non-Linux, on a vanished pid and on a malformed `status`. Three pin the contracts the call sites rest on: the `False` setting takes no `/proc` read at all, the pass leaves the shared `kiro_session_pids.txt` byte-equal while pruning a mapping, and a snapshot taken before this process existed does not cost the live mapping. Each of those was checked by INVERSION rather than only by passing -- removing the revalidation fails the snapshot test, and flipping the setting to the narrowed form fails the no-`/proc` test. 461 passed / 23 skipped across the two touched test files, against 447 / 23 on `main`; 500 passed across the publish-path, gateway, spawn-offload and sweep-helper suites; `mypy src/kiro_crew/` clean over 1344 source files. Deliberately deferred: `_skip_tagged` (session_pid.py) still asks only `pid_exists` of the owning gateway pid, so a gateway pid recycled as a tid keeps its orphans from being reaped. It is the same class this change fixes one pass later and the leaders set is already in hand, but narrowing a kill-safety predicate changes what gets SIGKILLed rather than what gets unlinked, and that belongs in its own change with its own tests. The comment-history gate added in kirodotdev#9328 judges every file a diff touches. Three comments this branch adds narrated change history, which docs/system-specs/common/code-style.md already forbids; they are reworded to give the same reason without the historical framing. `slack/gateway.py` needed two beyond those: its baseline entry records 67 while the file scans 69 on pristine `main`, and has since the baseline was introduced, so any diff touching that file inherits the overage. Both spans retired there are ones kirodotdev#9338 also deletes, and both sit within 25 lines of a hunk this branch already changes.

iamwhatever · 2026-09-09T07:18:50Z

span=c0f4646a1876 fixed — baseline shrink now declared in the description

The item is correct: the diff lowered comment-history-baseline.json and the body
never said so.
Fixed by adding a paragraph to "What changed" that states what the file is (the
ratchet check_comment_history.py reads), why the change is mechanically required
(cleaning a file drops its count, so the entry must be lowered or pruned in the
same change), and the numbers: 21 entries pruned, 22 lowered, none raised.
No code change; description only.

iamwhatever · 2026-09-09T07:18:57Z

span=f433a994cd7a fixed — leftover markers stripped, so the completeness claim holds

Checked every instance named and each was real, so the claim was at fault, not the
reviewer.
(#8150) at security/shell_normalizer.py:1055 and :1664 — stripped, keeping the
measured attack spellings ($(: # ), $(case x in x) printf token;; esac)).
"Three review rounds each closed one spelling" at acp/client.py:983 — rewritten to
"Closing one spelling at a time cannot close the class", which keeps the invariant
and drops the round count.
The GPT 5.6 / Opus 4.8 / Found in review / caught in review /
BLOCKING from provenance — 30 further sites across security/__init__.py,
security/argv_floor.py, security/paths.py, security/shell_normalizer.py — all
stripped. A further 54 sites in slack/gateway.py were redone after a rebase took
main's copy of that file.
Every verified attack spelling, every measured number and every threat-model
rationale is kept; only the provenance parenthetical is gone.
grep -rnE 'Found in review|caught in review|GPT 5|review round|BLOCKING from' over
the four packages now returns nothing.
Kept deliberately: acp/client.py:3198 names Opus 4.8/4.7, Sonnet 4.6 as MODEL
IDENTIFIERS in data, not as review provenance.

iamwhatever · 2026-09-09T07:19:02Z

span=a619e6111939 fixed — the subtraction was performed

Taken as written. This is the same subtraction the PR already performs on the
neighbouring comments, so leaving these behind made the PR internally inconsistent.
32 sites removed across security/__init__.py (7), security/argv_floor.py (8),
security/paths.py (2), security/shell_normalizer.py (14), acp/client.py (1).
The diff stays comments-and-docstrings-only: the executable token stream of all 43
changed files is byte-identical to the merge base with comments dropped and
docstrings normalised, verified per file.
flake8 clean on every touched file.

The code-style spec says a comment carries invariants, edge cases, units and the reason a surprising choice is correct -- not a task log. History lives in git. This applies that rule to the security, slack, acp and mcp_gateway packages, and is the companion to the same sweep over the top-level modules. Removed from comments and docstrings: PR, issue and ticket numbers, review-round and finding markers, and change-log sentences. Narration was rewritten into present-tense statements of what the code does now, so a reader can tell whether a comment describes the code in front of them. The threat-model reasoning in security/ is kept in full. Every verified attack spelling, every measured number, and every "why this apparently-redundant check is load bearing" survives; only the round and PR references around them are gone. Where a comment recorded why a rule is deliberately ABSENT, that argument is kept and restated in present tense, because it is what stops the rule being re-added. Comments and docstrings only. No executable code changed: the token stream of every file in this diff is identical to the base commit, with comments dropped and docstrings normalised.

iamwhatever requested a review from a team as a code owner September 8, 2026 01:40

iamwhatever requested a review from krishdhasmana September 8, 2026 01:40

github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 8, 2026

iamwhatever changed the title ~~chore(security,slack,acp,mcp-gateway): strip history narration from comments~~ chore(subsystems): strip history narration from comments Sep 8, 2026

github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 8, 2026

iamwhatever force-pushed the chore/comment-audit-subdirs branch from df5b0f3 to 3077ad6 Compare September 8, 2026 01:58

github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 8, 2026

iamwhatever force-pushed the chore/comment-audit-subdirs branch from 3077ad6 to 7d00414 Compare September 8, 2026 02:14

github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 8, 2026

CrysisDeu mentioned this pull request Sep 8, 2026

Main ratchet drift detected #9350

Open

github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Sep 8, 2026

chenmingwei23 mentioned this pull request Sep 8, 2026

New internal-content-scan lane froze PR Readiness pending on 22 open PRs, and the sweep re-fires forever #9357

Open

cixuuz previously approved these changes Sep 8, 2026

View reviewed changes

This was referenced Sep 8, 2026

comment-history baseline drift census: 33 files exceed or miss their entries on main #9384

Open

chore(pipeline-conductor): state the argv constraint in present tense #9393

Merged

iamwhatever dismissed cixuuz’s stale review via 8aef022 September 8, 2026 07:07

iamwhatever force-pushed the chore/comment-audit-subdirs branch from 7d00414 to 8aef022 Compare September 8, 2026 07:07

github-actions Bot added readiness: checking Automated validation is still running and removed readiness: passed Eligible automated validation passed for the current revision readiness: checking Automated validation is still running labels Sep 8, 2026

github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 8, 2026

iamwhatever force-pushed the chore/comment-audit-subdirs branch from 204c777 to ebf47cb Compare September 8, 2026 23:22

github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 9, 2026

iamwhatever force-pushed the chore/comment-audit-subdirs branch from ebf47cb to ee524dc Compare September 9, 2026 01:24

github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 9, 2026

iamwhatever force-pushed the chore/comment-audit-subdirs branch from ee524dc to 6f430e0 Compare September 9, 2026 05:39

iamwhatever force-pushed the chore/comment-audit-subdirs branch from 6f430e0 to a0ee498 Compare September 9, 2026 06:37

github-actions Bot added readiness: checking Automated validation is still running and removed readiness: passed Eligible automated validation passed for the current revision labels Sep 9, 2026

Navigation Menu

Uh oh!

chore(subsystems): strip history narration from comments - #9338

chore(subsystems): strip history narration from comments#9338
iamwhatever wants to merge 1 commit into
mainfrom
chore/comment-audit-subdirs

iamwhatever commented Sep 8, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 8, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 8, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 8, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 8, 2026 •

edited

Loading

Uh oh!

iamwhatever commented Sep 9, 2026

Uh oh!

iamwhatever commented Sep 9, 2026

Uh oh!

iamwhatever commented Sep 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

iamwhatever commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem / Motivation

Why it matters

What changed (motivation → approach → change)

Tests

Manual verification

Screenshots / video

Related Issues

Pattern harvest

Checklist

Contribution License Agreement

Uh oh!

github-actions Bot commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

GPT 5.6 Review — ✅ no blocking findings

Uh oh!

github-actions Bot commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Opus 4.8 Review — ✅ no blocking findings

Uh oh!

github-actions Bot commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Design Review (Fable 5) — ✅ PASS

Uh oh!

github-actions Bot commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

First Principles Review (Fable 5) — ✅ PASS

What this change ships

Uh oh!

iamwhatever commented Sep 9, 2026

Uh oh!

iamwhatever commented Sep 9, 2026

Uh oh!

iamwhatever commented Sep 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

iamwhatever commented Sep 8, 2026 •

edited

Loading

github-actions Bot commented Sep 8, 2026 •

edited

Loading

github-actions Bot commented Sep 8, 2026 •

edited

Loading

github-actions Bot commented Sep 8, 2026 •

edited

Loading

github-actions Bot commented Sep 8, 2026 •

edited

Loading