From 808acc34ba2faeccfbcfd23b282b50e28befb159 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Sat, 1 Aug 2026 01:47:46 +0000 Subject: [PATCH 1/9] =?UTF-8?q?feat:=20implement=20issue=20#1040=20?= =?UTF-8?q?=E2=80=94=20serialize=20audit=20state=20owner=20writes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add an exclusive-create sentinel critical section (_StateSection) around the single dispatch site in issue-audit-state.py's main() for every non-read-only subcommand, plus a per-writer tempfile.mkstemp temp path and a bounded PermissionError retry in save_state. Stdin ingestion is hoisted into main() above the section so no handler blocks on stdin inside it. Add a fail-closed transitive call-graph check proving save_state is unreachable from every read-only-classified subcommand. Co-Authored-By: Claude Opus 4.8 --- lib/test/check-audit-lifecycle-contracts.py | 211 +++++++ lib/test/modules/coverage-map.json | 4 + lib/test/modules/issue-audit-state.sh | 41 ++ lib/test/test_python_scripts.py | 628 +++++++++++++++++++- scripts/issue-audit-state.py | 463 ++++++++++++--- 5 files changed, 1270 insertions(+), 77 deletions(-) diff --git a/lib/test/check-audit-lifecycle-contracts.py b/lib/test/check-audit-lifecycle-contracts.py index a18a6bedf..139fe08e4 100755 --- a/lib/test/check-audit-lifecycle-contracts.py +++ b/lib/test/check-audit-lifecycle-contracts.py @@ -43,6 +43,8 @@ from __future__ import annotations import argparse +import ast +import builtins import contextlib import importlib.util import inspect @@ -174,6 +176,214 @@ def _subparser_of(parser, name): return None +# The builtins a call may name and still be provably unable to reach a module-level +# save_state — every builtin function, type, and exception (`len`, `print`, `sorted`, +# `SystemExit`, `ValueError`, …). `getattr` is safe in its BARE-name value form +# `getattr(x, y)`; the dangerous form `getattr(x, y)()` is a Call-as-callee, caught by the +# indirect-dispatch arm below regardless. +_SAFE_BUILTIN_CALLS = frozenset(dir(builtins)) + + +def _module_level_names(tree): + """(functions_by_name, safe_leaf_names) for the module. `functions_by_name` are the + function defs the walk FOLLOWS — every module-level def PLUS every nested/closure helper + they define, indexed by name, so a bare-name call to a helper the source statically + carries is followed (proving whether it reaches save_state) rather than flagged + unresolvable. `safe_leaf_names` are names a call may reference and still reach no + followable function by name — imported names and module-level class names (a class + instantiation cannot BE the `save_state` function; the two classes in this module — + StateError, _StateSection — reach no save_state). A name collision across scopes + over-approximates by keeping one body; that only ever follows MORE, which is the + fail-closed direction for an unreachability proof. + """ + functions = {} + for node in ast.walk(tree): + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + functions[node.name] = node + safe = set(_SAFE_BUILTIN_CALLS) + for node in tree.body: + if isinstance(node, ast.ClassDef): + safe.add(node.name) + elif isinstance(node, ast.Import): + for alias in node.names: + safe.add((alias.asname or alias.name).split(".")[0]) + elif isinstance(node, ast.ImportFrom): + for alias in node.names: + safe.add(alias.asname or alias.name) + return functions, safe + + +def check_readonly_complement(module, registered, report): + """Every read-only-classified subcommand's handler must be UNABLE to reach `save_state` + through the transitive call graph of module-level functions — not merely have it absent + from the handler's own source text (issue #1040). A source-text-only check would pass a + handler that reaches save_state one hop away through a helper, the fail-open shape this + exists to prevent. + + The walk's closed set is module-level functions; its complement is named and handled, + never left silent. A call target the walk cannot resolve to a module-level function — a + bare-name alias/closure/nested helper, or an indirect dispatch through a table or + getattr — makes the check FAIL CLOSED for that subcommand, reporting the unresolvable + call site rather than reporting clean. An attribute/method call reaches no module-level + function by name (module-level functions are called as bare names), so it cannot be + `save_state` and is safe. A read-only-classified handler is proved safe only when every + call on its transitive path resolved. + """ + # Analyze the PASSED module's own source (not IAS directly), so a fixture module drives + # this check exactly like check_emitting_complement — the positive controls in the test + # suite load a crafted module and expect a Refusal. + try: + source = inspect.getsource(module) + except (OSError, TypeError) as exc: + raise Refusal(f"readonly-complement: could not read the module source of " + f"{getattr(module, '__name__', module)!r}: {exc}") from exc + try: + tree = ast.parse(source) + except SyntaxError as exc: + raise Refusal(f"readonly-complement: could not parse the module source: " + f"{exc}") from exc + functions, safe = _module_level_names(tree) + + parser = module.build_parser() + name_to_handler = {} + for action in parser._actions: # noqa: SLF001 + if isinstance(action, argparse._SubParsersAction): # noqa: SLF001 + for name, sub in action.choices.items(): + fn = sub._defaults.get("func") # noqa: SLF001 + name_to_handler[name] = getattr(fn, "__name__", None) + + predicate = module._is_read_only # noqa: SLF001 + readonly = sorted(n for n in registered if predicate(n)) + if not readonly: + raise Refusal("readonly-complement: the read-only predicate selected NO registered " + "subcommand, so the critical section would wrap every command and this " + "complement check would be vacuous") + + # Module-level constants (name -> value node), so a read-only handler that dispatches + # over a module-level TABLE of producers (query-boundary's `for _, produce in + # _BOUNDARY_PRODUCERS: produce(...)`) is RESOLVED through the constant rather than + # failed closed: name-reachability propagates through the constant's value into the + # producer lambdas and the module functions they name. + constants = {} + for node in tree.body: + if isinstance(node, ast.Assign): + for tgt in node.targets: + if isinstance(tgt, ast.Name): + constants[tgt.id] = node.value + + def _targets_name(target, name): + """True iff an assignment/for target binds `name` (a Name, or a Tuple/List of + them).""" + if isinstance(target, ast.Name): + return target.id == name + if isinstance(target, (ast.Tuple, ast.List)): + return any(_targets_name(el, name) for el in target.elts) + return False + + def _local_call_resolvable(node, name): + """A bare-name call to a LOCAL `name` is resolvable only when `name` is bound within + `node` from a source that references a module-level function or constant — then the + call target is reached through name-propagation over that source (query-boundary's + `for _, produce in _BOUNDARY_PRODUCERS` binds `produce` from a module constant). A + name bound only from a getattr()/subscript/opaque source, or not bound at all (a + parameter or callback), is unresolvable and fails the subcommand closed. + """ + for sub in ast.walk(node): + srcs = [] + if isinstance(sub, ast.For) and _targets_name(sub.target, name): + srcs.append(sub.iter) + elif isinstance(sub, ast.Assign) and any( + _targets_name(t, name) for t in sub.targets): + srcs.append(sub.value) + elif isinstance(sub, ast.comprehension) and _targets_name(sub.target, name): + srcs.append(sub.iter) + for src in srcs: + for ref in ast.walk(src): + if isinstance(ref, ast.Name) and ( + ref.id in functions or ref.id in constants): + return True + return False + + def _analyze_node(node): + """(module-level names referenced in Load context, has-computed-callee, + [unresolvable local-call names]) in node's whole subtree — nested defs and lambdas + included, so a helper or a producer lambda is not a blind spot. A computed callee + (getattr(...)() / table[key]()) and a bare-name call to an unresolvable local + (a getattr-aliased callable) are the two shapes the name walk cannot resolve. + """ + names = set() + has_computed = False + unresolvable = [] + for sub in ast.walk(node): + if isinstance(sub, ast.Name) and isinstance(sub.ctx, ast.Load): + if sub.id in functions or sub.id in constants: + names.add(sub.id) + elif isinstance(sub, ast.Call): + fn = sub.func + if isinstance(fn, ast.Name): + nm = fn.id + if (nm != "save_state" and nm not in functions + and nm not in constants and nm not in safe + and not _local_call_resolvable(node, nm)): + unresolvable.append(nm) + elif not isinstance(fn, ast.Attribute): + has_computed = True + return names, has_computed, unresolvable + + def analyze(subcmd, handler): + # Transitive name-reachability over module-level functions AND constants. A read-only + # handler is proved safe only when `save_state` is unreachable by name AND no + # reachable function contains a computed (unresolvable) callee. This is sound by + # name — a handler that reaches save_state through any bare-name path (a call, a + # callback passed by name, a nested def, or a producer table) names it and is + # caught; the one residue the name walk cannot see, a getattr('save_state')() + # string dispatch, is caught by the computed-callee arm. + seen = set() + queue = [handler] + while queue: + nm = queue.pop() + if nm in seen: + continue + seen.add(nm) + node = functions.get(nm) + is_func = node is not None + if node is None: + node = constants.get(nm) + if node is None: + continue + names, has_computed, unresolvable = _analyze_node(node) + if is_func and has_computed: + raise Refusal( + f"readonly-complement: read-only subcommand {subcmd!r} reaches an " + f"indirect dispatch (a computed callee) in {nm!r}; the walk cannot " + "prove that path does not reach save_state — fail closed") + if is_func and unresolvable: + raise Refusal( + f"readonly-complement: read-only subcommand {subcmd!r} makes an " + f"unresolvable call {unresolvable[0]!r}() in {nm!r} (a local bound from " + "no module-level function or constant — a getattr alias, a closure, or a " + "callback); a read-only handler is proved safe only when every call " + "resolves") + if "save_state" in names: + raise Refusal( + f"readonly-complement: read-only subcommand {subcmd!r} reaches " + f"save_state (through {nm}); the read-only predicate has misclassified " + "a mutating subcommand as read-only") + queue.extend(names - seen) + + for name in readonly: + handler = name_to_handler.get(name) + if handler is None or handler not in functions: + raise Refusal( + f"readonly-complement: read-only subcommand {name!r} maps to handler " + f"{handler!r}, which is not a module-level function — the walk cannot begin, " + "so the classification is unproven") + analyze(name, handler) + + report.append(f"readonly-complement: all {len(readonly)} read-only subcommands proved " + "unable to reach save_state through the module-level call graph") + + def check_readbacks(module, registered, report): """The docstring's read-back enumeration vs. the dispatched `_MULTILINE_READBACKS`.""" doc = module.__doc__ or "" @@ -435,6 +645,7 @@ def main(): module = _load_module() registered = module.registered_subcommands() check_readbacks(module, registered, report) + check_readonly_complement(module, registered, report) check_emitting_complement(module, registered, report) check_round_defaulted(module, registered, report) check_next_action_routing_totality(module, report) diff --git a/lib/test/modules/coverage-map.json b/lib/test/modules/coverage-map.json index a10111b10..c67deef28 100644 --- a/lib/test/modules/coverage-map.json +++ b/lib/test/modules/coverage-map.json @@ -665,6 +665,10 @@ "note": "", "owner": "unmodularized" }, + "1040": { + "note": "", + "owner": "issue-audit-state" + }, "126": { "note": "", "owner": "unmodularized" diff --git a/lib/test/modules/issue-audit-state.sh b/lib/test/modules/issue-audit-state.sh index cebdeec67..5970ad27e 100644 --- a/lib/test/modules/issue-audit-state.sh +++ b/lib/test/modules/issue-audit-state.sh @@ -2196,3 +2196,44 @@ if [ -d "$WP_SB" ]; then "0" "$(sed -n 1p "$WP_SB/.wp-embed-rc" 2>/dev/null)" rm -rf "$WP_SB" fi + +# ── issue #1040: write-serialization sentinel at the process boundary ────────────── +# The shell-level assertions on the section's process boundary — the exit code and stderr +# breadcrumb the orchestrator routes on — driven with sub-second DEVFLOW_IAS_* overrides so +# the section's timing behavior is exercised in milliseconds rather than at the shipped +# 30s/45s bounds. Contention is created deterministically (the test plants the sentinel +# itself), never by racing two writers. +SL_SB="$(git_sandbox '#1040 cli_stale_break_exit_and_breadcrumb')" +( + cd "$SL_SB" || exit 1 + mkdir -p .prflow/tmp + # (1) stale break: plant a sentinel, age it past a sub-second stale_after_s, then run a + # real mutation. It must break the stale sentinel, proceed, and exit 0. + printf '4242' > .prflow/tmp/issue-audit-state-s.json.lock + sleep 0.2 + DEVFLOW_IAS_STALE_AFTER_S=0.05 DEVFLOW_IAS_ACQUIRE_WINDOW_S=0.5 \ + python3 "$IAS" init s > .sl-out 2> .sl-err + printf '%s' "$?" > .sl-rc + # (2) contention refusal: a FRESH sentinel with INVERTED bounds (window < stale) is never + # broken, so acquisition exhausts the window → exit non-zero, no state persisted, and + # the could-not-persist breadcrumb (the routing class the skill already carries). + printf '9999' > .prflow/tmp/issue-audit-state-s3.json.lock + DEVFLOW_IAS_ACQUIRE_WINDOW_S=0.2 DEVFLOW_IAS_STALE_AFTER_S=30 \ + python3 "$IAS" init s3 > .sl3-out 2> .sl3-err + printf '%s' "$?" > .sl3-rc + # the refused mutation left no state file for s3 + [ -f .prflow/tmp/issue-audit-state-s3.json ] && printf 'yes' > .sl3-state || printf 'no' > .sl3-state +) || true +assert_eq "#1040 cli_stale_break_exit_and_breadcrumb: the mutation breaks the stale sentinel and exits 0" \ + "0" "$(cat "$SL_SB/.sl-rc" 2>/dev/null)" +assert_eq "#1040 cli_stale_break_exit_and_breadcrumb: stderr carries the stale-break breadcrumb" \ + "1" "$(grep -c 'broke a stale audit-state sentinel' "$SL_SB/.sl-err" 2>/dev/null)" +assert_eq "#1040 cli_stale_break_exit_and_breadcrumb: the breadcrumb names the recorded pid" \ + "1" "$(grep -c '4242' "$SL_SB/.sl-err" 2>/dev/null)" +assert_eq "#1040 cli_contention_refusal: inverted bounds over a fresh sentinel exit non-zero (1)" \ + "1" "$(cat "$SL_SB/.sl3-rc" 2>/dev/null)" +assert_eq "#1040 cli_contention_refusal: the refusal names could-not-persist-state" \ + "1" "$(grep -c 'could not persist state' "$SL_SB/.sl3-err" 2>/dev/null)" +assert_eq "#1040 cli_contention_refusal: the refused mutation persisted no state file" \ + "no" "$(cat "$SL_SB/.sl3-state" 2>/dev/null)" +rm -rf "$SL_SB" diff --git a/lib/test/test_python_scripts.py b/lib/test/test_python_scripts.py index 6ee188100..24a1774ee 100755 --- a/lib/test/test_python_scripts.py +++ b/lib/test/test_python_scripts.py @@ -6962,6 +6962,617 @@ def _round709(**kw): "the failed persist", [], list((_ss_root / '.prflow' / 'tmp').glob('*.json.tmp'))) +# ── issue #1040: write-serialization sentinel + per-writer temp path ─────────────── +print() +print("issue-audit-state: #1040 write-serialization (state_section + mkstemp)") + +import time as _time1040 # noqa: E402 + + +def _ias1040_sentinel(root): + return str(issue_audit_state.state_path('s', root=root)) + '.lock' + + +def _ias1040_capture_stderr(fn): + _buf = io.StringIO() + with contextlib.redirect_stderr(_buf): + result = fn() + return result, _buf.getvalue() + + +# temp_path_is_unique — a decoy at the OLD fixed temp path is NOT clobbered by save_state, +# which now uses tempfile.mkstemp for a unique per-writer path. RED against the pre-#1040 +# save_state (path.with_suffix('.json.tmp') truncated exactly that path). +with tempfile.TemporaryDirectory() as _td: + _R = Path(_td) + _tmpdir = _R / '.prflow' / 'tmp' + _tmpdir.mkdir(parents=True) + _decoy = _R / '.prflow' / 'tmp' / 'issue-audit-state-s.json.tmp' + _decoy.write_bytes(b'DECOY-BYTES') + issue_audit_state.save_state(_state([]), 's', root=_R) + assert_eq("#1040 temp_path_is_unique: the decoy at the old fixed temp path is untouched", + b'DECOY-BYTES', _decoy.read_bytes()) + assert_eq("#1040 temp_path_is_unique: no stray .json.tmp survives a clean persist", + [_decoy], sorted(_tmpdir.glob('*.json.tmp'))) + # written_bytes_load: the bytes that landed re-load and re-validate. + _reloaded = issue_audit_state.load_state('s', root=_R) + assert_eq("#1040 written_bytes_load: the persisted bytes load and validate", + 0, len(_reloaded['rounds'])) + +# state_file_mode_is_0600 (POSIX only): mkstemp creates 0600 and os.replace carries it. +if os.name != 'nt': + with tempfile.TemporaryDirectory() as _td: + _R = Path(_td) + issue_audit_state.save_state(_state([]), 's', root=_R) + _mode = os.stat(issue_audit_state.state_path('s', root=_R)).st_mode & 0o777 + assert_eq("#1040 state_file_mode_is_0600: persisted state file mode is 0600 on POSIX", + 0o600, _mode) +else: + assert_eq("#1040 state_file_mode_is_0600: skipped on Windows (st_mode bits derive from " + "the read-only attribute, not the creating mode)", True, True) + +# mkstemp_failure_is_routed: a tempfile.mkstemp OSError surfaces as the could-not-persist +# StateError, never a bare OSError traceback. +with tempfile.TemporaryDirectory() as _td: + _R = Path(_td) + _orig_mkstemp = issue_audit_state.tempfile.mkstemp + + def _boom_mkstemp(*a, **k): + raise PermissionError('mkstemp denied') + + issue_audit_state.tempfile.mkstemp = _boom_mkstemp + try: + try: + issue_audit_state.save_state(_state([]), 's', root=_R) + assert_eq("#1040 mkstemp_failure_is_routed: raises StateError", + "raised", "no exception") + except issue_audit_state.StateError as _e: + assert_eq("#1040 mkstemp_failure_is_routed: message begins could-not-persist", + True, str(_e).startswith('could not persist state to ')) + finally: + issue_audit_state.tempfile.mkstemp = _orig_mkstemp + +# replace_retry_absorbs_permission_error: os.replace raising PermissionError once then +# succeeding lands the doc; raising every time raises could-not-persist and leaves no +# .json.tmp. +with tempfile.TemporaryDirectory() as _td: + _R = Path(_td) + _orig_replace = issue_audit_state.os.replace + _calls = {'n': 0} + + def _flaky_replace(src, dst): + _calls['n'] += 1 + if _calls['n'] == 1: + raise PermissionError('sharing violation') + return _orig_replace(src, dst) + + issue_audit_state.os.replace = _flaky_replace + try: + issue_audit_state.save_state(_state([]), 's', root=_R) + assert_eq("#1040 replace_retry_absorbs_permission_error: one PermissionError then " + "success lands the doc", 0, + len(issue_audit_state.load_state('s', root=_R)['rounds'])) + assert_eq("#1040 replace_retry_absorbs_permission_error: the retry actually re-ran " + "os.replace", 2, _calls['n']) + finally: + issue_audit_state.os.replace = _orig_replace + +with tempfile.TemporaryDirectory() as _td: + _R = Path(_td) + (_R / '.prflow' / 'tmp').mkdir(parents=True) + _orig_replace = issue_audit_state.os.replace + + def _always_fail_replace(src, dst): + raise PermissionError('always') + + issue_audit_state.os.replace = _always_fail_replace + try: + try: + issue_audit_state.save_state(_state([]), 's', root=_R) + assert_eq("#1040 replace_retry_absorbs_permission_error (exhausted): raises " + "StateError", "raised", "no exception") + except issue_audit_state.StateError as _e: + assert_eq("#1040 replace_retry_absorbs_permission_error (exhausted): could-not-" + "persist breadcrumb", True, + str(_e).startswith('could not persist state to ')) + assert_eq("#1040 replace_retry_absorbs_permission_error (exhausted): no .json.tmp " + "survives", [], list((_R / '.prflow' / 'tmp').glob('*.json.tmp'))) + finally: + issue_audit_state.os.replace = _orig_replace + +# uncontended_mutation + acquires_when_state_dir_absent: a section in a BARE sandbox (no +# .prflow/tmp) acquires, the mutation lands, and no sentinel remains. +with tempfile.TemporaryDirectory() as _td: + _R = Path(_td) + with issue_audit_state._StateSection('s', root=_R): + issue_audit_state.save_state(_state([]), 's', root=_R) + assert_eq("#1040 uncontended_mutation: the mutation persisted under the section", + 0, len(issue_audit_state.load_state('s', root=_R)['rounds'])) + assert_eq("#1040 acquires_when_state_dir_absent + uncontended_mutation: no sentinel " + "remains after a clean section", False, os.path.exists(_ias1040_sentinel(_R))) + +# sequential_writers_both_land: two sequential sections both write; the doc holds both +# rounds and no sentinel leaks (a leaked sentinel would refuse the second writer). +with tempfile.TemporaryDirectory() as _td: + _R = Path(_td) + with issue_audit_state._StateSection('s', root=_R): + issue_audit_state.save_state(_state([_round(1, 'inline', None)]), 's', root=_R) + with issue_audit_state._StateSection('s', root=_R): + _d = issue_audit_state.load_state('s', root=_R) + _d['rounds'].append(_round(2, 'inline', None)) + issue_audit_state.save_state(_d, 's', root=_R) + assert_eq("#1040 sequential_writers_both_land: both rounds are present", 2, + len(issue_audit_state.load_state('s', root=_R)['rounds'])) + assert_eq("#1040 sequential_writers_both_land: no sentinel leaked", False, + os.path.exists(_ias1040_sentinel(_R))) + +# section_released_on_raise: a handler raising inside the section, and save_state raising, +# both leave no sentinel behind. +with tempfile.TemporaryDirectory() as _td: + _R = Path(_td) + try: + with issue_audit_state._StateSection('s', root=_R): + raise ValueError('handler blew up') + except ValueError: + pass + assert_eq("#1040 section_released_on_raise (handler raises): no sentinel remains", + False, os.path.exists(_ias1040_sentinel(_R))) +with tempfile.TemporaryDirectory() as _td: + _R = Path(_td) + # Occupy the state path with a directory so save_state raises inside the section. + (_R / '.prflow' / 'tmp' / 'issue-audit-state-s.json').mkdir(parents=True) + try: + with issue_audit_state._StateSection('s', root=_R): + issue_audit_state.save_state(_state([]), 's', root=_R) + except issue_audit_state.StateError: + pass + assert_eq("#1040 section_released_on_raise (save_state raises): no sentinel remains", + False, os.path.exists(_ias1040_sentinel(_R))) + +# stale_sentinel_breaks: a sentinel back-dated past stale_after_s is broken; the section +# acquires, the breadcrumb names the path, pid and age, and no sentinel remains. +with tempfile.TemporaryDirectory() as _td: + _R = Path(_td) + (_R / '.prflow' / 'tmp').mkdir(parents=True) + _sent = _ias1040_sentinel(_R) + with open(_sent, 'w') as _fh: + _fh.write('4242') + os.utime(_sent, (_time1040.time() - 1000, _time1040.time() - 1000)) + + def _do_break(): + with issue_audit_state._StateSection('s', root=_R, acquire_window_s=2, + stale_after_s=1): + issue_audit_state.save_state(_state([]), 's', root=_R) + return True + + _ok, _err = _ias1040_capture_stderr(_do_break) + assert_eq("#1040 stale_sentinel_breaks: the mutation lands after breaking the stale " + "sentinel", 0, len(issue_audit_state.load_state('s', root=_R)['rounds'])) + assert_eq("#1040 stale_sentinel_breaks: breadcrumb names the sentinel path", True, + _sent in _err) + assert_eq("#1040 stale_sentinel_breaks: breadcrumb names the recorded pid", True, + '4242' in _err) + assert_eq("#1040 stale_sentinel_breaks: breadcrumb reports the observed age", True, + 'age ' in _err) + assert_eq("#1040 stale_sentinel_breaks: no sentinel remains after release", False, + os.path.exists(_sent)) + +# inverted_bounds_refuse_as_unpersistable: with acquire_window_s < stale_after_s and a +# FRESH sentinel, the section never breaks and exhausts the window → could-not-persist +# StateError naming the sentinel + pid, and the state file is byte-identical. +with tempfile.TemporaryDirectory() as _td: + _R = Path(_td) + issue_audit_state.save_state(_state([]), 's', root=_R) + _before = issue_audit_state.state_path('s', root=_R).read_bytes() + _sent = _ias1040_sentinel(_R) + with open(_sent, 'w') as _fh: + _fh.write('9999') + try: + with issue_audit_state._StateSection('s', root=_R, acquire_window_s=0.2, + stale_after_s=30): + issue_audit_state.save_state(_state([_round(1, 'inline', None)]), 's', root=_R) + assert_eq("#1040 inverted_bounds_refuse_as_unpersistable: raises StateError", + "raised", "no exception") + except issue_audit_state.StateError as _e: + assert_eq("#1040 inverted_bounds_refuse_as_unpersistable: could-not-persist " + "breadcrumb naming the sentinel", True, + str(_e).startswith('could not persist state to ') and _sent in str(_e)) + assert_eq("#1040 inverted_bounds_refuse_as_unpersistable: breadcrumb names the " + "recorded pid", True, '9999' in str(_e)) + os.unlink(_sent) + assert_eq("#1040 inverted_bounds_refuse_as_unpersistable: the state file is byte-" + "identical", _before, issue_audit_state.state_path('s', root=_R).read_bytes()) + +# acquire_oserror_fails_fast: an os.open OSError that is neither FileExists nor a missing +# parent raises StateError immediately (well under the acquire window), naming the sentinel. +with tempfile.TemporaryDirectory() as _td: + _R = Path(_td) + (_R / '.prflow' / 'tmp').mkdir(parents=True) + _orig_open = issue_audit_state.os.open + + def _denied_open(*a, **k): + raise PermissionError('open denied') + + issue_audit_state.os.open = _denied_open + try: + _t0 = _time1040.monotonic() + try: + with issue_audit_state._StateSection('s', root=_R, acquire_window_s=30, + stale_after_s=1): + pass + assert_eq("#1040 acquire_oserror_fails_fast: raises StateError", "raised", + "no exception") + except issue_audit_state.StateError as _e: + assert_eq("#1040 acquire_oserror_fails_fast: could-not-persist breadcrumb " + "naming the sentinel + error", True, + str(_e).startswith('could not persist state to ') + and 'open denied' in str(_e)) + assert_eq("#1040 acquire_oserror_fails_fast: fails FAST (well under the 30s window)", + True, (_time1040.monotonic() - _t0) < 5) + finally: + issue_audit_state.os.open = _orig_open + +# release_does_not_unlink_a_foreign_sentinel: when the live sentinel's identity no longer +# matches the token this section recorded at acquire (an age-break by another process +# replaced it), release leaves the file in place and breadcrumbs. Driven by forcing the +# recorded ownership token to a value the live stat cannot match — deterministic, unlike an +# unlink+recreate whose freed inode a filesystem may immediately reuse. +with tempfile.TemporaryDirectory() as _td: + _R = Path(_td) + _sent = _ias1040_sentinel(_R) + _sec = issue_audit_state._StateSection('s', root=_R) + _sec.__enter__() + _sec._token = (-1, -1) # the file at _sent is now "not ours" + _, _err = _ias1040_capture_stderr(lambda: _sec.__exit__(None, None, None)) + assert_eq("#1040 release_does_not_unlink_a_foreign_sentinel: the foreign file is left " + "in place", True, os.path.exists(_sent)) + assert_eq("#1040 release_does_not_unlink_a_foreign_sentinel: breadcrumb reports the " + "broken-by-another-process case", True, 'broken by another process' in _err) + os.unlink(_sent) + +# unlink_failure_never_replaces_the_real_exception: os.unlink raising at the release site +# while the handler raises leaves the handler's exception intact and breadcrumbs beside it. +with tempfile.TemporaryDirectory() as _td: + _R = Path(_td) + _orig_unlink = issue_audit_state.os.unlink + + def _denied_unlink(*a, **k): + raise PermissionError('unlink denied') + + _raised = {'kind': None} + _err_buf = io.StringIO() + _sec = issue_audit_state._StateSection('s', root=_R) + _sec.__enter__() + issue_audit_state.os.unlink = _denied_unlink + try: + with contextlib.redirect_stderr(_err_buf): + _sec.__exit__(ValueError, ValueError('the real one'), None) + except Exception as _e: # noqa: BLE001 + _raised['kind'] = type(_e).__name__ + finally: + issue_audit_state.os.unlink = _orig_unlink + assert_eq("#1040 unlink_failure_never_replaces_the_real_exception: __exit__ does not " + "raise the unlink failure (returns falsy, the real exception propagates from " + "the with-block)", None, _raised['kind']) + assert_eq("#1040 unlink_failure_never_replaces_the_real_exception: the release failure " + "is breadcrumbed", True, + 'could not unlink the audit-state sentinel' in _err_buf.getvalue()) + if os.path.exists(_ias1040_sentinel(_R)): + _orig_unlink(_ias1040_sentinel(_R)) + +# stale_break_rechecks_mtime: when os.stat reports a CHANGED mtime between judging staleness +# and unlinking, _break_if_stale performs no unlink and returns False (→ ordinary retry). +with tempfile.TemporaryDirectory() as _td: + _R = Path(_td) + (_R / '.prflow' / 'tmp').mkdir(parents=True) + _sent = _ias1040_sentinel(_R) + with open(_sent, 'w') as _fh: + _fh.write('5555') + _sec = issue_audit_state._StateSection('s', root=_R, acquire_window_s=2, stale_after_s=1) + _orig_stat = issue_audit_state.os.stat + _stat_calls = {'n': 0} + + class _FakeStat: + def __init__(self, mtime): + self.st_mtime = mtime + + def _shifting_stat(path, *a, **k): + if str(path) == _sent: + _stat_calls['n'] += 1 + # First stat: old (stale) mtime; second stat: a DIFFERENT mtime (a live touch). + return _FakeStat(1000.0 if _stat_calls['n'] == 1 else 2000.0) + return _orig_stat(path, *a, **k) + + issue_audit_state.os.stat = _shifting_stat + try: + _broke = _sec._break_if_stale() + finally: + issue_audit_state.os.stat = _orig_stat + assert_eq("#1040 stale_break_rechecks_mtime: a changed mtime aborts the break", False, + _broke) + assert_eq("#1040 stale_break_rechecks_mtime: the sentinel is left in place", True, + os.path.exists(_sent)) + _orig_unlink = issue_audit_state.os.unlink + _orig_unlink(_sent) + +# malformed_sentinel_pid: the five unreadable shapes all render 'unestablished'; a decimal +# pid with a trailing newline renders as the pid (the complement control). +with tempfile.TemporaryDirectory() as _td: + _R = Path(_td) + _p = Path(_td) / 'sent.lock' + assert_eq("#1040 malformed_sentinel_pid: file-read failure (absent) → unestablished", + 'unestablished', issue_audit_state._read_sentinel_pid(str(_p))) + _p.write_bytes(b'') + assert_eq("#1040 malformed_sentinel_pid: empty file → unestablished", + 'unestablished', issue_audit_state._read_sentinel_pid(str(_p))) + _p.write_bytes(b' \n\t ') + assert_eq("#1040 malformed_sentinel_pid: whitespace-only → unestablished", + 'unestablished', issue_audit_state._read_sentinel_pid(str(_p))) + _p.write_bytes(b'not-a-number') + assert_eq("#1040 malformed_sentinel_pid: non-decimal text → unestablished", + 'unestablished', issue_audit_state._read_sentinel_pid(str(_p))) + _p.write_bytes(b'1' * 65) + assert_eq("#1040 malformed_sentinel_pid: content exceeding 64 bytes → unestablished", + 'unestablished', issue_audit_state._read_sentinel_pid(str(_p))) + _p.write_bytes(b'12345\n') + assert_eq("#1040 malformed_sentinel_pid (control): a decimal pid with a trailing newline " + "renders as the pid", '12345', issue_audit_state._read_sentinel_pid(str(_p))) + +# bounds_override_ignores_unusable_values: absent/empty/non-numeric/zero/negative all yield +# the default; a positive value overrides. +_ENV = 'DEVFLOW_IAS_TEST_1040' +os.environ.pop(_ENV, None) +assert_eq("#1040 bounds_override_ignores_unusable_values: absent → default", 45.0, + issue_audit_state._positive_env_float(_ENV, 45.0)) +for _bad in ('', ' ', 'abc', '0', '-3', '0.0'): + os.environ[_ENV] = _bad + assert_eq(f"#1040 bounds_override_ignores_unusable_values: {_bad!r} → default", 45.0, + issue_audit_state._positive_env_float(_ENV, 45.0)) +os.environ[_ENV] = '0.05' +assert_eq("#1040 bounds_override_ignores_unusable_values: a positive value overrides", + 0.05, issue_audit_state._positive_env_float(_ENV, 45.0)) +os.environ.pop(_ENV, None) + +# _StateSection reads its two bounds from the test-only env vars (the mechanism the shell +# tests drive the process boundary with). +os.environ['DEVFLOW_IAS_ACQUIRE_WINDOW_S'] = '0.30' +os.environ['DEVFLOW_IAS_STALE_AFTER_S'] = '0.10' +try: + _sec = issue_audit_state._StateSection('s', root=Path('/tmp')) + assert_eq("#1040 env-override: acquire window read from DEVFLOW_IAS_ACQUIRE_WINDOW_S", + 0.30, _sec._acquire_window_s) + assert_eq("#1040 env-override: stale-after read from DEVFLOW_IAS_STALE_AFTER_S", + 0.10, _sec._stale_after_s) +finally: + os.environ.pop('DEVFLOW_IAS_ACQUIRE_WINDOW_S', None) + os.environ.pop('DEVFLOW_IAS_STALE_AFTER_S', None) + +# ── stdin-hoist behavior (issue #1040) ───────────────────────────────────────────── +# _selects_stdin mirrors each handler's own arg-based read trigger. +def _ns(**kw): + return argparse.Namespace(**kw) + + +assert_eq("#1040 _selects_stdin: record-dispatch inline arm reads stdin (even with a " + "--draft-file argument present)", True, + issue_audit_state._selects_stdin(_ns(cmd='record-dispatch', arm='inline', + draft_file='/x'))) +assert_eq("#1040 _selects_stdin: record-dispatch file arm reads no stdin", False, + issue_audit_state._selects_stdin(_ns(cmd='record-dispatch', arm='file', + draft_file='/x'))) +assert_eq("#1040 _selects_stdin: record-creation-attestation reads unless --unavailable", + True, issue_audit_state._selects_stdin(_ns(cmd='record-creation-attestation', + attestation_unavailable=False))) +assert_eq("#1040 _selects_stdin: --attestation-unavailable reads no stdin", False, + issue_audit_state._selects_stdin(_ns(cmd='record-creation-attestation', + attestation_unavailable=True))) +assert_eq("#1040 _selects_stdin: record-revision reads only with --stdin-digest", True, + issue_audit_state._selects_stdin(_ns(cmd='record-revision', stdin_digest=True))) +assert_eq("#1040 _selects_stdin: check-claim-staleness reads only with --domain-stdin", + True, issue_audit_state._selects_stdin(_ns(cmd='check-claim-staleness', + domain_stdin=True))) +assert_eq("#1040 _selects_stdin: an unrelated command selects no stdin", False, + issue_audit_state._selects_stdin(_ns(cmd='query-summary'))) + +# stdin_hoist_preserves_the_absent_stdin_guard: with sys.stdin None and a stdin-selecting +# command, _read_stdin_once records the closed-fd condition and _stdin_bytes_or_fail raises +# the SAME named breadcrumb (a SystemExit via _fail) — never an AttributeError traceback. +_orig_stdin = sys.stdin +try: + sys.stdin = None + _a = _ns(cmd='record-dispatch', arm='inline', draft_file=None) + issue_audit_state._read_stdin_once(_a) + assert_eq("#1040 stdin_hoist_preserves_the_absent_stdin_guard: closed fd 0 is recorded", + True, _a._stdin_missing) + _sysexit = {'raised': False, 'msg': ''} + _errbuf = io.StringIO() + try: + with contextlib.redirect_stderr(_errbuf): + issue_audit_state._stdin_bytes_or_fail(_a, 'record-dispatch', 'draft bytes') + except SystemExit: + _sysexit['raised'] = True + _sysexit['msg'] = _errbuf.getvalue() + assert_eq("#1040 stdin_hoist_preserves_the_absent_stdin_guard: _fail (SystemExit) fires", + True, _sysexit['raised']) + assert_eq("#1040 stdin_hoist_preserves_the_absent_stdin_guard: the named breadcrumb is " + "preserved verbatim", True, + 'could not read draft bytes from stdin: no stdin is attached (fd 0 is closed)' + in _sysexit['msg']) +finally: + sys.stdin = _orig_stdin + +# stdin_is_read_before_acquire: main() reads stdin BEFORE entering the section. Proven at +# the source level (an executable-order pin): _read_stdin_once precedes the _StateSection +# `with` in main(). +_ias1040_main_src = inspect.getsource(issue_audit_state.main) +assert_eq("#1040 stdin_is_read_before_acquire: main() calls _read_stdin_once before the " + "_StateSection with-block", True, + _ias1040_main_src.index('_read_stdin_once(args)') + < _ias1040_main_src.index('_StateSection(args.slug)')) + +# readers_are_not_serialized: the read-only predicate selects every query-* plus emit-body +# and check-claim-staleness, and NO record-* / init. +for _ro in ('query-summary', 'query-arm', 'emit-body', 'check-claim-staleness'): + assert_eq(f"#1040 readers_are_not_serialized: {_ro} is read-only (no section)", True, + issue_audit_state._is_read_only(_ro)) +for _mut in ('init', 'record-dispatch', 'record-return', 'record-claim-baseline', + 'write-dispatch-scope'): + assert_eq(f"#1040 readers_are_not_serialized: {_mut} is NOT read-only (takes section)", + False, issue_audit_state._is_read_only(_mut)) + +# stdlib_only_imports: the module imports only the standard library, and neither fcntl nor +# msvcrt. +_ias1040_tree = ast.parse((SCRIPTS / 'issue-audit-state.py').read_text(encoding='utf-8')) +_ias1040_imports = set() +for _node in ast.walk(_ias1040_tree): + if isinstance(_node, ast.Import): + for _al in _node.names: + _ias1040_imports.add(_al.name.split('.')[0]) + elif isinstance(_node, ast.ImportFrom) and _node.module and _node.level == 0: + _ias1040_imports.add(_node.module.split('.')[0]) +assert_eq("#1040 stdlib_only_imports: fcntl is not imported", False, + 'fcntl' in _ias1040_imports) +assert_eq("#1040 stdlib_only_imports: msvcrt is not imported", False, + 'msvcrt' in _ias1040_imports) +_ias1040_nonstdlib = sorted(m for m in _ias1040_imports + if m not in sys.stdlib_module_names) +assert_eq("#1040 stdlib_only_imports: no third-party imports", [], _ias1040_nonstdlib) + +# readonly_complement_fails_closed + readonly_predicate_fails_closed_on_unresolvable_calls: +# drive lib/test/check-audit-lifecycle-contracts.py's new check against crafted fixture +# modules — a query- handler reaching save_state directly, through a nested helper, through +# an indirect getattr alias, and through a module-level producer table — and assert each is +# reported (a Refusal), never clean; plus a clean control. +_alc1040 = _load('_alc1040', Path(__file__).resolve().parents[2] / 'lib' / 'test' + / 'check-audit-lifecycle-contracts.py') +_ALC_COMMON = ''' +import argparse + + +class StateError(Exception): + pass + + +def save_state(doc, slug, root=None): + return None + + +def _query_state(slug): + return {} + + +def _is_read_only(cmd): + return cmd.startswith("query-") or cmd in ("emit-body", "check-claim-staleness") + + +def registered_subcommands(): + for action in build_parser()._actions: + if isinstance(action, argparse._SubParsersAction): + return frozenset(action.choices) + raise AssertionError("no subparsers") +''' +_ALC_FIXTURES = { + 'clean': _ALC_COMMON + ''' +def cmd_query_ok(args): + return _query_state(args.slug) + + +def build_parser(): + p = argparse.ArgumentParser(); sub = p.add_subparsers(dest="cmd", required=True) + s = sub.add_parser("query-ok"); s.add_argument("slug"); s.set_defaults(func=cmd_query_ok) + return p +''', + 'direct': _ALC_COMMON + ''' +def cmd_query_bad(args): + return save_state({}, args.slug) + + +def build_parser(): + p = argparse.ArgumentParser(); sub = p.add_subparsers(dest="cmd", required=True) + s = sub.add_parser("query-bad"); s.add_argument("slug"); s.set_defaults(func=cmd_query_bad) + return p +''', + 'nested': _ALC_COMMON + ''' +def cmd_query_nested(args): + def _helper(): + return save_state({}, args.slug) + return _helper() + + +def build_parser(): + p = argparse.ArgumentParser(); sub = p.add_subparsers(dest="cmd", required=True) + s = sub.add_parser("query-nested"); s.add_argument("slug") + s.set_defaults(func=cmd_query_nested) + return p +''', + 'getattr': _ALC_COMMON + ''' +import sys as _sys + + +def cmd_query_indirect(args): + fn = getattr(_sys.modules[__name__], "save_state") + return fn({}, args.slug) + + +def build_parser(): + p = argparse.ArgumentParser(); sub = p.add_subparsers(dest="cmd", required=True) + s = sub.add_parser("query-indirect"); s.add_argument("slug") + s.set_defaults(func=cmd_query_indirect) + return p +''', + 'table': _ALC_COMMON + ''' +def _producer(s): + return save_state({}, s) + + +_TABLE = (("a", lambda s: _producer(s)),) + + +def cmd_query_table(args): + for _, produce in _TABLE: + produce(args.slug) + + +def build_parser(): + p = argparse.ArgumentParser(); sub = p.add_subparsers(dest="cmd", required=True) + s = sub.add_parser("query-table"); s.add_argument("slug") + s.set_defaults(func=cmd_query_table) + return p +''', +} + + +def _alc1040_run(name, source): + with tempfile.TemporaryDirectory() as _td: + _fp = Path(_td) / f'fx_{name}.py' + _fp.write_text(source, encoding='utf-8') + _spec = importlib.util.spec_from_file_location(f'_alcfx_{name}', _fp) + _mod = importlib.util.module_from_spec(_spec) + sys.modules[_spec.name] = _mod + _spec.loader.exec_module(_mod) + try: + _alc1040.check_readonly_complement(_mod, _mod.registered_subcommands(), []) + return None + except _alc1040.Refusal as _r: + return str(_r) + + +assert_eq("#1040 readonly_complement (clean control): a read-only handler that never " + "reaches save_state is reported clean", None, _alc1040_run('clean', + _ALC_FIXTURES['clean'])) +assert_eq("#1040 readonly_complement_fails_closed: a query- handler calling save_state " + "directly is reported (Refusal), never clean", True, + _alc1040_run('direct', _ALC_FIXTURES['direct']) is not None) +assert_eq("#1040 readonly_predicate_fails_closed_on_unresolvable_calls (nested helper): " + "reported, never clean", True, + _alc1040_run('nested', _ALC_FIXTURES['nested']) is not None) +assert_eq("#1040 readonly_predicate_fails_closed_on_unresolvable_calls (getattr alias): " + "reported, never clean", True, + _alc1040_run('getattr', _ALC_FIXTURES['getattr']) is not None) +assert_eq("#1040 readonly_predicate_fails_closed_on_unresolvable_calls (producer table): " + "reported, never clean", True, + _alc1040_run('table', _ALC_FIXTURES['table']) is not None) + # (2) The attestation trailing-newline tolerance swallows a _DigestError raised by # the SECOND (newline-stripped) hash: the compare stays a well-defined mismatch — # never an unhandled exception, which would leave the run with no attestation record @@ -6993,9 +7604,13 @@ def _hash_stub(data): try: with contextlib.redirect_stdout(_att_out), \ contextlib.redirect_stderr(io.StringIO()): - issue_audit_state.cmd_record_creation_attestation( - argparse.Namespace(slug='s', nonce='n0', - attestation_unavailable=False)) + # Mirror main(): the stdin read is hoisted (issue #1040), so drive + # _read_stdin_once before the handler to populate args._stdin_*. + _att_args = argparse.Namespace( + cmd='record-creation-attestation', slug='s', nonce='n0', + attestation_unavailable=False) + issue_audit_state._read_stdin_once(_att_args) + issue_audit_state.cmd_record_creation_attestation(_att_args) _att_ended = 'returned' except SystemExit as _e: _att_ended = f'SystemExit({_e.code})' @@ -13116,7 +13731,12 @@ def read(): sys.stdin = None if _stdin20 is None else _Stdin20() try: with contextlib.redirect_stderr(_err20): - issue_audit_state._ingest_ledger(1, 1) + # Mirror main(): the raw stdin read is hoisted (issue #1040) into + # _read_stdin_once, and _ingest_ledger consumes the buffer. Drive both, as + # main() does, so the closed-fd / read-error breadcrumb is still exercised. + _args20 = argparse.Namespace(cmd='record-adjudication', ledger_stdin=True) + issue_audit_state._read_stdin_once(_args20) + issue_audit_state._ingest_ledger(_args20, 1, 1) _rc20 = 'no exit' except SystemExit as _exc20: _rc20 = _exc20.code diff --git a/scripts/issue-audit-state.py b/scripts/issue-audit-state.py index 1d0e43521..783d0555a 100644 --- a/scripts/issue-audit-state.py +++ b/scripts/issue-audit-state.py @@ -73,6 +73,8 @@ import shlex import subprocess import sys +import tempfile +import time from pathlib import Path if sys.version_info < (3, 11): @@ -1043,6 +1045,230 @@ def _is_bound_path(p): and '\n' not in p and '\r' not in p) +# ── Issue #1040: write serialization via an exclusive-create sentinel ────────────── +# Two concurrent invocations for the same slug in one checkout must produce a state +# document reflecting one of them entirely and then the other entirely, never a mixture. +# The mechanism is an `os.open(O_CREAT|O_EXCL)` sentinel beside the state file (the +# single-owner pattern scripts/verification-flight.py already uses) plus a per-writer +# `tempfile.mkstemp` temp path in save_state. Read-only subcommands take no sentinel. +# Every failure the section raises is phrased as a `could not persist state to :` +# StateError, so it lands in the existing cannot-persist-state routing class rather than +# opening a fourth mutation-exit destination. + +# Test-only overrides so the shell-level tests drive the process boundary in +# milliseconds. NOT CLI flags and NOT read from .prflow/config.json — the shipped path has +# exactly one decided setting. The DEVFLOW_ prefix is the DECIDED choice (issue #1040): +# CLAUDE.md freezes that namespace pending the #1004 Tier-3 rename, so a PRFLOW_ spelling +# would be the one variable that ticket's sweep would miss. Both names are recorded in the +# #1040 changeset as members #1004 must migrate. +_IAS_ACQUIRE_WINDOW_ENV = 'DEVFLOW_IAS_ACQUIRE_WINDOW_S' +_IAS_STALE_AFTER_ENV = 'DEVFLOW_IAS_STALE_AFTER_S' + + +def _positive_env_float(name, default): + """The override in `name` when it holds a usable positive number, else `default`. + + A value that is absent, empty, non-numeric, or non-positive alike is ignored and the + shipped default applies — the closed set of rejected shapes stated by the acceptance + criteria. + """ + raw = os.environ.get(name) + if raw is None: + return default + raw = raw.strip() + if not raw: + return default + try: + val = float(raw) + except ValueError: + return default + return val if val > 0 else default + + +def _read_sentinel_pid(sentinel): + """The pid recorded in the sentinel as a decimal string, or the literal + `unestablished` for any of the five unreadable shapes — file-read failure, empty file, + whitespace-only content, non-decimal text, content exceeding 64 bytes. Staleness is + decided by mtime alone, so an unestablished pid never changes the acquire/refuse/break + decision; it only shapes the breadcrumb. A trailing newline is stripped, so a pid + written with one renders as the pid. + """ + try: + with open(sentinel, 'rb') as fh: + data = fh.read(65) # 65 so a body exceeding 64 bytes is detectable + except OSError: + return 'unestablished' + if len(data) > 64: + return 'unestablished' + text = data.decode('utf-8', 'replace').strip() + if not re.fullmatch(r'[0-9]+', text): + return 'unestablished' + return text + + +def _replace_with_retry(src, dst, *, attempts=5, delay=0.02): + """`os.replace(src, dst)` with a bounded retry over `PermissionError` only. + + On Windows a `MoveFileEx`-backed replace onto a path a lock-free reader currently has + open raises `PermissionError`; retry it briefly. Every OTHER `OSError` propagates on + the first attempt, unchanged, so the existing `could not persist state to ` breadcrumb + and its test row keep their shape. Exhausting the retries re-raises the last + `PermissionError` for the caller to route. + """ + for i in range(attempts): + try: + os.replace(src, dst) + return + except PermissionError: + if i == attempts - 1: + raise + time.sleep(delay) + + +class _StateSection: + """Serialize mutating state writes for one slug via an O_CREAT|O_EXCL sentinel. + + Entered around main()'s single dispatch site for every non-read-only subcommand, so a + handler's `load_state` .. `save_state` runs under exclusion and the second writer's + read happens after the first writer's write. No compare-and-swap token is needed + because the load sits inside the section. + """ + + def __init__(self, slug, root=None, *, acquire_window_s=45, stale_after_s=30): + # Compose the sentinel FROM the resolved state path (string-concatenate '.lock', + # never Path.with_suffix — a slug may itself contain a dot), so a run whose git + # resolution degraded still locks the file it actually writes. + self._state_path = state_path(slug, root) + self._sentinel = str(self._state_path) + '.lock' + self._parent = os.path.dirname(self._sentinel) + self._acquire_window_s = _positive_env_float( + _IAS_ACQUIRE_WINDOW_ENV, acquire_window_s) + self._stale_after_s = _positive_env_float(_IAS_STALE_AFTER_ENV, stale_after_s) + self._token = None # (st_dev, st_ino) recorded at acquire + + def _persist_error(self, detail): + return StateError(f'could not persist state to {self._state_path}: {detail}') + + def _try_create(self): + """Attempt the exclusive create once. True on success (ownership token recorded), + False on contention (FileExistsError) or a missing parent. A read-only filesystem + or permission denial raises a cannot-persist StateError immediately, since + retrying a condition that does not clear only converts a named failure into a + stall. + """ + try: + fd = os.open(self._sentinel, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600) + except FileExistsError: + return False + except FileNotFoundError: + # A missing parent — not the immediate-raise class. Recreate and let the + # bounded loop retry (should not recur, since __enter__ mkdir'd first). + os.makedirs(self._parent, exist_ok=True) + return False + except OSError as exc: + raise self._persist_error( + f'could not create the audit-state section sentinel ' + f'{self._sentinel}: {exc}') from exc + try: + st = os.fstat(fd) + self._token = (st.st_dev, st.st_ino) + os.write(fd, str(os.getpid()).encode('ascii')) + finally: + os.close(fd) + return True + + def _break_if_stale(self): + """When the held sentinel's mtime age exceeds stale_after_s, re-stat it and unlink + it only while the observed mtime is unchanged from the one judged stale, then + re-attempt the exclusive create EXACTLY ONCE. True iff the break-and-recreate + acquired the section. A changed mtime, a vanished sentinel, or a losing re-create + each return False → the ordinary retry loop. + """ + try: + first = os.stat(self._sentinel) + except OSError: + return False # vanished/unstattable — a create will win next iteration + age = time.time() - first.st_mtime + if age <= self._stale_after_s: + return False + pid = _read_sentinel_pid(self._sentinel) + try: + second = os.stat(self._sentinel) + except OSError: + return False + if second.st_mtime != first.st_mtime: + return False # a live holder touched it between judging and unlinking + try: + os.unlink(self._sentinel) + except OSError as exc: + # Both unlink sites catch every OSError (a directory planted at the path, a + # permission-denied parent, a Windows sharing violation), not only + # FileNotFoundError. A failing break unlink returns the mutation to its + # ordinary retry loop. + sys.stderr.write( + f'issue-audit-state.py: could not break the stale audit-state sentinel ' + f'{self._sentinel} (pid {pid}, age {age:.0f}s): {exc}\n') + return False + sys.stderr.write( + f'issue-audit-state.py: broke a stale audit-state sentinel {self._sentinel} ' + f'(pid {pid}, age {age:.0f}s) and proceeded\n') + return self._try_create() + + def __enter__(self): + # Create the parent directory BEFORE the first exclusive-create so a fresh clone, + # a fresh adopter checkout, and a bare test sandbox — none of which carry the + # ignored state tmp directory — acquire instead of raising on FileNotFoundError. + os.makedirs(self._parent, exist_ok=True) + deadline = time.monotonic() + self._acquire_window_s + while True: + if self._try_create(): + return self + if self._break_if_stale(): + return self + if time.monotonic() >= deadline: + # Under the shipped bound relation (window > stale) an abandoned sentinel is + # always broken strictly inside the window, so this arm is unreachable; it + # is the fail-closed arm for a host whose overrides invert the relation. The + # state file is left byte-identical (only the sentinel was ever touched). + pid = _read_sentinel_pid(self._sentinel) + raise self._persist_error( + f'the audit-state section sentinel {self._sentinel} is held by pid ' + f'{pid} and was not released within {self._acquire_window_s:g}s') + time.sleep(0.02) + + def __exit__(self, exc_type, exc, tb): + # Ownership-checked release on EVERY exit path — the mutation succeeding, + # save_state raising, and the handler raising. Best-effort and total: 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 it. Returning False never suppresses that exception. + try: + st = os.stat(self._sentinel) + except FileNotFoundError: + return False # already gone (age-broken by another process) — clean exit + except OSError as exc2: + sys.stderr.write( + f'issue-audit-state.py: could not stat the audit-state sentinel ' + f'{self._sentinel} on release: {exc2}\n') + return False + if self._token is not None and (st.st_dev, st.st_ino) == self._token: + try: + os.unlink(self._sentinel) + except OSError as exc2: + sys.stderr.write( + f'issue-audit-state.py: could not unlink the audit-state sentinel ' + f'{self._sentinel} on release: {exc2}\n') + else: + # After an age break the file here belongs to the breaker; unlinking it by + # path would strip a live holder's exclusion. Leave it and breadcrumb that + # this section's own sentinel was broken by another process. + sys.stderr.write( + f"issue-audit-state.py: this section's own audit-state sentinel " + f'{self._sentinel} was broken by another process; leaving the current ' + f'file in place\n') + return False + + # ── Digests ──────────────────────────────────────────────────────────────────── class _DigestError(Exception): @@ -2414,18 +2640,34 @@ def save_state(doc, slug, root=None): # invalid document fails HERE, loudly, instead of persisting silently and # collapsing the whole file to unestablished at the next load. _validate(doc, slug) - tmp = path.with_suffix('.json.tmp') try: path.parent.mkdir(parents=True, exist_ok=True) - tmp.write_text(json.dumps(doc, indent=2, sort_keys=True) + '\n', encoding='utf-8') - os.replace(tmp, path) - except OSError as exc: - # Best-effort cleanup of a partial temp file so a failed persist never leaves - # a stray .json.tmp in the evidence-bearing tmp directory. + # Per-writer temp path (issue #1040): tempfile.mkstemp gives each writer a UNIQUE + # temp name in the state file's own directory, so two concurrent writers never + # share and truncate one deterministic path. The '.json.tmp' suffix is retained so + # the existing #546 cleanup glob('*.json.tmp') still selects it. mkstemp sits inside + # this try and below the mkdir: unlike the pure path computation it replaces it + # touches the filesystem, so every OSError it raises (a missing parent, a read-only + # filesystem, a permission denial, an exhausted disk) surfaces as the same + # could-not-persist StateError below. mkstemp creates at 0600 and os.replace carries + # that mode onto the state file — the decided per-user-artifact mode on POSIX. + fd, tmp = tempfile.mkstemp(dir=str(path.parent), prefix=path.name + '.', + suffix='.json.tmp') try: - tmp.unlink() + with os.fdopen(fd, 'w', encoding='utf-8') as fh: + fh.write(json.dumps(doc, indent=2, sort_keys=True) + '\n') + # os.replace retried over PermissionError only (the Windows lock-free-reader + # sharing violation); every other OSError propagates on the first attempt. + _replace_with_retry(tmp, path) except OSError: - pass + # Best-effort cleanup of the partial temp file so a failed persist never leaves + # a stray .json.tmp in the evidence-bearing tmp directory. + try: + os.unlink(tmp) + except OSError: + pass + raise + except OSError as exc: raise StateError(f'could not persist state to {path}: {exc}') from exc return path @@ -5229,25 +5471,12 @@ def cmd_record_dispatch(args): except OSError as exc: _fail('record-dispatch', f'could not read draft file {args.draft_file}: {exc}') else: - # The sibling draft-file read above routes its failure through _fail; stdin is the - # same external input and gets the same treatment, or a broken fd 0 escapes as a - # raw traceback — breaking the mutation contract (non-zero WITH a named - # breadcrumb) the caller parses, and handing its stderr classification a Python - # traceback rather than one of this tool's own vocabulary strings. - # - # TWO distinct failures, deliberately handled separately. A CLOSED fd 0 does not - # raise from the read at all: CPython sets `sys.stdin = None` at startup, so the - # attribute access itself is what fails (AttributeError, never OSError) — an - # `except OSError` around the read is blind to exactly the shape the skill's shell - # pipelines can produce. Test the object first; keep the except for a genuine - # read-time error (an I/O failure part-way through a redirected file). - if sys.stdin is None: - _fail('record-dispatch', 'could not read draft bytes from stdin: no stdin is ' - 'attached (fd 0 is closed)') - try: - data = sys.stdin.buffer.read() - except OSError as exc: - _fail('record-dispatch', f'could not read draft bytes from stdin: {exc}') + # The draft bytes are read from stdin, hoisted into main() above the section (issue + # #1040) so this mutating handler performs no blocking sys.stdin read inside the + # section. `_stdin_bytes_or_fail` reproduces the former in-handler guards verbatim: + # a closed fd 0 and a read-time OSError each route through _fail with the same named + # breadcrumb (never a raw traceback that would break the mutation contract). + data = _stdin_bytes_or_fail(args, 'record-dispatch', 'draft bytes') if not data: _fail('record-dispatch', f'the {args.arm} arm requires the draft bytes on ' 'stdin; received none') @@ -5886,7 +6115,7 @@ def cmd_record_adjudication(args): '--ledger-stdin is only accepted on a REVISE adjudication with a ' 'settled unresolved count (ledger-not-applicable); a FILE verdict and ' f'a REVISE + {_UNESTABLISHED!r} adjudication record no ledger') - ledger = _ingest_ledger(args.must_revise, unresolved) + ledger = _ingest_ledger(args, args.must_revise, unresolved) elif ledger_shape: _fail('record-adjudication', f'a REVISE adjudication with a settled unresolved count requires ' @@ -5965,13 +6194,14 @@ def cmd_record_adjudication(args): f'invalid={args.invalid} superseded={superseded}') -def _read_stdin_lines(command, what, token): - """Read a quoted-heredoc line payload from stdin, or fail closed (issue #708). +def _read_stdin_lines(args, command, what, token): + """Decode a quoted-heredoc line payload from the hoisted stdin buffer, or fail closed + (issue #708). The raw byte read is hoisted into main() above the section (issue #1040) + and consumed here via `_stdin_bytes_or_fail`, which reproduces the closed-fd and + read-error breadcrumbs verbatim; the undecodable and empty arms stay here. - ONE implementation of the fail-closed byte-read the line-oriented stdin transports - share — a closed fd (CPython sets `sys.stdin` to None, so an attribute access would - otherwise leak a raw traceback), a read error, an undecodable payload, and an empty - one. Callers supply their own `command` (for the breadcrumb prefix), the human `what` + ONE implementation of the fail-closed decode the line-oriented stdin transports share. + Callers supply their own `command` (for the breadcrumb prefix), the human `what` they are reading, and the `token` their triage vocabulary uses, so every named breadcrumb stays exactly what it was when each caller inlined this block. @@ -5989,13 +6219,7 @@ def _read_stdin_lines(command, what, token): Returns the non-blank lines. Never returns on any degraded shape. """ - if sys.stdin is None: - _fail(command, f'could not read the {what} from stdin: no stdin is attached ' - f'(fd 0 is closed)') - try: - data = sys.stdin.buffer.read() - except OSError as exc: - _fail(command, f'could not read the {what} from stdin: {exc}') + data = _stdin_bytes_or_fail(args, command, f'the {what}') try: raw = data.decode('utf-8') except UnicodeDecodeError as exc: @@ -6007,7 +6231,7 @@ def _read_stdin_lines(command, what, token): return [ln for ln in raw.split('\n') if ln.strip()] -def _ingest_ledger(must_revise, unresolved): +def _ingest_ledger(args, must_revise, unresolved): """Read `--ledger-stdin` and build the round's ledger, or fail closed. The transport is deliberately line-oriented text, not a structured payload: the @@ -6025,7 +6249,7 @@ def _ingest_ledger(must_revise, unresolved): command's own: record-revision hashes the bytes and never decodes them, so it has no decode step to mirror. """ - lines = _read_stdin_lines('record-adjudication', 'finding ledger', 'ledger') + lines = _read_stdin_lines(args, 'record-adjudication', 'finding ledger', 'ledger') if len(lines) != must_revise: _fail('record-adjudication', f'the ledger carries {len(lines)} finding summaries but the adjudication ' @@ -6298,7 +6522,7 @@ def cmd_record_coverage(args): _fail('record-coverage', '--expected-keys repeats a dimension key ' '(coverage-expected-duplicate); the enumeration is keyed ' 'and its keys are unique by construction') - coverage = _ingest_coverage(expected) + coverage = _ingest_coverage(args, expected) rnd['coverage'] = coverage # Persist the enumeration totality was checked against. The state owner cannot # re-derive it (it holds no template), so `--expected-keys` is an orchestrator-supplied @@ -6321,7 +6545,7 @@ def cmd_record_coverage(args): f'backs_run={backs}') -def _ingest_coverage(expected_keys): +def _ingest_coverage(args, expected_keys): """Read `--coverage-stdin` and build the round's coverage list, or fail closed. One line per required dimension: `` [anchor text...]`` — the key and @@ -6333,7 +6557,7 @@ def _ingest_coverage(expected_keys): DOWNGRADED to `unestablished` with its anchor dropped — never rejected (unknown is not zero, and the coverage record must stay total over required dimensions). """ - lines = _read_stdin_lines('record-coverage', 'coverage list', 'coverage') + lines = _read_stdin_lines(args, 'record-coverage', 'coverage list', 'coverage') coverage = [] seen = set() for idx, line in enumerate(lines, start=1): @@ -6992,13 +7216,9 @@ def cmd_record_revision(args): # failed cannot masquerade as audited bytes. stdin_digest = None if getattr(args, 'stdin_digest', False): - if sys.stdin is None: - _fail('record-revision', 'could not read revised bytes from stdin: no stdin ' - 'is attached (fd 0 is closed)') - try: - data = sys.stdin.buffer.read() - except OSError as exc: - _fail('record-revision', f'could not read revised bytes from stdin: {exc}') + # Revised bytes read from stdin, hoisted into main() above the section (issue + # #1040); `_stdin_bytes_or_fail` reproduces the closed-fd and read-error breadcrumbs. + data = _stdin_bytes_or_fail(args, 'record-revision', 'revised bytes') if not data: _fail('record-revision', '--stdin-digest was given but no revised bytes were ' 'received on stdin') @@ -7609,22 +7829,11 @@ def cmd_record_creation_attestation(args): if args.attestation_unavailable: status = 'attestation-unavailable' else: - # Fail with the named breadcrumb rather than a raw traceback: this command IS the - # tamper-detection surface, so a crash here would leave the run with no - # attestation record at all — rendering `attestation=none` ("before any creation - # attempt"), the never-attempted misattribution that `attestation-unavailable` - # exists to prevent. A closed fd 0 fails at the ATTRIBUTE access (CPython sets - # `sys.stdin = None`), not from the read, so it is tested separately — an - # `except OSError` alone is blind to it. See record-dispatch's twin. - if sys.stdin is None: - _fail('record-creation-attestation', - 'could not read the fetched body from stdin: no stdin is attached ' - '(fd 0 is closed)') - try: - data = sys.stdin.buffer.read() - except OSError as exc: - _fail('record-creation-attestation', - f'could not read the fetched body from stdin: {exc}') + # The fetched body is read from stdin, hoisted into main() above the section (issue + # #1040); `_stdin_bytes_or_fail` reproduces the named closed-fd and read-error + # breadcrumbs verbatim rather than letting a raw traceback break the mutation + # contract on this tamper-detection surface (see record-dispatch's twin). + data = _stdin_bytes_or_fail(args, 'record-creation-attestation', 'the fetched body') # Empty fetched bytes are COMPARED, not laundered into unavailable: an empty # created body from a successful fetch is exactly the empty-bodied-issue # failure the posting guard exists to catch, and the recorded digest makes the @@ -7697,7 +7906,12 @@ def cmd_record_claim_baseline(args): _fail(prefix, f'domain-class-no-domain: a {args.claim_class} claim is identified ' f'by its re-executed full-domain search result; pipe it with ' f'--domain-stdin') - data = sys.stdin.buffer.read() + # The domain search result was read from stdin, hoisted into main() above the + # section (issue #1040). This site never carried the fd-0-closed guard, so a closed + # fd 0 leaves args._stdin_data None and falls through to the domain-class-empty-domain + # refusal below (a clean named breadcrumb where an in-handler bare read would have + # crashed with an AttributeError) — a deliberate, minor robustness improvement. + data = args._stdin_data if not data: _fail(prefix, 'domain-class-empty-domain: --domain-stdin produced no bytes; a ' 'search that emitted nothing cannot identify a baseline') @@ -7802,7 +8016,12 @@ def cmd_check_claim_staleness(args): print('claims=none') return keys = sorted(claims) - domain = sys.stdin.buffer.read() if args.domain_stdin else None + # The domain search result was read from stdin, hoisted into main() above dispatch + # (issue #1040). check-claim-staleness is read-only (no section), but the read still + # moves to main() so no handler touches sys.stdin. _read_stdin_once reads only when + # --domain-stdin is selected, so args._stdin_data is None otherwise — equivalent to the + # former `... if args.domain_stdin else None`. + domain = args._stdin_data # Memoized across the loop: a path cited by several location claims is hashed once. cache = {} for key in keys: @@ -7851,7 +8070,10 @@ def cmd_record_finding_evidence(args): doc = _load_for_mutation(prefix, args.slug, args.nonce) observed = None if args.observed_stdin: - raw = sys.stdin.buffer.read() + # Read from stdin, hoisted into main() above the section (issue #1040). This site + # never carried the fd-0-closed guard, so a closed fd 0 leaves args._stdin_data None + # and the decode below raises the same way the former bare sys.stdin read did. + raw = args._stdin_data # An empty read is NOT refused: issue #704 requires evidence that is absent or # incomplete to be RECORDED `incomplete` (never verified), which is what # `evidence_completeness` does with an empty `observed`. Refusing would record no @@ -9115,9 +9337,104 @@ def registered_subcommands(): raise AssertionError('issue-audit-state: build_parser() registered no subparsers') +# ── Issue #1040: read-only predicate + hoisted stdin read ───────────────────────── +# The read-only predicate decides which subcommands skip the critical section. It is the +# existing naming rule (a name beginning `query-`) plus the two non-query read surfaces +# already in _NEXT_CALL_EXCLUDED — it introduces no new closed set. Its complement is +# proved fail-closed against handler source by lib/test/check-audit-lifecycle-contracts.py. +_READONLY_EXTRA = frozenset(('emit-body', 'check-claim-staleness')) + + +def _is_read_only(cmd): + """True iff `cmd` acquires no sentinel (issue #1040).""" + return cmd.startswith('query-') or cmd in _READONLY_EXTRA + + +def _selects_stdin(args): + """Whether the parsed args select a stdin payload for this command (issue #1040). + + The read is hoisted to main() above the section, so this must mirror each handler's + OWN arg-based read trigger exactly — a flag for the four flag-gated payloads, and the + arm for record-dispatch (embed/inline draft bytes) and record-creation-attestation + (the fetched body). The scope is larger than the four stdin flags the issue enumerated + (record-dispatch, record-creation-attestation, and record-finding-evidence also read + stdin), and the hoist covers all of them so no handler performs a sys.stdin read. + """ + cmd = getattr(args, 'cmd', None) + if cmd == 'record-dispatch': + # The draft bytes are read from stdin on every arm EXCEPT the file arm (which reads + # `--draft-file`). The gate is the arm, not the presence of --draft-file: an + # embed/inline dispatch may still carry a --draft-file argument yet reads stdin. + return getattr(args, 'arm', None) != 'file' + if cmd == 'record-creation-attestation': + return not getattr(args, 'attestation_unavailable', False) + if cmd == 'record-revision': + return bool(getattr(args, 'stdin_digest', False)) + if cmd == 'record-adjudication': + return bool(getattr(args, 'ledger_stdin', False)) + if cmd == 'record-coverage': + return bool(getattr(args, 'coverage_stdin', False)) + if cmd in ('record-claim-baseline', 'check-claim-staleness'): + return bool(getattr(args, 'domain_stdin', False)) + if cmd == 'record-finding-evidence': + return bool(getattr(args, 'observed_stdin', False)) + return False + + +def _read_stdin_once(args): + """Hoist the single stdin read above main()'s dispatch and the critical section (issue + #1040), so no handler blocks on stdin inside the section and the section's duration is + bounded by one small-document read-modify-write. Records the payload (or the fd-0-closed + / read-error condition) on `args` for the handler to consume; reads nothing when the + parsed args select no payload. The existing per-handler absent-stdin guard moves with + the read (see `_stdin_bytes_or_fail`), so its behavior and breadcrumb are unchanged. + """ + args._stdin_data = None + args._stdin_missing = False + args._stdin_error = None + if not _selects_stdin(args): + return + if sys.stdin is None: + args._stdin_missing = True + return + try: + args._stdin_data = sys.stdin.buffer.read() + except OSError as exc: + args._stdin_error = exc + + +def _stdin_bytes_or_fail(args, command, phrase): + """Return the hoisted stdin bytes, reproducing the guarded sites' fd-0-closed and + read-error breadcrumbs verbatim (issue #1040). `phrase` is the exact wording each site + used after `could not read ` (`draft bytes`, `revised bytes`, `the fetched body`, `the + finding ledger`, `the coverage list`). + """ + if args._stdin_missing: + _fail(command, f'could not read {phrase} from stdin: no stdin is attached ' + f'(fd 0 is closed)') + if args._stdin_error is not None: + _fail(command, f'could not read {phrase} from stdin: {args._stdin_error}') + return args._stdin_data + + def main(): args = build_parser().parse_args() - ctx = args.func(args) + # Hoist stdin ABOVE the section (issue #1040): read any payload the parsed args select + # before dispatch, so a mutating handler's stdin read never blocks inside the section. + _read_stdin_once(args) + if _is_read_only(args.cmd): + # Read-only subcommands acquire no sentinel and are unaffected by one being held. + ctx = args.func(args) + else: + # Wrap the single dispatch site so the handler's load_state..save_state runs under + # exclusion. A section acquisition failure raises a cannot-persist StateError, which + # routes through _fail exactly like every other could-not-persist breadcrumb (no new + # mutation-exit class). __exit__ runs the ownership-checked release on every path. + try: + with _StateSection(args.slug): + ctx = args.func(args) + except StateError as exc: + _fail(args.cmd, str(exc)) # issue #795 — the SINGLE `next_call=` emission site. It runs after the command's own # function returned, so every existing decided line stays byte-identical and first, and # a refusal (which raises `SystemExit` out of `_fail`) never reaches here. From ffdf44c5c82e020f73a459ec9c1be17526c68724 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Sat, 1 Aug 2026 01:49:35 +0000 Subject: [PATCH 2/9] chore: add changeset for issue #1040 (#1045) Co-Authored-By: Claude Opus 4.8 --- .changeset/issue-1040-serialize-audit-state-writes.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/issue-1040-serialize-audit-state-writes.md diff --git a/.changeset/issue-1040-serialize-audit-state-writes.md b/.changeset/issue-1040-serialize-audit-state-writes.md new file mode 100644 index 000000000..7b4544abe --- /dev/null +++ b/.changeset/issue-1040-serialize-audit-state-writes.md @@ -0,0 +1,7 @@ +--- +bump: patch +type: Fixed +--- + +- **Serialize the create-issue audit state owner's writes so concurrent invocations cannot corrupt the record.** `scripts/issue-audit-state.py` now wraps every mutating subcommand in an exclusive-create sentinel critical section (a `.lock` file beside the state document, created with `os.O_CREAT | os.O_EXCL`), so two concurrent invocations for the same slug produce a document reflecting one of them entirely and then the other, never a mixture. `save_state` obtains a unique per-writer temporary path from `tempfile.mkstemp` (retaining the `.json.tmp` suffix) and retries `os.replace` over `PermissionError`, so two writers never share and truncate one temporary file. Read-only subcommands (`query-*`, `emit-body`, `check-claim-staleness`) acquire no sentinel and stay unserialized; an abandoned sentinel is recovered by age. A new fail-closed transitive call-graph check in `lib/test/check-audit-lifecycle-contracts.py` proves `save_state` is unreachable from every read-only-classified subcommand. The mechanism is standard-library only (no `fcntl`/`msvcrt`), adds no state-document field, and produces no new mutation-exit class — every section failure is a `could not persist state to …` condition the shipped routing already carries. The decision channel (`next_call=` / `query-*`) remains unserialized, and its non-authoritative-under-concurrency residual is stated in `docs/DEVFLOW_SYSTEM_OVERVIEW.md` §11. (#1045) +- **#1004 migration note.** This change adds two test-only environment variables in the frozen `DEVFLOW_` namespace — `DEVFLOW_IAS_ACQUIRE_WINDOW_S` and `DEVFLOW_IAS_STALE_AFTER_S` — which the Tier-3 env-var rename (#1004) must migrate alongside the existing `DEVFLOW_*` members. (#1045) From 7366a5133fe34a4e9b7cb37db7ea8e54d71f4f71 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Sat, 1 Aug 2026 02:07:03 +0000 Subject: [PATCH 3/9] fix: address review findings (iteration 1) - Route the section-setup OSErrors (__enter__ makedirs, _try_create fstat/write) through the cannot-persist StateError so no raw traceback escapes the mutation contract (code-reviewer). - Honor args._stdin_error at the two guardless stdin consumers so a mid-read OSError names its real cause instead of being misattributed (silent-failure-hunter). - Correct the inaccurate module-class enumeration in check_readonly_complement's docstring (comment-analyzer). - Add contend-then-acquire-after-release and reader-while-held coverage, plus the missing _selects_stdin arm assertions (pr-test-analyzer). Co-Authored-By: Claude Opus 4.8 --- lib/test/check-audit-lifecycle-contracts.py | 3 +- lib/test/modules/issue-audit-state.sh | 19 +++++++ lib/test/test_python_scripts.py | 47 +++++++++++++++-- scripts/issue-audit-state.py | 57 ++++++++++++++++----- 4 files changed, 108 insertions(+), 18 deletions(-) diff --git a/lib/test/check-audit-lifecycle-contracts.py b/lib/test/check-audit-lifecycle-contracts.py index 139fe08e4..b2812244b 100755 --- a/lib/test/check-audit-lifecycle-contracts.py +++ b/lib/test/check-audit-lifecycle-contracts.py @@ -191,8 +191,7 @@ def _module_level_names(tree): carries is followed (proving whether it reaches save_state) rather than flagged unresolvable. `safe_leaf_names` are names a call may reference and still reach no followable function by name — imported names and module-level class names (a class - instantiation cannot BE the `save_state` function; the two classes in this module — - StateError, _StateSection — reach no save_state). A name collision across scopes + instantiation cannot BE the `save_state` function, e.g. `StateError(...)`). A name collision across scopes over-approximates by keeping one body; that only ever follows MORE, which is the fail-closed direction for an unreachability proof. """ diff --git a/lib/test/modules/issue-audit-state.sh b/lib/test/modules/issue-audit-state.sh index 5970ad27e..631c5ae15 100644 --- a/lib/test/modules/issue-audit-state.sh +++ b/lib/test/modules/issue-audit-state.sh @@ -2237,3 +2237,22 @@ assert_eq "#1040 cli_contention_refusal: the refusal names could-not-persist-sta assert_eq "#1040 cli_contention_refusal: the refused mutation persisted no state file" \ "no" "$(cat "$SL_SB/.sl3-state" 2>/dev/null)" rm -rf "$SL_SB" + +# readers_are_not_serialized (process boundary): a read-only subcommand acquires no +# sentinel, so a query-* invoked WHILE a sentinel is held for the same slug still exits 0 +# (the AC: "a query-* invocation issued while a sentinel is held exits 0 with its decided +# answer line"). Plant a fresh sentinel and confirm query-nonce (read-only) is unaffected. +RO_SB="$(git_sandbox '#1040 readers_are_not_serialized_while_held')" +( + cd "$RO_SB" || exit 1 + python3 "$IAS" init s >/dev/null 2>&1 + mkdir -p .prflow/tmp + printf '4242' > .prflow/tmp/issue-audit-state-s.json.lock + python3 "$IAS" query-nonce s > .ro-out 2> .ro-err + printf '%s' "$?" > .ro-rc +) || true +assert_eq "#1040 readers_are_not_serialized: query-nonce exits 0 while a sentinel is held" \ + "0" "$(cat "$RO_SB/.ro-rc" 2>/dev/null)" +assert_eq "#1040 readers_are_not_serialized: the held sentinel is left untouched by the reader" \ + "1" "$( [ -f "$RO_SB/.prflow/tmp/issue-audit-state-s.json.lock" ] && echo 1 || echo 0 )" +rm -rf "$RO_SB" diff --git a/lib/test/test_python_scripts.py b/lib/test/test_python_scripts.py index 24a1774ee..e227b0951 100755 --- a/lib/test/test_python_scripts.py +++ b/lib/test/test_python_scripts.py @@ -7106,6 +7106,35 @@ def _always_fail_replace(src, dst): assert_eq("#1040 sequential_writers_both_land: no sentinel leaked", False, os.path.exists(_ias1040_sentinel(_R))) +# contend_then_acquire_after_release: a FRESH (non-stale) sentinel held by "another writer" +# that is released within the acquire window — the mutation must WAIT (FileExistsError → +# sleep → retry) and then acquire once the plain release frees the sentinel, NOT via a +# stale-break. Driven deterministically by a threading.Timer that unlinks the planted +# sentinel ~60ms in, with stale_after_s large enough that no break happens. This is the +# retry-loop's acquire-after-plain-release branch — the mechanism's whole purpose. +import threading as _threading1040 # noqa: E402 +with tempfile.TemporaryDirectory() as _td: + _R = Path(_td) + (_R / '.prflow' / 'tmp').mkdir(parents=True) + _sent = _ias1040_sentinel(_R) + with open(_sent, 'w') as _fh: + _fh.write('4242') # fresh mtime — NOT stale under stale_after_s below + _timer = _threading1040.Timer(0.06, lambda: os.unlink(_sent) if os.path.exists(_sent) + else None) + _timer.start() + _t0 = _time1040.monotonic() + with issue_audit_state._StateSection('s', root=_R, acquire_window_s=5, stale_after_s=30): + issue_audit_state.save_state(_state([]), 's', root=_R) + _waited = _time1040.monotonic() - _t0 + _timer.cancel() + assert_eq("#1040 contend_then_acquire_after_release: the mutation lands after the held " + "sentinel is released within the window", 0, + len(issue_audit_state.load_state('s', root=_R)['rounds'])) + assert_eq("#1040 contend_then_acquire_after_release: it actually WAITED for the release " + "(did not stale-break or fail)", True, _waited >= 0.05) + assert_eq("#1040 contend_then_acquire_after_release: its own sentinel is released after", + False, os.path.exists(_sent)) + # section_released_on_raise: a handler raising inside the section, and save_state raising, # both leave no sentinel behind. with tempfile.TemporaryDirectory() as _td: @@ -7251,9 +7280,10 @@ def _denied_unlink(*a, **k): _raised['kind'] = type(_e).__name__ finally: issue_audit_state.os.unlink = _orig_unlink - assert_eq("#1040 unlink_failure_never_replaces_the_real_exception: __exit__ does not " - "raise the unlink failure (returns falsy, the real exception propagates from " - "the with-block)", None, _raised['kind']) + assert_eq("#1040 unlink_failure_never_replaces_the_real_exception: __exit__ swallows the " + "unlink failure — it returns falsy without raising, so it neither replaces nor " + "suppresses the in-flight exception (with-block propagation is Python semantics, " + "not exercised here)", None, _raised['kind']) assert_eq("#1040 unlink_failure_never_replaces_the_real_exception: the release failure " "is breadcrumbed", True, 'could not unlink the audit-state sentinel' in _err_buf.getvalue()) @@ -7371,6 +7401,17 @@ def _ns(**kw): assert_eq("#1040 _selects_stdin: check-claim-staleness reads only with --domain-stdin", True, issue_audit_state._selects_stdin(_ns(cmd='check-claim-staleness', domain_stdin=True))) +assert_eq("#1040 _selects_stdin: record-coverage reads with --coverage-stdin", True, + issue_audit_state._selects_stdin(_ns(cmd='record-coverage', coverage_stdin=True))) +assert_eq("#1040 _selects_stdin: record-claim-baseline reads only with --domain-stdin", + True, issue_audit_state._selects_stdin(_ns(cmd='record-claim-baseline', + domain_stdin=True))) +assert_eq("#1040 _selects_stdin: record-claim-baseline without --domain-stdin reads no stdin", + False, issue_audit_state._selects_stdin(_ns(cmd='record-claim-baseline', + domain_stdin=False))) +assert_eq("#1040 _selects_stdin: record-finding-evidence reads only with --observed-stdin", + True, issue_audit_state._selects_stdin(_ns(cmd='record-finding-evidence', + observed_stdin=True))) assert_eq("#1040 _selects_stdin: an unrelated command selects no stdin", False, issue_audit_state._selects_stdin(_ns(cmd='query-summary'))) diff --git a/scripts/issue-audit-state.py b/scripts/issue-audit-state.py index 783d0555a..ea345b5b9 100644 --- a/scripts/issue-audit-state.py +++ b/scripts/issue-audit-state.py @@ -1170,11 +1170,24 @@ def _try_create(self): f'could not create the audit-state section sentinel ' f'{self._sentinel}: {exc}') from exc try: - st = os.fstat(fd) - self._token = (st.st_dev, st.st_ino) - os.write(fd, str(os.getpid()).encode('ascii')) - finally: - os.close(fd) + try: + st = os.fstat(fd) + self._token = (st.st_dev, st.st_ino) + os.write(fd, str(os.getpid()).encode('ascii')) + finally: + os.close(fd) + except OSError as exc: + # An OSError after the exclusive create succeeded (an ENOSPC on the pid write, + # an fstat failure) must still route as a cannot-persist StateError, not escape + # as a raw traceback that breaks the mutation contract. Best-effort unlink the + # partial sentinel so it does not block later acquires until it ages out. + try: + os.unlink(self._sentinel) + except OSError: + pass + raise self._persist_error( + f'could not initialize the audit-state section sentinel ' + f'{self._sentinel}: {exc}') from exc return True def _break_if_stale(self): @@ -1218,7 +1231,15 @@ def __enter__(self): # Create the parent directory BEFORE the first exclusive-create so a fresh clone, # a fresh adopter checkout, and a bare test sandbox — none of which carry the # ignored state tmp directory — acquire instead of raising on FileNotFoundError. - os.makedirs(self._parent, exist_ok=True) + # An OSError here (a non-directory occupies the path, a permission-denied parent) + # routes as a cannot-persist StateError, not a raw traceback — the section's + # single-failure-vocabulary contract holds on the setup path too. + try: + os.makedirs(self._parent, exist_ok=True) + except OSError as exc: + raise self._persist_error( + f'could not create the audit-state section directory ' + f'{self._parent}: {exc}') from exc deadline = time.monotonic() + self._acquire_window_s while True: if self._try_create(): @@ -7907,10 +7928,16 @@ def cmd_record_claim_baseline(args): f'by its re-executed full-domain search result; pipe it with ' f'--domain-stdin') # The domain search result was read from stdin, hoisted into main() above the - # section (issue #1040). This site never carried the fd-0-closed guard, so a closed - # fd 0 leaves args._stdin_data None and falls through to the domain-class-empty-domain - # refusal below (a clean named breadcrumb where an in-handler bare read would have - # crashed with an AttributeError) — a deliberate, minor robustness improvement. + # section (issue #1040). A mid-read OSError the hoist captured must name its real + # cause here rather than be laundered into the domain-class-empty-domain refusal + # below (which would hand downstream triage a trusted "empty search" token for a + # condition that never occurred). This site never carried the fd-0-closed guard, so + # a closed fd 0 leaves args._stdin_data None and falls through to + # domain-class-empty-domain (a clean named breadcrumb where an in-handler bare read + # would have crashed with an AttributeError) — a deliberate, minor improvement. + if args._stdin_error is not None: + _fail(prefix, 'could not read the domain search result from stdin: ' + f'{args._stdin_error}') data = args._stdin_data if not data: _fail(prefix, 'domain-class-empty-domain: --domain-stdin produced no bytes; a ' @@ -8070,9 +8097,13 @@ def cmd_record_finding_evidence(args): doc = _load_for_mutation(prefix, args.slug, args.nonce) observed = None if args.observed_stdin: - # Read from stdin, hoisted into main() above the section (issue #1040). This site - # never carried the fd-0-closed guard, so a closed fd 0 leaves args._stdin_data None - # and the decode below raises the same way the former bare sys.stdin read did. + # Read from stdin, hoisted into main() above the section (issue #1040). A mid-read + # OSError the hoist captured must name its real cause rather than surface as a + # NoneType decode crash below that discards it. This site never carried the + # fd-0-closed guard, so a closed fd 0 still leaves args._stdin_data None and the + # decode below raises the same AttributeError the former bare sys.stdin read did. + if args._stdin_error is not None: + _fail(prefix, f'could not read the observed output from stdin: {args._stdin_error}') raw = args._stdin_data # An empty read is NOT refused: issue #704 requires evidence that is absent or # incomplete to be RECORDED `incomplete` (never verified), which is what From 88ea72323824eeb652c35b14aa3a96b3e388bbd9 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Sat, 1 Aug 2026 02:45:13 +0000 Subject: [PATCH 4/9] docs: update documentation for issue #1040 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - docs/DEVFLOW_SYSTEM_OVERVIEW.md §11: state the write-serialization contract (exclusive-create sentinel, read-only subcommands unserialized, abandoned sentinel recovered by age, document-integrity-only guarantee + the decision-channel residual), canonical home for the system contract. - skills/create-issue/references/step-3-6-audit.md: the batching-caller operational rule (issue the batch, re-query after it completes; never act on a member's own next_call= line), pointing at §11 as canonical. Co-Authored-By: Claude Opus 4.8 --- docs/DEVFLOW_SYSTEM_OVERVIEW.md | 2 +- skills/create-issue/references/step-3-6-audit.md | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/DEVFLOW_SYSTEM_OVERVIEW.md b/docs/DEVFLOW_SYSTEM_OVERVIEW.md index 8650643ef..73958800a 100644 --- a/docs/DEVFLOW_SYSTEM_OVERVIEW.md +++ b/docs/DEVFLOW_SYSTEM_OVERVIEW.md @@ -635,7 +635,7 @@ The skill exists to prevent "option-listing" issues. Steps: 4. **Self-steelman (Step 3.5, mandatory — issue #304):** before the user sees the draft, the skill stress-tests it against the *actual code* — fresh targeted reads/greps, never the ambient context the draft came from. It verifies every load-bearing claim, file reference, and acceptance criterion; runs a **universal-quantifier sweep** (every "never/always/each/every/all/cannot" grounded — pinned per-arm, scoped, or removed — with a planted-defect positive-control obligation on detector-coverage claims); grounds every **occurrence count and coupled-site list** by an executed whitespace-normalized search or the cited evidence records, never recall; reads a "code does X" premise with its **enclosing gates/conditionals and their defaults**; hunts for missed ACs, missed edge cases, wrong assumptions, unstated scope, and an unreconciled multi-state contract (a within-text check that no summary or table form lists fewer causes for a state than the per-state ACs specify); it also flags an **AC mutual-consistency** conflict (no AC forbids a surface another AC's discharge must touch) and a **trust-boundary closure** gap (a protected executable-artifact set defined over the transitive source/exec/import closure of its entry points); binds every **stated-but-unbound input** (a mechanism input named only by role) to a named referent with a cited code reference or an implementer-obligation AC; revises the draft itself and re-runs the no-options gate on the revision; then — at every revision event, per the shared **Revision-delta verification** procedure (stated once, referenced by every revise-and-re-gate site) — walks the revision's edit-batch delta across six classes (mechanisms, lifecycle rules, execution-tier assumptions, dependencies, universal guarantees, and a total-making residual class) and verifies each non-empty class against the code, so audit rounds and the declined-re-audit filing path see delta that has been walked and verified rather than only language-gated; and reports a one-line outcome summary either way, including the universal-quantifier sweep's zero arm (a silent pass is indistinguishable from a skipped step). A genuinely new decision fork routes through the existing Step 2 question machinery, not a new path. 5. **Fresh-context audit (Step 3.6, mandatory — issue #443, extended by #522):** after Step 3.5 passes (which now also **self-checks the draft against the same audit dimension checklist** before any dispatch) and before the user sees the draft, the skill dispatches **one synchronous audit subagent** whose entire value is that it did **not** draft the issue — the mechanism is *information removal* (separated-context critics outperform same-session self-review on critical errors). On the normal **file arm**, the orchestrator writes the current rendered draft to the canonical `issue-draft-.md` before each round and the auditor **reads that file as the sole draft source** (closing the condensation-drift channel a hand-embedded copy opened) with a carriage/identity check; the drafting conversation, Step 1 findings, and the *reasoning* artifacts stay out of bounds while the draft file itself is the artifact under audit. When the write fails (read-only sandbox), a fallback **embed arm** carries the full body verbatim with its own sentinel carriage check, and the on-disk draft path stays out of bounds. The auditor runs an **adversarial pre-mortem** audit-prompt: a per-finding bar, scope exclusions at issue altitude, one assessed "Quiet Killer" slot, no cap on the number of findings paired with a per-finding length discipline, and a mandatory verdict line with three legal values — `VERDICT: FILE`, `VERDICT: REVISE`, or `VERDICT: DRAFT-UNREADABLE` (file arm only). On `REVISE` the orchestrator verifies each finding against the code, revises, re-runs the no-options gate, runs the shared **Revision-delta verification** procedure over the revision's delta (the same procedure the Step 3.5 and Step 4 revise loops reference), and re-audits **at most once** automatically — the audit informs, it never deadlocks filing. Past that automatic budget the skill **offers user-chosen rounds** (up to 3) via the question tool whenever the run is demonstrably unconverged — the user, not the skill, spends the tokens. Every accepted round's findings are **adjudicated** into must-revise / advisory / invalid; since issue #743 the advisory and invalid grades are no longer bare counts but carry a **durable per-finding record** — a one-line summary and rationale, an impact-class tag, and the auditor's returned finding block byte-preserved — recorded through the state owner (`record-adjudication --advisory-records-file/--invalid-records-file`, refused when a count and its records disagree), read back with `query-adjudication-records`, and rendered to the user **before** the approval election. A **calibration** layer (`query-calibration`, a `calibration=` sibling of the coverage boundary offer) surfaces an advisory grade on an impact-bearing finding (`implementation-correctness`/`scope`/`safety`/`verifiability`) that carries no recorded evidence, so an under-evidenced grade is named to the maintainer rather than silently converged past — disclosure only, **never a filing block**. Full evidence record: [`docs/advisory-adjudication-calibration.md`](advisory-adjudication-calibration.md). -**The lifecycle itself is owned by a tested state-owner CLI, not by prose (issue #546).** Every deterministic rule above — transition legality, round numbering, the automatic budget and the bounded retries, arm routing and its three embed markers, digest computation and comparison, sentinel generation and comparison, the offer triggers, override records, presentation eligibility, and the audit-summary field set — is executed by `scripts/issue-audit-state.py`; the skill records lifecycle events through it and **obeys its answers** rather than re-deriving them each turn. Its two-class contract is what the prose branches on: queries always exit 0 once the arguments parse (an argparse usage error still exits 2) with a decided answer line, except for the multi-line read-back queries `query-findings`, issue #704's `query-claim-baselines` / `query-finding-evidence`, `query-coverage`, and `query-adjudication-records`, which print one decided line per record, and issue #795's composite `query-boundary`, which prints one decided line per boundary component; all of them are strictly read-only, while mutations exit non-zero with a named breadcrumb on an illegal transition or an unpersistable state. Since issue #795 most subcommands print a second and final `next_call=` line naming the next legal invocation, with every state-derivable operand filled and every caller-supplied one bare in a `needs=` field; it is a generated suggestion the caller reviews before running, never an instruction, and the decided answer line is unchanged and stays first. The same issue lets the five subcommands whose round the state uniquely determines resolve an omitted `--round` from state, while every subcommand where the flag selects an operation or names a caller-chosen round keeps it required. Run state persists to a cwd/worktree-anchored `.prflow/tmp/issue-audit-state-.json`, replacing the markdown event log the offer used to read — the skill still writes the observable audit artifact (`.prflow/tmp/issue-audit-.md`, same gate/read-only-stand-in convention as the derivation artifact), and both state paths plus the **retired** `.md` leftover stay declared out of bounds so an auditor with repository read access cannot re-anchor on this run's prior verdicts. Only the current draft counts as audited: eligibility is grounded on a completed clean-verdict round whose recorded identity still holds — byte-digest equality against the canonical file on file-arm epochs, revision ordering on the embed and inline arms where no trustworthy file exists — or on an explicitly recorded override that no later revision has invalidated, and every `eligible` answer carries a deterministic token bound to the answering digest or revision ordinal (matching the ground that answered) which the summary line quotes verbatim. That **narrows** the prose-compliance gap and makes a skipped eligibility check detectable in the transcript; it does not close it, since no in-process component can force an orchestrator that never invokes it. Where the tool cannot run at all, a named bounded fallback runs one round, asks once, and marks the summary line `state-owner unavailable` — distinct from `degraded`, which keeps its meaning of the inline audit arm. +**The lifecycle itself is owned by a tested state-owner CLI, not by prose (issue #546).** Every deterministic rule above — transition legality, round numbering, the automatic budget and the bounded retries, arm routing and its three embed markers, digest computation and comparison, sentinel generation and comparison, the offer triggers, override records, presentation eligibility, and the audit-summary field set — is executed by `scripts/issue-audit-state.py`; the skill records lifecycle events through it and **obeys its answers** rather than re-deriving them each turn. Its two-class contract is what the prose branches on: queries always exit 0 once the arguments parse (an argparse usage error still exits 2) with a decided answer line, except for the multi-line read-back queries `query-findings`, issue #704's `query-claim-baselines` / `query-finding-evidence`, `query-coverage`, and `query-adjudication-records`, which print one decided line per record, and issue #795's composite `query-boundary`, which prints one decided line per boundary component; all of them are strictly read-only, while mutations exit non-zero with a named breadcrumb on an illegal transition or an unpersistable state. Since issue #795 most subcommands print a second and final `next_call=` line naming the next legal invocation, with every state-derivable operand filled and every caller-supplied one bare in a `needs=` field; it is a generated suggestion the caller reviews before running, never an instruction, and the decided answer line is unchanged and stays first. The same issue lets the five subcommands whose round the state uniquely determines resolve an omitted `--round` from state, while every subcommand where the flag selects an operation or names a caller-chosen round keeps it required. Run state persists to a cwd/worktree-anchored `.prflow/tmp/issue-audit-state-.json`, replacing the markdown event log the offer used to read — the skill still writes the observable audit artifact (`.prflow/tmp/issue-audit-.md`, same gate/read-only-stand-in convention as the derivation artifact), and both state paths plus the **retired** `.md` leftover stay declared out of bounds so an auditor with repository read access cannot re-anchor on this run's prior verdicts. **Writes to that state document are serialized (issue #1040).** Every mutating subcommand runs inside an exclusive-create sentinel critical section — a `.lock` file beside the state document, created with `os.O_CREAT | os.O_EXCL` (standard-library only) — so two concurrent invocations for the same slug produce a document reflecting one of them entirely and then the other, never an interleaved mixture; each writer also persists through its own unique `tempfile.mkstemp` temporary path (with a bounded `os.replace` retry over `PermissionError`) so two writers never share and truncate one temp file. **Read-only subcommands acquire no sentinel and stay unserialized** (`query-*`, `emit-body`, `check-claim-staleness`), and a fail-closed transitive call-graph check proves `save_state` is unreachable from every read-only-classified subcommand. An **abandoned sentinel is recovered by age** (a stale sentinel older than the acquire window is broken so a crashed writer cannot wedge the slug permanently). This guarantee is **document integrity only**. The **decision channel is a distinct quantity with a named residual**: because `_emit_next_call` re-reads the state after the critical section has released, 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 happened to observe — so those answers are **not authoritative under concurrent invocation**, even though the persisted document they read is always internally consistent. §11 is the canonical home for this system contract; the skill's operational rule points here rather than restating it. Only the current draft counts as audited: eligibility is grounded on a completed clean-verdict round whose recorded identity still holds — byte-digest equality against the canonical file on file-arm epochs, revision ordering on the embed and inline arms where no trustworthy file exists — or on an explicitly recorded override that no later revision has invalidated, and every `eligible` answer carries a deterministic token bound to the answering digest or revision ordinal (matching the ground that answered) which the summary line quotes verbatim. That **narrows** the prose-compliance gap and makes a skipped eligibility check detectable in the transcript; it does not close it, since no in-process component can force an orchestrator that never invokes it. Where the tool cannot run at all, a named bounded fallback runs one round, asks once, and marks the summary line `state-owner unavailable` — distinct from `degraded`, which keeps its meaning of the inline audit arm. **Runtime main-thread context (issue #767).** Separate from the skill's *structure* and *static shipped size* discussed above, a long create-issue run accumulates a large **runtime main-thread context** across its many turns. The behavioral instrument `scripts/create-issue-context-eval.py` (maintainer-run over a transcript corpus; never on the skill's runtime path, so no new tool grant) measures it, and the determination of which appended-content classes are authoritative versus safely-removable redundant additions — with the reduction that removes the primary safely-removable class (re-emission of an already-produced block, replaced by a pointer) — lives in [`docs/create-issue-context.md`](create-issue-context.md). That doc is the single source of truth for this axis; it is not paraphrased here. diff --git a/skills/create-issue/references/step-3-6-audit.md b/skills/create-issue/references/step-3-6-audit.md index 80e9a8e49..71d403498 100644 --- a/skills/create-issue/references/step-3-6-audit.md +++ b/skills/create-issue/references/step-3-6-audit.md @@ -291,6 +291,8 @@ The variants, each obeying the same contract: **The `next_call=` answer line (issue #795).** Every subcommand outside a named exclusion set prints, as its **final** stdout line, one of exactly three shapes: an invocation line, `next_call=none`, or `next_call=unestablished reason=`. An invocation line carries every **state-derivable** operand filled, every **caller-supplied** operand as a bare flag name, and a `needs=` field naming those flags — so you copy the shape and supply only what you alone observed. It is prefixed by the fixed placeholder `` in place of a program head; **substitute your own portable-anchor invocation** for that placeholder. The line is **a generated suggestion you review before running, never an instruction** — where it and the mandated next step of this procedure disagree, **this procedure wins**. Where the mandated next step is not a tool call at all — a user interaction or a required verification (a foreign nonce, an unestablished state, the boundary offer, the auditor dispatch, the advisory-record rendering `--landed` attests to, and the verify-then-revise chain) — the line answers `unestablished` with its reason rather than naming a command. The **excluded** set is `emit-body` (payload stdout), the multi-line read-backs, and `check-claim-staleness`; the dispatch-routing answers that name a call — `query-arm` on a fresh round, and `query-next-action`'s `dispatch-embed-retry` and `dispatch-inline-degraded` answers — render `record-dispatch … --round` **bare** in `needs=`, so the flag can no longer be forgotten while the number stays yours to supply. Those same rendered `record-dispatch` invocations carry the required `--kind` **filled** from the kind already recorded for the round the answer refers to, and on a targeted round they additionally render `--scope-file` **bare**, named in `needs=` alongside `--round` — the scope path is one you supply from `write-dispatch-scope`'s printed answer. Two answers name no call and render `unestablished reason=dispatch-arm-unestablished` instead: `dispatch-retry-same-arm`, because the arm to retry is whichever the round already ran, which the tool cannot name for you; and `confirm-whole-draft`, because it opens a round that does not yet exist, whose arm and kind you obtain from `query-arm` and `query-round-kind`. +**Batching state-owner mutations — re-query after the batch, never act on a member's own `next_call=` line (issue #1040).** When you issue several state-owner mutations as **one parallel tool-call batch** — each call is a separate process — the mutations to the state *document* are serialized by an exclusive-create sentinel beside the state file, so the document reflects one call entirely and then the next, never a mixture. The **decision channel is not** serialized: `_emit_next_call` re-reads the state after the section releases and every `query-*` reads unserialized, so each member's `next_call=` line (and each `query-*` answer) renders against whichever post-image that process happened to observe and is **not authoritative under concurrent invocation**. So the operational rule for a batch is: **issue the whole batch, then re-query once it has completed**, and act on that single post-batch answer — never on any individual member's own `next_call=` line. `docs/DEVFLOW_SYSTEM_OVERVIEW.md` §11 carries and is canonical for this system contract (the serialization guarantee, its document-integrity-only scope, and this decision-channel residual); this rule is the operational form the batching caller follows, not a restatement of it. + **Fallback — `state-owner unavailable`.** The two routing classes, the two exits that route elsewhere, and this arm's bounded one-round conduct are stated in `references/fallback-state-owner-unavailable.md`; load it per the root's routing table when the state owner stops answering. **Degraded arm — attempt-first, never pre-detected.** When no subagent tool is exposed, when the dispatch call itself errors, or when `query-next-action` answers `dispatch-inline-degraded`, follow `references/fallback-audit-dispatch-arms.md` — loaded per the root's routing table — and mark the audit summary line accordingly. From 1a10600e20e40a4887880d02ac70824e2f8b3b07 Mon Sep 17 00:00:00 2001 From: The01Geek Date: Fri, 31 Jul 2026 22:33:49 -0600 Subject: [PATCH 5/9] =?UTF-8?q?docs:=20correct=20the=20=C2=A711=20stale=20?= =?UTF-8?q?threshold=20and=20state=20the=20heartbeat-free=20exclusion=20bo?= =?UTF-8?q?und=20(#1040)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Documentation only. Proven behavior-inert: with docstrings stripped, the AST of scripts/issue-audit-state.py is identical before and after this commit. No locking behavior is changed, and no heartbeat is added — the fail-closed direction is untouched. 1. docs/DEVFLOW_SYSTEM_OVERVIEW.md §11 said an abandoned sentinel "older than the acquire window" is broken. The code compares mtime age against `stale_after_s`, not the longer `acquire_window_s`, so a reader trusting §11 would reason about the wrong bound. §11 now names the actual threshold, states the `stale_after_s < acquire_window_s` relation that is what makes "cannot wedge permanently" true, and notes that the acquire-window expiry is the fail-closed arm for a host whose overrides invert that relation. The parameters are named rather than their literals transcribed, so the text does not rot when a default moves. 2. _StateSection's docstring now records the heartbeat-free bound as a STATED BOUND, beside the __init__ that defines both parameters: the holder never refreshes the sentinel mtime, so a mutation occupying the section longer than `stale_after_s` can have its sentinel age-broken and the two writers overlap — the guarantee is "serialized up to `stale_after_s` of occupancy", not unconditional mutual exclusion. It records why that is accepted (occupancy is a sub-second load-modify-save holding no network call, subprocess, or stdin read; the (st_dev, st_ino) token means a broken holder releases nothing and cannot strip the breaker's lock) and that adding a heartbeat is a design change to be decided deliberately, not a bug fix. Co-Authored-By: Claude Opus 5 (1M context) --- docs/DEVFLOW_SYSTEM_OVERVIEW.md | 2 +- scripts/issue-audit-state.py | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/docs/DEVFLOW_SYSTEM_OVERVIEW.md b/docs/DEVFLOW_SYSTEM_OVERVIEW.md index 910f89b4e..e23065453 100644 --- a/docs/DEVFLOW_SYSTEM_OVERVIEW.md +++ b/docs/DEVFLOW_SYSTEM_OVERVIEW.md @@ -635,7 +635,7 @@ The skill exists to prevent "option-listing" issues. Steps: 4. **Self-steelman (Step 3.5, mandatory — issue #304):** before the user sees the draft, the skill stress-tests it against the *actual code* — fresh targeted reads/greps, never the ambient context the draft came from. It verifies every load-bearing claim, file reference, and acceptance criterion; runs a **universal-quantifier sweep** (every "never/always/each/every/all/cannot" grounded — pinned per-arm, scoped, or removed — with a planted-defect positive-control obligation on detector-coverage claims); grounds every **occurrence count and coupled-site list** by an executed whitespace-normalized search or the cited evidence records, never recall; reads a "code does X" premise with its **enclosing gates/conditionals and their defaults**; hunts for missed ACs, missed edge cases, wrong assumptions, unstated scope, and an unreconciled multi-state contract (a within-text check that no summary or table form lists fewer causes for a state than the per-state ACs specify); it also flags an **AC mutual-consistency** conflict (no AC forbids a surface another AC's discharge must touch) and a **trust-boundary closure** gap (a protected executable-artifact set defined over the transitive source/exec/import closure of its entry points); binds every **stated-but-unbound input** (a mechanism input named only by role) to a named referent with a cited code reference or an implementer-obligation AC; revises the draft itself and re-runs the no-options gate on the revision; then — at every revision event, per the shared **Revision-delta verification** procedure (stated once, referenced by every revise-and-re-gate site) — walks the revision's edit-batch delta across six classes (mechanisms, lifecycle rules, execution-tier assumptions, dependencies, universal guarantees, and a total-making residual class) and verifies each non-empty class against the code, so audit rounds and the declined-re-audit filing path see delta that has been walked and verified rather than only language-gated; and reports a one-line outcome summary either way, including the universal-quantifier sweep's zero arm (a silent pass is indistinguishable from a skipped step). A genuinely new decision fork routes through the existing Step 2 question machinery, not a new path. 5. **Fresh-context audit (Step 3.6, mandatory — issue #443, extended by #522):** after Step 3.5 passes (which now also **self-checks the draft against the same audit dimension checklist** before any dispatch) and before the user sees the draft, the skill dispatches **one synchronous audit subagent** whose entire value is that it did **not** draft the issue — the mechanism is *information removal* (separated-context critics outperform same-session self-review on critical errors). On the normal **file arm**, the orchestrator writes the current rendered draft to the canonical `issue-draft-.md` before each round and the auditor **reads that file as the sole draft source** (closing the condensation-drift channel a hand-embedded copy opened) with a carriage/identity check; the drafting conversation, Step 1 findings, and the *reasoning* artifacts stay out of bounds while the draft file itself is the artifact under audit. When the write fails (read-only sandbox), a fallback **embed arm** carries the full body verbatim with its own sentinel carriage check, and the on-disk draft path stays out of bounds. The auditor runs an **adversarial pre-mortem** audit-prompt: a per-finding bar, scope exclusions at issue altitude, one assessed "Quiet Killer" slot, no cap on the number of findings paired with a per-finding length discipline, and a mandatory verdict line with three legal values — `VERDICT: FILE`, `VERDICT: REVISE`, or `VERDICT: DRAFT-UNREADABLE` (file arm only). On `REVISE` the orchestrator verifies each finding against the code, revises, re-runs the no-options gate, runs the shared **Revision-delta verification** procedure over the revision's delta (the same procedure the Step 3.5 and Step 4 revise loops reference), and re-audits **at most once** automatically — the audit informs, it never deadlocks filing. Past that automatic budget the skill **offers user-chosen rounds** (up to 3) via the question tool whenever the run is demonstrably unconverged — the user, not the skill, spends the tokens. Every accepted round's findings are **adjudicated** into must-revise / advisory / invalid; since issue #743 the advisory and invalid grades are no longer bare counts but carry a **durable per-finding record** — a one-line summary and rationale, an impact-class tag, and the auditor's returned finding block byte-preserved — recorded through the state owner (`record-adjudication --advisory-records-file/--invalid-records-file`, refused when a count and its records disagree), read back with `query-adjudication-records`, and rendered to the user **before** the approval election. A **calibration** layer (`query-calibration`, a `calibration=` sibling of the coverage boundary offer) surfaces an advisory grade on an impact-bearing finding (`implementation-correctness`/`scope`/`safety`/`verifiability`) that carries no recorded evidence, so an under-evidenced grade is named to the maintainer rather than silently converged past — disclosure only, **never a filing block**. Full evidence record: [`docs/advisory-adjudication-calibration.md`](advisory-adjudication-calibration.md). -**The lifecycle itself is owned by a tested state-owner CLI, not by prose (issue #546).** Every deterministic rule above — transition legality, round numbering, the automatic budget and the bounded retries, arm routing and its three embed markers, digest computation and comparison, sentinel generation and comparison, the offer triggers, override records, presentation eligibility, and the audit-summary field set — is executed by `scripts/issue-audit-state.py`; the skill records lifecycle events through it and **obeys its answers** rather than re-deriving them each turn. Its two-class contract is what the prose branches on: queries always exit 0 once the arguments parse (an argparse usage error still exits 2) with a decided answer line, except for the multi-line read-back queries `query-findings`, issue #704's `query-claim-baselines` / `query-finding-evidence`, `query-coverage`, and `query-adjudication-records`, which print one decided line per record, and issue #795's composite `query-boundary`, which prints one decided line per boundary component; all of them are strictly read-only, while mutations exit non-zero with a named breadcrumb on an illegal transition or an unpersistable state. Since issue #795 most subcommands print a second and final `next_call=` line naming the next legal invocation, with every state-derivable operand filled and every caller-supplied one bare in a `needs=` field; it is a generated suggestion the caller reviews before running, never an instruction, and the decided answer line is unchanged and stays first. The same issue lets the five subcommands whose round the state uniquely determines resolve an omitted `--round` from state, while every subcommand where the flag selects an operation or names a caller-chosen round keeps it required. Run state persists to a cwd/worktree-anchored `.prflow/tmp/issue-audit-state-.json`, replacing the markdown event log the offer used to read — the skill still writes the observable audit artifact (`.prflow/tmp/issue-audit-.md`, same gate/read-only-stand-in convention as the derivation artifact), and both state paths plus the **retired** `.md` leftover stay declared out of bounds so an auditor with repository read access cannot re-anchor on this run's prior verdicts. **Writes to that state document are serialized (issue #1040).** Every mutating subcommand runs inside an exclusive-create sentinel critical section — a `.lock` file beside the state document, created with `os.O_CREAT | os.O_EXCL` (standard-library only) — so two concurrent invocations for the same slug produce a document reflecting one of them entirely and then the other, never an interleaved mixture; each writer also persists through its own unique `tempfile.mkstemp` temporary path (with a bounded `os.replace` retry over `PermissionError`) so two writers never share and truncate one temp file. **Read-only subcommands acquire no sentinel and stay unserialized** (`query-*`, `emit-body`, `check-claim-staleness`), and a fail-closed transitive call-graph check proves `save_state` is unreachable from every read-only-classified subcommand. An **abandoned sentinel is recovered by age** (a stale sentinel older than the acquire window is broken so a crashed writer cannot wedge the slug permanently). This guarantee is **document integrity only**. The **decision channel is a distinct quantity with a named residual**: because `_emit_next_call` re-reads the state after the critical section has released, 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 happened to observe — so those answers are **not authoritative under concurrent invocation**, even though the persisted document they read is always internally consistent. §11 is the canonical home for this system contract; the skill's operational rule points here rather than restating it. Only the current draft counts as audited: eligibility is grounded on a completed clean-verdict round whose recorded identity still holds — byte-digest equality against the canonical file on file-arm epochs, revision ordering on the embed and inline arms where no trustworthy file exists — or on an explicitly recorded override that no later revision has invalidated, and every `eligible` answer carries a deterministic token bound to the answering digest or revision ordinal (matching the ground that answered) which the summary line quotes verbatim. That **narrows** the prose-compliance gap and makes a skipped eligibility check detectable in the transcript; it does not close it, since no in-process component can force an orchestrator that never invokes it. Where the tool cannot run at all, a named bounded fallback runs one round, asks once, and marks the summary line `state-owner unavailable` — distinct from `degraded`, which keeps its meaning of the inline audit arm. +**The lifecycle itself is owned by a tested state-owner CLI, not by prose (issue #546).** Every deterministic rule above — transition legality, round numbering, the automatic budget and the bounded retries, arm routing and its three embed markers, digest computation and comparison, sentinel generation and comparison, the offer triggers, override records, presentation eligibility, and the audit-summary field set — is executed by `scripts/issue-audit-state.py`; the skill records lifecycle events through it and **obeys its answers** rather than re-deriving them each turn. Its two-class contract is what the prose branches on: queries always exit 0 once the arguments parse (an argparse usage error still exits 2) with a decided answer line, except for the multi-line read-back queries `query-findings`, issue #704's `query-claim-baselines` / `query-finding-evidence`, `query-coverage`, and `query-adjudication-records`, which print one decided line per record, and issue #795's composite `query-boundary`, which prints one decided line per boundary component; all of them are strictly read-only, while mutations exit non-zero with a named breadcrumb on an illegal transition or an unpersistable state. Since issue #795 most subcommands print a second and final `next_call=` line naming the next legal invocation, with every state-derivable operand filled and every caller-supplied one bare in a `needs=` field; it is a generated suggestion the caller reviews before running, never an instruction, and the decided answer line is unchanged and stays first. The same issue lets the five subcommands whose round the state uniquely determines resolve an omitted `--round` from state, while every subcommand where the flag selects an operation or names a caller-chosen round keeps it required. Run state persists to a cwd/worktree-anchored `.prflow/tmp/issue-audit-state-.json`, replacing the markdown event log the offer used to read — the skill still writes the observable audit artifact (`.prflow/tmp/issue-audit-.md`, same gate/read-only-stand-in convention as the derivation artifact), and both state paths plus the **retired** `.md` leftover stay declared out of bounds so an auditor with repository read access cannot re-anchor on this run's prior verdicts. **Writes to that state document are serialized (issue #1040).** Every mutating subcommand runs inside an exclusive-create sentinel critical section — a `.lock` file beside the state document, created with `os.O_CREAT | os.O_EXCL` (standard-library only) — so two concurrent invocations for the same slug produce a document reflecting one of them entirely and then the other, never an interleaved mixture; each writer also persists through its own unique `tempfile.mkstemp` temporary path (with a bounded `os.replace` retry over `PermissionError`) so two writers never share and truncate one temp file. **Read-only subcommands acquire no sentinel and stay unserialized** (`query-*`, `emit-body`, `check-claim-staleness`), and a fail-closed transitive call-graph check proves `save_state` is unreachable from every read-only-classified subcommand. An **abandoned sentinel is recovered by age**, and the threshold that decides it is `stale_after_s` — *not* the longer `acquire_window_s` a contending writer is willing to wait: a sentinel whose mtime age exceeds `stale_after_s` is broken and the break is re-attempted once. Because the shipped `stale_after_s` is strictly shorter than the shipped `acquire_window_s`, a writer contending with an abandoned sentinel always reaches that break inside its own acquire window, which is what makes a crashed writer unable to wedge the slug permanently; the acquire-window expiry is the fail-closed arm for a host whose overrides invert that relation, and it refuses the mutation rather than proceeding. That relation is the load-bearing part — an override that sets `stale_after_s` above `acquire_window_s` trades permanent-wedge immunity for the refusal. This guarantee is **document integrity only**. The **decision channel is a distinct quantity with a named residual**: because `_emit_next_call` re-reads the state after the critical section has released, 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 happened to observe — so those answers are **not authoritative under concurrent invocation**, even though the persisted document they read is always internally consistent. §11 is the canonical home for this system contract; the skill's operational rule points here rather than restating it. Only the current draft counts as audited: eligibility is grounded on a completed clean-verdict round whose recorded identity still holds — byte-digest equality against the canonical file on file-arm epochs, revision ordering on the embed and inline arms where no trustworthy file exists — or on an explicitly recorded override that no later revision has invalidated, and every `eligible` answer carries a deterministic token bound to the answering digest or revision ordinal (matching the ground that answered) which the summary line quotes verbatim. That **narrows** the prose-compliance gap and makes a skipped eligibility check detectable in the transcript; it does not close it, since no in-process component can force an orchestrator that never invokes it. Where the tool cannot run at all, a named bounded fallback runs one round, asks once, and marks the summary line `state-owner unavailable` — distinct from `degraded`, which keeps its meaning of the inline audit arm. **Runtime main-thread context (issue #767).** Separate from the skill's *structure* and *static shipped size* discussed above, a long create-issue run accumulates a large **runtime main-thread context** across its many turns. The behavioral instrument `scripts/create-issue-context-eval.py` (maintainer-run over a transcript corpus; never on the skill's runtime path, so no new tool grant) measures it, and the determination of which appended-content classes are authoritative versus safely-removable redundant additions — with the reduction that removes the primary safely-removable class (re-emission of an already-produced block, replaced by a pointer) — lives in [`docs/create-issue-context.md`](create-issue-context.md). That doc is the single source of truth for this axis; it is not paraphrased here. diff --git a/scripts/issue-audit-state.py b/scripts/issue-audit-state.py index ea345b5b9..bddf06393 100644 --- a/scripts/issue-audit-state.py +++ b/scripts/issue-audit-state.py @@ -1132,6 +1132,25 @@ class _StateSection: handler's `load_state` .. `save_state` runs under exclusion and the second writer's read happens after the first writer's write. No compare-and-swap token is needed because the load sits inside the section. + + STATED BOUND — exclusion is heartbeat-free, so it is bounded by `stale_after_s`. The + holder does not refresh the sentinel's mtime while it works, so a mutation that stays + inside the section for longer than `stale_after_s` can have its own sentinel judged + abandoned and age-broken by a contending writer, and the two then overlap: the + guarantee this class provides is therefore "serialized up to `stale_after_s` of + occupancy", not unconditional mutual exclusion. That is ACCEPTED here rather than + fixed, on two grounds. First, occupancy is a sub-second load-modify-save of one small + JSON document — the section holds no network call, no subprocess, and no stdin read + (main() hoists stdin above the section precisely so a handler cannot block on fd 0 + while holding it). Second, the `(st_dev, st_ino)` token recorded at acquire bounds the + blast radius on the way out: __exit__ unlinks only a sentinel that is still the one + this section created, so a holder whose sentinel was age-broken releases nothing and + cannot strip the breaker's exclusion — it breadcrumbs instead. Raising the bound by + adding a heartbeat (a keepalive touch, or a refresh on a long operation) is a DESIGN + CHANGE with its own failure modes, not a bug fix; do not introduce one without + deciding that trade deliberately. The relation `stale_after_s < acquire_window_s` is + the separate invariant that keeps a CRASHED writer from wedging the slug permanently; + see the acquire loop and docs/DEVFLOW_SYSTEM_OVERVIEW.md §11. """ def __init__(self, slug, root=None, *, acquire_window_s=45, stale_after_s=30): From 03021a2968a6e680a812481a7ba1dd4890dfb2d9 Mon Sep 17 00:00:00 2001 From: The01Geek Date: Fri, 31 Jul 2026 22:35:38 -0600 Subject: [PATCH 6/9] =?UTF-8?q?docs:=20name=20the=20re-attempted=20operati?= =?UTF-8?q?on=20precisely=20in=20the=20=C2=A711=20stale-break=20sentence?= =?UTF-8?q?=20(#1040)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-correction to the previous commit. That text read "... is broken and the break is re-attempted once", which names the wrong operation: _break_if_stale unlinks the stale sentinel and then re-attempts the EXCLUSIVE CREATE exactly once, returning to the ordinary acquire loop on any failure. Saying the break is re-attempted invites the reader to expect repeated unlink attempts, which is the same class of doc-code imprecision this pair of commits exists to remove. Documentation only; no executable line is touched. Co-Authored-By: Claude Opus 5 (1M context) --- docs/DEVFLOW_SYSTEM_OVERVIEW.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/DEVFLOW_SYSTEM_OVERVIEW.md b/docs/DEVFLOW_SYSTEM_OVERVIEW.md index e23065453..f911d831a 100644 --- a/docs/DEVFLOW_SYSTEM_OVERVIEW.md +++ b/docs/DEVFLOW_SYSTEM_OVERVIEW.md @@ -635,7 +635,7 @@ The skill exists to prevent "option-listing" issues. Steps: 4. **Self-steelman (Step 3.5, mandatory — issue #304):** before the user sees the draft, the skill stress-tests it against the *actual code* — fresh targeted reads/greps, never the ambient context the draft came from. It verifies every load-bearing claim, file reference, and acceptance criterion; runs a **universal-quantifier sweep** (every "never/always/each/every/all/cannot" grounded — pinned per-arm, scoped, or removed — with a planted-defect positive-control obligation on detector-coverage claims); grounds every **occurrence count and coupled-site list** by an executed whitespace-normalized search or the cited evidence records, never recall; reads a "code does X" premise with its **enclosing gates/conditionals and their defaults**; hunts for missed ACs, missed edge cases, wrong assumptions, unstated scope, and an unreconciled multi-state contract (a within-text check that no summary or table form lists fewer causes for a state than the per-state ACs specify); it also flags an **AC mutual-consistency** conflict (no AC forbids a surface another AC's discharge must touch) and a **trust-boundary closure** gap (a protected executable-artifact set defined over the transitive source/exec/import closure of its entry points); binds every **stated-but-unbound input** (a mechanism input named only by role) to a named referent with a cited code reference or an implementer-obligation AC; revises the draft itself and re-runs the no-options gate on the revision; then — at every revision event, per the shared **Revision-delta verification** procedure (stated once, referenced by every revise-and-re-gate site) — walks the revision's edit-batch delta across six classes (mechanisms, lifecycle rules, execution-tier assumptions, dependencies, universal guarantees, and a total-making residual class) and verifies each non-empty class against the code, so audit rounds and the declined-re-audit filing path see delta that has been walked and verified rather than only language-gated; and reports a one-line outcome summary either way, including the universal-quantifier sweep's zero arm (a silent pass is indistinguishable from a skipped step). A genuinely new decision fork routes through the existing Step 2 question machinery, not a new path. 5. **Fresh-context audit (Step 3.6, mandatory — issue #443, extended by #522):** after Step 3.5 passes (which now also **self-checks the draft against the same audit dimension checklist** before any dispatch) and before the user sees the draft, the skill dispatches **one synchronous audit subagent** whose entire value is that it did **not** draft the issue — the mechanism is *information removal* (separated-context critics outperform same-session self-review on critical errors). On the normal **file arm**, the orchestrator writes the current rendered draft to the canonical `issue-draft-.md` before each round and the auditor **reads that file as the sole draft source** (closing the condensation-drift channel a hand-embedded copy opened) with a carriage/identity check; the drafting conversation, Step 1 findings, and the *reasoning* artifacts stay out of bounds while the draft file itself is the artifact under audit. When the write fails (read-only sandbox), a fallback **embed arm** carries the full body verbatim with its own sentinel carriage check, and the on-disk draft path stays out of bounds. The auditor runs an **adversarial pre-mortem** audit-prompt: a per-finding bar, scope exclusions at issue altitude, one assessed "Quiet Killer" slot, no cap on the number of findings paired with a per-finding length discipline, and a mandatory verdict line with three legal values — `VERDICT: FILE`, `VERDICT: REVISE`, or `VERDICT: DRAFT-UNREADABLE` (file arm only). On `REVISE` the orchestrator verifies each finding against the code, revises, re-runs the no-options gate, runs the shared **Revision-delta verification** procedure over the revision's delta (the same procedure the Step 3.5 and Step 4 revise loops reference), and re-audits **at most once** automatically — the audit informs, it never deadlocks filing. Past that automatic budget the skill **offers user-chosen rounds** (up to 3) via the question tool whenever the run is demonstrably unconverged — the user, not the skill, spends the tokens. Every accepted round's findings are **adjudicated** into must-revise / advisory / invalid; since issue #743 the advisory and invalid grades are no longer bare counts but carry a **durable per-finding record** — a one-line summary and rationale, an impact-class tag, and the auditor's returned finding block byte-preserved — recorded through the state owner (`record-adjudication --advisory-records-file/--invalid-records-file`, refused when a count and its records disagree), read back with `query-adjudication-records`, and rendered to the user **before** the approval election. A **calibration** layer (`query-calibration`, a `calibration=` sibling of the coverage boundary offer) surfaces an advisory grade on an impact-bearing finding (`implementation-correctness`/`scope`/`safety`/`verifiability`) that carries no recorded evidence, so an under-evidenced grade is named to the maintainer rather than silently converged past — disclosure only, **never a filing block**. Full evidence record: [`docs/advisory-adjudication-calibration.md`](advisory-adjudication-calibration.md). -**The lifecycle itself is owned by a tested state-owner CLI, not by prose (issue #546).** Every deterministic rule above — transition legality, round numbering, the automatic budget and the bounded retries, arm routing and its three embed markers, digest computation and comparison, sentinel generation and comparison, the offer triggers, override records, presentation eligibility, and the audit-summary field set — is executed by `scripts/issue-audit-state.py`; the skill records lifecycle events through it and **obeys its answers** rather than re-deriving them each turn. Its two-class contract is what the prose branches on: queries always exit 0 once the arguments parse (an argparse usage error still exits 2) with a decided answer line, except for the multi-line read-back queries `query-findings`, issue #704's `query-claim-baselines` / `query-finding-evidence`, `query-coverage`, and `query-adjudication-records`, which print one decided line per record, and issue #795's composite `query-boundary`, which prints one decided line per boundary component; all of them are strictly read-only, while mutations exit non-zero with a named breadcrumb on an illegal transition or an unpersistable state. Since issue #795 most subcommands print a second and final `next_call=` line naming the next legal invocation, with every state-derivable operand filled and every caller-supplied one bare in a `needs=` field; it is a generated suggestion the caller reviews before running, never an instruction, and the decided answer line is unchanged and stays first. The same issue lets the five subcommands whose round the state uniquely determines resolve an omitted `--round` from state, while every subcommand where the flag selects an operation or names a caller-chosen round keeps it required. Run state persists to a cwd/worktree-anchored `.prflow/tmp/issue-audit-state-.json`, replacing the markdown event log the offer used to read — the skill still writes the observable audit artifact (`.prflow/tmp/issue-audit-.md`, same gate/read-only-stand-in convention as the derivation artifact), and both state paths plus the **retired** `.md` leftover stay declared out of bounds so an auditor with repository read access cannot re-anchor on this run's prior verdicts. **Writes to that state document are serialized (issue #1040).** Every mutating subcommand runs inside an exclusive-create sentinel critical section — a `.lock` file beside the state document, created with `os.O_CREAT | os.O_EXCL` (standard-library only) — so two concurrent invocations for the same slug produce a document reflecting one of them entirely and then the other, never an interleaved mixture; each writer also persists through its own unique `tempfile.mkstemp` temporary path (with a bounded `os.replace` retry over `PermissionError`) so two writers never share and truncate one temp file. **Read-only subcommands acquire no sentinel and stay unserialized** (`query-*`, `emit-body`, `check-claim-staleness`), and a fail-closed transitive call-graph check proves `save_state` is unreachable from every read-only-classified subcommand. An **abandoned sentinel is recovered by age**, and the threshold that decides it is `stale_after_s` — *not* the longer `acquire_window_s` a contending writer is willing to wait: a sentinel whose mtime age exceeds `stale_after_s` is broken and the break is re-attempted once. Because the shipped `stale_after_s` is strictly shorter than the shipped `acquire_window_s`, a writer contending with an abandoned sentinel always reaches that break inside its own acquire window, which is what makes a crashed writer unable to wedge the slug permanently; the acquire-window expiry is the fail-closed arm for a host whose overrides invert that relation, and it refuses the mutation rather than proceeding. That relation is the load-bearing part — an override that sets `stale_after_s` above `acquire_window_s` trades permanent-wedge immunity for the refusal. This guarantee is **document integrity only**. The **decision channel is a distinct quantity with a named residual**: because `_emit_next_call` re-reads the state after the critical section has released, 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 happened to observe — so those answers are **not authoritative under concurrent invocation**, even though the persisted document they read is always internally consistent. §11 is the canonical home for this system contract; the skill's operational rule points here rather than restating it. Only the current draft counts as audited: eligibility is grounded on a completed clean-verdict round whose recorded identity still holds — byte-digest equality against the canonical file on file-arm epochs, revision ordering on the embed and inline arms where no trustworthy file exists — or on an explicitly recorded override that no later revision has invalidated, and every `eligible` answer carries a deterministic token bound to the answering digest or revision ordinal (matching the ground that answered) which the summary line quotes verbatim. That **narrows** the prose-compliance gap and makes a skipped eligibility check detectable in the transcript; it does not close it, since no in-process component can force an orchestrator that never invokes it. Where the tool cannot run at all, a named bounded fallback runs one round, asks once, and marks the summary line `state-owner unavailable` — distinct from `degraded`, which keeps its meaning of the inline audit arm. +**The lifecycle itself is owned by a tested state-owner CLI, not by prose (issue #546).** Every deterministic rule above — transition legality, round numbering, the automatic budget and the bounded retries, arm routing and its three embed markers, digest computation and comparison, sentinel generation and comparison, the offer triggers, override records, presentation eligibility, and the audit-summary field set — is executed by `scripts/issue-audit-state.py`; the skill records lifecycle events through it and **obeys its answers** rather than re-deriving them each turn. Its two-class contract is what the prose branches on: queries always exit 0 once the arguments parse (an argparse usage error still exits 2) with a decided answer line, except for the multi-line read-back queries `query-findings`, issue #704's `query-claim-baselines` / `query-finding-evidence`, `query-coverage`, and `query-adjudication-records`, which print one decided line per record, and issue #795's composite `query-boundary`, which prints one decided line per boundary component; all of them are strictly read-only, while mutations exit non-zero with a named breadcrumb on an illegal transition or an unpersistable state. Since issue #795 most subcommands print a second and final `next_call=` line naming the next legal invocation, with every state-derivable operand filled and every caller-supplied one bare in a `needs=` field; it is a generated suggestion the caller reviews before running, never an instruction, and the decided answer line is unchanged and stays first. The same issue lets the five subcommands whose round the state uniquely determines resolve an omitted `--round` from state, while every subcommand where the flag selects an operation or names a caller-chosen round keeps it required. Run state persists to a cwd/worktree-anchored `.prflow/tmp/issue-audit-state-.json`, replacing the markdown event log the offer used to read — the skill still writes the observable audit artifact (`.prflow/tmp/issue-audit-.md`, same gate/read-only-stand-in convention as the derivation artifact), and both state paths plus the **retired** `.md` leftover stay declared out of bounds so an auditor with repository read access cannot re-anchor on this run's prior verdicts. **Writes to that state document are serialized (issue #1040).** Every mutating subcommand runs inside an exclusive-create sentinel critical section — a `.lock` file beside the state document, created with `os.O_CREAT | os.O_EXCL` (standard-library only) — so two concurrent invocations for the same slug produce a document reflecting one of them entirely and then the other, never an interleaved mixture; each writer also persists through its own unique `tempfile.mkstemp` temporary path (with a bounded `os.replace` retry over `PermissionError`) so two writers never share and truncate one temp file. **Read-only subcommands acquire no sentinel and stay unserialized** (`query-*`, `emit-body`, `check-claim-staleness`), and a fail-closed transitive call-graph check proves `save_state` is unreachable from every read-only-classified subcommand. An **abandoned sentinel is recovered by age**, and the threshold that decides it is `stale_after_s` — *not* the longer `acquire_window_s` a contending writer is willing to wait: a sentinel whose mtime age exceeds `stale_after_s` is unlinked, and the exclusive create is then re-attempted exactly once before control returns to the ordinary acquire loop. Because the shipped `stale_after_s` is strictly shorter than the shipped `acquire_window_s`, a writer contending with an abandoned sentinel always reaches that break inside its own acquire window, which is what makes a crashed writer unable to wedge the slug permanently; the acquire-window expiry is the fail-closed arm for a host whose overrides invert that relation, and it refuses the mutation rather than proceeding. That relation is the load-bearing part — an override that sets `stale_after_s` above `acquire_window_s` trades permanent-wedge immunity for the refusal. This guarantee is **document integrity only**. The **decision channel is a distinct quantity with a named residual**: because `_emit_next_call` re-reads the state after the critical section has released, 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 happened to observe — so those answers are **not authoritative under concurrent invocation**, even though the persisted document they read is always internally consistent. §11 is the canonical home for this system contract; the skill's operational rule points here rather than restating it. Only the current draft counts as audited: eligibility is grounded on a completed clean-verdict round whose recorded identity still holds — byte-digest equality against the canonical file on file-arm epochs, revision ordering on the embed and inline arms where no trustworthy file exists — or on an explicitly recorded override that no later revision has invalidated, and every `eligible` answer carries a deterministic token bound to the answering digest or revision ordinal (matching the ground that answered) which the summary line quotes verbatim. That **narrows** the prose-compliance gap and makes a skipped eligibility check detectable in the transcript; it does not close it, since no in-process component can force an orchestrator that never invokes it. Where the tool cannot run at all, a named bounded fallback runs one round, asks once, and marks the summary line `state-owner unavailable` — distinct from `degraded`, which keeps its meaning of the inline audit arm. **Runtime main-thread context (issue #767).** Separate from the skill's *structure* and *static shipped size* discussed above, a long create-issue run accumulates a large **runtime main-thread context** across its many turns. The behavioral instrument `scripts/create-issue-context-eval.py` (maintainer-run over a transcript corpus; never on the skill's runtime path, so no new tool grant) measures it, and the determination of which appended-content classes are authoritative versus safely-removable redundant additions — with the reduction that removes the primary safely-removable class (re-emission of an already-produced block, replaced by a pointer) — lives in [`docs/create-issue-context.md`](create-issue-context.md). That doc is the single source of truth for this axis; it is not paraphrased here. From cdf1d45985f11d6a6ee80c429af6821c8b09d0ee Mon Sep 17 00:00:00 2001 From: The01Geek Date: Fri, 31 Jul 2026 23:21:35 -0600 Subject: [PATCH 7/9] fix: route all three stdin consumers through the shared guard (#1040 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses both review findings. Both are in the stdin-hoist refactor, and both are cases where the hoist made a site QUIETER than the bare `sys.stdin` read it replaced. Must-fix — cmd_check_claim_staleness silently swallowed a read failure. `domain = args._stdin_data` checked neither `_stdin_error` nor `_stdin_missing`, so a mid-read OSError (loud before the hoist) and a closed fd 0 both collapsed to `domain = None` — the SAME value the intentional no---domain-stdin case produces. The handler then scored every claim against a domain search that never ran and printed a decided staleness line at exit 0, indistinguishable from a real answer. Should-fix — cmd_record_claim_baseline misdiagnosed a closed fd 0. It checked `_stdin_error` but not `_stdin_missing`, so an unattached fd 0 fell through to `domain-class-empty-domain: --domain-stdin produced no bytes; a search that emitted nothing cannot identify a baseline` — a breadcrumb that positively asserts a search ran and returned nothing. cmd_record_finding_evidence had the same gap and degraded to a bare AttributeError from `None.decode`. All three now consume the hoisted bytes through `_stdin_bytes_or_fail`, which already checked both conditions and emitted the named breadcrumbs. That removes the two hand-written `_stdin_error` checks, so the mechanism has ONE guard implementation rather than three partial ones — the inconsistency was the defect class, not just its two instances. Every breadcrumb is byte-identical to what the guarded sites already emitted, and the empty-read contract is untouched: record-finding-evidence still RECORDS an attached-but-empty read as incomplete (issue #704), and record-claim-baseline still refuses one as an empty domain. Scope: proven confined to the three handlers. With docstrings stripped, an AST comparison against the previous commit reports exactly cmd_check_claim_staleness, cmd_record_claim_baseline and cmd_record_finding_evidence changed, and nothing else — the locking mechanism, the ownership token, _replace_with_retry, _read_stdin_once, _stdin_bytes_or_fail, _selects_stdin and the AST call-graph check are byte-identical. Tests (all new; 16 of them are RED against the pre-fix module, verified by reverting scripts/issue-audit-state.py and re-running): - the three handlers driven in-process for both unreadable conditions, plus positive controls proving the no-flag, empty-read and normal paths still work; - the same three driven END-TO-END through the real CLI with fd 0 genuinely closed via `0<&-` (not /dev/null, which is an open descriptor at EOF), which is the only coverage that proves the whole chain from a real closed fd 0 to the named refusal; - the mid-read OSError arm of _read_stdin_once and of the shared guard — the `_stdin_error` field was written and read but no test ever set it; - _selects_stdin's False and attribute-ABSENT branches for all six flag-gated commands, each with a True positive control (only record-claim-baseline had a False row, so an inverted condition on the other five passed the suite); - _try_create's post-open failure path for both os.fstat and os.write, asserting the cannot-persist StateError routing AND that the partial sentinel is unlinked. The issue-audit-state module floor is unchanged at 238: these tests live in lib/test/test_python_scripts.py, not in that shell module. Co-Authored-By: Claude Opus 5 (1M context) --- lib/test/test_python_scripts.py | 319 ++++++++++++++++++++++++++++++++ scripts/issue-audit-state.py | 53 +++--- 2 files changed, 349 insertions(+), 23 deletions(-) diff --git a/lib/test/test_python_scripts.py b/lib/test/test_python_scripts.py index e227b0951..7e1103c4a 100755 --- a/lib/test/test_python_scripts.py +++ b/lib/test/test_python_scripts.py @@ -7415,6 +7415,30 @@ def _ns(**kw): assert_eq("#1040 _selects_stdin: an unrelated command selects no stdin", False, issue_audit_state._selects_stdin(_ns(cmd='query-summary'))) +# The FALSE branch of every flag-gated selector. Before this only record-claim-baseline had +# one, so an inverted condition (`not getattr(...)`) on any of the other five passed the +# suite: each TRUE row above stays green under inversion only if some row also pins the +# unset flag, and none did. Both unset shapes are driven, because `_selects_stdin` reads +# every flag through `getattr(args, flag, False)` and the two reach that default by +# different routes — the flag present and False (the parser's own `store_true` default), +# and the attribute ABSENT entirely (a sibling subcommand's Namespace, which is what +# main() hands it when the flag belongs to a different parser). A True row accompanies each +# pair as the positive control, so an always-False regression cannot pass the block. +for _cmd1040, _flag1040 in (('record-revision', 'stdin_digest'), + ('record-adjudication', 'ledger_stdin'), + ('record-coverage', 'coverage_stdin'), + ('record-claim-baseline', 'domain_stdin'), + ('check-claim-staleness', 'domain_stdin'), + ('record-finding-evidence', 'observed_stdin')): + assert_eq(f"#1040 _selects_stdin: {_cmd1040} with --{_flag1040} present-but-False " + f"selects no stdin", False, + issue_audit_state._selects_stdin(_ns(**{'cmd': _cmd1040, _flag1040: False}))) + assert_eq(f"#1040 _selects_stdin: {_cmd1040} with {_flag1040} ABSENT selects no stdin", + False, issue_audit_state._selects_stdin(_ns(cmd=_cmd1040))) + assert_eq(f"#1040 _selects_stdin: {_cmd1040} with {_flag1040} set DOES select stdin " + f"(positive control — the two rows above are not vacuously False)", True, + issue_audit_state._selects_stdin(_ns(**{'cmd': _cmd1040, _flag1040: True}))) + # stdin_hoist_preserves_the_absent_stdin_guard: with sys.stdin None and a stdin-selecting # command, _read_stdin_once records the closed-fd condition and _stdin_bytes_or_fail raises # the SAME named breadcrumb (a SystemExit via _fail) — never an AttributeError traceback. @@ -7442,6 +7466,248 @@ def _ns(**kw): finally: sys.stdin = _orig_stdin + +# ── the MID-READ OSError arm (issue #1040 review) ────────────────────────────────── +# `_stdin_error` is written by `_read_stdin_once` and read by the shared guard, but the only +# arm any row drove was the closed-fd-0 one above: deleting `_read_stdin_once`'s +# `except OSError` clause, or the guard's `_stdin_error` check, left the suite green. This +# is the EIO/EPIPE shape — fd 0 is attached and the read fails part way — which is a +# different condition from an absent fd 0 and must not be laundered into it. +class _RaisingStdin1040: + class _Buf: + @staticmethod + def read(): + raise OSError(5, 'Input/output error') + + buffer = _Buf() + + +_orig_stdin = sys.stdin +try: + sys.stdin = _RaisingStdin1040() + _err1040 = _ns(cmd='check-claim-staleness', domain_stdin=True) + issue_audit_state._read_stdin_once(_err1040) + assert_eq("#1040 stdin_read_error: a mid-read OSError is recorded on args", True, + isinstance(_err1040._stdin_error, OSError)) + assert_eq("#1040 stdin_read_error: ... and is NOT collapsed onto the closed-fd-0 " + "condition (they are different diagnoses)", False, _err1040._stdin_missing) + assert_eq("#1040 stdin_read_error: ... and no partial payload is presented as a value", + None, _err1040._stdin_data) +finally: + sys.stdin = _orig_stdin + +_errbuf1040 = io.StringIO() +_raised1040 = False +try: + with contextlib.redirect_stderr(_errbuf1040): + issue_audit_state._stdin_bytes_or_fail(_err1040, 'check-claim-staleness', + 'the domain search result') +except SystemExit: + _raised1040 = True +assert_eq("#1040 stdin_read_error: the shared guard fails closed (SystemExit) on a recorded " + "read error", True, _raised1040) +assert_eq("#1040 stdin_read_error: ... naming the real errno rather than an absent-or-empty " + "claim about the payload", True, + 'could not read the domain search result from stdin' in _errbuf1040.getvalue() + and 'Input/output error' in _errbuf1040.getvalue()) + + +def _drive1040(fn, args, root): + """Run a handler with args._stdin_* pre-populated, as main()'s hoist would leave them. + + Returns (ending, stderr, stdout). `ending` is a STRING rather than a raised/not-raised + boolean so a row distinguishes a named refusal from an incidental crash: before the + routing fix two of these sites ended `AttributeError`, which a truthy "did it raise" + assertion would have accepted as a pass. + """ + _orig_root = issue_audit_state._repo_root + _out, _err = io.StringIO(), io.StringIO() + try: + issue_audit_state._repo_root = lambda: root + with contextlib.redirect_stdout(_out), contextlib.redirect_stderr(_err): + try: + fn(args) + ending = 'returned' + except SystemExit as _e: + ending = f'SystemExit({_e.code})' + except Exception as _e: # noqa: BLE001 - the ending is the assertion operand + ending = type(_e).__name__ + finally: + issue_audit_state._repo_root = _orig_root + return ending, _err.getvalue(), _out.getvalue() + + +_IOERR1040 = OSError(5, 'Input/output error') + +# cmd_check_claim_staleness — the silent swallow (review Must-fix). A read failure produced +# `domain = None`, which is the SAME value the intentional no---domain-stdin case produces, +# so the handler scored every claim against a search that never ran and printed a DECIDED +# staleness line at exit 0. Nothing downstream could tell that from a real answer. +with tempfile.TemporaryDirectory() as _td: + _R = Path(_td) + _doc1040 = _state([]) + _doc1040['claims'] = {'k': {'claim_class': 'count', 'revision': 'r1', + 'identity': 'ID1'}} + issue_audit_state.save_state(_doc1040, 's', root=_R) + + _end, _err, _out = _drive1040( + issue_audit_state.cmd_check_claim_staleness, + _ns(cmd='check-claim-staleness', slug='s', nonce='n0', claim_key='k', + domain_stdin=True, _stdin_data=None, _stdin_missing=False, + _stdin_error=_IOERR1040), _R) + assert_eq("#1040 staleness_read_error: a mid-read OSError exits non-zero, never a " + "decided staleness line at exit 0", 'SystemExit(1)', _end) + assert_eq("#1040 staleness_read_error: ... naming the read failure and its errno", True, + 'could not read the domain search result from stdin' in _err + and 'Input/output error' in _err) + assert_eq("#1040 staleness_read_error: ... and NO staleness verdict is printed", '', _out) + + _end, _err, _out = _drive1040( + issue_audit_state.cmd_check_claim_staleness, + _ns(cmd='check-claim-staleness', slug='s', nonce='n0', claim_key='k', + domain_stdin=True, _stdin_data=None, _stdin_missing=True, + _stdin_error=None), _R) + assert_eq("#1040 staleness_closed_fd: a closed fd 0 exits non-zero", 'SystemExit(1)', _end) + assert_eq("#1040 staleness_closed_fd: ... naming the absent stdin", True, + 'no stdin is attached (fd 0 is closed)' in _err) + assert_eq("#1040 staleness_closed_fd: ... and NO staleness verdict is printed", '', _out) + + # Positive control: with the flag absent the handler still answers normally, so the two + # rows above lock out a failure rather than the feature. `_read_stdin_once` never reads + # in this shape, so all three fields hold their defaults exactly as main() leaves them. + _end, _err, _out = _drive1040( + issue_audit_state.cmd_check_claim_staleness, + _ns(cmd='check-claim-staleness', slug='s', nonce='n0', claim_key='k', + domain_stdin=False, _stdin_data=None, _stdin_missing=False, + _stdin_error=None), _R) + assert_eq("#1040 staleness_no_flag: without --domain-stdin the handler answers normally", + 'returned', _end) + assert_eq("#1040 staleness_no_flag: ... printing its decided staleness line", True, + _out.startswith('claim=k class=count state=')) + + # cmd_record_claim_baseline — the false diagnosis (review Should-fix). A closed fd 0 fell + # through to `domain-class-empty-domain: --domain-stdin produced no bytes; a search that + # emitted nothing cannot identify a baseline`, which ASSERTS that a search ran and + # returned nothing. Both halves are pinned: the right breadcrumb appears AND the wrong + # one does not, since the fix would be worthless if it merely added a second line. + _end, _err, _out = _drive1040( + issue_audit_state.cmd_record_claim_baseline, + _ns(cmd='record-claim-baseline', slug='s', nonce='n0', claim_key='k2', + claim_class='count', path=[], domain_stdin=True, _stdin_data=None, + _stdin_missing=True, _stdin_error=None), _R) + assert_eq("#1040 baseline_closed_fd: a closed fd 0 exits non-zero", 'SystemExit(1)', _end) + assert_eq("#1040 baseline_closed_fd: ... naming the absent stdin", True, + 'no stdin is attached (fd 0 is closed)' in _err) + assert_eq("#1040 baseline_closed_fd: ... and NOT as an empty search result, which would " + "assert a search that never ran", False, 'domain-class-empty-domain' in _err) + + # ... while a genuinely EMPTY read still earns the empty-domain refusal: the fix + # separates "never attached" from "attached and returned nothing", so this row proves + # the second diagnosis survives rather than being swallowed by the first. + _end, _err, _out = _drive1040( + issue_audit_state.cmd_record_claim_baseline, + _ns(cmd='record-claim-baseline', slug='s', nonce='n0', claim_key='k2', + claim_class='count', path=[], domain_stdin=True, _stdin_data=b'', + _stdin_missing=False, _stdin_error=None), _R) + assert_eq("#1040 baseline_empty_read: an attached-but-empty stdin still exits non-zero", + 'SystemExit(1)', _end) + assert_eq("#1040 baseline_empty_read: ... and KEEPS the empty-domain diagnosis", True, + 'domain-class-empty-domain' in _err) + + _end, _err, _out = _drive1040( + issue_audit_state.cmd_record_claim_baseline, + _ns(cmd='record-claim-baseline', slug='s', nonce='n0', claim_key='k2', + claim_class='count', path=[], domain_stdin=True, _stdin_data=None, + _stdin_missing=False, _stdin_error=_IOERR1040), _R) + assert_eq("#1040 baseline_read_error: a mid-read OSError exits non-zero", 'SystemExit(1)', + _end) + assert_eq("#1040 baseline_read_error: ... naming the errno, not the empty-domain claim", + True, 'Input/output error' in _err + and 'domain-class-empty-domain' not in _err) + + # cmd_record_finding_evidence — the third consumer. A closed fd 0 reached `raw.decode` + # as None and died with a bare AttributeError, which names nothing and discards the + # condition. The ending is compared as a STRING so this row cannot be satisfied by the + # crash it exists to remove. + def _ev1040(**kw): + base = dict(cmd='record-finding-evidence', slug='s', nonce='n0', round=1, + finding_id=1, observed_stdin=True, locator='src/x.py:1', + command='grep -c x', baseline_revision=None, baseline_identity=None, + _stdin_data=None, _stdin_missing=False, _stdin_error=None) + base.update(kw) + return _ns(**base) + + _end, _err, _out = _drive1040(issue_audit_state.cmd_record_finding_evidence, + _ev1040(_stdin_missing=True), _R) + assert_eq("#1040 evidence_closed_fd: a closed fd 0 is a named refusal, NOT the bare " + "AttributeError the decode used to raise", 'SystemExit(1)', _end) + assert_eq("#1040 evidence_closed_fd: ... naming the absent stdin", True, + 'no stdin is attached (fd 0 is closed)' in _err) + + _end, _err, _out = _drive1040(issue_audit_state.cmd_record_finding_evidence, + _ev1040(_stdin_error=_IOERR1040), _R) + assert_eq("#1040 evidence_read_error: a mid-read OSError is a named refusal", + 'SystemExit(1)', _end) + assert_eq("#1040 evidence_read_error: ... naming the errno", True, + 'could not read the observed output from stdin' in _err + and 'Input/output error' in _err) + + # Positive control for BOTH evidence rows: an attached, EMPTY stdin is still recorded + # (issue #704 requires absent/incomplete evidence to be RECORDED incomplete, never + # refused), so the two refusals above are not a blanket "any falsy payload fails". + _end, _err, _out = _drive1040(issue_audit_state.cmd_record_finding_evidence, + _ev1040(_stdin_data=b''), _R) + assert_eq("#1040 evidence_empty_read: an attached-but-empty stdin is still RECORDED, " + "not refused", 'returned', _end) + + +# _try_create's post-open failure path: an OSError raised AFTER the exclusive create +# succeeded (an ENOSPC on the pid write, a failing fstat). Untested before — the arm both +# unlinks the partial sentinel and converts the error into the cannot-persist vocabulary, +# and deleting either half left the suite green while a crashed create would have parked a +# sentinel nothing could distinguish from a live holder until it aged out. +class _OsShim1040: + """The real `os`, with exactly one attribute replaced by a raising stub.""" + + def __init__(self, fail_on): + self._fail_on = fail_on + + def __getattr__(self, name): + if name == self._fail_on: + def _raise(*_a, **_k): + raise OSError(28, 'No space left on device') + return _raise + return getattr(os, name) + + +for _failing1040 in ('fstat', 'write'): + with tempfile.TemporaryDirectory() as _td: + _R = Path(_td) + _sentinel1040 = _ias1040_sentinel(_R) + os.makedirs(os.path.dirname(_sentinel1040), exist_ok=True) + _sec1040 = issue_audit_state._StateSection('s', root=_R) + _ending1040, _msg1040 = 'returned', '' + _orig_os1040 = issue_audit_state.os + try: + issue_audit_state.os = _OsShim1040(_failing1040) + try: + _sec1040._try_create() + except issue_audit_state.StateError as _e: + _ending1040, _msg1040 = 'StateError', str(_e) + except Exception as _e: # noqa: BLE001 - the ending is the assertion operand + _ending1040 = type(_e).__name__ + finally: + issue_audit_state.os = _orig_os1040 + assert_eq(f"#1040 try_create_post_open_failure (os.{_failing1040}): an OSError after " + f"the exclusive create routes as a cannot-persist StateError, never a raw " + f"traceback", 'StateError', _ending1040) + assert_eq(f"#1040 try_create_post_open_failure (os.{_failing1040}): ... carrying the " + f"could-not-persist prefix the mutation contract routes on", True, + _msg1040.startswith('could not persist state to ')) + assert_eq(f"#1040 try_create_post_open_failure (os.{_failing1040}): ... and the " + f"partial sentinel is unlinked, so it cannot block later acquires until " + f"it ages out", False, os.path.exists(_sentinel1040)) + # stdin_is_read_before_acquire: main() reads stdin BEFORE entering the section. Proven at # the source level (an executable-order pin): _read_stdin_once precedes the _StateSection # `with` in main(). @@ -15121,6 +15387,59 @@ def _row704_30(r): _with_run704(_row704_30) +# ── issue #1040 (review): the stdin consumers refuse a genuinely CLOSED fd 0 ────────── +# End-to-end through the REAL CLI with fd 0 actually closed, rather than fabricating +# `_stdin_missing` on a Namespace. This is the row that proves the whole chain rather than +# one link of it: a real closed fd 0 makes CPython bind `sys.stdin` to None, which is what +# `_read_stdin_once` tests for, which is what the shared guard reads. `0<&-` is required — +# `/dev/null` and `subprocess.DEVNULL` both hand the process an OPEN descriptor at EOF, +# which is the empty-read case these three handlers treat differently and correctly. +# POSIX-only (the redirection is `sh` syntax); the in-process rows above cover Windows. +if os.name != 'nt': + def _row1040_e2e(r): + def _closed_fd0(*argv): + return _subprocess.run( + ['sh', '-c', 'exec "$@" 0<&-', 'sh', sys.executable, _IAS603, *argv, + '--nonce', r.nonce], + cwd=r.tmp, capture_output=True, text=True) + + r.write('anchor.md', 'alpha\n') + r.commit('A: add anchor') + _fix = r.baseline('c1', 'count', domain='hit-a\nhit-b\n') + assert_eq("#1040-e2e fixture: the count baseline records (guards the staleness row " + "below against passing on an absent claim)", 0, _fix.returncode) + + _got = _closed_fd0('check-claim-staleness', r.slug, '--claim-key', 'c1', + '--domain-stdin') + assert_eq("#1040-e2e: check-claim-staleness with fd 0 CLOSED exits non-zero, never " + "a decided staleness line at exit 0", 1, _got.returncode) + assert_eq("#1040-e2e: ... naming the absent stdin", True, + 'no stdin is attached (fd 0 is closed)' in _got.stderr) + assert_eq("#1040-e2e: ... and printing NO staleness verdict", '', _got.stdout) + + _got = _closed_fd0('record-claim-baseline', r.slug, '--claim-key', 'c2', + '--claim-class', 'count', '--domain-stdin') + assert_eq("#1040-e2e: record-claim-baseline with fd 0 CLOSED exits non-zero", + 1, _got.returncode) + assert_eq("#1040-e2e: ... naming the absent stdin, and NOT diagnosing it as an " + "empty search result (which asserts a search that never ran)", + (True, False), + ('no stdin is attached (fd 0 is closed)' in _got.stderr, + 'domain-class-empty-domain' in _got.stderr)) + + _got = _closed_fd0('record-finding-evidence', r.slug, '--round', '1', + '--finding-id', '1', '--observed-stdin') + assert_eq("#1040-e2e: record-finding-evidence with fd 0 CLOSED exits non-zero", + 1, _got.returncode) + assert_eq("#1040-e2e: ... naming the absent stdin", True, + 'no stdin is attached (fd 0 is closed)' in _got.stderr) + assert_eq("#1040-e2e: ... with NO Python traceback reaching the operator (the bare " + "AttributeError the None decode used to raise)", False, + 'Traceback (most recent call last)' in _got.stderr) + + _with_run704(_row1040_e2e) + + # ── issue #709: steering-absence establishment ──────────────────────────────── # The Move-3 named assertions from the issue, driven END-TO-END through the CLI over a # real generated instruction file — not against hand-built state — because the whole diff --git a/scripts/issue-audit-state.py b/scripts/issue-audit-state.py index bddf06393..195c6bf45 100644 --- a/scripts/issue-audit-state.py +++ b/scripts/issue-audit-state.py @@ -7947,17 +7947,15 @@ def cmd_record_claim_baseline(args): f'by its re-executed full-domain search result; pipe it with ' f'--domain-stdin') # The domain search result was read from stdin, hoisted into main() above the - # section (issue #1040). A mid-read OSError the hoist captured must name its real - # cause here rather than be laundered into the domain-class-empty-domain refusal - # below (which would hand downstream triage a trusted "empty search" token for a - # condition that never occurred). This site never carried the fd-0-closed guard, so - # a closed fd 0 leaves args._stdin_data None and falls through to - # domain-class-empty-domain (a clean named breadcrumb where an in-handler bare read - # would have crashed with an AttributeError) — a deliberate, minor improvement. - if args._stdin_error is not None: - _fail(prefix, 'could not read the domain search result from stdin: ' - f'{args._stdin_error}') - data = args._stdin_data + # section (issue #1040), and consumed through the SHARED guard so that BOTH + # unreadable conditions — a closed fd 0 and a mid-read OSError — name their own + # cause. Neither may reach the domain-class-empty-domain refusal below: that + # breadcrumb asserts "a search that emitted nothing", a positive claim about a + # search that actually ran, so emitting it when stdin was never attached (or the + # read failed part-way) hands downstream triage a trusted token for a condition + # that never occurred — the never-attempted misattribution this module refuses to + # make elsewhere. Reading `args._stdin_data` bare here is what produced it. + data = _stdin_bytes_or_fail(args, prefix, 'the domain search result') if not data: _fail(prefix, 'domain-class-empty-domain: --domain-stdin produced no bytes; a ' 'search that emitted nothing cannot identify a baseline') @@ -8064,10 +8062,20 @@ def cmd_check_claim_staleness(args): keys = sorted(claims) # The domain search result was read from stdin, hoisted into main() above dispatch # (issue #1040). check-claim-staleness is read-only (no section), but the read still - # moves to main() so no handler touches sys.stdin. _read_stdin_once reads only when - # --domain-stdin is selected, so args._stdin_data is None otherwise — equivalent to the - # former `... if args.domain_stdin else None`. - domain = args._stdin_data + # moves to main() so no handler touches sys.stdin. It goes through the SHARED guard + # rather than reading `args._stdin_data` bare, because on this subcommand a swallowed + # read failure is invisible by construction: the laundered value is None, which is the + # SAME value the intentional "no --domain-stdin" case produces, so a domain search that + # never ran is indistinguishable from one that was never asked for, and every claim is + # then scored against `domain=None` and printed as a decided staleness verdict. Reading + # it bare also made this the one stdin consumer QUIETER than before the hoist (a + # mid-read OSError used to propagate). `_read_stdin_once` reads only when + # --domain-stdin is selected, so with the flag absent all three fields hold their + # defaults and this returns None — equivalent to the former + # `... if args.domain_stdin else None`. Exiting non-zero here is in contract: this + # subcommand is neither a query nor a mutation and already exits non-zero on a + # caller-contract error (see the module docstring's check-claim-staleness carve-out). + domain = _stdin_bytes_or_fail(args, prefix, 'the domain search result') # Memoized across the loop: a path cited by several location claims is hashed once. cache = {} for key in keys: @@ -8116,14 +8124,13 @@ def cmd_record_finding_evidence(args): doc = _load_for_mutation(prefix, args.slug, args.nonce) observed = None if args.observed_stdin: - # Read from stdin, hoisted into main() above the section (issue #1040). A mid-read - # OSError the hoist captured must name its real cause rather than surface as a - # NoneType decode crash below that discards it. This site never carried the - # fd-0-closed guard, so a closed fd 0 still leaves args._stdin_data None and the - # decode below raises the same AttributeError the former bare sys.stdin read did. - if args._stdin_error is not None: - _fail(prefix, f'could not read the observed output from stdin: {args._stdin_error}') - raw = args._stdin_data + # Read from stdin, hoisted into main() above the section (issue #1040), and + # consumed through the SHARED guard so a mid-read OSError and a closed fd 0 alike + # name their own cause. Neither may reach the decode below as None: the OSError + # would surface as a NoneType AttributeError that discards the real errno, and the + # closed fd 0 did exactly that before this routing. An empty read is a different + # thing entirely and still reaches the decode (see the note below it). + raw = _stdin_bytes_or_fail(args, prefix, 'the observed output') # An empty read is NOT refused: issue #704 requires evidence that is absent or # incomplete to be RECORDED `incomplete` (never verified), which is what # `evidence_completeness` does with an empty `observed`. Refusing would record no From 55a5d00c132447e6975b4adc34a160bc5ecbd20d Mon Sep 17 00:00:00 2001 From: The01Geek Date: Fri, 31 Jul 2026 23:22:49 -0600 Subject: [PATCH 8/9] style: drop the self-referential field count from the two #1040 stdin comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both said "all three fields hold their defaults", an exact count of a same-file thing — the PR #553 rot class: adding or removing an `args._stdin_*` field silently falsifies the sentence. The #434 stale-prose lint flagged both as count-locked R3 advisories. Reworded count-free; comment text only, no executable line touched (AST comparison reports 0 definitions changed). Co-Authored-By: Claude Opus 5 (1M context) --- lib/test/test_python_scripts.py | 2 +- scripts/issue-audit-state.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/test/test_python_scripts.py b/lib/test/test_python_scripts.py index 7e1103c4a..cb2ecd1e1 100755 --- a/lib/test/test_python_scripts.py +++ b/lib/test/test_python_scripts.py @@ -7574,7 +7574,7 @@ def _drive1040(fn, args, root): # Positive control: with the flag absent the handler still answers normally, so the two # rows above lock out a failure rather than the feature. `_read_stdin_once` never reads - # in this shape, so all three fields hold their defaults exactly as main() leaves them. + # in this shape, so the fields it records keep the defaults main() leaves them at. _end, _err, _out = _drive1040( issue_audit_state.cmd_check_claim_staleness, _ns(cmd='check-claim-staleness', slug='s', nonce='n0', claim_key='k', diff --git a/scripts/issue-audit-state.py b/scripts/issue-audit-state.py index 195c6bf45..83ebb0de7 100644 --- a/scripts/issue-audit-state.py +++ b/scripts/issue-audit-state.py @@ -8070,8 +8070,8 @@ def cmd_check_claim_staleness(args): # then scored against `domain=None` and printed as a decided staleness verdict. Reading # it bare also made this the one stdin consumer QUIETER than before the hoist (a # mid-read OSError used to propagate). `_read_stdin_once` reads only when - # --domain-stdin is selected, so with the flag absent all three fields hold their - # defaults and this returns None — equivalent to the former + # --domain-stdin is selected, so with the flag absent the fields it records keep the + # defaults it set and this returns None — equivalent to the former # `... if args.domain_stdin else None`. Exiting non-zero here is in contract: this # subcommand is neither a query nor a mutation and already exits non-zero on a # caller-contract error (see the module docstring's check-claim-staleness carve-out). From 4d6a489c96fda7ed8319b55ffbf2c5f110e6ef4f Mon Sep 17 00:00:00 2001 From: The01Geek Date: Sat, 1 Aug 2026 00:47:13 -0600 Subject: [PATCH 9/9] fix: make section-release ownership unforgeable by inode reuse (#1040 re-review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding 1 (Important). __exit__ decided ownership by comparing the sentinel's (st_dev, st_ino) against the identity recorded at acquire. That is forgeable by the kernel: after an age break the breaker unlinks our inode and O_EXCL-creates its own file at the same path, and an inode-reusing filesystem may hand it the very identity we recorded — so the comparison matches a file we do not own and this section unlinks the LIVE holder's sentinel, exactly the outcome the check exists to prevent. _try_create now writes ` ` into the sentinel, where the nonce is 128 bits from 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?". The pid stays the FIRST field so the stale-break breadcrumb still names it, and a bare-pid body (a hand-planted fixture, and every pre-nonce writer) parses with owner=None, which no generated nonce equals — so such a sentinel is never adopted. self._token is assigned only after the write and close both succeed, so a section that failed mid-acquire owns nothing and releases nothing. Docstring and §11 corrected in the same change, because the STATED BOUND docstring's second acceptance ground — "a broken holder releases nothing and cannot strip the breaker's exclusion" — was FALSE under inode reuse. Both now name the nonce as what supports that claim, and both record that the identity form did not. The heartbeat-free residual is unchanged and is restated as such: the nonce bounds what a broken holder can destroy, not whether it can be broken. Tests, with mutation evidence from a faithful reproduction of the pre-fix check (identity stored in the same attribute, so the existing token-forcing test still applies) — 4 assertions RED, all new, no pre-existing test disturbed: - the inode-reuse defeat, driven DETERMINISTICALLY rather than probabilistically: the breaker's body is written IN PLACE, so its (st_dev, st_ino) is guaranteed equal to the one observed at acquire while its recorded owner is another holder's. The row asserts the identity really did match, so it cannot pass vacuously; - a bare-pid sentinel is not adopted; two acquisitions record different owners; the pid remains parseable as the first field; - _parse_sentinel_body over 13 body shapes (short, long, non-hex, uppercase, whitespace, empty, absent, over-cap); - an untouched sentinel IS still released (positive control). An existing row, release_does_not_unlink_a_foreign_sentinel, is corrected rather than left: its note said unlink+recreate was avoided because "a filesystem may immediately reuse" the freed inode. That reuse was not a test-only inconvenience, it was the defect, and side-stepping it is what kept it unobserved. It now forces a well-SHAPED foreign nonce (a wrong-typed sentinel value would compare unequal for the wrong reason and pass even with the owner comparison removed) and points at the deterministic rows for the real condition. Finding 2 (suspected over-grade, advisory). Added the N-subprocess stress row: 6 real processes record distinct claim keys on one slug concurrently, and the final document must load, validate, and carry ALL of them. Measured, not assumed — with serialization disabled only 1 of 6 claims survives, identically across 8 trials, while every writer still exits 0 with no traceback (the loss is silent, which is why the mechanism-only argument could not see it). On the fixed module: 6/6 across 12 trials. Assertions are on the final document, so the row cannot flake on a slow or fast host; it simply proves less on a run where the writers did not overlap. Scope: an AST comparison with docstrings stripped reports only _StateSection, _read_sentinel_pid and the two new helpers changed, plus the two new module constants. _replace_with_retry, _read_stdin_once, _stdin_bytes_or_fail, _selects_stdin, the AST call-graph check and the three stdin handlers are byte-identical. The issue-audit-state module floor is unchanged at 238. Co-Authored-By: Claude Opus 5 (1M context) --- docs/DEVFLOW_SYSTEM_OVERVIEW.md | 2 +- lib/test/test_python_scripts.py | 213 ++++++++++++++++++++++++++++++-- scripts/issue-audit-state.py | 122 +++++++++++++----- 3 files changed, 294 insertions(+), 43 deletions(-) diff --git a/docs/DEVFLOW_SYSTEM_OVERVIEW.md b/docs/DEVFLOW_SYSTEM_OVERVIEW.md index 59fae08c7..8f2145f24 100644 --- a/docs/DEVFLOW_SYSTEM_OVERVIEW.md +++ b/docs/DEVFLOW_SYSTEM_OVERVIEW.md @@ -635,7 +635,7 @@ The skill exists to prevent "option-listing" issues. Steps: 4. **Self-steelman (Step 3.5, mandatory — issue #304):** before the user sees the draft, the skill stress-tests it against the *actual code* — fresh targeted reads/greps, never the ambient context the draft came from. It verifies every load-bearing claim, file reference, and acceptance criterion; runs a **universal-quantifier sweep** (every "never/always/each/every/all/cannot" grounded — pinned per-arm, scoped, or removed — with a planted-defect positive-control obligation on detector-coverage claims); grounds every **occurrence count and coupled-site list** by an executed whitespace-normalized search or the cited evidence records, never recall; reads a "code does X" premise with its **enclosing gates/conditionals and their defaults**; hunts for missed ACs, missed edge cases, wrong assumptions, unstated scope, and an unreconciled multi-state contract (a within-text check that no summary or table form lists fewer causes for a state than the per-state ACs specify); it also flags an **AC mutual-consistency** conflict (no AC forbids a surface another AC's discharge must touch) and a **trust-boundary closure** gap (a protected executable-artifact set defined over the transitive source/exec/import closure of its entry points); binds every **stated-but-unbound input** (a mechanism input named only by role) to a named referent with a cited code reference or an implementer-obligation AC; revises the draft itself and re-runs the no-options gate on the revision; then — at every revision event, per the shared **Revision-delta verification** procedure (stated once, referenced by every revise-and-re-gate site) — walks the revision's edit-batch delta across six classes (mechanisms, lifecycle rules, execution-tier assumptions, dependencies, universal guarantees, and a total-making residual class) and verifies each non-empty class against the code, so audit rounds and the declined-re-audit filing path see delta that has been walked and verified rather than only language-gated; and reports a one-line outcome summary either way, including the universal-quantifier sweep's zero arm (a silent pass is indistinguishable from a skipped step). A genuinely new decision fork routes through the existing Step 2 question machinery, not a new path. 5. **Fresh-context audit (Step 3.6, mandatory — issue #443, extended by #522):** after Step 3.5 passes (which now also **self-checks the draft against the same audit dimension checklist** before any dispatch) and before the user sees the draft, the skill dispatches **one synchronous audit subagent** whose entire value is that it did **not** draft the issue — the mechanism is *information removal* (separated-context critics outperform same-session self-review on critical errors). On the normal **file arm**, the orchestrator writes the current rendered draft to the canonical `issue-draft-.md` before each round and the auditor **reads that file as the sole draft source** (closing the condensation-drift channel a hand-embedded copy opened) with a carriage/identity check; the drafting conversation, Step 1 findings, and the *reasoning* artifacts stay out of bounds while the draft file itself is the artifact under audit. When the write fails (read-only sandbox), a fallback **embed arm** carries the full body verbatim with its own sentinel carriage check, and the on-disk draft path stays out of bounds. The auditor runs an **adversarial pre-mortem** audit-prompt: a per-finding bar, scope exclusions at issue altitude, one assessed "Quiet Killer" slot, no cap on the number of findings paired with a per-finding length discipline, and a mandatory verdict line with three legal values — `VERDICT: FILE`, `VERDICT: REVISE`, or `VERDICT: DRAFT-UNREADABLE` (file arm only). On `REVISE` the orchestrator verifies each finding against the code, revises, re-runs the no-options gate, runs the shared **Revision-delta verification** procedure over the revision's delta (the same procedure the Step 3.5 and Step 4 revise loops reference), and re-audits **at most once** automatically — the audit informs, it never deadlocks filing. Past that automatic budget the skill **offers user-chosen rounds** (up to 3) via the question tool whenever the run is demonstrably unconverged — the user, not the skill, spends the tokens. Every accepted round's findings are **adjudicated** into must-revise / advisory / invalid; since issue #743 the advisory and invalid grades are no longer bare counts but carry a **durable per-finding record** — a one-line summary and rationale, an impact-class tag, and the auditor's returned finding block byte-preserved — recorded through the state owner (`record-adjudication --advisory-records-file/--invalid-records-file`, refused when a count and its records disagree), read back with `query-adjudication-records`, and rendered to the user **before** the approval election. A **calibration** layer (`query-calibration`, a `calibration=` sibling of the coverage boundary offer) surfaces an advisory grade on an impact-bearing finding (`implementation-correctness`/`scope`/`safety`/`verifiability`) that carries no recorded evidence, so an under-evidenced grade is named to the maintainer rather than silently converged past — disclosure only, **never a filing block**. Full evidence record: [`docs/advisory-adjudication-calibration.md`](advisory-adjudication-calibration.md). -**The lifecycle itself is owned by a tested state-owner CLI, not by prose (issue #546).** Every deterministic rule above — transition legality, round numbering, the automatic budget and the bounded retries, arm routing and its three embed markers, digest computation and comparison, sentinel generation and comparison, the offer triggers, override records, presentation eligibility, and the audit-summary field set — is executed by `scripts/issue-audit-state.py`; the skill records lifecycle events through it and **obeys its answers** rather than re-deriving them each turn. Its two-class contract is what the prose branches on: queries always exit 0 once the arguments parse (an argparse usage error still exits 2) with a decided answer line, except for the multi-line read-back queries `query-findings`, issue #704's `query-claim-baselines` / `query-finding-evidence`, `query-coverage`, and `query-adjudication-records`, which print one decided line per record, and issue #795's composite `query-boundary`, which prints one decided line per boundary component; all of them are strictly read-only, while mutations exit non-zero with a named breadcrumb on an illegal transition or an unpersistable state. Since issue #795 most subcommands print a second and final `next_call=` line naming the next legal invocation, with every state-derivable operand filled and every caller-supplied one bare in a `needs=` field; it is a generated suggestion the caller reviews before running, never an instruction, and the decided answer line is unchanged and stays first. The same issue lets the five subcommands whose round the state uniquely determines resolve an omitted `--round` from state, while every subcommand where the flag selects an operation or names a caller-chosen round keeps it required. Run state persists to a cwd/worktree-anchored `.prflow/tmp/issue-audit-state-.json`, replacing the markdown event log the offer used to read — the skill still writes the observable audit artifact (`.prflow/tmp/issue-audit-.md`, same gate/read-only-stand-in convention as the derivation artifact), and both state paths plus the **retired** `.md` leftover stay declared out of bounds so an auditor with repository read access cannot re-anchor on this run's prior verdicts. **Writes to that state document are serialized (issue #1040).** Every mutating subcommand runs inside an exclusive-create sentinel critical section — a `.lock` file beside the state document, created with `os.O_CREAT | os.O_EXCL` (standard-library only) — so two concurrent invocations for the same slug produce a document reflecting one of them entirely and then the other, never an interleaved mixture; each writer also persists through its own unique `tempfile.mkstemp` temporary path (with a bounded `os.replace` retry over `PermissionError`) so two writers never share and truncate one temp file. **Read-only subcommands acquire no sentinel and stay unserialized** (`query-*`, `emit-body`, `check-claim-staleness`), and a fail-closed transitive call-graph check proves `save_state` is unreachable from every read-only-classified subcommand. An **abandoned sentinel is recovered by age**, and the threshold that decides it is `stale_after_s` — *not* the longer `acquire_window_s` a contending writer is willing to wait: a sentinel whose mtime age exceeds `stale_after_s` is unlinked, and the exclusive create is then re-attempted exactly once before control returns to the ordinary acquire loop. Because the shipped `stale_after_s` is strictly shorter than the shipped `acquire_window_s`, a writer contending with an abandoned sentinel always reaches that break inside its own acquire window, which is what makes a crashed writer unable to wedge the slug permanently; the acquire-window expiry is the fail-closed arm for a host whose overrides invert that relation, and it refuses the mutation rather than proceeding. That relation is the load-bearing part — an override that sets `stale_after_s` above `acquire_window_s` trades permanent-wedge immunity for the refusal. This guarantee is **document integrity only**. The **decision channel is a distinct quantity with a named residual**: because `_emit_next_call` re-reads the state after the critical section has released, 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 happened to observe — so those answers are **not authoritative under concurrent invocation**, even though the persisted document they read is always internally consistent. §11 is the canonical home for this system contract; the skill's operational rule points here rather than restating it. Only the current draft counts as audited: eligibility is grounded on a completed clean-verdict round whose recorded identity still holds — byte-digest equality against the canonical file on file-arm epochs, revision ordering on the embed and inline arms where no trustworthy file exists — or on an explicitly recorded override that no later revision has invalidated, and every `eligible` answer carries a deterministic token bound to the answering digest or revision ordinal (matching the ground that answered) which the summary line quotes verbatim. That **narrows** the prose-compliance gap and makes a skipped eligibility check detectable in the transcript; it does not close it, since no in-process component can force an orchestrator that never invokes it. Where the tool cannot run at all, a named bounded fallback runs one round, asks once, and marks the summary line `state-owner unavailable` — distinct from `degraded`, which keeps its meaning of the inline audit arm. +**The lifecycle itself is owned by a tested state-owner CLI, not by prose (issue #546).** Every deterministic rule above — transition legality, round numbering, the automatic budget and the bounded retries, arm routing and its three embed markers, digest computation and comparison, sentinel generation and comparison, the offer triggers, override records, presentation eligibility, and the audit-summary field set — is executed by `scripts/issue-audit-state.py`; the skill records lifecycle events through it and **obeys its answers** rather than re-deriving them each turn. Its two-class contract is what the prose branches on: queries always exit 0 once the arguments parse (an argparse usage error still exits 2) with a decided answer line, except for the multi-line read-back queries `query-findings`, issue #704's `query-claim-baselines` / `query-finding-evidence`, `query-coverage`, and `query-adjudication-records`, which print one decided line per record, and issue #795's composite `query-boundary`, which prints one decided line per boundary component; all of them are strictly read-only, while mutations exit non-zero with a named breadcrumb on an illegal transition or an unpersistable state. Since issue #795 most subcommands print a second and final `next_call=` line naming the next legal invocation, with every state-derivable operand filled and every caller-supplied one bare in a `needs=` field; it is a generated suggestion the caller reviews before running, never an instruction, and the decided answer line is unchanged and stays first. The same issue lets the five subcommands whose round the state uniquely determines resolve an omitted `--round` from state, while every subcommand where the flag selects an operation or names a caller-chosen round keeps it required. Run state persists to a cwd/worktree-anchored `.prflow/tmp/issue-audit-state-.json`, replacing the markdown event log the offer used to read — the skill still writes the observable audit artifact (`.prflow/tmp/issue-audit-.md`, same gate/read-only-stand-in convention as the derivation artifact), and both state paths plus the **retired** `.md` leftover stay declared out of bounds so an auditor with repository read access cannot re-anchor on this run's prior verdicts. **Writes to that state document are serialized (issue #1040).** Every mutating subcommand runs inside an exclusive-create sentinel critical section — a `.lock` file beside the state document, created with `os.O_CREAT | os.O_EXCL` (standard-library only) — so two concurrent invocations for the same slug produce a document reflecting one of them entirely and then the other, never an interleaved mixture; each writer also persists through its own unique `tempfile.mkstemp` temporary path (with a bounded `os.replace` retry over `PermissionError`) so two writers never share and truncate one temp file. **Read-only subcommands acquire no sentinel and stay unserialized** (`query-*`, `emit-body`, `check-claim-staleness`), and a fail-closed transitive call-graph check proves `save_state` is unreachable from every read-only-classified subcommand. An **abandoned sentinel is recovered by age**, and the threshold that decides it is `stale_after_s` — *not* the longer `acquire_window_s` a contending writer is willing to wait: a sentinel whose mtime age exceeds `stale_after_s` is unlinked, and the exclusive create is then re-attempted exactly once before control returns to the ordinary acquire loop. Because the shipped `stale_after_s` is strictly shorter than the shipped `acquire_window_s`, a writer contending with an abandoned sentinel always reaches that break inside its own acquire window, which is what makes a crashed writer unable to wedge the slug permanently; the acquire-window expiry is the fail-closed arm for a host whose overrides invert that relation, and it refuses the mutation rather than proceeding. That relation is the load-bearing part — an override that sets `stale_after_s` above `acquire_window_s` trades permanent-wedge immunity for the refusal. **A broken holder releases nothing**, and the check that guarantees it compares an **owner nonce written into the sentinel**, not the sentinel's `(st_dev, st_ino)`: a writer unlinks on release only a sentinel whose recorded owner is still the nonce it wrote at acquire, so a holder whose sentinel was age-broken breadcrumbs instead of stripping the new holder's exclusion. The identity form of that check was unsound and is not what ships — the breaker unlinks the old inode and creates its own file at the same path, and an inode-reusing filesystem may hand it the very `(st_dev, st_ino)` the earlier holder recorded, so an identity comparison can match a file the releasing process does not own. The remaining residual is the one named above and is unchanged by this: exclusion is heartbeat-free, so a holder that occupies the section for longer than `stale_after_s` may still have its sentinel age-broken and overlap with the breaker — the nonce bounds what a broken holder can destroy, not whether it can be broken. This guarantee is **document integrity only**. The **decision channel is a distinct quantity with a named residual**: because `_emit_next_call` re-reads the state after the critical section has released, 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 happened to observe — so those answers are **not authoritative under concurrent invocation**, even though the persisted document they read is always internally consistent. §11 is the canonical home for this system contract; the skill's operational rule points here rather than restating it. Only the current draft counts as audited: eligibility is grounded on a completed clean-verdict round whose recorded identity still holds — byte-digest equality against the canonical file on file-arm epochs, revision ordering on the embed and inline arms where no trustworthy file exists — or on an explicitly recorded override that no later revision has invalidated, and every `eligible` answer carries a deterministic token bound to the answering digest or revision ordinal (matching the ground that answered) which the summary line quotes verbatim. That **narrows** the prose-compliance gap and makes a skipped eligibility check detectable in the transcript; it does not close it, since no in-process component can force an orchestrator that never invokes it. Where the tool cannot run at all, a named bounded fallback runs one round, asks once, and marks the summary line `state-owner unavailable` — distinct from `degraded`, which keeps its meaning of the inline audit arm. **Runtime main-thread context (issue #767).** Separate from the skill's *structure* and *static shipped size* discussed above, a long create-issue run accumulates a large **runtime main-thread context** across its many turns. The behavioral instrument `scripts/create-issue-context-eval.py` (maintainer-run over a transcript corpus; never on the skill's runtime path, so no new tool grant) measures it, and the determination of which appended-content classes are authoritative versus safely-removable redundant additions — with the reduction that removes the primary safely-removable class (re-emission of an already-produced block, replaced by a pointer) — lives in [`docs/create-issue-context.md`](create-issue-context.md). That doc is the single source of truth for this axis; it is not paraphrased here. diff --git a/lib/test/test_python_scripts.py b/lib/test/test_python_scripts.py index cb2ecd1e1..3f722a7ea 100755 --- a/lib/test/test_python_scripts.py +++ b/lib/test/test_python_scripts.py @@ -7241,17 +7241,23 @@ def _denied_open(*a, **k): finally: issue_audit_state.os.open = _orig_open -# release_does_not_unlink_a_foreign_sentinel: when the live sentinel's identity no longer -# matches the token this section recorded at acquire (an age-break by another process -# replaced it), release leaves the file in place and breadcrumbs. Driven by forcing the -# recorded ownership token to a value the live stat cannot match — deterministic, unlike an -# unlink+recreate whose freed inode a filesystem may immediately reuse. +# release_does_not_unlink_a_foreign_sentinel: when the live sentinel's recorded owner no +# longer matches the nonce this section wrote at acquire (an age-break by another process +# replaced it), release leaves the file in place and breadcrumbs. This is the smallest-scale +# unit form, driven by forcing the recorded token; the REAL condition — a breaker holding +# the same inode identity — is driven deterministically by the inode_reuse rows further +# down, which this row deliberately no longer stands in for. (Its earlier note called +# unlink+recreate non-deterministic "whose freed inode a filesystem may immediately reuse" +# and side-stepped it; that reuse was not a test-only inconvenience but the defect itself, +# so the avoidance is what kept it unobserved.) The forced value is a well-SHAPED foreign +# nonce, not a sentinel type like `(-1, -1)`: a wrong-typed token would compare unequal for +# the wrong reason and pass even if the owner comparison were removed. with tempfile.TemporaryDirectory() as _td: _R = Path(_td) _sent = _ias1040_sentinel(_R) _sec = issue_audit_state._StateSection('s', root=_R) _sec.__enter__() - _sec._token = (-1, -1) # the file at _sent is now "not ours" + _sec._token = 'f' * 32 # the file at _sent is now "not ours" _, _err = _ias1040_capture_stderr(lambda: _sec.__exit__(None, None, None)) assert_eq("#1040 release_does_not_unlink_a_foreign_sentinel: the foreign file is left " "in place", True, os.path.exists(_sent)) @@ -7662,10 +7668,12 @@ def _ev1040(**kw): # _try_create's post-open failure path: an OSError raised AFTER the exclusive create -# succeeded (an ENOSPC on the pid write, a failing fstat). Untested before — the arm both -# unlinks the partial sentinel and converts the error into the cannot-persist vocabulary, -# and deleting either half left the suite green while a crashed create would have parked a -# sentinel nothing could distinguish from a live holder until it aged out. +# succeeded. Every syscall the arm can now fail on is driven — the entropy draw for the +# owner nonce, the body write, and the close — because the arm does two things (unlink the +# partial sentinel, and convert the error into the cannot-persist vocabulary) and deleting +# either half left the suite green while a crashed create parked a sentinel nothing could +# distinguish from a live holder until it aged out. `close` is the arm reached from the +# inner `finally`, which is the one an eye-check most easily reads as uncovered. class _OsShim1040: """The real `os`, with exactly one attribute replaced by a raising stub.""" @@ -7680,7 +7688,7 @@ def _raise(*_a, **_k): return getattr(os, name) -for _failing1040 in ('fstat', 'write'): +for _failing1040 in ('urandom', 'write', 'close'): with tempfile.TemporaryDirectory() as _td: _R = Path(_td) _sentinel1040 = _ias1040_sentinel(_R) @@ -7708,6 +7716,127 @@ def _raise(*_a, **_k): f"partial sentinel is unlinked, so it cannot block later acquires until " f"it ages out", False, os.path.exists(_sentinel1040)) + +# ── issue #1040 (re-review): release ownership is CONTENT, not inode identity ────── +# `_parse_sentinel_body` establishes the two fields independently. The owner is +# shape-checked, so a truncated or garbled field reads unestablished (None) rather than +# being compared as data — and None is what makes a foreign or pre-nonce sentinel +# unadoptable, since __exit__ compares for equality against a nonce it generated. +for _body1040, _want1040 in ( + (b'4242', ('4242', None)), + (b'4242 ' + b'a' * 32, ('4242', 'a' * 32)), + (b'4242 ' + b'a' * 32 + b'\n', ('4242', 'a' * 32)), + (b' 4242 ', ('4242', None)), + (b'4242 ' + b'a' * 31, ('4242', None)), + (b'4242 ' + b'a' * 33, ('4242', None)), + (b'4242 ' + b'g' * 32, ('4242', None)), + (b'4242 ' + b'A' * 32, ('4242', None)), + (b'nope ' + b'a' * 32, (None, 'a' * 32)), + (b'', (None, None)), + (b' ', (None, None)), + (None, (None, None)), + (b'4' * (64 + 1), (None, None)), +): + assert_eq(f"#1040 sentinel_body: {_body1040!r} parses as {_want1040}", + _want1040, issue_audit_state._parse_sentinel_body(_body1040)) + +# The inode-reuse defeat. A contending writer age-breaks this section's sentinel (unlink of +# inode N) and O_EXCL-creates its own; on an inode-reusing filesystem the kernel may hand it +# the SAME (st_dev, st_ino) recorded at acquire, and an identity-based release check then +# unlinks the LIVE holder's sentinel. The simulation below is the LIMITING case of that and +# is deterministic rather than probabilistic: rather than unlinking and hoping the kernel +# recycles the inode number, the breaker's body is written IN PLACE, so the identity is +# GUARANTEED to equal the one observed at acquire while the recorded owner is another +# holder's. That is exactly the state __exit__ faces under real reuse, with the luck taken +# out — and it is RED against an (st_dev, st_ino) check by construction. +with tempfile.TemporaryDirectory() as _td: + _R = Path(_td) + _sent1040 = _ias1040_sentinel(_R) + _sec1040 = issue_audit_state._StateSection('s', root=_R) + _sec1040.__enter__() + _st = os.stat(_sent1040) + _ident_before1040 = (_st.st_dev, _st.st_ino) + _breaker_body1040 = b'99999 ' + b'b' * 32 + with open(_sent1040, 'r+b') as _fh1040: + _fh1040.seek(0) + _fh1040.truncate() + _fh1040.write(_breaker_body1040) + _st = os.stat(_sent1040) + assert_eq("#1040 inode_reuse: the simulated breaker really holds the SAME " + "(st_dev, st_ino) recorded at acquire — the row is not vacuous", + _ident_before1040, (_st.st_dev, _st.st_ino)) + _errbuf1040 = io.StringIO() + with contextlib.redirect_stderr(_errbuf1040): + _sec1040.__exit__(None, None, None) + assert_eq("#1040 inode_reuse: __exit__ does NOT unlink a live holder's sentinel that " + "occupies the recorded inode identity", True, os.path.exists(_sent1040)) + # Read defensively: on the failing side of this row the file is GONE, and an + # unguarded read_bytes() would abort the whole test file with a FileNotFoundError, + # masking every later row instead of reporting two clean failures here. + _survived1040 = (Path(_sent1040).read_bytes() + if os.path.exists(_sent1040) else None) + assert_eq("#1040 inode_reuse: ... and the surviving bytes are the BREAKER's", + _breaker_body1040, _survived1040) + assert_eq("#1040 inode_reuse: ... and the release breadcrumbs that this section's own " + "sentinel was broken, rather than exiting silently", True, + 'was broken by another process' in _errbuf1040.getvalue()) + +# Positive control: an untouched sentinel — still carrying THIS section's nonce — IS +# released. Without this the row above would also pass if __exit__ never unlinked anything. +with tempfile.TemporaryDirectory() as _td: + _R = Path(_td) + _sent1040 = _ias1040_sentinel(_R) + _sec1040 = issue_audit_state._StateSection('s', root=_R) + _sec1040.__enter__() + assert_eq("#1040 inode_reuse control: the section really acquired", True, + os.path.exists(_sent1040)) + _sec1040.__exit__(None, None, None) + assert_eq("#1040 inode_reuse control: an untouched sentinel IS released on exit", + False, os.path.exists(_sent1040)) + +# A bare-pid sentinel — what a hand-planted fixture writes, and what every writer wrote +# before the owner nonce existed — has NO established owner and is therefore never adopted +# as ours. Fail-closed in the right direction: leaving a foreign sentinel costs one +# stale-break window, whereas unlinking one strips a live holder's exclusion. +with tempfile.TemporaryDirectory() as _td: + _R = Path(_td) + _sent1040 = _ias1040_sentinel(_R) + _sec1040 = issue_audit_state._StateSection('s', root=_R) + _sec1040.__enter__() + with open(_sent1040, 'r+b') as _fh1040: + _fh1040.seek(0) + _fh1040.truncate() + _fh1040.write(b'4242') + with contextlib.redirect_stderr(io.StringIO()): + _sec1040.__exit__(None, None, None) + assert_eq("#1040 inode_reuse: a bare-pid sentinel (owner unestablished) is NOT adopted " + "as this section's own", True, os.path.exists(_sent1040)) + +# The acquire path really does record an owner, and it is not a constant across sections — +# so the equality above is a real discriminator rather than two Nones comparing equal. +with tempfile.TemporaryDirectory() as _td: + _R = Path(_td) + _sent1040 = _ias1040_sentinel(_R) + _sec1040 = issue_audit_state._StateSection('s', root=_R) + _sec1040.__enter__() + _owner_a1040 = issue_audit_state._parse_sentinel_body( + Path(_sent1040).read_bytes())[1] + _sec1040.__exit__(None, None, None) + _sec_b1040 = issue_audit_state._StateSection('s', root=_R) + _sec_b1040.__enter__() + _owner_b1040 = issue_audit_state._parse_sentinel_body( + Path(_sent1040).read_bytes())[1] + _sec_b1040.__exit__(None, None, None) + assert_eq("#1040 owner_nonce: an acquisition records an established owner", True, + _owner_a1040 is not None and len(_owner_a1040) == 32) + assert_eq("#1040 owner_nonce: two acquisitions record DIFFERENT owners (the token is " + "not a constant the kernel or a peer could reproduce)", False, + _owner_a1040 == _owner_b1040) + assert_eq("#1040 owner_nonce: the pid stays the FIRST field, so the stale-break " + "breadcrumb still names it", str(os.getpid()), + issue_audit_state._parse_sentinel_body( + f'{os.getpid()} {"a" * 32}'.encode('ascii'))[0]) + # stdin_is_read_before_acquire: main() reads stdin BEFORE entering the section. Proven at # the source level (an executable-order pin): _read_stdin_once precedes the _StateSection # `with` in main(). @@ -15440,6 +15569,68 @@ def _closed_fd0(*argv): _with_run704(_row1040_e2e) +# ── issue #1040 (re-review): the document-integrity claim, OBSERVED not argued ───────── +# The headline guarantee — a state document reflecting one writer entirely and then the +# next, never an interleaving — was established only by decomposing the mechanism +# (exclusive-create sentinel + per-writer temp path) and reasoning about it. The inode-reuse +# finding showed that style of argument can miss a real interleaving, so several REAL +# processes now mutate the same slug at once and the RESULT is checked. +# +# Each writer records a DISTINCT claim key, which is what gives the whole-write property a +# directly observable consequence: `record-claim-baseline` is a read-modify-write of one +# `claims{}` map, so a writer whose load happened before a peer's save and whose own save +# happened after it DROPS that peer's key. Unserialized, that loss is the expected outcome; +# serialized, all N keys survive. +# +# Deterministic by construction, not by timing: the assertions are on the FINAL document, +# and every one of them holds whether or not the writers actually overlapped in time. The +# test therefore cannot flake on a slow or a fast host — a run where the processes happened +# to serialize themselves still passes, it just proves less that time round. Nothing here +# waits on an interleaving occurring, and no bound is tightened to force one. +if os.name != 'nt': + def _row1040_stress(r): + r.write('anchor.md', 'alpha\n') + r.commit('A: add anchor') + _n1040 = 6 + _procs1040 = [ + _subprocess.Popen( + [sys.executable, _IAS603, 'record-claim-baseline', r.slug, + '--claim-key', f'p{_i}', '--claim-class', 'location', + '--path', 'anchor.md', '--nonce', r.nonce], + cwd=r.tmp, stdout=_subprocess.PIPE, stderr=_subprocess.PIPE, text=True) + for _i in range(_n1040)] + # communicate() before returncode: waiting first can deadlock a child that fills a + # pipe buffer, and this is the kind of fixture bug that reads as a concurrency + # failure in the feature under test. + _io1040 = [p.communicate(timeout=300) for p in _procs1040] + _codes1040 = [p.returncode for p in _procs1040] + + assert_eq("#1040 stress: every concurrent writer exited 0 (the shipped acquire " + "window is far above what N short mutations need)", + [0] * _n1040, _codes1040) + assert_eq("#1040 stress: no writer emitted a Python traceback", [], + [_e for _o, _e in _io1040 + if 'Traceback (most recent call last)' in _e]) + + _final1040 = r('query-claim-baselines', r.slug, nonce=True) + assert_eq("#1040 stress: the final document still loads and VALIDATES — a torn " + "write is rejected by _validate, so a readable answer here is the " + "integrity claim holding", 0, _final1040.returncode) + _keys1040 = sorted(_field704(_l, 'claim=') + for _l in _final1040.stdout.splitlines() if 'claim=' in _l) + assert_eq("#1040 stress: every concurrent writer's claim survived — no " + "read-modify-write was lost to an interleaving", + [f'p{_i}' for _i in range(_n1040)], _keys1040) + + _tmpdir1040 = Path(r.tmp, '.prflow', 'tmp') + assert_eq("#1040 stress: no sentinel leaked once every writer released", [], + sorted(_tmpdir1040.glob('*.lock'))) + assert_eq("#1040 stress: no per-writer temp file leaked", [], + sorted(_tmpdir1040.glob('*.json.tmp'))) + + _with_run704(_row1040_stress) + + # ── issue #709: steering-absence establishment ──────────────────────────────── # The Move-3 named assertions from the issue, driven END-TO-END through the CLI over a # real generated instruction file — not against hand-built state — because the whole diff --git a/scripts/issue-audit-state.py b/scripts/issue-audit-state.py index 83ebb0de7..78b07ece2 100644 --- a/scripts/issue-audit-state.py +++ b/scripts/issue-audit-state.py @@ -1085,25 +1085,57 @@ def _positive_env_float(name, default): return val if val > 0 else default -def _read_sentinel_pid(sentinel): - """The pid recorded in the sentinel as a decimal string, or the literal - `unestablished` for any of the five unreadable shapes — file-read failure, empty file, - whitespace-only content, non-decimal text, content exceeding 64 bytes. Staleness is - decided by mtime alone, so an unestablished pid never changes the acquire/refuse/break - decision; it only shapes the breadcrumb. A trailing newline is stripped, so a pid - written with one renders as the pid. - """ +# The sentinel body is ` `, capped so a reader never pulls an unbounded +# file into memory and so a body longer than the cap is DETECTABLE rather than truncated +# into something that parses. +_SENTINEL_MAX_BYTES = 64 +_SENTINEL_OWNER_HEX = 32 # os.urandom(16).hex() + + +def _read_sentinel_body(sentinel): + """The sentinel's raw bytes, or None when the read failed. Raises nothing.""" try: with open(sentinel, 'rb') as fh: - data = fh.read(65) # 65 so a body exceeding 64 bytes is detectable + # +1 so a body exceeding the cap is detectable rather than silently truncated. + return fh.read(_SENTINEL_MAX_BYTES + 1) except OSError: - return 'unestablished' - if len(data) > 64: - return 'unestablished' - text = data.decode('utf-8', 'replace').strip() - if not re.fullmatch(r'[0-9]+', text): - return 'unestablished' - return text + return None + + +def _parse_sentinel_body(data): + """`(pid, owner)` parsed from a sentinel body, each None when unestablished. + + The two fields are established INDEPENDENTLY and neither is inferred from the other. + A body that is absent, empty, whitespace-only, or longer than `_SENTINEL_MAX_BYTES` + yields `(None, None)`. A body carrying only a decimal pid — the shape a hand-planted + sentinel produces, and the shape every writer produced before the owner nonce existed — + yields that pid with `owner=None`, so such a sentinel can never be mistaken for one + THIS process owns: `None` is the unestablished reading, and `__exit__` compares the + owner for equality against a 32-hex-digit nonce it generated, which `None` never + matches. The owner is shape-checked rather than merely non-empty, so a truncated or + garbled field reads unestablished instead of being compared as data. + """ + if data is None or len(data) > _SENTINEL_MAX_BYTES: + return None, None + fields = data.decode('utf-8', 'replace').split() + if not fields: + return None, None + pid = fields[0] if re.fullmatch(r'[0-9]+', fields[0]) else None + owner = None + if len(fields) > 1 and re.fullmatch(rf'[0-9a-f]{{{_SENTINEL_OWNER_HEX}}}', fields[1]): + owner = fields[1] + return pid, owner + + +def _read_sentinel_pid(sentinel): + """The pid recorded in the sentinel as a decimal string, or the literal + `unestablished` when it cannot be established (see `_parse_sentinel_body`). Staleness + is decided by mtime alone, so an unestablished pid never changes the + acquire/refuse/break decision; it only shapes the breadcrumb. Surrounding whitespace + is ignored, so a pid written with a trailing newline renders as the pid. + """ + pid, _ = _parse_sentinel_body(_read_sentinel_body(sentinel)) + return pid if pid is not None else 'unestablished' def _replace_with_retry(src, dst, *, attempts=5, delay=0.02): @@ -1142,10 +1174,16 @@ class _StateSection: fixed, on two grounds. First, occupancy is a sub-second load-modify-save of one small JSON document — the section holds no network call, no subprocess, and no stdin read (main() hoists stdin above the section precisely so a handler cannot block on fd 0 - while holding it). Second, the `(st_dev, st_ino)` token recorded at acquire bounds the - blast radius on the way out: __exit__ unlinks only a sentinel that is still the one - this section created, so a holder whose sentinel was age-broken releases nothing and - cannot strip the breaker's exclusion — it breadcrumbs instead. Raising the bound by + while holding it). Second, the owner NONCE written into the sentinel bounds the blast + radius on the way out: __exit__ unlinks only a sentinel whose recorded owner is still + the nonce this section wrote, so a holder whose sentinel was age-broken releases + nothing and cannot strip the breaker's exclusion — it breadcrumbs instead. That second + ground previously rested on the sentinel's `(st_dev, st_ino)`, which does NOT support + it: the breaker unlinks our inode and creates its own file at the same path, and an + inode-reusing filesystem may hand it the identity we recorded, so the identity check + could match a file we do not own and unlink a live holder's sentinel. The nonce is + generated per acquisition and never reissued by the kernel, so it answers the question + the identity check only appeared to. Raising the bound by adding a heartbeat (a keepalive touch, or a refresh on a long operation) is a DESIGN CHANGE with its own failure modes, not a bug fix; do not introduce one without deciding that trade deliberately. The relation `stale_after_s < acquire_window_s` is @@ -1163,7 +1201,9 @@ def __init__(self, slug, root=None, *, acquire_window_s=45, stale_after_s=30): self._acquire_window_s = _positive_env_float( _IAS_ACQUIRE_WINDOW_ENV, acquire_window_s) self._stale_after_s = _positive_env_float(_IAS_STALE_AFTER_ENV, stale_after_s) - self._token = None # (st_dev, st_ino) recorded at acquire + # The owner nonce written into the sentinel, set only once an acquisition fully + # succeeded. None means this section holds nothing and must release nothing. + self._token = None def _persist_error(self, detail): return StateError(f'could not persist state to {self._state_path}: {detail}') @@ -1190,16 +1230,22 @@ def _try_create(self): f'{self._sentinel}: {exc}') from exc try: try: - st = os.fstat(fd) - self._token = (st.st_dev, st.st_ino) - os.write(fd, str(os.getpid()).encode('ascii')) + # A fresh unforgeable nonce per successful acquisition, written INTO the + # sentinel so ownership can be re-established from content at release. The + # pid stays first and unchanged — it is the breadcrumb operand, and a + # hand-planted bare-pid sentinel must keep parsing as one. + owner = os.urandom(_SENTINEL_OWNER_HEX // 2).hex() + os.write(fd, f'{os.getpid()} {owner}'.encode('ascii')) finally: os.close(fd) + self._token = owner except OSError as exc: - # An OSError after the exclusive create succeeded (an ENOSPC on the pid write, - # an fstat failure) must still route as a cannot-persist StateError, not escape - # as a raw traceback that breaks the mutation contract. Best-effort unlink the - # partial sentinel so it does not block later acquires until it ages out. + # An OSError after the exclusive create succeeded (an ENOSPC on the body write, + # an entropy or close failure) must still route as a cannot-persist StateError, + # not escape as a raw traceback that breaks the mutation contract. `self._token` + # is assigned only after the write and close both succeed, so a section that + # failed here owns nothing and releases nothing. Best-effort unlink the partial + # sentinel so it does not block later acquires until it ages out. try: os.unlink(self._sentinel) except OSError: @@ -1282,16 +1328,30 @@ def __exit__(self, exc_type, exc, tb): # 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 it. Returning False never suppresses that exception. + # + # Ownership is decided by the owner NONCE this section wrote into the sentinel, and + # deliberately NOT by the sentinel's `(st_dev, st_ino)`. An identity check is + # forgeable by the kernel: after an age break the breaker unlinks our inode and + # O_EXCL-creates its own file at the same path, and on an inode-reusing filesystem + # (ext4 and friends) it may be handed the SAME `(st_dev, st_ino)` we recorded at + # acquire — so an identity comparison would match and this section would unlink the + # LIVE holder's sentinel, precisely the outcome the check exists to prevent. A + # 128-bit nonce is not reissued by the kernel, so content equality answers "is this + # still the file I created?" where identity equality only answers "does this file + # occupy the slot mine did?". + if self._token is None: + return False # never acquired — this section owns nothing to release try: - st = os.stat(self._sentinel) + with open(self._sentinel, 'rb') as fh: + body = fh.read(_SENTINEL_MAX_BYTES + 1) except FileNotFoundError: return False # already gone (age-broken by another process) — clean exit except OSError as exc2: sys.stderr.write( - f'issue-audit-state.py: could not stat the audit-state sentinel ' + f'issue-audit-state.py: could not read the audit-state sentinel ' f'{self._sentinel} on release: {exc2}\n') return False - if self._token is not None and (st.st_dev, st.st_ino) == self._token: + if _parse_sentinel_body(body)[1] == self._token: try: os.unlink(self._sentinel) except OSError as exc2: