From 7351c403be95c589eb0a514af773696017aefe6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabor=20K=C3=B6rber?= Date: Fri, 16 Oct 2026 18:22:27 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=93=9D=20Headless=20forks=20Phase=202=20r?= =?UTF-8?q?e-check?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ...-10-16-19-headless-forks-phase2-recheck.md | 153 ++++++++++++++++++ 1 file changed, 153 insertions(+) create mode 100644 docs/workpad/reports/2026-10-16-19-headless-forks-phase2-recheck.md diff --git a/docs/workpad/reports/2026-10-16-19-headless-forks-phase2-recheck.md b/docs/workpad/reports/2026-10-16-19-headless-forks-phase2-recheck.md new file mode 100644 index 0000000..f29e895 --- /dev/null +++ b/docs/workpad/reports/2026-10-16-19-headless-forks-phase2-recheck.md @@ -0,0 +1,153 @@ +# Phase 2 re-check: headless forks (fix pass) + +- **Reviewer model id**: `claude-opus-5-5[1m]`. Fresh: did not plan, build, review or fix this phase. +- **Date**: 2026-10-16 18:21 +0200 (`date`) +- **Scope**: only the findings the fix pass took on from `reports/2026-10-16-18-headless-forks-phase2-review.md`: I1, I2 and Minors 1, 2, 8, 9, 10, 11. Fix commits `d2d5104` (code and tests), `ba463fc` and `94c1d8c` (run log, trunk notes), diffed against `9564c22`. Also checked: the fixer's re-check flags, the new `--delta-note` and `train --supersedes` surfaces, and the changed tests for weakened assertions. +- **Verdict**: all eight are **resolved**. **No Critical or Important finding survives.** The fixer's flags are acceptable as Minor. I found five new Minors in the new surfaces (N1–N5 below). None blocks Phase 3. Two of them need a line in the Phase 3 text. + +## Gates I ran + +| Gate | Result | +|---|---| +| G-unit `uv run plugins/spire/skills/forks/tests/test_forks.py` (run once, 18:15–18:18) | exit 1. `Ran 137 tests in 150.191s`, `FAILED (failures=1)`. The failure is `LaunchTests.test_refusals`, `test_forks.py:2870`: `kill` returned `ok: false`, which means a process from the snapshot was still alive after `KILL_VERIFY_S`. Neither that test nor `kill_job`/`kill_tree` (`forks.py:861-895`, `:1756-1786`) is in the fix diff. Run alone, the test passed 3 of 3 times (3.8 s each). I count it as a load flake from Phase 1 code, outside this re-check (see "Outside scope"). There was no `ResourceWarning` line anywhere in the log. | +| G-where `uv run plugins/spire/skills/forks/forks.py where` | exit 0. One JSON line: `ok: true`, `spawn: cim-hidden`, binary 2.1.299 via `CLAUDE_CODE_EXECPATH`, `warnings: []` | +| New and changed tests against the unfixed helper | I ran the 13 touched tests in a scratch copy: `forks.py`, `preamble.md` and `templates/` from `9564c22`, plus the HEAD test file. Result: `failures=9, errors=3`. Each fails at the assertion for its finding (details below). `test_tick_runs_in_wait_and_supervisor` passes there but prints the `ResourceWarning` (Minor 10). `test_ping_end_to_end_through_wait` passes there too, as expected: it only moved its setup to a margin the new rule accepts. | +| Mutants of the fixed helper | I made three mutants of the fixed helper and ran the matching test against each. All three were killed: (1) `reconcile_ledger` without the lock fails `test_r1_waits_for_the_registry_lock` (`[['j0010']] != []`). (2) `new_to_base` ignoring `trained_at` fails `test_delta_tokens_counts_only_what_the_base_does_not_know` at `:3299`. (3) Superseding a named base of any status fails `test_train_supersedes` at `:3351` (`'superseded' != 'failed'`). | +| Smoke state, copied to scratch, through the fixed helper | `usage` exits 0, `status_from_result: []` (no false positive on real data), `reconciled: []`, total $0.468495, unchanged. `usage --phase 2 --format md` gives the same $0.1450 as before. The two old pings keep `phase: null`, as flagged. `list`: B1 `superseded`, B2 `active`, `margin_s` 1800, which is the smoke's 3500 capped at 3600/2. No reasons. | +| Leftover processes (18:20, by command line: `forks-test-`, `_supervise`, `poscontrol`, `claude-stub`) | none, apart from my own query | +| Commit hygiene | Code changed only in `d2d5104`; `ba463fc` and `94c1d8c` touch only the run log and trunk notes. No attribution trailers in `9564c22..HEAD`. | + +I made no live `claude` calls. + +## Findings, one by one + +### I1. `delta_tokens` counts every `.md` file a brief names: **resolved** + +- **What was done.** `forks.py:2360-2415` and `:2573`. `_NOTE_REF` and `listed_notes` are gone. `delta_tokens` is now chars/4 of three parts: + - the delta header; + - the `--delta-note` files whose mtime is after the base's `trained_at` (`new_to_base`, `:2400`); + - the body of a `## Changes since base` section (`changes_section`, `:2370`). +- **The review's failure scenario is gone.** A brief that names the build plan, the run log and files to edit adds nothing. Because the mtime gate compares against `trained_at`, a rebase clears the reason. The review's three-part fix direction is met: + - count only real deltas; + - the rule is routed to the Phase 3 text (successor boot, run log `:514`); + - a test where a large named document leaves the flag off (`test_forks.py:3258`, and `:3195` in `test_due_reasons_each_alone`). +- **Test strength.** On `9564c22`, `:3274` ("named only: nothing") fails, and so does `:3195`. The `trained_at` mutant fails at `:3299`. +- **Fixer flags**: + - *Whole-second `trained_at`*: acceptable. `_iso` truncates (`isoformat(timespec="seconds")`, `forks.py:221`), so `trained_at` is never later than the true end. A note written in the same second, before the end, counts as new. That is the safe side: at worst it raises a spurious advisory flag. + - *Reused `## Changes since base` section*: acceptable as Minor. It is a brief-writing rule, and it is routed to the Phase 3 text (run log `:486`, `:514`). + +### I2. Pings have `phase: null`: **resolved** + +- **What was done.** `ping_phase` (`forks.py:2959`) takes the phase of the latest train, fork, continue or rebase job on the base that has one. If none has one, it takes the base's own phase, else null. `tick_pass` records it on the entry (`:3070`), so the result, the ledger and `usage --phase N` all carry it. +- **Test strength.** `test_forks.py:1926` checks the fallbacks, the json `kinds.ping`, the md `ping` and total rows and `Priced total`, and that `--phase 1` has no rows. On `9564c22` it errors (`ping_phase` does not exist). The md and json assertions would also fail if the phase were dropped at finalise. +- **Fixer flag** (old pings keep null; a rebased base takes its forks' or its own phase): acceptable. Only the smoke's two pings are affected, as confirmed on the copy. The rule that `rebase` always gets `--phase` is already routed to the Phase 3 text. + +### Minor 1. A re-train never supersedes the old base: **resolved** + +- **What was done.** `train --supersedes BASE` (repeatable, `:1917`). + - Unknown names and the base's own name are refused with exit 2, before and under the lock (`:2686-2709`). + - On an `ok` training, each named base that is `active` becomes `superseded` with `superseded_by` and `superseded_at` (`:1598-1604`). A failed training changes nothing. +- **Test.** `test_forks.py:3321` covers the refusals, the failed training, the ok training, the ping decision `status:superseded` and fork-needs-`--force`. The status-gate mutant is killed. +- One new edge case is listed as N4. + +### Minor 2. `--warm-margin-s` is not checked against the TTL: **resolved** + +- **What was done.** + - `init` refuses an effective margin ≥ the effective `ttl_s` with exit 2 and writes nothing (`:2184-2187`). This includes a re-run that lowers `ttl_s` under the current margin. + - `base_warmth` caps a configured margin at the base's TTL/2 (`:1357`). The review's probe (TTL 300, margin 600) now reads `warm` with `margin_s` 150 (`test_forks.py:1702`), and the smoke copy shows 3500 → 1800. +- **Fixer flag** (the smoke's `--warm-margin-s 3500` acts as 1800, at most 1800 at 1 h): acceptable. It is in the successor-boot list. + +### Minor 8. `init` re-run reverts omitted settings: **resolved** + +- **What was done.** + - Every `init` flag defaults to None. On a re-run, an omitted flag keeps its current value (`build_init_config`, `:2121-2188`). + - The first init still needs `--model`; without it the exit is 2 with `error: usage`, as before. + - `test_forks.py:724` shows a `--warm-margin-s`-only re-run with `changed_fields == ["warm_margin_s"]`, and launch fields, thresholds and run-wide knobs kept. +- **Fixer flags**: + - *`--tools` alone keeps the old `allowed_tools`*: acceptable as Minor. Note that this is a **behaviour change**: before the fix, a re-run with `--tools` re-derived `allowed_tools`. It now matches `train` (review Minor 12), and "pass `--allowed-tools` with `--tools`" is in the Phase 3 list. + - *Cannot clear `warm_margin_s` or the agent back to none*: acceptable. "Never re-run `init` to reset" is in the Phase 3 list. + - *Exit 2 on a concurrent config change* (`:2208-2209`): consistent. Every refusal in the CLI is a `ForksError` with the default `EXIT_USAGE` (2), and there is no separate "busy" code. Only `init` writes `reg["config"]` (`:2228` is the only writer), so the check fires only on a concurrent `init`, not on a supervisor's registry write. This path is untested (N5). +- One unflagged effect of the same change is listed as N3. + +### Minor 9. A test that cannot fail for its stated reason: **resolved** + +`test_forks.py:2310` now calls `reconcile_ledger` directly in a thread. It asserts that the line is absent while the lock is held, and present exactly once after. My lock-free mutant fails it. Stronger than before. + +### Minor 10. The `ResourceWarning`: **resolved** + +`Supervisor.run` closes `p.stdout` after `reader.join` when the reader has ended (`:3301-3303`). My full run printed no warning line; the unfixed helper prints it for the same test. The pipe is left open when the reader outlives `READER_JOIN_S` (fixer flag). That is acceptable: the detached supervisor exits right after. + +### Minor 11. R1 ledger and results can disagree: **resolved** + +- **What was done.** The ledger stays append-only. `usage` takes `status`, `flags` and `reason` from `results/.json` when they differ from the ledger line, keeps the ledger's tokens and cost, and lists the job in the new `status_from_result` (`:3686-3704`, `:3721`). +- **Why the comparison is sound.** The ledger line and the result are built from the same `status`, `flags` and `reason` (`:1538-1556`), so a normal job never differs. The smoke copy confirms `[]`. +- **Test.** `test_forks.py:2330`. +- **Fixer flag** (the md block table does not show status): acceptable. + +## The new surfaces + +- **`fork`/`rebase --delta-note PATH`.** + - All refusals exit 2 with one JSON line, before any write: + - a missing file gives `cannot read delta note`, the same as `cannot read brief`; + - `''` gives `empty path`; + - a directory gives `cannot read delta note` after about 2 s of `PermissionError` retries. + - Notes are read before the lock and before the job entry (`:2746`, `:2877`). + - `continue` does not take the flag, so argparse gives `error: usage`, exit 2. +- **`train --supersedes`.** + - The refusal payload `{"error": "unknown base", "bases": [...], "option"}` matches `expire`'s (`:3481`). + - `fork` and `rebase` use `base` plus `bases` = the known list, so the same key means a different list there. Cosmetic. + - The train return and its result do not say which bases were superseded; `list` shows it. + +## New Minors in the fix surfaces + +- **N1. A `#` line inside a fenced code block ends the `## Changes since base` section** (`forks.py:2367`, `:2377`). + - Probe: a section holding a `bash` fence with `# comment` yields only the text up to the comment (`'A\n```bash'`). + - Failure: a large delta written as shell snippets under-counts, so `delta_tokens` stays off. The flag is advisory and the brief still reaches the fork, so only the rebase advice is lost. + - Fix: skip fenced blocks when looking for the next heading, or say in the Phase 3 text that the section must not contain level-1/2-looking lines. +- **N2. The mtime rule is a proxy for "the base knows it"** (`:2400-2404`). + - A note written before a rebase or training ended, but not given to it (not attached and not in its brief), counts as known. Example: a parallel fork's note lands during a rebase. + - Failure: an under-count of the advisory flag only. The note is still put into the prompt in full. + - Acceptable. The Phase 3 text should attach to a rebase every note written since the base's `trained_at`. +- **N3. An `init` re-run no longer re-resolves the binary** (`:2155-2156`). Without `--binary`, a re-run keeps the stored path, where it used to follow `$CLAUDE_CODE_EXECPATH`. + - The `where` output shows a versioned path (`…\claude-code\2.1.299\…\claude.exe`). + - Failure: after a Claude Code update that leaves the old version on disk, a re-run (for example to change `--pings`) silently keeps 2.1.299 for new trains, and `changed_fields` shows nothing. If the old file is gone, the re-run exits 2 with `claude binary not found` and the old path, which is loud. + - This is consistent with "omitted keeps", and the Phase 3 text forbids re-running `init` anyway. It should be recorded with the other Minor 8 flags. +- **N4. A rebase already running on a base that a re-train supersedes still creates an active child** (`_rebase_succeeded`, `:1612-1643`, which does not check the old base's status). + - Scenario: `rebase --base B1 --as B1r` is running, and `train --base B3 --supersedes B1` ends ok first. When the rebase then ends ok: + - B1r is created `active` and gets pings; + - B1's `superseded_by` is overwritten to B1r. + - The cost is bounded by the ping cap. It needs two overlapping lineage operations, so it is rare. + - Fix: `_rebase_succeeded` creates the new base as `superseded` when the old one is no longer active, or the Phase 3 text says not to re-train while a rebase runs. +- **N5. Two new refusal paths are untested**: `init`'s "the config changed while init ran" (`:2208-2209`), and `train --supersedes`'s re-check under the lock (`:2709`, effectively unreachable because bases are never deleted). By reading, both are correct. + +## Weakened checks and tests that cannot fail + +I went through every assertion line the fix diff removes or changes in `test_forks.py`: + +- `test_boundaries_1h_and_5m`: the 5-minute case with margin 900 changed from "expiring from the hit" to the capped schedule (warm at 0 and 149 s, expiring at 150 and 299 s, cold at 300 s, `margin_s` 150). This is the specified change and it has more points than before. The 1 h cases are unchanged. +- `test_ping_rule_states_status_and_pings_off`: "pings at T0+100" became "pings at T0+1800, and `state:warm` at T0+1799". Stricter: it now pins the boundary from both sides. +- `test_ping_end_to_end_through_wait`: the margin went from 3600 to 1800 and the last hit from 100 s to 2000 s back. The base is in the same `expiring` state, and every assertion is unchanged. +- `test_due_reasons_each_alone`: "a brief naming the big note is due" became "not due". The due case moved to `--delta-note`, and its three assertions are unchanged. This is the I1 change. +- `test_r1_waits_for_the_registry_lock`: stronger (Minor 9). + +Nothing was loosened. Every new test fails on `9564c22`, or against a targeted mutant. + +## Built but not asked for (on record only) + +These came from the fix pass beyond the review's literal fix directions: + +- the `## Changes since base` section, alongside `--delta-note` (the Athanor built both options); +- `--delta-note` on `rebase`, which carries the notes into the rebase prompt; +- the `### Attached delta notes` prompt section; +- `delta_notes` on job entries; +- `supersedes` on the training base; +- `status_from_result` in the `usage` json; +- `init`'s concurrent-change refusal; +- the change to a first `init` without `--model`, which is now a `ForksError("usage")` from the helper rather than from argparse (same shape, same exit). + +## Outside scope (on record) + +- `test_refusals` flaked once under full-suite load (`test_forks.py:2870`, `kill` `ok: false`), and passed 3 of 3 times alone. + - The code involved is Phase 1 code that the fix did not touch. + - It is the first failure of this test in the recorded runs: the Phase 2 close, the review and the fix pass were all green. + - If it recurs, find out which snapshot pid survives `KILL_VERIFY_S`. For the product, it is a reminder that `kill` can report `ok: false` under load (the Phase 3 text already keys on `ok` and `status`).