From 16f1fd4dceeffa1929a2f2fa3f605c28f302fc97 Mon Sep 17 00:00:00 2001 From: thisisjun786 <259586770+thisisjun786@users.noreply.github.com> Date: Mon, 21 Sep 2026 18:49:57 +0900 Subject: [PATCH 01/15] Keep the Stop hook reachable when its version cache is replaced 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 /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. --- docs/plugin-packaging.md | 82 +++++ plugins/crw/wiring/crw_stop_hook.py | 25 ++ .../hooks/stop-recording-completion.json | 2 +- scripts/ci/tests/test_plugin_transition.py | 16 +- scripts/ci/tests/test_plugin_wiring.py | 332 ++++++++++++++++++ scripts/crw_runtime/completion.py | 185 ++++++++++ scripts/crw_transition/steps.py | 116 +++++- scripts/runtime_install.py | 61 +++- 8 files changed, 797 insertions(+), 22 deletions(-) diff --git a/docs/plugin-packaging.md b/docs/plugin-packaging.md index 7c9c52e24..dfbc3c69c 100644 --- a/docs/plugin-packaging.md +++ b/docs/plugin-packaging.md @@ -187,6 +187,88 @@ install the plugin again: a cached version changes only on installation, and a t already running keeps the package its session started with. +## The cache lifetime + +Installing a version replaces the cache directory whole. `codex plugin add` removes the +previous version directory, and so does a rollback to an earlier one. Anything a running +session still points into that directory stops resolving at that moment, and the question for +each declared surface is whether its reference outlives the directory it names. + +| Reference | Bound to the cache | What a replacement does to it | Owner | +| --- | --- | --- | --- | +| Stop launcher, first candidate | Yes | Falls through to the second candidate | This package | +| Stop launcher, second candidate at `/crw-stop-hook.py` | No | Nothing | `runtime_install.py hook --owner plugin` | +| Stop settings at `/crw-completion-hook.json` | No | Nothing | The same command | +| Adapter, relay and bridge executables | No, they sit under the installer pointer | Nothing | `runtime_install.py install` | +| Hook document path in the run identifier | Yes | Held as an identifier and never re-read | The host | +| MCP start `cwd` and `args` | Yes | A server already running survives, because it has already replaced itself with the installed bridge. A restart inside that session fails | The host | +| Skill reads | Yes | The read fails at the moment it is made | The host | + +Two of those this package can answer for and two it cannot, and the difference is a host rule +rather than a preference. A hook command goes through a shell, so it can be written to resolve +its own program at run time. An MCP `command` must be a bare executable name or a contained +`./` path, and its `cwd` must be a contained `./` path, `${PLUGIN_ROOT}` or `${PLUGIN_DATA}`; +skills are read by the host from the directory the manifest names. Neither can be pointed +outside the version cache by anything this package declares. + +### Why the Stop hook is declared as a bootstrap + +A 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. Replace the package while a +turn is open and the command names a file that no longer exists. `python3` exits **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 a real host: one removed directory, the same +error 74 times in one turn, and a task that could not finish until it was interrupted by hand. + +So the declaration names two candidates and opens the first one it can read: + +1. `${PLUGIN_ROOT}/wiring/crw_stop_hook.py` — the packaged copy. Always the current version, so a + fallback left by an older install can never outrank it. +2. `/crw-stop-hook.py` — the copy `runtime_install.py` places. Reached only when + the first one is already gone. + +If neither can be opened it exits 0 and prints nothing. That is not error suppression: the +launcher’s own contract has always been that 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 fails while running is a different thing and is reported, as +exit 1, which the host reads as an ordinary failure rather than as a hold. Once a candidate has +been read it owns that Stop: the second one is not tried, because a launcher that raised after +doing half its work has already acted on the turn. + +Changing the command text changes the hook’s `trusted_hash`, so an update that changes it needs +one re-trust per installed hook identity. Trust is keyed to the declaration content and not to +the version path, so an update that leaves the command alone keeps its trust. + +### The supported range + +| When the update lands | Stop | MCP restart | Skill reads | +| --- | --- | --- | --- | +| Between turns | Safe: the next turn resolves everything afresh | Safe | Safe | +| During a turn | Safe: the second candidate answers | **Still fails** | **Still fails** | + +The second row is the honest limit. This package cannot move those two references off the +cache, so an update during an open turn is avoided rather than survived. + +### Updating safely + +1. Check that no turn is open. +2. Run `python3 scripts/runtime_install.py hook --adapter completion --owner plugin --apply` + first, so the fallback is current before the directory it backs up can disappear. +3. Run `codex plugin add crw@`. +4. Re-trust the hook once if its command changed. +5. Keep the previous version directory until nothing references it, then release it. + +Preserving that directory is an operator step. `codex plugin add` removes it and this +repository does not own that command, so nothing here can hold it open. +`python3 scripts/plugin_transition.py swap-state` reports what a replacement actually left, +and that is what to read before releasing a preserved copy. + +A host carrying temporary compatibility files — an old cache path kept alive by hand after an +update went wrong — needs a record of its own, kept with the task record outside this +repository. Record the path, who made it, why, the condition under which it may be removed, and +the command that shows whether anything still references it. Such a file is a repair, not a +guarantee that the next update will be survivable. + ## Update and roll back The cache keeps one version per plugin, and installing a new version replaces the diff --git a/plugins/crw/wiring/crw_stop_hook.py b/plugins/crw/wiring/crw_stop_hook.py index ed63874c9..5dac5dd32 100644 --- a/plugins/crw/wiring/crw_stop_hook.py +++ b/plugins/crw/wiring/crw_stop_hook.py @@ -13,6 +13,16 @@ It parses no arguments. The host reads exit 2 as the blocking code and argparse exits 2 on any usage error, so there is no argument parser here and nothing imported that has one. +It is not reached directly. The declaration runs a fixed bootstrap that opens the first of two +candidates it can read: this file under the version cache, and a copy the runtime installer +places at /crw-stop-hook.py. The cache copy comes first so the current version +always wins and a stale copy can never outrank it; the installed copy exists for the one case +the cache cannot answer, which is a plugin update landing in the middle of a turn. The turn's +hook command is fixed when the turn starts, so a cache directory removed underneath it leaves +an absolute path to a file that is gone, and python3 exits 2 for a missing script -- the same +number the hook protocol reads as "block this turn". That collision is what turned one missing +file into a termination loop, and it is why the declaration no longer names only a cache path. + It writes nothing to stderr and exits 0 on every path, including the paths where it does nothing at all. An unreliable detector must degrade into no detector, never into a stuck session. @@ -34,6 +44,18 @@ SETTINGS_NAME = "crw-completion-hook.json" PLUGIN_OWNER = "plugin" +# What marks a copy of this launcher as CRW's to replace or remove. It says whose file this is +# and nothing more: it does not establish who wrote it, and it does not establish that the bytes +# around it are intact. Those are separate questions, and the installer answers the second one +# by reporting this file's digest beside the checkout's rather than by trusting the marker. +LAUNCHER_MARKER = "crw-stop-hook/1" +# The settings contract this launcher implements, mirrored from scripts/crw_runtime/completion.py +# CONFIG_VERSION, which this file cannot import. An installed copy outlives the package that +# wrote it, so it can meet a document written for a later contract. Absent reads as the first +# contract, because a host that installed before the key existed holds a document without it. +# Anything else is a contract this copy does not implement, and the answer to that is to stand +# down rather than to act on a document it would be guessing about. +CONFIG_VERSION = 1 # Kept under the timeout this hook is registered with, so the host does not kill the adapter # in the middle of recording why it could not answer. MARGIN_SECONDS = 2 @@ -78,6 +100,9 @@ def adapter_call(document): """ if not isinstance(document, dict) or document.get("owner") != PLUGIN_OWNER: return None + version = document.get("configVersion") + if version is not None and version != CONFIG_VERSION: + return None interpreter = document.get("adapterInterpreter") entry_point = document.get("adapterEntryPoint") for value in (interpreter, entry_point): diff --git a/plugins/crw/wiring/hooks/stop-recording-completion.json b/plugins/crw/wiring/hooks/stop-recording-completion.json index 70d2fbe65..ed61fd659 100644 --- a/plugins/crw/wiring/hooks/stop-recording-completion.json +++ b/plugins/crw/wiring/hooks/stop-recording-completion.json @@ -5,7 +5,7 @@ "hooks": [ { "type": "command", - "command": "python3 \"${PLUGIN_ROOT}/wiring/crw_stop_hook.py\"", + "command": "python3 -c \"\nimport os, sys\nnamed = os.environ.get('CODEX_HOME')\nhome = named if named else os.path.join(os.path.expanduser('~'), '.codex')\nfor candidate in (sys.argv[1] if len(sys.argv) > 1 else '',\n os.path.join(home, 'crw-stop-hook.py')):\n if not candidate:\n continue\n try:\n with open(candidate, 'rb') as handle:\n source = handle.read()\n except OSError:\n continue\n try:\n exec(compile(source, candidate, 'exec'),\n {'__name__': '__main__', '__file__': candidate})\n except SystemExit:\n pass\n break\nraise SystemExit(0)\n\" \"${PLUGIN_ROOT}/wiring/crw_stop_hook.py\"", "timeout": 10, "statusMessage": "(crw) Checking the completion records" } diff --git a/scripts/ci/tests/test_plugin_transition.py b/scripts/ci/tests/test_plugin_transition.py index 13d156b93..831c0245b 100644 --- a/scripts/ci/tests/test_plugin_transition.py +++ b/scripts/ci/tests/test_plugin_transition.py @@ -2519,8 +2519,7 @@ def test_a_cached_package_that_declares_something_else_is_refused(self): before = host.hooks_document() code, answer = host.transition("--apply") self.assertEqual(code, 1, json.dumps(answer["results"])[:700]) - self.assertIn("this checkout declares python3 wiring/crw_stop_hook.py", - answer["results"][0]["detail"]) + self.assertIn("this checkout declares", answer["results"][0]["detail"]) self.assertIn("the same surface, once each", answer["results"][0]["detail"]) self.assertEqual(host.hooks_document(), before) @@ -2561,8 +2560,7 @@ def test_a_cache_whose_manifest_declares_other_documents_is_refused(self): before = host.hooks_document() code, answer = host.transition("--apply") self.assertEqual(code, 1, json.dumps(answer["results"])[:700]) - self.assertIn("this checkout declares python3 wiring/crw_stop_hook.py", - answer["results"][0]["detail"]) + self.assertIn("this checkout declares", answer["results"][0]["detail"]) self.assertEqual(host.hooks_document(), before) def test_a_command_that_only_mentions_the_launcher_is_not_running_it(self): @@ -2689,8 +2687,8 @@ def test_a_cache_declaring_a_python_that_is_not_one_is_refused(self): before = host.hooks_document() code, answer = host.transition("--apply") self.assertEqual(code, 1, json.dumps(answer["results"])[:700]) - self.assertIn("this checkout declares python3 wiring/crw_stop_hook.py", - answer["results"][0]["detail"]) + self.assertIn("python3-does-not-exist", answer["results"][0]["detail"]) + self.assertIn("this checkout declares", answer["results"][0]["detail"]) self.assertEqual(host.hooks_document(), before) def test_a_watched_path_is_locked_across_the_removal_too(self): @@ -2901,10 +2899,8 @@ def test_an_interpreter_somewhere_else_is_not_the_one_this_package_declares(self before = host.hooks_document() code, answer = host.transition("--apply") self.assertEqual(code, 1, json.dumps(answer["results"])[:700]) - self.assertIn("/definitely/missing/python3 wiring/crw_stop_hook.py", - answer["results"][0]["detail"]) - self.assertIn("this checkout declares python3 wiring/crw_stop_hook.py", - answer["results"][0]["detail"]) + self.assertIn("/definitely/missing/python3", answer["results"][0]["detail"]) + self.assertIn("this checkout declares", answer["results"][0]["detail"]) self.assertEqual(host.hooks_document(), before) def test_a_cached_command_that_runs_the_launcher_twice_is_refused(self): diff --git a/scripts/ci/tests/test_plugin_wiring.py b/scripts/ci/tests/test_plugin_wiring.py index 8cb692ee0..4b65267c7 100644 --- a/scripts/ci/tests/test_plugin_wiring.py +++ b/scripts/ci/tests/test_plugin_wiring.py @@ -13,6 +13,7 @@ import json import os from pathlib import Path +import shutil import subprocess import sys import tempfile @@ -1125,3 +1126,334 @@ def test_a_cached_manifest_that_is_not_an_object_refuses_rather_than_crashing(se self.assertNotEqual(emitted.get("outcome"), "internal_error") self.assertIn("could not be read", emitted["detail"]) self.assertEqual(self.config(), before) + + +# ------------------------------------------------- the declaration that outlives the cache + + +class DeclaredStopCommandTest(unittest.TestCase): + """CRW-178. The command a turn holds must survive its version cache being replaced. + + A plugin hook command is fixed when the turn starts, with the plugin root already resolved + into it. Replacing the package removes that directory whole, and python3 exits 2 for a + missing script -- the number the hook protocol reads as "block this turn". Measured on the + real incident: one removed directory, 74 repeated Stop prompts, and a turn that could not + end. So these cases drive the ACTUAL declared command through a shell, the way the host + runs it, rather than asserting anything about its text. + """ + + DECLARATION = ROOT / "plugins/crw/wiring/hooks/stop-recording-completion.json" + PAYLOAD = b'{"hook_event_name": "Stop", "session_id": "s", "turn_id": "t"}' + + def setUp(self): + self.stack = contextlib.ExitStack() + self.addCleanup(self.stack.close) + self.root = Path(self.stack.enter_context(tempfile.TemporaryDirectory(prefix="crw178-"))) + document = json.loads(self.DECLARATION.read_text(encoding="utf-8")) + entry = document["hooks"]["Stop"][0]["hooks"][0] + self.command = entry["command"] + self.timeout = entry["timeout"] + self.witness = self.root / "witness.txt" + + def plant(self, path, tag, body="raise SystemExit(0)\n"): + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text("open(%r, 'a').write(%r + chr(10))\n%s" % (str(self.witness), tag, body), + encoding="utf-8") + + def fire(self, plugin_root, codex_home, command=None, with_root=True): + if self.witness.exists(): + self.witness.unlink() + environment = {"PATH": os.environ["PATH"], "CODEX_HOME": str(codex_home)} + if with_root: + environment["PLUGIN_ROOT"] = str(plugin_root) + done = subprocess.run(["/bin/sh", "-lc", command or self.command], input=self.PAYLOAD, + stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=environment) + ran = self.witness.read_text(encoding="utf-8").split() if self.witness.exists() else [] + # The one number that must never appear, whatever else happened. + self.assertNotEqual(done.returncode, 2, + "the blocking exit code escaped: " + repr(done.stderr[:200])) + return done, ran + + def homes(self): + """An ordinary home, and one whose path carries a space, a quote and a dollar.""" + plain = self.root / "plain home" + awkward = self.root / "it's $HOME really" + for home in (plain, awkward): + home.mkdir(parents=True, exist_ok=True) + return plain, awkward + + def test_the_packaged_copy_runs_while_the_cache_is_there(self): + """The current version always wins, so an older fallback can never outrank it.""" + cache = self.root / "cache" / "0.4.0" + self.plant(cache / "wiring" / "crw_stop_hook.py", "PACKAGED") + for home in self.homes(): + self.plant(home / "crw-stop-hook.py", "FALLBACK") + done, ran = self.fire(cache, home) + self.assertEqual((done.returncode, ran), (0, ["PACKAGED"]), str(home)) + self.assertEqual(done.stdout, b"") + + def test_the_fallback_answers_when_the_cache_was_replaced_mid_turn(self): + """The incident itself: the path the turn holds is gone before the turn ends.""" + gone = self.root / "cache" / "0.3.0-removed" + for home in self.homes(): + self.plant(home / "crw-stop-hook.py", "FALLBACK") + done, ran = self.fire(gone, home) + self.assertEqual((done.returncode, ran), (0, ["FALLBACK"]), str(home)) + self.assertEqual((done.stdout, done.stderr), (b"", b"")) + + def test_neither_candidate_releases_the_turn_in_silence(self): + """A host with the package and no runtime has nothing to run and loses nothing. + + This is the case the old declaration turned into a termination loop, and it is not a + blanket suppression: the launcher's own contract is already that a Stop it cannot judge + is a Stop it releases. What sat outside that contract was the interpreter failing to + open its own argument, and that is what this closes. + """ + gone = self.root / "cache" / "0.3.0-removed" + home = self.root / "empty home" + home.mkdir() + done, ran = self.fire(gone, home) + self.assertEqual((done.returncode, ran, done.stdout, done.stderr), (0, [], b"", b"")) + + def test_a_candidate_that_fails_while_running_is_reported_and_not_retried(self): + """Read failure and run failure are different, and only the first may fall through. + + A launcher that raises after doing half its work has already acted on this Stop, so + running the fallback as well would process one Stop twice. The error surfaces instead, + as exit 1, which the host reads as an ordinary failure rather than as a hold. + """ + cache = self.root / "cache" / "0.4.0" + self.plant(cache / "wiring" / "crw_stop_hook.py", "PACKAGED", + "raise OSError(13, 'after side effects')\n") + home = self.root / "home" + home.mkdir() + self.plant(home / "crw-stop-hook.py", "FALLBACK") + done, ran = self.fire(cache, home) + self.assertEqual(ran, ["PACKAGED"]) + self.assertEqual(done.returncode, 1) + self.assertIn(b"OSError", done.stderr) + + def test_a_truncated_launcher_is_reported_rather_than_run(self): + cache = self.root / "cache" / "0.4.0" + (cache / "wiring").mkdir(parents=True) + (cache / "wiring" / "crw_stop_hook.py").write_text("def (\n", encoding="utf-8") + home = self.root / "home" + home.mkdir() + done, ran = self.fire(cache, home) + self.assertEqual((done.returncode, ran), (1, [])) + self.assertIn(b"SyntaxError", done.stderr) + + def test_a_directory_at_either_candidate_is_stepped_over(self): + cache = self.root / "cache" / "0.4.0" + (cache / "wiring" / "crw_stop_hook.py").mkdir(parents=True) + home = self.root / "home" + home.mkdir() + (home / "crw-stop-hook.py").mkdir() + done, ran = self.fire(cache, home) + self.assertEqual((done.returncode, ran, done.stdout), (0, [], b"")) + + def test_an_unexpanded_plugin_root_falls_through_rather_than_failing(self): + """A host that does not substitute the variable leaves the literal text behind.""" + home = self.root / "home" + home.mkdir() + self.plant(home / "crw-stop-hook.py", "FALLBACK") + done, ran = self.fire(None, home, with_root=False) + self.assertEqual((done.returncode, ran), (0, ["FALLBACK"])) + + def test_the_real_launcher_still_answers_through_the_declaration(self): + """Not a stub: the packaged launcher, reached the way the host reaches it.""" + cache = self.root / "cache" / "0.4.0" + (cache / "wiring").mkdir(parents=True) + shutil.copyfile(ROOT / "plugins/crw/wiring/crw_stop_hook.py", + cache / "wiring" / "crw_stop_hook.py") + home = self.root / "home" + home.mkdir() + adapter = home / "adapter.py" + seen = home / "seen.txt" + adapter.write_text("import sys\n" + "open(%r, 'a').write(sys.stdin.read())\n" + "sys.stdout.write('{\"decision\": \"block\"}')\n" % str(seen), + encoding="utf-8") + (home / "crw-completion-hook.json").write_text(json.dumps( + {"configVersion": 1, "event": "Stop", "owner": "plugin", "mode": "observe", + "timeoutSeconds": 5, "adapterInterpreter": sys.executable, + "adapterEntryPoint": str(adapter)}), encoding="utf-8") + done, _ = self.fire(cache, home) + self.assertEqual((done.returncode, done.stdout), (0, b'{"decision": "block"}')) + self.assertIn("turn_id", seen.read_text(encoding="utf-8")) + + def test_the_declaration_stays_within_the_timeout_the_host_clamps(self): + self.assertLessEqual(self.timeout, plugin.HOOK_TIMEOUT_SECONDS) + + +class LauncherContractVersionTest(unittest.TestCase): + """An installed fallback outlives the package that wrote it, so it may meet a later contract.""" + + LAUNCHER = ROOT / "plugins/crw/wiring/crw_stop_hook.py" + + def setUp(self): + self.stack = contextlib.ExitStack() + self.addCleanup(self.stack.close) + self.home = Path(self.stack.enter_context(tempfile.TemporaryDirectory(prefix="crw178-"))) + self.seen = self.home / "seen.txt" + self.adapter = self.home / "adapter.py" + self.adapter.write_text("open(%r, 'a').write('ran')\n" % str(self.seen), + encoding="utf-8") + + def fire(self, **overrides): + document = {"configVersion": 1, "event": "Stop", "owner": "plugin", "mode": "observe", + "timeoutSeconds": 5, "adapterInterpreter": sys.executable, + "adapterEntryPoint": str(self.adapter)} + document.update(overrides) + for key in [name for name, value in document.items() if value is None]: + del document[key] + (self.home / "crw-completion-hook.json").write_text(json.dumps(document), + encoding="utf-8") + done = subprocess.run([sys.executable, str(self.LAUNCHER)], input="{}", + capture_output=True, text=True, + env={"PATH": os.environ["PATH"], "CODEX_HOME": str(self.home)}) + return done, self.seen.exists() + + def test_the_contract_it_implements_is_acted_on(self): + done, ran = self.fire() + self.assertEqual((done.returncode, ran), (0, True)) + + def test_settings_written_before_the_key_existed_are_still_acted_on(self): + done, ran = self.fire(configVersion=None) + self.assertEqual((done.returncode, ran), (0, True)) + + def test_a_contract_it_does_not_implement_is_stood_down_from(self): + """A copy left by an older install does nothing rather than guess at a later document.""" + done, ran = self.fire(configVersion=2) + self.assertEqual((done.returncode, done.stdout, done.stderr, ran), (0, "", "", False)) + + def test_the_marker_it_carries_is_the_one_the_installer_looks_for(self): + self.assertIn(completion.LAUNCHER_MARKER, self.LAUNCHER.read_text(encoding="utf-8")) + + +class StableLauncherPlacementTest(unittest.TestCase): + """Who may write the fallback, and what it refuses to write over.""" + + SOURCE = ROOT / "plugins/crw/wiring/crw_stop_hook.py" + + def setUp(self): + self.stack = contextlib.ExitStack() + self.addCleanup(self.stack.close) + self.home = Path(self.stack.enter_context(tempfile.TemporaryDirectory(prefix="crw178-"))) + self.path = completion.launcher_path(self.home) + + def place(self, **kwargs): + return completion.place_launcher(self.path, self.SOURCE, **kwargs) + + def test_a_dry_run_writes_nothing_and_says_what_it_would_do(self): + answer = self.place() + self.assertEqual(answer["outcome"], completion.LAUNCHER_WOULD_PLACE) + self.assertFalse(answer["applied"]) + self.assertFalse(self.path.exists()) + + def test_applying_installs_the_bytes_this_checkout_ships_and_reads_them_back(self): + answer = self.place(apply=True) + self.assertEqual(answer["outcome"], completion.LAUNCHER_PLACED) + self.assertEqual(self.path.read_bytes(), self.SOURCE.read_bytes()) + self.assertEqual(answer["digest"], answer["sourceDigest"]) + + def test_a_second_run_changes_nothing(self): + self.place(apply=True) + self.assertEqual(self.place(apply=True)["outcome"], completion.LAUNCHER_UNCHANGED) + + def test_a_file_without_the_marker_is_left_exactly_where_it_is(self): + """Ownership, not provenance: what it proves is that CRW put a launcher here.""" + self.path.write_text("not ours\n", encoding="utf-8") + answer = self.place(apply=True) + self.assertEqual(answer["outcome"], completion.LAUNCHER_FOREIGN) + self.assertEqual(self.path.read_text(encoding="utf-8"), "not ours\n") + + def test_a_symlink_is_reported_by_kind_and_never_followed(self): + target = self.home / "elsewhere.py" + target.write_text("someone else\n", encoding="utf-8") + self.path.symlink_to(target) + answer = self.place(apply=True) + self.assertEqual(answer["outcome"], completion.LAUNCHER_NOT_A_FILE) + self.assertEqual(answer["kind"], "symlink") + self.assertTrue(self.path.is_symlink()) + self.assertEqual(target.read_text(encoding="utf-8"), "someone else\n") + + def test_a_directory_is_refused_rather_than_replaced(self): + self.path.mkdir() + answer = self.place(apply=True) + self.assertEqual((answer["outcome"], answer["kind"]), + (completion.LAUNCHER_NOT_A_FILE, "directory")) + self.assertTrue(self.path.is_dir()) + + def test_a_packaged_source_that_cannot_be_read_places_nothing(self): + answer = completion.place_launcher(self.path, self.home / "absent.py", apply=True) + self.assertEqual(answer["outcome"], completion.LAUNCHER_SOURCE_MISSING) + self.assertFalse(self.path.exists()) + + def test_the_state_reader_separates_the_marker_from_the_digest(self): + self.place(apply=True) + state = completion.launcher_state(self.home, self.SOURCE) + self.assertEqual((state["kind"], state["carriesMarker"], state["matchesCheckout"]), + ("file", True, True)) + self.path.write_text(self.SOURCE.read_text(encoding="utf-8") + "# drift\n", + encoding="utf-8") + drifted = completion.launcher_state(self.home, self.SOURCE) + self.assertTrue(drifted["carriesMarker"]) + self.assertFalse(drifted["matchesCheckout"]) + + +class StableLauncherRemovalTest(unittest.TestCase): + """Removal takes only the file it put there, and says what the host held afterwards.""" + + SOURCE = ROOT / "plugins/crw/wiring/crw_stop_hook.py" + + def setUp(self): + self.stack = contextlib.ExitStack() + self.addCleanup(self.stack.close) + self.home = Path(self.stack.enter_context(tempfile.TemporaryDirectory(prefix="crw178-"))) + self.path = completion.launcher_path(self.home) + sys.path.insert(0, str(ROOT / "scripts")) + from crw_transition import steps + self.steps = steps + self.host = {"codexHome": str(self.home)} + + def remove(self, **kwargs): + return self.steps.launcher_remove(self.host, {}, **kwargs) + + def test_nothing_there_is_already_done(self): + self.assertEqual(self.remove(apply=True)["outcome"], self.steps.ALREADY) + + def test_a_dry_run_removes_nothing(self): + completion.place_launcher(self.path, self.SOURCE, apply=True) + self.assertEqual(self.remove()["outcome"], self.steps.WOULD) + self.assertTrue(self.path.is_file()) + + def test_the_file_it_installed_is_the_file_it_removes(self): + completion.place_launcher(self.path, self.SOURCE, apply=True) + answer = self.remove(apply=True) + self.assertEqual(answer["outcome"], self.steps.SETTLED) + self.assertFalse(self.path.exists()) + + def test_a_file_without_the_marker_is_refused(self): + self.path.write_text("not ours\n", encoding="utf-8") + answer = self.remove(apply=True) + self.assertEqual(answer["outcome"], self.steps.REFUSED) + self.assertEqual(self.path.read_text(encoding="utf-8"), "not ours\n") + + def test_a_symlink_is_refused_and_its_target_is_untouched(self): + target = self.home / "elsewhere.py" + target.write_text("someone else\n", encoding="utf-8") + self.path.symlink_to(target) + answer = self.remove(apply=True) + self.assertEqual(answer["outcome"], self.steps.REFUSED) + self.assertIn("symlink", answer["detail"]) + self.assertTrue(target.is_file()) + + def test_settings_written_back_around_the_run_are_reported_not_hidden(self): + """Two files, two locks. The race is not prevented here; it is made impossible to miss.""" + completion.place_launcher(self.path, self.SOURCE, apply=True) + (self.home / completion.CONFIG_NAME).write_text("{}", encoding="utf-8") + answer = self.remove(apply=True) + self.assertEqual(answer["outcome"], self.steps.SETTLED) + self.assertTrue(answer["settingsPresent"]) + self.assertIn("are NOT stopped", answer["detail"]) diff --git a/scripts/crw_runtime/completion.py b/scripts/crw_runtime/completion.py index 2d0e8a37f..b08a7c816 100644 --- a/scripts/crw_runtime/completion.py +++ b/scripts/crw_runtime/completion.py @@ -27,12 +27,14 @@ """ import errno +import hashlib import json import os import re import shlex import shutil import signal +import stat import subprocess import time import uuid @@ -320,6 +322,35 @@ def registration_complaints(event): # list kept equal by hand. CONFIG_SETTLED = (CONFIG_CREATED, CONFIG_UNCHANGED, CONFIG_WOULD_CREATE) +# The copy of the packaged launcher this command installs beside these settings, and why it +# exists at all. A plugin-declared hook command is fixed when a turn starts, with the version +# cache path already resolved into it. Replacing the package removes that directory whole, so a +# turn still running holds an absolute path to a file that is gone -- and python3 exits 2 for a +# missing script, which is the number the hook protocol reads as "block this turn". One removed +# directory therefore became a termination loop rather than one silent miss. +# +# The declaration opens the cache copy first and this one only when the cache copy cannot be +# opened. That ordering matters: the cache copy is always the current version, so this copy can +# never outrank it, and the only moment it is reached is the moment the cache cannot answer. +LAUNCHER_NAME = "crw-stop-hook.py" +LAUNCHER_SOURCE = "plugins/crw/wiring/crw_stop_hook.py" +# Mirrored from that file, which cannot import this module. It marks the file as CRW's to +# replace and establishes nothing else: not who wrote it, and not that its bytes are whole. The +# digest reported beside it answers the second question; nothing answers the first. +LAUNCHER_MARKER = "crw-stop-hook/1" + +LAUNCHER_PLACED = "launcher_placed" +LAUNCHER_UNCHANGED = "launcher_unchanged" +LAUNCHER_WOULD_PLACE = "launcher_would_place" +LAUNCHER_FOREIGN = "launcher_foreign" +LAUNCHER_NOT_A_FILE = "launcher_not_a_file" +LAUNCHER_UNREADABLE = "launcher_unreadable" +LAUNCHER_SOURCE_MISSING = "launcher_source_missing" +LAUNCHER_APPLIED_UNVERIFIED = "launcher_applied_unverified" +LAUNCHER_CHANGED_UNDERNEATH = "launcher_changed_underneath" +# The outcomes that leave a usable fallback, so a caller gates on one name rather than a list. +LAUNCHER_SETTLED = (LAUNCHER_PLACED, LAUNCHER_UNCHANGED, LAUNCHER_WOULD_PLACE) + # What status() answers with when it did not ask. Distinct from an absence, which is an answer. NOT_READ = "not_read" @@ -1314,6 +1345,160 @@ def _cell(value, evidence, **extra): return answer +def launcher_path(codex_home): + """Beside the settings, for the same reason the settings live where they do. + + The packaged bootstrap derives this path from CODEX_HOME and nothing else, so both sides + compute it identically instead of agreeing about a value. + """ + return Path(codex_home) / LAUNCHER_NAME + + +def launcher_kind(path): + """What is actually at that path, without following anything. + + lstat rather than exists(), because the symlink is the case that matters: replacing through + one writes to wherever it points, which is a file this command was never given. + """ + try: + mode = os.lstat(str(path)).st_mode + except FileNotFoundError: + return "absent" + except OSError as error: + return "unreadable (" + type(error).__name__ + ")" + if stat.S_ISLNK(mode): + return "symlink" + if stat.S_ISDIR(mode): + return "directory" + if stat.S_ISREG(mode): + return "file" + return "other" + + +def place_launcher(path, source, *, apply=False): + """Install the fallback launcher, and never over something that is not ours. + + Ours means the file carries the marker. That is a claim of ownership, not of provenance: it + says CRW put a launcher here, not who ran the command and not that the bytes are intact. So + the answer carries both digests, and a caller comparing them learns what the marker cannot + tell it. + + The decision is taken twice and only the second one is acted on, the way write_configuration + does it: the first reading answers the caller, the second happens under the lock, and a file + that moved in between is reported rather than written over. + + The lock covers this path only. There is deliberately no lock spanning this file and the + settings. Widening one means reworking a write path that is already proven, and it is not + needed: every state the two files can be left in is harmless. A launcher with no settings + stands down, and settings with no launcher are what the host had before this file existed. + What an interleaving can still do is make a receipt wrong, and the caller closes that by + reading both paths back at the end rather than by holding a wider lock. + """ + path, source = Path(path), Path(source) + answer = {"launcher": str(path), "source": str(source), "outcome": None, "applied": False, + "wrote": False, "kind": None, "digest": None, "sourceDigest": None} + try: + wanted = source.read_bytes() + except OSError as error: + answer["outcome"] = LAUNCHER_SOURCE_MISSING + answer["detail"] = ("the packaged launcher could not be read at " + str(source) + " (" + + type(error).__name__ + ": " + str(error) + "), so nothing was" + " placed") + return answer + answer["sourceDigest"] = hashlib.sha256(wanted).hexdigest() + + def judge(): + kind = launcher_kind(path) + if kind == "absent": + return kind, None, None + if kind != "file": + return kind, None, LAUNCHER_NOT_A_FILE + try: + return kind, path.read_bytes(), None + except OSError: + return kind, None, LAUNCHER_UNREADABLE + + kind, found, refusal = judge() + answer["kind"] = kind + if refusal == LAUNCHER_NOT_A_FILE: + answer["outcome"] = refusal + answer["detail"] = ("the launcher path holds a " + kind + "; this command does not" + " follow it and does not replace it") + return answer + if refusal == LAUNCHER_UNREADABLE: + answer["outcome"] = refusal + answer["detail"] = "a file is there and its bytes could not be read, so it is left alone" + return answer + if found is not None: + answer["digest"] = hashlib.sha256(found).hexdigest() + if found == wanted: + answer["outcome"] = LAUNCHER_UNCHANGED + answer["detail"] = "the launcher this checkout ships is already installed" + return answer + if LAUNCHER_MARKER.encode("utf-8") not in found: + answer["outcome"] = LAUNCHER_FOREIGN + answer["detail"] = ("a file is already there and it does not carry " + + LAUNCHER_MARKER + ", so it is not this command's to replace") + return answer + if not apply: + answer["outcome"] = LAUNCHER_WOULD_PLACE + answer["detail"] = "would install the launcher; nothing was written" + return answer + with hostrecord.Locked(path): + again_kind, again, again_refusal = judge() + if again_kind != kind or again != found or again_refusal is not None: + answer["kind"] = again_kind + answer["outcome"] = LAUNCHER_CHANGED_UNDERNEATH + answer["detail"] = ("the launcher path changed after it was read, so nothing was" + " written; rerun to decide against the file as it now stands") + return answer + hostrecord.atomic_write(path, wanted.decode("utf-8")) + try: + back = path.read_bytes() + except OSError: + back = None + answer["applied"] = True + answer["wrote"] = True + answer["kind"] = launcher_kind(path) + answer["digest"] = hashlib.sha256(back).hexdigest() if back is not None else None + if back != wanted: + answer["outcome"] = LAUNCHER_APPLIED_UNVERIFIED + answer["detail"] = ("the launcher was written and could not be read back as written, so" + " the fallback cannot be claimed to be installed") + return answer + answer["outcome"] = LAUNCHER_PLACED + answer["detail"] = "installed the launcher this checkout ships" + return answer + + +def launcher_state(codex_home, source): + """What is at the fallback path now, for a command that writes nothing. + + Reported beside the settings rather than merged into them. An installed fallback says a turn + whose cache was replaced has something to run; it says nothing about whether the hook is + registered, trusted, or has ever fired. + """ + path = launcher_path(codex_home) + state = {"launcher": str(path), "kind": launcher_kind(path), "digest": None, + "sourceDigest": None, "matchesCheckout": None, "carriesMarker": None} + try: + state["sourceDigest"] = hashlib.sha256(Path(source).read_bytes()).hexdigest() + except OSError: + state["sourceDigest"] = None + if state["kind"] != "file": + return state + try: + found = path.read_bytes() + except OSError: + state["kind"] = "unreadable" + return state + state["digest"] = hashlib.sha256(found).hexdigest() + state["carriesMarker"] = LAUNCHER_MARKER.encode("utf-8") in found + state["matchesCheckout"] = (state["sourceDigest"] is not None + and state["digest"] == state["sourceDigest"]) + return state + + def resource_key(path): """One key for one resource, for the spellings that are provably one resource. diff --git a/scripts/crw_transition/steps.py b/scripts/crw_transition/steps.py index e49d0c4dd..81abdba18 100644 --- a/scripts/crw_transition/steps.py +++ b/scripts/crw_transition/steps.py @@ -30,6 +30,11 @@ BUSY = "busy" NOT_REACHED = "not_reached" +# The launchers this package ships, named rather than globbed. Adding a launcher means adding it +# here; adding an ordinary helper under wiring/ must not appear here, because everything in this +# tuple is compared byte for byte and a mismatch refuses a transition. +LAUNCHERS = ("wiring/crw_stop_hook.py", "wiring/crw_bridge_mcp.py") + # Steps that changed something are reported apart from steps that found nothing to do, because # "converged" and "did the work" are different answers and a rerun has to be able to say which. DONE = (SETTLED, ALREADY) @@ -662,12 +667,20 @@ def _declared(root, repo_root): # the expansion that makes ${PLUGIN_ROOT} a path: python3 '${PLUGIN_ROOT}/...' # tokenises exactly like the declaration this package ships and starts # nothing, because the literal directory does not exist. + # + # The matcher and the timeout are appended to BOTH shapes. They used to ride + # only on the readable one, which was harmless while every declared command + # resolved to a file. It stopped being harmless when the Stop hook became a + # python3 -c bootstrap: an unreadable command carried neither field, so a + # cache that changed only the matcher or only the timeout compared equal and + # was accepted as the replacement. Those are exactly the two replacements + # that do not replace. events.setdefault(event, []).append( (shape + " as written " + repr(written.strip()) - + " matcher=" + repr((group or {}).get("matcher")) - + " timeout=" + repr((hook or {}).get("timeout"))) if shape else - ("a command this cannot read: " - + repr(written))) + if shape else + "a command this cannot read, as written " + repr(written)) + + " matcher=" + repr((group or {}).get("matcher")) + + " timeout=" + repr((hook or {}).get("timeout"))) named = manifest.get("mcpServers") if isinstance(named, str) and named.strip(): path = Path(root) / _relative(named) @@ -767,15 +780,38 @@ def launcher_complaints(repo_root, cache_version): of the plugin root -- the same test same_adapter makes of a registered adapter. A launcher the cache carries and this checkout does not is reported rather than skipped, since it is a program a declaration names and nothing here can vouch for. + + The declared set alone is no longer enough. The Stop hook is declared as a python3 -c + bootstrap, and _resolve deliberately answers None for -c because what follows it is source + text rather than a file. That is the right answer for the parser and the wrong amount of + coverage here, so the comparison takes the union of the scripts the declarations name and + LAUNCHERS, the launchers this package actually ships. LAUNCHERS is written out rather than + globbed: a glob would take a future non-executable helper under wiring/ for a launcher and + refuse a transition over a file nothing runs. """ ours = Path(repo_root) / "plugins" / "crw" events, servers, unread = _declared(cache_version, repo_root) found, seen = [], set() + census = [] for value in [item for values in events.values() for item in values] + list(servers.values()): + # The unreadable-command marker is recognised on the VALUE, not on what is left after a + # word is taken off the front of it. "a command this cannot read: ..." loses its leading + # "a " to the split below, so a test for that prefix on the remainder never matched and + # the marker sentence itself was carried forward as though it were a script path. That + # went unnoticed while every declared command resolved; declaring the Stop hook as a + # python3 -c bootstrap makes it the ordinary case. + if value.startswith("a command this cannot read"): + continue script = value.split(" as written ")[0].split(" ", 1)[-1] if " " in value else None - if not script or script in seen or script.startswith("a command this cannot read"): + if not script or script in seen: continue seen.add(script) + census.append(script) + for script in LAUNCHERS: + if script not in seen: + seen.add(script) + census.append(script) + for script in census: cached, mine = Path(cache_version) / script, ours / script try: same = mine.is_file() and cached.is_file() \ @@ -2388,10 +2424,80 @@ def remove(host, options, *, apply=False): " are: removing them now would take the skills from an install" " this command did not disable")) return results + results.append(launcher_remove(host, options, apply=apply)) results.append(skill_unlink(host, options, apply=apply)) return results +def launcher_remove(host, options, *, apply=False): + """Delete the fallback Stop launcher this repository installed, and only that file. + + Ours means it carries completion.LAUNCHER_MARKER. That marker is a claim of ownership and + not of provenance: it says CRW put a launcher at this path, not who ran the command. A file + without it is somebody else's and is left where it is; a symlink or a directory is reported + by kind and never followed, because removing through a link deletes a file nobody named. + + This runs after the settings are retired, so the only window it can leave is a launcher with + no settings, and that combination stands down in silence. The reverse order would leave + settings whose fallback is gone, which looks installed and is not. + + The marker is proved twice: once to answer the caller, once inside the lock immediately + before the unlink, so a file replaced during the run is not deleted as though it were still + ours. The lock is on this path alone. What the run cannot prevent is the settings being + written again around it, so the answer reports what the settings path held afterwards rather + than claiming new invocations are stopped on the strength of its own earlier step. + """ + step = "stable launcher" + home = Path(host["codexHome"]) + path = completion.launcher_path(home) + settings = home / completion.CONFIG_NAME + + def observed(): + return {"settingsPath": str(settings), "settingsPresent": settings.exists()} + + kind = completion.launcher_kind(path) + if kind == "absent": + return _answer(step, ALREADY, "there is nothing at " + str(path), **observed()) + if kind != "file": + return _answer(step, REFUSED, "the launcher path holds a " + kind + ", so it is not" + " this command's to remove and it is not followed", **observed()) + try: + found = path.read_bytes() + except OSError as error: + return _answer(step, REFUSED, "the launcher at " + str(path) + " could not be read (" + + type(error).__name__ + ": " + str(error) + "), so it is left alone", + **observed()) + if completion.LAUNCHER_MARKER.encode("utf-8") not in found: + return _answer(step, REFUSED, "the file at " + str(path) + " does not carry " + + completion.LAUNCHER_MARKER + ", so it is not this command's to remove", + **observed()) + if not apply: + return _answer(step, WOULD, "would remove " + str(path), **observed()) + try: + with hostrecord.Locked(path): + if completion.launcher_kind(path) != "file": + return _answer(step, REFUSED, "the launcher path changed after it was read, so" + " nothing was removed", **observed()) + again = path.read_bytes() + if completion.LAUNCHER_MARKER.encode("utf-8") not in again: + return _answer(step, REFUSED, "the file at " + str(path) + " was replaced while" + " this ran and no longer carries the marker, so it was left", + **observed()) + path.unlink() + except hostrecord.Busy as error: + return _answer(step, BUSY, str(error), **observed()) + except OSError as error: + return _answer(step, REFUSED, "the launcher could not be removed (" + + type(error).__name__ + ": " + str(error) + ")", **observed()) + answer = _answer(step, SETTLED, "removed " + str(path), applied=True, wrote=True, + **observed()) + if answer["settingsPresent"]: + answer["detail"] += ("; the settings are present again at " + str(settings) + + ", so a supported installer wrote them back around this run and" + " new adapter invocations are NOT stopped") + return answer + + def swap_state(host, options): """What a version replacement left, with the pointer and the record reported apart.""" point = host["pointer"] diff --git a/scripts/runtime_install.py b/scripts/runtime_install.py index d260dfb69..bd1d6c550 100755 --- a/scripts/runtime_install.py +++ b/scripts/runtime_install.py @@ -2368,6 +2368,29 @@ def cmd_hook(args): # Preconditions are settled. Now the writes, settings before the hook that reads them: # a hook registered against settings that are not there releases on every Stop and says # so nowhere, while settings with no hook cost nothing at all. + # + # And before both, for the plugin owner, the fallback launcher. The order is chosen from + # what each half does alone: a launcher with no settings stands down in silence, while + # settings whose fallback was never installed look installed and are not. So the half + # that is harmless on its own goes first, and a refusal here leaves the host untouched. + # The user owner gets none of this: its registration runs this checkout's own script by + # absolute path, with no version cache underneath it to be replaced. + placement = None + if owner == completion.OWNER_PLUGIN: + launcher = completion.launcher_path(codex_home) + try: + placement = completion.place_launcher( + launcher, ROOT / completion.LAUNCHER_SOURCE, apply=args.apply) + except hostrecord.Busy as error: + return _hook_busy(adapter, path, error, settings=None, locked=launcher) + if placement["outcome"] not in completion.LAUNCHER_SETTLED: + emit({"command": "hook", "adapter": adapter, "owner": owner, "event": event, + "settings": None, "launcher": placement, "hookFile": str(path), + "result": None, "error": placement.get("detail"), + "note": ("Nothing was written. The fallback launcher is installed before" + " the settings, so a refusal here leaves this host as it was" + " rather than with settings whose fallback is missing.")}) + return EXIT_REFUSED try: settings = completion.write_configuration( configuration, wanted, apply=args.apply) @@ -2375,10 +2398,13 @@ def cmd_hook(args): return _hook_busy(adapter, path, error, settings=None, locked=configuration) if settings["outcome"] not in completion.CONFIG_SETTLED: emit({"command": "hook", "adapter": adapter, "settings": settings, - "hookFile": str(path), "result": None, + "hookFile": str(path), "result": None, "launcher": placement, "note": ("The settings were not written, so no hook was appended. A hook" " registered against settings it cannot act on is installed and" - " inert, which is the one outcome worth refusing outright.")}) + " inert, which is the one outcome worth refusing outright. A" + " launcher already placed is left where it is: alone it stands" + " down, so removing it would buy nothing and could take a fallback" + " from a registration this command did not install.")}) return EXIT_REFUSED if owner == completion.OWNER_PLUGIN: # The registration is the plugin package's to declare, so this command writes the @@ -2388,15 +2414,32 @@ def cmd_hook(args): # Reported as what it is: settings written, nothing registered. A caller reading # only the exit status would otherwise record an installed hook, and on this host # there is none until the plugin is installed. + # + # Both paths are read back here, at the end, because these two files are written + # under separate locks. A concurrent removal can retire the settings and delete the + # launcher around this run, and the receipt would otherwise say a fallback was + # installed when the host no longer has one. This does not prevent that race; it + # stops it from ending quietly. + observed = completion.launcher_state(codex_home, + ROOT / completion.LAUNCHER_SOURCE) + lost = args.apply and placement is not None \ + and placement["outcome"] in (completion.LAUNCHER_PLACED, + completion.LAUNCHER_UNCHANGED) \ + and observed["kind"] != "file" emit({"command": "hook", "adapter": adapter, "owner": owner, "event": event, "settings": settings, "hookFile": str(path), "result": None, - "registrations": [], + "registrations": [], "launcher": placement, "launcherObserved": observed, + "error": ("the fallback launcher is no longer at " + observed["launcher"] + + " (" + str(observed["kind"]) + "); something removed it while" + " this run was writing, so this host has no fallback despite the" + " settings being installed") if lost else None, "note": ("Settings written; no registration was made and the hook file was" " not touched. The " + completion.OWNER_PLUGIN + " owner registers" " this event through the plugin package's own manifest, so install" " that package to register it. Written, registered and observed to" - " have fired stay three separate claims.")}) - return EXIT_OK + " have fired stay three separate claims. launcherObserved is what" + " both paths held after the writes, not what this run intended.")}) + return EXIT_INCOMPLETE if lost else EXIT_OK else: command = args.hook_command event = args.event or SESSION_START @@ -2491,7 +2534,13 @@ def cmd_hook_status(args): are separate cells here for exactly that reason. """ codex_home = Path(args.codex_home or os.environ.get("CODEX_HOME") or Path.home() / ".codex") - emit(completion.status(codex_home=codex_home, event=args.event or completion.EVENT)) + answer = completion.status(codex_home=codex_home, event=args.event or completion.EVENT) + # Beside the rest, never merged into it. An installed fallback says a turn whose version + # cache was replaced still has something to run; it says nothing about registration, trust + # or firing, and matchesCheckout is how a copy left behind by an older install becomes + # visible instead of staying quietly stale. + answer["launcher"] = completion.launcher_state(codex_home, ROOT / completion.LAUNCHER_SOURCE) + emit(answer) return EXIT_OK From 61955976cdbb839962a3a648a4995e0c6b1b2704 Mon Sep 17 00:00:00 2001 From: thisisjun786 <259586770+thisisjun786@users.noreply.github.com> Date: Mon, 21 Sep 2026 19:26:02 +0900 Subject: [PATCH 02/15] Report the three failures Devin found instead of absorbing them 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. --- docs/plugin-packaging.md | 6 +++ .../hooks/stop-recording-completion.json | 2 +- scripts/ci/tests/test_plugin_wiring.py | 47 ++++++++++++++++++- scripts/crw_transition/steps.py | 27 ++++++++++- scripts/plugin_transition.py | 4 +- scripts/runtime_install.py | 24 ++++++---- 6 files changed, 97 insertions(+), 13 deletions(-) diff --git a/docs/plugin-packaging.md b/docs/plugin-packaging.md index dfbc3c69c..9022050d3 100644 --- a/docs/plugin-packaging.md +++ b/docs/plugin-packaging.md @@ -235,6 +235,12 @@ exit 1, which the host reads as an ordinary failure rather than as a hold. Once been read it owns that Stop: the second one is not tried, because a launcher that raised after doing half its work has already acted on the turn. +A launcher that exits non-zero deliberately is reported the same way. The launcher's own +contract is to exit 0 on every path, so a copy that breaks it is saying something, and turning +that into a success would hide the one failure it went out of its way to report. It surfaces as +exit 1 rather than as the code the launcher chose, because 2 is the blocking code and no path +through this declaration may produce it. + Changing the command text changes the hook’s `trusted_hash`, so an update that changes it needs one re-trust per installed hook identity. Trust is keyed to the declaration content and not to the version path, so an update that leaves the command alone keeps its trust. diff --git a/plugins/crw/wiring/hooks/stop-recording-completion.json b/plugins/crw/wiring/hooks/stop-recording-completion.json index ed61fd659..a27594cc5 100644 --- a/plugins/crw/wiring/hooks/stop-recording-completion.json +++ b/plugins/crw/wiring/hooks/stop-recording-completion.json @@ -5,7 +5,7 @@ "hooks": [ { "type": "command", - "command": "python3 -c \"\nimport os, sys\nnamed = os.environ.get('CODEX_HOME')\nhome = named if named else os.path.join(os.path.expanduser('~'), '.codex')\nfor candidate in (sys.argv[1] if len(sys.argv) > 1 else '',\n os.path.join(home, 'crw-stop-hook.py')):\n if not candidate:\n continue\n try:\n with open(candidate, 'rb') as handle:\n source = handle.read()\n except OSError:\n continue\n try:\n exec(compile(source, candidate, 'exec'),\n {'__name__': '__main__', '__file__': candidate})\n except SystemExit:\n pass\n break\nraise SystemExit(0)\n\" \"${PLUGIN_ROOT}/wiring/crw_stop_hook.py\"", + "command": "python3 -c \"\nimport os, sys\nnamed = os.environ.get('CODEX_HOME')\nhome = named if named else os.path.join(os.path.expanduser('~'), '.codex')\nfor candidate in (sys.argv[1] if len(sys.argv) > 1 else '',\n os.path.join(home, 'crw-stop-hook.py')):\n if not candidate:\n continue\n try:\n with open(candidate, 'rb') as handle:\n source = handle.read()\n except OSError:\n continue\n try:\n exec(compile(source, candidate, 'exec'),\n {'__name__': '__main__', '__file__': candidate})\n except SystemExit as ending:\n if ending.code:\n raise SystemExit(1)\n break\nraise SystemExit(0)\n\" \"${PLUGIN_ROOT}/wiring/crw_stop_hook.py\"", "timeout": 10, "statusMessage": "(crw) Checking the completion records" } diff --git a/scripts/ci/tests/test_plugin_wiring.py b/scripts/ci/tests/test_plugin_wiring.py index 4b65267c7..cd3b69003 100644 --- a/scripts/ci/tests/test_plugin_wiring.py +++ b/scripts/ci/tests/test_plugin_wiring.py @@ -1282,6 +1282,35 @@ def test_the_real_launcher_still_answers_through_the_declaration(self): self.assertEqual((done.returncode, done.stdout), (0, b'{"decision": "block"}')) self.assertIn("turn_id", seen.read_text(encoding="utf-8")) + def test_a_launcher_that_exits_nonzero_is_reported_not_swallowed(self): + """Devin review: an explicit failure must not become a success. + + The launcher's own contract is to exit 0 on every path. A copy that breaks it is saying + something, and the bootstrap converting that into 0 would hide exactly the failure the + launcher went out of its way to report. It is reported as 1 rather than as the code the + launcher chose, because 2 is the host's blocking code and no path here may produce it. + """ + for code in (1, 3, 2): + with self.subTest(code=code): + cache = self.root / ("cache-%d" % code) / "0.4.0" + self.plant(cache / "wiring" / "crw_stop_hook.py", "PACKAGED", + "raise SystemExit(%d)\n" % code) + home = self.root / ("home-%d" % code) + home.mkdir(parents=True, exist_ok=True) + self.plant(home / "crw-stop-hook.py", "FALLBACK") + done, ran = self.fire(cache, home) + self.assertEqual(ran, ["PACKAGED"]) + self.assertEqual(done.returncode, 1) + + def test_a_launcher_that_exits_zero_explicitly_is_still_a_success(self): + cache = self.root / "cache-ok" / "0.4.0" + self.plant(cache / "wiring" / "crw_stop_hook.py", "PACKAGED", "raise SystemExit(0)\n") + home = self.root / "home-ok" + home.mkdir(parents=True, exist_ok=True) + done, ran = self.fire(cache, home) + self.assertEqual((done.returncode, ran), (0, ["PACKAGED"])) + + def test_the_declaration_stays_within_the_timeout_the_host_clamps(self): self.assertLessEqual(self.timeout, plugin.HOOK_TIMEOUT_SECONDS) @@ -1454,6 +1483,22 @@ def test_settings_written_back_around_the_run_are_reported_not_hidden(self): completion.place_launcher(self.path, self.SOURCE, apply=True) (self.home / completion.CONFIG_NAME).write_text("{}", encoding="utf-8") answer = self.remove(apply=True) - self.assertEqual(answer["outcome"], self.steps.SETTLED) + # Not settled: settled is read as "stopped", and a host whose settings came back can be + # called again through the packaged copy. The file was still removed, and the detail says so. + self.assertEqual(answer["outcome"], self.steps.LIVE_AGAIN) + self.assertFalse(self.path.exists()) self.assertTrue(answer["settingsPresent"]) self.assertIn("are NOT stopped", answer["detail"]) + + def test_a_surface_that_came_back_is_not_listed_as_stopped(self): + """Devin review: the aggregate claim has to agree with the host, not with an earlier step.""" + completion.place_launcher(self.path, self.SOURCE, apply=True) + (self.home / completion.CONFIG_NAME).write_text("{}", encoding="utf-8") + claims = self.steps.stop_claims([self.remove(apply=True)], self.steps.REMOVE_CLAIMS) + self.assertFalse(any("fallback" in claim for claim in claims["stopped"])) + self.assertTrue(any("fallback" in claim for claim in claims["stillLive"])) + + def test_disable_never_claims_the_fallback_it_does_not_touch(self): + """disable stops calls and deletes nothing, so the fallback is not its surface to report.""" + claims = self.steps.stop_claims([]) + self.assertFalse(any("fallback" in claim for claim in claims["stillLive"])) diff --git a/scripts/crw_transition/steps.py b/scripts/crw_transition/steps.py index 81abdba18..f4a1cfd5d 100644 --- a/scripts/crw_transition/steps.py +++ b/scripts/crw_transition/steps.py @@ -29,6 +29,10 @@ REFUSED = "refused" BUSY = "busy" NOT_REACHED = "not_reached" +# The surface was acted on and is live again anyway, because something outside this run put +# back what it had just retired. Deliberately not SETTLED: settled is read as "stopped", and a +# host whose settings reappeared is a host whose adapter can be called again. +LIVE_AGAIN = "live_again" # The launchers this package ships, named rather than globbed. Adding a launcher means adding it # here; adding an ordinary helper under wiring/ must not appear here, because everything in this @@ -2386,18 +2390,33 @@ def preserved_paths(host): ("bridge record", "new bridge starts, because the packaged launcher has no record to read"), ) +# What remove claims on top of those. Its own set, because disable deliberately deletes nothing: +# it stops calls by retiring the settings and leaves the fallback where it is, so listing that +# file among the surfaces disable did not stop would report a step that was never its to run. +# +# remove does run it, and it runs it AFTER the settings are retired. A supported installer +# writing the settings back in between leaves the earlier claim true of a host that no longer +# exists, and this is the step positioned to notice. +REMOVE_CLAIMS = STOP_CLAIMS + ( + ("stable launcher", "the fallback the declaration reaches when the version cache is gone"), +) + -def stop_claims(results): +def stop_claims(results, claims=STOP_CLAIMS): """The stop claims split by what this run actually did to each surface. settled and already_done are both true of the host now: one because this run moved the record, the other because there was none there to move. would_change is what an --apply would do and nothing more. Every other outcome leaves the surface live, and the reason travels with it rather than being left for a reader to infer from the step list. + + The claim set is a parameter because the two callers own different surfaces, and a claim + about a step its caller never runs reads as a surface left live rather than as one nobody + asked about. """ answers = {item["step"]: item for item in results} stopped, projected, live = [], [], [] - for step, claim in STOP_CLAIMS: + for step, claim in claims: item = answers.get(step) if item is None: live.append(claim + " -- NOT stopped: " + step + " did not run") @@ -2492,6 +2511,10 @@ def observed(): answer = _answer(step, SETTLED, "removed " + str(path), applied=True, wrote=True, **observed()) if answer["settingsPresent"]: + # The launcher is gone and the settings are back, so the packaged copy under the current + # version cache can answer a Stop again. Reporting settled here would put this surface in + # the stopped list and say the opposite of what the host now does. + answer["outcome"] = LIVE_AGAIN answer["detail"] += ("; the settings are present again at " + str(settings) + ", so a supported installer wrote them back around this run and" " new adapter invocations are NOT stopped") diff --git a/scripts/plugin_transition.py b/scripts/plugin_transition.py index c670eed44..4a693bc67 100755 --- a/scripts/plugin_transition.py +++ b/scripts/plugin_transition.py @@ -249,7 +249,9 @@ def cmd_disable(args): def cmd_remove(args): host = host_of(args) results = steps.remove(host, options_of(args), apply=bool(args.apply)) - claims = steps.stop_claims(results) + # remove's own claim set: it is the command that runs the stable-launcher step, and disable + # is not, so the fallback is a surface only this caller can answer for. + claims = steps.stop_claims(results, steps.REMOVE_CLAIMS) emit({"command": "remove", "applied": bool(args.apply), "results": results, "stops": claims["stopped"], "wouldStop": claims["wouldStop"], diff --git a/scripts/runtime_install.py b/scripts/runtime_install.py index bd1d6c550..a27f4b9c7 100755 --- a/scripts/runtime_install.py +++ b/scripts/runtime_install.py @@ -2422,17 +2422,25 @@ def cmd_hook(args): # stops it from ending quietly. observed = completion.launcher_state(codex_home, ROOT / completion.LAUNCHER_SOURCE) - lost = args.apply and placement is not None \ - and placement["outcome"] in (completion.LAUNCHER_PLACED, - completion.LAUNCHER_UNCHANGED) \ - and observed["kind"] != "file" + # Gone, or there and not what this run put there. A concurrent installer from another + # revision leaves a regular file at the same path, and a check that only asks whether + # SOMETHING is there reports this run's fallback installed while the bytes belong to + # a different checkout. + settled = placement["outcome"] in (completion.LAUNCHER_PLACED, + completion.LAUNCHER_UNCHANGED) \ + if placement is not None else False + lost = args.apply and settled \ + and (observed["kind"] != "file" + or observed["digest"] != placement["sourceDigest"]) emit({"command": "hook", "adapter": adapter, "owner": owner, "event": event, "settings": settings, "hookFile": str(path), "result": None, "registrations": [], "launcher": placement, "launcherObserved": observed, - "error": ("the fallback launcher is no longer at " + observed["launcher"] - + " (" + str(observed["kind"]) + "); something removed it while" - " this run was writing, so this host has no fallback despite the" - " settings being installed") if lost else None, + "error": ("the fallback launcher at " + observed["launcher"] + " is not the" + " one this run installed (kind " + str(observed["kind"]) + + ", digest " + str(observed["digest"]) + ", expected " + + str(placement["sourceDigest"]) + "); something replaced or removed" + " it while this run was writing, so the fallback this receipt would" + " have claimed is not what this host holds") if lost else None, "note": ("Settings written; no registration was made and the hook file was" " not touched. The " + completion.OWNER_PLUGIN + " owner registers" " this event through the plugin package's own manifest, so install" From d11f9ec33491cd470d4610178331e38352b0e1d4 Mon Sep 17 00:00:00 2001 From: thisisjun786 <259586770+thisisjun786@users.noreply.github.com> Date: Mon, 21 Sep 2026 19:42:29 +0900 Subject: [PATCH 03/15] Follow the interpreter's exit rule, and let a live surface fail the command 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. --- docs/plugin-packaging.md | 5 +++ .../hooks/stop-recording-completion.json | 2 +- scripts/ci/tests/test_plugin_wiring.py | 43 +++++++++++++++++++ scripts/plugin_transition.py | 12 ++++++ 4 files changed, 61 insertions(+), 1 deletion(-) diff --git a/docs/plugin-packaging.md b/docs/plugin-packaging.md index 9022050d3..5c86c1dbb 100644 --- a/docs/plugin-packaging.md +++ b/docs/plugin-packaging.md @@ -241,6 +241,11 @@ that into a success would hide the one failure it went out of its way to report. exit 1 rather than as the code the launcher chose, because 2 is the blocking code and no path through this declaration may produce it. +Which exits count as success follows the interpreter rather than the code's truthiness. +`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. An empty string and `0.0` are +therefore failures, and the declaration treats them as failures. + Changing the command text changes the hook’s `trusted_hash`, so an update that changes it needs one re-trust per installed hook identity. Trust is keyed to the declaration content and not to the version path, so an update that leaves the command alone keeps its trust. diff --git a/plugins/crw/wiring/hooks/stop-recording-completion.json b/plugins/crw/wiring/hooks/stop-recording-completion.json index a27594cc5..4c0eaf0b5 100644 --- a/plugins/crw/wiring/hooks/stop-recording-completion.json +++ b/plugins/crw/wiring/hooks/stop-recording-completion.json @@ -5,7 +5,7 @@ "hooks": [ { "type": "command", - "command": "python3 -c \"\nimport os, sys\nnamed = os.environ.get('CODEX_HOME')\nhome = named if named else os.path.join(os.path.expanduser('~'), '.codex')\nfor candidate in (sys.argv[1] if len(sys.argv) > 1 else '',\n os.path.join(home, 'crw-stop-hook.py')):\n if not candidate:\n continue\n try:\n with open(candidate, 'rb') as handle:\n source = handle.read()\n except OSError:\n continue\n try:\n exec(compile(source, candidate, 'exec'),\n {'__name__': '__main__', '__file__': candidate})\n except SystemExit as ending:\n if ending.code:\n raise SystemExit(1)\n break\nraise SystemExit(0)\n\" \"${PLUGIN_ROOT}/wiring/crw_stop_hook.py\"", + "command": "python3 -c \"\nimport os, sys\nnamed = os.environ.get('CODEX_HOME')\nhome = named if named else os.path.join(os.path.expanduser('~'), '.codex')\nfor candidate in (sys.argv[1] if len(sys.argv) > 1 else '',\n os.path.join(home, 'crw-stop-hook.py')):\n if not candidate:\n continue\n try:\n with open(candidate, 'rb') as handle:\n source = handle.read()\n except OSError:\n continue\n try:\n exec(compile(source, candidate, 'exec'),\n {'__name__': '__main__', '__file__': candidate})\n except SystemExit as ending:\n code = ending.code\n if not (code is None or (isinstance(code, int) and code == 0)):\n raise SystemExit(1)\n break\nraise SystemExit(0)\n\" \"${PLUGIN_ROOT}/wiring/crw_stop_hook.py\"", "timeout": 10, "statusMessage": "(crw) Checking the completion records" } diff --git a/scripts/ci/tests/test_plugin_wiring.py b/scripts/ci/tests/test_plugin_wiring.py index cd3b69003..06741d009 100644 --- a/scripts/ci/tests/test_plugin_wiring.py +++ b/scripts/ci/tests/test_plugin_wiring.py @@ -1311,6 +1311,28 @@ def test_a_launcher_that_exits_zero_explicitly_is_still_a_success(self): self.assertEqual((done.returncode, ran), (0, ["PACKAGED"])) + def test_system_exit_codes_follow_the_interpreter_not_their_truthiness(self): + """Devin review: SystemExit.code is not restricted to integers. + + CPython exits 0 only for None and an integer zero, and False is an integer zero. Every + other object, including a falsey one like '' or 0.0, exits 1. Testing truthiness would + call those two a success and hide a launcher that failed on purpose. + """ + successes = ("None", "0", "False") + failures = ("1", "3", "2", "''", "0.0", "'boom'", "[]") + for literal in successes + failures: + with self.subTest(code=literal): + cache = self.root / ("c-" + str(abs(hash(literal)))) / "0.4.0" + self.plant(cache / "wiring" / "crw_stop_hook.py", "PACKAGED", + "raise SystemExit(%s)\n" % literal) + home = self.root / ("h-" + str(abs(hash(literal)))) + home.mkdir(parents=True, exist_ok=True) + done, ran = self.fire(cache, home) + self.assertEqual(ran, ["PACKAGED"]) + self.assertEqual(done.returncode, 0 if literal in successes else 1, + "SystemExit(%s) was mapped to %d" % (literal, done.returncode)) + + def test_the_declaration_stays_within_the_timeout_the_host_clamps(self): self.assertLessEqual(self.timeout, plugin.HOOK_TIMEOUT_SECONDS) @@ -1498,6 +1520,27 @@ def test_a_surface_that_came_back_is_not_listed_as_stopped(self): self.assertFalse(any("fallback" in claim for claim in claims["stopped"])) self.assertTrue(any("fallback" in claim for claim in claims["stillLive"])) + def test_a_surface_that_came_back_makes_the_command_exit_nonzero(self): + """Devin review: shell automation reads the exit status, not the JSON. + + live_again means the file really was removed and the surface is callable again anyway. + A zero here would let a cleanup script carry on against a host where the adapter can + still be reached, so it gets its own status rather than being folded into a refusal. + """ + sys.path.insert(0, str(ROOT / "scripts")) + import plugin_transition + + completion.place_launcher(self.path, self.SOURCE, apply=True) + (self.home / completion.CONFIG_NAME).write_text("{}", encoding="utf-8") + answer = self.remove(apply=True) + self.assertEqual(answer["outcome"], self.steps.LIVE_AGAIN) + self.assertEqual(plugin_transition.verdict([answer]), plugin_transition.EXIT_INCOMPLETE) + self.assertNotEqual(plugin_transition.EXIT_INCOMPLETE, plugin_transition.EXIT_OK) + # settled alone still exits zero, so the new status is not a blanket nonzero. + settled = dict(answer, outcome=self.steps.SETTLED) + self.assertEqual(plugin_transition.verdict([settled]), plugin_transition.EXIT_OK) + + def test_disable_never_claims_the_fallback_it_does_not_touch(self): """disable stops calls and deletes nothing, so the fallback is not its surface to report.""" claims = self.steps.stop_claims([]) diff --git a/scripts/plugin_transition.py b/scripts/plugin_transition.py index 4a693bc67..b80250167 100755 --- a/scripts/plugin_transition.py +++ b/scripts/plugin_transition.py @@ -46,6 +46,11 @@ def completion_event(): # The same three the runtime installer uses, so a caller reading both does not have to learn two # meanings for one number. EXIT_OK, EXIT_REFUSED, EXIT_USAGE = 0, 1, 2 +# A fourth answer, for the same reason runtime_install.py declares one: "nothing happened" and +# "it happened and did not stay in effect" are two results and one status cannot carry both. +# A step that removed its file and then found the surface live again is not a refusal -- bytes +# were removed -- and it is not success either, because the operation did not remain done. +EXIT_INCOMPLETE = 3 def emit(document): @@ -166,10 +171,17 @@ def verdict(results): itself into the refusals: the surface IS stopped and something was left behind that another cooperating writer will trip over, and an operator who reads only the exit code has to learn that from it. + + And live_again is nonzero on its own status. It means this run did what it was asked and + something put the surface back around it, so an operator reading only the exit code would + otherwise run the next cleanup step against a host where the adapter is callable again. + Reported apart from a refusal because bytes really were removed. """ if any(item["outcome"] in (steps.REFUSED, steps.BUSY) for item in results) \ or any(item.get("lockCleanupFailed") for item in results): return EXIT_REFUSED + if any(item["outcome"] == steps.LIVE_AGAIN for item in results): + return EXIT_INCOMPLETE return EXIT_OK From 5151ef889a962e4c0edcdbb9363cf39f20f8d748 Mon Sep 17 00:00:00 2001 From: thisisjun786 <259586770+thisisjun786@users.noreply.github.com> Date: Mon, 21 Sep 2026 19:55:56 +0900 Subject: [PATCH 04/15] Read an exit code's value instead of asking it whether it equals zero 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. --- .../hooks/stop-recording-completion.json | 2 +- scripts/ci/tests/test_plugin_wiring.py | 28 +++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/plugins/crw/wiring/hooks/stop-recording-completion.json b/plugins/crw/wiring/hooks/stop-recording-completion.json index 4c0eaf0b5..d0deafb11 100644 --- a/plugins/crw/wiring/hooks/stop-recording-completion.json +++ b/plugins/crw/wiring/hooks/stop-recording-completion.json @@ -5,7 +5,7 @@ "hooks": [ { "type": "command", - "command": "python3 -c \"\nimport os, sys\nnamed = os.environ.get('CODEX_HOME')\nhome = named if named else os.path.join(os.path.expanduser('~'), '.codex')\nfor candidate in (sys.argv[1] if len(sys.argv) > 1 else '',\n os.path.join(home, 'crw-stop-hook.py')):\n if not candidate:\n continue\n try:\n with open(candidate, 'rb') as handle:\n source = handle.read()\n except OSError:\n continue\n try:\n exec(compile(source, candidate, 'exec'),\n {'__name__': '__main__', '__file__': candidate})\n except SystemExit as ending:\n code = ending.code\n if not (code is None or (isinstance(code, int) and code == 0)):\n raise SystemExit(1)\n break\nraise SystemExit(0)\n\" \"${PLUGIN_ROOT}/wiring/crw_stop_hook.py\"", + "command": "python3 -c \"\nimport os, sys\nnamed = os.environ.get('CODEX_HOME')\nhome = named if named else os.path.join(os.path.expanduser('~'), '.codex')\nfor candidate in (sys.argv[1] if len(sys.argv) > 1 else '',\n os.path.join(home, 'crw-stop-hook.py')):\n if not candidate:\n continue\n try:\n with open(candidate, 'rb') as handle:\n source = handle.read()\n except OSError:\n continue\n try:\n exec(compile(source, candidate, 'exec'),\n {'__name__': '__main__', '__file__': candidate})\n except SystemExit as ending:\n code = ending.code\n if not (code is None or (isinstance(code, int) and int(code) == 0)):\n raise SystemExit(1)\n break\nraise SystemExit(0)\n\" \"${PLUGIN_ROOT}/wiring/crw_stop_hook.py\"", "timeout": 10, "statusMessage": "(crw) Checking the completion records" } diff --git a/scripts/ci/tests/test_plugin_wiring.py b/scripts/ci/tests/test_plugin_wiring.py index 06741d009..d3bd2bf98 100644 --- a/scripts/ci/tests/test_plugin_wiring.py +++ b/scripts/ci/tests/test_plugin_wiring.py @@ -1333,6 +1333,34 @@ def test_system_exit_codes_follow_the_interpreter_not_their_truthiness(self): "SystemExit(%s) was mapped to %d" % (literal, done.returncode)) + def test_an_integer_subclass_is_judged_by_its_value_not_its_equality(self): + """Devin review: CPython derives the status from the stored value, not from __eq__. + + int is subclassable and __eq__ is overridable, so comparing the code with zero can + dispatch into a method that answers something unrelated to the number. int(code) reads + the value the interpreter would use, which is the rule this declaration claims to follow. + """ + lying = ("class E(int):\n" + " def __eq__(self, other):\n" + " return %s\n" + " def __hash__(self):\n" + " return 0\n" + "raise SystemExit(E(%d))\n") + for says, value, expected in ((True, 5, 1), (False, 0, 0)): + with self.subTest(value=value, says=says): + tag = "%s-%d" % (says, value) + cache = self.root / ("sub-" + tag) / "0.4.0" + self.plant(cache / "wiring" / "crw_stop_hook.py", "PACKAGED", + lying % (says, value)) + home = self.root / ("subhome-" + tag) + home.mkdir(parents=True, exist_ok=True) + done, ran = self.fire(cache, home) + self.assertEqual(ran, ["PACKAGED"]) + self.assertEqual(done.returncode, expected, + "E(%d) with __eq__ -> %s was mapped to %d" + % (value, says, done.returncode)) + + def test_the_declaration_stays_within_the_timeout_the_host_clamps(self): self.assertLessEqual(self.timeout, plugin.HOOK_TIMEOUT_SECONDS) From 60d9583e300b001c0f3108c51eab180987318c50 Mon Sep 17 00:00:00 2001 From: thisisjun786 <259586770+thisisjun786@users.noreply.github.com> Date: Mon, 21 Sep 2026 20:07:15 +0900 Subject: [PATCH 05/15] Read the base int, not whatever the subclass says its value is __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. --- .../hooks/stop-recording-completion.json | 2 +- scripts/ci/tests/test_plugin_wiring.py | 47 +++++++++++-------- 2 files changed, 29 insertions(+), 20 deletions(-) diff --git a/plugins/crw/wiring/hooks/stop-recording-completion.json b/plugins/crw/wiring/hooks/stop-recording-completion.json index d0deafb11..7ce485b6d 100644 --- a/plugins/crw/wiring/hooks/stop-recording-completion.json +++ b/plugins/crw/wiring/hooks/stop-recording-completion.json @@ -5,7 +5,7 @@ "hooks": [ { "type": "command", - "command": "python3 -c \"\nimport os, sys\nnamed = os.environ.get('CODEX_HOME')\nhome = named if named else os.path.join(os.path.expanduser('~'), '.codex')\nfor candidate in (sys.argv[1] if len(sys.argv) > 1 else '',\n os.path.join(home, 'crw-stop-hook.py')):\n if not candidate:\n continue\n try:\n with open(candidate, 'rb') as handle:\n source = handle.read()\n except OSError:\n continue\n try:\n exec(compile(source, candidate, 'exec'),\n {'__name__': '__main__', '__file__': candidate})\n except SystemExit as ending:\n code = ending.code\n if not (code is None or (isinstance(code, int) and int(code) == 0)):\n raise SystemExit(1)\n break\nraise SystemExit(0)\n\" \"${PLUGIN_ROOT}/wiring/crw_stop_hook.py\"", + "command": "python3 -c \"\nimport os, sys\nnamed = os.environ.get('CODEX_HOME')\nhome = named if named else os.path.join(os.path.expanduser('~'), '.codex')\nfor candidate in (sys.argv[1] if len(sys.argv) > 1 else '',\n os.path.join(home, 'crw-stop-hook.py')):\n if not candidate:\n continue\n try:\n with open(candidate, 'rb') as handle:\n source = handle.read()\n except OSError:\n continue\n try:\n exec(compile(source, candidate, 'exec'),\n {'__name__': '__main__', '__file__': candidate})\n except SystemExit as ending:\n code = ending.code\n if not (code is None or (isinstance(code, int)\n and int.__int__(code) == 0)):\n raise SystemExit(1)\n break\nraise SystemExit(0)\n\" \"${PLUGIN_ROOT}/wiring/crw_stop_hook.py\"", "timeout": 10, "statusMessage": "(crw) Checking the completion records" } diff --git a/scripts/ci/tests/test_plugin_wiring.py b/scripts/ci/tests/test_plugin_wiring.py index d3bd2bf98..77ea2a446 100644 --- a/scripts/ci/tests/test_plugin_wiring.py +++ b/scripts/ci/tests/test_plugin_wiring.py @@ -1340,25 +1340,34 @@ def test_an_integer_subclass_is_judged_by_its_value_not_its_equality(self): dispatch into a method that answers something unrelated to the number. int(code) reads the value the interpreter would use, which is the rule this declaration claims to follow. """ - lying = ("class E(int):\n" - " def __eq__(self, other):\n" - " return %s\n" - " def __hash__(self):\n" - " return 0\n" - "raise SystemExit(E(%d))\n") - for says, value, expected in ((True, 5, 1), (False, 0, 0)): - with self.subTest(value=value, says=says): - tag = "%s-%d" % (says, value) - cache = self.root / ("sub-" + tag) / "0.4.0" - self.plant(cache / "wiring" / "crw_stop_hook.py", "PACKAGED", - lying % (says, value)) - home = self.root / ("subhome-" + tag) - home.mkdir(parents=True, exist_ok=True) - done, ran = self.fire(cache, home) - self.assertEqual(ran, ["PACKAGED"]) - self.assertEqual(done.returncode, expected, - "E(%d) with __eq__ -> %s was mapped to %d" - % (value, says, done.returncode)) + shapes = { + "__eq__": "class E(int):\n" + " def __eq__(self, other):\n" + " return %s\n" + " def __hash__(self):\n" + " return 0\n" + "raise SystemExit(E(%d))\n", + "__int__": "class E(int):\n" + " def __int__(self):\n" + " return %s\n" + "raise SystemExit(E(%d))\n", + } + for name, template in sorted(shapes.items()): + lies = "True" if name == "__eq__" else "0" + truths = "False" if name == "__eq__" else "5" + for says, value, expected in ((lies, 5, 1), (truths, 0, 0)): + with self.subTest(override=name, value=value, says=says): + tag = "%s-%s-%d" % (name.strip("_"), says, value) + cache = self.root / ("sub-" + tag) / "0.4.0" + self.plant(cache / "wiring" / "crw_stop_hook.py", "PACKAGED", + template % (says, value)) + home = self.root / ("subhome-" + tag) + home.mkdir(parents=True, exist_ok=True) + done, ran = self.fire(cache, home) + self.assertEqual(ran, ["PACKAGED"]) + self.assertEqual(done.returncode, expected, + "E(%d) with %s -> %s was mapped to %d" + % (value, name, says, done.returncode)) def test_the_declaration_stays_within_the_timeout_the_host_clamps(self): From 35f83def1cf28538a80b1e62de858f0a04c6a2dd Mon Sep 17 00:00:00 2001 From: thisisjun786 <259586770+thisisjun786@users.noreply.github.com> Date: Mon, 21 Sep 2026 20:19:51 +0900 Subject: [PATCH 06/15] Give the migration the fallback it owes, and read both halves back 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. --- scripts/ci/tests/test_plugin_transition.py | 35 ++++++++++++++++++ scripts/crw_transition/steps.py | 28 ++++++++++++++ scripts/runtime_install.py | 43 ++++++++++++++++------ 3 files changed, 94 insertions(+), 12 deletions(-) diff --git a/scripts/ci/tests/test_plugin_transition.py b/scripts/ci/tests/test_plugin_transition.py index 831c0245b..e68ee2657 100644 --- a/scripts/ci/tests/test_plugin_transition.py +++ b/scripts/ci/tests/test_plugin_transition.py @@ -324,6 +324,41 @@ def test_the_operational_locations_are_carried_forward_rather_than_relocated(sel for field in ("markerRoot", "dbPath", "journalRoot", "mode"): self.assertEqual(after[field], before[field], field) + def test_a_migrated_host_gets_the_fallback_launcher_too(self): + """Devin review: this command writes the same settings, so it owes the same fallback. + + Without it a migrated host ends with plugin-owned settings and one candidate again, and + the first package replacement during an open turn lands back in the loop CRW-178 closes. + """ + import sys as _sys + _sys.path.insert(0, str(ROOT / "scripts")) + from crw_runtime import completion as _completion + + host = self.ready() + placed = Path(host.home) / _completion.LAUNCHER_NAME + self.assertFalse(placed.exists()) + code, answer = host.transition("--apply") + self.assertEqual(code, 0, json.dumps(answer["results"])[:700]) + self.assertTrue(placed.is_file(), json.dumps(answer["results"])[:700]) + self.assertEqual(placed.read_bytes(), + (ROOT / _completion.LAUNCHER_SOURCE).read_bytes()) + step = [item for item in answer["results"] if item["step"] == "stable launcher install"] + self.assertEqual(len(step), 1, json.dumps(answer["results"])[:700]) + self.assertIn(step[0]["outcome"], ("settled", "already_done")) + + def test_a_dry_run_migration_places_no_launcher(self): + import sys as _sys + _sys.path.insert(0, str(ROOT / "scripts")) + from crw_runtime import completion as _completion + + host = self.ready() + code, answer = host.transition() + self.assertEqual(code, 0, json.dumps(answer["results"])[:700]) + self.assertFalse((Path(host.home) / _completion.LAUNCHER_NAME).exists()) + step = [item for item in answer["results"] if item["step"] == "stable launcher install"] + self.assertEqual([item["outcome"] for item in step], ["would_change"]) + + def test_the_retired_settings_and_record_are_kept_not_deleted(self): host = self.ready() host.transition("--apply") diff --git a/scripts/crw_transition/steps.py b/scripts/crw_transition/steps.py index f4a1cfd5d..cbcc35224 100644 --- a/scripts/crw_transition/steps.py +++ b/scripts/crw_transition/steps.py @@ -1479,6 +1479,33 @@ def settings_install(host, options, *, apply=False, previous=None): applied=written.get("applied")) +def launcher_install(host, options, *, apply=False): + """Place the fallback Stop launcher, as part of the migration and not only beside it. + + This command writes the same plugin-owned settings runtime_install.py hook writes, through + settings_install, so a host migrated here would otherwise end with the settings and no + fallback: the declaration would have one candidate again, and the first package replacement + during an open turn would land back in the loop this whole change exists to close. + + Before settings install, for the reason that ordering exists everywhere else here: a launcher + with no settings stands down in silence, while settings whose fallback was never placed look + installed and are not. A refusal here therefore stops the sequence with the host unchanged. + """ + step = "stable launcher install" + path = completion.launcher_path(Path(host["codexHome"])) + source = Path(host["repoRoot"]) / completion.LAUNCHER_SOURCE + try: + placed = completion.place_launcher(path, source, apply=apply) + except hostrecord.Busy as error: + return _answer(step, BUSY, str(error)) + outcome = placed["outcome"] + settled = SETTLED if outcome == completion.LAUNCHER_PLACED else ( + ALREADY if outcome == completion.LAUNCHER_UNCHANGED else ( + WOULD if outcome == completion.LAUNCHER_WOULD_PLACE else REFUSED)) + return _answer(step, settled, placed.get("detail"), launcher=placed, + applied=placed.get("applied"), wrote=placed.get("wrote")) + + def mcp_record_retire(host, options, *, apply=False): """Retire the user-owned record, because owner is part of what makes a record the same one.""" record = host["mcp"].get("record") @@ -1885,6 +1912,7 @@ def skill_unlink(host, options, *, apply=False): # against absent settings -- it releases in silence and records nothing -- and no window in which # two adapters run, because the plugin-owned settings are still not installed. ORDER = (("settings retire", settings_retire), ("hook standdown", hook_standdown), + ("stable launcher install", launcher_install), ("settings install", settings_install), ("mcp record retire", mcp_record_retire), ("mcp table standdown", mcp_table_standdown), ("mcp record install", mcp_record_install), ("skill unlink", skill_unlink)) diff --git a/scripts/runtime_install.py b/scripts/runtime_install.py index a27f4b9c7..a1dd63361 100755 --- a/scripts/runtime_install.py +++ b/scripts/runtime_install.py @@ -2422,31 +2422,50 @@ def cmd_hook(args): # stops it from ending quietly. observed = completion.launcher_state(codex_home, ROOT / completion.LAUNCHER_SOURCE) - # Gone, or there and not what this run put there. A concurrent installer from another - # revision leaves a regular file at the same path, and a check that only asks whether - # SOMETHING is there reports this run's fallback installed while the bytes belong to - # a different checkout. + # BOTH halves, read back at the end. The launcher and the settings are written under + # separate locks, so either can be taken away between its own write and this point, + # and a receipt that checked only one would report an installation the host does not + # have. For the launcher: gone, or there and not what this run put there, because a + # concurrent installer from another revision leaves a regular file at the same path. + # For the settings: retired by a concurrent removal, which leaves a declared hook + # that finds nothing to act on and stands down on every Stop in silence. settled = placement["outcome"] in (completion.LAUNCHER_PLACED, completion.LAUNCHER_UNCHANGED) \ if placement is not None else False - lost = args.apply and settled \ + launcher_lost = args.apply and settled \ and (observed["kind"] != "file" or observed["digest"] != placement["sourceDigest"]) + back = reading.read_json(configuration, "the completion hook configuration") + settings_now = {"configuration": str(configuration), "state": back.state, + "matchesWanted": bool(back.usable and back.value == wanted)} + settings_lost = args.apply and not settings_now["matchesWanted"] + reasons = [] + if launcher_lost: + reasons.append("the fallback launcher at " + observed["launcher"] + " is not the" + " one this run installed (kind " + str(observed["kind"]) + + ", digest " + str(observed["digest"]) + ", expected " + + str(placement["sourceDigest"]) + ")") + if settings_lost: + reasons.append("the settings at " + str(configuration) + " are no longer the ones" + " this run wrote (reading " + str(back.state) + "), so the" + " declared hook has nothing to act on and stands down on every" + " Stop") + lost = bool(reasons) emit({"command": "hook", "adapter": adapter, "owner": owner, "event": event, "settings": settings, "hookFile": str(path), "result": None, "registrations": [], "launcher": placement, "launcherObserved": observed, - "error": ("the fallback launcher at " + observed["launcher"] + " is not the" - " one this run installed (kind " + str(observed["kind"]) - + ", digest " + str(observed["digest"]) + ", expected " - + str(placement["sourceDigest"]) + "); something replaced or removed" - " it while this run was writing, so the fallback this receipt would" - " have claimed is not what this host holds") if lost else None, + "settingsObserved": settings_now, + "error": ("; ".join(reasons) + ". Something changed this host while the run was" + " writing, so what this receipt would have claimed is not what the" + " host holds") if lost else None, "note": ("Settings written; no registration was made and the hook file was" " not touched. The " + completion.OWNER_PLUGIN + " owner registers" " this event through the plugin package's own manifest, so install" " that package to register it. Written, registered and observed to" " have fired stay three separate claims. launcherObserved is what" - " both paths held after the writes, not what this run intended.")}) + " the launcher path held after the writes and settingsObserved is" + " what the settings path held, neither of them what this run" + " intended.")}) return EXIT_INCOMPLETE if lost else EXIT_OK else: command = args.hook_command From 89a169fc007e1f27a8f898de8c00921685ba4b7b Mon Sep 17 00:00:00 2001 From: thisisjun786 <259586770+thisisjun786@users.noreply.github.com> Date: Mon, 21 Sep 2026 20:33:36 +0900 Subject: [PATCH 07/15] Let a busy launcher lock stop the migration instead of answering it 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. --- scripts/ci/tests/test_plugin_transition.py | 33 ++++++++++++++++++++++ scripts/crw_transition/steps.py | 11 +++++--- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/scripts/ci/tests/test_plugin_transition.py b/scripts/ci/tests/test_plugin_transition.py index e68ee2657..e81a9637e 100644 --- a/scripts/ci/tests/test_plugin_transition.py +++ b/scripts/ci/tests/test_plugin_transition.py @@ -359,6 +359,39 @@ def test_a_dry_run_migration_places_no_launcher(self): self.assertEqual([item["outcome"] for item in step], ["would_change"]) + def test_a_held_launcher_lock_stops_the_sequence_before_the_settings(self): + """Devin review: a busy launcher must not let the settings be written without it. + + transition() owns the Busy handler: it names the step that was running and marks every + later step not reached. Answering busy inside the step instead would hand the main loop + an ordinary result it does not break on, and the host would end with plugin-owned + settings and no fallback -- which is the combination the step exists to prevent. + """ + import sys as _sys + _sys.path.insert(0, str(ROOT / "scripts")) + from crw_runtime import completion as _completion, hostrecord as _hostrecord + + host = self.ready() + launcher = Path(host.home) / _completion.LAUNCHER_NAME + lock = Path(str(launcher) + _hostrecord.LOCK_SUFFIX) + lock.parent.mkdir(parents=True, exist_ok=True) + lock.write_text("99999999", encoding="utf-8") + self.addCleanup(lambda: lock.exists() and lock.unlink()) + code, answer = host.transition("--apply") + self.assertEqual(code, 1, json.dumps(answer["results"])[:700]) + outcomes = {item["step"]: item["outcome"] for item in answer["results"]} + self.assertEqual(outcomes.get("stable launcher install"), "busy", + json.dumps(answer["results"])[:700]) + self.assertEqual(outcomes.get("settings install"), "not_reached", + json.dumps(answer["results"])[:700]) + self.assertFalse(launcher.exists()) + # No plugin-owned settings were written, which is the claim. 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, not something this one adds. + landed = host.settings() + self.assertTrue(landed is None or landed.get("owner") != "plugin", landed) + + def test_the_retired_settings_and_record_are_kept_not_deleted(self): host = self.ready() host.transition("--apply") diff --git a/scripts/crw_transition/steps.py b/scripts/crw_transition/steps.py index cbcc35224..657203d70 100644 --- a/scripts/crw_transition/steps.py +++ b/scripts/crw_transition/steps.py @@ -1490,14 +1490,17 @@ def launcher_install(host, options, *, apply=False): Before settings install, for the reason that ordering exists everywhere else here: a launcher with no settings stands down in silence, while settings whose fallback was never placed look installed and are not. A refusal here therefore stops the sequence with the host unchanged. + + hostrecord.Busy is deliberately NOT caught. transition() catches it, names the step that was + running and marks every later step not reached; catching it here would return an ordinary + busy answer that the main loop does not break on, and settings install would then write a + plugin-owned document for a host whose fallback was never placed -- the exact combination + this step was added to prevent. """ step = "stable launcher install" path = completion.launcher_path(Path(host["codexHome"])) source = Path(host["repoRoot"]) / completion.LAUNCHER_SOURCE - try: - placed = completion.place_launcher(path, source, apply=apply) - except hostrecord.Busy as error: - return _answer(step, BUSY, str(error)) + placed = completion.place_launcher(path, source, apply=apply) outcome = placed["outcome"] settled = SETTLED if outcome == completion.LAUNCHER_PLACED else ( ALREADY if outcome == completion.LAUNCHER_UNCHANGED else ( From a8918e0b927748097aa6d74d6d64f523666922fc Mon Sep 17 00:00:00 2001 From: thisisjun786 <259586770+thisisjun786@users.noreply.github.com> Date: Mon, 21 Sep 2026 21:10:28 +0900 Subject: [PATCH 08/15] Place the launcher before anything is taken away, and share one budget 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. --- scripts/ci/tests/test_plugin_transition.py | 53 +++++++++++++++++----- scripts/ci/tests/test_plugin_wiring.py | 51 +++++++++++++++++++++ scripts/crw_runtime/completion.py | 28 +++++++++--- scripts/crw_transition/steps.py | 26 ++++++----- 4 files changed, 130 insertions(+), 28 deletions(-) diff --git a/scripts/ci/tests/test_plugin_transition.py b/scripts/ci/tests/test_plugin_transition.py index e81a9637e..be24ffbcf 100644 --- a/scripts/ci/tests/test_plugin_transition.py +++ b/scripts/ci/tests/test_plugin_transition.py @@ -360,12 +360,16 @@ def test_a_dry_run_migration_places_no_launcher(self): def test_a_held_launcher_lock_stops_the_sequence_before_the_settings(self): - """Devin review: a busy launcher must not let the settings be written without it. + """A busy launcher must not let the rest of the sequence run without it. transition() owns the Busy handler: it names the step that was running and marks every later step not reached. Answering busy inside the step instead would hand the main loop - an ordinary result it does not break on, and the host would end with plugin-owned - settings and no fallback -- which is the combination the step exists to prevent. + an ordinary result it does not break on. + + The step runs first, ahead of everything destructive, so a refusal leaves the host + exactly as it was. Placed after the retire and the standdown it did the opposite: the + settings were archived and the manual registration removed with nothing to put them + back, which is a worse host than the one the command started with. """ import sys as _sys _sys.path.insert(0, str(ROOT / "scripts")) @@ -377,20 +381,41 @@ def test_a_held_launcher_lock_stops_the_sequence_before_the_settings(self): lock.parent.mkdir(parents=True, exist_ok=True) lock.write_text("99999999", encoding="utf-8") self.addCleanup(lambda: lock.exists() and lock.unlink()) + before = host.settings() + hooks_before = host.hooks_document() code, answer = host.transition("--apply") self.assertEqual(code, 1, json.dumps(answer["results"])[:700]) outcomes = {item["step"]: item["outcome"] for item in answer["results"]} self.assertEqual(outcomes.get("stable launcher install"), "busy", json.dumps(answer["results"])[:700]) - self.assertEqual(outcomes.get("settings install"), "not_reached", - json.dumps(answer["results"])[:700]) + for later in ("settings retire", "hook standdown", "settings install"): + self.assertEqual(outcomes.get(later), "not_reached", later) self.assertFalse(launcher.exists()) - # No plugin-owned settings were written, which is the claim. 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, not something this one adds. - landed = host.settings() - self.assertTrue(landed is None or landed.get("owner") != "plugin", landed) + self.assertEqual(host.settings(), before) + self.assertEqual(host.hooks_document(), hooks_before) + + def test_a_foreign_launcher_stops_the_sequence_before_anything_is_taken_away(self): + """The same ordering question asked with a refusal instead of a contended lock.""" + import sys as _sys + _sys.path.insert(0, str(ROOT / "scripts")) + from crw_runtime import completion as _completion + host = self.ready() + launcher = Path(host.home) / _completion.LAUNCHER_NAME + foreign = "not ours\n" + launcher.write_text(foreign, encoding="utf-8") + before = host.settings() + hooks_before = host.hooks_document() + code, answer = host.transition("--apply") + self.assertEqual(code, 1, json.dumps(answer["results"])[:700]) + outcomes = {item["step"]: item["outcome"] for item in answer["results"]} + self.assertEqual(outcomes.get("stable launcher install"), "refused", + json.dumps(answer["results"])[:700]) + for later in ("settings retire", "hook standdown", "settings install"): + self.assertEqual(outcomes.get(later), "not_reached", later) + self.assertEqual(launcher.read_text(encoding="utf-8"), foreign) + self.assertEqual(host.settings(), before) + self.assertEqual(host.hooks_document(), hooks_before) def test_the_retired_settings_and_record_are_kept_not_deleted(self): host = self.ready() @@ -1253,7 +1278,13 @@ def test_stopping_after_the_retire_still_leaves_the_custom_file_discoverable(sel fixed.unlink() host.install_plugin() snapshot = inventory.snapshot(host.home, repo_root=ROOT) - self.assertEqual(steps.ORDER[0][0], "settings retire") + # The invariant this case is about is the retire running before the standdown, not + # the retire being first overall: the non-destructive launcher step now precedes + # both so that a refusal there takes nothing away. + order = [name for name, _ in steps.ORDER] + self.assertLess(order.index("settings retire"), order.index("hook standdown")) + self.assertLess(order.index("stable launcher install"), + order.index("settings retire")) self.assertEqual(steps.settings_retire(snapshot, {}, apply=True)["outcome"], "settled") # Stopped here: the registration is still in place and the archive is already in the home. recovered, name = inventory.newest_retired(host.home) diff --git a/scripts/ci/tests/test_plugin_wiring.py b/scripts/ci/tests/test_plugin_wiring.py index 77ea2a446..ee3e18cc6 100644 --- a/scripts/ci/tests/test_plugin_wiring.py +++ b/scripts/ci/tests/test_plugin_wiring.py @@ -1582,3 +1582,54 @@ def test_disable_never_claims_the_fallback_it_does_not_touch(self): """disable stops calls and deletes nothing, so the fallback is not its surface to report.""" claims = self.steps.stop_claims([]) self.assertFalse(any("fallback" in claim for claim in claims["stillLive"])) + +class PluginGuardBudgetTest(unittest.TestCase): + """The budget a plugin-owned document may record, enforced where both writers validate. + + The packaged launcher waits min(timeoutSeconds + MARGIN, CEILING). Refusing only at the + ceiling let 8 through, and at 8 the launcher deadline is 9 while the adapter is still + allowed 8: the margin is gone and the launcher kills the adapter in the middle of writing + the record of its own timeout. The bound lived in the transition only, so + runtime_install.py hook --owner plugin accepted a document the transition refused. + """ + + def document(self, budget): + return {"configVersion": 1, "event": "Stop", "owner": "plugin", "mode": "observe", + "timeoutSeconds": budget, "adapterInterpreter": "/usr/bin/python3", + "adapterEntryPoint": "/opt/crw/bin/crw-completion-hook", + "relayExecutable": "/opt/crw/bin/codex-session-relay", + "markerRoot": "/opt/crw/marker"} + + def test_the_bound_is_the_ceiling_minus_the_margin(self): + self.assertEqual(completion.MAX_PLUGIN_GUARD_SECONDS, + completion.LAUNCHER_CEILING_SECONDS + - completion.LAUNCHER_MARGIN_SECONDS) + self.assertEqual(completion.MAX_PLUGIN_GUARD_SECONDS, 7) + + def test_a_budget_that_collapses_the_margin_is_refused(self): + for budget in (8, 8.5, 9, 10): + with self.subTest(budget=budget): + found = completion.complaints(self.document(budget)) + self.assertTrue([item for item in found if "timeoutSeconds" in item], + "budget %r was accepted: %r" % (budget, found)) + + def test_a_budget_that_keeps_the_margin_is_accepted(self): + for budget in (1, 5, 7): + with self.subTest(budget=budget): + found = completion.complaints(self.document(budget)) + self.assertEqual([item for item in found if "timeoutSeconds" in item], [], + "budget %r was refused" % budget) + + def test_the_transition_and_the_installer_share_one_bound(self): + """They disagreed before: one refused 8 and the other wrote it.""" + sys.path.insert(0, str(ROOT / "scripts")) + from crw_transition import steps + + self.assertEqual(steps.MAX_GUARD_SECONDS, completion.MAX_PLUGIN_GUARD_SECONDS) + self.assertEqual(steps.LAUNCHER_MARGIN_SECONDS, completion.LAUNCHER_MARGIN_SECONDS) + + def test_the_launcher_mirrors_the_same_two_numbers(self): + """The launcher cannot import this module, so the numbers are asserted to agree.""" + source = (ROOT / "plugins/crw/wiring/crw_stop_hook.py").read_text(encoding="utf-8") + self.assertIn("MAX_SECONDS = " + str(completion.LAUNCHER_CEILING_SECONDS), source) + self.assertIn("MARGIN_SECONDS = " + str(completion.LAUNCHER_MARGIN_SECONDS), source) diff --git a/scripts/crw_runtime/completion.py b/scripts/crw_runtime/completion.py index b08a7c816..b6a593c2c 100644 --- a/scripts/crw_runtime/completion.py +++ b/scripts/crw_runtime/completion.py @@ -106,6 +106,19 @@ # would have explained the timeout, and then releases the turn saying nothing. Mirrored in # plugins/crw/wiring/crw_stop_hook.py, which cannot import this module. LAUNCHER_CEILING_SECONDS = 9 +# The margin that launcher keeps between its own deadline and the adapter's budget, mirrored +# from the same file. The two numbers only mean something together: the launcher waits +# min(timeoutSeconds + MARGIN, CEILING), so a budget above CEILING - MARGIN collapses the margin +# the launcher exists to keep, and the launcher's deadline then arrives while the adapter is +# still writing the record of its own timeout. +# +# This bound used to live only in scripts/crw_transition/steps.py, which meant the transition +# refused such a document and runtime_install.py hook --owner plugin accepted it. That was +# tolerable while the packaged launcher was reached only through the cache; it is not now that +# the same command installs the fallback every plugin host depends on, so the bound moved here, +# where both writers already validate. +LAUNCHER_MARGIN_SECONDS = 2 +MAX_PLUGIN_GUARD_SECONDS = LAUNCHER_CEILING_SECONDS - LAUNCHER_MARGIN_SECONDS # The largest budget any of these checks will entertain. Compared against rather than converted, # because an arbitrary-precision integer cannot always become a float: math.isfinite raises @@ -511,13 +524,16 @@ def complaints(document): " this repository's adapter, so the install records it here") budget = document.get("timeoutSeconds") if isinstance(budget, (int, float)) and not isinstance(budget, bool) \ - and budget >= LAUNCHER_CEILING_SECONDS: - # The packaged launcher sits between the host and the adapter and caps its own - # deadline here, so a guard budget at or above that ceiling lets the launcher kill - # the adapter first and release the turn without the record that explains it. - found.append("timeoutSeconds must be under " + str(LAUNCHER_CEILING_SECONDS) + and budget > MAX_PLUGIN_GUARD_SECONDS: + # The packaged launcher waits min(budget + MARGIN, CEILING). Refusing only at the + # ceiling let 8 through, and at 8 the launcher's own deadline is 9 while the adapter + # is still allowed 8: the margin is gone, the launcher kills the adapter first, and + # the turn is released without the record that explains it. + found.append("timeoutSeconds must not exceed " + str(MAX_PLUGIN_GUARD_SECONDS) + " when owner is " + OWNER_PLUGIN + ", because the packaged launcher" - " caps its own deadline there and has to outlast the adapter it runs") + " waits the budget plus " + str(LAUNCHER_MARGIN_SECONDS) + "s capped at " + + str(LAUNCHER_CEILING_SECONDS) + "s and has to outlast the adapter" + " it runs") budget = document.get("timeoutSeconds") if budget is not None and not usable_seconds(budget): found.append("timeoutSeconds must be a positive number of seconds, at most " diff --git a/scripts/crw_transition/steps.py b/scripts/crw_transition/steps.py index 657203d70..3e9c4e2a4 100644 --- a/scripts/crw_transition/steps.py +++ b/scripts/crw_transition/steps.py @@ -43,15 +43,13 @@ # "converged" and "did the work" are different answers and a rerun has to be able to say which. DONE = (SETTLED, ALREADY) -# The packaged launcher waits min(timeoutSeconds + MARGIN, MAX) seconds, with MARGIN 2 and MAX the -# number completion.py calls LAUNCHER_CEILING_SECONDS. So a budget at the ceiling is not the -# problem the ceiling was written for: every budget above MAX - MARGIN collapses the margin the -# launcher exists to keep, and at 8.999 the launcher's deadline arrives first and discards the -# record the adapter was in the middle of writing. What a plugin-owned document may record is -# therefore the ceiling minus the margin, and this is where that is enforced, because the -# validation the two adapters share lives in a module this branch does not own. -LAUNCHER_MARGIN_SECONDS = 2 -MAX_GUARD_SECONDS = completion.LAUNCHER_CEILING_SECONDS - LAUNCHER_MARGIN_SECONDS +# The packaged launcher waits min(timeoutSeconds + MARGIN, CEILING), so a budget above +# CEILING - MARGIN collapses the margin the launcher exists to keep. This used to be enforced +# here alone, which left runtime_install.py hook --owner plugin accepting a document this branch +# would refuse. The bound now lives with the validation both writers share and these names are +# aliases of it, so the two paths cannot drift apart again. +LAUNCHER_MARGIN_SECONDS = completion.LAUNCHER_MARGIN_SECONDS +MAX_GUARD_SECONDS = completion.MAX_PLUGIN_GUARD_SECONDS def stamp(): @@ -1914,8 +1912,14 @@ def skill_unlink(host, options, *, apply=False): # host with no completion hook. Retiring first costs a window in which the old registration runs # against absent settings -- it releases in silence and records nothing -- and no window in which # two adapters run, because the plugin-owned settings are still not installed. -ORDER = (("settings retire", settings_retire), ("hook standdown", hook_standdown), - ("stable launcher install", launcher_install), +# The launcher goes FIRST, ahead of everything that takes something away. It is the only +# non-destructive step here and the only one that can refuse on a condition outside this command +# -- a foreign file at the path, or another run holding its lock. Placed after the retire and the +# standdown, such a refusal left the host with its settings archived and its manual registration +# removed and nothing to restore them, which is a worse host than the one the command started +# with. First, it refuses before anything is taken away. +ORDER = (("stable launcher install", launcher_install), + ("settings retire", settings_retire), ("hook standdown", hook_standdown), ("settings install", settings_install), ("mcp record retire", mcp_record_retire), ("mcp table standdown", mcp_table_standdown), ("mcp record install", mcp_record_install), ("skill unlink", skill_unlink)) From ad4dc4e075f5588e7057e18835b9d57e0aac7d15 Mon Sep 17 00:00:00 2001 From: thisisjun786 <259586770+thisisjun786@users.noreply.github.com> Date: Mon, 21 Sep 2026 21:24:08 +0900 Subject: [PATCH 09/15] Pin the precondition that keeps an unusable document away from the fallback 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. --- plugins/crw/wiring/crw_stop_hook.py | 9 ++++---- scripts/ci/tests/test_plugin_wiring.py | 31 ++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/plugins/crw/wiring/crw_stop_hook.py b/plugins/crw/wiring/crw_stop_hook.py index 5dac5dd32..d6b40e40d 100644 --- a/plugins/crw/wiring/crw_stop_hook.py +++ b/plugins/crw/wiring/crw_stop_hook.py @@ -65,10 +65,11 @@ # adapter only while the recorded budget stays at or under MAX_SECONDS - MARGIN_SECONDS: above # that the cap eats the margin, and at a budget just under MAX_SECONDS this deadline arrives # while the adapter is still writing the record of its own timeout. Settings that record such a -# budget are refused where they are written -- scripts/crw_transition/steps.py, which derives its -# limit from these two numbers -- rather than here, because this launcher cannot wait longer than -# the hook it is registered under. If one of these numbers moves, that limit has to move with it; -# a test asserts they still agree, because this file cannot import that module. +# budget are refused where they are written rather than here, because this launcher cannot wait +# longer than the hook it is registered under. That limit is completion.MAX_PLUGIN_GUARD_SECONDS, +# derived from these same two numbers and shared by both writers; scripts/crw_transition/steps.py +# aliases it rather than keeping a second copy. If one of these numbers moves, that limit moves +# with it; a test asserts they still agree, because this file cannot import that module. def settings_path(): diff --git a/scripts/ci/tests/test_plugin_wiring.py b/scripts/ci/tests/test_plugin_wiring.py index ee3e18cc6..142b7b329 100644 --- a/scripts/ci/tests/test_plugin_wiring.py +++ b/scripts/ci/tests/test_plugin_wiring.py @@ -1478,6 +1478,37 @@ def test_a_packaged_source_that_cannot_be_read_places_nothing(self): self.assertEqual(answer["outcome"], completion.LAUNCHER_SOURCE_MISSING) self.assertFalse(self.path.exists()) + def test_a_legacy_document_is_refused_before_a_fallback_is_placed(self): + """Devin review: a fallback must not be wired to settings this command will not accept. + + Measured rather than assumed: the ownership precondition reads the live document and + runs the same complaints() over it, so a host carrying the old eight second budget is + refused there, before the placement block is reached at all. The emitted receipt carries + no launcher cell and nothing is written. This case pins that ordering, because the + placement is only safe while some earlier precondition keeps an unusable document from + ever reaching it. + """ + home = self.home / "legacy" + home.mkdir() + (home / completion.CONFIG_NAME).write_text(json.dumps( + {"configVersion": 1, "event": "Stop", "owner": "plugin", "mode": "observe", + "timeoutSeconds": 8, "adapterInterpreter": sys.executable, + "adapterEntryPoint": sys.executable, "relayExecutable": "/bin/true", + "markerRoot": str(home / "marker")}), encoding="utf-8") + relay = self.home / "relay" + relay.write_text("#!/bin/sh\nexit 0\n", encoding="utf-8") + relay.chmod(0o755) + done = subprocess.run( + [sys.executable, str(ROOT / "scripts" / "runtime_install.py"), "hook", + "--adapter", "completion", "--owner", "plugin", "--codex-home", str(home), + "--relay-command", str(relay), "--apply"], capture_output=True, text=True) + self.assertNotEqual(done.returncode, 0, done.stdout[:400]) + emitted = json.loads(done.stdout) + self.assertIsNone(emitted.get("launcher")) + self.assertIn("timeoutSeconds", emitted["error"]) + self.assertFalse(completion.launcher_path(home).exists()) + + def test_the_state_reader_separates_the_marker_from_the_digest(self): self.place(apply=True) state = completion.launcher_state(self.home, self.SOURCE) From 8abd13ccc3136ec6f9c2ceca74658e524086241f Mon Sep 17 00:00:00 2001 From: thisisjun786 <259586770+thisisjun786@users.noreply.github.com> Date: Mon, 21 Sep 2026 21:30:19 +0900 Subject: [PATCH 10/15] Document the fallback where each command owns it, and compare bytes 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. --- docs/plugin-transition.md | 40 +++++++++++++++------- docs/runtime-install.md | 39 +++++++++++++++++++++ scripts/ci/tests/test_plugin_transition.py | 25 +++++++++----- 3 files changed, 84 insertions(+), 20 deletions(-) diff --git a/docs/plugin-transition.md b/docs/plugin-transition.md index 026ec482c..4f4c2fd28 100644 --- a/docs/plugin-transition.md +++ b/docs/plugin-transition.md @@ -52,17 +52,33 @@ declaration is the double fire this exists to prevent, whoever created it. ## The order, and the two windows preflight - 1 retire the settings the registration names - 2 remove the registration that runs our adapter - 3 write the plugin-owned settings, recording the adapter under the destination pointer - 4 retire the user-owned bridge record - 5 remove the config.toml table - 6 write the plugin-owned bridge record - 7 remove the CRW-owned skill links - -Part of that order is forced, and the forced part is what prevents doubles: the registration + 1 place the fallback Stop launcher this host will need + 2 retire the settings the registration names + 3 remove the registration that runs our adapter + 4 write the plugin-owned settings, recording the adapter under the destination pointer + 5 retire the user-owned bridge record + 6 remove the config.toml table + 7 write the plugin-owned bridge record + 8 remove the CRW-owned skill links + +Step 1 goes first because it is the only step here that takes nothing away and the only one that +can refuse on a condition outside this command: a file at the stable launcher path that is not +ours, or another run holding its lock. Placed after the retire and the standdown, such a refusal +left the host with its settings archived and its manual registration removed and nothing to put +them back, which is a worse host than the one the command started with. First, it refuses before +anything has been taken. + +It is here at all because this command writes the same plugin-owned settings +`runtime_install.py hook --owner plugin` writes. A migration that stopped at the settings would +leave the package's Stop declaration with one candidate again, and the first package replacement +during an open turn would land back in the loop that declaration exists to avoid. See +[the fallback launcher](runtime-install.md#the-fallback-launcher-and-why-this-command-places-it) +for what it refuses and who removes it, and [the cache lifetime](plugin-packaging.md#the-cache-lifetime) +for why a second candidate is needed at all. + +The rest of that order is forced, and the forced part is what prevents doubles: the registration goes before the new settings, the old settings go before the new ones, and the table goes before the -plugin record. Steps 4 to 6 are held under the same ownership lock `register-mcp` takes, because a +plugin record. Steps 5 to 7 are held under the same ownership lock `register-mcp` takes, because a user-owned registration landing in the middle would put the record back and leave the host with no bridge. @@ -84,12 +100,12 @@ where it found nothing to archive and where every document it did find already n Every step decides from the host as it stands at that step, not from the snapshot the run opened with. The bridge surface is re-read inside the ownership lock, the hook file is re-read and its registrations re-proved inside the hook lock, the bridge table's span and its proof are re-derived -before a byte is removed, and the skills directory is inventoried again at step 7 and once more +before a byte is removed, and the skills directory is inventoried again at step 8 and once more after it. That last one is a weaker guarantee than the other two and is named as such: the directory has no lock, so a link arriving during the removals is reported rather than prevented, and the run refuses instead of reporting success over it. -Step 7 removes nothing until every link it would remove has been proved, and then proves each one +Step 8 removes nothing until every link it would remove has been proved, and then proves each one again in the moment before it is unlinked. Neither pass is a lock. The first stops a refusal from leaving half a manual installation behind; the second stops a link replaced during the removals from being deleted as though it were still ours, and narrows that window to the gap between a diff --git a/docs/runtime-install.md b/docs/runtime-install.md index f289733a9..83cb5cc3b 100644 --- a/docs/runtime-install.md +++ b/docs/runtime-install.md @@ -1393,6 +1393,45 @@ malformed. The adapter's budget is checked against the registered timeout at the because that is the one value whose meaning needs both files: a budget the host's timeout does not exceed lets the host kill the adapter before it records why it did not answer. + +### The fallback launcher, and why this command places it + +With `--owner plugin` this command also writes `crw-stop-hook.py` beside those settings, and it +writes it **before** them. That file is the second candidate the package’s Stop declaration +opens, and it exists because a hook command is fixed when a turn starts, with the plugin root +already resolved into it. Installing a version removes the previous cache directory whole, so an +update landing mid-turn leaves that turn’s command naming a file that is gone, and `python3` +exits 2 for a missing script — the hook protocol’s blocking code. Measured on a real host: the +same missing-file error 74 times in one turn, and a task that could not finish. +[Plugin packaging](plugin-packaging.md#the-cache-lifetime) owns the full reference table and the +supported range; what belongs here is who writes the file and what that writer refuses. + +| Question | Answer | +| --- | --- | +| Which command writes it | This one, with `--owner plugin`. `plugin_transition.py transition` writes it too, because it installs the same plugin-owned settings and would otherwise leave a host with the settings and no fallback | +| In what order | Launcher first. A launcher with no settings stands down in silence; settings whose fallback was never placed look installed and are not | +| What it refuses | A file that does not carry the launcher marker, and anything that is not a regular file. A symlink is reported by kind and never followed, because replacing through one writes to a file this command was never given | +| What the marker proves | That CRW put a launcher at that path. Not who ran the command, and not that the bytes are intact. The digest is reported beside it for the second question | +| How it is replaced | Temp file and `os.replace`, then read back, with the kind and the marker re-judged under the lock immediately before the write | +| Which command removes it | `plugin_transition.py remove`, under the launcher’s own lock and only while the marker is still there | +| What `disable` does to it | Nothing. `disable` stops new calls by retiring the settings and deletes no bytes | + +The two files take separate locks and there is deliberately no lock spanning them. Widening one +means reworking a write path that is already proven, and it is not needed: every state the pair +can be left in is harmless. A launcher alone stands down. Settings alone are what the host had +before this file existed. What an interleaving can still do is make a receipt wrong, so the run +reads both paths back at the end and reports `launcherObserved` and `settingsObserved` — what the +host held, not what the run intended — and exits `3` when either half is missing, changed, or no +longer what it wrote. That status is the same fourth answer `install` uses: not a refusal, +because bytes really were written, and not success, because the result did not stay in effect. + +A plugin-owned budget is capped at `completion.MAX_PLUGIN_GUARD_SECONDS`, the launcher ceiling +minus its margin. The launcher waits `min(timeoutSeconds + 2, 9)`, so a budget above 7 collapses +the margin it exists to keep and the launcher’s deadline arrives while the adapter is still +writing the record of its own timeout. That bound lived in the transition alone until this +launcher became something every plugin host depends on; it now sits with the validation both +writers share, and the transition aliases it rather than keeping a second copy. + ### Who registers the hook The plugin package declares this hook as well, and a host holding both registrations runs both diff --git a/scripts/ci/tests/test_plugin_transition.py b/scripts/ci/tests/test_plugin_transition.py index be24ffbcf..546d4df1f 100644 --- a/scripts/ci/tests/test_plugin_transition.py +++ b/scripts/ci/tests/test_plugin_transition.py @@ -152,6 +152,19 @@ def settings(self): path = self.home / "crw-completion-hook.json" return json.loads(path.read_text(encoding="utf-8")) if path.is_file() else None + def untouched(self): + """The raw bytes of the two files a refused run must not have moved. + + Parsed documents compare equal across a rewrite that reorders keys or changes spacing, + and "nothing was taken away" is a claim about the files rather than about their meaning. + None for an absent file, so a retire that moved one is a difference rather than a crash. + """ + out = {} + for name in ("crw-completion-hook.json", "hooks.json"): + path = self.home / name + out[name] = path.read_bytes() if path.is_file() else None + return out + def record(self): path = self.home / "crw-bridge-mcp.json" return json.loads(path.read_text(encoding="utf-8")) if path.is_file() else None @@ -381,8 +394,7 @@ def test_a_held_launcher_lock_stops_the_sequence_before_the_settings(self): lock.parent.mkdir(parents=True, exist_ok=True) lock.write_text("99999999", encoding="utf-8") self.addCleanup(lambda: lock.exists() and lock.unlink()) - before = host.settings() - hooks_before = host.hooks_document() + untouched_before = host.untouched() code, answer = host.transition("--apply") self.assertEqual(code, 1, json.dumps(answer["results"])[:700]) outcomes = {item["step"]: item["outcome"] for item in answer["results"]} @@ -391,8 +403,7 @@ def test_a_held_launcher_lock_stops_the_sequence_before_the_settings(self): for later in ("settings retire", "hook standdown", "settings install"): self.assertEqual(outcomes.get(later), "not_reached", later) self.assertFalse(launcher.exists()) - self.assertEqual(host.settings(), before) - self.assertEqual(host.hooks_document(), hooks_before) + self.assertEqual(host.untouched(), untouched_before) def test_a_foreign_launcher_stops_the_sequence_before_anything_is_taken_away(self): """The same ordering question asked with a refusal instead of a contended lock.""" @@ -404,8 +415,7 @@ def test_a_foreign_launcher_stops_the_sequence_before_anything_is_taken_away(sel launcher = Path(host.home) / _completion.LAUNCHER_NAME foreign = "not ours\n" launcher.write_text(foreign, encoding="utf-8") - before = host.settings() - hooks_before = host.hooks_document() + untouched_before = host.untouched() code, answer = host.transition("--apply") self.assertEqual(code, 1, json.dumps(answer["results"])[:700]) outcomes = {item["step"]: item["outcome"] for item in answer["results"]} @@ -414,8 +424,7 @@ def test_a_foreign_launcher_stops_the_sequence_before_anything_is_taken_away(sel for later in ("settings retire", "hook standdown", "settings install"): self.assertEqual(outcomes.get(later), "not_reached", later) self.assertEqual(launcher.read_text(encoding="utf-8"), foreign) - self.assertEqual(host.settings(), before) - self.assertEqual(host.hooks_document(), hooks_before) + self.assertEqual(host.untouched(), untouched_before) def test_the_retired_settings_and_record_are_kept_not_deleted(self): host = self.ready() From 00e5594025749d544930e15175850343b5622b17 Mon Sep 17 00:00:00 2001 From: thisisjun786 <259586770+thisisjun786@users.noreply.github.com> Date: Mon, 21 Sep 2026 21:36:32 +0900 Subject: [PATCH 11/15] Say in AGENTS.md that a launcher also lives outside the cache 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. --- AGENTS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 0129cc3c2..08af57972 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -3,7 +3,7 @@ - Read [POLICY.md](POLICY.md) for repository CI, merge and release authority and [CONTRIBUTING.md](CONTRIBUTING.md) for the contribution path. `dev` is the default integration branch; `main` accepts only explicitly authorized release promotions from this repository's `dev`. - This repository holds the skill instructions under `plugins/crw/skills/` and two imported Python packages under `packages/`. The repository root keeps `skills` as a link to that directory so installations made before the move still resolve; `scripts/install.py`, the checks and the package all read the path the manifest declares. Read the target skill and its linked references before editing. Keep skill names, folder names, UI prompts, and relative links consistent. - `plugins/crw` is the published plugin package and `.agents/plugins/marketplace.json` is its marketplace entry. Installation copies the plugin root verbatim, including untracked and ignored files, so what may live there is `.codex-plugin/`, `LICENSE` and the components the manifest declares: today `skills/` and `wiring/`. A component nobody declares installs without ever loading, so adding a directory means declaring it. Run `python3 scripts/ci/plugin.py` after touching the package, the manifest or the skills; it derives the permitted roots from the manifest. See [plugin packaging](docs/plugin-packaging.md). -- `plugins/crw/wiring` holds the declared Stop hook and MCP server and the two launchers they start. The launchers stay standard-library-only and never bundle a runtime: the version cache is replaced wholesale on every install. One owner registers each surface; `scripts/runtime_install.py` writes the records they read and refuses the second owner. See [runtime installation](docs/runtime-install.md). +- `plugins/crw/wiring` holds the declared Stop hook and MCP server and the two launchers they start. The launchers stay standard-library-only and never bundle a runtime: the version cache is replaced wholesale on every install. That applies to the launchers themselves, so the Stop declaration opens the packaged copy first and a copy at `/crw-stop-hook.py` when the cache path is already gone; `scripts/runtime_install.py` and `scripts/plugin_transition.py` both place that copy, launcher before settings. One owner registers each surface; `scripts/runtime_install.py` writes the records they read and refuses the second owner. See [runtime installation](docs/runtime-install.md) and [the cache lifetime](docs/plugin-packaging.md#the-cache-lifetime). - Maintain the `plugins/crw/skills/crw-*` directories together. Shared workflow rules live in `plugins/crw/skills/crw-plan/references/integrations.md`; operation-specific rules stay in their owning skill. - `packages/codex-thread-bridge` and `packages/codex-session-relay` keep their own `pyproject.toml`, module names, CLI names, tests and `.gitignore`. Preserve the bridge's upstream MIT notice. The root `pyproject.toml` and `uv.lock` are the uv workspace; run `python3 scripts/ci/packages.py` after changing either package. - Keep product documents in Linear and private receipts outside this repository. Do not vendor CXC/Paperthin, credentials, session history, or generated runtime state. From c4ba2c8f2f858507d4dcac4fc88ee41282120d4f Mon Sep 17 00:00:00 2001 From: thisisjun786 <259586770+thisisjun786@users.noreply.github.com> Date: Mon, 21 Sep 2026 21:56:20 +0900 Subject: [PATCH 12/15] Judge the document before placing the fallback, which I said was unnecessary 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. --- scripts/ci/tests/test_plugin_wiring.py | 34 ++++++++++++++++++++++++++ scripts/runtime_install.py | 10 ++++++++ 2 files changed, 44 insertions(+) diff --git a/scripts/ci/tests/test_plugin_wiring.py b/scripts/ci/tests/test_plugin_wiring.py index 142b7b329..57a98a0d8 100644 --- a/scripts/ci/tests/test_plugin_wiring.py +++ b/scripts/ci/tests/test_plugin_wiring.py @@ -1509,6 +1509,40 @@ def test_a_legacy_document_is_refused_before_a_fallback_is_placed(self): self.assertFalse(completion.launcher_path(home).exists()) + def test_a_budget_the_plugin_owner_may_not_record_places_no_fallback(self): + """Devin review, by the route the first measurement missed. + + The earlier check exercised a host that already carried a bad document, and the + ownership precondition caught that one before the placement. A fresh host with the + budget on the command line took a different road: the document was only judged inside + write_configuration, which runs after the launcher is placed, so the run left a + launcher on disk and then refused the settings. The judgement moved into the + preconditions, where "nothing was written" is promised. + """ + relay = self.home / "relay" + relay.write_text("#!/bin/sh\nexit 0\n", encoding="utf-8") + relay.chmod(0o755) + for budget, expected in ((8, False), (7, True)): + with self.subTest(budget=budget): + home = self.home / ("budget-%d" % budget) + done = subprocess.run( + [sys.executable, str(ROOT / "scripts" / "runtime_install.py"), "hook", + "--adapter", "completion", "--owner", "plugin", "--codex-home", str(home), + "--relay-command", str(relay), "--guard-timeout", str(budget), "--apply"], + capture_output=True, text=True) + placed = completion.launcher_path(home).exists() + self.assertEqual(placed, expected, + "budget %d: launcher placed=%s\n%s" + % (budget, placed, done.stdout[:400])) + settled = (home / completion.CONFIG_NAME).exists() + self.assertEqual(settled, expected, "budget %d: settings=%s" % (budget, settled)) + if expected: + self.assertEqual(done.returncode, 0, done.stdout[:300]) + else: + self.assertNotEqual(done.returncode, 0, done.stdout[:300]) + self.assertIn("timeoutSeconds", json.loads(done.stdout)["error"]) + + def test_the_state_reader_separates_the_marker_from_the_digest(self): self.place(apply=True) state = completion.launcher_state(self.home, self.SOURCE) diff --git a/scripts/runtime_install.py b/scripts/runtime_install.py index a1dd63361..dde169d6a 100755 --- a/scripts/runtime_install.py +++ b/scripts/runtime_install.py @@ -2324,6 +2324,16 @@ def cmd_hook(args): adapter_interpreter=interpreter, adapter_entry_point=ROOT / "scripts" / completion.ENTRY_POINT_NAME, ) + # The document this run would write, judged HERE rather than inside + # write_configuration. That is the same check, and it used to be the first + # thing write_configuration did -- but write_configuration runs after the + # fallback launcher is placed, so a budget the plugin owner may not record + # (anything above completion.MAX_PLUGIN_GUARD_SECONDS) left a launcher on + # disk and then refused the settings. A launcher alone is harmless, but + # installing one for a document this command just called unusable is not + # something to do quietly, and the preconditions are where "nothing was + # written" is promised. + refused += completion.complaints(wanted) except ValueError as error: refused = [str(error)] if refused: From 668bb1e0f685e941d8e966b425acc233bfa5d5ed Mon Sep 17 00:00:00 2001 From: thisisjun786 <259586770+thisisjun786@users.noreply.github.com> Date: Tue, 22 Sep 2026 04:00:24 +0900 Subject: [PATCH 13/15] Count what the records hold, and say which failures were measured 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. --- docs/plugin-packaging.md | 50 ++++++++++++------- docs/plugin-transition.md | 3 +- docs/runtime-install.md | 7 +-- .../crw-plan/references/integrations.md | 2 +- scripts/ci/tests/test_plugin_wiring.py | 7 +-- 5 files changed, 42 insertions(+), 27 deletions(-) diff --git a/docs/plugin-packaging.md b/docs/plugin-packaging.md index 5c86c1dbb..10524f6df 100644 --- a/docs/plugin-packaging.md +++ b/docs/plugin-packaging.md @@ -184,7 +184,8 @@ that layout. Bump `version` in the manifest when the change should reach installations, and install the plugin again: a cached version changes only on installation, and a task -already running keeps the package its session started with. +already running may still hold cache-bound references to the version its session started +with — the same directory the next install removes. ## The cache lifetime @@ -201,8 +202,8 @@ each declared surface is whether its reference outlives the directory it names. | Stop settings at `/crw-completion-hook.json` | No | Nothing | The same command | | Adapter, relay and bridge executables | No, they sit under the installer pointer | Nothing | `runtime_install.py install` | | Hook document path in the run identifier | Yes | Held as an identifier and never re-read | The host | -| MCP start `cwd` and `args` | Yes | A server already running survives, because it has already replaced itself with the installed bridge. A restart inside that session fails | The host | -| Skill reads | Yes | The read fails at the moment it is made | The host | +| MCP start `cwd` and `args` | Yes | A server already running survives, because it has already replaced itself with the installed bridge. A restart inside that session is expected to fail: inferred from the `cwd` the host holds, not measured here | The host | +| Skill reads | Yes | A read against the removed directory is expected to fail: inferred from where the host reads skills, not measured here | The host | Two of those this package can answer for and two it cannot, and the difference is a host rule rather than a preference. A hook command goes through a shell, so it can be written to resolve @@ -215,10 +216,13 @@ outside the version cache by anything this package declares. A 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. Replace the package while a -turn is open and the command names a file that no longer exists. `python3` exits **2** for a +task still holds that command and it names a file that no longer exists; nothing measured shows +a later turn of the same task resolving it afresh. `python3` exits **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 a real host: one removed directory, the same -error 74 times in one turn, and a task that could not finish until it was interrupted by hand. +to the model and fires Stop again. Measured on the user's host: one removed directory, eleven +repeated Stop prompts in a single turn of one task and eight in a single turn of a second, and +neither able to finish until interrupted by hand. An isolated reproduction of the same manoeuvre +produced thirty-seven in one turn. So the declaration names two candidates and opens the first one it can read: @@ -252,27 +256,34 @@ the version path, so an update that leaves the command alone keeps its trust. ### The supported range -| When the update lands | Stop | MCP restart | Skill reads | +| Task holding the old package reference | Stop | MCP restart | Skill reads | | --- | --- | --- | --- | -| Between turns | Safe: the next turn resolves everything afresh | Safe | Safe | -| During a turn | Safe: the second candidate answers | **Still fails** | **Still fails** | +| Existing task, including an idle interval between turns | The fallback is available if installed; a quiet turn does not establish package-reference reload | Cache-bound reference may be stale; not measured as safe | Cache-bound reference may be stale; not measured as safe | +| Existing task during a turn, removed cache | Fallback invocation measured after removal | Failure is inferred from the retained cache-bound reference, not independently measured here | Failure is inferred from the retained cache-bound reference, not independently measured here | +| Fresh task created after update | Invocation measured from the updated installation | Verify the new task's actual MCP call | Verify the new task's actual skill read | -The second row is the honest limit. This package cannot move those two references off the -cache, so an update during an open turn is avoided rather than survived. +The fallback protects the Stop launcher. It does not make every old package reference survive +replacement. A task can remain alive across many turns; an idle interval is not a session reload. ### Updating safely -1. Check that no turn is open. +1. Identify tasks that still hold the version being replaced. Finish and replace those tasks + through the supported new-task path, or establish a supported way to keep every referenced + path available continuously. Merely observing no active turn is insufficient. 2. Run `python3 scripts/runtime_install.py hook --adapter completion --owner plugin --apply` first, so the fallback is current before the directory it backs up can disappear. 3. Run `codex plugin add crw@`. 4. Re-trust the hook once if its command changed. -5. Keep the previous version directory until nothing references it, then release it. +5. Read back the installed payload and validate each required surface in a fresh task. + Preserve existing recovery evidence and compatibility paths until their readers are gone. -Preserving that directory is an operator step. `codex plugin add` removes it and this -repository does not own that command, so nothing here can hold it open. -`python3 scripts/plugin_transition.py swap-state` reports what a replacement actually left, -and that is what to read before releasing a preserved copy. +`codex plugin add` removes the prior version cache. This procedure does not promise to retain +that directory automatically or prescribe copying it back after a gap as uninterrupted support. +If path continuity cannot be established before replacement, use the finished-task boundary +in step 1; do not proceed on an assumption that the old cache will remain. +`python3 scripts/plugin_transition.py swap-state` reports what a replacement actually left. It +reads the pointer and the host records, not the tasks holding references, so it cannot establish +that the last reader is gone; that needs evidence of its own. A host carrying temporary compatibility files — an old cache path kept alive by hand after an update went wrong — needs a record of its own, kept with the task record outside this @@ -286,8 +297,9 @@ The cache keeps one version per plugin, and installing a new version replaces th previous directory instead of keeping both. Rolling back therefore means making the source offer the earlier revision again and reinstalling it, not selecting an older copy from the cache. Bump `version` in the manifest for a release; a new -task picks up the new package when its session starts, and work already running -keeps the version it started with. +task picks up the new package when its session starts, and work already running may still hold +cache-bound references to the version it started with, which is the directory the new install +removes. Removing the plugin deletes the cached version directory and the plugin entry in `config.toml`. It leaves the marketplace registration, so removing that is a diff --git a/docs/plugin-transition.md b/docs/plugin-transition.md index 4f4c2fd28..7d54d3a26 100644 --- a/docs/plugin-transition.md +++ b/docs/plugin-transition.md @@ -71,7 +71,8 @@ anything has been taken. It is here at all because this command writes the same plugin-owned settings `runtime_install.py hook --owner plugin` writes. A migration that stopped at the settings would leave the package's Stop declaration with one candidate again, and the first package replacement -during an open turn would land back in the loop that declaration exists to avoid. See +while a task still held the old command would land back in the loop that declaration exists to +avoid. See [the fallback launcher](runtime-install.md#the-fallback-launcher-and-why-this-command-places-it) for what it refuses and who removes it, and [the cache lifetime](plugin-packaging.md#the-cache-lifetime) for why a second candidate is needed at all. diff --git a/docs/runtime-install.md b/docs/runtime-install.md index 83cb5cc3b..eecfd815f 100644 --- a/docs/runtime-install.md +++ b/docs/runtime-install.md @@ -1400,9 +1400,10 @@ With `--owner plugin` this command also writes `crw-stop-hook.py` beside those s writes it **before** them. That file is the second candidate the package’s Stop declaration opens, and it exists because a hook command is fixed when a turn starts, with the plugin root already resolved into it. Installing a version removes the previous cache directory whole, so an -update landing mid-turn leaves that turn’s command naming a file that is gone, and `python3` -exits 2 for a missing script — the hook protocol’s blocking code. Measured on a real host: the -same missing-file error 74 times in one turn, and a task that could not finish. +update landing while a task still holds that command leaves it naming a file that is gone, and +`python3` exits 2 for a missing script — the hook protocol’s blocking code. Measured on the +user's host: eleven repeated Stop prompts in one turn of one task and eight in a turn of a +second, and neither able to finish. An isolated reproduction produced thirty-seven in one turn. [Plugin packaging](plugin-packaging.md#the-cache-lifetime) owns the full reference table and the supported range; what belongs here is who writes the file and what that writer refuses. diff --git a/plugins/crw/skills/crw-plan/references/integrations.md b/plugins/crw/skills/crw-plan/references/integrations.md index 51350bef2..c88f8429d 100644 --- a/plugins/crw/skills/crw-plan/references/integrations.md +++ b/plugins/crw/skills/crw-plan/references/integrations.md @@ -841,7 +841,7 @@ A delivery report answers two questions no status word answers: how far this cha Choose the stages from what happened rather than from the chain's full length. Walk the chain in order, naming each stage this delivery has evidence for and, where one exists, the first stage that is missing or was never observed, saying which of the two it is. A delivery with no gap has no such stage to name. The stages are independent: never infer a later one from an earlier one, and never drop an observed later stage because an earlier one is absent, since a commit that was never pushed can still be live through a link resolving to that checkout. A stage this change cannot have, such as an installation surface it never touches, is left out rather than reported as passing, and a stage the issue's own criteria require is always named, as unverified when nothing was observed. A report that lists every stage every time trains its reader to skip the one that matters. -Changing source, installing it, and refreshing an already-loaded conversation are three events, and the installation method decides what the third one costs. A linked installation resolves each skill through a symlink, so an edit is visible to the next read of that file, no reinstall is involved, and a conversation that already read the old text keeps it until the file is read again or a new task starts. A versioned plugin installation resolves a cached copy of a published version, so an edit reaches nobody until the version is bumped and installed again, and a session already running keeps the package it started with. Report the method actually observed and what the reader must do under it; where it was not checked, say so instead of assuming a linked installation. Reading a link's own target is what establishes which checkout an installed skill resolves to. +Changing source, installing it, and refreshing an already-loaded conversation are three events, and the installation method decides what the third one costs. A linked installation resolves each skill through a symlink, so an edit is visible to the next read of that file, no reinstall is involved, and a conversation that already read the old text keeps it until the file is read again or a new task starts. A versioned plugin installation resolves a cached copy of a published version, so an edit reaches nobody until the version is bumped and installed again, and a session already running may still hold cache-bound references to the version it started with, which is the copy the next install removes. Report the method actually observed and what the reader must do under it; where it was not checked, say so instead of assuming a linked installation. Reading a link's own target is what establishes which checkout an installed skill resolves to. Usable now is a claim about a representative user path, run through the installed entry point, with the time and environment of the most recent such run attached. Passing checks, a listed hook, an accepted delivery and a completed issue each establish only themselves. [OPS-6.1](../../crw-run/references/operations.md#ops-61-six-states-that-never-imply-one-another) already records which states never imply one another, and [OPS-11.3](../../crw-run/references/operations.md#ops-113-four-stages-that-are-not-one-event) separates a landed source change from what a host installs and executes. Use those meanings rather than restating them here. diff --git a/scripts/ci/tests/test_plugin_wiring.py b/scripts/ci/tests/test_plugin_wiring.py index 57a98a0d8..45bd5b985 100644 --- a/scripts/ci/tests/test_plugin_wiring.py +++ b/scripts/ci/tests/test_plugin_wiring.py @@ -1137,9 +1137,10 @@ class DeclaredStopCommandTest(unittest.TestCase): A plugin hook command is fixed when the turn starts, with the plugin root already resolved into it. Replacing the package removes that directory whole, and python3 exits 2 for a missing script -- the number the hook protocol reads as "block this turn". Measured on the - real incident: one removed directory, 74 repeated Stop prompts, and a turn that could not - end. So these cases drive the ACTUAL declared command through a shell, the way the host - runs it, rather than asserting anything about its text. + real incident: one removed directory, eleven repeated Stop prompts in one turn and eight in + a second task's turn, and neither able to end; an isolated reproduction of the same manoeuvre + produced thirty-seven in one turn. So these cases drive the ACTUAL declared command through + a shell, the way the host runs it, rather than asserting anything about its text. """ DECLARATION = ROOT / "plugins/crw/wiring/hooks/stop-recording-completion.json" From 0b36dfba4242b42e9dbe68478732e825b9bba8b0 Mon Sep 17 00:00:00 2001 From: thisisjun786 <259586770+thisisjun786@users.noreply.github.com> Date: Tue, 22 Sep 2026 04:23:36 +0900 Subject: [PATCH 14/15] Report how those turns actually ended, and drop the only-case claim 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. --- docs/plugin-packaging.md | 3 ++- docs/runtime-install.md | 3 ++- plugins/crw/wiring/crw_stop_hook.py | 5 +++-- scripts/ci/tests/test_plugin_wiring.py | 5 +++-- 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/docs/plugin-packaging.md b/docs/plugin-packaging.md index 10524f6df..f96136d42 100644 --- a/docs/plugin-packaging.md +++ b/docs/plugin-packaging.md @@ -221,7 +221,8 @@ a later turn of the same task resolving it afresh. `python3` exits **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: one removed directory, eleven repeated Stop prompts in a single turn of one task and eight in a single turn of a second, and -neither able to finish until interrupted by hand. An isolated reproduction of the same manoeuvre +neither turn able to finish while that path was absent. Both completed once a compatibility path +was restored there, at 06:35:13Z and 06:35:59Z. An isolated reproduction of the same manoeuvre produced thirty-seven in one turn. So the declaration names two candidates and opens the first one it can read: diff --git a/docs/runtime-install.md b/docs/runtime-install.md index eecfd815f..edf628568 100644 --- a/docs/runtime-install.md +++ b/docs/runtime-install.md @@ -1403,7 +1403,8 @@ already resolved into it. Installing a version removes the previous cache direct update landing while a task still holds that command leaves it naming a file that is gone, and `python3` exits 2 for a missing script — the hook protocol’s blocking code. Measured on the user's host: eleven repeated Stop prompts in one turn of one task and eight in a turn of a -second, and neither able to finish. An isolated reproduction produced thirty-seven in one turn. +second, and neither turn able to finish until a compatibility path was restored. An isolated +reproduction produced thirty-seven in one turn. [Plugin packaging](plugin-packaging.md#the-cache-lifetime) owns the full reference table and the supported range; what belongs here is who writes the file and what that writer refuses. diff --git a/plugins/crw/wiring/crw_stop_hook.py b/plugins/crw/wiring/crw_stop_hook.py index d6b40e40d..8b26b7b9f 100644 --- a/plugins/crw/wiring/crw_stop_hook.py +++ b/plugins/crw/wiring/crw_stop_hook.py @@ -16,8 +16,9 @@ It is not reached directly. The declaration runs a fixed bootstrap that opens the first of two candidates it can read: this file under the version cache, and a copy the runtime installer places at /crw-stop-hook.py. The cache copy comes first so the current version -always wins and a stale copy can never outrank it; the installed copy exists for the one case -the cache cannot answer, which is a plugin update landing in the middle of a turn. The turn's +always wins and a stale copy can never outrank it; the installed copy exists for what the cache +cannot answer, which is a task still holding a command into a version that has been replaced, +with nothing measured showing a later turn of that task resolving it afresh. The turn's hook command is fixed when the turn starts, so a cache directory removed underneath it leaves an absolute path to a file that is gone, and python3 exits 2 for a missing script -- the same number the hook protocol reads as "block this turn". That collision is what turned one missing diff --git a/scripts/ci/tests/test_plugin_wiring.py b/scripts/ci/tests/test_plugin_wiring.py index 45bd5b985..1eec9e780 100644 --- a/scripts/ci/tests/test_plugin_wiring.py +++ b/scripts/ci/tests/test_plugin_wiring.py @@ -1138,8 +1138,9 @@ class DeclaredStopCommandTest(unittest.TestCase): into it. Replacing the package removes that directory whole, and python3 exits 2 for a missing script -- the number the hook protocol reads as "block this turn". Measured on the real incident: one removed directory, eleven repeated Stop prompts in one turn and eight in - a second task's turn, and neither able to end; an isolated reproduction of the same manoeuvre - produced thirty-seven in one turn. So these cases drive the ACTUAL declared command through + a second task's turn, and neither turn able to end until a compatibility path was restored; + an isolated reproduction of the same manoeuvre produced thirty-seven in one turn. So these + cases drive the ACTUAL declared command through a shell, the way the host runs it, rather than asserting anything about its text. """ From 351f8cf2cd96bde00413c03edc169d58b2dd482e Mon Sep 17 00:00:00 2001 From: thisisjun786 <259586770+thisisjun786@users.noreply.github.com> Date: Tue, 22 Sep 2026 05:15:02 +0900 Subject: [PATCH 15/15] Stop asking for a command that cannot exist 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. --- docs/plugin-packaging.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/plugin-packaging.md b/docs/plugin-packaging.md index f96136d42..907c69c1d 100644 --- a/docs/plugin-packaging.md +++ b/docs/plugin-packaging.md @@ -288,9 +288,10 @@ that the last reader is gone; that needs evidence of its own. A host carrying temporary compatibility files — an old cache path kept alive by hand after an update went wrong — needs a record of its own, kept with the task record outside this -repository. Record the path, who made it, why, the condition under which it may be removed, and -the command that shows whether anything still references it. Such a file is a repair, not a -guarantee that the next update will be survivable. +repository. Record the path, who made it, why, and the condition under which it may be removed. +Nothing here enumerates the tasks still holding a reference, so the evidence that the last reader +is gone has to come from the host; record which observation was used. Such a file is a repair, not +a guarantee that the next update will be survivable. ## Update and roll back