Per-session fix-loop state; ownership before commit staleness (1.7.0) - #5
Merged
Conversation
Two gaps found by running the plugin's own /codex-review against the
1.6.0 merges, both in how the multi-session model composes with linked
worktrees:
1) HEAD is per-worktree, so a bystander session's HEAD never matches
the owner's saga sha, and the old check order (staleness before
ownership) let any bystander's turn end clear a fresh FAIL through the
superseded-commit rule. The FAIL branch now runs age (global) ->
ownership (foreign state untouchable) -> superseded-commit (judged
only against the owner's own worktree).
2) The state files were repo-wide singletons: a second failing saga
overwrote the first session's verdict and shared its loop counter, so
one session's Stop could release another's block. State and counters
are now keyed by the committing session (".<session-id>" suffix).
Filename-safe ids (UUIDs, the normal case) key files verbatim; other
ids get a safe prefix plus cksum+length so distinct ids can never
collapse to one key. The bare names remain the legacy/anonymous bucket
-- pre-1.7.0 state and session-less commits -- with the 1.6.0
ownership rules intact, and a live pre-1.7.0 saga is adopted by its
session on its next commit: the bare state and counters migrate to the
suffixed names so loop and round counts survive the upgrade instead of
tripping the Stop hook's corruption guard.
Every Stop garbage-collects any session's state that has outlived its
window, with its counters, so abandoned sessions clean up regardless
of who stops next. The RUNNING window is 960s -- past the 900s hook
ceiling -- so a live review can never be swept even at the maximum
configured CODEX_REVIEW_TIMEOUT; FAILs expire after an hour; corrupt
or future stamps read as stale; unknown verdicts stay preserved for
inspection. File-sourced numbers are normalized as base-10 before
entering arithmetic, so zero-padded corruption (a "08" timestamp)
reads as stale instead of aborting the hook on an octal parse error.
/codex-review-waive resolves the review to waive from the caller's own
worktree HEAD (history is repo-wide; "latest FAIL" could be another
session's) and its release step removes only state matching that sha,
so waiving in one session can neither waive nor release another
session's unrelated failure. Version 1.7.0.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the two P1s that
/codex-reviewfound when run against the merged #1 (see the discussion there): in linked worktrees, a bystander session's turn end could clear a fresh FAIL through the superseded-commit staleness rule (HEAD is per-worktree, so it never matches the owner's saga sha), and the singleton state file let concurrent failing sagas overwrite each other's verdict and share one loop counter.Design:
codex-review-state.<session-id>etc. Suffixed state needs no ownership comparison — the filename is the owner. Bare names remain the legacy/anonymous bucket (pre-1.7.0 state, session-less commits) with ownership rules intact, and a live pre-1.7.0 saga is adopted by its session on its next commit, migrating counters so the upgrade never resets or releases an active loop.=-prefixed SHA-1-derived key (git hash-object);=is stripped from literal ids, so literal and derived namespaces are disjoint by construction. Legacy owner fields hold raw ids and are compared raw-only.08timestamp reads as stale instead of aborting the hook on octal parsing)./codex-review-waiveresolves the review to waive from the caller's worktree HEAD and releases only state matching that sha — waiving in one session can no longer release another session's unrelated block.Tests: 81/81 (18 new), shellcheck clean,
claude plugin validatepassing. Version 1.7.0.Dogfooding note: this commit went through four rounds of its own post-commit review before passing. Codex successively caught: the waive release wildcard nuking unrelated sessions, missing legacy counter migration, octal timestamp crashes, a GC window shorter than the configured review timeout, lossy key sanitization, un-namespaced derived keys, raw-vs-derived legacy ambiguity — and in the final round produced a working CRC-32 collision pair to prove
cksuminsufficient, which is what bought the SHA-1 scheme. The "fix round N of max 8" counter in each blocking message was #2's round cap policing its own successor.🤖 Generated with Claude Code