dirigence/reliquary
1
0
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases Wiki Activity

📝 Headless forks Phase 2 review

Browse Source
This commit is contained in:
g4borg
2026-10-16 17:57:28 +02:00
parent 3b1de6cc54
commit 9564c22b34
1 changed files with 230 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/workpad/reports/2026-10-16-18-headless-forks-phase2-review.md
+230
View File
@@ -0,0 +1,230 @@
# Phase 2 review: headless forks (bookkeeping)
- **Reviewer model id**: `claude-opus-5-5[1m]`. Fresh: did not plan, build or fix this phase.
- **Date**: 2026-10-16 17:55 +0200 (`date`)
- **Scope**: Phase 2, commits `571a3d6..3b1de6c` on top of Phase 1 (`60fdea0`). Code `fff43a0` (2.1), `5fda2e2` (2.2), `4b92b51` (2.3); the rest is docs. Inputs: the build plan §4 and §6 Phase 2, the plan §4 and §7 Phase 2, design §5–§6, the Phase 2 contract digest, the run log's Phase 2 sections, and the Phase 1 re-check.
- **Verdict**: the phase works as built and its smoke numbers check out against the raw streams. There are **0 Critical, 2 Important and 14 Minor** findings. Both Important findings are contract gaps that the Athanor-written digest left open: what `delta_tokens` counts, and which phase a ping belongs to.
## Gates I ran
| Gate | Result |
|---|---|
| G-unit `uv run plugins/spire/skills/forks/tests/test_forks.py` (run once) | exit 0, `Ran 130 tests in 130.444s`, `OK`, no skips. One `ResourceWarning` (unclosed `BufferedReader`, from `test_tick_runs_in_wait_and_supervisor`, `test_forks.py:2963`) |
| G-where `uv run plugins/spire/skills/forks/forks.py where` | exit 0. stdout is one line holding one JSON object: `ok: true`, `spawn: cim-hidden`, binary 2.1.299 via `CLAUDE_CODE_EXECPATH`, `warnings: []` |
| Smoke figures against the raw streams | Script over `…\headless-forks-p0\smoke2\…-forks\`. Checked for all 7 jobs: ledger `tokens` = result `tokens` = the raw `result.usage`. `cost_delta_usd` = raw `total_cost_usd` − entry `cost_before`. First-event `cache_read` against `base_context` gives the stated verdicts. Phase sums are 1: $0.309559 and 2: $0.145025. Pings (phase `null`) come to $0.013911. Ledger total $0.468495. The run log's figures match. |
| `delta_tokens` probe | `listed_notes` on a brief that names the build plan, the run log and `plugins/spire/agents/athanor.md` (I1) |
| Leftover processes (17:54, by command line: `forks-test-`, `_supervise`, `poscontrol`) | none |
| Commit hygiene | `4b92b51..3b1de6c` touches only the run log, so no code changed after the phase-end G-unit run. There are no attribution trailers in `60fdea0..3b1de6c`. |
I made no live `claude` calls.
## Important
### I1. `delta_tokens` counts every `.md` file a brief names, and no rebase can clear it
`forks.py:2302-2321` (`_NOTE_REF`, `listed_notes`) and `:2484-2486` (`due_flags`).
- **What it counts.** "Notes listed in the brief" is implemented as every token ending in `.md` that resolves to an existing file, first against the base's cwd and then against the state dir. That includes the build plan, the run log, the design, and files the fork is told to **edit**.
- **Probe.** A brief that names `docs/workpad/plans/2026-10-16-02-headless-forks-build.md`, the run log and `plugins/spire/agents/athanor.md` gives 124,721 chars, which is `delta_tokens:31180` against a threshold of 12,000. The run log alone is 84,641 chars, or 21k tokens.
- **Failure scenario.** The Athanor's briefs normally cite the build plan and the run log. Every fork then returns `rebase_due: true`. The reason does not depend on the base, so after `rebase --as B2` the next fork with the same kind of brief reports it again. The procedure obeys flags ("flags, not judgment"; design §5 says to rebase before forking), so each fork is preceded by a rebase. After four of them, `retrain_due: rebases:4` forces a re-train.
- **Fix direction.**
- Count only handoff notes: files under the state dir's `notes/`, or notes whose mtime is after the base's `trained_at`. Never count files the brief tells the fork to edit.
- Pin the rule in the Phase 3 text.
- Add a test where a brief names a large non-note document and the flag stays off.
- **Digest.** The digest repeats the design's phrase "notes listed in the brief" without defining it. The builder flagged the reading (2.2).
### I2. Pings have `phase: null`, so no phase table ever shows them
`forks.py:2951-2958` (the ping entry carries no phase or block) and `:3573-3575` (`usage --phase` filters on equality).
- **What the contract says.** Design §6 has the phase close carry `usage --phase <n> --format md` with "per-kind totals (train, rebase, fork, continue, ping)". Plan 2.3's done-when asks for "per-kind totals … and a priced total".
- **What happens.** A ping row can never appear in a phase table, and the phase's priced total leaves out keep-warm cost.
- **Smoke evidence.**
- Phase 1 is $0.3096 and phase 2 is $0.1450, together $0.4546.
- The ledger holds $0.4685.
- The $0.0139 of the two pings appears in no phase table. Only the whole-run `usage` shows it.
- On a 250k base each ping is about 0.1 × prefix, so a multi-hour phase under-reports by several pings.
- **Fix direction.** A ping takes its base's phase: `base.phase`, or else the highest phase of the base's forks. A simpler alternative: `--phase N` also includes pings on bases whose phase is N.
- **Digest.** The digest leaves the ping's phase open. The builder flagged this (2.1 "block/phase null", 2.4 first flag).
## Minor
1. **A re-train never supersedes the old base** (`cmd_train`, `forks.py:2588-2627`). Plan §4.3's state machine says `active ─► superseded (rebase / re-train)`, but nothing in the helper sets `superseded` on a re-train.
- The abandoned base stays `active` and can be forked without `--force`.
- The ping rule's activity condition is run-wide (`run_active`, `:2864-2878`), so the forks of the new base keep pinging the old one, up to `break_even` pings.
- The cost is bounded at about 1.9 × the old prefix, which is one cold start.
- Fix: `train --supersedes <base>`, or have the Phase 3 text `expire` the old base.
- To the brief's question: **superseded** bases are never pinged (status gate, `:2893`), and a ping already in flight when a base is superseded ends normally and is not followed by another.
2. **`--warm-margin-s` is not checked against the TTL** (`:1355-1356`, `:1881`). When margin ≥ TTL, a base is `expiring` right after every hit. A ping then fires on every tick, every 30 s from `wait`, until the cap.
- Probe: a base with inferred TTL 300 and `warm_margin_s` 600 reads `expiring` 1 s after its hit.
- This happens without anyone choosing it when a margin tuned for 1 h meets a 5-minute downgrade.
- The cap bounds the waste at about 1.9 × prefix per real call.
- Fix: cap the margin below the TTL, for example at `ttl/2`.
3. **A failed or missed ping is retried on the next tick with no backoff** (`:1525-1528`, `:2881-2907`).
- Example: a usage-limit 429. Each ping clears `ping_in_flight`, leaves `last_hit` alone, and the next tick launches again: a CIM spawn plus `claude` every 30–60 s through the expiring window, up to `break_even`.
- Counting failed pings toward the cap is what bounds this. That is good, but a short backoff after `ping_missed` would be better.
4. **A `partial` ping that re-wrote most of the prefix carries no flag** (`:1518-1528`). A ping that read only the shared system prefix (P0.6's case) is really a cold write of the base. `cold_start` is set for fork, continue and rebase only, so `usage`'s `cold_starts` misses it. The next fork then shows a clean `hit`.
5. **A `continue`'s `cold_start` follows the base's warmth, which a continue never refreshes (I3)** (`:1518-1520`, `:2738`).
- Scenario: a continue reads its fork's own warm prefix and gets `hit`, but the base went cold meanwhile (pings off, or a 5 m TTL). The continue is still flagged `cold_start` and counted in `usage`.
- The rule dates from Phase 1. It now feeds the Phase 2 totals.
6. **The supervisor's synchronous tick can break M3's bound on the loop body** (`:3171-3173` against `SupClock.tick`, `:3010-3019`).
- The Phase 1 re-check accepted `CLOCK_JUMP_S` = 60 s because the loop body stays under about 45 s.
- A tick now adds a `claude --version` (60 s timeout), a lock wait (up to 45 s), the CIM spawn (120 s timeout) and a second lock.
- A slow tick then reads as a host suspend:
- a spurious `clock_jump` flag;
- the gap is excluded from `max_minutes`;
- the orphan watchdog is re-armed.
- It is unlikely, but cheap to avoid: run the tick in a thread, or credit tick time back to the clock.
7. **The hand-back seam with pings** (`:3243-3245`, `:3252-3253`, `:3337-3340`).
- `wait --all` excludes pings, and `wait <ping>` exits 2, so no command waits on a ping.
- `list.running` includes pings, though.
- The last `wait --all` before hand-back ticks at its start. It can launch a ping and still return 0, and `list` then shows a running job. The procedure's "nothing running at hand-back" and plan 4.2's trial criterion would read that as a leak.
- The Phase 3 text must say how to treat pings there (ignore them: they end within `PING_MAX_S`), or `list` should report them separately.
8. **`init` after bases exist** (`:2146-2177`). The question was whether it can silently change a live base's fingerprint. **No.**
- Every base launches from its own `launch` record (M10).
- A record-less base from before 2.2 is backfilled from the old config before the config is replaced.
- `fingerprint` hashes are never compared, only recorded.
- The concern is elsewhere: every `init` flag has a default, so a partial re-run (as the smoke's `init --warm-margin-s 3500`) silently reverts any omitted run-wide knob: `pings`, `parallel_cap`, thresholds, `ttl_s`, `warm_margin_s`, `orphan_after`. The exit is 0, with the reverts listed in `changed_fields`.
- This is a refusal downgraded by the digest, not by the build plan. It is acceptable for launch fields, but the Phase 3 text should forbid re-running `init` (A7 already says so for successors). Alternatively, `init` could keep omitted values.
9. **A test that cannot fail for its stated reason.** `UsageTests.test_r1_waits_for_the_registry_lock` (`test_forks.py:2187-2202`) asserts that `usage` blocks on a held lock. But `cmd_usage` takes the lock in `touch_orchestrator` (`forks.py:3565`) before `reconcile_ledger`, so a reconcile that took no lock would pass too.
- Fix: call `reconcile_ledger` directly, as `test_r1_nothing_missing_takes_no_lock` does, or set `SPIRE_FORKS_CHILD` for the call.
10. **The `ResourceWarning`, confirmed** (handoff flag). `Supervisor.run` opens `claude` with `stdout=PIPE` (`:3103-3105`) and never closes `p.stdout` after `reader.join` (`:3181-3185`).
- In the detached supervisor this is harmless: the process exits right after.
- In-process it leaks a handle, which is what the test run shows.
- Fix: close `p.stdout` after the join when the reader has ended.
11. **R1 can leave the ledger and the results disagreeing (no double count).** Scenario: a result file exists but its registry entry still reads `running` (a kill between the result write and the registry write). `reconcile_ledger` (`:3410-3444`) appends its line from that result. A later `reap_lost` or `kill` then finalises the entry again (`:1638-1662`), overwrites the result with `lost`/`killed`, and the ledger append is skipped.
- The ledger keeps the first status. Tokens and cost come from the same stream, so totals do not drift.
- **Idempotency itself holds**:
- the fast path takes no lock;
- the slow path re-reads the ledger under the lock;
- `ledger_append` dedupes on job id;
- a second `usage` adds nothing (tested).
12. **`train --tools` does not re-derive `allowed_tools`** (`:2570-2573`; the test codifies it at `test_forks.py:3120`). `init` derives `allowed = tools`. Under `dontAsk`, a tool added by a `train --tools` override is therefore denied, and the result reads `needs_attention`.
13. **Two sequential forks can run at once under `parallel_cap` 2.** Their `head_at_end` ranges then overlap, so each Commit cell can list the other's commits (`:3516-3525`). The builder flagged it. The Phase 3 text should run sequential forks one at a time.
14. **Cosmetic or untested.**
- `list` computes warmth for superseded and retired bases. The smoke's superseded B1 shows `expiring` (`:3326`).
- Untested: a ping launched by a supervisor's own tick, end to end (the env and interpreter path read correct); `defining:` for fields other than `model`; `rebase_conflict`. The last one is unreachable in practice: `train` refuses a pending name and `rebase` refuses an existing one.
- The M8 desktop-wide diff fails if the user opens a console during the ~1 s sample.
## The brief's hunt list
- **TTL inference.**
- `infer_ttl` gives 3600 when 1 h tokens were written, 300 when only 5 m tokens were written, and otherwise keeps the previous value, falling back to the config. It runs after every call with a `result`, ping and continue included.
- The deviation from the digest (a read-only call keeps 300 instead of reverting to 3600) is right: reverting would show a 5-minute base as warm for an hour. It recovers on the next 1 h write.
- **Does a ping refresh the prefix it claims to?** Yes.
- Its argv is the base's record plus `--resume <base> --fork-session --session-id <new> --max-turns 1`. That request reads the base prefix.
- Smoke: `j0006` read 25,277 of B2's 25,279. `j0007` read 25,719, which includes j0006's identical suffix.
- `last_hit` is set to the ping's first event at finalise, only for an `ok` ping with `hit` or `partial`.
- Caveat: Minor 4.
- **The cap math.**
- `break_even` is computed in integer hundredths: (200−10)//10 = 19 and (125−10)//10 = 11. Correct, and tested.
- Pings since the last real call are counted by job number. Every status counts, and the count is per base. A new base created by a rebase counts its pings from creation, because the rebase job belongs to the old base.
- **A ping storm from concurrent tickers.** Not possible.
- The decision, the job entry and `ping_in_flight` are written in one locked write.
- The unlocked first look is redone under the lock.
- The finaliser clears the mark and sets `last_hit` in one write.
- `test_two_tickers_one_ping` forces both tickers past the unlocked look with a barrier, so it would fail without the locked re-check.
- What remains is retry without backoff (Minor 3) and margin ≥ TTL (Minor 2).
- **Cost leaks from pings on superseded bases.** None, because of the status gate. The leak is through bases a re-train abandons (Minor 1).
- **Rebase, old/new switch.**
- The new base is created only when the job is `ok`, in `_rebase_succeeded` (`:1599-1636`).
- On `failed`, `killed`, `timeout` or `lost`, B stays `active` and NEW never exists. This is tested for `is_error` and kill.
- Smoke: the new base's `cost_cum_usd` is the rebase's cumulative total, $0.2435012, and that is exactly `cost_before` of fork `j0005`.
- **`last_hit` on a failed rebase.** It is refreshed from the rebase's first assistant event, both live (`bump_last_hit`) and at finalise. That is correct: the first request read B's prefix whatever happened afterwards. An error before any assistant event refreshes nothing.
- **`init` after bases exist.** It cannot change a live base's fingerprint (Minor 8).
- **R1.** Idempotent, and no double count (Minor 11 for the one inconsistency).
- **Usage numbers.** They match the ledger, the result files and the raw `result` events for all 7 smoke calls. The md totals equal the sums.
- **M13 `cygpath`.**
- Correct: drive forms skip it, and `/tmp/x` goes through `cygpath -w`.
- With no `cygpath`, or a failing one, the command exits 2 naming the path, and nothing is written on the cwd's drive (tested end to end through `init --run /tmp/…`).
- `//server` paths are untouched.
- In contexts without Git Bash on PATH, such as a supervisor or PowerShell, rooted paths are refused rather than guessed. That is the right failure.
## Weakened checks and tests that cannot fail
I went through every assertion line the Phase 2 diff removes from `test_forks.py`:
- `/tmp/x → G:\tmp\x` was removed. M13 forbids that mapping, and two new tests cover the input.
- `test_changed_values_after_base_refused` became `…_allowed`. This is the digest's specified change; the test still checks that the base keeps its record and that identical values stay byte-equal.
- `test_phase2_not_implemented` was removed: the commands are implemented, and `test_commands_need_init` gains them.
- The window assertions moved into `test_no_visible_window`, which is wider and has a positive control. The `assertGreater(len(all_windows), 0)` became a skip, as M8 specified.
- The `running[]` key `turns` became `messages` (M16).
- In `test_train_fork_continue_end_to_end` the note is now written after the launch (M11), with its assertions unchanged.
Nothing was loosened. The billed-error test (`test_error_after_requested_model_billed`) fails if the result reads `model_refused`. M8's positive control proves the detector sees a console window.
There is one test that cannot fail for its stated reason (Minor 9).
## Handoff flags, one by one
| # | Block | Flag | Answer |
|---|---|---|---|
| 1 | digest | Athanor-written. Check it against the build plan | It is consistent with the build plan except in three places. (a) It puts `rebase` in `parallel_cap` (`CAPPED_KINDS`, `forks.py:159`), while plan §4.3's invariant names fork/continue. That is stricter and harmless, but the run log says "none against the build plan". (b) It leaves the ping's phase open (I2). (c) It leaves "notes listed in the brief" undefined (I1). Replacing `init`'s refusal is the digest's own decision, not the coordinator's (Minor 8). The coordinator items R1, M6, M7, M8, M10, M11, M13, M16 and the billed-error test are all implemented as decided. |
| 2 | 2.1 | A read-only call keeps the previous TTL even when it is 300 | Accepted (see the hunt list) |
| 3 | 2.1 | `messages` also in `wait` `running[]` and `list` running rows | Accepted. The Phase 3 text must use `messages` there and `turns` in results |
| 4 | 2.1 | Pings use the base's record | Accepted. 2.2 finished M10 for every launch |
| 5 | 2.1 | `wait` ticks at its start | Accepted. Note Minor 7: the last `wait` before hand-back can launch a ping |
| 6 | 2.1 | Naming a ping job in `wait` exits 2 | Accepted as such, but nothing can wait on a ping (Minor 7) |
| 7 | 2.1 | Activity = `last_wait_at` or a fork/continue in the last 600 s; a hand `tick` is not activity | Matches design §5. Activity is run-wide (Minor 1) |
| 8 | 2.1 | Pings counted by job number, every status counts | Accepted. Counting failures is what bounds Minor 3 |
| 9 | 2.1 | `break_even` uses the 1 h ratio above 300 s | Accepted |
| 10 | 2.1 | Ping wall limit 300 s; block/phase null | The wall limit is fine. Phase null is **I2** |
| 11 | 2.1 | Binary check before the lock; a missing binary skips the base | Accepted. Tested (`binary_missing` why, no job) |
| 12 | 2.1 | The supervisor's tick pauses its loop 1–3 s | The worst case is much longer and breaks M3's bound (Minor 6) |
| 13 | 2.1 | No N3 check inside `tick_pass` | Accepted. `tick` is refused in a fork, a fork's `wait` skips the tick, and the supervisor's ticks are the design's own |
| 14 | 2.1 | A supervisor ticks first after 60 s | Accepted. `wait` covers short jobs |
| 15 | 2.1 | A ping's `last_hit` is set at ping end | Accepted. `ping_in_flight` covers the gap. The timestamp is the first event, not the end |
| 16 | 2.1 | TTL is inferred from failed calls too | Accepted: billed tokens are real writes |
| 17 | 2.1 | M8 positive control is minimised; the desktop diff can flake | Accepted, Minor 14 |
| 18 | 2.1 | `ResourceWarning` | Confirmed, Minor 10 |
| 19 | 2.1 | Untested: a ping from a supervisor's own tick | Still untested (Minor 14). By reading, the env (child env, scrubbed) and interpreter path are right |
| 20 | 2.2 | The new base records the binary the rebase ran under | Correct, tested (`b4` against the current stub) |
| 21 | 2.2 | `rebase` returns train's shape plus `parent` | Accepted |
| 22 | 2.2 | `phase_boundary` follows the digest's "or" literally | Accepted. Repeating until a rebase fits design §5's "the first fork of a new phase comes from a rebased base" |
| 23 | 2.2 | A rebased base's phase is `--phase`, not inherited | Accepted. Without `--phase` the base's own side is unknown, so the Phase 3 text should always pass `--phase` to `rebase` |
| 24 | 2.2 | `delta_tokens` counts the `*.md` paths in the brief | **I1** |
| 25 | 2.2 | `lines` includes uncommitted changes | Accepted (design: `git diff --shortstat`) |
| 26 | 2.2 | "Last call" for `runtime_drift` | Accepted. A rebase clears it: the new base's reference is the rebase's own init |
| 27 | 2.2 | The `defining:` field set (effort and allow/deny lists are not defining) | Matches design §5's list |
| 28 | 2.2 | Transcript read from the tail; only a synthetic test | Accepted. It falls back to the stored context |
| 29 | 2.2 | `list` computes flags for active bases only | Accepted. The warmth is still shown for others (Minor 14) |
| 30 | 2.2 | `train` re-validates the whole record | Accepted. `--tools` without `allowed_tools` is Minor 12. There is still no way to clear the config's agent |
| 31 | 2.2 | A rebase's first event refreshes the old base's `last_hit` even if the rebase fails | Correct (see the hunt list) |
| 32 | 2.2 | NEW's training cost is the rebase's cost delta; its forks' basis is the rebase's cumulative total | Verified in the smoke (`j0005.cost_before` = 0.2435012) |
| 33 | 2.2 | Rebases are not activity | Accepted. A `wait` loop over a rebase is activity anyway |
| 34 | 2.2 | M11 has 2 s slack | Accepted |
| 35 | 2.2 | M7 hashes a constant empty server set | Accepted. The helper writes that file itself |
| 36 | 2.2 | M13: paths in a brief that cygpath cannot convert are skipped | Accepted |
| 37 | 2.2 | `rebase_conflict` untested | Unreachable in practice (Minor 14) |
| 38 | 2.2 | Untested `defining:` fields, `train --cwd/--agent/--binary`, pre-2.2 bases | Still untested. The code paths are shared with the tested `model` case |
| 39 | 2.3 | The whole-run md uses the phase layout | Accepted |
| 40 | 2.3 | Killed/lost calls count as calls with null sums | Accepted. Consistent between json and md |
| 41 | 2.3 | The md phase table omits zero-call kinds | Accepted. The json always has all five kinds |
| 42 | 2.3 | R1 also reconciles a result whose registry entry still reads `running` | No double count, but the ledger can keep a status the result no longer has (Minor 11) |
| 43 | 2.3 | `usage` stamps the orchestrator call (not from a fork) | Accepted |
| 44 | 2.3 | `head_at_end` can include another fork's commit, so Commit is shown for sequential forks only | Two sequential forks can still overlap (Minor 13) |
| 45 | 2.3 | Commit lists 7-character hashes | Accepted |
| 46 | 2.4 | Pings are left out of `usage --phase` | **I2** |
| 47 | 2.4 | The superseded B1 shows `expiring` | Cosmetic, Minor 14. No ping went to it (status gate) |
| 48 | 2.4 | The partial-ping refresh was not reproduced live | Accepted. The unit test covers it (`test_ping_end_to_end_through_wait`) |
| 49 | 2.4 | The smoke used `init --warm-margin-s` instead of a 54-minute wait | Accepted for the smoke. The tunable needs a guard (Minor 2) |
## Built but not asked for (on record only)
- `init --warm-margin-s` and the config key `warm_margin_s`. The digest added it for the smoke; it is not in the build plan.
- The flag `ping_missed`. `list` base rows gain `ttl_s`, `margin_s`, `ttl_from` and `ping_in_flight`, and bases store `ping_in_flight_at`.
- `wait` refuses ping job ids and ticks at its start. `wait --all` excludes pings.
- `messages` in `wait` `running[]` and in `list` running rows. This changes the Phase 1 digest's shape.
- `rebase` additions:
- the options `--max-minutes` and `--max-budget-usd`;
- the refusals "the base is being rebased", and on `train`, "a running rebase creates this base";
- `rebase_conflict`;
- `parent` in the return.
- `init` output gains `bases`, and `init` backfills record-less bases.
- Job-entry fields `created_ts`, `head_at_end`, `runtime_drift`, `rebase_as` and `new_launch`.
- `usage` json additions:
- `reconciled`, `cache_hits`, `cache_verdicts` and `scope`;
- ledger lines marked `"reconciled": true`;
- `--format md` written as UTF-8 bytes.
- Test hooks `CLAUDE_PROJECTS_DIR`, `TRANSCRIPT_TAIL_BYTES` and `find_cygpath`.
- Stub directives `billed`, `ttl=5m`, `nover=` and `skill=`, and the stub now honours `--max-turns`.