Files
reliquary/docs/workpad/reports/2026-10-16-19-gremlins-phase3-digest-check.md

126 lines
14 KiB
Markdown
Raw Permalink Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# Pre-build check: Phase 3 contract digest (`spire:gremlins`)
- **Checker model id**: `claude-opus-5-5[1m]`. Fresh: did not write the digest and will not build or review Phase 3.
- **Date**: 2026-10-16
- **Checked**: `docs/workpad/plans/2026-10-16-18-gremlins-phase3-contract.md` at `5092ed8`, against the goal on `main` (`docs/workpad/goals/headless-forks.md`), build plan §4 and §6 Phase 3, the Phase 2 re-check (N1–N5, the flake), the run log's routed items, the Phase 1 review and re-check, and `plugins/spire/skills/forks/forks.py` / `tests/test_forks.py` as they are now. No tests were run and no live calls were made.
- **Verdict**: 1 BLOCKER, 12 ADJUST, the rest OK. The interface list is complete against the code. The rename map has no gap that breaks state. The gate tests, C16 and the release wording need changes before the build.
## BLOCKER
**B1. GateTests cannot work on the Windows host as specified (§4 "Tests").** They patch `gremlins.PLATFORM` to a `PosixPlatform`. Every path then goes through `norm_path` → `PLATFORM.norm_path` → `PosixPlatform`, and §3.4/C8 makes that refuse drive forms.
- `where --binary C:\…\claude-stub.cmd` refuses in `resolve_binary` (`forks.py:1004`), and so does `script_path()` (`:1067`). Today `cmd_where` does not catch either (`:2006`, `:2023`).
- `init --run C:\…` refuses in `state_dir_for` (`:340`) before any gate check.
- What happens:
- The override half ("`where` exits 0", "`init` proceeds") fails on Windows.
- The closed `init` case ("exits 2 and creates no state dir") passes for the wrong reason, the path refusal. So it is a test that cannot fail on the gate.
- The builder has to improvise the fix, which is how earlier digests produced Important findings.
- **Fix**: the fake platform is a `WindowsPlatform` subclass, or `PLATFORM` with `supported` and `describe` patched. It has `supported = False`, `is_wsl() = True` and `describe()` = `{name: "posix", sys_platform: "linux", wsl: true, …}`, so path handling stays Windows. `init` asserts `error == "platform not supported"`. The gate check is the first thing in `init`, before `state_dir_for` and any `norm_path`. Keep the pure `PosixPlatform().supported is False` unit tests as written.
## ADJUST
**A1. The gate leaves launches open when a registry exists (§4, C11; §7.2 intake).** "Every other command needs `init`" is only true for a fresh run. The state dir is committed at phase close (§7.2 commit ownership: everything except `raw/` and locks). §7.2 intake says: if the run log has a `Gremlins:` line, run `list` and adopt, without `where`. On Linux, a successor Athanor adopting a run that was started on Windows would then run `spawn`, `rebase`, `continue` and `train`. `wait` and `tick` would launch pings. All of this happens on a closed platform. The sessions are not on that host, so the jobs fail, but each failure costs a launch, and pings keep retrying (Phase 2 Minor 3).
- **Fix**:
- Refuse on a closed gate in the dispatcher for `train`, `spawn`, `continue`, `rebase` and `tick`, in the same place as the N3 refusal.
- The tick inside `wait` and the supervisor launches no pings on a closed gate.
- The intake runs `where` first, then adopts.
- Add one GateTests case: a registry present, gate closed, `spawn` exits 2.
**A2. C16 as written hides real survivors (§5.3).** The rule is "a snapshot pid that is gone from the table or now carries another name is dead". It fails in three cases:
- (i) `process_table()` returns `[]`. On macOS that is always (no `/proc`); on Windows it happens when Toolhelp fails (`:750-752`). Every snapshot pid then counts as "gone", so `kill` reports `survivors: []`, `ok: true`.
- (ii) The snapshot name is `None`. The root's name is `names.get(root)` (`:805`), so a root missing from the table gets `None`, and `None` never matches a name.
- (iii) On POSIX, `comm` changes on `exec`, for example an `sh` wrapper that execs python or claude. A live process then reads "renamed".
The defect that is actually observed is narrower: access denied makes `pid_alive` return True without a creation-time check (`:719-721`).
- **Fix**: drop a snapshot pid only if one of these holds:
- the table is non-empty and lacks the pid;
- the snapshot name is known, the table name differs, and `proc_ctime(pid)` is `None` (the unverifiable case, i.e. class (a)).
In every other case `record_alive` decides, as today.
- Read the table through the module-level `process_table` (`ProcessTreeTests` patch module names).
- Tests: an empty table keeps the survivor; a snapshot name `None` keeps it; a name differing while the ctime is verifiable leaves the decision to `record_alive`.
**A3. The third N3 test in §5.1 contradicts itself.** "Stored file deleted, env unset, PATH without claude → stored path kept with the 'could not re-resolve' warning (then `build_launch` refuses…)". `build_init_config` calls `build_launch` → `binary_info`, which raises `claude binary not found` (`:2162`, `:1025-1026`). The warning can never appear in the output.
- **Fix**: split it into two tests.
- Stored file present, env unset, PATH without claude: path kept, warning shown, exit 0.
- Stored file deleted: exit 2, `claude binary not found` (today's loud failure).
**A4. C14 can switch installs silently.** If the stored `binary_resolved_by` is `CLAUDE_CODE_EXECPATH` and a re-run comes from a shell without it, `resolve_binary(None)` falls through to the PATH `claude` (`:1005-1010`). That can be another install, with another version or auth. A warning is given, but it still crosses sources.
- **Fix**: re-resolve only through the stored source (EXECPATH from EXECPATH, PATH from PATH). If that source gives nothing, keep the stored path and warn.
**A5. Path matrix row 3 (`C:`, `C:x`) contradicts "as today" (§3.4).** On today's Windows branch, `ntpath.join(base, "C:x")` returns `C:x`. Its rest does not start with `\`, so the result is `ForksError("cannot normalise path")` (`:322-332`). `C:` goes the same way. Also, "assert absolute with drive `C:`" cannot be pure string logic on a POSIX host (WSL runs the matrix), because `ntpath` has no per-drive cwd there.
- **Fix**: row 3 on Windows is **refused**, as today.
**A6. The C18 wording fails G-text 1 (§7.2 vs §9.2.1).** §7.2 proposes "A fresh agent reads **the code the phase touches** and writes the trunk notes". G-text 1 requires that phrase to be absent from `athanor.md`.
- **Fix**: reword, for example "a fresh agent reads the code the blocks change and writes the trunk notes".
**A7. "Windows-only in 0.3.0" contradicts the goal's "Amended … release gating".** 0.3.0 is merged and released only after the Linux and macOS Igors lift their gates, so the 0.3.0 that ships is not Windows-only. The phrase is in four places:
- the SKILL.md description (§7.1);
- the README row (§9.1);
- the gate reason (§4, "spire:gremlins 0.3.0 runs on Windows only");
- the release commit body (§9.4).
Porters can change the first three. They cannot change a commit body without rewriting a pushed history.
- **Fix**: use version-free wording.
- Text: "Windows-tested; other platforms fail closed until tested (see Platforms)".
- Gate reason: "spire:gremlins is not tested on <sys_platform><, WSL> yet; …".
- Changelog: "tested on Windows; Linux and macOS gated pending the platform handoff".
- Handoff §8.8 lists every place the porter updates.
**A8. Three routed text items are missing from §7.7.**
- (a) `preamble.md:9` points gremlins at `plugins/spire/skills/implementation/references/run-log.md`. The Phase 1 review ("Two extra preamble lines" row) says this does not resolve in other projects, because the plugin lives in the cache. Phase 3 should give the format inline or by an absolute path. §2.6 says "keep the other rules", so this would ship broken.
- **Fix**: inline the handoff-note format in `preamble.md`. Or have the helper put the absolute path into the templates (it knows `script_path()`). The preamble hash changes in 3.1 anyway.
- (b) `stalled` together with `clock_jump` is not a hung gremlin (Phase 1 re-check, M3 builder flag).
- (c) The remedy depends on the reason. Only `model_refused` points at `availableModels`. `usage_limit`, `overloaded`, `api_error` and `prompt_too_long` are not a settings problem (Phase 1 review, the 429 → `model_refused` finding). SKILL §6 lists the reasons but no action for each.
**A9. Vacuous negative tests if the rename misses them (§2.7).** `test_forks.py:155`, `:934`, `:1011` and `:1079` assert `assertFalse(os.path.exists(… "-forks"))`. After the rename the state dir is `-gremlins`, so any line left unchanged can never fail. The residue grep shows them, but nothing in §2.8 says they are wrong.
- **Fix**: name these lines in §2.7 and derive the path from `gremlins.state_dir_for(run)`, not a literal suffix.
**A10. The §2.8 residue categories miss legitimate hits outside the skill.** These are:
- `igor/SKILL.md:66` (the D4 follow-up, out of scope);
- `igor/references/mindset.md:11` and `igor/references/harness-notes.md:3`;
- `implementation/references/superpowers.md:36` (repository forks).
- **Fix**: add a category "Igor text (D4) and non-session meanings: leave untouched", so 3.1 neither edits these lines nor reports them as residue.
**A11. A closed-gate `where` does not always "still fill everything else" (§4).** `cmd_where` (`:2006`) does not catch a path refusal from `resolve_binary`. On WSL with a Windows `CLAUDE_CODE_EXECPATH` (via `WSLENV`), or with `--binary C:\…`, §3.4 now refuses. The output is then exit 2 with a path error, without the gate reason and without the report the porters need.
- **Fix**: catch `GremlinsError` from `resolve_binary` into `binary.error` and `reasons`.
**A12. `platform.override` is undefined while the env is set on a supported platform.** C13 sets the env for every test, so the question comes up in every test. §4 says that with the override, `where` reports `override: true`.
- **Fix**:
- Define `override = env set and not PLATFORM.supported`. Windows `CliTests` then assert `override: false` even with the env set.
- `PosixPlatform.describe()` reads `self.supported` instead of a literal `False`, so lifting the gate is one change (§4 "Lifting the gate").
- `platform_gate()` reads `PLATFORM.describe()`, never `sys.platform`, so it passes the §3.7 seam grep.
## OK, with notes
- **Interface (§3.2)**: complete against the code. These are every OS-specific site in `forks.py`, and §3.1–§3.3 places each one:
- `IS_WINDOWS`/`SPAWN_METHOD`/`_NOWIN` `:47-52`;
- cygpath `:276-296`;
- the lock restore `:500-503`;
- the ctypes block `:630-664`;
- `proc_ctime`/`epoch_to_ctime`/`pid_alive`/`process_table`/`terminate` `:667-853`;
- `_taskkill`/`kill_tree` `:856-895`;
- `_run` `:901`;
- `_powershell`/`spawn_supervisor` `:1421-1460`;
- `where` `:2028-2030`;
- `Supervisor.run` `:3223-3224`.
The `transcript_path` slug and the scrub lists are neutral code that was only observed on Windows. They belong in the handoff, where §8 puts them. Do not name any local variable `platform`: the module imports `platform` (`:2003`).
- **Rename map**: no gap that breaks state. No `-forks` state dir is tracked in git, and nothing outside the skill references `skills/forks`. The base key `"forks"` has three writers (`:1637`, `:2716`, `:2782`) plus `LIST_BASE_KEYS` `:3431`; all are covered by §2.4. `run_for_state` `:2935` is covered by §2.1. The preamble text change alters the `append_system_prompt` hash (a defining field). That is harmless, because no base from before the rename is reused (C6).
- **C1–C7, C9, C10, C19, C20**: OK. For C20, see the A7 wording.
- **C8**: OK. The POSIX refusal of a backslash is a behaviour change, and the handoff (§8.4) already lists it.
- **C11**: OK in itself. It is not enough on its own: see A1.
- **C12**: OK. Naming: `SPIRE_GREMLINS_UNSUPPORTED_OK` has a plural prefix, `SPIRE_GREMLIN_CHILD` and `SPIRE_GREMLIN_DETACH_*` have a singular one. Pick one before 3.2 writes them.
- **C13**: OK. It also means that a green suite on Linux says nothing about the gate; only `GateTests` cover it. The handoff should say so.
- **C15**: OK.
- `rebase` refuses a base that is not active, at launch and again under the lock (`:2873`, `:2893`).
- Only one rebase per base runs at a time (`:2901`).
- So "not active at finalise" can only mean "changed during the run".
- `_conclude` runs inside `finalize_job`'s lock with the same registry read as `_rebase_succeeded` (`:1656-1665`, `:1607`). Computing `parent_inactive` there is therefore consistent.
- Set the flag only when NEW is actually created, not on the `rebase_conflict` early return (`:1622-1624`).
- **C16 (b)**: raising `KILL_VERIFY_S` to 30 s is fine for `kill`. `expire` kills its jobs one after another, though, so several stuck jobs × 30 s can pass the Bash tool's default 2 min. The tool then backgrounds the call (P0.1(d)). The SKILL text should give `kill`/`expire` a long Bash `timeout`.
- **C17**: acceptable. The stub tests exercise the real CIM detach after the rename and the seam refactor. The release commit is not a merge. Phase 4 must smoke the renamed helper before anything ships.
- **C18**: a reasonable reading of "the 0.2.2 shape" against "the Athanor does not read code". Wording: see A6.
- **Push**: the goal says the branch "gets pushed". The digest says no push, which follows Igor's brief (precedence 1). Pushing is a post-build user step.
- **The two flagged risks** (an `!` injection exiting 2 on a closed platform; installing uv in WSL only through the permission prompt): correctly flagged, nothing to add.
- **The flake plan (§5.3)**: the bounded investigation and "never two suites at once" are right. Only the C16 rule needs A2.