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. diff --git a/docs/plugin-packaging.md b/docs/plugin-packaging.md index 7c9c52e24..907c69c1d 100644 --- a/docs/plugin-packaging.md +++ b/docs/plugin-packaging.md @@ -184,8 +184,114 @@ 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 + +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 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 +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 +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 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 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: + +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. + +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. + +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. + +### The supported range + +| Task holding the old package reference | Stop | MCP restart | Skill reads | +| --- | --- | --- | --- | +| 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 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. 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. 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. + +`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 +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 @@ -193,8 +299,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 026ec482c..7d54d3a26 100644 --- a/docs/plugin-transition.md +++ b/docs/plugin-transition.md @@ -52,17 +52,34 @@ 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 +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. + +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 +101,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..edf628568 100644 --- a/docs/runtime-install.md +++ b/docs/runtime-install.md @@ -1393,6 +1393,47 @@ 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 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 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. + +| 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/plugins/crw/skills/crw-plan/references/integrations.md b/plugins/crw/skills/crw-plan/references/integrations.md index 7567ed147..a2c1ed7ca 100644 --- a/plugins/crw/skills/crw-plan/references/integrations.md +++ b/plugins/crw/skills/crw-plan/references/integrations.md @@ -881,7 +881,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/plugins/crw/wiring/crw_stop_hook.py b/plugins/crw/wiring/crw_stop_hook.py index ed63874c9..8b26b7b9f 100644 --- a/plugins/crw/wiring/crw_stop_hook.py +++ b/plugins/crw/wiring/crw_stop_hook.py @@ -13,6 +13,17 @@ 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 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 +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 +45,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 @@ -43,10 +66,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(): @@ -78,6 +102,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..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 \"${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_transition.py b/scripts/ci/tests/test_plugin_transition.py index 13d156b93..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 @@ -324,6 +337,95 @@ 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_a_held_launcher_lock_stops_the_sequence_before_the_settings(self): + """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. + + 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")) + 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()) + 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"]} + self.assertEqual(outcomes.get("stable launcher install"), "busy", + 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()) + 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.""" + 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") + 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"]} + 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.untouched(), untouched_before) + def test_the_retired_settings_and_record_are_kept_not_deleted(self): host = self.ready() host.transition("--apply") @@ -1185,7 +1287,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) @@ -2519,8 +2627,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 +2668,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 +2795,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 +3007,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..1eec9e780 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,577 @@ 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, eleven repeated Stop prompts in one turn and eight in + 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. + """ + + 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_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_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_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. + """ + 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): + 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_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_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) + 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) + # 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_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([]) + 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 2d0e8a37f..b6a593c2c 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 @@ -104,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 @@ -320,6 +335,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" @@ -480,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 " @@ -1314,6 +1361,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..3e9c4e2a4 100644 --- a/scripts/crw_transition/steps.py +++ b/scripts/crw_transition/steps.py @@ -29,20 +29,27 @@ 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 +# 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) -# 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(): @@ -662,12 +669,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 +782,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() \ @@ -1439,6 +1477,36 @@ 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. + + 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 + 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 ( + 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") @@ -1844,7 +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), +# 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)) @@ -2350,18 +2425,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") @@ -2388,10 +2478,84 @@ 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"]: + # 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") + 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/plugin_transition.py b/scripts/plugin_transition.py index c670eed44..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 @@ -249,7 +261,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 d260dfb69..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: @@ -2368,6 +2378,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 +2408,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 +2424,59 @@ 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) + # 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 + 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": [], + "registrations": [], "launcher": placement, "launcherObserved": observed, + "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.")}) - return EXIT_OK + " have fired stay three separate claims. launcherObserved is what" + " 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 event = args.event or SESSION_START @@ -2491,7 +2571,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