Two instruction sentences that describe a relay that is not there - #25
Merged
Merged
Conversation
Independent review after #22 landed found one stale and one overreaching. crw-run/SKILL.md still said re-review runs under a fresh execution generation rather than by re-deciding the settled event. Same old contract as the four files #22 corrected, different wording, which is why a phrase grep missed it. It now names the two routes in one sentence and links to the relay reference rather than repeating the detail a fifth time; copying it again is how these drift apart. relay.md's new list was wrong in the other direction. It named a relationship that is paused, cancelled, archived or superseded as a case where a fresh generation is the route, but registry.open_generation calls require_active, so generation-open refuses all three with relationship_not_active. Reproduced for each status. What the reader actually needs first is relationship-resume, which is not a status flip because relationship-status accepts deactivations only, and which restates the generation and scope being re-authorized. A superseded relationship never comes back, and a cancelled or archived one whose issue has a new owner is refused duplicate_assignment. Two more from sweeping by meaning rather than by phrase. The assignment-mark paragraph did not say the mark is refused with criteria_set_changed once the set has moved, nor that an integration recorded earlier reads as the current mark again after the re-review lands, so nobody re-records it. I-54's enforcement column named only _re_review_open when claiming also requires _ruling_is_current to be false. Two tests pin the procedure the instructions now prescribe: generation-open refused on each inactive status and accepted after resume, and set_status refusing "active" so resume is the only way back.
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. |
Review caught this branch making the mistake it exists to remove, twice. Listing inactivity among the fresh-generation cases reads as a reason to open one. It is not: it closes both routes while it lasts and neither afterwards. A coordinator following that list would resume, open a generation and dispatch a child turn to re-emit bytes that never changed, when claiming the same event would have returned proceed. Inactivity is now a precondition stated once, before either route, and the fresh-generation list is back to the three cases where the same event genuinely cannot be the answer. The resume example also could not have worked. An ordinary registration authorizes both the parent and the child, and resume compares the restated scope for whole-list equality, so the single --expect-allowed-recipient I wrote would have been refused with "allowed recipients differ from the restated scope". The example now repeats the flag for both, and the text says the comparison is exact so the reader repeats it for their own roots too. A third test pins that: a one-recipient restatement of a two-recipient registration is refused, and the full one resumes.
Replacing the inactivity bullet left the bullet above it duplicated, so the route-selection list read as though it were missing a case. Removed. The SKILL.md summary said the route turns on whether the artifact has to change. That is one of the three grounds, not all of them: an event already ruled needs_changes, unverified or aborted takes a new generation even when the artifact is fine, which is a common retry path. The summary now states the condition for reusing the same revision and names the three cases that do not, and still sends the reader to the reference for the rest.
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
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.
Follow-up to #22, found by independent review after it landed. Two instruction sentences described a relay that is not there, one stale and one overreaching, plus more found by sweeping the same ground by meaning instead of by phrase.
Stale: a fifth file still carried the old contract
skills/crw-run/SKILL.mdsaid re-review "runs under a fresh execution generation rather than by re-deciding the settled event" — the same contract #22 corrected in four other files, worded differently, which is why a phrase grep forsettled verdict is immutable/contract working, not a fault/only routedid not find it.It now names the two routes and the condition that chooses between them, then links to the relay reference. The detail is not repeated a fifth time on purpose: copying it again is how these drifted apart in the first place.
Overreaching: #22's own new list prescribed a command that refuses
relay.mdlisted "the relationship is paused, cancelled, archived or superseded" as a case where a fresh generation is the route. It is not, becauseregistry.open_generationcallsrequire_activeandopen_generation_inchecks the status again, sogeneration-openrefuses all three withrelationship_not_active. Reproduced for each status rather than read off the source alone.Inactivity is now a precondition stated once, before either route: while the relationship is down both routes are closed, and neither is preferred once it is back. Review pushed on this and was right to — leaving it in the fresh-generation list still read as a reason to open one, which would have a coordinator dispatch a child turn to re-emit bytes that never changed.
The reference also never mentioned
relationship-resume, which is what the reader needs first. Reactivating is not a status flip, sincerelationship-statusaccepts onlypaused,cancelledandarchived. Resume restates the generation and scope, and compares the restated scope for whole-list equality, so the example repeats--expect-allowed-recipientfor both the parent and the child an ordinary registration authorizes. A superseded relationship never comes back; a cancelled or archived one whose issue has a new owner is refusedduplicate_assignment.More, from sweeping by meaning
The
assignment-markparagraph said only that naming a stale revision is refused. It did not say the mark is also refused withcriteria_set_changedonce the criteria have moved — the reason to re-review before recording an integration — nor that an integration recorded earlier reads as the current mark again after that re-review lands, so nobody records it twice. Existing behaviour with existing tests; only the prose was missing.docs/invariants.mdI-54 named only_re_review_openin its enforcement column. Claiming also requires_ruling_is_currentto be false, which is what closed the last I-54 hole in #22.Validation
Three tests pin the procedures the instructions now prescribe, because a documented procedure that fails at its first command is the defect class this PR exists to remove:
Lifecycle::test_an_inactive_relationship_opens_no_generation_until_it_is_resumed—generation-openrefused on paused, cancelled and archived, accepted afterresume.Lifecycle::test_reactivating_is_not_a_status_flip—set_statusrefusesactive, so resume is the only way back.Lifecycle::test_a_restatement_names_every_registered_recipient— a one-recipient restatement of a two-recipient registration is refused; the full one resumes.All checks exit 0:
validate.py(7 skills, local links, syntax),contracts.py,secrets.sh,CRW_PACKAGES_TMPDIR=/var/tmp packages.pyreporting codex-session-relay 911, codex-thread-bridge 119 against 908/119 ondev.git diff --checkclean.How the sweep was done, since a phrase grep missed one
Concept vocabulary rather than known sentences, across
skills/andpackages/codex-session-relay/docs/: re-review, fresh/new generation,generation-open,already_claimed, idempotent, immutable, settled verdict/event, re-decide, replay, criteria changed/edited, wording,expect-criteria-digest, claim binds, duplicate delivery, second claim. Ranked the 14 matching files by hit count, read every hit in context, and judged each against source — in both directions, asking whether the sentence understates what the code now does or claims something it does not. Then a second pass over the prescribed command sequences, checking each documented command against the refusals its implementation can raise, which is what surfacedrequire_active.Cleared as accurate:
operations.mdOPS-6.4's five conditions,scenarios.mdS11 (corrected in #22),native-worktree-verification.mdstep 8 (a correction, which does open a generation),protocol-v1.md§6,cxc-contract-map.md, and theimmutable/replayhits inhook-contract.md,bridge.md,task-packet.mdandcounterexamples.md, which are about hook identity, commits and probe replay rather than verdicts.I also re-checked my own #22 claims against source, including two I had taken from review rather than verified myself: the
sync_idargument list really does omit the criteria digest withINSERT OR IGNORE, and anabortedruling really does leave re-claim atalready_claimedwith the verdict replaying.Follows #22 for JUN-167.