Document the state and ownership refusals, and make their printed recovery runnable - #18
Merged
Merged
Conversation
…close Six operator-visible refusal reasons landed with JUN-103 and none of them appear anywhere in the package docs. An operator who hits ambiguous_state_directory has the reason string, the candidate paths, and nothing to read. The issue's own criteria ask that the selected database and the diagnostics be discoverable, so this is the deliverable finishing rather than new scope. operations.md gains two pieces. Under "Where the state lives", what happens when the precedence rules land on something ambiguous: the three selection refusals, when each fires, and the inspect-before-adopt recovery written as commands that carry the socket. Two limits are stated rather than glossed - choosing one of two claiming stores does not remove the ambiguity, so every participant has to pass the same explicit --state until one store is retired; and the different-socket refusal prints no recovery list because using a store does not rewrite the socket it recorded. The doctor and ack-proof exemption is scoped to these guards, since doctor still refuses an unproven same-store comparison. Under "Ownership", the refusal vocabulary as a reason-to-action table, because the distinctions decide the operator's next move: not_ours is a definite mismatch, ownership_unverifiable is insufficient evidence and often resolves itself, replaced_by_new_launch means the service is NOT stopped and the original launch may already have been terminated, supervised_store_mismatch is not repaired by a restart. invariants.md corrects a row that claimed more than the code delivers. "stop and disable hold the lock across the decision" omitted the handoff, so it now says what is true - they decide under the lock and stop writes its record under it - and three limits are recorded beneath it: lock_is_held reporting an unopenable lock file as not held, disable and enable writing shared intent outside the lock on the accept path, and start confirming from a record plus a separate probe. Those three are the seam that produced a new finding in five consecutive review rounds on PR #8. They are documented here rather than patched, because each narrow fix so far revealed the next window and the seam wants one deliberate revision.
The recovery list these refusals print is the whole of what an operator has to work from, and it did not survive being read as instructions. It ended with "--state <the directory above> once, to adopt it deliberately", which is not a command, never says which directory, and silently drops the --socket that made two stores candidates for each other in the first place. It also promised an adoption that does not happen: choosing one of two claiming stores leaves both still recording the socket, so the next invocation that relies on default discovery is refused again, and every participant of that assignment needs the same explicit directory until one store is retired. The refusal now says so. The different-socket refusal printed no recovery at all. That one has nothing to adopt, because using a store does not rewrite the socket it recorded, so it now offers the two readings that identify the matching pair and states the reason there is no third option. Both lists are complete commands carrying the socket, and every one of them only reads: provenance is written by opening a store, which is the thing the refusal prevented, so inspection is safe to repeat. The program name comes from argv rather than a constant, because the console script and python3 -m are both ordinary ways to get here and a command naming the wrong one is a command the operator has to translate. Two regression tests pin the payload shape - every command carries the socket, every candidate is inspected, the vanished "directory above" phrasing is gone, and the contested case states that choosing does not retire. Both fail against the previous payload.
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.
Review found both halves of the same mistake in the recovery commands this PR
added, and both are mine.
The paths went in unquoted. A state directory with a space splits into two
arguments; one carrying shell syntax runs it. The whole point of printing a
command is that it gets pasted, so this made following the refusal's own advice
the dangerous act. Every interpolated path is quoted now.
And _program discarded the directory. A console script that is not on PATH
rendered as a bare name, so pasting reached a different installation or nothing
at all. It keeps the directory when argv[0] has one.
The tests assert the property rather than the spelling: each command is put
through shlex.split and the path has to come back as one intact argument. That
matters because the first version of these assertions looked for "$(" and failed
on correctly quoted output, where the substitution is literal. Both fail against
the unquoted form, which splits a spaced path in two.
![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.
Review found four ways the refusal advice this branch added was wrong about its own behaviour. The socket-first line has no --state to override anything, so when CODEX_SESSION_RELAY_STATE is what pinned the selection in the first place, pasting 'find the store that belongs to this socket' re-selected the store that had just produced the refusal and returned the same error. It is printed with env -u now, and only when the variable is actually set, so the ordinary case stays plain. The module form named a bare python3. The relay may be running under a virtualenv or a versioned interpreter that is not on PATH under that name, where the printed line reaches a different installation or nothing at all. It names the running interpreter, quoted. And operations.md described code that no longer exists. It said the different-socket refusal prints no recovery list, which stopped being true one commit later; the point it was making - that there is nothing here to adopt - survives and is now made about the list that is actually printed. It also claimed every lifecycle refusal is returned before the command acts, directly contradicting the replaced_by_new_launch row three lines above it, which says the launch may already have been terminated. That one is a refusal to claim success, not a refusal to act, and the text says so. Both code fixes were confirmed failing against the previous commit. The env-u regression runs the printed command rather than matching its text. Separately, and not from the review: StateDirectory did not pin HOME, so resolving a socket's directory read the siblings next to the REAL one and opened the databases this host actually runs on. Only their shared-memory index was touched, never the databases or their WAL, but a unit test has no business reaching them at all. It now pins HOME the way the Precedence class beside it already did. This predates the branch; it is fixed here because it is what made the isolation evidence for this work untrue.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c4cd41476
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The previous commit removed CODEX_SESSION_RELAY_STATE from the socket-first recovery line whenever the variable was set. But --state wins over the variable, so it can cause this refusal while the variable still points at the store that does record the requested socket. Reproduced: with --state on a store recording another socket and the variable on the store recording the wanted one, the printed line landed on ~/.local/state/codex-session-relay/39579c81447aa5d7 with exists=False, under a caption promising the store that belongs to this socket. The variable would have found it. The prefix is now conditioned on the selection's own source rather than on the variable merely being set, so it appears exactly in the case that motivated it. Confirmed failing against the previous commit.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f25f73472
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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.
Closes the discoverability half of JUN-103's diagnostics criterion: that an operator can find out which database was selected and why a command refused.
The problem
JUN-103 shipped six operator-visible refusal reasons —
ambiguous_state_directory,unidentified_state_directory,state_directory_serves_another_socket,supervised_store_mismatch,replaced_by_new_launch,ownership_unverifiable. At1f04c91all six appear insrc/and none appear anywhere indocs/. An operator who hits one has the reason string, maybe a list of candidate paths, and nothing to read.Worse, the recovery advice the code prints does not survive being read as instructions. The ambiguity refusal ended with:
That is not a command. It never says which directory. It drops the
--socketthat made two stores candidates for each other in the first place. And it promises an adoption that does not happen — choosing one of two claiming stores leaves both still recording the socket, so the very next invocation relying on default discovery is refused again. The different-socket refusal printed no recovery at all.What changed
cli.py—_recovery_commands()builds complete, runnable commands that carry the socket and inspect each candidate with bothdoctorandservice status. Every command only reads: provenance is recorded by opening a store with a socket, which is exactly what the refusal prevented, so inspection is safe to repeat. The contested case now states in the payload that choosing does not retire the other store and that every participant needs the same--stateuntil one is gone. The different-socket refusal gains its own two-command list plus anoteexplaining there is nothing to adopt, because using a store does not rewrite the socket it recorded._program()derives the invocation fromargvrather than hardcoding it, since the console script andpython3 -m codex_session_relay.cliare both ordinary ways to reach this code and a command naming the wrong one is a command the operator has to translate.docs/operations.md— a new "When the rules cannot pick a store" subsection covering the three selection refusals, when each fires, and the inspect-before-adopt recovery. Two limits are stated rather than glossed. Under Ownership, a reason-to-action table for the four ownership refusals, because the distinctions decide the operator's next move:not_oursis a definite mismatch,ownership_unverifiableis insufficient evidence and often resolves itself,replaced_by_new_launchmeans the service is not stopped and the original launch may already have been terminated,supervised_store_mismatchis not repaired by a restart.docs/invariants.md— one row claimed more than the code delivers. "stopanddisablehold the lock across the decision" omitted the handoff window, so it now says what is true, and three limits are recorded beneath it:lock_is_heldreporting an unopenable lock file as not held,disable/enablewriting shared intent outside the lock on the accept path, andstartconfirming from a record plus a separate probe.Those three are the seam that produced a new finding in five consecutive review rounds on #8. They are documented here rather than patched, because each narrow fix revealed the next window and the seam wants one deliberate revision.
Accuracy corrections caught in review
An independent audit of the plan corrected three things before they shipped:
doctorstill exits non-zero when a same-store comparison it was asked to make comes back unproven or mismatched.service statusincluded. The text says "when default discovery would select a new database", not "only for commands that create stores".store_socketconverts read errors toNone.Validation
Two regression tests in
tests/test_cli.py::ContestedSocket, both verified red against the previous payload (KeyError: 'recover'for the different-socket case; the vanished "directory above" phrasing still present for the ambiguity case):test_the_refusal_prints_commands_an_operator_can_actually_run— every command carries the socket, every candidate is inspected by bothdoctorandservice status, the unnamed-directory phrasing is gone, and the payload states that choosing does not retire the other store.test_the_wrong_socket_refusal_offers_a_matching_pair_not_an_adoption— both sockets appear in the recovery list and the note explains why there is no adoption.Relay suite 792 passed / 30 skipped / 6 subtests.
validate.py,contracts.py,check_operations_contract.py,unittest discover -s scripts/ci/tests,secrets.sh,git diff --checkall exit 0.CRW_PACKAGES_TMPDIR=/var/tmp packages.pyexits 0 — bridge 119 tests, relay 852.Scope
Source and docs only, inside
packages/codex-session-relay/. No behaviour change to delivery, the daemon, or the transport; the only runtime difference is the content of two refusal payloads that already existed.