feat(sel): bound the Security Event Log with rotation and retention

[rnoack1]

Problem / Motivation

The Security Event Log is an append-only, HMAC-chained audit file with no size bound, so on a long-lived host it grows without a ceiling. An age bound already existed and still does — the heartbeat prunes entries past retention_days (365) once a day — but nothing caps how large the log gets within that window, so event volume rather than elapsed time sets the file size. This adds size-based rotation with bounded retention, and extends the sensitive-path floor to cover the files rotation creates.

Verified on upstream/main before starting: src/kiro_crew/sel.py is 1179 lines with the chain intact (prev_hash x8, hmac x25, verify_integrity x5) and every rotation symbol absent — backup_count, max_bytes, retention_days, _maybe_rotate, evicted all zero occurrences. The defect is live today.

Why it matters

Every tool invocation, MCP call and dashboard mutation appends here, so on a busy long-lived host the single active file is the thing that grows unchecked, and both read paths made that size directly painful: at the base commit verify_integrity() and recent() each loaded the whole file into memory in one read_text() (sel.py:937 and :965), and those two methods back the /api/sel/verify and /api/sel/events dashboard endpoints plus kirocrew security verify / kirocrew security events. So an audit read got steadily more expensive as the log grew, on the exact hosts that have been up longest.

The honest severity is moderate and slow-burn — disk consumption and read cost on old hosts, not corruption, lost events, or a security hole — and the fix bounds the problem rather than eliminating it: verification still walks every retained segment, now capped near 600 MB at the shipped defaults instead of unbounded, while recent() normally touches only the active file.

What changed (motivation → approach → change)

Rotation seals the active audit file into numbered segments under a sel/ subdirectory and keeps a bounded number of them, and security.py adds that one directory to the sensitive-path floor so every segment inherits the protection the base file already had. The two halves ship together because splitting them would create an exposure (see "Why security.py is in the same PR").

Segment numbers are monotonic: allocated once, never renamed, never reused. A higher number is a newer segment, so eviction deletes the lowest numbers. That single choice is what removes the machinery earlier revisions of this PR needed — there is no shift-rename sequence on each roll, so no cross-process rotation lease to serialise it; no temp-name staging, so no crash-left residue class to detect and refuse; and no dot-suffixed siblings of the active file, so no prefix-family regex replicated across four matchers and no second accessor for consumers to remember. See "Layout decision" below for why it landed here rather than in a follow-up.

How rotation keeps the chain verifiable

When the active file exceeds max_bytes it is sealed into sel/security_events.jsonl.<N>, where N is the next unused number, and the lowest numbers are deleted once more than backup_count segments exist. The HMAC chain is deliberately not re-anchored: the fresh active file's first entry chains off the just-sealed segment's tip, so verify_integrity() and recent() walk every segment oldest-to-newest as one continuous stream.

Concurrency is handled by the numbering rather than by a lock. Each process claims its number with O_CREAT|O_EXCL, so two processes rolling at once cannot take the same one, and the seal is a single os.replace of the active path — no existing segment is ever a rename source. The loser of the race finds the active file already moved and discards its claim. The worst concurrent outcome is one extra small segment. CORRECTED in round 13 below: that claim was wrong -- the atomic claim does not order the seal, so a lower number could receive newer data. A cross-process lease now spans claim + replace. Worth stating plainly because it is easy to get wrong: the atomic claim is load-bearing, not decoration. A plain max(existing) + 1 is a read-modify-write, and two processes would compute the same number.

The one boundary that cannot be linkage-checked is the oldest surviving entry. Once older segments have been evicted its prev_hash legitimately points at an entry no longer on disk, so verification adopts that value as the chain baseline instead of forcing it to "" — otherwise every host that had rotated past backup_count would report a false "chain break at entry 1".

That relaxation is gated only on a sticky sel/evicted marker, written whenever a segment is really deleted. Gating on "does a sealed segment exist" instead would be weaker: a host that has rotated but never evicted anything still holds its genesis entry in the oldest sealed segment, so the genesis anchor must still be enforced there. As it stands, head-truncating that oldest segment surfaces as valid<total. The gate is the marker alone and is deliberately not combined with max_bytes > 0, so an operator who evicts under rotation and then sets max_bytes=0 keeps the relaxed baseline — the physical chain still lacks its genesis prefix, so re-enforcing genesis there would false-alarm.

Other invariants worth calling out for review:

• Verify pins each segment by an open handle. _walk_chain opens every segment under _lock, then _walk_handles does the reading and HMAC work after the lock is released. Pinning is a correctness requirement, not an optimization — see "Review round 2" below.
• The .evicted marker is authenticated. Its contents are a domain-separated MAC under the SEL key, compared in constant time; anything missing, empty, or non-matching reads as no marker (fail-closed).
• A missing MIDDLE segment never reads clean. The walk covers every numbered segment on disk, so a hole makes the next segment chain off a deleted entry: the mismatch lands in total and never in valid. This needs no separate bookkeeping. A gap at the BOTTOM of the numbering is not a fault at all — that is what ordinary eviction leaves behind, which is why segment numbers are sorted numerically rather than lexically (.10 must not sort between .1 and .2).
• Eviction deletes the OLDEST, and the direction is pinned by a test. With monotonic numbers the oldest segment is the lowest, so eviction takes a prefix. The previous layout was the reverse — the oldest carried the highest index — so applying the old rule here would silently delete the newest audit history and keep the oldest. TestEvictionDeletesTheOldestNotTheNewest asserts the direction rather than only asserting that the count is bounded, and it fails if the ends are ever flipped back.
• Chain-tip discovery is bounded. _tip_hash_of scans a segment backward for the last complete record, holding back the bytes it has not yet split into whole lines. A segment with no newline never yields a complete line, so that buffer would grow to the size of the file — reached from the constructor. The scan is floored at 1 MiB; on giving up it logs at ERROR and reports no tip for that segment, so _read_last_hash falls through to an older one rather than silently re-anchoring the chain to genesis. The age scan _newest_timestamp_of is bounded by the same floor (round 14), and additionally trims its buffer each step so the scan is linear rather than quadratic inside that window.
• Destructive paths fail closed. An unparseable timestamp means "cannot prove this is aged", so the entry or segment is kept. Age-pruning also stops at the first segment it cannot prove is aged, because eviction has to be a prefix operation — segment timestamps are only weakly monotonic, and dropping a middle segment would leave the next one chaining off a deleted entry.
• The backup_count=0 discard leaves a seam with TWO legitimate anchors, and both are adoptable at most once. That path truncates the active file rather than unlinking it, so a rival process holding an O_APPEND fd keeps writing into the live file; its prev_hash then names the tip the truncate destroyed, which the discard records under a domain-separated MAC. The discarding process ALSO re-anchors itself to genesis and carries on logging, so the file can hold a genesis-anchored chain and a destroyed-tip-anchored chain in either order. verify therefore treats "" and the recorded tip as re-anchor points at any position, each usable once per walk, with its own self-HMAC still checked. Everything else still breaks -- including a head truncation on a log that HAS discarded -- and with no authenticated record nothing is adoptable at all.
• Rotation failure cannot block an append. A failure (disk full, EPERM) is swallowed so the audit append always proceeds, but it increments kirocrew.sel.rotation_failed.count so a persistent failure — which would silently degrade back to unbounded growth — stays visible. It is a counter rather than a SEL event because rotation runs inside the writer thread mid-flush, and it is emitted after _lock is released so a slow metrics backend cannot stall the writer.

Why security.py is in the same PR

Rotation turns one file into a family. security_events.jsonl was already on the sensitive-path floor as an exact-name entry, which covers only the base file — so shipping rotation alone would leave the sealed segments, which hold the bulk of the audit history including the oldest evidence, outside the read/write gate the base file has. The port itself would create the exposure, which is why the two halves are not separable.

The fix is one line of list membership: sel joins _CREW_SECRET_LEAVES, which puts <crew>/sel on the floor. The membership test already treats a registered path as a subtree (cand == base or cand.startswith(base + os.sep)), so every matcher that derives from the sensitive list — the tool gate (is_sensitive_path / is_sensitive_write_path), the POSIX bash matcher, the Windows-native spelling matcher, and the relative-traversal symlink-staging matcher — covers every segment without being taught anything new.

The property that matters most is what this covers that an enumeration could not: a segment number no release has ever emitted is protected the moment it is created. An earlier revision of this PR carried a prefix-family regex plus a second accessor (sensitive_home_prefix_families()) that consumers had to remember alongside sensitive_home_dirs(), and each consumer could only ever hide the sibling names that already existed. Two consumers had already missed it — the LaTeX hide-set and the posture surface — and one of them was the subject of a blocking review finding: it globbed existing siblings once, so a segment sealed after that snapshot stayed readable to the TeX compiler. Naming the directory removes the window rather than narrowing it, and security_posture.py ends up byte-identical to the base commit because it needs no special case at all.

The entry is derived, not hardcoded. It is expanded over the same _CREW_HOME_PREFIXES the other leaves use and resolved through the same _home_dir_targets() helper, so it inherits both-homes coverage (~/.kiro/crew and the legacy ~/.kirocrew) and the existing KIROCREW_HOME re-anchoring. That matters because a hardcoded home-relative literal that failed to match the real home would silently un-protect the whole audit family with nothing failing — the gate would simply return False.

It is a path boundary, not a string prefix, so a similarly-named sibling such as sel2/ or selfie.txt is deliberately not swept in.

Tests

Full repository suite: 51,529 passed, 220 skipped, 6 xfailed, 4 failed + 1 error. Those five are pre-existing and were proven so rather than asserted: the same five reproduce with identical signatures on a clean checkout of the parent commit with no local modifications (test_dashboard_handlers_core_coverage.py pip-channel and STT-capability cases, test_gateway_lock_diagnosis.py::test_flock_is_held_by_a_fork_orphan, and a StopIteration error in test_wecom_client_cov80.py). None of them is in a file this PR touches.

Affected suites together (test_sel, test_security, test_security_posture, test_papyrus_latex, test_live_target, test_sandbox_argv, and the three deny suites): 1,257 passed, 2 skipped, 0 failed. All blocking lint steps clean at the versions CI pins: isort --check-only, flake8, and mypy src/kiro_crew/ (Success: no issues found in 1004 source files; the count tracks the base, which has moved on, so it is reported as measured here rather than asserted equal to CI's).

Measured end to end at max_bytes=300, backup_count=5: 40 events produce the active file plus sel/security_events.jsonl.35 through .39 and a sel/evicted marker. Numbers keep rising and the surviving five are the newest, which is the observable signature of eviction taking the oldest.

Each new invariant is negative-controlled, each control in its own run, and each was confirmed to fail for the intended reason:

revert	the guard that fires
evict the highest numbers (the old rule)	nothing was evicted from the bottom — the oldest segment survived
remove the tip-scan floor	the backward scan is unbounded: read 1572864 bytes
ignore concurrent growth in the active-file prune	the aged entries were rewritten away instead of the cycle being skipped
drop the `O_CREAT	O_EXCLclaim for a plainmax+1`

sel.py was restored byte-identically after each control, with zero sabotage markers left in the tree.

Two properties are pinned specifically because they are the ones that would fail silently. The eviction-direction test asserts which end is deleted, not merely that the count is bounded. And a test builds the LaTeX hide-set before sealing a segment and then confirms the segment is covered, which is what distinguishes a directory entry from a sibling glob.

The change was also negative-controlled against unmodified upstream/main, which establishes the security half is load-bearing rather than decorative: on the base commit is_sensitive_path returns false for a sealed segment path while returning true for the base file.

One finding from that exercise is worth flagging rather than hiding. Removing the segment directory from the POSIX bash alternation breaks no behavioural test, because win_sep is [\\/] — it accepts a forward slash, so the Windows-native branch also matches POSIX spellings and shadows the POSIX one. That shadowing is incidental, not designed: tightening the Windows matcher to backslash-only would silently drop coverage for every POSIX spelling. Since the base entries are deliberately present in both branches for the same reason, the directory stays in both, and TestSelSegmentDirPosixBranchIsPinned adds structural assertions so the POSIX branch's contribution is no longer silent.

Retiring the prefix family also strengthened one existing guard. TestSensitivePathAlternationSourcesAreNeverEmpty previously could only assert that what matched changed when the alternation source was emptied, because an empty family branch left a bare ~/.kiro/ satisfiable. With one source, emptying it makes the probe stop matching entirely, so the fail-open is now demonstrated outright.

Layout decision: resolved, and this revision is the resolution

Earlier revisions shipped a flat security_events.jsonl.1..N layout and flagged the choice as a one-way door that closed at merge, because migrating an audit surface after first release is not free. The Design lane's position — that merging as-is is the decision, made by default — was correct, and the owner made it explicitly: take the subdirectory with monotonic numbering now, while rotation is unshipped and migration costs nothing.

The evidence for it was this PR's own review history. Four separate blocking findings traced to the shift-rename convention rather than to independent defects: verify snapshotting paths that rebind to different inodes, .tmp_rot staging residue being adoptable into the chain, rotation not being atomic across writer processes, and the daily prune bypassing the lease that was added for it. Every fix written for those was compensatory. Monotonic numbering removes their shared premise — a segment's name is assigned once and never changes — so the lease, the residue class, the orphan accounting and the first-gap special case all go rather than being guarded. Sealing into a subdirectory removes a second cluster: the prefix-family regex across four matchers, its Windows twin, and the two-accessor split that had already caused two consumer misses inside this PR.

Stated so it is not oversold, because two things this does not fix:

• Multi-writer appends remain broken. Each process caches its chain tip in memory and never re-reads it; measured with rotation disabled, two processes writing 40 events each produced total=81 valid=8. That is fail-loud with every entry still on disk, and it needs a single-writer daemon or an append-path lease under either layout.
• The active file's own rewrite still has a residual window. See round 12 below.

Review round 23: three real defects, one of which deletes the live audit log

All three blocking items are confirmed and fixed. One of them is the most serious finding on this PR so far, and I nearly refuted it on a partial read — worth recording why.

BLOCKING — a non-canonical numeric suffix can resolve to the ACTIVE log (sel.py:1470). Accepted, fixed. I started to refute this: the lane says index 0 "resolves to the active file", and _next_segment_index returns 1 or more, so .0 looked unreachable. Reading _segment_path in full settles it the other way. Its own docstring says "Path of a sealed segment (index>=1) or the active file (index 0)", and the body is if index <= 0: return self._path. So index 0 IS the active log, by design, and the guard against a .0 ever being parsed was simply absent.

Measured end to end: a security_events.jsonl.0 planted in the segment directory with an aged stamp parsed to [0], _segment_path(0) compared equal to the active file, and one _prune_sealed_by_age call deleted the live log and its five entries. Nothing in this module writes .0, but an operator, a partial restore or a pre-upgrade layout can leave one, and the cost of accepting it is the current audit log.

The lane also asked, implicitly, about soundness separately from reachability, and that half is real too. .01 is a distinct FILE from .1 yet parses to the same index, so it inflates the eviction budget — one extra real segment deleted per roll, the same shape as the empty-claim bug in round 21 — while every path operation acts on .1 and .01 is what was listed. The fix answers both halves at once: require suffix == str(int(suffix)) and a value of at least 1, which rejects .0, .00, .01 and any other non-canonical spelling. No legitimate segment is refused, because a real one is always written as str(int). The existing isascii() guard stays and continues to exclude the Unicode digits isdigit() accepts.

BLOCKING — same-size replacement can discard concurrent events (sel.py:2436). Accepted, fixed. Nothing else serialises this path: Stage 2 holds _lock, which is in-process only, and it does not take the seal lease, so cross-process there is no protection at all — which is why this description already declared the window as the append-path remainder. What the lane adds is correct and narrower than the declared remainder: size equality is not file identity. A rival that rotates, sealing the active file away and letting appends recreate it, leaves a DIFFERENT file, and byte-size equality between old and new is a coincidence the check reads as "nothing changed", after which the stale os.replace discards the recreated file's events. Fixed by capturing (st_dev, st_ino) alongside the size and re-comparing both — the same discriminator _snapshot_drift uses against a reused segment number, and cheap for the same reason. Where a filesystem reports no usable inode both sides compare equal, so this degrades to the size check rather than false-skipping.

BLOCKING — retention counting bypasses the bounded reader (sel.py:912). Accepted, fixed, and this corrects a refutation I made in round 21. Round 21 declined this same code on reachability: _prune_sealed_by_age gates on _newest_timestamp_of, which fails closed when no timestamp parses, so a pathological segment supposedly never reaches the count. That argument only holds when the oversized line is the LAST one. A 6 MB line in the MIDDLE of a segment whose final line carries an ordinary aged stamp passes the gate cleanly — measured — and the old for ln in f then allocated it. The earlier refutation was incomplete and is withdrawn.

I did not take the lane's fix as written. Routing through _segment_lines caps each line but accumulates every line into a list, so it is O(file) in aggregate and would load a 100 MB segment whole — the opposite trade from what counting needs. The count now reads through _open_segment (picking up O_NOFOLLOW and S_ISREG like every other segment read) and caps each line at _SEGMENT_LINE_CAP while still streaming one line at a time, so it is bounded on both axes. On an over-cap line the count becomes a FLOOR, which is acceptable precisely because the count is observational — no caller gates on its exactness — and the alternative is the exhaustion the cap exists to stop. The recommendation engine's "Protect against denial of service" (BEST_PRACTICE) names this directly under Resource Exhaustion Protection, and "Coral framework — set a maximum decompression size in CompressionHandler" (BEST_PRACTICE) is the same shape: an unbounded read a caller can force to buffer.

FINDING — the function-local get_recorder import. Tenth re-raise, unchanged; rounds 17-19 and 21 carry the answer. Nothing spent.

Design Review CONCERNS — two decisions for the owner

Should the single-writer daemon be sequenced FIRST? The lane's observation is fair and I want it stated plainly rather than buried: this PR adds cross-process machinery (the seal lease, inode-based _snapshot_drift, handle pinning in verify) while this same description establishes that multi-writer APPEND already corrupts the chain — total=81 valid=8 measured — and names a single-writer daemon as the real fix. So in a genuine multi-writer deployment, verify is being made rigorous over a chain that append can still break, and if the daemon lands, some of the cross-process work becomes redundant.

My recommendation is to merge this now rather than sequence the daemon first, for three reasons. The lane itself carves out the single-process correctness work — the empty-claim sweep, the discard ordering, the eviction direction, and now the .0 alias and the bounded count — as standing on its own, and that is the majority of what these last several rounds fixed; none of it is throwaway. The size and retention bounds are the point of the PR and they are independent of writer count. And the cross-process pieces are small and load-bearing today for the rotation path even on a single-writer host, because the dashboard gateway and the MCP gateway daemon both construct SecurityEventLog() on a normal install, so rotation is already multi-process whatever happens to append. That said, the call is rnoack's, and if the daemon is close then deferring is defensible.

Do we accept three KIROCREW_SEL_* env vars as a public contract now? They govern deletion on an audit surface, and once shipped operators depend on them, so the promised sel config section must keep them working rather than replace them. I recommend accepting: KiroCrewConfig has no sel section today, reading one would be a static type error plus an unreachable branch (mypy rejected exactly that in an earlier revision), and shipping rotation with no operator control at all is worse than shipping it with env knobs. The follow-up adds the config section and slots it between env and the defaults in the single knob-resolution block, keeping the env vars as an override. Confirmation requested rather than assumed.

The lane's Suggestion needs no action and is being followed: the subdirectory + monotonic-numbering layout stays, and the security.py change stays bundled here because that bundling is what closes the unprotected-segment window.

Verification (round 23)

Eleven test definitions added (test_sel.py 231 → 242), which pytest collects as 15 cases because the suffix test is parametrized over five non-canonical spellings. Each fix is negative-controlled by reverting only that fix:

control	observed
shipped suffix parse	.0, .00, .000, .01, .007 all refused; .1/.2/.10/.137 still parse
old parse restored (isdigit() only)	reproduces the deletion — the ACTIVE log is unlinked by age-pruning
bounded count reverted to open() + for ln in f	fails: the 6 MB middle line is allocated
identity comparison reverted to size-only	fails: the stale rewrite replaces a file it never read
ordinary segment, no pathology	count is exact (7), blank line not counted, non-regular file returns 0

One test pins the premise itself — _segment_path(0) == self._path — so if that mapping ever changes, the guard's rationale gets re-derived rather than silently outliving it.

Affected suites: 830 passed, 1 skipped. mypy clean over 982 source files, flake8 and isort clean — the three blocking lint gates. sel.md documents all three fixes, including that the round-21 reachability argument for the unbounded count was wrong and why.

Review round 22: the event-loop stall does not survive measurement; the discard ordering hazard does

BLOCKING — rotation does blocking filesystem work on the gateway event loop (sel.py:452). Refuted on the CONSEQUENCE, not on the chain. The chain is real and I am not disputing any link of it. Line 452 is self._maybe_rotate(), exactly as cited. log(critical=True) calls self._flush_batch([event], raise_on_error=True) synchronously rather than queueing, so a critical write does reach rotation on the caller's thread. _handle_yolo at slack/events.py:373 is an async def on the gateway loop, and so.activate("slack") reaches _commit_activation, which writes with critical=True (safety_override.py:318) — with no to_thread or executor hop anywhere in between. And _maybe_rotate is genuinely NEW here: it has 0 occurrences at the merge base, while raise_on_error already had 6, so this PR does add rotation work to a pre-existing synchronous-on-the-loop write.

What does not hold is "scans files inline → gateway sessions and heartbeat stall". Measured on this workspace, with backup_count=5, retention_days=365, five 2 MB sealed segments and the active file driven to its cap so the expensive branch actually fires:

operation	cost
_maybe_rotate() under cap (the common case)	0.012 ms — one stat and return
_maybe_rotate() AT CAP, doing the full seal + evict + age-prune	1.9 ms
one critical SEL write with rotation OFF (pre-existing cost)	0.216 ms median, 0.242 ms p95

So the added worst case is 1.9 ms, it fires once per max_bytes of log growth (100 MB by default), and it is about nine times a single write on a call that was already doing synchronous file IO. It is bounded by construction rather than by luck: the tip scans are capped at _TIP_SCAN_MAX_BYTES (1 MB) each, and _seal_leased deliberately passes count=False so the full-segment _entry_count_of scan never runs on the rotation path. There is no unbounded inline scan to find.

The lane's proposed fix is also the wrong direction. Skipping rotation for raise_on_error writes and leaving it to the background writer means a process whose only SEL traffic is critical writes never rolls at all — the active file grows past max_bytes forever, which is precisely the unbounded growth this PR exists to prevent and which the seal path already names as the condition it must never silently degrade into. Trading a measured 1.9 ms once per 100 MB for an unbounded log is not a trade worth making. No code spent; if the repo wants SEL IO off the loop entirely that is a call-site change across every critical=True caller, and it is a different piece of work from this one.

BLOCKING — discard deletes active events before sealed cleanup succeeds (sel.py:646). Accepted and fixed. This is NOT the round-21 argument. Round 21 refuted a different mechanism at this same line — "concurrent discard loses appended audit events" — on the ground that backup_count<=0 is a documented knob that accepts recent-history loss. That argument does not answer this one, and re-reading the operator note with the right question shows why: it promises the loss of recent EVENTS ("drops up to max_bytes of the MOST RECENT events at the rotation boundary"), and says nothing about the CHAIN. Losing the chain is outside what the knob buys, so the documented-knob defence does not reach this claim.

Both premises verified. recent() takes _lock only to snapshot the segment list and then opens each segment OUTSIDE the lock, while _discard_leased runs under it — so a sealed segment really can be held open by a concurrent recent(), and on Windows that unlink fails with a sharing violation. missing_ok=True suppresses only FileNotFoundError, so that error is not masked; it propagates. And the old order was active-file-first, with the tip reset sitting AFTER the sealed loop, so the failure landed in the worst possible place: the active file already deleted, the sealed segments still on disk, and _last_hash not yet reset. _flush_batch then caught the error, logged "appending without rotating", and appended with prev_hash naming an entry that no longer exists anywhere on disk. The code's own comment says the genesis re-anchor exists to prevent exactly that, which is what makes the ordering the defect rather than the design.

Fixed by making the discard fail CLOSED: sealed segments are removed first, then the active file, then the tip is re-anchored with nothing fallible in between (a pure assignment cannot fail, so the tip can never be left naming a deleted entry). A sealed unlink that fails now aborts with every file and the tip untouched, and the roll is simply skipped and retried, which is the degradation _flush_batch already handles. The marker clear moved last on purpose: if it is the step that fails, the only effect is that the first-entry check stays relaxed, which is permissive rather than corrupt.

FINDING — the function-local get_recorder import. Ninth re-raise, unchanged, and rounds 17-19 and 21 already carry the answer. Nothing added.

Opus 4.8 lane on the prior sha: PASS with no findings. Its body reads "No findings" — the two "Candidate" paragraphs are Opus falsifying its OWN candidates, not live work, so there was nothing to fold in. Worth noting that it reached the same conclusion this description already held on one of them: the backup_count<=0 to discard mapping is "documented behavior, not a defect."

Verification (round 22)

Three tests added (test_sel.py 228 → 231). The simulated sharing violation keeps them cross-platform — they assert the ORDERING invariant, which holds on every OS, rather than reproducing WinError 32:

control	observed
shipped order, sealed unlink refused	active file still present, tip unchanged
OLD active-first order, same refusal	reproduces it — active file gone, tip left naming the deleted entry
clean discard, no refusal	active and sealed both gone, tip re-anchored to "", marker cleared

Affected suites: 815 passed, 1 skipped. mypy clean over 982 source files, flake8 and isort clean — the three blocking lint gates. sel.md now records the ordering invariant and, explicitly, that the backup_count=0 contract covers recent events and not the chain.

Review round 21: one real crash-residue defect, one refutation whose proposed fix would undo this PR

BLOCKING — concurrent discard loses appended events (sel.py:646). Refuted, and the proposed fix is the wrong direction. Verified at source: line 646 is self._path.unlink(missing_ok=True) inside _discard_leased, reached only from the if self._backup_count <= 0: branch at line 620. The question that decides this is what backup_count<=0 MEANS here, and the class answers it in two places. max_bytes<=0 is already the rotation off-switch — _maybe_rotate returns immediately at line 577 — and backup_count<=0 is a separate knob whose contract is spelled out in the operator note above the unlink: "backup_count=0 means keep at most one active file and drop everything else on roll; use it only where that recent-history loss is acceptable (keep backup_count>=1 to retain a sealed tail)." So the unlink is the documented behaviour of the knob, not an accident of it.

The lane proposes treating backup_count<=0 as rotation DISABLED. That would do two bad things. It duplicates max_bytes<=0, which already occupies exactly that role, leaving two knobs with one meaning and no way to ask for "roll but keep nothing". And it reintroduces unbounded growth for those operators — the active file would then grow past max_bytes forever, which is the precise failure this PR exists to prevent and which the seal path calls out by name as the condition it must never silently degrade into.

What survives is the concurrent-fd window, and that is the declared multi-writer append remainder, already dispositioned in round 17 with the sub-case named: on the SEAL path a concurrent appender's bytes land on the renamed inode and survive as segment N, whereas on the DISCARD path the inode is unlinked and they are gone, so discard is the one variant where the loss is unrecoverable. That distinction is already written down. Closing it needs an append-path lease, which this description scopes out. Superseded on the mechanism — see the later paragraph in this description that records the change. The discard path no longer removes the active file; it empties it in place, so a concurrent appender holding an O_APPEND fd still writes into the live file and those bytes survive on disk instead of going with a detached inode. What is recorded here is the reasoning as it stood in that round, kept for the history; the later paragraph is authoritative on what ships.

BLOCKING — a crash-left claim causes premature audit eviction (sel.py:1508). Accepted, fixed, and it was worse than the lane described. Verified at source: line 1508 is the os.open(..., O_CREAT | O_EXCL | O_WRONLY, 0o600) claim, and it runs BEFORE the os.replace that fills it, so a process killed in that window leaves a zero-byte segment. Both in-process failure paths in _seal_leased unlink it, but nothing cleans up after termination, and there is no zero-byte check anywhere in the listing or eviction path.

The lane called it "premature eviction". It is also permanent and silent, which I established rather than assumed. The residue holds no entries, so the chain walks straight through it and verify reports total == valid — it does NOT surface as a chain break, contrary to what the seal-path comment implied — and _newest_timestamp_of yields no stamp, so age-pruning fails closed and keeps it forever. So the budget is short by one for the life of the install, with no operator signal at any point. Measured at backup_count=3 with segments 8, 9 and 10 retained: a zero-byte claim at 11 evicted segment 8 along with its 439 bytes of real audit history, and verify still read total=3 valid=3.

Fixed by _drop_empty_claims, which unlinks the residue and returns only the numbers holding history, before the budget is computed. Deleting is lossless by definition and safe specifically here: _evict_over_budget and _next_segment_index have exactly one caller each, both inside _seal_leased under _lock AND the cross-process seal lease, so no rival process can be holding a live claim at that moment — only the crash-left kind reaches it. A stat failure deliberately keeps the number, because this is a budget input and guessing that an unstattable segment is empty would evict real history. Truncation of a REAL segment to zero is a different case and stays loud either way: its successor's prev_hash still names the tip that was truncated away, so the chain breaks and valid < total.

FINDING — the function-local get_recorder import (sel.py:533). Declined; this is the eighth re-raise and rounds 17-19 are the answer. The line is where the lane says it is and the code is unchanged. Nothing new is argued: the rule is documented blocking: false, hoisting adds 81 modules to a module constructed on the boot path via provider.py:42's module-scope config.loader import, and the cycle it holds open is named in metrics/provider.py's own docstring and at acp/client.py:3321.

Opus advisory — _entry_count_of has no per-line cap (sel.py:889, not 840). Refuted as unreachable, with the chain read at source. The lane's own refutation of the symlink half is correct, and the surviving half does not reach either. _entry_count_of has exactly one live caller, _prune_sealed_by_age at line 839, and it sits behind a gate: _newest_timestamp_of runs first and, if no timestamp parses, the loop breaks and fails closed. A segment pathological enough to matter here — one enormous line with no newlines — is exactly the shape whose trailing timestamp cannot be found inside _TIP_SCAN_MAX_BYTES, so it fails that gate and _entry_count_of is never called on it. The function also iterates the handle line by line rather than reading the file whole, so it does not violate the bounded-read invariant round 17 established (which forbids whole-segment .read()), which is why the structural guard does not flag it. No code spent; note the line number, 889 rather than 840.

Verification (round 21)

Four tests added (test_sel.py 224 → 228). The negative control is what makes the set discriminating, since at budget with no claim there is nothing to evict either way:

control	observed
shipped sweep	segments 8-10 all survive; the empty claim at 11 is removed
_drop_empty_claims patched to identity	reproduces the eviction — a valid segment goes
sweep run against three non-empty segments	returns all three; nothing dropped
verify after the sweep	total > 0 and valid == total — removing the residue does not sever the chain

Affected suites: 812 passed, 1 skipped. mypy clean over 982 source files, flake8 and isort clean — the three blocking lint gates. sel.md documents the sweep, including that the residue does not read as a chain break, which the seal-path comment previously implied it would.

Review round 19: the stability check I added in round 18 was identity-blind

BLOCKING — a reused segment number defeats the round-18 stability check. Accepted, fixed. This is a finding against round 18's own fix, and it is right. That check compared the sealed NUMBER set before and after opening. Numbering is monotonic only while a sealed segment SURVIVES: _next_segment_index is max(existing)+1 and falls back to 1 on an empty set, so once the last segment is pruned the next seal REUSES its number. The number set can therefore be identical across the snapshot while a number names a DIFFERENT file — and the handle already pinned keeps reading the unlinked inode.

The consequence is worse than the tail staleness round 18 described, and it is why this earns an fstat rather than a refutation. Measured on an aged .1 pruned and the active file resealed onto 1: total=6 valid=6, i.e. integrity: ok, over the 6 entries that had just been evicted, while the 3 entries that were actually retained on disk were never read. Verify vouched for history that is gone and skipped the history that is there. That is a substitution, not a stale count.

Fixed by judging stability on IDENTITY. _snapshot_drift compares (st_dev, st_ino) captured by fstat at open against the path's current identity, alongside the number set. Two deliberate details. It degrades to the number-set signal where a filesystem reports no usable inode, rather than false-retrying. And a pinned-but-now-absent path is NOT treated as drift — that is an ordinary eviction and the handle still holds the bytes — so the check does not fire on the benign case it would be easiest to over-trigger on. The same helper now also drives the exhausted-attempts branch: previously that branch only folded in numbers that APPEARED, so a pure substitution would have fallen through it silently even after the retries ran out; it now marks every path the snapshot could not pin cleanly, so total > valid reports loud.

I did not adopt the lane's fix shape as given ("retry when pinned handles no longer match the identities at their current paths OR the active-file state changes"). The handle-identity half is exactly right and is what shipped. The active-file-state half is already covered: a seal of the active file ADDS a number, which the set comparison catches, and a prune() Stage 2 rewrite replaces the active inode, which the identity comparison catches — so a separate active-file predicate would be a third spelling of two checks that already fire.

FINDING — the function-local get_recorder import. This is the seventh re-raise, at a new line number, and rounds 17 and 18 are the answer. The line moved 523 → 533 because round 18 edited the comment above it; the code is unchanged. Both sites (533 and 540) were already identified when this was first worked. Nothing new is argued: AUTOSDE top-level-imports is documented blocking: false, hoisting adds 81 modules to a module constructed on the boot path (provider.py:42 imports config.loader at module scope), and metrics/provider.py's own docstring plus the # circular import note at acp/client.py:3321 name the cycle this shape exists to hold open — config.loader -> acp.types -> acp.client -> metrics.provider -> config.loader, with config/loader.py:3686 importing sel back. Zero code spent.

Verification (round 19)

Four tests added (test_sel.py 220 → 224). The two scenario tests are POSIX-only, for the same reason TestVerifyPinsSegmentsByHandle's are: they must unlink a segment while verify holds it OPEN — the pinned handle reading the unlinked inode IS the mechanism — which Windows refuses with WinError 32, so the substitution cannot be constructed there at all. The first revision of this round did not gate them and the Windows shard caught it: the PermissionError surfaced through the patched _open_segment, was absorbed by the except OSError in _walk_chain, and the active file was counted UNVERIFIED — total=7 valid=6, the product failing CLOSED exactly as designed while the harness precondition never landed. _snapshot_drift is therefore also driven DIRECTLY by a cross-platform test, so the discriminator stays pinned on every platform; that test is RED without the identity check (it returns no drift for a substituted file). The negative control is the one that pins the fix to identity rather than to the retry loop, since the retry already existed and did not catch this:

control	observed
shipped fix	total=3 valid=3 over the 3 RETAINED entries
drift judged by NUMBER only (_snapshot_drift patched)	reproduces the false clean: total == valid with a retained-set size that disagrees
uncontended verify	total=9 valid=9 — the identity check does not make a quiet log dirty

Affected suites: 807 passed, 1 skipped. mypy clean over 982 source files, flake8 and isort clean — the three blocking lint gates. sel.md is corrected in the same revision: its round-18 wording ("redo if a number appeared") described the check this round replaced.

Review round 18: both blocking findings were real, and one of them was the silent kind

Both GPT blocking items are confirmed and fixed in this revision. They are the same class of bug — a guard that is real but does not run on the path that needs it — and in both cases the existing tests passed throughout, which is the part worth reading.

BLOCKING 1 — a planted segment-dir link is still trusted on the READ paths. Accepted, fixed. The write-path guard has been here since the layout change: _ensure_segment_dir() removes a planted sel symlink or junction (the link, never its target) and creates a real directory, and two tests cover it. What neither test covers is that the guard is only reached from _rotate_now and _next_segment_index. _list_sealed_indices — which verify_integrity, recent and prune Stage 1 all reach — called the raw _segment_dir() and listed straight through the link. With rotation off, or simply not yet due, nothing repairs it before those three read it. The consequence is not abstract: iterdir enumerates the link TARGET, so any file there named security_events.jsonl.<n> is treated as this log's own sealed segment, surfaced by recent() as audit events, and unlinked by eviction and age-pruning, both of which delete whatever the listing returns. Pointing one install's sel at another's is the realistic aim, since that is where files with those names actually exist.

Fixed by refusing in _list_sealed_indices itself, which is deliberately NOT the fix the lane proposed. The lane said to call _ensure_segment_dir() before listing; that helper MUTATES — it unlinks, it mkdirs, and it raises OSError when the result is still not a directory — so calling it from here would make a documented read-only verify_integrity() write to disk and raise into dashboard callers that have no handler for it. The read side gets the non-mutating half instead: same refusal, no side effect, returning no segments, fail-closed in the same shape as _has_evicted. Rotation still repairs the link, because the two write-path callers keep the guard.

BLOCKING 2 — verify is not synchronized with a cross-process seal. Accepted, fixed, and this one failed silently. _lock is a threading.Lock, and this module already documents that SEL has more than one writer process on a normal host — the dashboard gateway and the MCP gateway daemon each construct SecurityEventLog() with no base_dir and resolve the same file. Handle-pinning fixes the inodes the walk reads; it does nothing about a segment that was never in the listing. A rival seal os.replaces the ACTIVE file onto a fresh number and only recreates it on the next append, so verify opening the active path inside that window gets ENOENT, cannot lstat it, and takes the ordinary "no active file yet" branch — while the entries that were in it now live in a number the listing predates. Everything actually opened then validates. Measured against the unfixed code: total=19 valid=19, i.e. integrity: ok, with all 20 entries still on disk. The one failure mode this log exists to detect, reported as clean.

Fixed by taking the snapshot until it is STABLE — re-list after opening, redo if a number appeared, bounded at three attempts — and once those are spent, counting the numbers that appeared as UNVERIFIED so total > valid reports loud instead of clean. Again not the lane's proposed fix, which was to hold the seal lease and fail loud if unavailable. That lease is non-blocking BY DESIGN so rotation skips a roll rather than waiting on the writer thread; a reader holding it would both block the writer and turn every concurrent roll into a verify failure, which is a worse trade than one extra readdir. It is also not the retry loop round 16 removed: that one keyed on an OSError from a vanished path, a signal that never fired, whereas this keys on the segment set CHANGING, which is the thing that actually varies.

FINDING — the function-local get_recorder import. Declined again; this is the sixth re-raise and round 17 is the answer. Nothing new is argued here, and the figures were re-verified rather than copied: sel pulls 190 modules and hoisting adds 81 more, because provider.py:42 imports config.loader at module scope. AUTOSDE top-level-imports is documented blocking: false in this repo, and the deviation does not fit any of its three literal exceptions, so it is a judgement call rather than a defect — the judgement, with the precedent cutting both ways (D-76 declined on measured boot cost, D-126 and D-136 hoisted where there was none), is in round 17. The one change made here is to sel.py's own comment, which had been carrying the weaker reason: it now records the measured cost so a seventh reviewer finds the argument at source instead of only in this description.

Design Review CONCERNS — no code spent, and all three Watch items were already on record. They are merger-acceptance calls rather than defects, and each is stated where a merger will look: the downgrade one-way door under round 3's "no downgrade story" disposition, the three KIROCREW_SEL_* variables becoming surface before a sel config section exists under round 4's operator-control change and the "No config coupling" scope note, and the Stage-2 same-size append window under the declared remainders and round 12's BLOCKING 3. Nothing was added for them beyond this pointer.

Verification (round 18)

Six tests added (test_sel.py 214 → 220). Each fix is negative-controlled by reverting only that fix:

control	observed
read-side link refusal reverted	3 failed, 2 passed — the three new read-path tests fail while both pre-existing write-path tests still pass, which is why the existing coverage could not see this
stable snapshot reverted (single pass)	verify accounted for 19 of 20 entries: the segment the rival seal created was omitted from the snapshot
stable snapshot reverted, retries pinned to 1	total==19 valid==19 reads as integrity: ok
uncontended verify (negative control)	passes against the UNFIXED code, so it cannot mask either defect

The read-path tests carry a positive control: the same planted file in a real directory IS listed ([1]), so the refusal is attributable to the link rather than to the filename or the timestamp. The prune test plants an AGED, parseable stamp on purpose, because _prune_sealed_by_age fails closed on a stamp it cannot parse and an unparseable one would let the deletion assertion pass with the guard removed.

Affected suites: 804 passed, 1 skipped. mypy clean over 982 source files, flake8 and isort clean — the three blocking lint gates. black --check is disabled in CI by a documented repo-wide backlog, and it reports the same reformat on this file at the merge base, so it is unchanged by this revision. sel.md is updated in the same revision: the verify bullet's "the remaining hazard is an UNLINK" was made accurate rather than left contradicting the code, and the segment-dir guard is now documented in both directions.

Review round 17: the cap reached one of three reads, and the local-import refusal was resting on the wrong reason

Two more unbounded segment reads. Accepted, fixed. Round 16 bounded the verify walk and described it as the third instance of one class. That was true and still incomplete: _count_entries_in and recent() each slurped a whole segment too, so the planted file kept two other ways in and the "one class" framing read as if it were closed. All three now go through _segment_lines, the same way every segment OPEN goes through _open_segment, and for the same reason — bounding one site by hand left the others open, which is exactly how this recurred.

A structural test is the part that actually prevents a fourth. Per-site behavioural tests cannot catch a call site that does not exist yet, so one test now asserts that no bare whole-segment read survives anywhere in the module, with a positive control proving the detector can see the bounded form it permits.

recent() also carried a second, unrelated hazard that the switch closes. It decoded with a bare decode("utf-8"), and UnicodeDecodeError is not an OSError, so it escaped the handler directly above it: one non-UTF-8 byte in any segment took down the events endpoint. The shared reader decodes with errors="replace", and a test pins it.

backup_count=0 discard, revisited — the refutation stands, with one sub-case it did not name. The earlier answer rested on this: the path's contract is to delete the active file, so an append arriving microseconds before the unlink is inside that deletion rather than a new harm, and full serialisation means an append-path lease, which this description already scopes out. That still holds. But it under-described one thing, and the distinction is worth having in writing: on the SEAL path a concurrent appender's bytes land on the renamed inode, which survives on disk as segment N and is recoverable, whereas on the DISCARD path the inode is unlinked and they are gone. So the discard case is not merely a smaller instance of the declared multi-writer remainder — it is the one variant where the loss is unrecoverable rather than merely mis-chained. It is still inside what backup_count=0 promises to delete, so no code changed; but the declared-remainder paragraph now says so explicitly instead of leaving a reader to assume the seal-path reasoning covers both. Superseded on the mechanism — see the later paragraph in this description that records the change. The discard path no longer removes the active file; it empties it in place, so a concurrent appender holding an O_APPEND fd still writes into the live file and those bytes survive on disk instead of going with a detached inode. What is recorded here is the reasoning as it stood in that round, kept for the history; the later paragraph is authoritative on what ships.

Function-local get_recorder import — still declined, and the reason it has been declined on was wrong. Earlier rounds refused the hoist by citing metrics/provider.py's contract for callers inside config.loader's import chain, which reads as though a top-level import would fail. sel.py's own comment says the opposite in as many words: the cycle is not import-time-fatal and a top-level import "loads cleanly in all three orders", measured. Repeating a reason the code beside it disclaims is worse than having no reason, so here is the real one, measured the way this repo's own D-76 entry measured its equivalent: importing sel pulls 190 modules, and adding metrics.provider at module scope pulls 81 more, because provider imports config.loader at module scope (provider.py:42). sel is constructed on the gateway boot path, by the MCP server and by every CLI invocation, and it deliberately keeps its import side effects minimal for that reason — the same reason _default_dir() resolves config_dir() lazily.

The precedent cuts both ways and it is worth naming rather than only citing the half that agrees: D-136 and D-126 both hoisted function-local imports whose guard was except Exception rather than the narrow except ImportError the rule exempts, which is the shape here, while D-76 declined a hoist with the boot-path cost measured. This one has the measured cost, so it follows D-76. The rule is also documented non-blocking in this repo, so this is a judgement call rather than a defect — and the judgement is recorded here rather than left to the fifth re-raise.

Verification (round 17)

Five tests added (test_sel.py 227 → 232). Four negative controls, each reverting one site alone:

control	observed
_count_entries_in unbounded	assert 1 == 0
recent() unbounded	UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff
structural guard, count site reverted	unbounded whole-segment read(s) reintroduced: ['return sum(1 for ln in fh.read()...']
structural guard, recent() site reverted	unbounded whole-segment read(s) reintroduced: ['lines = fh.read().decode("utf-8")...']

Full repo suite: 55,717 passed, with 72 failures matching the count and distribution on a clean checkout of the same parent and none in a file this PR touches. mypy clean over 982 files, flake8 and isort clean, affected suites 900 passed.

Review round 16: three fixes, one refutation, and one guard that turned out not to be load-bearing

Segment numbers are now parsed ASCII-only. Accepted, fixed. The listing accepted any suffix str.isdigit() liked and handed it to int(), and those two do not agree. The disagreement fails in two directions, which is why the narrow-looking fix matters. A superscript is isdigit() but int("\u00b2") raises, so a planted name crashed the listing — and the listing is reached from rotation, from verify and from recent(). The quieter half is worse: a non-ASCII DECIMAL digit is accepted by BOTH, so int("\u0663") returns 3 and a planted file was silently adopted as segment 3, alongside the real one. Handling the ValueError would have fixed only the crash and left the collision. _segment_path writes ASCII and nothing else, so ASCII is the entire legitimate set.

The eviction-marker open now matches _open_segment. Accepted, fixed — and only half of it is load-bearing, which is worth saying rather than leaving implied. The marker path is agent-writable before the sensitive-path family lands, and the relaxation it gates is what stops a head-truncated log from verifying clean, so its read deserves the same guards as a segment read. O_NONBLOCK is the half that matters: opening a fifo read-only BLOCKS until a writer appears, so a planted fifo hung inside os.open and neither the byte cap nor anything else downstream ever ran. S_ISREG is the other half, and it is defence in depth rather than a reachable hole — measured, not assumed: os.read on a directory raises EISDIR, a fifo opened non-blocking raises EAGAIN, and a device's bytes fail the authenticated-token comparison, so every non-regular case already failed closed. I could not write a control that fails when only S_ISREG is reverted, and the test says so in its own docstring instead of implying coverage it does not have. It stays because drifting apart from the sibling helper is what produced this finding in the first place, and a test now pins the two flag sets as equal.

The verify walk no longer reads a segment in one allocation. Accepted, fixed. It did fh.read() over a whole segment. _open_segment refuses a symlink, a fifo and a device, but a large REGULAR file passes all of it — the same gap that already needed its own bound for the chain-tip scan and for the eviction marker, so this is the third instance of one class. Round 17 below corrects this: two more reads were still unbounded, so bounding this one did not close the class. A per-line cap is the right shape here: verify has to read an entire segment to count its entries, so it cannot be bounded in total without breaking a legitimately large one, whereas a single line is a JSON record of a few hundred bytes and a megabyte-long one means the file is not a segment. The reader raises OSError, which the walk's existing handler already treats as an unverifiable segment — logged, folded into total, never into valid. No new control flow.

Two honest notes on that last one. The verdict was already fail-loud without the cap, because a newline-free file splits into unparseable chunks that count toward total and never toward valid — so the first version of my test passed with the fix reverted, and it now asserts the code PATH (the refusal is logged) rather than the verdict. And the switch from read().splitlines() to readline narrows line splitting to \n, which is what the writer emits; splitlines also split on \v, \f and the Unicode line separators, so an embedded control character used to inflate total with fragments of a single record.

backup_count=0 losing a concurrent append. Refuted, with the mechanism accepted. The claim is accurate as a statement — appends do not join the seal lease, so an append landing between the re-stat and the unlink is destroyed — but it does not describe a harm distinct from what this path is for. The operator note on that branch already states that it deletes the WHOLE active file at a roll, up to max_bytes of the most recent events. Deleting an append that arrived microseconds earlier is that same deletion, not a new one. What round 14 fixed was the genuinely different case: acting on a stale size and unlinking a file another process had ALREADY rolled, which appends had since recreated well under the cap. That deletion was outside anything the operator opted into, and the re-stat closes it. Fully serialising appends against this unlink means putting the append path under the lease, which is the multi-writer append hazard this description already declares unsupported, so it is not fixed here and not pretended away.

Function-local get_recorder import. Unchanged, and still declined, but see round 17: the ground it had been refused on was wrong, and the real one is the measured 81-module boot cost.

Verification (round 16)

Ten tests added (test_sel.py 217 → 227). Five of the six negative controls fire for the intended reason; the sixth is the S_ISREG case above, which has none and is documented as such rather than counted as covered.

control	observed
accept any isdigit() suffix	ValueError: invalid literal for int() with base 10: '\u00b2'
accept any isdigit() suffix	the planted name was adopted as a number
drop O_NONBLOCK from the marker open	marker flags {'O_NOFOLLOW'} drifted from segment flags
drop the per-line cap (unit)	DID NOT RAISE <class 'OSError'>
drop the per-line cap (verify)	the planted segment was parsed as records rather than refused
drop S_ISREG from the marker open	no control exists — see above

Two existing tests broke and were repaired rather than relaxed, both because the production change was legitimate: a spy that raised on read() had to raise on readline() too, since a real I/O error on an open descriptor fails both; and the structural pin that the walk reads from a handle now spans the extracted reader, asserting the walk delegates to it and the reader reads from the handle.

Full repo suite: 55,712 passed. 72 failures, matching the count and distribution on a clean checkout of the same parent; none in a file this PR touches. The one entry new to that list, test_pid_lifecycle.py, was proven pre-existing by running it on the stashed baseline — identical result, with both files restored byte-identically afterwards. mypy clean over 982 files, flake8 and isort clean, affected suites 895 passed.

Round 15 (CI triage, no code change): the Windows shard-2 timeout is not reachable from this diff

Backend Tests (Windows) (2) was cancelled at its 40-minute cap on this sha, having passed in 10.8 minutes on the previous one. That looks damning for a diff this size in this subsystem, so it was worth checking properly rather than either dismissing it or writing a speculative fix. It is not reachable from these changes, and the evidence is a shard-membership measurement rather than an argument.

Every SEL test runs in shard 3, and shard 3 passed on this sha. The suite is split by pytest-split. There is no .test_durations file in the repo, so the split is by test COUNT, not duration. Collected locally with the same deselects CI uses, all 217 test_sel.py tests — including the 11 added this round — fall in shard 3, whose range runs from test_mcp_core.py to test_skill_listing_cost.py. Shard 2 spans test_cse_2026_08_07_fixes.py to test_mcp_core.py and holds no SEL test at all. On the failing run, shard 3 succeeded in 9.9 minutes, shard 1 in 8.7 and shard 4 in 9.3. If the rotation or seal-lease code deadlocked, shard 3 is the shard that would hang.

The seal lease is not new on this sha. It shipped in the previous revision — the one where shard 2 passed in 10.8 minutes. This round only widened it to cover the zero-backup discard path.

The module code is gated behind rotation. Everything changed in sel.py this round runs only once the active file reaches max_bytes, which defaults to 100 MB and disables rotation entirely at zero. A test that does not configure the rotation knobs cannot execute any of it. The one shard-2 file named for SEL pruning, test_heartbeat_sel_prune_offload.py, replaces the whole singleton with a MagicMock, so the real prune never runs there either.

The boundary shift is real but harmless. Because the split is by count, adding 11 tests does move the boundaries: shard 2 gained exactly four tests from test_mcp_artifacts.py and lost two from test_cron_trigger.py. All four gained tests are single-patch() unit tests — one mock, one call, one assertion, no filesystem, no threads, no subprocesses. They cannot hang.

What the failure actually looks like, read from the job log. Progress ran normally from 22:57 to 23:11:56 — about 15 minutes, which is within the range this shard shows elsewhere — and then stopped dead for 24.0 minutes with no output at all until the cap. It is a total freeze at the end of an otherwise normal run, not a slow crawl and not an early hang. Two details support that reading: the run carries a 180-second per-test timeout that never fired, which is what one expects from an unresponsive worker rather than a hanging test, and this repo's own pytest configuration already documents the symptom, warning that workers dying under memory pressure produce "~20 minutes of zero progress and an empty log".

Shard 2 is also the slowest shard structurally, not just here: sampled across eight recent unrelated CI runs it takes 11.2 to 14.4 minutes while the other three sit at 9 to 10. Splitting by count rather than duration is why, and it means shard 2 carries the least headroom under the 40-minute cap. That is a CI-infrastructure observation, not something to fold into an audit-log commit.

Re-triaged after a second, sharper report, which supplied one datum the first pass lacked and one claim that is wrong. The new datum is accurate and worth recording: pytest progress reached [ 99%] at 23:11:56 and then went silent, so nearly every test in the shard had already run and the stall is at the very end. That is what makes session teardown, interpreter exit, or the xdist controller the natural place to look, since the 180-second per-test cap is disarmed outside a test body.

The wrong claim is the attribution, and it is decidable rather than arguable. It was put as "the last sha where this shard was green did not contain the SEL work". It did. 4f5a7450 is the same feature commit, not a pre-SEL baseline: _SEAL_LOCK_FILE = "seal.lock" sits at line 106 there and _seal_lease at line 1296. The cross-process lease shipped in the revision where this shard passed in ten minutes. What this revision did was widen that lease to also cover the zero-backup discard path, and correct prose which still read "no cross-process lock is taken" — so the comment change is being read as if the mechanism were new when only the description of it changed.

Four further checks, each at source, close the remaining mechanisms:

• The exit path is untouched. The whole revision is seven hunks in sel.py, and grepping that diff for atexit, _writer, daemon, Thread, _pending, flush, close and _decr_pending returns nothing. The writer thread is created daemon=True, so it cannot hold up interpreter exit, and its atexit-registered flush is older than this revision and unchanged by it.
• Rotation is unreachable without opting in. Everything changed here runs only once the active file reaches max_bytes, which defaults to 100 MB. KIROCREW_SEL_MAX_BYTES is set in exactly one file, test/test_sel.py, and there only through monkeypatch, so it never leaks to another test. No conftest sets it. A shard that holds no SEL test therefore cannot enter _rotate_now, _newest_timestamp_of, or the lease at all.
• The lock file cannot be deleted by the discard path. _list_sealed_indices accepts an entry only when it starts with security_events.jsonl. and the remainder is all digits, so seal.lock is never returned and never unlinked. That closes the "unlink a file whose handle is still open" shape, which is the one that would actually matter on Windows.
• Nothing hangs on Linux. The full suite, with this revision applied, completes in 155 seconds — including every test the failing shard holds.

No code changed for this item. One message-only amend was later authorised to re-observe the shard — identical tree, identical commit message, no file edits — on the reasoning that a re-observation is informative either way: a green shard says the freeze was environmental, and a second timeout says it is reachable after all and there are two measurements instead of one argument. That is the whole spend; no further amend follows a second timeout. Writing a Windows fix for a lease that the failing shard never executes would be a guess, and it would have to be verified on a shard that already passes. The honest limit of this analysis: the precise cause of the 24-minute freeze is not established, only that it is not reachable from this diff, and the shard membership was computed on Linux with CI's deselect list rather than observed directly on the Windows runner.

Review round 14: three real concerns, all fixed; one completes a round-13 claim

All three were verified at source before any code changed, and all three are real, so nothing is refuted this round. Two share a shape worth naming, because it is the shape that hides from review: a guard that was added to one path and not to its sibling, with a nearby comment asserting the protection is already in place. A reader checking the comment finds the right words and stops.

Concern 1 — the backup_count=0 discard path had neither the seal lease nor the re-stat. Accepted, fixed. Round 13 added a cross-process lease plus a re-stat under it, because the file size that triggers a roll is measured by a pre-check that takes no lock. Both landed on the sealing path only. The backup_count=0 branch returned before reaching either — and its action is unlink, not a rename, which makes a stale size worse there rather than harmless: one process rolls the oversized file, ordinary appends recreate a small active file, and the second process then deletes that fresh file. Those events are simply gone, with nothing logged. The comment immediately above the lease already spelled out the hazard ("the lease holder may have just rolled this very file"), so the asymmetry was documented against itself. Both terminal actions now run inside one lease behind one re-stat, and the discard moved into _discard_leased() so that its lease requirement is stated where a future caller will see it.

Severity, so it is not oversold: the default backup count is 5, so reaching this needs KIROCREW_SEL_BACKUP_COUNT=0 together with more than one writer process on the host. The operator note on that branch does knowingly accept losing one full active file at a roll. It does not accept a second, stale unlink destroying a file that appends recreated after that roll, so it is not a defence for this.

Concern 2 — the age scan was unbounded and quadratic. Accepted, fixed. These are two faults, and a bound alone fixes one of them. Two lanes reported this, at different line numbers, and it is one defect in one function. f.read(step) + buf appears at exactly one site in the file, so GPT's citation is the accurate one; the other lane's line number lands inside _load_or_create_hmac_key, an unrelated function, which is the usual signature of a line number taken from a diff view rather than the file. Fixed once. _newest_timestamp_of walks a segment backward in 4 KB steps looking for the newest parseable record. Its sibling _tip_hash_of does two things it did not. First, it floors the walk: without a floor, a segment containing no newline never yields a complete line, so the held-back buffer grows to the size of the whole file. Second, it trims that buffer each step to just the possibly-incomplete first line. Without the trim the buffer keeps every byte read so far, and the split runs over that entire accumulation again on every step — quadratic CPU even once memory is bounded. Both faults sit on the writer thread while _lock is held, reachable from both the size cap and the daily age prune, so either one stalls all audit logging rather than just slowing a read. The reviewer supplied that reachability chain and it checks out at source, function span by function span: _flush_batch calls _maybe_rotate, which calls _rotate_now, which calls _seal_leased, which calls _evict_over_budget, which calls _newest_timestamp_of. The second route is the daily prune through _prune_sealed_by_age, which reaches the same function.

The docstring made this worse than a plain omission: it already claimed the bounded behaviour ("Reads only the trailing chunk … so age-pruning a 100 MB sealed segment doesn't load it fully into memory"), so the code and its own description disagreed, and the description was the reassuring one.

Fixed by copying both halves from _tip_hash_of, with the same 1 MiB floor. On hitting the floor with nothing parseable it logs at ERROR and reports no timestamp. That direction is deliberate: both callers read a missing timestamp as "cannot prove this segment is aged", so retention keeps the segment instead of deleting a file whose contents it was never able to read.

Concern 3 — a segment that opened but failed to READ still verified clean. Accepted, fixed — and this corrects round 13 below. Round 13 says unreadable segments are folded into total and never into valid, forcing valid < total. That is true only of segments that fail to open. _walk_handles reads from handles the caller already pinned, and a read error there took a bare continue. The total counter is incremented per line, inside the loop that continue skips, and the fold-in list was appended at exactly one place — the open failure. So a segment that opened and then failed to read contributed nothing to either counter, valid == total held, and the endpoint reported integrity: ok over history it never read. The comment on that branch stated the opposite, claiming the segment still counted toward total.

That path is not exotic: it is the ordinary I/O error on an already-open descriptor. Fixed by recording the path in the same fold-in list and correcting the comment to say what the code does.

Four prose corrections in the same pass, all cases where the text now contradicts the shipped mechanism. Two files said rotation needs no cross-process lock, which stopped being true when round 13 added the lease — sel.md and the _rotate_now docstring. _maybe_rotate's docstring said monotonic numbering retired the lease; what it retired is the wide lease that had to span a rename of every segment, and a much narrower one remains. sel.md still named the eviction marker by its old flat-layout path, and a constants comment still described a segment being "aged to .2", which is shift-rename language that the code explicitly disclaims — segment numbers are allocated once and never renamed.

Verification (round 14)

Each fix has a negative control that reverts it alone and shows the matching test fail for the intended reason. The trim is controlled separately from the floor, because a floor alone leaves the quadratic re-split and a single test could easily have hidden that.

control	observed failure
discard outside the lease	rival holds the lease and the active file is discarded anyway
remove the re-stat	the freshly recreated active file is unlinked
remove the scan floor	the backward scan is unbounded: read 1572864 bytes
remove the buffer trim, keep the floor	the buffer is not trimmed: 81551 parse attempts for 3200 lines
skip a read-error segment again	a segment that opened but could not be read verified clean: 19/19

The trim number is the point of that control: 81,551 parse attempts for 3,200 lines is 25x the linear cost, measured with the floor still in place, so the two fixes are shown to be independently load-bearing.

Eleven tests added (test_sel.py 206 → 217). Full repo suite: 55,701 passed. 72 of the 73 failures reproduce with identical node ids on a clean checkout of the same parent; the one extra is a thread-timing watcher test in a module that imports neither sel nor security, its file reports the same failure count on both trees across three runs each, and this branch produced 51 and then 52 failures on two runs of its own tree. Affected suites: 885 passed, 0 failed. mypy clean over 982 files, flake8 and isort clean.

Review round 13: two real gaps I had argued away, one false clean, one refutation

Three of the four findings are accepted and fixed. One of them contradicts a claim this description previously made, so that correction comes first.

BLOCKING 2 — the atomic segment claim does not ORDER the roll. Accepted, and it refutes what I wrote in round 12. This description previously said the worst concurrent outcome was "one extra small segment, in correct chain order because a higher number is strictly newer". That is wrong. The claim stops two processes taking the same number; it does not order the seal. Process A claims N, B claims N+1, B moves the active file onto N+1 first, appends recreate the active file, and A then moves that newer data onto N — so a lower number holds newer events. The FileNotFoundError branch does not save it, because by then the active file exists again. Eviction deletes the lowest numbers, so it would drop the newer history first.

I checked whether this is reachable before accepting it, since a single-writer deployment would make it moot: it is not single-writer. heartbeat.py runs the daily prune in the dashboard process while mcp_gateway/gatewayd.py, backend.py, app_call.py and mcp_apps.py each construct SecurityEventLog() with no base_dir, so all of them resolve the same file in separate processes.

The fix is a cross-process lease around claim + replace only (_seal_lease), non-blocking, so losing the race means "skip this roll" rather than waiting on the writer thread while holding _lock. This partially reverses a round-12 claim and it is worth being precise about how much: the lease that monotonic numbering retired had to span a shift-rename sequence over every segment, which is what made rotation destructive under concurrency. This one spans two operations and no existing segment is ever inside it. The rest of the Option B subtraction stands — no shift renames, no .tmp_rot residue class, no prefix-family regex.

BLOCKING 3 — an unreadable segment made verification read clean. Accepted. _walk_chain opened each segment and, on any OSError, skipped it with continue. That is correct for a segment that is absent (no active file yet, or one evicted between listing and opening) but it also swallowed a segment that is present and unopenable — a permission change, an I/O error, or the non-regular-file refusal — so total == valid reported integrity: ok while audit history was unaccounted for. Absent and present-but-unreadable are now distinguished by lstat (not exists(), so a dangling symlink counts as present), the unreadable ones are logged at ERROR, and they are folded into total and never into valid, forcing valid < total. One per segment is a deliberate fail-loud floor rather than an entry count: the file could not be read, so the true count is unknown. CORRECTED in round 14 above: this covered only segments that fail to OPEN. A segment that opened and then failed to read was still skipped without being folded in, so it went on reporting clean until round 14.

BLOCKING 1 — a planted sel link would have received the segments. Accepted. mkdir(parents=True, exist_ok=True) follows an existing symlink or junction, so an agent that plants sel as a link before this feature ships would have every sealed segment written to its own target — outside the sensitive-path floor, since the floor protects the registered path and not wherever it points. _ensure_segment_dir() now takes the same posture, and the same platform_compat helpers, as the trust directory already does a few hundred lines above: remove the link (never its target) and create a real directory in its place, and refuse to rotate if the link cannot be removed. An un-rolled oversized log is recoverable; history written to an attacker-chosen location is not.

FINDING — the function-local metrics import. Refuted, unchanged. metrics/provider.py's own docstring states it is imported lazily, "never during config.loader's import chain, so its top-level config.loader import cannot form a cycle. Callers that reach it from inside that chain (e.g. acp.client) MUST import get_recorder lazily". config/loader.py imports sel, so sel is inside that chain, and the precedent is real in acp/client.py with the cycle spelled out in a comment. provider also runs a module-level OpenTelemetry probe, so hoisting would put that work in the audit log's own import path. This is the allowed exception the rule contemplates, named at source.

Verification

Full repository suite: 52,054 passed, 220 skipped, 6 xfailed. Affected suites together: 1,064 passed, 2 skipped, 0 failed (test_sel.py alone 206, up from 198). mypy clean over 982 files; isort and flake8 rc=0.

Each fix is negative-controlled, each control in its own run, and each was confirmed to fail for the intended reason:

revert	the guard that fires
follow a planted sel link	the planted link survived
seal without holding the lease	sealed without the lease — [1,2,3,4,5] == [1,2,3,4]
skip an unreadable segment again	an unreadable segment must never read as a clean chain — assert 19 < 19

sel.py was restored byte-identically after each control, with zero markers left in the tree.

One of those controls caught a vacuous test of mine, which is worth recording rather than quietly fixing. The first version of the unreadable-segment test made a middle segment unreadable, and it passed with the fix reverted — skipping a middle segment breaks the chain by itself, so valid < total held for the wrong reason. The discriminating shape is the oldest segment with the eviction marker set, because the marker relaxes the genesis anchor and there is then no broken link: the one-entry deficit comes purely from the fold-in. Measured both ways before rewriting it — total=19 valid=17 with a break present, total=19 valid=18 with the marker set and no break.

Because a single process cannot observe the seal interleaving, TestSealIsSerializedAcrossProcesses pins two things instead: behaviourally, that a rival holding the lease makes the roll decline; and structurally, by AST, that the claim and the replace both sit inside the with self._seal_lease() block, so a later edit cannot hoist the claim back out while every other test still passes.

Five failures in the full run are pre-existing and were proven so rather than asserted: four (test_dashboard_handlers_core_coverage.py pip-channel and STT cases, test_gateway_lock_diagnosis.py::test_flock_is_held_by_a_fork_orphan) reproduce with identical signatures on a clean checkout of this same parent commit. The fifth, test_playwright_cli_installer.py::test_the_installers_own_mktemp_stays_inside_the_test_tmp_dir, is a whole-host observation — it snapshots Path("/tmp").glob("tmp.*") before and after and asserts no new entry appears, so any other process on the machine creating a mktemp -d directory during that window fails it. This diff adds zero mktemp calls, the build host had 502 such directories present from unrelated processes, and the test passes on re-run.

Review round 12: the layout change, plus the two findings it does not fix

Round 11 verified all four of the GPT lane's blocking findings at source and spent no commit, because two of them were artefacts of the layout the owner had just decided to replace. This revision is that replacement, and it carries the two that were layout-independent.

BLOCKING 1 — the hide-set snapshot left a later segment readable. Closed at the root. latex.py globbed the segment siblings once while building the hide-set, so a segment sealed after that snapshot was readable by the TeX compiler. This is not fixable by enumeration: any snapshot has the same window, and re-globbing only narrows it. The hide-set now names the directory, so a member created later is covered with no enumeration at all — and the on-loop glob that a separate blocking finding was about disappears with it. A test builds the hide-set before sealing a segment and then asserts coverage, which is the assertion an enumeration could not pass.

BLOCKING 4 — residue reporting on event-loop call paths. Closed by deletion. _report_unadopted_residue() ran in the constructor, and SecurityEventLog() is genuinely constructed inside async dashboard handlers, so the finding was real. One correction to it: the work was a single directory glob, not the unbounded filesystem walk claimed. The function is gone, because .tmp_rot residue only existed to stage a renumber that no longer happens.

BLOCKING 2 — unbounded backward scan for the chain tip. Accepted; a genuine gap, layout-independent. _tip_hash_of accumulates read chunks and, with no newline anywhere in the file, retains the entire segment until it reaches offset 0 — reached from the constructor, with no bound anywhere. The symlink guard added in an earlier round refuses symlinks, fifos and devices; a large regular file walks straight through it, which is why this needed its own fix. Now floored at 1 MiB. On giving up it logs at ERROR and reports no tip for that segment only, so _read_last_hash continues to older segments rather than silently restarting the chain at genesis — a test asserts exactly that fall-through.

BLOCKING 3 — the destructive active-file rewrite. Accepted, and the layout does not fix it. Stage 2 of prune() streams the active file, filters it, and os.replaces the result. _lock is in-process only, so another SEL writer process appending between the read pass and the replace has its events discarded — silently, since os.replace neither fails nor logs. This is worth separating from the append-path remainder, which I had folded it into: that one is fail-loud with every entry still on disk, whereas this is loss.

The fix compares the active file's size immediately before the replace and, on any change, skips the cycle rather than applying a stale rewrite. Prune runs daily, so the cost of skipping is at most a day of over-retention. Stated precisely, because it is a narrowing and not a closure: this reduces the window from the whole read pass — seconds on a ~100 MB file — to a single stat, and an append landing inside that one-syscall gap is still lost. Closing it entirely needs a lock the append path also honours, which is the same remainder as multi-writer appends. The test proves growth during the filter turns into a skip; the negative control confirms a quiet log is still pruned, so a guard that always skipped could not pass unnoticed.

FINDING — the function-local metrics import. Refuted, with the imported module's own contract. metrics/provider.py's docstring states it is imported lazily, "never during config.loader's import chain, so its top-level config.loader import cannot form a cycle. Callers that reach it from inside that chain (e.g. acp.client) MUST import get_recorder lazily". config/loader.py:3648 imports sel, so sel is inside that chain, and the cited precedent is real in acp/client.py with the cycle spelled out in a comment. provider also runs a module-level OpenTelemetry availability probe, so hoisting would put that work in the audit log's own import path. This is the allowed exception the rule contemplates, named at source.

Two things found while doing the work, not by a reviewer

A test of mine caught a regression in a fix of mine. The first version of the seal caught OSError broadly and returned quietly, which swallowed a genuine rotation failure — destroying the observability this feature exists to provide (a silent degrade back to unbounded growth is the exact condition it guards against). Only FileNotFoundError is benign there, meaning another process sealed the file first; every other OSError now discards the claimed placeholder and re-raises, so _flush_batch logs it and emits the failure counter.

The eviction ends inverted, and that is the most dangerous part of this change. Under shift-renames the oldest segment carried the highest index and eviction ran from the top; with monotonic numbers the oldest is the lowest. Carrying the old rule over would delete the newest audit history and keep the oldest, which is silent and unrecoverable. The directional test asserts which end goes, and its negative control confirms it fails when flipped.

What the subtraction actually removed

removed	why it existed
_rotation_lease + the .rotlock file	serialising shift-renames across processes
_report_unadopted_residue, .tmp_rot staging	crash residue from the renumber pass
_orphaned_sealed_segments + the orphan fold-in in verify	the walk stopped at the first gap, so stranded segments had to be recovered
_SENSITIVE_HOME_PREFIXES, _PREFIX_FAMILY_SUFFIX and its Windows twin, three injection sites	matching dot-suffixed siblings of the active file
sensitive_home_prefix_families() and both its consumers	the two-accessor split Design flagged as a silent-miss pattern

security_posture.py is byte-identical to the parent commit as a result, so this revision touches 9 files rather than 10.

Review history (rounds 1-11)

Read the sections below as history. They record how each review finding was dispositioned, and they are kept because the reasoning is often still the reason a current invariant exists. But many describe the flat .1..N layout that this revision replaced, so their mechanisms — the .rotlock cross-process lease, the .tmp_rot residue class, shift-renames, the prefix-family regex and the sensitive_home_prefix_families() accessor — no longer exist in the code. Where a round below describes one of those, the current behaviour is in "What changed" above and in round 12.

Review fix: residue is no longer promoted into the chain

A GPT 5.6 review raised a blocking finding on the earlier .tmp_rot recovery path, and it was right. I confirmed the mechanism at source before changing anything: the recovery function contained no reference to hmac, _compute_hash, entry_hash or verify — it gated purely on a parseable timestamp and ordered by it. So a file the process never wrote could be renamed into .1 and become numbered audit history on nothing stronger than a well-formed date.

The reachability is a pre-upgrade window, which is what makes it worth acting on rather than dismissing. Measured against unmodified upstream/main: is_sensitive_path returns False for security_events.jsonl.1, for security_events.jsonl.<x>.tmp_rot, and for security_events.jsonl.evicted, while returning True for the base log and for the HMAC key at both of its locations. This PR is what closes the first three. Until it lands, an agent's file tools can write a residue name — and can read a real sealed segment.

That read is the part that decides the fix. The obvious repair is to verify each residue entry's HMAC before adopting it, but it does not close the hole, because an actor never needs to forge anything: a genuine sealed segment can simply be copied to a .tmp_rot name, and its HMACs are real. Adopting the copy inflates the sealed-segment count, the next roll evicts idx >= backup_count — the oldest indices, i.e. real history — and that eviction sets the sticky marker which relaxes verify's genesis anchor, so a truncated chain then reads clean. Replay defeats validation; only refusing to adopt defeats replay. Forged entries, by contrast, were always the loud case: without the key they fail HMAC and show up as valid<total.

So recovery is removed. Residue is logged at ERROR on construction and on every prune, counted into verify's total and never into valid, and left on disk for inspection. The cost is real and worth stating: a genuine crash mid-renumber now needs an operator instead of self-healing. That is the same posture _maybe_rotate's direct shift renames already took for a numeric gap, so both crash paths now agree, and no data is lost in either.

On the reviewer's sorted() note — worth separating the two orderings. sorted() over that glob only built the gather list; adoption order came from keyed.sort() on the parsed timestamp, so lexical-vs-numeric was not the live defect. The real problem was upstream of both: the ordering key was attacker-supplied file content. Refusing to adopt removes the question rather than repairing the sort.

Guards, each negative-controlled: TestResidueIsNeverAdopted covers the exact shape the old path would have adopted, adoption at construction and via prune(), a byte-identical copy of a real sealed segment (the case HMAC validation could not have caught), and that verify still reports valid<total so refusal does not mean silence. Re-introducing adoption fails 4 of them; removing .tmp_rot from the orphan set fails the fail-loud one; restoring the old method name fails the structural one.

Review response: the lazy metrics import stays

The same review flagged the function-local from kiro_crew.metrics.provider import get_recorder at the rotation-failure counter as a top-level-imports violation, suggesting the block be removed. Keeping it, for two checkable reasons. There is no such rule enforced here: function-local kiro_crew imports occur 321 times across 61 files in src/kiro_crew/ (36 in cli.py, 24 in sandbox.py, 17 in hooks.py), and flake8's E402 neither applies to function-local imports nor is configured to flag them. And the block is the only observability for a rotation failure silently degrading the log back to unbounded growth, which is the specific failure this feature exists to prevent.

I did correct the comment above it, because my stated reason was wrong. It claimed a top-level import "would cycle" through sel -> metrics.provider -> config.loader -> sel. The cycle exists on paper — metrics/provider.py:42 imports the loader at module scope, and config/loader.py:3648 imports sel — but loader's import is function-local, so it is never import-time-fatal: a top-level import here loads cleanly in all three orders I tried. The accurate reason is narrower, and is what the comment now says: it keeps the config layer off this very-early module's import graph, the same reason _default_dir() resolves config_dir() lazily.

Review round 2: one blocker confirmed, one refuted

BLOCKING sel.py verify snapshot — CONFIRMED, fixed. GPT was right, and I reproduced it before changing anything rather than reasoning about the window. The walk snapshotted PATHS under _lock and read them after releasing it. A concurrent roll shifts .k → .k+1 and reseals the active file as .1, so every snapshotted path still exists while naming a different inode. The walk then reads a set that is internally chain-adjacent but silently omits the segment renamed out from under it — and because the sticky eviction marker suppresses the genesis-anchor check, the omission does not surface as a break either. Measured on 30 entries across 7 segments with a marker present: total=26 valid=26, i.e. integrity: ok, with all 30 entries still on disk. Four retained entries went unverified and the endpoint said everything was fine. Note the retry loop could never have caught this: it retried on OSError from a vanished path, and in this race no path vanishes. Fixed as the reviewer suggested — segments are opened under the lock and the walk reads those handles, since an open handle follows the inode and neither a rename nor an unlink can disturb a walk in progress. The retry machinery is gone with it, because the condition it guarded was not the condition that occurred.

BLOCKING sel.py blocking read on the event loop — REFUTED as a regression; remainder declared. The property is real but it is pre-existing and this change does not introduce or worsen it. At the PR base, sel.py:937 (verify_integrity) and sel.py:965 (recent) already did self._path.read_text(...) — synchronous whole-file reads — and core.py's api_sel_verify is already async def calling verify_integrity() directly on the loop. dashboard/handlers/core.py appears zero times in this PR's diff. What changed is the volume, in the safe direction: before this PR the active file was unbounded, so the loop read an arbitrarily large single file; it is now bounded at roughly 600 MB across segments. Opus 4.8 reached the same conclusion independently on this sha, citing the same base line numbers.

I am declining the suggested remedy specifically, because reverting the multi-segment read would be a correctness regression, not a de-risking. A verify that reads only the active file cannot validate a rotated chain at all: on any host that has rolled, the first entry it sees is non-genesis, so it would report valid<total on a perfectly intact log. That trades a pre-existing latency property for a permanent false alarm on the integrity surface.

The remainder, stated at the right level: the fix belongs at the handler, not in sel.py, and it already exists as #3995 — perf(dashboard): offload SEL audit-log reads off the event loop (open, non-draft), whose only files are dashboard/handlers/core.py and its test. Offloading there fixes it for recent() and verify_integrity() together, including the pre-existing case, which an edit inside sel.py cannot do.

Review round 2: CONCERNS dispositions

Design — unauthenticated marker: agreed, fixed. This was the sharpest finding in the round. The relaxation was gated on a bare touch-file, which handed it to precisely the adversary the module already defends against — the actor with write access to the log directory, which is why the key lives outside that directory. That actor could touch the marker, head-truncate a never-evicted log, and verify would adopt the surviving first entry's prev_hash and read clean, defeating the case test_never_evicted_log_enforces_genesis exists to protect. The marker's contents are now a domain-separated MAC under the SEL key, compared in constant time, and anything missing, empty, unreadable, or non-matching reads as no marker. That direction is deliberate: a forgery restores the genesis anchor it meant to suppress, while a corrupt-but-genuine marker only makes a legitimately-evicted host false-alarm — loud and recoverable. Rotation has never shipped, so there are no unsigned markers to migrate. test_forged_marker_cannot_hide_head_truncation covers the end-to-end attack.

Design — no downgrade story: agreed, documented. sel.md now carries a rotation downgrade caveat in the same spirit as the key-migration rollback caveat: an older binary post-rotation sees only the active file, verify false-alarms, and a restart re-anchors at genesis, permanently splitting the chain. Fail-loud rather than data-loss, and it does not self-heal on re-upgrade, so the note says to archive the segment set if continuity across a downgrade matters.

Design — stale sync-exclude reference: agreed, dropped. The _EVICTED_MARKER_FILE comment cited a security_events.jsonl.* sync-exclude that this repo has no sync module to host. Removed; the comment now describes the MAC instead.

Design — monotonically increasing segment numbers: declined for this PR, with reason. The reviewer is right that sealing to .N+1 and evicting the lowest index would delete the shift-renames, the .tmp_rot staging, the residue class, and much of the orphan analysis. It is the better on-disk convention and I am not arguing otherwise. But it is a redesign of the layout rather than an adjustment to it — new numbering, new oldest/newest resolution, new gap semantics, and a fresh set of crash cases — landing in the same change that is already carrying a security fix and a floor extension. The reviewer's own framing is what makes deferring defensible: the concern is that the .1..N convention ossifies, and rotation has not shipped, so nothing is ossified yet and the swap stays cheap until it does.

First Principles — drop the rotation kwargs: agreed, applied. The review's consumer count matches mine: zero production callers passed max_bytes / backup_count / retention_days; only test/ did. They are gone, along with the _warn_ignored_rotation_kwargs path they existed to explain and the __new__(*args, **kwargs) widening they forced — __new__ carries its precise (base_dir, sync) signature again. Tests set the attributes directly, which is what test_marker_only_gate_survives_rotation_disabled_after_eviction already did, centralised in the one _rot_log helper. The shipped bound is unchanged: rotation runs on the module constants either way. test_constructor_exposes_no_rotation_kwargs pins the narrowed signature so the surface cannot creep back. I also dropped the two negative-value normalisation tests with the kwargs, since with no caller able to pass a negative they asserted against a surface that no longer exists; the off-switches remain enforced at the point of use in _maybe_rotate and _prune_sealed_by_age.

Review round 3: the Windows failure was in the tests, not the code

Backend Tests (Windows) shard 3/4 failed on the previous head with WinError 32 ("the process cannot access the file because it is being used by another process") in two tests this PR adds, TestVerifyPinsSegmentsByHandle::test_unlink_during_verify_still_reads_the_pinned_inode and ::test_roll_during_verify_does_not_fake_a_clean_chain. Linux and macOS were green. That failure is real and reachable, so it is fixed here rather than argued with — but the defect is in the tests' portability, not in the shipped behaviour, and this section shows the reading that settles which.

Both tests must rename or unlink a segment while verify holds it open, because that is the race they exist to reproduce. POSIX permits that (the inode stays alive for whoever holds a handle), and Windows refuses it. So on Windows the race cannot be set up at all: the kernel blocks the very rename the race depends on, which means verify still cannot return a vacuous integrity: ok there. The guarantee holds on both platforms; only the mechanism differs, and Windows' is the stricter one.

What that does cost on Windows is that a roll landing while a verify is in flight FAILS instead of succeeding. That is already contained and loses nothing: sel.py:306-315 wraps _maybe_rotate() in try/except Exception, logs SEL rotation failed; appending without rotating, sets a flag that emits kirocrew.sel.rotation_failed.count at sel.py:385 after the lock is released, and still appends the batch to the un-rotated active file. The practical effect is that the log stays over max_bytes until the next flush rolls it. The daily prune() path is guarded the same way by its own caller at heartbeat.py:188, which is not in this diff.

The sharper question — whether rotation could hit WinError 32 on its own, with no concurrent verify — is answered no, and this is the part worth checking rather than trusting. No SEL code path opens a segment and then renames it in the same call. Every read helper closes its handle first via a with block: _entry_count_of at sel.py:633, _newest_timestamp_of at sel.py:652, _tip_hash_of at sel.py:1196. _walk_handles never opens anything by path at all — it reads only from handles the caller pinned, and its entry counter takes a handle rather than a path (_count_entries_in(fh: IO[bytes]), sel.py:609). In prune(), the os.replace onto the active file at sel.py:1867 sits outside both of the with blocks opened at sel.py:1839-1840, so neither handle is still open when the replace runs. So the Windows failure is only ever reachable through genuine concurrency, never self-inflicted.

Accordingly sel.py is unchanged in this round. The two scenario tests now carry @pytest.mark.skipif(os.name == "nt", ...) with the OS reason stated, matching the seven pre-existing POSIX skips already in test_sel.py. A bare skip would have been too cheap on its own, though, because those two tests are what proves the pin is real — so three guards were added that hold on every platform, and each was confirmed to fail on its own mutation:

• test_walk_handles_never_reads_a_segment_by_path walks the AST of _walk_handles and rejects any read_text access, with a positive control asserting the same detector does find read_text in _has_evicted so it cannot pass vacuously. Reverting the handle read to path.read_text(...) fails this test — which is the point: that revert is caught on Windows even with both scenario tests skipped.
• test_segment_handles_are_opened_while_the_lock_is_held asserts the ordering that makes the pin atomic against rotation. Moving the opens outside _lock fails it, and notably does not fail the two scenario tests, so it covers something they do not.
• test_rotation_rename_failure_is_contained raises PermissionError(32) from Path.rename and asserts events still append, the chain still verifies, and the warning is still logged. Narrowing the except Exception in _flush_batch fails it with that PermissionError propagating, so the containment relied on above is pinned rather than assumed.

The platform split is documented in docs/system-specs/modules/sel.md under the pin-by-handle bullet, including the containment behaviour and why the scenario tests skip.

Verified by forcing the skip condition true on a scratch copy: the class then reports 2 skipped and 4 passed, which is the shape the Windows shard will now see.

Review round 4: cross-process rotation, operator control, and three subtractions

BLOCKING -- rotation is not atomic across SEL writer processes: CONFIRMED, fixed

The premise is correct and I could not talk it away. There really is more than one SEL writer process: mcp_gateway/gatewayd.py:1284 (and backend.py, app_call.py) call SecurityEventLog() with no base_dir, so they resolve the same file as the dashboard gateway, and sel.py contains no fcntl/flock of any kind -- _lock is a threading.Lock, which orders writers only inside one process.

_maybe_rotate now takes an exclusive non-blocking lease on security_events.jsonl.rotlock via platform_compat.try_acquire_lock (the repo's existing primitive: flock on POSIX, msvcrt.locking on Windows, already used by gateway_lock.py and cron.py), and re-stats the file under the lease because the holder may have just rolled it. Non-blocking is deliberate: rotation is best-effort and already contained, so losing the race means "skip this roll", never "wait on the writer thread while holding _lock". The lease file lands on the sensitive-path floor, so an agent cannot unlink it and leave two processes locking different inodes. I did not take the reviewer's "revert the hunk" option -- deleting the feature to quiet the finding would leave the log unbounded, which is the defect this PR exists to fix.

One correction to the finding's framing, measured rather than argued. This is not a regression introduced by rotation: two processes appending concurrently already corrupt the chain at base, because each caches its tip in _last_hash (read once at sel.py:232, never re-read per append). With rotation disabled -- the exact base behaviour -- two processes writing 40 events each produced total=81 valid=8, i.e. 73 chain breaks. So multi-writer SEL is already broken independently of this PR.

That does not make the finding wrong, because the two damages differ in kind, and that difference is why I fixed the rotation half and only the rotation half. A broken chain is fail-loud with every entry still on disk and forensically recoverable; a clobbered segment is silent and gone. Declared remainder: making SEL genuinely multi-writer needs a single-writer daemon or an append-path lease. It is pre-existing, strictly larger than this PR, and belongs in its own change -- it is not fixed here and the docs say so.

BLOCKING -- full SEL reads on the gateway event loop: REFUTED as a regression, remainder unchanged

Verified at the base commit rather than recalled. api_sel_events and api_sel_verify are async def and call _sel().recent(...) / _sel().verify_integrity() directly with no offload -- at upstream/main, unchanged by this PR. dashboard/handlers/core.py appears 0 times in this diff, so the blocking-on-loop behaviour is pre-existing, not introduced.

The stronger point is that for /api/sel/events this change makes things better, not worse, which the finding has backwards. recent() reads newest-first (sel.py:1878) and returns as soon as it has limit entries (sel.py:1898-1899), so at the default limit=100 it normally reads the active file only -- exactly one file, as before -- and reaches a sealed segment just after a roll. That one file was unbounded at base and is now capped at max_bytes. verify_integrity() does read every retained segment, because it is the integrity oracle and cannot validate a rotated chain otherwise, and its volume moves from an unbounded single file to roughly 600 MB bounded at the shipped defaults.

I did not route through asyncio.to_thread here for a specific reason: the fix belongs in core.py, which this PR does not touch, and #3995 (OPEN, non-draft) already does exactly that for both recent() and verify_integrity(), including the pre-existing case. Editing core.py here would duplicate that work and conflict with it on the same lines.

FINDING -- function-local metrics import: REFUTED

Not a repo standard: function-local kiro_crew imports occur 321 times across 61 files in src/kiro_crew/ (36 in cli.py, 24 in sandbox.py, 17 in hooks.py), and flake8 neither applies E402 to function-local imports nor is configured to flag them, so "no permitted exception" does not describe this repo. The narrower true reason is in the comment: it keeps the config layer off this very-early module's import graph, the same reason _default_dir() resolves config_dir() lazily. A dependency cycle does exist on paper (metrics/provider.py -> config.loader -> sel) but is not import-time-fatal, because loader's sel import is itself function-local -- I verified a top-level import loads cleanly in all three orders and corrected the comment in round 2 rather than leave the stronger claim standing.

Design -- eviction precedes the knob: AGREED, operator control added

This was the strongest objection and it is now backed by more than review opinion. Amazon's audit-log retention guidance requires the retention period to be operator-settable and security events kept at least 365 days, and the ARCC anti-pattern is stated directly: "Audit logs without a retention policy may be deleted before compliance review periods end." A compile-time-only cap on a host whose volume outruns it is that anti-pattern.

Two changes, because operator control and visibility are different needs. The knobs are now settable per host via KIROCREW_SEL_MAX_BYTES, KIROCREW_SEL_BACKUP_COUNT, KIROCREW_SEL_RETENTION_DAYS, falling back to the module constants; a malformed value is ignored with a warning rather than guessed at, so a typo cannot quietly move a deletion bound. KIROCREW_SEL_MAX_BYTES=0 disables size rotation and is the documented opt-out, with the 365-day age prune still active, so a retention ceiling exists either way. Env rather than config because KiroCrewConfig has no sel section and reading one would be a static type error plus an unreachable branch; the config follow-up slots between env and the defaults in the one knob-resolution block.

Second, the case the reviewer is really worried about is now loud instead of silent. Size and retention are independent bounds and size wins, so a high-volume host can evict a segment the retention window still wants. The eviction path compares each doomed segment's newest entry against the retention cutoff and, when it is inside the window, warns with the count and names the two knobs to raise, and increments kirocrew.sel.early_eviction.count. An operator now learns their cap is too small for their volume instead of discovering the gap at review time.

Design -- .1..N numbering ossifies at first release: NOT changed, and this is a decision for the human before publish

I am not arguing the reviewer is wrong. Monotonically increasing numbers are the better convention, and the ossification argument is sound: merging is what ossifies it, and once hosts hold .1..N segments a swap becomes a migration. Monotonic numbering would also be a root-cause fix for the blocker above, because sealing to the next unused number removes the shift-rename sequence entirely rather than serializing around it.

I did not do it in this amend. It is an on-disk layout redesign of an audit log, and landing it hastily in the same commit as a concurrency fix -- with each change needing its own controls -- is the wrong trade on this surface. Since rotation is unreleased, nothing has ossified yet, so the decision is still free; it stops being free at merge, not later. Flagging it as a pre-publish decision rather than a silent deferral.

First Principles -- seal segments into a SUBDIRECTORY: verified real, same pre-publish decision

I checked the mechanism and the reviewer is right, with one correction: trust and run are not bare entries in _SENSITIVE_HOME_DIRS, they are _CREW_SECRET_LEAVES entries expanded under the crew prefixes. The substance holds -- a registered directory covers arbitrary children by registration alone, via cand_cf.startswith(sensitive_path + os.sep) at security.py:5095. Measured: ~/.kiro/crew/trust/whatever.new is sensitive purely because trust is registered, and ~/.kiro/crew/sel/security_events.jsonl.1 is currently not covered. So sealing segments into .kiro/crew/sel/ and adding one sel leaf would replace the whole prefix-family apparatus, and because rotation is unreleased there would be no migration.

That is a real simplification and I am recording it as verified rather than merely acknowledged. Not done here for the same reason as the numbering: it is the other half of one layout decision, and the two should be taken together, before first release, not stacked onto a security fix. Worth noting on the other side of the ledger: the family matcher covered the new .rotlock lease file automatically on all three surfaces (tool gate, write gate, bash matcher) without being touched, which is the property a per-suffix enumeration would not have had.

First Principles -- drop _CREW_SECRET_PREFIX_LEAVES: AGREED, inlined

Confirmed one entry and exactly one consumer, so the indirection earned nothing. Inlined into the comprehension; _CREW_SECRET_PREFIX_LEAVES now appears 0 times. Matcher behaviour is unchanged, boundary control included: security_events.jsonl.1, .evicted and .rotlock all still match and security_events.jsonl2 still does not.

First Principles -- delete the import-time non-empty raise: AGREED, and the measurement went further than the finding

Deleted, and the invariant moved to TestSensitivePathAlternationSourcesAreNeverEmpty. The reviewer's reasoning was right that it cannot fire in a shipped build, and testing it surfaced something that supports deleting it more strongly: the removed comment's stated danger -- that an empty alternation silently disables credential-read detection -- is not reproducible. Emptying _SENSITIVE_HOME_DIRS does not un-match an exact leaf such as the Midway cookie path, because other regex branches also cover it. What an empty list actually does is the over-block direction: the matched span degrades from the full segment path to a bare ~/.kiro/ prefix. The test asserts that measurable behaviour change and explicitly does not assert the under-block, rather than inheriting a claim I could not demonstrate.

Spec sync

sel.md said later SecurityEventLog(...) calls "ignore the kwargs (logging at debug level if any were passed)" -- describing surface removed in round 2, and contradicting the same document's own line 80. That was my own stale text, corrected here, along with new documentation for the env knobs, the cross-process lease and its explicit scope limit, and the early-eviction warning.

Review round 5: the ratio failure is not reachable from this diff

Backend Tests (3.10, shard 2/4) failed on this branch at test/test_denied_commands_security.py:672 with assert 3.37919910817109 < 3.0. The review that flagged it argued the cause is this PR's new regex fragments, on the grounds that "the function it measures IS in your diff". That premise is wrong, and it is decidable rather than arguable: the test measures a different regex from the one this PR changes, and the regex it does measure is byte-for-byte identical between the merge-base and this branch. No code changed for this item.

What the test actually measures. The test calls is_denied(), which lives at security.py:7314 and matches against the deny corpus (BUILTIN_DENY_PATTERNS plus user regexes and fnmatch globs). This PR's new fragments live in _build_sensitive_regex(), which feeds a different pattern — the sensitive-path matcher. The two are siblings, not nested: the only consumer of the sensitive pattern is is_sensitive_bash_command(), at security.py:5349 (if _get_sensitive_re().search(command):) and security.py:5357 (if _RELATIVE_SENSITIVE_RE.search(command):), and is_denied never calls it. Every caller of is_sensitive_bash_command is in hooks.py, llm_helpers.py, mcp_cron.py or computer_use/policy.py; the only mentions inside security.py are two comments.

Five independent checks, each with a control that can fail.

1. The measured object is unchanged. Hashing the deny corpus on both shas gives BUILTIN_DENY_PATTERNS len=7729 sha256=c2a035ac630e0501 — identical. The sensitive pattern does differ (_RELATIVE_SENSITIVE_RE 2605 → 2713 chars), which is exactly the change this PR makes, and is not what the test reads.
2. Zero calls into changed code. Wrapping every function this diff modifies (_build_sensitive_regex, _get_sensitive_re, _path_in_home_dirs, is_sensitive_path, is_sensitive_bash_command, _check_sensitive_via_normalizer) with counters and running the exact measured input gives 0 for all six, and 0 SecurityEventLog.append calls. Positive control on the same counters via is_sensitive_bash_command: total 2, so the counters work.
3. Sentinel on the compiled patterns. Replacing both sensitive patterns with an object that raises if consulted, is_denied on the measured input completes normally and returns None. The positive control fires (SENTINEL _SENSITIVE_RE consulted), and a post-check confirms the sentinel was still installed, so the negative result is not a lost patch. An earlier version of this probe was blind — it patched the lazy cache variable rather than the accessor, and used is_sensitive_path as its control, which never consults the patterns at all (security.py:5121 delegates to _path_in_home_dirs). That blind result was discarded, not reported.
4. Paired ratio, same machine and interpreter. Replicating the test's own harness (thread_time, best-of-3 at n and 2n, test_denied_commands_security.py:560), six samples per side: branch min 1.87 / median 2.01 / max 2.11; merge-base min 1.96 / median 1.98 / max 2.12. 0 of 12 samples reached the 3.0 ceiling (_MAX_DOUBLING_RATIO, line 509).
5. The real test, under emulated-runner contention. Running the actual pytest test pinned to 2 CPUs with two spinners on those same cores: 6/6 pass on the branch and 6/6 on the merge-base. The source swap was proved before measuring, by a discriminator rather than an assumption — the merge-base does not match security_events.jsonl.1 (False) while the branch does (True).

The mechanism claim is empirically backwards for this pattern. The review's reasoning was that an optional greedy run added per alternative amplifies backtracking. Measured directly against the sensitive matcher with four adversarial inputs at n=2000 and n=4000, the merge-base is super-linear on all four (ratios 3.69–4.03, costs 92–355 ms) while this branch is 1.84–1.93 on three of them, at 29–132 microseconds — three to four orders of magnitude cheaper. The reason is that the family now matches early instead of driving the engine through every alternative to failure; the added suffix reduces backtracking here rather than amplifying it. The fourth shape (a stem followed by non-dot junk, which can never match) stays super-linear at 4.08 versus the base's 4.03 — indistinguishable, because that input fails on the stem and never engages the suffix group. That is a pre-existing property of the sensitive matcher, unreachable from is_denied, and recorded here as a remainder rather than patched inside a rotation PR.

The security properties the review asked to be proved both hold, and are asserted in TestSelRotationFamilyPosix* / the tool-gate tests: the family matches security_events.jsonl.<suffix> (.1 → True) and does not match security_events.jsonl2 (→ False), with the 2 case kept as an explicit boundary control.

What I could not establish, stated plainly. I cannot reproduce 3.379 on either sha on this machine, so I cannot name the mechanism that produced it, and I am not calling the test flaky — the threshold, the budget and the test's skip status are all untouched. Two things are worth recording for whoever sees it next. The obvious hypothesis, that this PR moved the test into a different shard, is refuted by measurement: pytest-split puts it in group 2 on both shas. What does change is its neighbours — the repo ships no .test_durations, so per the workflow's own comment the split "falls back to an even split by test count", and this PR's added tests grow the total from 55,458 to 55,577 and group 2 from 13,865 to 13,895. Since CI runs each shard with -n auto, that is 30 more tests co-resident with a CPU-time ratio measured as best-of-3 at n followed by best-of-3 at 2n; thread_time excludes other processes' CPU but still counts this thread's memory-stall cycles, so a contention episode spanning the later window inflates the ratio without any regex changing. That is a hypothesis I have not measured, offered as a lead rather than a conclusion. Also relevant: the other six open PRs in this batch currently pass this shard, so the "same failure across the batch" argument does not apply here and was not used.

Review round 6: the same regex passed three shas and failed one

Round 5 refuted the ratio finding structurally and could not name a mechanism. There is now a decisive piece of evidence that round 5 did not have, and it settles the question without appeal to timing: Backend Tests (3.10, 2) passed on 8f2ea8c31, 2625c71bd and e4275cace, and failed only on 917559227 — while the two regex fragments the finding blames are byte-identical across all four.

Verified at source rather than inferred. git show e4275cace:src/kiro_crew/security.py and git show 917559227:src/kiro_crew/security.py both carry _PREFIX_FAMILY_SUFFIX = r"(?:\.[^\s/'\"]*)?" and _PREFIX_FAMILY_SUFFIX_ANYSEP = r"(?:\.[^\s\\/'\"]*)?" verbatim; only their line numbers moved (4472→4467, 4475→4470) because 15 lines were deleted above them. The entire security.py delta between the last passing sha and the failing one is 7 insertions and 22 deletions, and it is exactly two things: a one-element list comprehension inlined, and an import-time raise guard plus its comment removed. Neither changes the compiled pattern. The comprehension equivalence is measured, not argued — both forms produce the identical prefix list, sha256=26424fd05a2bf45f either way, and the live _SENSITIVE_HOME_PREFIXES equals the new form. So the accused regex is the same object on a sha that passed and a sha that failed, which is sufficient on its own: a deterministic super-linear regex cannot pass three times.

The specific new reachability argument, tested and refuted precisely. The claim was that /.ssh/ is a sensitive-path needle, so the test's inputs traverse the matcher family this PR widened. The first half is true and worth stating plainly: /.ssh/ appears in 9 deny patterns (.*cat.*/\.ssh/.*, .*head.*/\.ssh/.*, .*tail.*/\.ssh/.*, .*less.*/\.ssh/.*, …). But those are entries in the deny corpus, which is what is_denied() matches and which hashes len=7729 sha256=c2a035ac630e0501 identically on both trees. The sensitive-path matcher is a separate compiled object that is_denied never consults. Measured with counters on all six functions this diff modifies, driven by all four exact test inputs (both builds at n=2000 and n=4000): 0 calls each, on both trees, with a positive control on the same counters returning 2 so the counters demonstrably work. The same string appearing in two pattern families is not the same thing as one family being reached.

Paired local discriminator, run through the test's own method. Rather than a replica, this imports TestIsDeniedReDoSResistance and calls its real _doubling_ratio, so the measurement uses the test's own _cpu_cost/_elapsed and the real is_denied. Twenty samples per tree:

tree	min	median	mean	max	stdev	samples ≥ 3.0
head 917559227	1.956	1.995	2.000	2.125	0.038	0 / 20
parent 9a7a19d05	1.923	2.001	2.004	2.063	0.036	0 / 20

The distributions overlap completely and the means differ by 0.004. Neither tree approaches the 3.0 ceiling, let alone straddles it.

Confirmed at the CI level with an identical-content control. The commit was then amended with no content change at all — git commit --amend --no-edit, producing sha 9e770227a whose tree hash is 518fef7ce1fe45fbe6cc9112780f35ba333dfa8d, byte-identical to 917559227's. Backend Tests (3.10, 2) passed on that byte-identical tree, as did all four 3.10 shards. So the same bytes produce a fail and a pass on consecutive runs: the failure is nondeterministic, which is a property of the runner and not of the diff. This is stated as a measurement with its control, not as a flake dismissal — the ratio ceiling, the budget and the test's marker set are all untouched.

The fragment-reverted negative control. A reviewer asked for the one control that separates "my change did this" from "this runner was slow": revert only the new suffix fragments and re-measure. Both were set to the empty string, which removes the optional group from all three injection sites (security.py:4697, :4770, :5258) while leaving everything else intact; the revert was proved to take effect before measuring, since the family stops matching security_events.jsonl.1 while the base file stays protected. Twenty samples: min 1.959, median 1.994, mean 1.995, max 2.051, 0 above 3.0 — indistinguishable from head's mean of 2.000. Removing the accused fragments changes the ratio by 0.005. The file was restored from a byte-level backup afterwards (sha256 match, zero residue), and the affected test plus the surrounding security and SEL suites pass: 1209 passed, 1 skipped.

The call path, read rather than inferred. is_denied spans security.py:7314-7546. Its transitive call closure inside security.py is 64 functions, and not one of them references _PREFIX_FAMILY_SUFFIX, _PREFIX_FAMILY_SUFFIX_ANYSEP, _get_sensitive_re, _build_sensitive_regex, _SENSITIVE_RE or _RELATIVE_SENSITIVE_RE; _build_sensitive_regex is not in that closure at all. The same analysis run on is_sensitive_bash_command as a control traces the whole chain — is_sensitive_bash_command → _get_sensitive_re → _build_sensitive_regex → the two fragments — so the detector demonstrably finds the reference when it exists. The reviewer's description of the injection sites is accurate; what does not hold is that is_denied reaches them.

Taken together — identical regex across a pass and a fail, zero calls into changed code on the exact inputs, an unchanged deny corpus, and 0 of 40 local samples above 2.13 — the failure is not reachable from this diff. Nothing was suppressed to reach that conclusion: _MAX_DOUBLING_RATIO is untouched, no budget was widened, and the test carries no skip or xfail marker. The remaining honest gap is unchanged from round 5: 3.379 is not reproducible here on either tree, so the mechanism that produced it is still unnamed, and the neighbour-contention lead recorded in round 5 stays a lead rather than a conclusion.

Review round 7: the reachability chain was traced in a tree without this diff

A further review closed the reachability chain at source and cited security.py:5169 for if _get_sensitive_re().search(command):, with _build_sensitive_regex() at ~4529 and _get_sensitive_re() at 4670-4674, naming the checkout it read. Those line numbers do not belong to this branch. On this branch the same three sites are at 4665, 4822 and 5349; the cited numbers match main at 91097e5d2 exactly. That tree contains zero occurrences of _PREFIX_FAMILY_SUFFIX and zero of _SENSITIVE_HOME_PREFIXES, yet its line 5169 already reads if _get_sensitive_re().search(command):. So the chain from _build_sensitive_regex() through the _SENSITIVE_RE cache to that .search() call is pre-existing architecture that exists without this PR, and the step attributing that alternation to this diff cannot be verified in the tree it was read from. Steps 1 through 3 of that trace are all true statements about the codebase; the step that does not hold is the last one, that the failing test measures that path.

Settled by direct experiment rather than by reading. The test's measurement funnel is one line — test/test_denied_commands_security.py:529, return self._cpu_cost(lambda: is_denied(command)) — and the whole test file references is_sensitive_bash_command zero times. Two sabotage runs make the consequence unambiguous:

sabotage installed as _get_sensitive_re()	positive control	test_mid_dotstar_chain_spam_stays_linear
object that raises on any use	is_sensitive_bash_command raised	passed
deliberately super-linear pattern (own doubling ratio 3.86)	patch confirmed installed	passed

The second row is the decisive one. A sensitive-path regex whose own ratio on the test's exact input shape is 3.86 — worse than the 3.379 CI reported — leaves this test passing untouched. A regex the test cannot feel when it is pathological is not a regex the test can be failing because of, and widening its alternation therefore cannot be the cause.

The fragment-reverted control agrees. Setting both new fragments to the empty string, which removes the optional group from all three injection sites, moves the mean doubling ratio from 2.000 to 1.995 across twenty samples — a change of 0.005, with zero samples above 3.0 in either condition.

No regex shape was changed, so there is no before/after ratio to record for one: the acceptance signal asked for is a ratio below 3.0, and it is already 1.99 on this branch, on the parent, and with the fragments reverted. _MAX_DOUBLING_RATIO stays at 3.0, no cap was widened, nothing was marked skip or xfail, and the prefix-family matching this PR exists to add is untouched — the security behaviour and the timing property are not in tension here, so no design decision is owed.

Review round 8: three new findings, two of them security gaps this PR opened

This round examined surfaces the earlier ones did not — the marker write path, the sensitive_home_dirs() consumer surface, and the new subprocess test. All three reproduce at source and all three are fixed.

N1 — the eviction marker write followed a symlink and truncated its target. Confirmed: _mark_evicted called write_text on the marker path, which opens for truncation and follows a link. An earlier round authenticated the marker's contents with a MAC; that says nothing about the write path. Reachability is the same pre-upgrade window this PR closes: the marker name is not on the sensitive-path floor until this feature lands, so an agent can pre-place security_events.jsonl.evicted as a link to any file it wants destroyed, and the next eviction truncates that file and writes the token into it. Fixed by routing through the shared atomic_write helper, which writes a temp file and os.replaces it: replace acts on the NAME, so it swaps the symlink out instead of opening the target. Atomicity comes free, which also removes the torn-marker case where a crash mid-write leaves bytes that fail the MAC and read as tampering.

The read path had the same defect class and is fixed with it. _has_evicted used read_text, which follows a link too, and unbounded — so a marker linked to an endless source (/dev/zero) would hang verify_integrity() rather than fail closed. It now opens O_NOFOLLOW and reads through a 256-byte cap, roughly 4x a genuine 64-char hex payload. Windows has no O_NOFOLLOW, so the flag degrades to 0 there and the cap is what remains; stated rather than implied.

N2 — rotated segments were absent from the LaTeX compile sandbox's hide-set. Confirmed: sensitive_home_dirs() returns tuple(_SENSITIVE_HOME_DIRS), which lists paths matched exactly, and the prefix families live in a separate list. latex.py's _sensitive_hidden_dirs() built its hide-set solely from the first, so it hid security_events.jsonl and not the .1..N segments, the .evicted marker or the .rotlock lease — history with the same sensitivity as the base log, left readable to a hostile .tex via \verbatiminput. The base log is covered at both base and head, so the exposed set is exactly the family this PR introduces. Fixed by adding a public sensitive_home_prefix_families() accessor and having the hide-set expand each family over its real dot-suffixed siblings; the suffixes are unbounded so they cannot be listed, and a glob is exact for a hide-set because only a file that exists can be read.

The other consumer of that accessor is fixed in the same pass. security_posture.py enumerates the same list to describe what is blocked, so it understated the floor by the same family; it now shows each as <base>.*. Those two are the only functional consumers — the third mention, in sandbox.py, is a comment.

N3 — subprocess.Popen without cwd= in the concurrent-rotation test. Confirmed and fixed. With -c, Python prepends the inherited working directory to sys.path, so a stray kiro_crew there would be imported in place of the package under test.

Tests, each negative-controlled in its own run (the runner stops at the first failing assertion, so one control cannot vouch for its siblings). Reverting the write fix fails the symlink test precisely at assert victim.read_text(...) == original — the victim file is destroyed. Reverting the no-follow read fails with "the marker read followed a symlink", while that test's own in-line control still passes, so the result is attributable to the link and not to a wrong token. Reverting the hide-set expansion fails naming exactly the four missing members, while its base-log control passes, which proves the hide-set resolved the data home and the miss is the family's.

Two existing tests failed as a direct consequence and were fixed at the right level rather than relaxed. The round-3 AST guard used _has_evicted's read_text as its positive control, and this round removed that call; the control now points at recent, which legitimately reads a segment by path — the very thing the walk must not do. And the posture test pinned item count to a single source; it now counts both and additionally asserts a family label is present, so a dropped family entry fails something.

On the append-path lease. A finding this round reads the rotation lease as not covering append or prune. That is the same remainder already declared, restated from the other side, and the disposition is unchanged — but naming it explicitly here so the reposition is closed: the cross-process lease serialises rotation only. Concurrent appends from separate processes still break the hash chain, because each process caches its tip in memory and never re-reads it; measured with rotation disabled, two processes writing 40 events each produced total=81 valid=8. That is pre-existing, fail-loud, and loses nothing — every entry stays on disk — whereas a clobbered segment is silent and gone, which is why only that half was closed here. Making SEL genuinely multi-writer needs a single-writer daemon or an append-path lease, and is strictly larger than this PR.

Verification (round 8)

gate	result
mypy src/kiro_crew/	Success: no issues found in 979 source files
isort / flake8	rc=0 / rc=0
SEL + security + posture + LaTeX + deny suites	1353 passed, 1 skipped, 0 failed
test_live_target + test_sandbox_argv (accessor consumers)	190 passed, 1 skipped
comment linter (--staged)	0 errors, 0 warnings

Review round 9: prune now takes the cross-process lease

BLOCKING — sealed-segment pruning bypassed the rotation lease. Confirmed, fixed. The finding is right, and the chain is reachable across processes rather than only in theory. Verified at source: prune() Stage 1 renumbered sealed segments under self._lock alone, and self._lock is a threading.Lock (sel.py:231), so it excludes nothing outside this process. The same renumbering routine, _prune_sealed_by_age, has two callers — one inside _rotate_leased, which runs under the cross-process .rotlock lease, and one inside prune, which never referenced _rotation_lease at all. Two processes could therefore renumber the same .1..N namespace at once, and a survivor rename could land on a segment the other had just sealed: silent, permanent loss of audit history.

The cross-process part is not hypothetical. prune() runs in the dashboard process — heartbeat.py:188 calls sel().prune on a maintenance executor once per day — while mcp_gateway/gatewayd.py, mcp_gateway/backend.py, mcp_gateway/app_call.py and dashboard/handlers/mcp_apps.py all construct SecurityEventLog() with no base_dir, so they resolve the same file in separate processes and rotate on append.

Fixed as the reviewer suggested: Stage 1 now holds the lease. Two details that mattered while doing it. The lease is taken after self._lock, matching the order _flush_batch → _maybe_rotate already uses — the reverse order would be an AB-BA deadlock inside a single process, which would have been worse than the bug. And because the lease is non-blocking, prune declines Stage 1 when a rival holds it. That is not a retention gap: age pruning has a second driver on the rotation path (_rotate_leased calls _prune_sealed_by_age under the same lease), and prune runs daily, so the cost of declining is at most a day of over-retention, never data loss. Residue reporting is read-only and still runs either way.

Scope boundary, checked rather than assumed: prune() exists at the merge base and its Stage 2 — the streaming rewrite of the active file — is pre-existing behaviour, while _prune_sealed_by_age and _rotation_lease are both absent at base (zero occurrences). So Stage 1 is this PR's own gap and is closed here; Stage 2's active-file concurrency falls under the append-path remainder already declared above, and is not a regression this PR introduces.

Three guards, negative-controlled. Reverting Stage 1 to self._lock alone fails two of them for the intended reasons — "prune renumbered sealed segments without holding the cross-process lease" and "the lease was never taken by prune". The third deliberately still passes under that revert, because its job is the opposite: it asserts prune does drop aged segments when the lease is free, which is what catches the failure mode where a lease that can never be obtained silently disables retention while the first test goes green. A fixture note worth recording, since it surprised me: filling with 400-day-old entries left nothing to prune, because the rotation path's own age-prune had already dropped them — which is itself a live demonstration of the second driver relied on above. The tests use 30-day-old entries inside a 365-day retention window instead.

Verification (round 9)

gate	result
mypy src/kiro_crew/	Success: no issues found in 979 source files
isort / flake8	rc=0 / rc=0
SEL + security + posture + LaTeX + deny suites	1356 passed, 1 skipped, 0 failed (was 1353)
comment linter (--staged)	0 errors, 0 warnings

Review round 10: two blocking fixes, one refutation, and a layout decision for the owner

BLOCKING — rotated-segment reads followed pre-existing symlinks. Confirmed, fixed at every site. Verified at source: no segment read had a symlink or non-regular-file guard, and the only O_NOFOLLOW in the module was the eviction-marker read added a round earlier. Segment names are predictable and, before this feature's sensitive-path family lands, agent-writable — so a planted .1 pointing at an endless source turned a verify or an events fetch into an unbounded read.

Rather than patch the one line cited, all five segment reads now go through a single named helper, _open_segment, so a read site added later cannot silently miss the check: _newest_timestamp_of, _tip_hash_of, both opens in _walk_chain, and recent. A structural test asserts no bare open(path, "rb") survives in the module, which is what keeps that true.

Two guards, and they are complementary rather than belt-and-braces. O_NOFOLLOW refuses a symlinked final component — the plant itself. S_ISREG, checked by fstat on the already-open descriptor so there is no check-to-open window, refuses a fifo, device or directory, which O_NOFOLLOW permits. Windows has no O_NOFOLLOW, so it degrades to 0 there and S_ISREG is what remains.

A third flag turned out to be load-bearing, and my own test is what found it. The first version of the helper hung for the full 120-second test timeout on a planted fifo: opening a fifo read-only BLOCKS until a writer appears, so os.open never returned and the S_ISREG check below it never ran — the same denial of service by another route, with the guard I had just written sitting uselessly one line further down. O_NONBLOCK is what makes the S_ISREG half reachable at all; it is a no-op for regular files, which is every legitimate segment.

BLOCKING — segment discovery ran filesystem I/O on the event loop. Confirmed, fixed by moving it, not by deleting it. The ordering claim is exactly right, and the mechanism is subtle enough to be worth stating: _sensitive_hidden_dirs() was passed as extra_hidden_dirs=... into a functools.partial, and only a partial's TARGET is deferred to the executor — the expressions building its arguments are evaluated eagerly, on the calling thread, which here is the loop. So the spawn was correctly offloaded while the glob I added a round earlier was not. _run's own docstring, directly above the call, says "The chokepoint itself is called OFF the loop" for precisely this reason; the glob undercut it.

Fixed by computing the hide-set inside the executor: the partial now targets a small helper that builds the hide-set and spawns, so both land off-loop. I deliberately did not take the reviewer's "revert this glob" wording literally — reverting would drop rotated segments from the hide-set, which is a correctness regression on a sensitive-path surface, and a test asserts the segment is still present in the set the spawn receives.

FINDING — function-local metrics import. Refuted, and this time with the authority rather than a head-count. Two earlier rounds argued this from prevalence. The actual answer is in the imported module's own contract: metrics/provider.py's docstring states that it is imported lazily, "never during config.loader's import chain, so its top-level config.loader import cannot form a cycle. Callers that reach it from inside that chain (e.g. acp.client) MUST import get_recorder lazily". config/loader.py:3648 imports sel, so sel is squarely inside that chain, and the cited precedent is real — acp/client.py:3310-3314 carries the same lazy import with the cycle spelled out as config.loader → acp.types → acp.client → metrics.provider → config.loader. provider also runs a module-level OpenTelemetry availability probe, so hoisting would put that work in the import path of the audit log itself. This is the allowed exception the rule contemplates, named at source.

Screenshot Evidence — no waiver needed, and adding one would be wrong. Head carries TWO check-runs of that name, both from .github/workflows/screenshot-evidence.yml on the same pull_request event: one success and one cancelled by concurrency after two pushes landed close together. The rollup resolves to pass. The <!-- no-visual-delta --> marker waives a requirement that is currently satisfied, so it is not being added. The only genuine failing check is the GPT lane; PR Readiness is pending, not failed.

Verification (round 10)

gate	result
mypy src/kiro_crew/	Success: no issues found in 979 source files
isort / flake8	rc=0 / rc=0
SEL + security + posture + LaTeX + deny + accessor-consumer suites	1552 passed, 2 skipped, 0 failed (was 1356)
comment linter (--staged)	0 errors, 0 warnings

Each fix is negative-controlled in its own run. Removing both segment guards fails the symlink test with "DID NOT RAISE OSError"; restoring the hide-set as a partial argument fails the executor-boundary test. Two pre-existing tests broke as a consequence and were repaired at the right level rather than relaxed: the round-3 AST guard's positive control had been pinned to recent's read_text, which this round removed, and is now scoped to an attribute inside _walk_handles itself so it cannot rot when an unrelated site changes — that control had already broken twice for exactly this reason. The lock-ordering spy watched builtins.open and now watches _open_segment, the single chokepoint.

Review round 11: dispositions, and why three of these are not being patched in place

The layout question the Design lane raises is no longer open: the owner has chosen the sel/
subdirectory with monotonic segment numbers (Option B), and that work is in progress. That
changes the right disposition for two of the four blocking items, because they are artefacts of
flat naming rather than defects to patch inside it.

BLOCKING 1 — hide-set snapshot leaves a later segment readable. CONFIRMED, and it is not
fixable by enumeration. Verified at this sha: latex.py globs the segment siblings once while
building the hide-set, so a rotation that seals a new segment after that snapshot produces a file
the sandbox was never told to deny. Any snapshot has the same window — widening or re-globbing
only narrows it — so the fix is to stop enumerating. Under the subdirectory layout the hide-set
names the DIRECTORY, and every future member is covered with no enumeration at all. That is why
this is being closed by the layout change rather than patched here. The lane's suggested remedy,
reverting rotation, would also drop the capability; the directory form keeps it.

BLOCKING 4 — residue reporting in the constructor puts filesystem I/O on loop call paths.
CONFIRMED. _report_unadopted_residue() runs in __init__, and SecurityEventLog() is
constructed inside async dashboard handlers, so the scan does land on the loop. It is a single
directory glob rather than the unbounded walk the finding describes, but the placement is real.
This one disappears with the layout: .tmp_rot residue only exists because survivors are
renumbered through temp names, and monotonic numbering never renumbers, so the residue class and
the function that reports it are both deleted.

BLOCKING 2 — chain-tip discovery can be made to read an entire planted segment. CONFIRMED, and
accepted as a real gap independent of layout. _tip_hash_of scans backward in 4 KiB chunks and
accumulates them in buf; when a segment contains NO newline, buf.split(b"\n") yields one
element, so the deferred-partial-line branch keeps the whole accumulation and the loop runs to
pos == 0 with the entire file in memory. There is no size bound anywhere in that scan. The
round-10 _open_segment guards refuse a symlink, fifo or device, but a large REGULAR file passes
them, and _read_last_hash() runs this from the constructor. This is the same class of defect as
the eviction-marker read that already carries _MARKER_READ_CAP, so the remedy is consistent
with what this PR already does elsewhere: bound the backward scan and treat "no newline within
the bound" as no parseable record, loudly. Layout has no bearing on it, so it is being fixed as
part of the same work rather than deferred.

BLOCKING 3 — the rotation lease does not cover the destructive stage-2 rewrite. CONFIRMED, and
this sharpens a remainder rather than restating it. The lease wraps Stage 1 only, by design and
as documented. What the finding adds is the consequence for Stage 2, and it is worse than the
remainder declared above: that remainder is about concurrent APPENDS breaking the hash chain,
which is fail-loud and loses nothing, whereas Stage 2 streams the active file and os.replaces
it — a read-then-replace that, across processes, can silently drop events another process appended
after the read pass. In-process self._lock covers it; nothing covers it between processes. That
is data loss, not a loud break, so it should not have been filed under the append remainder and is
accepted here as its own item. Note the layout change does NOT fix this one — Stage 2 rewrites the
active file, which the subdirectory does not touch.

FINDING — function-local metrics import. Refuted, third time, and the authority is in the
imported module. metrics/provider.py's own docstring states it is imported lazily, "never
during config.loader's import chain, so its top-level config.loader import cannot form a
cycle. Callers that reach it from inside that chain (e.g. acp.client) MUST import
get_recorder lazily". config/loader.py:3648 imports sel, so sel is inside that chain, and
the cited precedent is real at acp/client.py:3310-3314, where the same lazy import carries the
cycle written out as config.loader → acp.types → acp.client → metrics.provider → config.loader.
provider additionally runs a module-level OpenTelemetry availability probe, so hoisting would
put that work in the import path of the audit log itself. This is the exception the rule
contemplates, and it survives the layout change unchanged.

Why no sha was spent on this round. Two of the four blocking items are compensations for flat
naming that the accepted layout deletes outright, and patching them in place would be throwaway
work on code scheduled for removal — which is the pattern that produced the preceding rounds of
lease, residue and prefix-family fixes. The two that are layout-independent are accepted and are
being fixed in the same change. The reviewer's "revert rotation" framing is declined explicitly:
it would drop the feature to avoid its current shape, where changing the shape closes the same
findings and keeps it.

Deliberate divergences from the internal source

Two places where copying the source would have regressed hardening that KiroCrew has and the internal version does not:

• Chain-tip recovery preserved. The source replaces _read_last_hash with a simpler backward scan. KiroCrew's version skips a corrupt or truncated tail line rather than resetting the chain to genesis, so instead of overwriting it I generalized that body into _tip_hash_of(path) — keeping the skip logic — and made _read_last_hash walk segments newest-to-oldest over it.
• Streaming prune preserved. The source rewrites the active file with atomic_write("\n".join(...)), which would load a max_bytes-sized file into memory. KiroCrew already streams line-by-line through mkstemp + os.replace, and mkstemp creates the temp file 0o600 which os.replace carries onto the destination — so the owner-only mode the source secures with an explicit mode= argument is already correct here. Stage 2 keeps KiroCrew's implementation.

Scope notes for the reviewer

• No config coupling, so the config follow-up stays a genuine follow-up. KiroCrewConfig has no sel section (43 declared fields, none of them sel), so sel.py reads no config at all: rotation runs on module constants, overridable per instance through constructor kwargs, which is how the tests drive it cheaply. An earlier revision did try to read KiroCrewConfig.load().sel behind a try/except, and CI's blocking mypy step correctly rejected it — "KiroCrewConfig" has no attribute "sel" [attr-defined]. The runtime fallback worked, but the branch that would have read real values could never execute in this repo, which is untestable code in an audit module. I removed the lookup rather than reaching for getattr(cfg, "sel", None) (same unreachable branch, just invisible to mypy) or # type: ignore (suppressing a true signal). Making the knobs operator-settable is a follow-up that adds the config section plus its loader-side clamps and wires them into the single knob-resolution block in _init_locked. sel.py keeps its own floor regardless: a negative value reads as disabled rather than flowing into the size and cutoff comparisons, and there is deliberately no upper floor on max_bytes because the tests depend on small caps. Two regression guards, one behavioural and one structural: test_unset_knobs_take_the_module_defaults pins the defaults, and test_sel_module_does_not_import_the_config_loader walks the AST for a loader import — it has to walk the AST rather than grep, because the module legitimately mentions config.loader in a comment about an import cycle and legitimately imports config.paths for config_dir().
• prune() signature. A bare prune() now defaults keep_days to the instance's configured retention_days rather than the module constant, matching the rotation path. The one production caller is the daily heartbeat prune at heartbeat.py:188, which passes no argument; until a sel config section exists both resolve to the same 365 days, so this is behaviour-preserving today.
• No sync change, because there is no sync module. The internal change also adds a sealed-segment glob to a sync exclusion list. KiroCrew has no equivalent: SyncOrchestrator, NEVER_SYNC, _RSYNC_EXCLUDE_BASE and _validate_exclusions return zero hits across src/, and there is no sync/ package or docs/system-specs/modules/sync.md (a positive control on sel_hmac.key matched 10 files, so the search itself works). There is therefore no sync path that could carry sealed audit history off-host, and nothing to exclude.

Review round 24: two CI test failures, both in test harnesses, no product change

CI went red on 207dda436 with three failing backend shards and a failing Coverage Gate. Both underlying failures are defects in test code, not in the product. The product's identity guard behaved correctly throughout, and no src/ file changed in this round.

Failure A — test_a_same_size_replacement_is_detected_and_skipped (shards 3/4 on both Python 3.10 and 3.12). This test is new in this PR. It swaps the active log file mid-prune for a file with the same bytes but a different inode, and asserts the pre-replace identity check notices and skips the rewrite. The swap was written as unlink() followed by write_text(), and that is the bug: ext4's inode allocator commonly hands back the inode the unlink just freed, so on the CI runner the "new" file arrived wearing the original's identity. The guard then correctly saw an unchanged file and correctly proceeded with the rewrite — so the test failed while the product was right. It passed locally because this workspace's /tmp is tmpfs and $HOME is xfs, neither of which recycled the inode in 20 trials. The fix allocates the replacement while the original is still linked and then renames it over the top, which no filesystem can collapse into the same inode. A negative control now asserts the replacement's (st_dev, st_ino) actually differs from the original's, so the test can never again pass by proving nothing: with the inode deliberately reused the control fires and the test fails, which is the check that discriminates.

Failure B — test_first_deferral_logs_on_a_freshly_booted_host (shard 1/4 on Python 3.10 only). assert len(caplog.records) == 1 saw 4 records. The extra three were asyncio ERROR records ("Task was destroyed but it is pending!") from tasks left pending by unrelated generate_session_summary tests sharing the same xdist worker. caplog.at_level(..., logger=_LOGGER_NAME) raises the level of one logger, but caplog's handler still captures every propagated record from every logger, so a foreign ERROR counts toward a whole-capture assertion. Provenance, stated precisely: test/test_acp_watchdog_windows.py is unchanged by this PR — the diff against this PR's base commit is empty — and the leaking generate_session_summary tasks are not in the diff either. (It is not byte-identical to current origin/main, which is 1931 commits ahead and has since moved one import line; that difference is unrelated to these assertions.) What this PR changes is group membership: pytest-split balances shards by recorded test duration, so adding roughly 3,155 test lines re-shuffled which tests share a worker, and that moved the leaking work alongside this pre-existing fragile assertion for the first time. The fix scopes the assertions to the logger under test via two small helpers, _own_records and _own_text, rather than trying to hold the split still, which would not be durable. All seven whole-record assertions are routed through the first; a further six caplog.text assertions are routed through the second, which was found by probe rather than by inspection — the assert caplog.text == "" case is exposed to exactly the same foreign record, and an emptiness assertion is the one a stray log line is guaranteed to break. Both fixes are proven to discriminate: injecting one foreign asyncio ERROR into the call phase fails seven of the unscoped assertions and none of the scoped ones. The pending-task leak itself is worth a separate ticket; scoping the assertions is the minimal correct fix here and widening this PR to chase the leak is not.

Coverage Gate failed only because it fails closed on the backend lane (backend-test=failure), and the three failing shards skipped their coverage upload, so there was nothing to combine. It carries no independent defect and clears when A and B do.

One note on the product comment at the identity check: it already explains that a filesystem reporting no usable inode degrades to the size comparison rather than false-skipping, but it does not mention inode reuse after an unlink. That distinction cost a test round, and it is recorded here rather than grown into the comment block.

Review round 25: the Backend Tests (3.10, 4) hang is not in this PR's content

Backend Tests (3.10, 4) was cancelled at the 30-minute job cap on 8fd00a9ae. It is a real, PR-specific CI signal — the same shard passes at the current head of #3240, #3998, #4137 and #4138 — but it is not caused by anything this change contains, and no code was written for it. The round-24 test fixes did work: 3.10/1, 3.10/3 and 3.12/3, the three shards that failed at the previous head, all pass on this one.

What the log actually shows. The shard ran 13:11:37 to 13:41:13. Two events matter and neither was in the original report. At 13:18:01 pytest-xdist logged [gw0] node down: Not properly terminated followed by replacing crashed worker gw0 — a worker process died outright rather than a test failing, and the single F at that same timestamp is xdist marking the in-flight test failed, which accounts for the failure colour visible on the progress line. Output then reached 99% and stopped for 22 minutes until the runner killed the job, leaving the controller and all four workers alive as orphans (pytest 2286 plus python 2290/2293/2296 and 5398, the late-pid replacement gw0). Every orphan is a Python/pytest process; a leaked LaTeX child would appear under its own binary name, which is the first piece of evidence against a subprocess leak out of the papyrus change.

Shard 4 does not contain the files under suspicion. The log also records [pytest-split] No test durations found, so the split is even by count, which makes shard membership deterministic and locally reproducible. Reproducing the exact split on Python 3.10 with the same deselect list places test/test_acp_watchdog_windows.py in shard 1, and test/test_sel.py and test/test_papyrus_latex.py both in shard 3. None of the three is in shard 4, which is 404 files consisting of test/test_ws_offload.py, the test_xdist_* modules and 99 src/kiro_crew/apps/builtins/**/tests files. There are also zero papyrus test files anywhere under src/kiro_crew/apps/builtins/papyrus, so the changed latex.py is reachable only from test/test_papyrus_latex.py in shard 3. Independently: Backend Tests (3.12, 4) passed in the same run with the same shard membership, so whatever this is depends on the interpreter, not on which tests are in the group.

The one genuine coupling, stated plainly. Because the split is by count and this PR adds test items early in the collection order, the group boundaries shift. Comparing shard 4 at this head against the PR's base commit: the file list is identical (404 both sides), and at item level 47 items moved in and none moved out. All 47 come from a single pre-existing file, test/test_session_usage.py, and none from any file this PR touches. So this PR changed which group some pre-existing tests run in, exactly as it did for the watchdog test in round 24, without changing those tests. src/kiro_crew/security.py's change is a denylist addition (a sel secret leaf), a comment and a regex line-wrap; it can cause a refusal, never a hang.

Not reproduced, and therefore not patched. Shard 4 was run locally on Python 3.10.21 with the CI flags and completed in 3m16s with no worker crash and no stall; test/test_session_usage.py on its own passes in 1.6s. The shard was then re-run with -n 4, matching CI's worker count exactly, and completed in 3m21s with the same result -- so non-reproduction holds across every variable that can be controlled here: interpreter (3.10.21 against CI's 3.10.20), pytest version, worker count, split, group and deselect list. What remains uncontrolled is the machine: this host has 48 cores, so a 4-worker run leaves most of them idle, whereas the CI runner has 4 cores and real contention, which is the condition a shutdown or teardown race needs. A plausible mechanism is that two tests in that file block a thread in released.wait(30) while the surrounding asyncio.wait_for(..., timeout=10) cancels only the coroutine — cancelling a run_in_executor future does not stop the thread — and that loop.shutdown_default_executor() gained a timeout parameter only in 3.12, so 3.10 can wait on such a thread. That is an inference from reading the code and the version history, not a measurement, and it is recorded here as a lead rather than as a diagnosis. No fix is being pushed for it, because a fix whose regression test cannot be shown to fail first would be indistinguishable from a guess.

Two further checks, both prompted by the suggestion that the surviving child processes point at something in this change spawning without reaping. They do not. The session header records created: 4/4 workers, and the cleanup list is one pytest controller (pid 2286) plus exactly four python children (2290, 2293, 2296, 5398) — which is precisely the expected pytest-xdist process tree for a four-worker run, with nothing extra. The pid distribution corroborates that reading rather than merely permitting it: three of the four sit at consecutive low pids from the initial spawn, while the fourth is far higher because it is the replacement worker created after gw0 died six and a half minutes into the run. The original gw0 is absent from the cleanup list, which is what "it crashed" looks like from the outside. So the four children are accounted for as workers, and there is no unreaped-spawn residue to attribute to anything in this diff.

The shard did run the full backend suite rather than a narrowed one — REDUCED: false, surface selector unusable, 4 workers [14223 items] — but that is not a distinguishing feature either. Backend Tests (3.10, 4) on #3997 (13,769 items) and #3998 (14,143 items) logged the same selector fallback, also with REDUCED: false and four workers, and both passed. Together with #3240, #4137 and #4138 that is five full-suite control runs of this shard that succeed where this head hangs, which is what makes the signal PR-specific; it is also why the fallback itself can be set aside as a cause.

Review round 26: four blocking items fixed in code, one anchor escalated

All four BLOCKING items from the GPT lane are fixed in code, each with a regression test that fails before the fix and passes after. The fifth item is a non-blocking FINDING whose anchor cannot be satisfied without a measured startup regression; it is reduced but not cleared, and the remaining decision is flagged for rnoack below.

Rotation no longer runs on the asyncio event loop. A critical=True audit writes synchronously on the caller's thread, and at least one caller reaches that path with no executor hop — spine/agent_runner.py:1589 calls sel().log_tool_invocation(..., critical=True) directly inside async def _approve, so it runs on the loop. (Two sibling async sites are already offloaded via asyncio.to_thread, which is what makes the un-offloaded one the exception rather than the rule.) _flush_batch now asks whether it is on a loop thread and, if so, hands rotation to a short-lived helper thread instead of running it inline; every other caller — background writer, sync=True, CLI — still rotates inline, because blocking is fine there and routing everything to a helper would change those paths for no reason. The loop check reads sys.modules rather than importing asyncio at module scope: a process that never imported asyncio cannot have a loop, so this costs nothing on the boot path. The hand-off is single-flight, so a burst of critical audits cannot spawn a thread per event. Cost of deferring: the active file can overshoot max_bytes by the appends that land before the helper takes _lock — the same soft-cap overshoot _maybe_rotate already documented, not an unbounded one, because the hand-off is immediate rather than conditional on later traffic. Two tests: one asserts rotation does not run on the loop thread, and a negative control asserts an off-loop caller still rotates inline (without it, deferring unconditionally would pass the first test while silently changing the writer path).

A zero-backup discard no longer destroys a concurrent append. The append path opens with O_APPEND (_flush_batch), so after the old unlink a writer in another process that already held the fd wrote to an orphaned inode and its bytes vanished with no error anywhere. The discard now truncates the active file in place instead: the fd still names the live file, so that write lands at the new EOF and survives. Its prev_hash then refers to a tip that is gone, which verify reports as a chain break — fail-loud, which is strictly better than the silent loss it replaces. This deliberately does not claim to make multi-writer append correct; that remains the single-writer daemon named elsewhere in this description. It removes the narrower case where discarding destroys a write that had already succeeded. The lane's own remedy — disable zero-backup rotation until append and discard share the lease — was not adopted, because it removes a configuration the feature offers rather than fixing the race, and sharing the lease on every append is a hot-path contract change well beyond this PR. One existing negative control asserted the active file was gone after a clean discard; that assertion described the disposal mechanism rather than the property, and now asserts the file is emptied.

Partial overflow eviction can no longer strand deletions without the marker. _mark_evicted() ran after the eviction loop. missing_ok=True suppresses only FileNotFoundError, so a permission or other OS error on a later segment carried control out of _evict_over_budget — and _flush_batch swallows that (its _maybe_rotate guard), so the marker was simply never written while the earlier deletions stood, and verify would report a genesis chain break against legitimately retained history with nothing logged. The marker is now written on the first successful deletion, guarded so it still writes at most once per pass: no new cost.

The concurrent-rotation test now reaps every child. for p in procs: assert p.wait(...) == 0 exits the loop on the first non-zero exit — and p.wait() itself can raise TimeoutExpired — leaving the remaining children neither signalled nor waited for. Cleanup moved into a finally that kills only what is still running and raises nothing of its own; verified that the original assertion still fails for its own reason by forcing the children to exit non-zero.

For rnoack — the top-level-imports anchor on the local metrics import. This one cannot be cleared without the regression the code comment describes, so it needs your call rather than a decision of mine. Measured at this head: importing sel leaves 190 modules in sys.modules; adding kiro_crew.metrics.provider at module scope pulls 81 more (271 total), including OpenTelemetry, onto every importer of the security event log — which is constructed very early in startup. Correction, made in round 27: this paragraph originally reported 145 modules for sel and said the code comment's figure of 190 "does not reproduce". That was wrong, and the two numbers are the same measurement: 190 is the total sys.modules count after importing sel, the convention both the comment and this repo's D-76 entry use, and 145 is that figure minus the 45 modules a bare interpreter already holds. The comment's 190 and its +81 both reproduce exactly, so nothing in the code comment needed changing. What did change: the two identical local imports the lane did not both cite are now one, so the finding is no longer misread as a one-line hoist. The choice is between accepting the local import as the documented exception, or accepting +81 modules and OpenTelemetry on the boot path. I have not weakened the anchor or the comment either way.

Review round 27: three reads of an untrusted segment, and a telemetry regression from round 26's own fix

The GPT lane raised three BLOCKING items, all in sel.py, all reachable from a segment this process did not write — a planted symlink, a crash-truncated tail, and a line of valid-but-non-object JSON. All three are fixed in code with a regression test each, and each test was verified to fail with only its own fix reverted, identified by failing test name rather than by a count. The Opus lane then raised a non-blocking FINDING which is a real regression introduced by round 26's event-loop fix; it is fixed here rather than left for a later round to re-raise as blocking.

A planted symlink is now refused where O_NOFOLLOW does not exist. _open_segment built its flags as os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0) | ..., so on Windows — which has no O_NOFOLLOW — that term became 0 and the protection silently disappeared on exactly the platform the finding names. The S_ISREG check beside it does not cover for that: fstat runs on a descriptor the open has already followed, so it reports the target, and a link aimed at a large regular file passes it cleanly. Segment names are agent-writable before this feature's sensitive-path floor lands, so the consequence is an unbounded read through /api/sel/events into the gateway. Where the flag is unavailable the link is now refused explicitly with an lstat pre-check before the open. That is honestly weaker than a kernel-level refusal — it trades a guaranteed follow for a lose-the-race window, which is the strongest refusal a platform without O_NOFOLLOW offers — and the docstring now says so rather than claiming S_ISREG stands in for the flag. The existing symlink test used to skip itself when O_NOFOLLOW was absent, on the reasoning this finding refutes; that skip is gone, so it now covers both branches. Windows itself was not tested, because this host cannot: the new test removes the flag instead of claiming a Windows run.

A corrupt newest record no longer makes a recent segment look aged. _newest_timestamp_of walks the tail backwards and continued past any unparseable line. Entries append in time order, so the next line back is older: a segment whose newest record was truncated mid-write reported the older record's timestamp, and age pruning then deleted recent forensic data on the strength of a stamp that did not describe it. It now returns None at the first unparseable tail record, which is the fail-closed answer both callers already read as "cannot prove aged" and keep. The AttributeError catch stays for the reason its old comment gives — a bare scalar has no .get, and uncaught it escapes to _maybe_rotate and permanently disables rotation — so that escape remains closed; only the fall-through is gone. One existing test asserted the fall-through by value while its own docstring named the real intent ("must not raise"); it now asserts the property instead of the mechanism.

recent() no longer returns non-dictionaries. It appended json.loads(line) for anything that parsed, so a segment line of 123 put an int into a list annotated -> list[dict], and kirocrew security events raised AttributeError on e.get(...) part-way through listing. It now appends only parsed dictionaries. That the module already knew non-object lines occur in these segments is not an inference: the AttributeError comment in _newest_timestamp_of says so explicitly.

Deferred rotation emits its counters again (Opus FINDING, and a regression from round 26). Round 26 moved rotation off the event loop by handing it to a sel-rotate thread. _flush_batch captures early_before under _lock and compares it just after releasing — but on the deferred path it has only spawned the thread, so at that moment nothing has rotated and self._early_evictions > early_before is false. kirocrew.sel.early_eviction.count — the signal that audit evidence was dropped before its retention window — therefore under-reported to approximately never on the loop-driven path most gateway audits take, and a failure inside the deferred rotation set no rotation_failed flag either. The emit block is now a shared _emit_rotation_counters helper, called from whichever path actually did the work: inline callers as before, and the deferred body from inside itself once the rotation has happened. Only telemetry was affected — _evict_over_budget's logger.warning fired unconditionally throughout — so this was never data loss, but it silenced the one counter that reports it. A negative control asserts the inline path still emits exactly once and no more, so relocating the emit could not quietly drop or double it.

The top-level-imports FINDING remains rnoack's call and is unchanged. Nothing is red because of it: the only failure-class check on this head is the GPT lane itself, and the rule is documented blocking: false in this repo's own code (cli_bench.py). The measurement is restated correctly in the round 26 entry above, along with a correction to the figure that entry originally reported.

Verification (round 27)

Each of the four fixes was reverted individually, and the result was captured by failing test name rather than by count: reverting the symlink refusal fails test_symlinked_segment_is_refused_when_nofollow_is_unavailable with "DID NOT RAISE"; reverting the tail fix fails test_corrupt_newest_record_does_not_make_a_recent_segment_look_aged together with the updated existing test; reverting the dictionary filter fails test_recent_skips_non_object_json_lines; reverting the counter relocation fails test_deferred_rotation_emits_the_early_eviction_counter while its inline negative control keeps passing throughout. No revert broke another item's test, which is what shows the four are independent rather than one defect seen four ways. Gates: 5116 passed / 8 skipped across the 60 test files that reference sel, mypy clean over 982 source files, flake8 and isort clean. Three failures in test_dashboard_handlers_core_coverage.py (TestPipInstallChannel, TestSttConfigEndpoint) are environment-dependent and were reproduced identically on an unmodified baseline with this PR's two files stashed, so they are not called pre-existing on assertion alone.

Review round 28: two of my own tests asserted on bytes they had written in text mode

Backend Tests (Windows) (3) failed with assert b'y\r\n' == b'y\n' in two test_sel.py tests. Both wrote their fixture with Path.write_text("y\n") and then byte-compared the file read back in binary. write_text opens in text mode and passes no newline=, so "\n" is translated to os.linesep on the way to disk — three bytes on Windows, two on POSIX — and the comparison fails on the fixture rather than on anything the test is about. Fixed in the fixture with write_bytes(b"y\n"), which puts identical bytes on disk on every platform. No product code was touched: sel.py's newline handling is unchanged, and changing it to satisfy a test would rewrite the durable log format on every platform for a fixture bug.

One of the two, test_open_segment_refuses_a_symlink, is pre-existing — but it had never run on Windows before, because it used to skip itself when os.O_NOFOLLOW was absent. Round 27 removed that skip (the reasoning behind it was the very claim the GPT sel.py:149 finding refuted), which is what first exposed this latent fixture bug. So the failure is properly attributed to round 27 rather than to the older test.

Confirmed by execution rather than by reading the code, because the mechanism is platform-dependent and this host is Linux: emulating Windows text mode (open(..., newline="\r\n")) puts b"y\r\n" on disk against b"y\n" on POSIX, and re-running the two real tests under a shim that gives Path.write_text Windows newline semantics reproduces the CI message byte for byte — AssertionError: assert b'y\r\n' == b'y\n', both tests. After the fixture change both pass under that same shim and on POSIX. Windows itself was not run; the shim reproduces the translation, not the platform.

Neither assertion was weakened, and that is verified rather than asserted. test_symlinked_segment_is_refused_when_nofollow_is_unavailable is round 27's regression test for the planted-symlink finding, so a version of it that passed without the guard would be worthless. With the is_symlink() pre-check and O_NOFOLLOW both reverted, the two tests fail with DID NOT RAISE <class 'OSError'> — on POSIX and under the Windows shim. They still fail for the reason they exist for.

An audit of the whole file found no third instance: there are five byte-literal assertions in test_sel.py, and the other three read data written with write_bytes. Worth recording as a possible latent issue, not acted on here: newline= appears nowhere in this repo's text-mode writes generally, so any future test that byte-compares a write_text fixture will hit the same trap on Windows. That is a repo-wide observation about test fixtures and not a defect in this PR's product code.

Also worth correcting from the brief that raised this: the known marginal Windows hang is shard (2) at the 40-minute cap, and on this head shard (2) completed success — as did (1) and (4). Only (3) failed, on a real assertion in 9m45s, so nothing here is that hang.

Verification (round 28)

5116 passed / 8 skipped across the 60 test files that reference sel; mypy clean over 982 source files; flake8 and isort clean. src/kiro_crew/sel.py has a zero-line diff for this round, which is the check that the fix went into the fixture. The three test_dashboard_handlers_core_coverage.py failures (TestPipInstallChannel, TestSttConfigEndpoint) are the same environment-dependent ones reproduced on an unmodified baseline in round 27.

Review round 29: a concurrent seal-then-prune could strand an in-flight append

The GPT lane raised one BLOCKING item: with two SEL processes, one can seal the active log and age-prune the resulting segment while the other still holds an open append fd, and on POSIX that write then lands on an unlinked inode and the audit event is gone. The finding is real and is fixed in code. The lane's own remedy — revert rotation until appends are serialized with sealing and deletion — was not adopted: it removes the feature rather than closing the window, and serializing every append behind the cross-process lease would put a file lock on the hot audit path.

The existing lease is genuinely cross-process, and that still does not refute the finding. _seal_lease opens a lock file in the segment directory and takes platform_compat.try_acquire_lock(fd, exclusive=True), which is fcntl.flock(LOCK_EX|LOCK_NB) on POSIX and msvcrt.locking on Windows — a real filesystem lock a second process observes, not in-process bookkeeping. But it serialises rotator against rotator, which is what it was written for: it stops two processes sealing out of order. The append path takes no lease at all, so it does nothing about appender against rotator, which is precisely the scenario raised. Quoting the lease here would have been a refutation of a claim nobody made.

The fix distinguishes the two things a concurrent rotation can do to an open fd, because only one of them loses data. A seal alone is a rename: the link count stays 1, the fd still names that inode under its new segment number, and the record lands at the end of that sealed segment, correctly chained and readable — nothing is owed, and re-writing it would duplicate an audit record. A seal followed by the prune's unlink drops the link count to 0, and only then are the bytes reachable by nobody. So after flushing, the append asks os.fstat(fd).st_nlink == 0 — a question about the inode it actually wrote to, not about a path a concurrent roll has already replaced — and on a positive answer re-writes those records once to whatever is now the active file. Any OSError answers False, because unprovable loss must not trigger a re-append. Windows reports 1 here and refuses to unlink an open file, so the race cannot arise there.

Two honest limits. The retry is bounded to one attempt rather than spinning on the writer thread under _lock; when that one attempt cannot place the records either, it raises, and round 32 below is what turned that into a refusal for a fail-closed caller and a warning for everyone else. And the re-written records' prev_hash may now refer to a tip the prune deleted, which verify_integrity reports as a chain break — the same trade the zero-backup discard path already takes, on the same reasoning: a break verify REPORTS beats evidence that silently is not there. This does not claim to make multi-writer append correct; the single-writer daemon named elsewhere in this description is still the answer for that. It removes the case where rotation DESTROYS an append that had already succeeded.

The top-level-imports FINDING is unchanged and remains rnoack's call. It was not re-litigated and the import was not hoisted: the measurement (+81 modules, including OpenTelemetry, on every importer of the security event log) is recorded in the round 27 entry above and in the code comment at the call site, and no check is red because of it.

Verification (round 29)

The regression test drives the rival seal + prune from inside the real append window — the hook fires between the open and the write, so the race is created rather than simulated afterwards — and asserts the record is still readable. It discriminates in both directions, which matters because the two failure modes here are opposite. Disabling the detector fails only test_stranded_append_is_rewritten_and_still_readable, with both controls still passing. Making the detector always fire — the shape a naive "the active path changed, so re-append" fix would take — instead fails both controls: test_a_normal_append_is_not_written_twice reports every record duplicated (['c','c','b','b','a','a']), and test_a_seal_without_a_prune_needs_no_rewrite fails because a rename is not a loss. A fix that is too eager is caught as loudly as one that is too weak.

Gates: 5119 passed / 8 skipped across the 60 test files that reference sel, mypy clean over 982 source files, flake8 and isort clean. The three test_dashboard_handlers_core_coverage.py failures are the same environment-dependent ones reproduced on an unmodified baseline in round 27. Opus 4.8 Review had already landed "No findings." on the replaced head before this push, so no in-flight verdict was discarded.

Review round 30: round 29's own tests constructed a race Windows cannot have

Backend Tests (Windows) (3) failed two of the three tests round 29 added, both with the audit record missing (observed operations=['before'], and an empty ops list). The log named the cause: PermissionError: [WinError 32] ... security_events.jsonl -> sel/security_events.jsonl.1, followed by SEL append failed for 1 events. The product code is not at fault and was not changed this round -- src/kiro_crew/sel.py has a zero-line diff here.

What held a handle open across the rename was the append's own write fd, and only because the TEST put it there. Those two tests inject a rival seal from a hook on _ends_without_newline(), which is called inside the with os.fdopen(fd, "a") block the append opens two lines earlier. On POSIX that is exactly the race being tested. On Windows a rename fails while any handle on the source is open, so the injected os.replace raises WinError 32 from inside the append, the append is abandoned, and the assertion then fails on a fixture that cannot exist on that platform rather than on the behaviour under test.

Production never does this, verified at source rather than assumed. In _flush_batch the rotation runs at :498/:501 and the append opens its fd afterwards at :526, so the two never overlap in one process. Across processes on Windows a rival holding the active file open makes OUR seal raise instead -- and _seal_leased already anticipates precisely that, with an except OSError whose comment names "a Windows sharing violation": it drops the claimed placeholder and re-raises, and _flush_batch degrades that to "appending without rotating" plus the rotation-failure counter. The file rolls on a later flush and no audit record is lost. During rotation the only touches to the active path are a stat() and the os.replace itself, so no reader handle is held across it either.

The fix scopes the two POSIX-semantics tests to POSIX and adds the Windows-side coverage that was genuinely missing. The mechanism they exercise is an inode with st_nlink == 0 -- unreachable on Windows, which refuses both to rename and to unlink an open file, which is the same reason _fd_is_unlinked's own docstring already gave for the race not arising there. They now carry @pytest.mark.skipif(os.name == "nt", ...), the idiom this file already uses seven times. No assertion was weakened or removed: all of them still run in full on the platform where the mechanism exists. In exchange, a new test_a_seal_that_fails_does_not_lose_the_append runs on every platform and pins the behaviour Windows actually has -- that a refused seal must not take the append down with it, since rotation runs first and a fatal rotation would drop every audit event during a contended roll.

Verification (round 30)

The failure was reproduced on this Linux host before anything was changed, by emulating Windows rename semantics for the seal only: both tests then failed with the same two assertion messages as CI, the same sel.py:566 warning, and the same 2-failed/1-passed split. Scoping that emulation mattered -- a first attempt refused every os.replace and failed all three tests during setup on an unrelated HMAC-key write, which reproduced nothing and was my harness rather than the defect.

The new Windows-side test was made to discriminate rather than merely pass. As first written it was vacuous: with no prior content _maybe_rotate returns before the seal, so the refusal never fired and the test passed even with the rotation guard reverted to re-raise. It now primes the active file, asserts the refusal actually fired, and fails with PermissionError when the guard is reverted. The two skipped tests were re-checked with their markers in place and still discriminate on POSIX: disabling _fd_is_unlinked fails test_stranded_append_is_rewritten_and_still_readable while the other three pass, and all four run (none accidentally skipped) on this host.

One limit stated plainly: the skip firing on Windows is standard pytest behaviour and matches the seven existing uses in this file, but it was not executed on Windows -- this host cannot, and an attempt to force os.name = "nt" in-process is not a substitute (it breaks stdlib shutil and ctypes, which branch on it legitimately). The Windows shard on this sha is the real check.

Gates: 5120 passed / 8 skipped across the 60 test files that reference sel, mypy clean over 982 source files, flake8 and isort clean. The three test_dashboard_handlers_core_coverage.py failures are the same environment-dependent ones reproduced on an unmodified baseline in round 27.

Review round 31: a shared black-baseline graduation, and the rebase it required

Backend Lint & Type Check (3.12) failed at Check formatting (black, baselined) with black gate FAILED: 0 new offender(s), 1 graduated entr(y/ies) to prune naming src/kiro_crew/mcp_gateway/preflight.py. Read the two halves: this diff introduced no formatting violation. The failure is the opposite direction -- a file listed in .github/black-baseline.txt became black-clean on main, and the gate requires the stale entry be pruned. Confirmed at source in scripts/check_black_formatting.py: new offenders are scoped to changed files (new_offenders = sorted(unlisted & changed), line 233) while the graduation half is global (graduated = sorted(baseline - unformatted), line 234), so every open PR reddens on a file it never touched. Eight other-author PRs (#4451, #4450, #4446, #4440, #4434, #4412, #4388, #4386) carry the same one-line deletion, each showing .github/black-baseline.txt +0 -1.

Fixed, and the diff is exactly that one deleted line (git diff --numstat reports 0 1). But it needed a step the gate's own message does not mention: this branch was 119 commits behind main and predated #4244, so it did not contain .github/black-baseline.txt at all. Deleting a line from a file absent from the branch is not expressible -- materialising it would have shown up as a ~1,400-line file ADDITION rather than a one-line deletion, which is the broad-rewrite outcome that would rightly draw a finding. So the commit was first rebased onto current main (79e7268e9), which brings the baseline in as pre-existing context, and only then was the line removed. The rebase was proved clean in a throwaway worktree before the real branch was touched: rc=0, zero conflicted files. It also takes this PR from 119-behind to 0-behind, which removes the stale-base analyzer pathology this PR hit in earlier rounds.

The graduated entry is the ONLY one owed, verified rather than assumed. Every unformatted Python file this PR touches is already baselined -- sel.py, test_sel.py, security.py, latex.py, test_papyrus_latex.py, test_security.py -- which is precisely why CI reports 0 new offender(s); the two that are black-clean (test_acp_watchdog_windows.py, test_security_posture.py) are correctly unlisted. No file in this diff is both baselined and clean, so none would newly require pruning.

Verification (round 31)

The gate's graduated half went 1 to 0 on the rebased tree, and the baseline went 1,441 lines to 1,440 with the removal at line 399.

Two local-only artifacts are worth naming so they are not mistaken for regressions. First, run outside CI the script prints black gate scope: undeterminable (judging the whole tree) and reports 13 new offenders -- the unscoped fallback at line 227. Those 13 were checked against this PR's changed files and the intersection is empty; they are main's own baselined-exempt files and cannot fire on a scoped run. Second, the rebase raised local failures from 3 to 19: 16 of those are in test/test_file_sheet.py, which is absent from the pre-rebase head and present on main, so it arrived with the rebase; 13 failed on ModuleNotFoundError: openpyxl and the other 3 on the 501 that handlers/files.py:3256 documents as the soft-import-absent path. Installing openpyxl cleared all 16, leaving exactly the 3 environment-dependent test_dashboard_handlers_core_coverage.py failures reproduced on an unmodified baseline in round 27.

Post-rebase gates: 5158 passed / 8 skipped, this PR's own 9 rotation tests all passing, mypy clean over 987 source files (up from 982 as main added modules), flake8 and isort clean.

Review round 32: a stranded audit could report success with its evidence unreachable

The AI review lane raised, as blocking, that the stranded-append retry added in round 29 discarded its own outcome: _reappend_stranded swallowed both of the ways it can fail, so _flush_batch returned normally either way. That matters because of what critical=True promises. It is the audit-or-deny contract — the caller writes the audit first and performs the action only if the write did not raise — so a silent give-up means the caller grants a permission with no record of it anywhere. That is fail-open, the one direction the contract exists to prevent.

I reproduced it before changing anything rather than reasoning from the code. Driving the real rival seal-then-prune from inside the append window and then making the retry's open fail, a critical=True caller observed no exception at all, the granted operation was absent from every readable record, and the chain tip had advanced onto records no reader can reach — so the next batch would have chained off a hash that is not on disk.

The fix makes _reappend_stranded raise OSError on both failure modes instead of logging and returning: when the re-open or write fails, and when the retry is itself stranded (detected, never signalled before). Nothing new handles that raise — the append's existing OSError handler already does exactly the right two things, rolling the chain tip back off the unreachable records and re-raising only when raise_on_error is set. So a critical caller now refuses the action, and a best-effort caller still gets the documented swallow-and-warn. The synthetic error uses EIO rather than ENOENT deliberately: after a rival's roll the active path usually does exist, and ENOENT would surface as FileNotFoundError, a name that invites a future caller to treat an unreachable audit as "not created yet, benign".

Both directions are checked, because both failure shapes here are silent. Against the pre-fix code the two new fail-closed tests fail with DID NOT RAISE, while the four earlier race tests are untouched. Against an over-eager variant that propagates for every caller, the opposite test — test_a_best_effort_append_still_does_not_raise_when_the_retry_fails — is the one that fails, with the EIO error, and the fail-closed pair keeps passing. A fix that raises too readily is caught as loudly as one that stays too quiet.

The function-local metrics import flagged alongside it is unchanged and remains rnoack's decision, on the measurement already recorded above: hoisting it pulls 81 further modules onto the import graph of every module that imports the security event log. It is a non-blocking finding, so it is not what gates this branch.

Review round 33: a rejected operator override was copied into the log verbatim

_env_int reads the three rotation knobs during SecurityEventLog.__init__ and fails soft on a value it cannot parse, logging a warning and using the default. The warning interpolated the rejected value with %r, so whatever the operator had put in the environment variable was written into the log in full. An operator who pastes a token into the wrong variable — the variables sit next to each other in the same block of configuration — has it copied into a log file, which is the opposite of what a security audit log should do. I reproduced it before changing anything: a distinctive sentinel set as KIROCREW_SEL_MAX_BYTES came back in the emitted record character for character.

The obvious fix is to drop the value from the message, and I did not take it, because the value is the only thing that line exists to say. _env_int deliberately refuses to guess: it will not silently substitute a number the operator did not write, so it tells them their knob was ignored. Strip the value and the operator learns a knob was ignored but not which spelling was rejected, which is the half that leads to the typo.

So the value is echoed only when it cannot carry a secret. It is shown when it is at most 16 characters AND spelled entirely from digits and integer punctuation (0-9, +, -, _, ., ,, space, tab); anything else is replaced by its length. A credential cannot be written in that alphabet, and the typo an operator most needs to see always can — 1,048,576, 1 000, 1.5 are all shown as typed. Note that the exclusion of letters is deliberate rather than incidental: allowing them would show 100MB, but it would equally show a short password, so 100MB is reported as <5 chars>. The knob name and the default are unconditional in both branches, because those are what make the warning actionable at all. The rule sits in one module-level constant next to the defaults rather than inline, and the character set is checked with a frozenset rather than a regular expression so the module's import graph is unchanged.

Both directions are pinned by tests, because either mistake is silent. test_a_rejected_override_that_could_be_a_secret_is_not_echoed sets a secret-shaped value, asserts it appears in no captured record, and asserts the length summary and the knob name do; against the previous code it fails with the leaked record quoted back. test_a_numeric_typo_is_still_echoed_so_the_warning_stays_useful is the guard in the other direction: against a variant that redacts unconditionally, it fails because the diagnostic is gone. Each control fails only its own test while the other passes.

Two further items raised alongside this one were left alone deliberately. The function-local metrics import is unchanged and remains rnoack's decision on the measurement already recorded above — hoisting it pulls 81 further modules onto the import graph of every importer of the security event log. The segment-number reuse note is already documented in two places with a measurement and covered by its own test class, so there was nothing to add without duplicating what is there.

Review round 34: retention could delete a segment another process had just written

Two blocking items, both in code this branch adds.

The first is a real data-loss window in prune(). Its first stage deletes whole sealed segments it has proved older than the retention cutoff, and it held only _lock — a threading.Lock, which orders nothing against a second writer process. Segment numbers are reused: once a prune empties the sealed set the next seal allocates 1 again, so a path this stage proved aged can name a different, brand-new, fully-populated segment by the time it is unlinked. The stage also counts entries first, which streams the whole segment, so the gap between proving a file aged and deleting it was a full ~100 MB read. The rotation path was already safe — it runs inside the cross-process seal lease — so the exposure was exactly one caller, and the function's own docstring had drifted into claiming no cross-process lock was needed, reasoning only about prune-versus-prune and never prune-versus-seal. That docstring is corrected.

I took the seal lease around the stage and skip the stage when a rival holds it, rather than the narrower alternative of re-checking the segment's identity immediately before the unlink. The reason is written a few lines further down in this same function, about the second stage: identity re-checking narrows the window to one stat but closing it needs a lock the other writer also honours. For the second stage no such lock exists, so narrowing was the best available. For seals it does exist, every seal takes it, so closing is available and worth taking. Skipping costs at most a day of over-retention because prune runs daily, which is the price this file already accepts for the second stage. The lease is taken at the call site rather than inside the pruning helper on purpose: the lock is not re-entrant within one process, so acquiring it inside the helper would make the rotation path — which already holds it — silently skip its own retention pass.

Adding the lease initially broke the planted-link protection, and the existing test for it caught the mistake rather than my own review. The lease file lives inside the sealed-segment directory, so opening it before the directory guard runs creates it through a planted symlink, outside the sensitive-path floor — precisely what that guard exists to prevent. The seal path already documents the required ordering, and the stage now follows it: the directory guard runs first, then the lease.

The second item is test hygiene with a real cross-test cost. The deferred-rotation test polls a spy that records the thread and then calls the real rotation, so the poll released the moment the spy was entered rather than when rotation finished. The test returned while the helper thread was still sealing, evicting, pruning and emitting metrics, all inside the per-test directory that the fixture was about to delete and against a singleton a sibling fixture was about to reset. The thread is now joined with a bounded timeout and asserted stopped before the test returns; nothing was removed.

Both fixes are pinned in both directions. Removing the lease makes the new prune test fail with the aged segment deleted under a rival's lease, while the control that retention still works passes; forcing the stage to skip unconditionally fails that control instead. For the thread test, removing the join while keeping the assertion fails — but only intermittently, one run in three, because the thing it detects is itself a race. That is worth stating plainly rather than dressing up: the assertion documents the invariant, and the join is what makes the outcome deterministic.

Two further items raised in the same review were left alone. The function-local metrics import is unchanged and remains rnoack's decision on the measurement recorded above. The watchdog-log filtering hunk stays: the repo's own scope check passes at this head, and removing it would re-expose the suite to the cross-talk it was added to stop while all four Windows shards pass with it present.

Review round 35: a truncated segment read clean, and a planted one was served as audit

Two blocking items, both in sealed-segment code this branch adds, and both attacking the same property: the local chain is tamper-evident, not tamper-proof, so anything that lets it read clean over altered history removes the only guarantee it offers.

The first: seeding the chain tip walks newest to oldest and takes the first tip it finds, because right after a rotation the active file is legitimately empty while the tip lives in the newest sealed segment. That fallback also swallowed a sealed segment truncated to zero — the tip came from an older segment, later entries chained onto it, and the walk reported equal totals over history that was gone. I reproduced it before changing anything: a healthy fixture verified four of four, then emptying the newest sealed segment gave one of one, clean, with four records unaccounted for.

The remedy offered was to revert the fallback, and I did not, because that is broader than the defect. It would restore the false break at every rotation seam the fallback exists to prevent, and it would also break the absent and legitimately-empty cases, which are not what failed. The discrimination the finding actually needs turns out to be available from the path's ROLE: sealing only ever moves a non-empty active file onto a number, so a sealed segment holding no record at all is proof of truncation, while only the active file can legitimately be empty. The verify walk now counts such a segment the same way it counts an unreadable one — a mechanism this file already had, and whose comment already spells out why it is load-bearing: a segment contributing nothing to either counter otherwise reports success over a segment that went unread. The tip seeding logs the same condition loudly instead of passing over it in silence.

The second: the newest-first display reads sealed segments without checking their MAC. A numbered segment is a plain file in a directory an agent may already be able to write, so a planted one is parsed like any other and an attacker-authored approval reaches the events API and the CLI listing as an audit record. Records from sealed segments are now authenticated against this install's key before being returned, reusing the digest the verify walk already computes and the constant-time comparison already used for the eviction marker.

Two constraints shaped how. Failing authentication drops the record and continues rather than raising, for the same reason a valid-JSON-but-not-an-object line already does: the method is annotated as returning dicts and every consumer calls .get on the elements, so raising would abort a listing mid-way. And the drop is counted once per call rather than logged per line, so a planted segment cannot double as a way to flood the log. The second remedy offered — reverting the sealed-segment read — would delete the feature this branch exists to add, so it was not taken.

One scope limit stated rather than left implicit: the active file is still read without authentication, exactly as before this branch. The surface this change closes is the sealed-segment read, which is what this branch introduces; authenticating the active file too is a wider change to behaviour that predates it.

All four directions are pinned. Reverting the truncation detection makes the truncated-segment test fail with the clean-chain message while the other four pass; dropping its role check instead makes the empty-active-file test fail with a manufactured defect. Reverting authentication makes the planted-record test fail with the forged operation served; rejecting every sealed record instead makes the genuine-records test fail at one of four served. Each control fails only its own test.

The function-local metrics import raised alongside these is unchanged and remains rnoack's decision on the measurement recorded above.

Review round 36: the per-line cap bounded each line and left the aggregate unbounded

One blocking item, in the shared segment reader this branch adds. The reader refused any single line over 1 MiB and then appended every line it accepted to a list, so a segment made of ordinary short lines tripped no cap and still materialised whole. I measured it before changing anything: a 3.2 MB segment drove peak traced allocation to 22.9 MB on both the verify and the events path, roughly seven times the file because a decoded string costs several times its bytes. At the 100 MB active-file cap that is hundreds of MB of live objects on a read, which is the gateway OOM the finding names.

The tempting refutation is to point at the per-line cap and call the read already bounded. It does not hold, and the distinction is the whole finding: the cap is per LINE and the exhaustion is the aggregate LIST. Two hundred thousand short lines pass the cap individually and cost the sum of them together.

The argument was already in this branch, on the finding's side. The entry counter hand-rolled its own streaming loop specifically because the shared reader accumulated, and its docstring said so in those terms. What was left was that the other three call sites still went through the accumulating helper — the residue of a fix already made once.

So the reader now yields instead of accumulating, rather than growing a fourth hand-rolled loop. That choice follows the helper's own stated reason for existing: every segment read goes through one place because, when three sites each slurped a whole segment, bounding one of them left the other two open. Yielding bounds all three at the source, and two of them become constant in the file's size for free. The events path additionally needs a tail rather than a stream, so it now fills a bounded double-ended queue sized to the events it can still use, built while the handle is open — a generator consumed after the enclosing block would be reading from a closed descriptor.

One narrowing I took deliberately and want visible: the retained window is raw lines, so a segment whose newest lines are blank, corrupt or forged now yields fewer events there and the walk falls through to the next older segment instead. That only arises on a segment already truncated or planted, this method is documented as best-effort newest-first display rather than the integrity oracle, and the verify walk is unaffected — so it is the right trade against exhausting memory on a healthy log.

The conversion introduced two structural hazards of its own, and both are closed. The first is the more serious: last round's empty-sealed-segment detection tested the segment with a pre-pass over its lines, which is safe against a list and destructive against a generator — the pre-pass would consume the segment and leave the accounting loop with nothing, so verify would report no entries at all for every sealed segment and call that clean. That is the precise failure last round's fix exists to prevent, and it would have been silent. The observation is now recorded during the single pass instead, and the control proves the change was load-bearing rather than cosmetic: restoring the pre-pass makes both of last round's tests fail. The second is that the over-cap error now surfaces mid-iteration rather than from the call, because a generator body does not run until advanced, so each call site keeps its handler around the loop and not merely around the call; the planted-segment test confirms that path still folds into the unverifiable count instead of escaping.

The entry counter's docstring is corrected in the same commit, because it asserted the shared reader accumulates and that is no longer true. It still keeps its own read, but for a different reason, which the docstring now gives: it degrades to a floor count on an over-cap line where the shared reader raises, and an observational count wants the floor.

Three controls, each discriminating independently. Reverting the reader to accumulate fails all four new tests, naming the bytes retained against the segment size. Keeping the reader lazy but restoring the whole-file read on the events path fails only the events test, isolating the tail bound from the streaming half. Restoring the consuming pre-pass fails last round's truncation and empty-active-file tests instead, which is the regression that would otherwise have shipped unnoticed.

The function-local metrics import raised alongside this item is unchanged and remains rnoack's decision on the measurement recorded above.

Review round 37: proving a segment aged is not the same as trusting the proof

One blocking item, in the sealed-segment age pruning this branch adds. The prune decided deletion from the newest record's timestamp, and that record was never authenticated. A writer with the segment directory open — agent-writable until the sensitive-path floor lands — edits the oldest segment's final record so its stamp reads older than any cutoff. The stamp parses, compares aged, and the segment is unlinked down the correct code path.

The obvious defence is the fail-closed guard already sitting on the line above, which keeps a segment whose stamp is absent or unparseable. It is a real property and it is not this one: a forged stamp is neither absent nor unparseable, so nothing fails and the guard never fires. The two propositions are compatible, which is exactly why quoting the guard would settle nothing.

What makes it worse than a wrong deletion is what follows the unlink. The prune marks the log evicted, and the verify walk treats a genuine eviction marker as licence to adopt the next surviving segment's own prev_hash as its baseline rather than demanding the genesis anchor. So the erasure authenticates itself: the chain reports clean over records deleted on a forged stamp. That is the same tamper-evidence property the previous two rounds were defending, attacked from a third direction.

Of the two remedies offered I took the narrow one and not the broad one. Authenticating the record before it may authorise a delete targets exactly the described defect and reuses machinery this branch already wrote — the self-HMAC the verify walk recomputes, and the constant-time comparison already used for the eviction marker and, since last round, for sealed records on the read path. Reverting sealed-segment age pruning would delete the retention half of the feature this branch exists to add, so it was not taken.

Getting the record to the check needed one structural change, and I made it a projection rather than a second reader. The backward tail scan returned only the stamp, so the caller had nothing to authenticate; it now returns the record, and the stamp-only accessor became a thin wrapper over it. The size-cap eviction path keeps that wrapper deliberately: it reads the stamp purely to warn that the cap is outrunning the retention window and evicts either way, because the size bound must still hold. A caller that only orders by the stamp does not need to authenticate it; a caller that deletes because of it does.

Failing the check keeps the segment, and that cost is real rather than free: a corrupt-but-genuine oldest segment now holds retention open indefinitely instead of aging out. It is the right side to err on, because the opposite error deletes audit history and then reports success — and it is the same trade this function already accepted for an unparseable stamp. The refusal logs at ERROR, because it means either tampering or a key that no longer matches the records it signed, and both need a human.

One consequence worth stating plainly, because it changed more lines than the fix did. Several existing fixtures planted segment records with a fabricated hash literal, which was harmless while nothing authenticated a segment record and is now — correctly — indistinguishable from a forged one. Rather than weaken what those tests assert, they now build records through the product's own digest, via a helper that self-checks against the authentication predicate so it cannot drift from the thing it exists to satisfy. One of them was the lease control added two rounds ago, which would otherwise have kept passing for a reason that had nothing to do with the lease.

Three cases pinned, in both directions. Removing the authentication makes the forged-stamp test fail with the segment deleted, while the legitimate-retention control still passes. Treating every segment as unauthenticated instead makes that control fail with retention broken. The third test is the tightest: it keeps a segment and then prunes it while the timestamp is byte identical in both phases, so only the MAC can be the discriminator — and it fails at the first phase under one revert and at the second under the other.

The function-local metrics import raised alongside this item is unchanged and remains rnoack's decision on the measurement recorded above.

Review round 38: an empty segment was erased before anything could report it

One blocking item, and it is the residue of last round's rather than a restatement of it. Three call sites unlink a sealed segment; last round authenticated the timestamp that authorises one of them. This is a different one: the budget sweep deleted any zero-byte sealed segment outright.

The sweep exists for a real reason, which the fix keeps. An uncounted zero-byte segment inflates the eviction budget, so every roll evicts one additional segment holding real history — measured previously at 439 bytes lost per roll. What was wrong was achieving the exclusion by deleting the file, because a zero-byte segment has two possible provenances and nothing on disk tells them apart: a crash-left number claim that never held history, and a real segment truncated after sealing. There is no sealed manifest, and an empty file has no content to authenticate, so the self-HMAC that guards age pruning is unavailable at this site. I looked for a discriminator and there is none.

The in-file argument for deleting anyway was that a truncated real segment stays loud either way, because its successor's prev_hash still names the tip that was truncated away. I measured it and it is false at the position this sweep deletes from. Baseline relaxation covers the oldest surviving entry, so it catches a mid-chain seam but not the head of the run — and eviction removes a prefix, which is precisely the oldest. On a forty-entry log with an authenticated eviction marker present, truncating the oldest sealed segment and keeping the file reported thirty-nine of forty and correctly refused to read clean; unlinking it reported thirty-nine of thirty-nine, clean, over a record that was gone. The marker made the loss re-baselineable, so the erasure authenticated itself. The docstring carrying that argument is corrected in the same commit.

So the number is dropped from the budget and the file stays. The reporting half needed no new code: the verify walk already treats a sealed segment holding no record as unverifiable, which is the detection added three rounds ago for truncation, and it fires here for the same reason.

Keeping the file has a cost that had to be decided rather than left implicit, because the two directions err oppositely and both are silent. Age pruning stops at the first segment it cannot prove aged, and a zero-byte segment can never be proved aged, so at the oldest position a retained one would defer retention permanently — measured: zero of two genuinely aged and authentic segments removed. That trades a silent data-loss path for a silent denial of the feature this branch adds. An empty segment is chain-transparent, though: it holds no entries, so nothing chains through it and the next segment's first prev_hash already names the tip of the newest segment that holds records. Retention therefore steps past it rather than stopping, which opens no mid-chain seam, and it does not delete it — deleting it there would simply restore the erasure at a different call site.

That leaves removal as a human decision, which is the honest answer when the code cannot distinguish a placeholder from a truncated segment. The residue accumulates at one file per crash in a narrow window, costs no disk space, and is reported by verification on every run until an operator resolves it. Loud and permanent is the right side of that trade against silent and erased.

Two existing tests asserted the old behaviour and were rewritten rather than weakened, because their intent survives and only their expectation changed. One asserted the residue was gone; it now asserts the residue is kept, and still asserts the budget property it was written for. The other asserted the log reads clean after the sweep, which was true only because the sweep had deleted the evidence — that is the behaviour under review, so it now asserts the chain is intact and the residue contributes exactly one unverifiable segment on top.

Four cases pinned, in three directions. Restoring the unlink fails the survival test and reproduces the clean read over a truncated segment. Putting empty segments back into the budget fails the budget control with real history evicted. Making retention stop at a retained empty segment instead of stepping past it fails the retention control with nothing pruned. Each control fails only its own tests.

The function-local metrics import raised alongside this item is unchanged and remains rnoack's decision on the measurement recorded above.

Review round 39: the zero-backup discard seam was only half-covered

One blocking item, against this PR's own sel.py, and it is a residue of the round-38 fix rather than a restatement of it. Reproduced by execution before anything changed, which is what settled it: the surrounding comment already anticipates a concurrent append across the discard, so reading the code would have argued the wrong way.

The recorded-tip mechanism authenticates the destroyed tip and adopts it as a baseline, but only for the FIRST entry of the walk. That covers a rival record that is the file's only record, which is the shape every existing test had. It does not cover ordinary operation, because the process that rolled the log re-anchors ITSELF to genesis and keeps logging, so a second legitimate anchor appears in the same file. Measured on the pre-fix head with the owner appending on each side of the rival: owner-then-rival gave total=2 valid=1 with SEL chain break at entry 2 and NO attribution message at all, since the rival is not entry 1; rival-then-owner attributed entry 1 and then broke at entry 2 on the owner's own genesis anchor. On an audit surface that reads identically to tampering.

The lane's remedy was to disable rotation at backup_count <= 0. Not adopted, and the reason is a measurement rather than a preference: the discard path is how the active file is bounded in that configuration, so disabling it removes the size bound this PR exists to add and lets the audit file grow without limit. test_rotation_is_still_enabled_at_backup_count_zero pins that direction and is unchanged. The fix closes the window instead: one discard creates one seam, so the two anchor values it can produce ("" and the recorded tip) are each adoptable once per walk, at any position. Nothing about what gets deleted changed, and the sticky marker is still not written.

Four directions pinned, each control confirmed to fail for its intended reason by deliberate breakage. Reverting the widening fails both new seam tests. Dropping the authenticated-record gate makes a spliced genesis-anchored record read clean after a real eviction -- that control had to be rebuilt, because on a genesis-anchored baseline the spent-once guard refuses it first and the gate is never consulted, so the first version of it passed with the gate removed and measured nothing. Dropping the spent-once guard lets a third anchor through. Widening the anchor set to any value masks a head truncation on a log that had discarded. Affected suites: 1,257 passed, 2 skipped.

[iamwhatever]

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

[iamwhatever]

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

[github-actions]

Design Review (Fable 5, fork) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound, exhaustively-verified rotation design; the residual risks are sequencing against the single-writer daemon and new public knobs the author left as open owner decisions.

Watch

• The cross-process machinery (seal lease, snapshot-drift fstat, handle pinning, stranded-append re-append) is built atop an append path the PR itself measures as broken multi-writer (total=81 valid=8) and names a single-writer daemon as the real fix — if that daemon lands, parts of this become redundant compensations. The description explicitly defers the sequencing call to the owner ("the call is rnoack's"); that confirmation should happen before merge, since sel.py roughly triples in size and the 35 review rounds show how large the concurrency state space this shape carries is.
• KIROCREW_SEL_MAX_BYTES/BACKUP_COUNT/RETENTION_DAYS become a public operator contract governing deletion on an audit surface before the promised config section exists ("Confirmation requested rather than assumed") — once shipped, the follow-up must keep them working as overrides forever.
• Rotation defaults ON: existing hosts move from unbounded retention to ~600 MB, and size eviction can delete evidence inside the 365-day window (loud, but real). This is a changed default on an audit surface and needs a "Before you upgrade" callout at the release that ships it.

[DESIGN-REVIEWED] c83a251

[github-actions]

First Principles Review (Fable 5, fork) — ⏭️ skipped

Revision c83a251fc5a41efc678cc3be31885a17f7cbb6d1 ships no reviewable capability, so there is nothing to inventory. Advisory — does not block merge.

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

No findings.
[GPT-REVIEWED] c83a251

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

I've now thoroughly analyzed the candidate against the code. Let me assess the falsification.

The candidate (CANDIDATE 1, self-rated "low" confidence) claims prune's Stage 1 count=True full-segment read under _lock can stall the event loop, citing no-blocking-call-on-event-loop (a blocking: true rule).

Falsification:

1. The blocking read is on a worker thread, not the loop. prune() is invoked via loop.run_in_executor(maintenance_executor(), sel().prune) (heartbeat.py:201). The _entry_count_of full-segment read therefore runs on a maintenance_executor worker. The cited rule explicitly lists as Allowed (do NOT flag): "the call is already inside run_in_executor(...) or handed to a thread." So the rule the candidate invokes does not apply to prune's read — its actual blocking syscall is on a thread the rule permits.

2. (a) is unconfirmed and speculative. The real mechanism the candidate needs is lock contention: a critical=True audit issued directly on the event-loop thread (un-offloaded) blocking on _lock while the worker holds it. The discovery pass itself states it "could not confirm from the diff that a critical=True audit is ever issued directly on the loop thread concurrently with a prune." That is the "if a caller were to" case Step 1 requires me to drop. I could not re-derive a concrete, in-practice loop-thread critical caller racing a heartbeat prune from the code I opened.

3. It is a magnification of pre-existing behavior, and the consequence class is wrong. The candidate concedes the extended hold "is otherwise a magnification of the pre-existing active-file-rewrite lock hold" — the old prune() already held _lock across a full read of the (unbounded) active log. And even granting the race, the outcome is added latency/contention, not a crash, data loss, corruption, or a security hole with a named trigger — so it would not meet the BLOCKING class in any event.

The candidate does not answer (a) cleanly. It dies under falsification. I found nothing else in the sel.py changes (or the smaller security.py / latex.py hunks) that grounds to the 80+ bar with a concrete input, call path, and observable wrong outcome.

No findings.

[OPUS-REVIEWED] c83a251

[iamwhatever]

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

[iamwhatever]

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

[iamwhatever]

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

[iamwhatever]

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

[iamwhatever]

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

[iamwhatever]

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

[bolichen97]

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

[iamwhatever]

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

[rnoack1]

Closing this PR as superseded. The feature it implements — bounding security_events.jsonl by size, rotating into numbered segments, and extending the age bound to whole segments — landed on main independently while this was open, and the two implementations cannot be reconciled by a rebase.

Why no rebase resolves it. The collision is not textual. The two designs disagree about the HMAC chain across the rotation seam:

• main makes each segment an independent chain from genesis, and its rotation record deliberately carries only previous_segment / previous_bytes — the predecessor entry_hash claim and the cross-segment assertion were removed on purpose.
• This PR deliberately does not re-anchor on roll, so verify_integrity() walks every segment as one continuous stream, with the oldest surviving entry's prev_hash as a relaxed baseline.

The two authenticated markers here (sel/evicted and sel/discarded-tip, MAC'd and constant-time compared) exist specifically to make that relaxation safe against someone with log-dir write access head-truncating a never-evicted log and reading clean. Under main's per-segment chains there is no genesis-relaxation to authenticate, so the markers — the most careful work in this branch — have nothing left to protect.

The surface symptom of the same fork: _segment_dir is an attribute on main (sel.py:279, used as a Path) and a method on this head (sel.py:1901, called). Any merge taking both sides fails at runtime.

What was salvaged. I audited this branch's hardening against live main rather than assuming it was all additive, and most of it was not:

• Already present on main — the per-line cap and chunked backward reader (_TAIL_CHUNK_BYTES, _MAX_LINE_BYTES).
• Inapplicable — the marker read cap (no markers on main) and the truncate-not-unlink discard (no backup_count=0 path on main).
• Not a defect — the O_CREAT|O_EXCL segment-number claim; main's max+1 runs under its rotation advisory lock.
• Genuinely absent and worth having, now filed as standalone issues against main's architecture: issue 4992 (segment reads validate the dirent then re-open by path with no O_NOFOLLOW, so a link planted in that window is followed) and issue 4993 (the size cap and keep-count are source-only constants with no runtime override).

One item I could not settle and deliberately did not file: whether the seal-then-prune stranding race this branch guards with _fd_is_unlinked / _reappend_stranded is reachable on main. main has no st_nlink check, but it rotates with os.replace (the inode survives under a new name) and does a post-write fstat on every append, so I could not establish the race exists there. Worth a look by someone with more context on that path than I have.

No action needed here. The branch stays in this PR's history if any of the above needs referring back to.