Serialize the audit state owner's writes so concurrent invocations cannot corrupt the record

[prflow-implementer]

Summary

• Serialize the create-issue audit state owner's (scripts/issue-audit-state.py) mutating writes with an exclusive-create .lock sentinel critical section, so two concurrent invocations for the same slug produce a state document reflecting one of them entirely and then the other — never an interleaved mixture.
• Give each writer a unique tempfile.mkstemp temporary path (with a bounded os.replace/PermissionError retry) so two writers never share and truncate one deterministic temp file, and hoist every stdin read into main() above the section so no handler blocks on stdin inside it.
• Prove the read-only classification fails closed with a transitive call-graph check that save_state is unreachable from every read-only subcommand. Standard-library only; no new state-document field; no new mutation-exit class.

Changes

Audit state owner (scripts/issue-audit-state.py): added the _StateSection exclusive-create sentinel critical section wrapping the single main() dispatch site for every non-read-only subcommand — acquire with os.O_CREAT | os.O_EXCL, ownership-checked release keyed on st_dev/st_ino, age-based stale-break, and every failure routed through the existing could not persist state to … StateError (no new exit class). Rewrote save_state to use a per-writer tempfile.mkstemp temp path (0600 on POSIX, .json.tmp suffix retained) with a bounded os.replace PermissionError retry. Hoisted all eight stdin reads into main() via _read_stdin_once / _selects_stdin / _stdin_bytes_or_fail, so no handler performs a sys.stdin read.
Contract test (lib/test/check-audit-lifecycle-contracts.py): added check_readonly_complement, a fail-closed transitive call-graph walk proving save_state is unreachable from every read-only-classified subcommand (query-*, emit-body, check-claim-staleness).
Tests (lib/test/test_python_scripts.py, lib/test/modules/issue-audit-state.sh): added deterministic (never schedule-dependent) coverage — the section acquire/stale-break/release, mkstemp/replace-retry, malformed-pid, contention-refusal, contend-then-acquire-after-release, reader-while-held, the stdin hoist, and the call-graph check's clean/direct/nested/getattr/table fixtures.
Docs: docs/DEVFLOW_SYSTEM_OVERVIEW.md §11 now states the serialization contract — document-integrity-only, with the decision-channel non-authoritative-under-concurrency residual named — and skills/create-issue/references/step-3-6-audit.md carries the batching-caller operational rule pointing at §11 as canonical.
Changeset: .changeset/issue-1040-serialize-audit-state-writes.md (bump: patch).

Resolves

Resolves #1040

View run

Test Plan

• lib/test/run-module.sh issue-audit-state exits 0 (238 assertions, including the new #1040 CLI stale-break / contention-refusal / reader-while-held rows).
• The #1040 Python rows in lib/test/test_python_scripts.py pass (full file: 3334 assertions, 0 failed), including the deterministic contention, stale-break, mkstemp, replace-retry, and call-graph-fixture rows.
• lib/test/run.sh reports zero failed on a committed tree.

Visual Changes

N/A

Breaking Changes

None. The change is standard-library only, adds no state-document field (SCHEMA_VERSION stays the integer 3), and produces no new mutation-exit class. The one decided behavior change is that the persisted state file's POSIX permission mode becomes 0600 (a per-user run artifact under an ignored tmp directory) — asserted rather than incidental.

[The01Geek]

Requesting review. Implements #1040: serializes scripts/issue-audit-state.py's mutating writes behind an exclusive-create .lock critical section, and gives each writer a unique tempfile.mkstemp path instead of the single deterministic one every writer of a slug shared.

Context for severity: the failure this prevents is total, not partial. _validate rejects a corrupt document and there is no schema_version migration path, so a torn write collapses the whole audit record — every round, verdict, finding ledger, claim baseline and override — to unestablished at once.

Two things I specifically want pressure-tested:

• Deadlock is the new failure mode this introduces. A stale .lock left behind by a crashed or killed writer must not wedge every future invocation for that slug forever. A lock that never releases is worse than the race it prevents, because the race is rare and the wedge is permanent. Please check the stale-lock path exists, is bounded, and fails in the safe direction — and that its recovery cannot itself race.
• Concurrency claims need driven tests, not the presence of a lock call. Assertions that merely confirm the lock is acquired do not establish serialization. Look for a test that actually runs concurrent writers and asserts the document reflects one entirely and then the other, never an interleaving.

Also worth confirming: issue-audit-state is an exact-pinned module, so its tally must match at both coupled sites; and any value deciding a selection must not be derived through a non-preflight PATH tool.

/prflow:review

[prflow-reviewer]

Devflow Review — PR #1045 · REJECT (changes requested)

Reviewed HEAD: a32814d927f998e8a3f4c04a4d601cc03b61bb7c
Diff classification: engine_self_modifying + detect_all_audit → full checklist + forced completeness-critic pass.
Test evidence (CI ground truth for this commit): lib + python tests: success; lint (shellcheck + actionlint + ruff): success; all shard (*) jobs success. Suite is green.

This is a careful, thorough change — the O_CREAT|O_EXCL sentinel critical section, the per-writer mkstemp temp path with a bounded os.replace retry, the stale-sentinel age-break recovery, the new fail-closed AST call-graph check, the docs/changeset, and the test suite are all high quality. The concurrency mechanism itself is race-safe for the scenarios exercised, and the run.sh 230→238 bump matches the registry and the eight new shell assertions. It is rejected on one confirmed silent-failure regression and one related diagnostic-accuracy issue, both in the stdin-hoist refactor.

🔴 Must fix — silent swallow of a stdin read error in cmd_check_claim_staleness

scripts/issue-audit-state.py, cmd_check_claim_staleness:

domain = args._stdin_data

replaces the prior domain = sys.stdin.buffer.read() if args.domain_stdin else None. _read_stdin_once records a mid-read OSError into args._stdin_error (leaving args._stdin_data = None) and a closed fd 0 into args._stdin_missing. This site checks neither. So when --domain-stdin is selected and the hoisted read genuinely fails, the failure is silently absorbed into domain = None and staleness is computed and printed as though no domain search were piped — indistinguishable from the intentional "no --domain-stdin" case.

• Regression: before this PR a mid-read OSError propagated loudly; now it is swallowed. On an audit re-verification path, that is exactly the "unknown is not zero" silent-failure class this repo's conventions forbid.
• Inconsistency: every sibling stdin consumer this same PR hardened routes through _stdin_bytes_or_fail, which checks both flags and emits a named breadcrumb. Only this handler (and the two below) hand-roll a partial check — and this one checks nothing.
• No comment or test documents this as intentional (unlike the two sites below, which carry deliberate-behavior comments).
• Fix: mirror cmd_record_claim_baseline — if args._stdin_error is not None: _fail(prefix, f'could not read the domain search result from stdin: {args._stdin_error}') before using args._stdin_data. Add a test exercising the _stdin_error branch (currently untested anywhere in the mechanism).

🟠 Should fix — closed-fd 0 misdiagnosed as "empty domain search" in cmd_record_claim_baseline

scripts/issue-audit-state.py, cmd_record_claim_baseline checks args._stdin_error but not args._stdin_missing. A closed fd 0 leaves _stdin_missing=True, _stdin_error=None, _stdin_data=None, so it falls through to _fail(prefix, 'domain-class-empty-domain: --domain-stdin produced no bytes ...'). The PR comment calls this a "deliberate, minor improvement," but the emitted diagnostic actively asserts "a search that emitted nothing" when in fact stdin was never attached — a false, misleading breadcrumb of exactly the "never-attempted misattribution" kind record-creation-attestation's own comment says the tool exists to prevent. Route through _stdin_bytes_or_fail (or add the _stdin_missing arm) so the closed-fd case names its real cause. cmd_record_finding_evidence has the same missing _stdin_missing check (there it degrades to a raw AttributeError on decode — pre-existing and comment-documented, but the hoist is the natural point to close it since _stdin_missing is now computed and sitting unused on args).

Coverage gaps worth closing in the same change (not independently blocking)

• The _stdin_error (mid-read OSError) branch of _read_stdin_once / _stdin_bytes_or_fail and the two new hand-written _stdin_error checks in cmd_record_claim_baseline / cmd_record_finding_evidence are exercised by no test — only the closed-fd branch is.
• _selects_stdin's False/absent-flag branch is untested for record-revision, record-coverage, check-claim-staleness, record-finding-evidence (an inverted-condition typo would pass the suite).
• _try_create's post-os.open failure path (fstat/write/close OSError after a successful exclusive create → unlink-then-raise) is untested.

Verified sound (no findings)

_StateSection acquire/release ownership token (st_dev, st_ino), the re-stat-before-unlink staleness recheck, __exit__'s ownership-checked release (never suppresses an in-flight exception), _replace_with_retry (non-PermissionError propagates first attempt; exhaustion re-raises loudly into the could not persist state to … class), _selects_stdin's mirror of every handler's read trigger (exact; no handler retains an in-section sys.stdin read), and check_readonly_complement's fail-closed AST walk (four adversarial positive controls — direct/nested/getattr/table reach → Refusal — plus a clean control).

---

Engine: engine_self_modifying + detect_all_audit. Findings corroborated across code-reviewer, silent-failure-hunter, and pr-test-analyzer.

[The01Geek]

Re-requesting review. The previous verdict was REJECT, so this is a fresh round rather than a merge on the prior approval.

Both findings are closed at 55a5d00c:

• 🔴 cmd_check_claim_staleness silent swallow — domain = args._stdin_data now routes through _stdin_bytes_or_fail, so a mid-read OSError and a closed fd 0 each name their own cause instead of collapsing into domain = None.
• 🟠 false "empty domain search" diagnosis — cmd_record_claim_baseline and cmd_record_finding_evidence route through the same guard, which removed the two hand-written partial _stdin_error checks. The mechanism now has one guard rather than three partial ones; the inconsistency was the defect class, not just its instances.

What I would like this round to check specifically:

• The consolidation did not change any breadcrumb or contract. Every emitted message is claimed byte-identical to what the guarded sites already produced, and both empty-read contracts are claimed intact (evidence records an empty read as incomplete per create-issue: record and reuse evidence provenance — baseline-grounded claims and reproducible audit findings #704; baseline refuses one). Please verify rather than take that.
• Scope: an AST comparison with docstrings stripped reports exactly cmd_check_claim_staleness, cmd_record_claim_baseline, cmd_record_finding_evidence changed and nothing else — the locking mechanism, ownership token, _replace_with_retry, _read_stdin_once, _selects_stdin and the AST call-graph check byte-identical. Those were all verified sound last round and should not have moved.
• The mutation evidence has two deliberate non-reds: baseline_closed_fd: exits non-zero and evidence_closed_fd: exits non-zero were already non-zero before the fix — via the wrong breadcrumb and via a traceback. The exit code was never the defect; the diagnosis was. Confirm the companion assertions pinning which breadcrumb appears (and that the wrong one does not) are the ones doing the work.

Also from last round, unchanged and still in scope: the STATED BOUND docstring recording that a holder exceeding stale_after_s can have its sentinel age-broken, and the §11 correction naming stale_after_s rather than the acquire window as the staleness threshold.

/prflow:review

[prflow-reviewer]

Devflow Review — PR #1045

Status: 🎉 Complete
Diff profile: engine_self_modifying (+ detect_all_audit) → full checklist + completeness-critic pass
Run: View run
Reviewed HEAD: 55a5d00
Last updated: Phase 4 (final)

Blueprint

• Classify diff (Phase 0.5)
• Generate verification checklist (Phase 1)
• Verify checklist (Phase 2)
• Review agents (Phase 3)
• Aggregate & verdict (Phase 4)

Verdict: APPROVE with notes

A correct, defensively-engineered serialization of the audit state owner's writes (issue #1040). The mechanism (exclusive-create .lock sentinel wrapping the single main() dispatch site, stdin hoisted above the section, per-writer tempfile.mkstemp in save_state) matches the acceptance criteria across every item checked, and the change additionally repairs three latent silent-failure/false-diagnosis bugs in the stdin path — each backed by a RED-first regression test. CI is green for the reviewed commit (lint, all five shards, and lib + python tests all success). No Critical findings and no self-contradicting-diff carve-out; under the default critical verdict threshold this is an APPROVE. The notes below are worth a follow-up but do not gate merge.

Issue Compliance

Reviewed against issue #1040: Serialize the audit state owner's writes so concurrent invocations cannot corrupt the record — criteria from the /prflow:implement workpad comment (this run's authoritative set); scope unchanged. No divergence between the workpad's criteria and the issue body's. Spot-checked invariants all hold: SCHEMA_VERSION = 3 (no new field), stdlib-only imports (no fcntl/msvcrt), the exactly these 7 paths / exactly these 9 files out-of-bounds enumerations are byte-identical (untouched by the diff), and the read-only predicate + fail-closed transitive-call-graph completeness check are present.

Verification Checklist Results

Engine-self-modifying profile → full checklist. Key mechanical claims verified directly: args.cmd is guaranteed (add_subparsers(dest='cmd', required=True)); every one of the 38 mutating subcommands carries slug, so _StateSection(args.slug) resolves; the read-only predicate (query-* + emit-body + check-claim-staleness) skips the section; save_state routes every OSError through the existing could not persist state to … StateError. No FAIL, no INCONCLUSIVE. Completeness-critic pass (detect_all_audit): the new check_readonly_complement audit enumerates its population from registered_subcommands() (the parser registry) rather than a grep pattern and fails closed on unresolvable call targets; independent re-enumeration from the parser add_parser names is a subset — audit complete with respect to that independent signal.

Code Review Findings

🟠 Important / Major

1. (st_dev, st_ino) ownership token is defeatable by inode reuse, so __exit__ can strip a live sentinel (raised by 2/6 agents — silent-failure-hunter, type-design-analyzer). After a contending writer age-breaks this section's sentinel (unlink of inode N) and O_EXCL-creates its own, an inode-reusing filesystem (ext4, …) may hand the breaker the same (st_dev, st_ino) this section recorded; this section's __exit__ then matches its stale token and unlinks the breaker's live sentinel, re-opening the interleaved-write window. Compound preconditions — occupancy exceeding stale_after_s (reachable via a SIGSTOP'd/paused/debugged/swap-thrashing holder or an inverted override), an age-break, and inode reuse — make this narrow but real, and the class docstring's "cannot strip the breaker's exclusion" reads as a stronger guarantee than the token provides. The dedicated truthfulness agent (comment-analyzer) verified the docstring and judged it accurate, so this is not routed as a self-contradicting-diff falsehood — but a content nonce (secrets.token_hex() written at create, re-compared at release) would close both inode- and pid-reuse, and at minimum the docstring should state the inode-reuse residual. Recommend a hardening follow-up; does not gate merge at the critical threshold.
2. Headline document-integrity guarantee is proved only by decomposed mechanism, never by real concurrency (raised by 1/6 — pr-test-analyzer) [suspected over-grade: shape 3 — uncorroborated single-source from pr-test-analyzer; advisory only, verdict unchanged]. No test spawns two writers at once; contention is planted deterministically. Mechanism decomposition is strong, but a cheap N-subprocess stress smoke test (assert the final document is valid JSON reflecting a serial order) would guard the exact behavior the change exists to provide.

🟡 Suggestion / Minor

1. _stdin_bytes_or_fail docstring's phrase list omits 3 of 8 call sites (comment-analyzer) — the parenthetical enumerates 5 phrases but omits the domain search result (record-claim-baseline, check-claim-staleness) and the observed output (record-finding-evidence), the very sites this PR adds. Behavior-inert docstring prose (limb one: no tool parses it for behavior; limb two: internal code doc, not a shipped user-facing surface) → behavior-inert prose cap applies, capped at Suggestion. Make the list illustrative (e.g. …) or add the two phrases.
2. _try_create's except FileNotFoundError re-makedirs is unguarded (silent-failure-hunter; untested per pr-test-analyzer) — a mid-run OSError from the recreate escapes raw out of __enter__/main(), bypassing the section's single could not persist state to … vocabulary. Rare (the __enter__ makedirs makes it so) but the arm exists to defend exactly the mid-run case. Wrap it through _persist_error like the __enter__ guard.
3. main() reads args.slug unconditionally for every non-read-only command (silent-failure-hunter) — latent only; all 38 current mutating subcommands define slug (verified), but a getattr(args, 'slug', None) guard with an explicit _fail would keep the no-raw-traceback property total for future authors.
4. stale_after_s < acquire_window_s coupling is neither validated nor expressed at the type boundary (type-design-analyzer) — the two bounds are independent kwargs with independent env overrides; inversion silently flips behavior (fail-closed by design). A one-line __init__ note naming the coupling would keep the invariant with the type.
5. _break_if_stale emits "…and proceeded" before it knows the re-create succeeded (type-design-analyzer) — when the re-create loses to a racing writer the breadcrumb misleads a reader diagnosing contention. Emit the outcome ("and acquired" / "but lost the re-create") based on the _try_create result.
6. _positive_env_float accepts inf (requesting-code-review) — test-only knobs, within the letter of the spec (numeric+positive); a math.isfinite(val) guard closes it.
7. Minor test gaps (pr-test-analyzer) — __exit__'s non-FileNotFound stat branch, _break_if_stale's unlink-failure breadcrumb, the check_readonly_complement empty-read-only-set vacuity refusal, and _try_create's missing-parent recreate branch are undriven (several effectively unreachable in normal flow).

over-grade annotation: finding flagged (Important #2, shape 3, advisory).
truthfulness sweep: no finding promoted.
intra-diff contradiction scan: no contradiction found.

Strengths

• Uniform failure vocabulary: every section/save_state failure surfaces as could not persist state to … StateError → _fail, so no new mutation-exit class.
• save_state's move to tempfile.mkstemp preserves the .json.tmp suffix (create-issue: extract the Step 3.6/Step 4 audit lifecycle into a tested state-owner CLI (scripts/issue-audit-state.py) #546 glob), keeps mkstemp inside the try below the mkdir, and adds a bounded os.replace PermissionError retry; 0600 mode decided and asserted (POSIX; Windows skipped with a named reason).
• The stdin hoist repairs three genuine latent bugs (check-claim-staleness silent swallow, record-claim-baseline false empty-domain diagnosis, record-finding-evidence bare AttributeError), each with wrong-breadcrumb-absent + right-breadcrumb-present regression tests.
• Coupled-invariant hygiene: assertion count 230→238 reconciled in both run.sh and workflow-flight-recorder-registry.json; §11 canonical statement + the step-3-6 operational-rule pointer edited together; coverage-map entry added; DEVFLOW_ env prefix correctly frozen per PRFlow rename Tier 1: atomic directory/vendored-path/config-key/workflow-body migration + /prflow:init (from #992) #1002/PRFlow rename Tier 3: /prflow:init advisory report for out-of-repo DEVFLOW_* identifiers (from #992) #1004 and recorded in the changeset.

Review agents

6/6 returned (code-reviewer, silent-failure-hunter, comment-analyzer, type-design-analyzer, pr-test-analyzer, requesting-code-review). Final-pass extension load: loaded-empty (no consumer review-and-fix extension content).

[prflow-reviewer]

Verdict: APPROVE with notes — full report in PR comment

The complete review report (checklist results, findings, details) is in the
Devflow Review progress comment on this PR.

[The01Geek]

Re-requesting review at 4d6a489c. Round 2 raised a 🟠 Important that the owner's gate does not let through, so this is a fresh round rather than a merge on the prior APPROVE with notes.

🟠 1 — (st_dev, st_ino) defeatable by inode reuse — closed. _try_create now writes <pid> <owner-nonce> (128 bits, os.urandom) and __exit__ unlinks only when the sentinel's recorded owner still equals the nonce this section wrote. Content equality answers "is this still the file I created?"; identity equality only answered "does this file occupy the slot mine did?"

🟠 2 — guarantee proved only by decomposed mechanism — closed, and it discriminated. With serialization disabled, 1 of 6 concurrent claims survives, identically across 8 trials, while every writer still exits 0 with no traceback. The loss is silent, which is exactly why the decomposition argument could not see it. Fixed module: 6/6 across 12 trials.

What I would like this round to scrutinise:

• The inode-reuse test is deterministic by construction, not probabilistic — the breaker's body is written in place, so (st_dev, st_ino) is guaranteed equal to the acquire-time value while the recorded owner differs. Confirm the row first asserts the identity really did match, so it cannot pass vacuously.
• A pre-nonce or foreign sentinel must never be adopted. A bare-pid body yields owner=None, which no generated nonce equals; the owner is also shape-checked ([0-9a-f]{32}) so a truncated field reads unestablished rather than comparing as data. Please probe for any body shape that could be adopted.
• self._token is assigned only after write and close succeed, so a section that failed mid-acquire owns nothing and releases nothing. Worth confirming there is no window where it is set early.
• An existing test was corrected, not just added to. release_does_not_unlink_a_foreign_sentinel previously avoided unlink+recreate with a note that "a filesystem may immediately reuse" the inode — that side-step is what kept this defect unobserved for two rounds. It now forces a well-shaped foreign nonce; the old (-1, -1) compared unequal for the wrong reason and would have passed even with the owner comparison deleted. Please confirm the replacement is genuinely load-bearing.
• The STATED BOUND docstring and §11 now record that the identity form was unsound, rather than quietly substituting the new claim. The heartbeat-free residual is unchanged and explicitly stated: the nonce bounds what a broken holder can destroy, not whether it can be broken.

Scope: an AST comparison with docstrings stripped reports only _StateSection, _read_sentinel_pid, two new helpers and two new constants changed — _replace_with_retry, _read_stdin_once, _stdin_bytes_or_fail, _selects_stdin, the AST call-graph check and the three stdin handlers fixed in round 2 are byte-identical.

/prflow:review

[prflow-reviewer]

Verdict: APPROVE with notes (sound write-serialization change; a few narrow robustness/precision nits, none merge-blocking)

Issue Compliance

Reviewed against issue #1040: Serialize the audit state owner's writes so concurrent invocations cannot corrupt the record — criteria from the /prflow:implement workpad comment (this run's authoritative set); scope unchanged. No divergence between the workpad's criteria and the issue body's. Requirement-based checks are reflected in the verification below.

Criteria came from the /prflow:implement workpad comment — this run's authoritative set, possibly narrowed from the issue.

Verification Checklist Results

Engine profile: engine_self_modifying (+ has_new_types + detect_all_audit) → full checklist; Phase 3.1.5 completeness-critic forced.

Test evidence (from the run's engine-ground-truth block, read from the GitHub API for the reviewed HEAD 4d6a489): all checks success — lint (shellcheck + actionlint + ruff), shard (modules-rest), shard (modules-pin), shard (monolith), shard (python-pool), shard (modules-large), and lib + python tests.

Key acceptance criteria verified against HEAD by the review agents:

• Critical section wired at the single main() dispatch site for every non-read-only subcommand; dest='cmd' and a positional slug exist on every subcommand, so _StateSection(args.slug) / _is_read_only(args.cmd) never AttributeError on a mutating path. ✅
• Ownership-checked release uses the owner nonce (content equality), not (st_dev, st_ino); _parse_sentinel_body shape-checks the 32-hex owner, so bare-pid/foreign sentinels read owner=None and are never adopted. ✅
• save_state uses a per-writer tempfile.mkstemp temp path (0600, .json.tmp suffix retained for the #546 cleanup glob) with a PermissionError-only bounded os.replace retry; partial temp is cleaned on failure and routed through the existing could not persist state to … StateError. ✅
• Stdin hoisted into main() above the section; _selects_stdin's read trigger is a superset of every handler's own read trigger (safe direction — _stdin_bytes_or_fail never hands a handler a laundered None), including the previously-silent check-claim-staleness fix. ✅
• check_readonly_complement transitive call-graph check present with all four positive controls (direct / nested-helper / getattr / table dispatch) plus a clean control; fails closed on empty population and unresolvable call sites. ✅
• SCHEMA_VERSION still integer 3, no new state-document field, no new mutation-exit class; stdlib-only (no fcntl/msvcrt). ✅
• The exactly these 7 paths / exactly these 9 files out-of-bounds enumerations stayed byte-identical (diff touches none). ✅
• DEVFLOW_IAS_* env overrides use the frozen DEVFLOW_ namespace per CLAUDE.md's #1002/#1004 rule and are disclosed in the changeset. ✅
• docs/DEVFLOW_SYSTEM_OVERVIEW.md §11 states the serialization contract (document-integrity-only, read-only unserialized, age-recovery, the heartbeat-free residual named); skills/create-issue/references/step-3-6-audit.md carries the batching-caller operational rule pointing at §11. ✅
• Engine-surface change ships a .changeset/*.md (bump: patch). ✅

Completeness critic (Phase 3.1.5, detect_all_audit): independently re-enumerating the population — every non-read-only subcommand is wrapped structurally in main() (no enumeration needed there), and the audit's read-only population equals the actual _is_read_only set — shows the check_readonly_complement matched set is a superset of the real read-only surface. No completeness gap.

Code Review Findings

🟡 Suggestion / Minor

1. _StateSection._try_create's FileNotFoundError recovery arm calls os.makedirs(self._parent, exist_ok=True) unwrapped. __enter__ calls _try_create() in its loop with no surrounding try, and main()'s section wrapper catches only StateError, so a non-exist_ok-suppressed OSError there (a file planted at an ancestor path → NotADirectoryError, a permission-denied parent, ENOSPC) escapes as a raw traceback — bypassing the stated "every section failure routes as a could not persist state to … StateError / no new mutation-exit class" contract. Reachability is narrow (parent removed after __enter__'s makedirs, then obstructed) and it fails loud (nonzero exit, state left byte-identical) rather than silently corrupting, hence Minor. One-line fix: wrap it in the same try/except OSError → raise self._persist_error(...) __enter__'s makedirs already uses (or have main()'s wrapper route a stray OSError through _fail). (raised by 1/4 agents)
2. The stale-break / release is a non-atomic check-then-unlink-by-path. The in-code docstring's "a holder whose sentinel was age-broken … cannot strip the breaker's exclusion" is slightly more absolute than the code delivers: the owner-nonce check bounds (does not eliminate) a narrow TOCTOU window in the already-accepted sub-stale_after_s break-race. This is behavior-inert docstring precision — and §11 already states it more accurately ("the nonce bounds what a broken holder can destroy, not whether it can be broken"). Consider softening the in-code wording to match §11. (raised by 2/4 agents)
3. _selects_stdin returns bool(args.ledger_stdin) for record-adjudication, but the handler only reads stdin when ledger_stdin AND ledger_shape; passing --ledger-stdin on a FILE/unestablished verdict makes the hoist read-and-discard (harmless over-read in the normal piped shape, but blocks against an interactive fd 0, and the docstring's "mirrors each trigger exactly" is a superset here). Tighten the selector for this command or soften the wording. (raised by 1/4 agents)
4. stale_after_s < acquire_window_s is described as an invariant but is not validated in __init__; an inverted env override degrades silently to "refuse under any contention" (and, with window < stale, a crashed holder wedges the slug). Handled fail-closed for document integrity, but a one-line __init__ breadcrumb when the relation inverts would make the misconfiguration diagnosable at its source. (raised by 1/4 agents)
5. Coverage gaps on secondary error-routing branches (headline guarantees are covered deterministically): _replace_with_retry never drives a non-PermissionError OSError to confirm it is not retried; _break_if_stale's unlink-failure breadcrumb, __enter__'s makedirs-OSError routing, and __exit__'s non-FileNotFoundError read-failure arm are each independently deletable without turning the suite red. Worth closing if the error-routing vocabulary is meant to be regression-proof. (raised by 1/4 agents)

ℹ️ Informational

• The persisted state file's POSIX mode becoming 0600 is an intended, tested (state_file_mode_is_0600, Windows-skipped with a stated reason), and disclosed decided change (changeset + PR body). Not a finding.

over-grade annotation: no finding flagged
truthfulness sweep: no finding promoted
intra-diff contradiction scan: no contradiction found

Verdict Criteria

No verification checklist FAIL or INCONCLUSIVE (all CI shards green for the reviewed HEAD; agent verification confirmed the load-bearing ACs). No review-agent finding at or above the critical verdict threshold — every finding is Minor/Suggestion, fails loud/closed, and none is a self-contradicting-diff carve-out (the two prose-precision nits are behavior-inert internal docstrings, capped at Suggestion). → APPROVE with notes.

This is a careful, unusually well-tested concurrency change: the tests assert final-state invariants with synthetically-created contention (planted sentinels, forced tokens, in-place body rewrites, threading.Timer releases) rather than racing real writers, so they are deterministic by construction. The findings above are robustness/precision refinements, not blockers.

[The01Geek]

Round-3 verdict on 4d6a489c (current head): APPROVE with notes — 🟡 Suggestion only, "none merge-blocking". Zero Critical/Important. Merging.

On the two verdicts that appear to disagree, since the record is confusing and someone reading later deserves the resolution: the progress comment showing a 🟠 Important section is keyed Reviewed HEAD: 55a5d00c — that is round 2, the inode-reuse finding, which this PR fixed. Round 3 wrote its full report into the review body instead and the progress comment was never updated for it. The immutable per-round PR review record is unambiguous:

round	submitted	verdict	commit
1	04:20:43Z	CHANGES_REQUESTED	8e4b33df
2	05:49:46Z	APPROVE with notes (🟠)	55a5d00c
3	07:13:15Z	APPROVE with notes (🟡)	4d6a489c

This is a live instance of #1030 — verdict identity resting on agent-authored prose and an edited-in-place comment, so "which findings describe the current head" is not answerable from the obvious surface.

Three rounds of real findings, all closed: the stdin silent-swallow regression, the false empty-domain diagnosis, and the (st_dev, st_ino) ownership token defeatable by inode reuse. The last one also invalidated a guarantee we had documented, and the docstring now records that the identity form was unsound rather than quietly substituting the new claim.