Let a criteria re-review actually happen, and stop a losing ACK overwriting a verified one - #22
Merged
Merged
Conversation
…riting a verified one Editing the canonical criteria after a verified verdict was already reported correctly: the assignment drops out of verified into re_review_needed and asks the parent to verify again. Nothing could then do it. record_verdict returned the historical ruling before it looked at the new set, the review could not be claimed again onto the new digest, and an event id is derived from the artifact revision, so re-emitting unchanged bytes produced no new event either. The assignment stayed blocked until the child changed a byte it had no reason to change. The same dead end sat one step earlier, with no verdict at all: claim, edit a criterion, and the ruling is refused with "claim the review again", which INSERT OR IGNORE made impossible. ack._re_review_open is now the one condition both entry points read, so the claim and the ruling cannot disagree about whether a re-review is open. It is the state AssignmentView already reports: the set in force differs from the one this review was decided against, the event is still the revision this generation stands on, and any ruling recorded for it is a verified one. needs_changes already moved to a new generation and aborted ended the assignment, so neither reopens. Currency is required, so a ruling some later generation left behind cannot be rewritten. record_verdict does not rebind the digest itself: that would bypass the guard whose whole purpose is that findings made against one wording cannot certify another, so the parent re-claims and submits fresh findings. verdict_context now updates on conflict, without which the assignment would read re_review_needed forever, and the replaced ruling is journalled with both digests since the schema has one verdict row per event. The second defect is decided by scheduling. acknowledge reads the existing acknowledgement outside its write lock and returned early only for a verified one, so two processes disposing of one event both passed that read. evaluate reports duplicate_event for a settled event, but the conflict guard refused it only for accepted=True, so a rejection carried on into the upsert and replaced a verified acceptance with a verified rejection. The acknowledgement is now re-read as the first statement inside the transaction and a verified one is returned unchanged, whatever the second caller asked for. Both short-circuits now report _verified and _replay, so a caller can tell a settled duplicate from its own result instead of reading a verified record as unestablished. Each defect has a regression test that fails on the source before this change: 14 failed, exit 1. The race is interleaved rather than raced - each process gets its own connection and the winner commits inside the pre-transaction host read - so nothing depends on timing.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Independent review caught a hole the first version opened. Re-claiming rebound claim_context to the edited set, and nothing ties a ruling to the caller that made the claim, so two sequences let findings made against the old wording certify the new one: a second reviewer re-claims and the first submits what it read against the previous text, or the original verified ruling is simply re-submitted after somebody re-claims. Both recorded verified against the new digest and cleared re_review_needed without a review. Before this branch the first was refused and the second stayed historical, so this weakened exactly the guard the branch promised to preserve. An original claim can pin the set because the relay watched that review start. After an edit it cannot tell whether the caller that rules next is the one that re-read the artifact, so re-claiming now deletes the binding and does not replace it. The ruling names the set it read instead, which is the alternative criteria.coverage already provides for and which the CLI already exposes as --expect-criteria-digest. A caller still holding the old findings has nothing it can truthfully name: with no digest it is refused review_not_bound, and with the digest it actually read it is refused criteria_set_changed. Both sequences are now regression tests, and they failed on the previous commit with "RelayError not raised".
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…happened Two review rounds found two ways the reopened path gave away more than it meant to. A re-review could rule unverified or aborted. Both replaced the verified ruling, and the assignment has no state for either: the view fell through to the claim and reported verifying forever, while _re_review_open then refused to reopen because the ruling of record was no longer a verified one. That is the deadlock this branch exists to remove, rebuilt one disposition over. Only verified and needs_changes may replace a certification now. Neither disposition is lost: both still apply to an event with no ruling, and an assignment nobody intends to finish is paused or cancelled on the relationship rather than annotated onto one of its events. The previous commit cleared the review binding instead of moving it, to stop a caller holding old findings from being certified by somebody else's re-claim. That traded one hole for a worse one: with nothing bound, every later claim looked like another re-review, so the claim became one anyone could take and I-54 was undone. Reopening rebinds to the set in force again, and the fence moves to where it can be enforced: a re-review names the set it read. Without expect_criteria_digest the ruling being replaced is indistinguishable from an actual re-review, so re-submitting it is refused. cli.py already carries the flag as --expect-criteria-digest, so the path stays reachable. What that leaves is recorded rather than implied: a claim locks an event, it does not identify who rules. A caller that attests the current digest is accepted whether or not it read that set, exactly as the documented expect_criteria_digest alternative has always been, and two callers sharing one claim could always submit each other's findings. Closing it needs a review token in verification_claims and a parameter on record_verdict for the CLI to pass. Both findings are regression tests. They failed on 99a8cf7 with "RelayError not raised" and with a claim returning proceed where it should have been refused.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The docstring claimed the condition is the state AssignmentView reports as re_review_needed. That is true only where a verified ruling exists. Claiming hands in the digest the review is BOUND to, so the condition also fires on a review that was claimed and never ruled, and the view calls that one verifying: nothing has been certified, only the binding no longer matches. One condition still, because the remedy is one thing, but not because the view names the two alike. Raised as a low finding in review and fixed in the invariants table without the code catching up.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There are two records of what a review was decided against, because there are two ways to decide one. Claiming binds the set and the ruling inherits that binding; stating the digest is the alternative record_verdict accepts instead, and that path never touches claim_context. A legacy review claimed before any criteria existed keeps a NULL binding, so once criteria are registered and the re-review is recorded by attestation, the binding still names nothing while the ruling names the set in force. Reading only the binding called that review stale for as long as the assignment lived and handed the claim to whoever asked next, which is I-54 again on a path the earlier fix never ran. Claiming now asks whether a ruling has already been made against the set in force before it considers reopening. Ruling still asks whether the ruling itself is stale, which is a different question with a different answer once a review has been claimed again: the re-claim refreshes the binding, and the standing ruling is exactly what the re-review is there to replace. Regression test fails on d14eb76 with 'proceed' != 'already_claimed'.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The instructions still said a settled verdict is immutable, that re-claiming returns already_claimed, and that a fresh execution generation is the only route from there — relay.md called that "the contract working, not a fault". It is the defect JUN-167 scoped, and the code no longer behaves that way. Left alone, the repository would keep asserting a contract it does not keep, and the next agent to observe this would dismiss a real fault because the documentation calls it correct. relay.md now names two routes and says which applies. Judging the same revision again: claim returns proceed where the set moved under a review of the revision the assignment still stands on, rebinds to the set in force, and the verdict rules verified or needs_changes. --expect-criteria-digest is required only where a verdict had already settled, because that is the only case a re-submission of the replaced ruling could be mistaken for a review. A fresh generation is still the route when the artifact has to change, when the event was ruled needs_changes, unverified or aborted, when the event is no longer the current revision, and when the relationship is not active. The generation machinery below it is unchanged and still correct. crw-check and crw-plan's integrations repeated the same claim in one sentence each, and operations/scenarios.md S11 did too — "Preserved: the settled verdict, which is immutable". That one is not in the reported set; the grep for the sentence found it. It now says what is actually preserved, which is the record of what was judged rather than the judgement. Two tests pin the sequences the instructions now prescribe: the unruled re-review that needs no stated digest, and the unverified ruling that still takes a fresh generation.
thisisjun786
added a commit
that referenced
this pull request
Sep 17, 2026
The merge brought PR #21 and #22 into the relay package, so the recorded tree and OPS-1.2 digest no longer describe this checkout. verify-definition caught the drift and these are the values it derives now. The identity test is the component's own subtree, so this is the relay changing and not an unrelated commit.
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.
The problem
Editing an assignment's canonical criteria after a verified verdict was already reported
correctly:
AssignmentViewdrops out ofverifiedintore_review_neededand asks the parent toverify again. Nothing could then actually do it.
record_verdictreturned the historical ruling before it ever looked at the new set, the reviewcould not be claimed again onto the new digest, and an event id is derived from the artifact
revision, so re-emitting unchanged bytes produced no new event to rule on either. The assignment
stayed in
re_review_neededuntil the child happened to change a byte it had no reason to change.The same dead end sits one step earlier, with no verdict at all. Claim a review, let somebody edit
a criterion while it is being read, and the ruling is refused with "claim the review again" -
which
INSERT OR IGNOREonverification_claimsandclaim_contextmade impossible.Separately, which of two competing acknowledgements won was decided by scheduling.
acknowledgereads the existing acknowledgement outside its write lock and returned early only for a verified
one, so an accepting and a rejecting process both passed that read.
evaluatereportsduplicate_eventfor a settled event, but the conflict guard refused it only foraccepted=True,so the rejection carried on into the upsert and replaced a verified acceptance with a verified
rejection. Since a verdict requires a verified acceptance (I-73), that also closed the verdict
path for a revision the parent had already accepted.
Before / after
_replay; assignment blocked indefinitelyverifiedCRITERIA_SET_CHANGEDsays "claim the review again"; re-claiming is a no-op_replayHow
ack._re_review_openis the single condition both entry points read, so the claim and the rulingcannot disagree about whether a re-review is open. The set in force differs from the one this
review was decided against, the event is still the revision this generation stands on, the
relationship is readable, and any ruling recorded for it is a verified one.
needs_changesalready moved the assignment to a new generation whose revision arrives as its ownevent with its own claim, and
abortedends the assignment, so neither reopens - a second rulingthere would be a second allocation, not a re-review. Currency is required, so a verified ruling
that some later generation left behind cannot be rewritten.
Two fences keep reopening from becoming a way around the guard it reopens:
expect_criteria_digestthe ruling beingreplaced is indistinguishable from a genuine re-review, so re-submitting it unchanged would
clear
re_review_neededwith nobody having read the new wording.cli.pyalready carries theflag as
--expect-criteria-digest.record_verdictnever rebinds the digest itself; that wouldbypass the guard whose entire purpose is that findings made against one wording cannot certify
another.
verifiedorneeds_changes. Those are the two dispositions theassignment has a state for. Replacing a certification with
unverifiedoraborteddropped itback to
verifyingwith the claim still held, and_re_review_openthen refused to reopenbecause the ruling of record was no longer verified - the same deadlock one disposition over.
Both remain available on an event with no ruling; an assignment nobody intends to finish is
paused or cancelled on the relationship.
verdict_contextnow updates on conflict; leaving the stale digest would leave the assignmentreading
re_review_neededforever. The schema has one verdict row per event and this change addsno table, so the replaced ruling is journalled as
verdict_supersededwith both digests.For the race, the acknowledgement is re-read as the first statement inside the write transaction
and a verified one is returned unchanged whatever the caller asked for. Both short-circuits now
report
_verifiedand_replay, so a caller can tell a settled duplicate from its own resultinstead of reading a verified record as unestablished.
Validation
Both regression suites were written first and fail on the source before this change:
The race is interleaved, not raced: each process gets its own connection to the same store and
the winner commits inside the window
acknowledgedocuments as its pre-transaction host read.Nothing sleeps and nothing depends on an ordering the test does not itself impose. PR #19's
per-recipient transport concurrency moved when acknowledgements arrive; it did not move where
that window is.
Repository checks, all exit 0:
validate.py,contracts.py,secrets.sh,CRW_PACKAGES_TMPDIR=/var/tmp packages.py- the last reports codex-session-relay 901 tests,codex-thread-bridge 119, against a 872/119 baseline on
dev; the 29 added are exactly the twonew files. Every test runs inside its own temporary tree, so none opens a real store.
Preserved and pinned by tests rather than assumed: replay idempotency for an unchanged set
(including
verifiedthenverified),CRITERIA_SET_CHANGED,REVIEW_NOT_BOUND,duplicate-delivery claim idempotency (I-54, including after a re-claim and after a landed
re-review), the deferred
verify_pending_acksupgrade, and the currency, stale-generation andpaused refusals.
docs/invariants.mdgains I-143 through I-146 and an updated I-54.Known limitations, recorded not fixed
Both are in the "Recorded limits" table in
docs/invariants.md.A claim locks an event, it does not identify who rules.
record_verdicthas no parameternaming the claim it rules under, so a caller that attests the current digest is accepted whether
or not it read that set - the same trust the documented
expect_criteria_digestalternative hasalways carried. This predates re-review: two callers sharing one claim could always submit each
other's findings. Closing it needs a review token in
verification_claimsand a parameter onrecord_verdictfor the CLI to pass, which is a larger change than the deadlock this removes.A same-disposition re-review is not re-synchronised.
syncderivessync_idwithout thecriteria digest and enqueues with
INSERT OR IGNORE, so a secondverifiedruling collides withthe first and the coordination document keeps the summary written against the earlier wording. A
re-review that changes the disposition does enqueue. The local record is complete either way.
sync.pyis outside this task's edit surface.Also noticed and left alone: after an
unverifiedverdict on an event the view reportsVERIFYING/parent_verifieswhilerecord_verdictreplays indefinitely. That is a pre-existingview/verdict inconsistency unrelated to criteria edits and not one of the two defects this issue
scoped.
Addresses defects 3 and 4 of JUN-167.