Keep the Stop hook reachable when its version cache is replaced, and correct what the evidence actually shows - #92
Merged
Conversation
A plugin hook command is fixed when a turn starts, with the plugin root already resolved into it, and the whole turn reuses that string including every Stop re-fire. Installing a version removes the previous cache directory whole, so an update landing mid-turn leaves the command naming a file that is gone. python3 exits 2 for a missing script, and 2 is the hook protocol's blocking code, so the host fed the error back to the model and fired Stop again. Measured on a real host: the same error 74 times in one turn, and a task that could not finish until it was interrupted by hand. The declaration now names two candidates and opens the first it can read: the packaged copy under the plugin root, then a copy runtime_install.py places at <CODEX_HOME>/crw-stop-hook.py. The packaged copy comes first so it is always the current version and an older fallback can never outrank it; the installed copy is reached only when the cache path is already gone. If neither opens, the command exits 0 in silence, which is the launcher's existing contract rather than error suppression: the one failure outside that contract was the interpreter failing to open its own argument. A candidate that opens and then raises is reported as exit 1 and the fallback is not tried, because a launcher that failed after acting has already handled that Stop. The launcher gains the marker the installer identifies its own file by and a configVersion guard, so a copy left by an older install stands down rather than acting on a document written for a later contract. Placement refuses a file without the marker and never follows a symlink, and each file is written under its own lock: there is no lock spanning the launcher and the settings, because every state the pair can be left in is harmless. What an interleaving can still do is make a receipt wrong, so hook and remove read both paths back at the end and report what the host actually held. Two references this package cannot move off the cache: an MCP restart and a skill read inside a session whose version was replaced. docs/plugin-packaging.md now enumerates every reference, states that supported range plainly, and gives the install order that avoids it. Two defects surfaced by the change and fixed with it. launcher_complaints tested for the unreadable-command marker after a word had been taken off the front of it, so the marker sentence was carried forward as a script path; it went unnoticed while every declared command resolved. And the matcher and timeout rode only on the readable declaration shape, so a cache changing only one of those compared equal and was accepted as the replacement. Refs CRW-178.
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.
A launcher that exits non-zero on purpose was turned into a success: the bootstrap caught SystemExit and exited 0 regardless of its code. The launcher's contract is to exit 0 on every path, so a copy that breaks it is saying something, and swallowing that hid the one failure it went out of its way to report. It now surfaces as exit 1, not as the code the launcher chose, because 2 is the blocking code and no path here may produce it. The installer's final read-back accepted any regular file at the stable path. A concurrent installer from another revision leaves one, so this run could report its fallback installed while the bytes belonged to a different checkout. It now compares the digest it wrote. launcher_remove returned settled when the settings had reappeared around it, and settled is read as stopped. The launcher is gone but the packaged copy can answer a Stop again through the restored settings, so that surface is live and now says so. The claim is scoped to remove: disable deletes nothing and the fallback is not its surface to report, so stop_claims takes its claim set as a parameter rather than owning one list for two callers. Refs CRW-178.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ommand
Two follow-ons from the previous round, both found by Devin.
SystemExit.code is not restricted to integers. CPython exits 0 only for
None and an integer zero, and every other object exits 1 even when it is
falsey, so testing the code's truthiness called SystemExit('') and
SystemExit(0.0) successes. The declaration now applies the interpreter's
own rule, and the test matrix carries None, 0, False, 1, 3, 2, '', 0.0,
a string and a list.
remove exited 0 when a step came back live. live_again already said the
adapter is callable again through the restored settings, but verdict only
rejected refused and busy, so shell automation reading the status would
carry on against a host where the surface is not stopped. It now has its
own status, EXIT_INCOMPLETE, for the same reason runtime_install declares
one: bytes really were removed, so this is not a refusal, and the operation
did not remain in effect, so it is not success.
Refs CRW-178.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
int is subclassable and __eq__ is overridable, so comparing the code with zero could dispatch into a method that answers something unrelated to the number the interpreter would actually use. CPython derives the status from the stored value, so the declaration now reads int(code). Found by Devin. The test raises SystemExit from an int subclass whose __eq__ contradicts its value, in both directions: a 5 that claims equality with zero still fails, and a 0 that denies it still succeeds. Refs CRW-178.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
__int__ is overridable too, so int(code) reintroduced the dispatch that switching away from == was meant to remove. int.__int__(code) reads the stored value CPython itself uses, and there is no further level below it. Measured: a launcher raising SystemExit from an int subclass whose __int__ returns 0 while its value is 5 exits 5 when run directly; int(code) == 0 released it as a success and int.__int__(code) == 0 reports it. The test now covers both overrides in both directions. Found by Devin. Refs CRW-178.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
plugin_transition writes the same plugin-owned settings runtime_install writes, through its own settings_install step, so a host migrated with transition --apply ended with the settings and no fallback: the declaration had one candidate again, and the first package replacement during an open turn would land back in the loop this change exists to close. The migration now places the launcher as a step of its own, before settings install, for the reason that ordering exists everywhere else here. The installer's final read-back checked only the launcher. The two files are written under separate locks, so a concurrent removal could retire the settings after write_configuration released its lock and the run would exit 0 with a declared hook that finds nothing to act on. Both paths are read back now and the receipt carries settingsObserved beside launcherObserved. Found by Devin, the first one at high severity and rightly so: it was a hole in the fix's coverage rather than a rough edge on it. Refs CRW-178.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
launcher_install caught hostrecord.Busy and returned an ordinary busy result, and the transition loop only breaks on a refusal. So a contended launcher lock let the sequence carry on and settings install wrote a plugin-owned document for a host whose fallback was never placed, which is exactly the combination the step was added to prevent. Busy now propagates the way every other ordered step leaves it to do. transition() already owns that handler: it names the step that was running and marks the rest not reached. The test holds the launcher lock and asserts the step reports busy, that settings install is not reached, that no launcher was placed and that no plugin-owned settings landed. The settings the retire step had already archived stay archived; that window belongs to the retire-first order and is the same for every step that refuses after it. Found by Devin. Refs CRW-178.
…t bound Two defects an independent audit of the delivered change found. The launcher step sat after settings retire and hook standdown, which are destructive. It is the one step here that can refuse on a condition outside this command -- a foreign file at the path, or another run holding its lock -- so a refusal left the host with its settings archived and its manual registration removed and nothing to put them back. That is a worse host than the one the command started with. The step now runs first. Two cases cover it: a held lock and a foreign file, each asserting that retire, standdown and install all report not_reached and that the settings and the hook document are exactly what they were. The plugin-owned budget was refused only at the launcher ceiling of 9, but the launcher waits min(budget + 2, 9), so 8 collapses the margin: the launcher's deadline is 9 while the adapter is still allowed 8, and the launcher kills it in the middle of writing the record of its own timeout. The bound was enforced in the transition alone, so runtime_install.py hook --owner plugin wrote documents the transition refused. It now lives beside the validation both writers share, and the transition's names are aliases of it so they cannot drift apart again. Refs CRW-178.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…llback Devin raised a host carrying the old eight second budget: the placement would install a fallback and the settings would then be refused, leaving a fallback wired to a document this command had just called unusable. Measured instead of assumed, and the scenario does not occur. The ownership precondition reads the live document and runs the same complaints() over it, so such a host is refused there, before the placement block is reached at all: the receipt carries no launcher cell and nothing is written. An explicit second gate was written first and then removed once the measurement showed it was unreachable; dead code with a confident comment is worse than none. What the change keeps is a case that pins the ordering, because the placement is only safe while some earlier precondition keeps an unusable document from ever reaching it. Also corrects the launcher comment, which still named the transition as the owner of the guard bound after that bound moved to the module both writers share. Refs CRW-178.
runtime-install.md owns the hook command and said nothing about the file that command now places. It gets the contract: who writes it, in what order and why, what it refuses, what the marker proves and what it does not, how it is replaced, who removes it, what disable does to it, why there is no lock spanning it and the settings, and where the seven second guard bound comes from. plugin-transition.md listed the migration's steps and did not list the one that was added to it. The launcher is step 1 now, ahead of everything destructive, and the surrounding step numbers and references move with it. The two ordering regressions compared parsed documents, which read equal across a rewrite that only reorders keys or changes spacing. "Nothing was taken away" is a claim about the files, so they compare raw bytes now. Refs CRW-178.
The wiring note told a contributor that the launchers ship in the package and that the version cache is replaced wholesale, and then stopped. It did not say that the same sentence applies to the launchers themselves, so the file the Stop declaration falls back to, and the two commands that place it, were invisible to anyone reading the repository's own instructions. Refs CRW-178.
…cessary A previous commit answered this finding with a measurement showing the scenario could not occur, and that measurement was of the wrong path. It used a host that already carried a bad document, which the ownership precondition catches before the placement. A fresh host with the budget on the command line takes a different road: the document was only judged inside write_configuration, which runs after the launcher is placed, so --guard-timeout 8 left a launcher on disk and then refused the settings. Caught by an end-to-end probe at the delivered head, not by the tests. The judgement moves into the preconditions, where "nothing was written" is promised, so the run now refuses before touching anything. The regression drives the real command on a fresh home for 8 and 7 and asserts both the launcher and the settings are absent for the refused budget and present for the accepted one. Refs CRW-178.
The supported range promised that an update landing between turns was safe for Stop, for an MCP restart and for skill reads, and that the next turn resolved everything afresh. Nothing measured establishes that a turn boundary re-resolves a package reference; a task outlives many turns and an idle interval is not a session reload. The MCP restart and skill read outcomes are inferred from the declaration constraint this section already explains, not observed, so the reference table now says so in the rows that carry them rather than stating a flat outcome next to a table that calls the same thing an inference. The repeated-prompt count was wrong in both number and provenance. 74 is a line count over rollout-repro-before.jsonl, where each prompt is serialized twice, once as response_item and once as event_msg; that file holds 37 distinct prompts in one turn and its paths are under a scratch reproduction home. The user's host measured 11 in one turn of 01a0bf20 and 8 in one turn of 01a0bfa0, so two tasks were blocked rather than one. Both numbers are reported with the source they came from. The exposure is scoped to a task still holding the old command instead of to an open turn, because an open turn is a sufficient condition and was being read as the only one. For the same reason a running task is described as holding cache-bound references to the version it started with rather than keeping that version, which the sentence about removing the directory already contradicted. Updating safely no longer takes an idle turn as its precondition and no longer promises the previous cache is kept until nothing references it, and swap-state is no longer offered as the check before releasing a preserved copy: it reads the pointer and the host records, never the tasks holding references, so it cannot establish that the last reader is gone. The five comments that describe an open turn as the trigger are left alone; each states a true sufficient condition. Only the count leaves the code. Refs CRW-178.
The corrected paragraph kept the original's ending: a task that could not finish until it was interrupted by hand. The records do not carry that. Turn 01a0c288 of 01a0bfa0 emitted task_complete at 06:35:13Z and turn 01a0c2a2 of 01a0bf20 at 06:35:59Z, neither with turn_aborted. What ended the loop was a compatibility path restored at the missing location, not a hand interrupting the turn. The three places that repeated it now say completion was blocked while that path was absent and resumed once it was restored. The launcher's own docstring said the installed copy exists for "the one case" the cache cannot answer, a plugin update landing in the middle of a turn. That exclusivity is the same overclaim the documents just lost: an open turn is a sufficient condition and not the only one, and nothing measured shows a later turn of the same task resolving the command afresh. The sentence names the task as the thing that holds the command instead. The docstring is the only change in that file and no executable line moves. Refs CRW-178.
The record for a temporary compatibility file was told to carry "the command that shows whether anything still references it". This document now says, a few lines above, that swap-state reads the pointer and the host records and not the tasks holding references, so it cannot establish that the last reader is gone. Nothing else here enumerates those tasks either: the relay's own currency and delivery paths reason about revisions and delivery rows, never about which task holds a path. The instruction therefore asked an operator to record something the repository cannot produce, which is the same shape of claim the rest of this change removed. It asks for the evidence instead, and says where that evidence has to come from. Refs CRW-178.
thisisjun786
changed the title
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 defect
A plugin hook command is fixed when a turn starts, with
${PLUGIN_ROOT}already resolved into it, and the whole turn reuses that string including every Stop re-fire. Installing a version removes the previous cache directory whole, so an update landing while a task still holds that command leaves it naming a file that is gone.python3exits 2 for a missing script, and 2 is the hook protocol's blocking code, so the host feeds the error back to the model and fires Stop again.Measured on the user's host on 2026-09-21. Two tasks were caught, not one:
01a0bf20-d8ee-7c11-8da6-df3c969f666f01a0c2a2-9be7-7c91-bcd1-273e5abe546atask_completeat 06:35:59.494Z01a0bfa0-1593-7981-82ae-280cff4de28401a0c288-571f-79e3-9f2c-973dca9de6ddtask_completeat 06:35:13.170ZBoth were blocked while
.../crw/0.3.0/wiring/crw_stop_hook.pywas absent and both finished once a compatibility path was restored there. Neither was interrupted by hand. An isolatedCODEX_HOMEreproduction with real plugin bytes produced 37 repeated prompts in one turn.Nothing measured establishes that a later turn of the same task re-resolves the command, so the exposure is scoped to a task still holding it rather than to an open turn.
The change
The Stop declaration names two candidates and opens the first it can read:
${PLUGIN_ROOT}/wiring/crw_stop_hook.py— the packaged copy, always the current version.<CODEX_HOME>/crw-stop-hook.py— a copy the installer places, reached only when the first is already gone.If neither opens it exits 0 and prints nothing. That is the launcher's existing contract, not error suppression: a Stop it cannot judge is a Stop it releases, and the one failure outside that contract was the interpreter failing to open its own argument. A candidate that opens and then raises is reported as exit 1 and the fallback is not tried, because a launcher that failed after acting has already handled that Stop. A launcher that exits non-zero deliberately is reported too, following the interpreter's own rule rather than the code's truthiness:
int.__int__(code), soSystemExit(''),SystemExit(0.0)and anintsubclass lying through__eq__or__int__are all judged by the value CPython would use. Nothing ever emits 2.The packaged copy comes first so a fallback left by an older install can never outrank it. The launcher carries the marker the installer identifies its own file by and a
configVersionguard, so a copy left by an older install stands down rather than acting on a document written for a later contract.Both writers place it.
runtime_install.py hook --owner plugindoes, and so doesplugin_transition.py transition, which writes the same plugin-owned settings and would otherwise leave a migrated host with the settings and no fallback. In the migration it is step 1, ahead of everything destructive: it is the only step that can refuse on a condition outside the command — a foreign file at the path, or another run holding its lock — and placed later such a refusal left the host with its settings archived and its manual registration removed and nothing to put them back.Placement refuses a file without the marker and never follows a symlink. Launcher and settings are written under separate locks: every state the pair can be left in is harmless, so no cross-file lock is needed. What an interleaving can still do is make a receipt wrong, so
hookandremoveread both paths back at the end, reportlauncherObservedandsettingsObserved, and exit non-zero when either half is missing, changed, or no longer what the run wrote.Correcting what the documents claimed about it
The lifecycle documentation overstated what this fix and the surrounding evidence establish, in the same measured register as the parts that were real. That is corrected here, because shipping a fallback under false claims about its reach is the more expensive half of the defect.
The supported range. The table said an update landing between turns was safe for Stop, for an MCP restart and for skill reads, and that "the next turn resolves everything afresh". Nothing measured establishes that a turn boundary re-resolves a package reference; a task outlives many turns and an idle interval is not a session reload. The table now keys on the task holding the old reference and says plainly that the cache-bound references are not measured as safe.
Measured versus inferred. A same-session MCP restart failure and a post-removal skill read failure are derived from the host's declaration constraint, not observed. The reference table said them as flat fact while the range table called the same thing an inference. Both now agree, and each says which it is.
The count. "74 times in one turn" was a line count over
rollout-repro-before.jsonl, where every prompt is serialized twice, once asresponse_itemand once asevent_msg. That file holds 37 distinct prompts, and its paths are under a scratch reproduction home rather than the user's host. The production numbers are 11 and 8, across two tasks. The claim also said the task "could not finish until it was interrupted by hand"; both turns emittedtask_completeonce the path was restored.The procedure. "Check that no turn is open" is not a sufficient precondition, and
codex plugin addremoves the prior cache, so the procedure no longer promises the previous directory is kept until nothing references it.swap-stateis no longer offered as the check before releasing a preserved copy: it reads the pointer and the host records, never the tasks holding references. For the same reason the record for a temporary compatibility file no longer asks for "the command that shows whether anything still references it" — nothing here enumerates those tasks, so it asks for the evidence and says it has to come from the host.Retained deliberately. Five comments describe an open turn as the trigger. Each states a true sufficient condition and is left alone. Only the false count left the code, in one docstring.
What this does not fix
An MCP restart and a skill read inside a session whose version was replaced are expected to fail, inferred from the host restricting an MCP
cwdto a contained./,${PLUGIN_ROOT}or${PLUGIN_DATA}and reading skills from the directory the manifest names, so nothing this package declares can point them outside the cache. Neither was independently measured here, and the documents now say so.Defects surfaced while fixing this
launcher_complaintstested for the unreadable-command marker after a word had been taken off the front of it, so the marker sentence was carried forward as a script path. Latent while every declared command resolved.min(budget + 2, 9), so 8 collapsed the margin and could kill the adapter mid-record. The bound existed in the transition alone, so the installer wrote documents the transition refused. It now lives with the validation both writers share.Verification
python3 scripts/ci/plugin.pyexit 0,python3 scripts/ci/validate.pyexit 0,git diff --checkclean.python3 -m unittest discover -s scripts/ci/tests— 1715 tests, OK, 3 skipped, bound to the commit by a check receipt withdirty: false.python3 scripts/ci/packages.py— codex-thread-bridge 295 tests, codex-session-relay 2126 tests, both wheels built.Isolated
CODEX_HOMEwith real plugin bytes, anapp-serverdaemon and a turn held open across a realcodex plugin add:Also verified: a new task on the new version, the removal path, a foreign file and a symlink at the stable path (both refused, bytes untouched, and no settings written — which is what the launcher-first order is for), a held launcher lock and a foreign launcher each stopping the migration before anything is taken away, no process restarted, and no approval or sandbox setting changed.
The documentation correction carries no automated gate:
validate.pyresolves link paths anddev-gateaggregates job results, and neither can check whether a sentence is true. It rests on the counts re-derived from the rollouts and the incident record, and on two independent review rounds against the diff.Changing the command text changes the hook's
trusted_hash, so this update needs one re-trust per installed hook identity. Trust is keyed to declaration content, not to the version path.Refs CRW-178. Real-verification defect of CRW-116; its canonical design text is unchanged.