15 KiB
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 commitsd2d5104(code and tests),ba463fcand94c1d8c(run log, trunk notes), diffed against9564c22. Also checked: the fixer's re-check flags, the new--delta-noteandtrain --supersedessurfaces, 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-2415and:2573._NOTE_REFandlisted_notesare gone.delta_tokensis now chars/4 of three parts:- the delta header;
- the
--delta-notefiles whose mtime is after the base'strained_at(new_to_base,:2400); - the body of a
## Changes since basesection (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:3195intest_due_reasons_each_alone).
- Test strength. On
9564c22,:3274("named only: nothing") fails, and so does:3195. Thetrained_atmutant fails at:3299. - Fixer flags:
- Whole-second
trained_at: acceptable._isotruncates (isoformat(timespec="seconds"),forks.py:221), sotrained_atis 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 basesection: acceptable as Minor. It is a brief-writing rule, and it is routed to the Phase 3 text (run log:486,:514).
- Whole-second
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_passrecords it on the entry (:3070), so the result, the ledger andusage --phase Nall carry it. - Test strength.
test_forks.py:1926checks the fallbacks, the jsonkinds.ping, the mdpingand total rows andPriced total, and that--phase 1has no rows. On9564c22it errors (ping_phasedoes 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
rebasealways gets--phaseis 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
oktraining, each named base that isactivebecomessupersededwithsuperseded_byandsuperseded_at(:1598-1604). A failed training changes nothing.
- Unknown names and the base's own name are refused with exit 2, before and under the lock (
- Test.
test_forks.py:3321covers the refusals, the failed training, the ok training, the ping decisionstatus:supersededand 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.
initrefuses an effective margin ≥ the effectivettl_swith exit 2 and writes nothing (:2184-2187). This includes a re-run that lowersttl_sunder the current margin.base_warmthcaps a configured margin at the base's TTL/2 (:1357). The review's probe (TTL 300, margin 600) now readswarmwithmargin_s150 (test_forks.py:1702), and the smoke copy shows 3500 → 1800.
- Fixer flag (the smoke's
--warm-margin-s 3500acts 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
initflag 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 witherror: usage, as before. test_forks.py:724shows a--warm-margin-s-only re-run withchanged_fields == ["warm_margin_s"], and launch fields, thresholds and run-wide knobs kept.
- Every
- Fixer flags:
--toolsalone keeps the oldallowed_tools: acceptable as Minor. Note that this is a behaviour change: before the fix, a re-run with--toolsre-derivedallowed_tools. It now matchestrain(review Minor 12), and "pass--allowed-toolswith--tools" is in the Phase 3 list.- Cannot clear
warm_margin_sor the agent back to none: acceptable. "Never re-runinitto reset" is in the Phase 3 list. - Exit 2 on a concurrent config change (
:2208-2209): consistent. Every refusal in the CLI is aForksErrorwith the defaultEXIT_USAGE(2), and there is no separate "busy" code. Onlyinitwritesreg["config"](:2228is the only writer), so the check fires only on a concurrentinit, 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.
usagetakesstatus,flagsandreasonfromresults/<job>.jsonwhen they differ from the ledger line, keeps the ledger's tokens and cost, and lists the job in the newstatus_from_result(:3686-3704,:3721). - Why the comparison is sound. The ledger line and the result are built from the same
status,flagsandreason(: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 ascannot read brief; ''givesempty path;- a directory gives
cannot read delta noteafter about 2 s ofPermissionErrorretries.
- a missing file gives
- Notes are read before the lock and before the job entry (
:2746,:2877). continuedoes not take the flag, so argparse giveserror: usage, exit 2.
- All refusals exit 2 with one JSON line, before any write:
train --supersedes.- The refusal payload
{"error": "unknown base", "bases": [...], "option"}matchesexpire's (:3481). forkandrebaseusebaseplusbases= 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;
listshows it.
- The refusal payload
New Minors in the fix surfaces
- N1. A
#line inside a fenced code block ends the## Changes since basesection (forks.py:2367,:2377).- Probe: a section holding a
bashfence with# commentyields only the text up to the comment ('A\n```bash'). - Failure: a large delta written as shell snippets under-counts, so
delta_tokensstays 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.
- Probe: a section holding a
- 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
initre-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
whereoutput 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, andchanged_fieldsshows nothing. If the old file is gone, the re-run exits 2 withclaude binary not foundand the old path, which is loud. - This is consistent with "omitted keeps", and the Phase 3 text forbids re-running
initanyway. It should be recorded with the other Minor 8 flags.
- The
- 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 B1ris running, andtrain --base B3 --supersedes B1ends ok first. When the rebase then ends ok:- B1r is created
activeand gets pings; - B1's
superseded_byis overwritten to B1r.
- B1r is created
- The cost is bounded by the ping cap. It needs two overlapping lineage operations, so it is rare.
- Fix:
_rebase_succeededcreates the new base assupersededwhen the old one is no longer active, or the Phase 3 text says not to re-train while a rebase runs.
- Scenario:
- N5. Two new refusal paths are untested:
init's "the config changed while init ran" (:2208-2209), andtrain --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_s150). 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, andstate:warmat 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 sameexpiringstate, 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 basesection, alongside--delta-note(the Athanor built both options); --delta-noteonrebase, which carries the notes into the rebase prompt;- the
### Attached delta notesprompt section; delta_noteson job entries;supersedeson the training base;status_from_resultin theusagejson;init's concurrent-change refusal;- the change to a first
initwithout--model, which is now aForksError("usage")from the helper rather than from argparse (same shape, same exit).
Outside scope (on record)
test_refusalsflaked once under full-suite load (test_forks.py:2870,killok: 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 thatkillcan reportok: falseunder load (the Phase 3 text already keys onokandstatus).