154 lines
15 KiB
Markdown
154 lines
15 KiB
Markdown
# 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/<job>.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`).
|