Skip to content

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

Description

@The01Geek

Problem Statement

scripts/issue-audit-state.py owns the entire create-issue Step 3.6 audit lifecycle, and every one of its mutations is a read-modify-write of a single JSON state document. It has no locking, no compare-and-swap, and a single deterministic temporary-file path shared by every writer of a given slug. It is safe today only because exactly one process writes at a time — a property nothing in the code enforces and nothing detects when it is violated.

The person who pays is the maintainer running an audit. A corrupt write is not a degraded field: _validate rejects the document, a schema_version mismatch has no migration path, and the whole run's audit record — every round, verdict, finding ledger, claim baseline, and override — collapses to unestablished at once.

This is latent-but-real today, because the state file is anchored per repository/worktree root rather than per process: two invocations for the same slug inside one checkout share one file. It is also the blocking prerequisite for recording a round that had more than one auditor, because the natural way to record several returns is to issue the per-lens calls in one parallel tool-call batch, and each such call is a separate process.

Current Behavior

  • Every mutating subcommand runs load_state → mutate → save_state with nothing serializing the triple.
  • save_state validates the in-memory document it was handed, writes path.with_suffix('.json.tmp'), and calls os.replace(tmp, path). The temporary path is one fixed name derived from the state path. Two concurrent writers for the same slug write the identical temporary file; the second truncates and rewrites it while the first may already have replaced it, so os.replace can install a partially-overwritten document.
  • Because the validation runs on the in-memory document immediately before the write and never on the bytes that land, corruption of the written bytes is not caught at write time.
  • Every subcommand outside _NEXT_CALL_EXCLUDED re-reads the state file from disk after its handler returns, to render its next_call= line. Every query-* subcommand reads with no serialization at all. Readers, not only writers, are collision partners.
  • A schema_version mismatch raises with the message ending (no migration path), so a corrupt write destroys the whole record rather than one field.

Desired Behavior

Two concurrent invocations of scripts/issue-audit-state.py against the same slug in the same checkout produce a state document that reflects one of them entirely and the other entirely, in some serial order, and not a mixture of the two — a guarantee scoped to the mechanism's supported form, which is every schedule except the stale-break residual the Implementation Notes state and bound. The second writer waits for the first and then applies its own mutation on top of the first writer's result.

A writer killed mid-mutation does not wedge later mutations for that slug beyond the stale bound: the next writer waits, then detects the abandoned section, recovers it with a named breadcrumb, and proceeds. Acquisition is bounded so that it terminates in a wait-then-acquire and otherwise in a wait-then-break, adding no contention-refusal exit. The one non-zero exit the section can produce is an environmental failure to create the sentinel at all, which falls inside the cannot-persist-state class the skill already routes rather than opening a new one.

The guarantee is about the state document. The tool's decision channel — the next_call= line and the query-* answers — stays unserialized, and its residual is stated rather than closed here.

Read-only subcommands stay unserialized and keep their existing exit-0 contract.

User Impact

The maintainer running a create-issue audit stops carrying an unbounded risk of losing the entire audit record to an accidental second invocation. The three follow-ups this unblocks — the lens partition, the multi-auditor round record, and the audit panel — become implementable, because each of them issues several state-owner calls from separate processes.

Technical Context

Scope note: The files and details below are the known starting points, not the full
list. Before implementing, trace the change through the codebase to find every affected
call site, consumer, and layer — this issue maps the work, it does not bound it.

Relevant Classes/Files

  • scripts/issue-audit-state.pyload_state, save_state, state_path, _repo_root, _validate, _query_state, _emit_next_call, main's dispatch wrapper, SCHEMA_VERSION, _new_doc, and the four closed sets _CALLER_SUPPLIED_FLAGS, _NEXT_CALL_EXCLUDED, _MULTILINE_READBACKS, _ROUND_DEFAULTED.
  • lib/test/test_python_scripts.py — the registered focused test for this file, and the home of the existing #546 save_state_cleanup_rows block.
  • lib/test/modules/issue-audit-state.sh — the shell-level CLI drivers.
  • lib/test/check-audit-lifecycle-contracts.py — the closed-set reconciliation.
  • docs/DEVFLOW_SYSTEM_OVERVIEW.md §11 — the canonical statement of where run state persists, and the canonical home of the decision-channel concurrency contract this change adds.
  • skills/create-issue/references/step-3-6-audit.md — the surface that actually issues the state-owner calls and reads next_call=, and therefore the only place the batching caller's operational re-query rule can reach its reader.
  • skills/create-issue/references/fallback-state-owner-unavailable.md — the shipped routing partition over the tool's non-zero mutation exits, which this change must leave byte-identical.

Verified premises

  • Verified: in scripts/issue-audit-state.py, executed grep -c fcntl scripts/issue-audit-state.py and grep -c flock scripts/issue-audit-state.py each print 0; the file's only persistence comment is "Best-effort cleanup of a partial temp file so a failed persist never leaves". No locking primitive exists in the file. claim=save-state-no-lock revision=b8b1594dd8b534a5972385b33fc9beede5a1d034 identity=1a1a2104496050685913a83c148ebaca6ff6df25
  • Verified: scripts/issue-audit-state.py, save_state contains "tmp = path.with_suffix('.json.tmp')" followed by "tmp.write_text(json.dumps(doc, indent=2, sort_keys=True)" and "os.replace(tmp, path)". One deterministic temporary name per slug, no mkstemp. claim=save-state-no-lock revision=b8b1594dd8b534a5972385b33fc9beede5a1d034 identity=1a1a2104496050685913a83c148ebaca6ff6df25
  • Verified: scripts/issue-audit-state.py, save_state docstring and comment — """Persist atomically. Raises StateError when the state cannot be persisted.""" above # Re-validate at the construction boundary: a mutation bug that assembled an / # invalid document fails HERE, loudly, instead of persisting silently and / # collapsing the whole file to unestablished at the next load. The validation runs on the in-memory argument, above the write.
  • Verified: scripts/issue-audit-state.py contains "SCHEMA_VERSION = 3", an integer, and the mismatch raise whose message ends "{SCHEMA_VERSION} (no migration path)')".
  • Verified: scripts/issue-audit-state.py, _new_doc carries the issue-create-issue: record and reuse evidence provenance — baseline-grounded claims and reproducible audit findings #704 comment "dedicated per-finding evidence channel. Both are additive under the UNCHANGED" and, for issue create-issue Step 3.6: make final-byte audit coverage a reported, non-substitutable lifecycle fact #792, "file written before this feature still loads and reports the axis as". The established convention is additive fields under an unchanged schema_version, read with a default everywhere.
  • Verified: scripts/issue-audit-state.py, state_path docstring — """`.prflow/tmp/issue-audit-state-<slug>.json`, anchored to the repo/worktree root.Deliberately NOT the main-worktree root the draft file uses: sharing one record across concurrent worktree runs would let a foreign cold-start wipe this run's state.""" The anchor is _repo_root() — a native git rev-parse --show-toplevel subprocess — with Path.cwd() as the degraded fallback. The isolation is between worktrees; two processes inside one worktree share the file whatever their working directory.
  • Verified: scripts/issue-audit-state.py, _emit_next_call contains "state = _query_state(args.slug)" under the comment "The POST-mutation state: re-read from disk after the command ran". Every subcommand outside _NEXT_CALL_EXCLUDED performs this unserialized read.
  • Verified: scripts/verification-flight.py contains "fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600)" under the comment "Single-owner guarantee: O_CREAT|O_EXCL means at most one concurrent caller". The exclusive-create pattern this issue adopts is already in this repository. Executed git grep -n 'os\.link' returns no hit, so the link-based swap has no precedent here and is not the pattern chosen.
  • Verified: tempfile.mkstemp is already the repository's per-writer temporary-path idiom: scripts/stage-draft-write.py contains "fd, tmp = tempfile.mkstemp(prefix=target.name + '.', suffix='.tmp', dir=str(directory))" and scripts/export-workflow-lifecycle-census.py contains "fd, tmp = tempfile.mkstemp(dir=str(parent), prefix=".
  • Verified: executed git grep -n 'json\.tmp' — 5 hits: scripts/issue-audit-state.py (2) and lib/test/test_python_scripts.py (3), of which one is executable — assert_eq("#546 save_state_cleanup_rows: ... and no partial .json.tmp survives " "the failed persist", [], list((_ss_root / '.prflow' / 'tmp').glob('*.json.tmp'))). That assertion reads the temporary path through its suffix, so it stays meaningful when the stem becomes unique and is silently neutered if the suffix changes. claim=json-tmp-inventory revision=b8b1594dd8b534a5972385b33fc9beede5a1d034 identity=cf69c04fe37a05f92234dae8abf917d3851bf451
  • Verified: executed git grep -l 'exactly these 7 paths' and git grep -l 'exactly these 9 files' — their union is four files: skills/create-issue/references/step-3-6-audit.md, which originates the contract, plus skills/create-issue/references/audit-prompt-template.md, skills/create-issue/references/fallback-audit-dispatch-arms.md, and lib/test/test_render_audit_prompt.py, where they are asserted at the rendered boundary: self.assertIn("exactly these 7 paths", out) and self.assertIn("exactly these 9 files", out). These count-locked enumerations are in this change's blast radius and stay byte-identical. claim=oob-count-literals revision=b8b1594dd8b534a5972385b33fc9beede5a1d034 identity=308fd051ecdc930b3d2e822974056c2d7bcebb4e
  • Verified: the read-only predicate is sound at the baseline commit, established by execution rather than by reading names. A transitive call-graph walk over scripts/issue-audit-state.py — whose registry helper is "def registered_subcommands():" with the docstring "The subcommand names the parser actually exposes (issue Cut create-issue Step 3.6 audit-state round-trips and first-try contract failures #795)." — mapping each add_parser/set_defaults pair to whether save_state is reachable from that handler, reports save_state reachable from init and from every record-* handler, and unreachable from all 18 query-* handlers, from emit-body, from check-claim-staleness, and from write-dispatch-scope. The one case that looks like a counterexample is not one: query-arm accepts --write-landed, and cmd_query_arm uses it purely as an input to "arm, marker = route_arm(args.write_landed == 'yes', hash_ok, prior_unreadable)", writing nothing. write-dispatch-scope is classified mutating by the predicate and reaches no save_state; it therefore takes the section unnecessarily, which is the conservative direction and is accepted. claim=readonly-predicate-soundness revision=b8b1594dd8b534a5972385b33fc9beede5a1d034 identity=1d1284de519b74649cf9b4a8b88aab324be49478
  • Verified: lib/test/check-audit-lifecycle-contracts.py contains "def check_emitting_complement(module, registered, report):", the existing complement-shaped check the new one mirrors, alongside "def check_readbacks(module, registered, report):". lib/test/module-harness.sh contains "git_sandbox() { # assertion-name -> prints an isolated temp dir (rc 0)". docs/DEVFLOW_SYSTEM_OVERVIEW.md carries the heading "11. Deep dive: /prflow:create-issue".
  • Verified: .prflow/config.json, prflow_implement.allowed_tools contains "Bash(lib/test/run.sh:*)" and "Bash(lib/test/run-module.sh:*)", and contains no Bash(python3:*) entry and no entry naming scripts/issue-audit-state.py. Both verification commands this issue names are granted at the baseline commit, so no acceptance criterion here depends on a grant this change would ship. claim=allowed-tools-grants revision=b8b1594dd8b534a5972385b33fc9beede5a1d034 identity=cc544b5eea9b3017dec383f241a5718dedff65f2
  • Verified: executed git check-ignore -v .prflow/tmp/issue-audit-state-x.json.lock, which reports the matching ignore rule at line 5 of the repository's gitignore file, so the sentinel is ignored here and leaves no dirty-tree artifact. That same file records that in an adopter repository the scaffolder's own ignore file covers the state tmp directory, so the sentinel is ignored there too.
  • Verified: scripts/issue-audit-state.py imports the standard library only; its import block runs "import argparse" through "import sys" and closes with "from pathlib import Path", with no third-party name among them. The mechanism's primitives are all standard library and need no platform split.

Flagged assumptions

  • On Windows, a MoveFileEx-backed os.replace onto a path another process currently has open raises PermissionError, which makes a lock-free reader a collision partner for the writer — assumption, confirm before implementing. This premise is asserted from platform semantics and was not measured on this baseline; no Windows host was available. The design does not rest on it: the bounded os.replace retry it motivates is harmless where the condition never arises, and its behavior is driven deterministically by an injected PermissionError rather than by a real Windows host.

Documentation Drift

The Step 1 pass returned DOCS ACCURATE; no drift was found. It did record a gap in depth rather than a contradiction: no file under docs/ states the single-writer assumption, no file states the persistence mechanism, and no file makes a serialization claim, so there is nothing inaccurate to correct — only something absent to add. The Step 1 evidence was not degraded.

Architecture Alignment

The mechanism reuses two patterns already in this repository: the O_CREAT|O_EXCL single-owner create from scripts/verification-flight.py, and tempfile.mkstemp for a per-writer temporary path. It adds no dependency, no platform branch, and no new binary on the execution path. It also honors the repository's fail-closed discipline for a guard whose comparand can be absent: the read-only classification that decides which subcommands skip the section is proved against handler source by a test rather than trusted.

Dependencies

The Python standard library only — os, time, and tempfile. No third-party package, no fcntl, and no msvcrt.

Data/Schema Considerations

The state document gains no field and SCHEMA_VERSION stays the integer 3. The sentinel lives in its own file beside the state document, so an already-written state file loads unchanged and the (no migration path) arm is never reached by this change. This follows the convention _new_doc states for issues #792 and #704 rather than departing from it — with the stronger property that nothing additive is needed at all.

Cross-layer Impact

One Python CLI, its two test surfaces, and one documentation section. No workflow, no shell helper, no config key, and no cloud allowlist token. The change deliberately produces no new mutation-exit class: every non-zero exit the section can raise states that the state could not be persisted, which the shipped routing already carries, so skills/create-issue/references/fallback-state-owner-unavailable.md and its count-locked class enumeration stay byte-identical. scripts/issue-audit-state.py reaches consumer repositories through the prflow_version vendor fetch, so install.sh's workflow copy loop is uninvolved and no half-landed install skew exists. The local/interactive tier is the only tier that runs this file: prflow_implement.allowed_tools grants neither bare python3 nor this script, so no cloud implement run invokes it directly.

Acceptance Criteria

  • scripts/issue-audit-state.py gains a critical-section helper that acquires by creating a sentinel file at the state file's path with .lock appended, using os.open with os.O_CREAT | os.O_EXCL, writing the creating process's pid as the file's sole content, and releasing by unlinking that file.
  • The critical section opens above the load_state call and closes after save_state returns, so the load that a mutation modifies is read inside the section. No compare-and-swap token, no revision counter, and no generation field is added to the state document, because a comparison whose load sits inside the section is vacuous.
  • Stdin ingestion is hoisted out of the handlers into main(), ahead of the dispatch call: main() reads the payload for whichever of --ledger-stdin, --coverage-stdin, --stdin-digest, and --domain-stdin the parsed arguments select, and hands the bytes to the handler. No handler performs a sys.stdin read of its own after this change.
  • The section is applied at the single dispatch site in main() that invokes a subcommand handler — the site that runs after the hoisted stdin read, so the section genuinely is one site and genuinely excludes the blocking read. It is skipped only for subcommands the read-only predicate selects. The read-only predicate selects a subcommand whose name begins query-, plus emit-body and check-claim-staleness; it introduces no new closed set.
  • lib/test/check-audit-lifecycle-contracts.py gains one check proving the read-only predicate's complement fails closed: for every name in registered_subcommands() the predicate classifies read-only, save_state is unreachable from that subcommand's handler through the transitive call graph of module-level functions, not merely absent from the handler's own source text. A source-text-only check passes a handler that reaches save_state one hop away through a helper, which is the fail-open shape this check exists to prevent. The new check takes the loaded module as its first parameter, matching the existing check_emitting_complement(module, registered, report) signature, so a fixture module drives it directly and the positive control below needs no change to how main() resolves its target. The walk's closed set is module-level functions, and its complement is named and handled rather than left silent: a call target the walk cannot resolve to a module-level function — a nested helper, a closure-defined helper, a bound method, and an indirect dispatch through a table and through getattr — makes the check fail closed for that subcommand, reporting the unresolvable call site rather than reporting clean. A read-only-classified handler is therefore proved safe only when every call on its transitive path resolved. The four existing closed sets _CALLER_SUPPLIED_FLAGS, _NEXT_CALL_EXCLUDED, _MULTILINE_READBACKS, and _ROUND_DEFAULTED are unchanged, because this change adds no subcommand and no flag.
  • The section helper takes two keyword arguments — acquire_window_s defaulting to 45 and stale_after_s defaulting to 30 — and the defaults satisfy acquire_window_s > stale_after_s, so an abandoned sentinel is always broken strictly inside the acquisition window and acquisition terminates in a wait-then-acquire and otherwise in a wait-then-break. Neither value is a CLI flag and neither is read from .prflow/config.json. Each is overridable only through a test-only environment variable, DEVFLOW_IAS_ACQUIRE_WINDOW_S and DEVFLOW_IAS_STALE_AFTER_S, so the shell-level tests drive the section's process-boundary behavior in milliseconds; a value that is absent, empty, non-numeric, and non-positive alike is ignored and the default applies. The DEVFLOW_ prefix is the decided choice, not an inherited default: CLAUDE.md freezes that namespace pending the Tier 3 rename tracked by PRFlow rename Tier 3: /prflow:init advisory report for out-of-repo DEVFLOW_* identifiers (from #992) #1004, and a new member spelled PRFLOW_ would be the one variable that ticket's sweep is not looking for. Both names are added to the change's own record of what PRFlow rename Tier 3: /prflow:init advisory report for out-of-repo DEVFLOW_* identifiers (from #992) #1004 must migrate.
  • Because the stdin read is hoisted above the dispatch site, no blocking sys.stdin read happens inside the section and the section's duration is bounded by one small-document read-modify-write. The existing absent-stdin guard each handler applies today — CPython sets sys.stdin to None on a closed descriptor — moves with the read, so its behavior and breadcrumb are unchanged.
  • The section creates the state file's parent directory with parents=True, exist_ok=True before its first exclusive-create attempt, so a fresh clone, a fresh adopter checkout, and a bare test sandbox — none of which carry the ignored state tmp directory — acquire successfully instead of raising on FileNotFoundError.
  • Release is ownership-checked, not path-based: the section records the sentinel's st_dev and st_ino from os.fstat on the descriptor it created, and on release unlinks the path only while a fresh os.stat of it still reports that identity. A mismatch leaves the file untouched and writes a stderr breadcrumb naming the sentinel path and the fact that this section's own sentinel was broken by another process, so an age-broken holder can never delete the breaker's live sentinel.
  • Both unlink sites — the stale break and the ownership-checked release — catch every OSError, not only FileNotFoundError, and breadcrumb it naming the sentinel path and the underlying error. A directory planted at the sentinel path, a permission-denied parent, and a Windows sharing violation are the reaching cases. On the release path the catch is total and the release is best-effort, so a failing unlink never replaces an in-flight exception: the section's own outcome and its routed could not persist state to breadcrumb stand, and the release failure is reported beside them. On the break path a failing unlink returns the mutation to its ordinary retry loop.
  • A mutation that finds the sentinel held retries acquisition with bounded backoff for acquire_window_s. Exhausting that window raises StateError, exits non-zero, leaves the state file byte-identical, and writes a stderr breadcrumb naming the sentinel path, the pid recorded in the sentinel, and that the state could not be persisted — so the exit falls inside the existing cannot-persist-state routing class and creates no fourth mutation-exit destination. Under the shipped bound relation it is unreachable; it is retained as the fail-closed arm for a host whose overrides invert that relation.
  • A mutation that finds a sentinel whose mtime age exceeds stale_after_s re-stats the sentinel immediately before unlinking it and unlinks it only while the observed mtime is unchanged from the one it judged stale. It then re-attempts the exclusive create exactly once, proceeds with the mutation, exits 0, and writes a stderr breadcrumb naming the sentinel path, the pid recorded in the sentinel, and the observed age in seconds. A re-stat showing a changed mtime, a vanished sentinel, and a losing re-create each return the mutation to its ordinary retry loop rather than to a second break.
  • An acquire attempt that fails with an OSError that is neither FileExistsError nor a missing parent directory — a read-only filesystem and a permission denial are the reaching cases — raises StateError immediately, naming the sentinel path, the underlying error, and that the state could not be persisted, rather than retrying for the whole acquire_window_s. A read-only sandbox therefore reports a named cause in under a second through that same existing routing class.
  • A sentinel whose recorded content is unreadable in any of the following shapes — exactly these 5, complete by construction: file-read failure, empty file, whitespace-only content, non-decimal text, content exceeding 64 bytes — is treated as an unestablished holder pid. The breadcrumb renders the pid as the literal unestablished, staleness is still decided by mtime alone, and the mutation's acquire, refuse, and break behavior is otherwise identical to the well-formed case.
  • The ownership-checked release runs on every exit path out of the section — the mutation succeeding, save_state raising, and the handler raising — so a completed process that still owns its sentinel leaves none behind.
  • save_state obtains its temporary path from tempfile.mkstemp in the state file's own directory, with the suffix .json.tmp retained, so two writers never share a temporary path and the existing #546 save_state_cleanup_rows glob assertion keeps its meaning.
  • The mkstemp call sits inside save_state's existing try, and below its path.parent.mkdir(parents=True, exist_ok=True). Unlike the pure path computation it replaces, mkstemp touches the filesystem, so every OSError it raises — a missing parent, a read-only filesystem, a permission denial, an exhausted disk — surfaces as the existing StateError whose message begins could not persist state to . That keeps every save_state failure inside the cannot-persist-state routing class, which is what the no-new-exit-class criterion asserts about the section and must equally hold here.
  • On POSIX hosts the persisted state file's permission mode becomes 0600, and that is the decided new mode, replacing today's umask-derived mode that write_text produced. tempfile.mkstemp creates at 0600 and os.replace carries that mode onto the state file, so the change is decided and asserted rather than incidental: the state file is a per-user run artifact under an ignored tmp directory, read by no other user and by no tooling that runs as one. On Windows CPython derives st_mode's permission bits from the read-only attribute rather than from the creating mode, so the mode equality is neither asserted nor claimed there and the criterion's assertion is skipped on that platform with a named reason. This is the one place the change is platform-conditional, and it is conditional in the assertion only — the mechanism itself still needs no platform split.
  • save_state retries os.replace on PermissionError with bounded backoff, and on exhausting the retries raises the existing StateError whose message begins could not persist state to , removing the partial temporary file exactly as it does today.
  • Read-only subcommands and _emit_next_call's post-mutation re-read acquire no sentinel and are unaffected by one being held. A query-* invocation issued while a sentinel is held exits 0 with its decided answer line.
  • The guarantee is stated as document integrity only, with the decision channel's residual named beside it. Because _emit_next_call re-reads after the section releases and every query-* reads unserialized, a batch of concurrent mutations renders each next_call= line and each query answer against whichever post-image that process observed, so those answers are not authoritative under concurrent invocation. docs/DEVFLOW_SYSTEM_OVERVIEW.md §11 carries that system contract and is canonical for it.
  • skills/create-issue/references/step-3-6-audit.md carries the operational rule the batching caller follows — issue the batch, then re-query once it has completed, rather than acting on any member's own next_call= line — and points at the overview section as canonical rather than restating the contract. That reference is where the rule has to live because it is the surface the agent that issues these calls actually loads; the overview page is loaded by no create-issue surface. Closing the residual itself is follow-up work this issue unblocks and is out of scope here.
  • The state document gains no field and SCHEMA_VERSION remains the integer 3.
  • scripts/issue-audit-state.py imports only the standard library, and imports neither fcntl nor msvcrt. This is asserted by a test row and measured by lib/test/run.sh.
  • The sentinel path is added to no out-of-bounds enumeration. The literals exactly these 7 paths and exactly these 9 files in all four files carrying them — skills/create-issue/references/step-3-6-audit.md, skills/create-issue/references/audit-prompt-template.md, skills/create-issue/references/fallback-audit-dispatch-arms.md, and their rendered-boundary assertions in lib/test/test_render_audit_prompt.py — stay byte-identical, because the sentinel carries no audit reasoning and re-anchors no auditor.
  • docs/DEVFLOW_SYSTEM_OVERVIEW.md §11 states that mutations to the run state are serialized by an exclusive-create sentinel beside the state file, that read-only subcommands are not serialized, and that an abandoned sentinel is recovered by age.
  • lib/test/run-module.sh issue-audit-state exits 0 with no failing assertion, and lib/test/run.sh, run on a committed tree, prints a terminal summary line reporting zero failed. The summary line is the success-path channel for this criterion; a green exit code alone does not discharge it, because the suite's exit code is unchanged by a self-skip. Any skip the summary reports is enumerated and adjudicated rather than tolerated, and the criterion is met when no reported skip belongs to a surface this change touches. The committed-tree qualifier is load-bearing: the stale-prose gate self-skips on a dirty tree by design, for a reason unrelated to this change, so a mid-iteration skip there is expected and is not a failure to discharge.

Implementation Notes

Approach

Add a context-manager helper to scripts/issue-audit-state.py — call it state_section(slug, root) — and enter it at main()'s dispatch site, around the handler call, for every subcommand the read-only predicate does not select.

Acquire. Compose the sentinel path by appending .lock to the string form of state_path(slug, root), giving .prflow/tmp/issue-audit-state-<slug>.json.lock. Build it by string concatenation rather than Path.with_suffix, since a slug may itself contain a dot under the existing [A-Za-z0-9][A-Za-z0-9._-]* guard. That guard already runs inside state_path before the slug keys any path, so the sentinel inherits the existing traversal protection and adds no new surface. mkdir the state file's parent with parents=True, exist_ok=True first, since that directory is ignored and absent on every fresh checkout and every bare test sandbox. Then create the sentinel with os.open(sentinel, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600), write str(os.getpid()), record os.fstat(fd).st_dev and st_ino as the section's ownership token, and close.

On FileExistsError, sleep briefly and retry until acquire_window_s is spent; on each retry read the existing sentinel's mtime and, when its age exceeds stale_after_s, re-stat it and unlink it while the mtime is still the one judged stale, then re-attempt the exclusive create once. Because the acceptance criterion fixes acquire_window_s above stale_after_s, an abandoned sentinel is always broken strictly inside the window, so the loop terminates in an acquire and never in a contention refusal on the shipped bounds. Any other OSError from the create — a read-only filesystem, a permission denial — is raised as a StateError immediately rather than retried, since retrying a condition that does not clear only converts a named failure into a stall. Every StateError this helper raises names the sentinel path and states that the state could not be persisted, which is what keeps the section's failures inside the routing class the skill already carries for a state the tool cannot write, rather than introducing a fourth destination the shipped routing partition does not know.

The mtime re-stat narrows, and does not close, a break-path race: two writers that both judge the same sentinel stale can interleave such that the second unlinks the first's freshly created sentinel. The re-stat makes that require both writers to observe the identical stale mtime and to interleave inside the stat-to-unlink window, against a stale_after_s measured in tens of seconds and a section measured in milliseconds. The residual is accepted and stated rather than eliminated, because eliminating it needs an atomic compare-and-unlink the standard library does not offer portably.

Critical section, and what it deliberately excludes. main() reads any stdin payload the parsed arguments select, then enters state_section, then calls the handler. The handler's own load_state therefore executes inside the section, which is what makes the mutation a genuine read-modify-write under exclusion and is why no compare-and-swap token is needed: two writers cannot both hold a stale read, because the second one's read happens after the first one's write.

Hoisting the stdin read into main() is what makes those two properties hold at once. Wrapping the dispatch site is the only way the section is genuinely one site rather than one edit per mutating handler; but sys.stdin.buffer.read() — reached today by the handlers taking --ledger-stdin, --coverage-stdin, --stdin-digest, and --domain-stdin — is an unbounded blocking read, and leaving it inside a handler the dispatch site wraps would make the section's duration a function of the caller's input latency rather than of a small JSON write, which is the premise the stale bound rests on. Moving the read above the wrap satisfies both. The handlers keep receiving the same bytes, and the existing closed-descriptor guard travels with the read rather than being reimplemented.

The _emit_next_call re-read runs after the section is released, unchanged, so it stays a lock-free read exactly as it is today.

Release, ownership-checked. A finally block stats the sentinel path and unlinks it only while st_dev/st_ino still match the token recorded at acquire, tolerating FileNotFoundError so a section whose sentinel was age-broken by another process still exits cleanly. The identity check is what makes the release safe in the direction a bare path unlink gets wrong: after an age break, the file at that path belongs to the breaker, and unlinking it by path would strip a live holder's exclusion and let a third writer in on top of it. On a mismatch the section leaves the file alone and breadcrumbs that its own sentinel was broken.

Temporary path. In save_state, replace path.with_suffix('.json.tmp') with tempfile.mkstemp(dir=str(path.parent), prefix=path.name + '.', suffix='.json.tmp'), write through the returned descriptor, then os.replace. Keep the .json.tmp suffix so the existing cleanup assertion's glob('*.json.tmp') still selects it. Keep the existing best-effort unlink on the failure path. mkstemp creates at 0600 and os.replace carries that mode across, so the persisted state file's mode changes from today's umask-derived value; that is the decided outcome, asserted rather than left as a side effect.

Replace retry. Wrap the os.replace call in a bounded retry over PermissionError only, leaving every other OSError on today's immediate-failure path so the existing could not persist state to breadcrumb and its test row are unchanged in shape.

Readers. Readers stay lock-free by decision, and the decision is falsifiable: on POSIX, os.replace is atomic, so a reader that opens the state path sees one whole image — the pre-image before the replace lands, the post-image after it — and never a torn document. The hazard that a reader creates is Windows-specific and lands on the writer, which the replace retry above absorbs. Serializing readers would tax every query-* call and every post-mutation next_call= render to defend against a hazard the reader does not itself suffer.

Tunable bounds. state_section takes the two keyword arguments the acceptance criteria define, with the defaults and the ordering relation stated there and not restated here — a numeric pair carried in two places is the rot class this repository already names, and the criterion is the single statement. The helper reads each default from its environment variable when that variable holds a usable positive number, and falls back to the shipped default otherwise. That reader exists so the shell-level assertions drive the section's process boundary in milliseconds; neither value is a CLI flag and neither is read from .prflow/config.json, so the closed-set vocabulary is untouched and the shipped path has one decided setting.

Residuals, stated rather than hidden. A live holder whose section exceeds stale_after_s has its section broken by the next writer, and the ownership-checked release above is what keeps that break from cascading into a lost update. The section is a read, an in-memory mutation, and a write of a small JSON document, so this bound is generous by orders of magnitude; the residual is accepted rather than eliminated, because the one other staleness predicate available — process liveness — has no uniform standard-library spelling across POSIX and Windows, and adopting it would reintroduce exactly the platform split this design avoids. A filesystem whose mtime granularity is coarse relative to the reading process can misjudge that age in both directions; the consequence is bounded to two outcomes — breaking a live section early, and waiting out the acquisition window late — and never to a corrupt document, since the exclusive create still arbitrates. A consumer running an older vendored copy of the tool alongside a newer one does not take the sentinel; the per-writer temporary path is what keeps that case from corrupting bytes, which is why both halves of this change ship together rather than the lock alone.

Code Patterns

  • scripts/verification-flight.py's os.open(path, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600) single-owner create.
  • scripts/stage-draft-write.py's tempfile.mkstemp(prefix=…, suffix=…, dir=…) per-writer temporary path.
  • lib/test/check-audit-lifecycle-contracts.py's existing check_emitting_complement, whose shape the new complement check mirrors: assert the property over the complement of a classification rather than over the classification itself.
  • lib/test/modules/issue-audit-state.sh's git_sandbox fixture and assert_eq "<description>" <expected> <actual> assertion form.

Testing Strategy

Boundary and levels. Every behavior here has an executable boundary: a process exit code, a stderr breadcrumb, the bytes on disk, a raised StateError, and the loadability of the written file. Two levels are used, and neither substitutes for the other. Python-level rows in lib/test/test_python_scripts.py drive save_state, state_section, and the injected-failure paths directly. Shell-level rows in lib/test/modules/issue-audit-state.sh drive the real CLI end to end inside a git_sandbox.

No test spawns two writers and asserts a schedule-dependent outcome. Such a test passes on a contended schedule and fails on a healthy one, and this repository states that there is no known-flake set and every failing assertion is a real failure to diagnose. Contention is instead created deterministically by the test itself, which holds the section by writing the sentinel file directly, and every timing bound the assertions depend on is driven through the test-only override rather than through wall-clock waits.

governing conventions consulted: CLAUDE.md (the six-shape adversarial matrix for a best-effort parser, and the no-known-flake rule), CONTRIBUTING.md, docs/, .prflow/prompt-extensions/create-issue.md (the closed-set complement convention)

Complement of the sentinel-content closed set. Outside the five unreadable shapes lies exactly one class: content that strips to a non-empty decimal integer. Surrounding whitespace, a trailing newline included, is stripped before the decimal test, so a pid written with a trailing newline is well-formed and renders as the pid rather than as unestablished. A well-formed control row asserts that stripping.

Named assertions. The assertion set is closed by this list.

  • uncontended_mutation — a single mutation in a fresh sandbox exits 0, the state document reflects the mutation, and no sentinel file remains. Fails first because state_section does not exist.
  • sequential_writers_both_land — two mutations run one after the other; the document holds both records. This is the assertion where both writers legitimately succeed, and it exists so the mechanism cannot be satisfied by a design that refuses the second writer unconditionally. Fails first against a section that leaks its sentinel, because the second mutation is then refused.
  • contended_wait_then_acquire — the test writes the sentinel itself with a current mtime and a short stale_after_s, then runs a mutation with the shipped bound relation. The mutation waits, breaks the stale sentinel, lands, and exits 0. Fails against today's code by exhibiting the defect: today the mutation ignores the sentinel entirely and writes immediately.
  • inverted_bounds_refuse_as_unpersistable — with the bounds overridden into acquire_window_s < stale_after_s, the test writes a fresh sentinel and runs a mutation. It exits non-zero, the state file is byte-identical to its pre-call bytes, and stderr carries the sentinel path, the recorded pid, and the could-not-persist wording that keeps the exit inside the existing routing class.
  • cli_stale_break_exit_and_breadcrumb — the shell-level assertion at the process boundary: inside a git_sandbox, with DEVFLOW_IAS_STALE_AFTER_S and DEVFLOW_IAS_ACQUIRE_WINDOW_S set to sub-second values, a sentinel is planted and a real CLI mutation is invoked. The assertion is on the process exit code and the stderr breadcrumb text, since those are what the orchestrator routes on.
  • acquires_when_state_dir_absent — in a bare sandbox with no state tmp directory, the first mutation exits 0 and the directory is created. Fails against a design that attempts the exclusive create before the mkdir, by raising on FileNotFoundError.
  • release_does_not_unlink_a_foreign_sentinel — the section's sentinel is replaced mid-section by a file with a different inode; the assertion is that release leaves that file in place, breadcrumbs the broken-section case, and exits without deleting another holder's exclusion.
  • stdin_is_read_before_acquire — a mutation taking a stdin payload is driven with a stub that asserts no sentinel exists at the moment the stdin read begins, pinning the ordering the stale bound depends on, and that no sys.stdin read occurs after the section is entered.
  • state_file_mode_is_0600 — on a POSIX host, after a mutation, os.stat(state_path).st_mode & 0o777 equals 0o600. Fails against today's code, which lands the umask-derived mode. The assertion is skipped on Windows with a named reason, since st_mode's permission bits there are derived from the read-only attribute rather than from the creating mode.
  • mkstemp_failure_is_routedtempfile.mkstemp is stubbed to raise PermissionError, and a paired row drives a genuinely read-only parent where the platform permits one. Each asserts save_state raises StateError with a message beginning could not persist state to , never a bare OSError traceback, so the failure stays inside the routing class the skill already carries.
  • unlink_failure_never_replaces_the_real_exceptionos.unlink is stubbed to raise PermissionError at the release site while the handler is already raising a StateError. The assertion is that the StateError and its could not persist state to breadcrumb survive, and that the release failure is breadcrumbed beside it rather than in place of it. A paired row stubs the same failure at the stale-break site and asserts the mutation returns to its retry loop.
  • readonly_predicate_fails_closed_on_unresolvable_calls — the complement control for the call-graph walk: a fixture module in which a query--prefixed handler reaches save_state through a nested helper, a bound method, and an indirect getattr dispatch. Each asserts the check reports the unresolvable call site rather than reporting clean.
  • bounds_override_ignores_unusable_values — one row per shape the override rejects: absent, empty, non-numeric, zero, and negative. Each asserts the shipped default applies.
  • stdin_hoist_preserves_the_absent_stdin_guard — with sys.stdin set to None, a mutation taking a stdin payload produces the same breadcrumb and exit code it produces today, pinning that hoisting the read above the dispatch site did not drop the existing guard.
  • stale_sentinel_breaks — the test writes the sentinel and back-dates its mtime with os.utime past stale_after_s, then runs a mutation. It exits 0, the mutation landed, stderr carries the path, the pid, and the observed age, and no sentinel remains.
  • stale_break_rechecks_mtimeos.stat is stubbed so the second stat returns a different mtime from the first, and the assertion is that no unlink occurs and the mutation returns to its retry loop. This is the break-path race narrowing, driven deterministically by the stub rather than by a real interleaving.
  • acquire_oserror_fails_fastos.open is stubbed to raise PermissionError, and the assertion is that StateError is raised naming the sentinel path and the underlying error within a duration far below acquire_window_s, rather than after the window elapses. A paired assertion drives the same path through a genuinely read-only directory where the platform permits creating one.
  • malformed_sentinel_pid — one assertion per shape the acceptance criterion enumerates: file-read failure, empty file, whitespace-only content, non-decimal text, and content exceeding 64 bytes. Each asserts the breadcrumb renders the pid as unestablished and that the acquire and break decision is unchanged from the well-formed case. A well-formed control row writes a decimal pid followed by a trailing newline and asserts it renders as the pid, pinning the complement boundary.
  • section_released_on_raise — one assertion forces the handler to raise inside the section, one forces save_state to raise. Both assert a non-zero exit and that no sentinel remains.
  • temp_path_is_unique — a decoy file is placed at the old fixed path issue-audit-state-<slug>.json.tmp with known bytes; a mutation runs and succeeds; the decoy is asserted byte-identical afterwards. This fails against today's code by exhibiting the exact defect: today's save_state truncates and rewrites that path.
  • replace_retry_absorbs_permission_erroros.replace is stubbed to raise PermissionError once and then succeed; the mutation exits 0 and the document lands. A paired assertion stubs it to raise on every attempt and asserts StateError with a message beginning could not persist state to , and that no .json.tmp file survives.
  • readers_are_not_serialized — with a sentinel held, a query-* invocation exits 0 and prints its decided answer line. This pins the decided complement of the read-only predicate and fails RED if a later change makes readers take the section.
  • written_bytes_load — after each success arm above, a subsequent load_state succeeds and _validate passes, so the claim is about the bytes that landed rather than the document held in memory.
  • readonly_complement_fails_closed — the new lib/test/check-audit-lifecycle-contracts.py check is driven with a fixture in which a subcommand whose name begins query- reaches save_state, and asserts the check reports the violation. This is the planted-defect positive control for the claim that the predicate cannot silently classify a mutating subcommand as read-only.
  • stdlib_only_imports — asserts the module's import set contains neither fcntl nor msvcrt, and nothing outside the standard library.

Coverage mapping. Every acceptance criterion above maps to at least one named assertion, and every assertion maps back to a criterion, with two stated exceptions. The criterion requiring the out-of-bounds literals to stay byte-identical is discharged by the existing rendered-boundary assertions in lib/test/test_render_audit_prompt.py, which this change adds nothing to and must leave green. The criterion requiring the docs/DEVFLOW_SYSTEM_OVERVIEW.md §11 update, and the criterion stating the decision channel's non-authoritative-under-concurrency contract in that same section, are both discharged by reading the rendered section in review, deliberately without a new assertion, because this repository prohibits documentation-presence and wording-only pins and neither sentence carries a machine-consumed contract. Those two and the out-of-bounds criterion are the only criteria without a new named assertion, and each says why.

Measurement. The assertions above are measured by two commands, both already granted in prflow_implement.allowed_tools at the baseline commit: lib/test/run-module.sh issue-audit-state for the shell-level assertions, and lib/test/run.sh for the Python-level assertions and the full gate. Neither bare python3 nor scripts/issue-audit-state.py is a granted head, so no criterion here asks a cloud run to invoke an ungranted helper.

Dimensions deliberately dropped. Scale and performance carry no rows: the section is bounded by a single small-document read-modify-write and no acceptance criterion implies a throughput property. Security and authorization carry no rows beyond the path-traversal guard the sentinel inherits from state_path's existing slug validation, since the change crosses no access boundary and reads no third-party text.

Documentation Needed

  • docs/DEVFLOW_SYSTEM_OVERVIEW.md
  • skills/create-issue/references/step-3-6-audit.md

A changeset file under .changeset/ is required, since this change touches the engine surface. The default bump is patch.

Potential Gotchas

  • The four closed sets in scripts/issue-audit-state.py are total over registered_subcommands(). Adding a subcommand, and adding a flag, would each force a classification into all four in the same change. This design adds neither, and the read-only predicate is deliberately derived from the existing naming rule plus two literals already in _NEXT_CALL_EXCLUDED rather than becoming a fifth closed set.
  • lib/test/test_python_scripts.py's #546 save_state_cleanup_rows assertion selects the temporary file by the glob *.json.tmp. Changing the suffix silently neuters that assertion while leaving it green.
  • The out-of-bounds enumerations in the create-issue references carry count-locked literals asserted at the rendered boundary. Adding the sentinel to those lists breaks those assertions and is not required, since the sentinel holds a pid and nothing else.
  • state_path resolves through _repo_root(), a git rev-parse --show-toplevel subprocess with a Path.cwd() fallback. The sentinel must be composed from the resolved state path rather than recomputed, so a run whose git resolution degraded still locks the file it actually writes.
  • The shipped skill routing partitions the tool's non-zero mutation exits across a closed set of destinations, and its class counts are stated as prose in skills/create-issue/references/fallback-state-owner-unavailable.md. A new refusal class would land outside that partition and degrade a whole run to the bounded one-round fallback over transient contention. This design avoids that by wording every section failure as a cannot-persist-state condition and by choosing bounds under which contention never refuses at all.
  • tempfile.mkstemp creates at 0600 while write_text creates at the process umask, so the temp-path change silently alters the persisted state file's mode unless the new mode is decided and asserted.
  • This change is scoped to write safety, and to document integrity specifically. The decision channel's concurrency residual is stated, not closed. The lens partition, the multi-auditor round record, and the audit panel are explicitly out of scope; they are the follow-up work this unblocks, each depends on this landing first, and the multi-auditor round record is where the parallel-batch decision-channel residual must actually be resolved.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions