159 lines
16 KiB
Markdown
159 lines
16 KiB
Markdown
# Gremlins helper: code verdict
|
||
|
||
- Date: 2026-10-16
|
||
- Reviewer model: claude-opus-5-5
|
||
- Timebox: 25 minutes, done by one agent with no sub-agents
|
||
- Target: `.claude/worktrees/headless-forks/plugins/spire/skills/gremlins/` on branch `headless-forks` at `ab1d945`
|
||
- `gremlins.py`: 4,155 lines
|
||
- `tests/test_gremlins.py`: 4,235 lines, about 180 tests
|
||
- `SKILL.md`: 120 lines
|
||
- `preamble.md` and three templates: 69 lines in total
|
||
|
||
All line references below point to `gremlins.py` unless another file is named.
|
||
|
||
## 1. Verdict
|
||
|
||
The code is careful, consistent and well tested, and it works: the live happy path ran cleanly, with a cache hit on the fork. It is still over-built by about four to five times. Roughly two thirds of the lines run and police **detached** processes: a PowerShell/CIM detach, heartbeats, reaping of lost jobs, an orphan watchdog, clock-jump detection, a tree kill that guards against pid reuse, and breaking of stale locks. The rest is mostly a layer of heuristic rules (pings, rebase and re-train thresholds, fingerprint drift) that the requirement never asked for.
|
||
|
||
**Rewrite it small, keeping the core.** Do not throw it away. Keep the stream parsing, the usage and cache accounting, the env scrub, the argv builder and the templates. Replace the supervisor stack with one foreground call that the orchestrator puts in the background through the Bash tool.
|
||
|
||
## 2. What it contains
|
||
|
||
| Part | Lines | Rough size |
|
||
|---|---|---|
|
||
| Constants and tunables (about 60 named constants, many "so tests can shorten them") | 1–178 | 180 |
|
||
| Small helpers: emit, clock, ISO times, sha, fingerprint hash | 180–265 | 85 |
|
||
| Platform seam: `Platform` base, `WindowsPlatform` (ctypes, CIM detach, Toolhelp32, taskkill, EnumWindows), `PosixPlatform` (gated off) | 267–734 | 470 |
|
||
| Platform gate and state-dir layout | 737–822 | 85 |
|
||
| Atomic IO with retries, `RegistryLock` (breaks stale locks), ledger append | 824–1060 | 235 |
|
||
| Process records (pid + ctime), `process_tree`, `kill_tree`, `snapshot_survivor`, git | 1062–1228 | 170 |
|
||
| Env scrub, binary resolution, `where` sub-reports, model admission | 1230–1452 | 225 |
|
||
| `StreamState`, runtime fingerprint, `usage_from_result`, API-error mapping, cache verdict, TTL, warmth | 1454–1691 | 240 |
|
||
| Job finalising: `_conclude`, `_rebase_succeeded`, `finalize_job`, orchestrator stamps, `reap_lost`, `kill_job` | 1693–2107 | 415 |
|
||
| argparse CLI | 2109–2313 | 205 |
|
||
| `where` and `init` (`build_launch`, binary re-resolution, config diff) | 2315–2598 | 285 |
|
||
| Launch helpers: brief, binary check, argv, delta header | 2599–2700 | 100 |
|
||
| Rebase and re-train heuristics: git delta counts, `## Changes` parsing, transcript tail read, drift checks, `due_flags` | 2700–2957 | 260 |
|
||
| `train`, `spawn`, `continue`, `rebase` commands | 2959–3291 | 335 |
|
||
| Keep-warm pings and `tick` | 3293–3476 | 185 |
|
||
| `SupClock` and `Supervisor` (the detached `_supervise` process) | 3478–3705 | 230 |
|
||
| `wait`, `list`, `kill`, `expire` | 3707–3863 | 158 |
|
||
| `usage` (ledger reconcile, aggregate, markdown tables) | 3865–4096 | 230 |
|
||
| Dispatch and main | 4097–4155 | 60 |
|
||
|
||
The code makes about 100 references to review-finding IDs (`M7`, `N4`, `C16`, `I2`, `A19`, `Minor 13` and others; 49 distinct) and about 30 to "design N", "plan N" or "digest §N". The code reads like layers of review fixes, and those IDs mean nothing without the old review documents.
|
||
|
||
## 3. Keep (as it is, or nearly)
|
||
|
||
- **`StreamState`** (1459–1522). This is a small, correct stream-json reader: init event, first and last usage, result, and the model-restriction warning.
|
||
- **`usage_from_result`** (1573–1601). It computes tokens with the 1h/5m split, `models_used` from the `modelUsage` delta, and the cost delta against the session's cumulative total. This is the real know-how for usage tracking on forks.
|
||
- **`cache_verdict`, `infer_ttl`, `wrote_only_5m`, `base_warmth`** (1640–1690). This is exactly the "is the base's cache still warm" requirement, in about 50 lines.
|
||
- **`api_error_reason`** (1604–1637). It is a table-driven mapping of failure reasons and is already shaped well.
|
||
- **The `SCRUB_*` constants and `scrubbed_env`** (89–114, 1235–1282). They hold the empirically gathered list of Desktop and SDK env vars that must not leak into a child `claude -p`. This is hard-won knowledge.
|
||
- **`claude_argv`** (2652–2666) and the fork argv: `--resume <base> --fork-session --session-id <uuid>`.
|
||
- **Small helpers:** `atomic_write_bytes` and `_retry` (829–866), which handle Windows sharing violations.
|
||
- **`templates/*.md` and `preamble.md`.** These are short and to the point. The default sequential `commit_rule` (3152) says "This gremlin commits" even when the brief says not to. The smoke gremlin noticed the conflict and followed the brief. Make the rule an explicit flag.
|
||
- **Ledger plus the `usage --format md` tables** (3877–4096), but trimmed by half (see below).
|
||
|
||
## 4. Delete or simplify
|
||
|
||
### 4a. The detached supervisor and everything that exists because of it
|
||
|
||
The whole stack exists so that launches can return at once (SKILL.md:70). It is then needed to stop jobs from leaking once nobody watches them. The Bash tool already moves a call past its timeout into the background and sends a completion notice, and `run_in_background` does the same on purpose. The orchestrator can run `gremlins.py spawn …` as a **blocking** call in the background and read the result JSON from the notice. Without detach, the following code goes away:
|
||
|
||
- **Detach.** `WindowsPlatform.detach` with its PowerShell/CIM hidden window (356–362, 476–495), `detach_supervisor` (1745), `supervisor_interpreter` (1398), and the `detach_failed` reason.
|
||
- **The supervisor.** `Supervisor` (3517–3684) and `cmd_supervise` (3687–3704). The run loop polls every 0.25 s and handles heartbeat, stall, orphan check, keep-warm tick and max time in one `while` loop (3613–3659). `os.environ.clear()` (3579) mutates its own process environment to reach the child's PATH.
|
||
- **`SupClock`** (3481–3514). It detects host suspend so that suspends are not counted toward `max_minutes` and so the orphan watchdog can be re-armed. This is a special case that exists only to patch another special case (the watchdog).
|
||
- **The orphan watchdog** (3639–3651) and the stamps behind it: `stamp_orchestrator` and `touch_orchestrator` (2009–2030). `in_child` checks guard both functions, so a gremlin cannot keep its own watchdog alive (that is the M4 fix). Again, this is a patch on a patch.
|
||
- **Heartbeats and reaping.** `reap_lost`, `last_alive_at`, `LOST_AFTER_S` and the `lost` status (2040–2073).
|
||
- **`wait`** (3720–3790). Its `delivered_at`/`settled_at` double bookkeeping, `--any`, exit codes 3 and 4, and the 540/570 s caps all work around the Bash timeout. With completion notices there is nothing left to wait on. A plain `result <job>` read would do.
|
||
- **The pid-reuse-safe tree kill.** `process_tree` uses dead-root bounds and `ALIVE_UNTIL_SLACK_S` (1099–1151). `snapshot_survivor` is "C16 as narrowed by A2" (1192–1205). `kill_tree` sweeps and verifies for 10 s (1161–1189). If `claude` is a child of the foreground helper, `taskkill /T /F /PID <child>` on Windows (or `killpg` on POSIX) while the helper is alive covers it. Python's `subprocess` with a Job Object is the robust variant.
|
||
- **`RegistryLock`** (881–1005, 125 lines). It breaks stale locks by renaming the lock aside and putting it back if the content changed. `Platform.restore_no_replace` exists only for this. Writes per job (one `jobs/<id>.json` per call) plus append-only ledger lines mostly remove the shared read-modify-write. What remains needs, at most, a 20-line `O_EXCL` lock with a pid check.
|
||
- **About 60 % of the platform seam.** `Platform` has 14 abstract methods for one tested platform. `PosixPlatform` (604–727) is 125 lines behind a closed gate, plus `UNSUPPORTED_OK_ENV` (127–129, 745). `visible_windows` (338, 583–601) is used only by tests. Without detach the real differences come down to path normalisation and the kill call.
|
||
|
||
### 4b. Heuristics nobody asked for
|
||
|
||
- **The ping machinery** (3293–3476, 185 lines). It covers `pings_since_real_call`, `ping_phase`, `run_active` with its 10-minute activity window, a `break_even` cap computed from price ratios, and `ping_in_flight` marks. `tick_pass` decides twice, outside and inside the lock, and so needs a special `why="eligible_since_this_tick_began"` (3424). Every supervisor ticks every 60 s and every `wait` every 30 s. With a 1 h TTL, `list` already reports `warm_until`. A plain `ping --base B`, called when the orchestrator sees `expiring`, is about 20 lines.
|
||
- **The rebase and re-train flags.** `due_flags` (2910–2956) combines five thresholds (`rebase_commits`, `rebase_lines`, `rebase_delta_tokens`, `max_base_tokens`, `max_rebases`) with `defining_drift` (2859–2888), `binary_state` drift by version, size and mtime (2891–2907), runtime drift from the init event (1548–1570, 2847), and a transcript tail read (2778–2830). They are all advisory (the docstring says only `binary_missing` stops a launch), yet each one is an init flag (2217–2218). Keep `phase_boundary` and `max_base_tokens`, and drop the rest.
|
||
- **The `_conclude` god function** (1752–1904, 150 lines). It mixes four things:
|
||
- the status decision: the if-chain at 1771–1790, which should be a `classify(ss, exit_code)` function returning `(status, reason, detail)`
|
||
- eight flag rules (1793–1824), which should be a table of `(flag, predicate)`
|
||
- two near-duplicate dict literals, the result and the ledger line (1833–1851)
|
||
- the base state machine for train, rebase, ping and supersede (1867–1903)
|
||
|
||
Split it into classify, a flags table, `record()`, and a separate base-transition function.
|
||
- **The race rule for "rebase finishes after a retrain superseded it".** `_rebase_target_free`, the `old_status` branch of `_rebase_succeeded` and the `parent_inactive` flag (1819–1824, 1907–1963) handle it. Refusing `train --supersedes X` while a rebase of X runs would replace all of this with a single check.
|
||
- **Flags and options that are not needed:**
|
||
- `--method` has one choice, `("fork",)` (164, 2256). It still sets `method` on every job, result and ledger row.
|
||
- `launch_overrides` (2241–2249) adds hidden flags on spawn, continue and rebase only to refuse them. argparse already rejects unknown flags.
|
||
- `PHASE2_COMMANDS = ()` (119, 4120) is dead.
|
||
- `train` re-accepts every launch field as a per-base override (2227–2236, `train_launch` 3015–3036). That forces the "override or config" logic in `defining_drift` (2866–2870). One launch config per run is enough, and a new run can change it.
|
||
- `--ttl-s`, `--warm-margin-s` with its runtime caps (2212, 1681, SKILL.md:30), `--stall-after-s`, `--orphan-after`, `--pings on|off` and `--parallel-cap` are tunables without a user.
|
||
- On an `init` re-run, `rerun_binary` (2458–2475) re-resolves the binary by its original source.
|
||
- `model_admission` (1426–1452) is "permissive at init". It reads settings and enforces nothing.
|
||
- **`where`.** Its env-scrub section lists "unknown" vars and a `drop_path_entries` prose rule. That is useful once when porting, and noise on every skill load (the SKILL.md front matter injects it).
|
||
- **The tests.** 4,235 lines, more than the code. Most of them pin the machinery above (lock races, pid reuse, clock jumps, ping caps). They shrink with it.
|
||
|
||
## 5. The minimal version
|
||
|
||
One file, about **600–800 lines**, standard library only, run in the foreground:
|
||
|
||
| Part | Lines |
|
||
|---|---|
|
||
| Config and state dir: `run.json` (launch config + bases), `jobs/<id>.json`, `ledger.jsonl`, `notes/`, `raw/` | 80 |
|
||
| Env scrub + argv builder (kept code) | 120 |
|
||
| Stream reader + usage + cache verdict + warmth (kept code) | 220 |
|
||
| Commands | 250 |
|
||
| Windows path norm + child-tree kill (taskkill / killpg) | 60 |
|
||
| argparse | 60 |
|
||
| Tests | about 500 |
|
||
|
||
The commands:
|
||
|
||
- `init --run R --model M [--effort] [--agent] [--cwd]` writes the launch config.
|
||
- `train --base B --brief F` runs `claude -p … --session-id <uuid>` in the foreground and streams it to `raw/<job>.jsonl`. On exit it writes the result and a ledger line, marks the base active, and records `last_hit`, `context_tokens` and the TTL.
|
||
- `spawn --base B --block X --brief F` does the same with `--resume <base> --fork-session`. The prompt is the spawn template plus a delta header. The result carries `cache`, `base_state` at launch, and the note path.
|
||
- `continue --job J --brief F` resumes the gremlin session (no fork).
|
||
- `ping --base B` is a one-turn fork with `--max-turns 1`, only when asked.
|
||
- `list` shows bases with `base_state` and `warm_until`, and jobs with their status.
|
||
- `usage [--block|--phase] [--format md]`.
|
||
- `kill --job J` kills the recorded child pid tree.
|
||
|
||
The happy path from the orchestrator (Athanor), each call in Bash with `run_in_background: true`. The completion notice carries the result JSON.
|
||
|
||
```
|
||
G init R --model claude-opus-5-5[1m]
|
||
G train R --base B1 --brief train.md # background; notice → {status, session, cost, ...}
|
||
G spawn R --base B1 --block 1.1 --brief b.md # background; notice → {status, cache: hit, note, ...}
|
||
G spawn R --base B1 --block 1.2 --brief c.md # parallel = just two background calls
|
||
G usage R --phase 1 --format md
|
||
```
|
||
|
||
Rebase is just "spawn, and register its session as a new base". It is about 30 lines inside `spawn` (`--as NEW`), not a separate state machine.
|
||
|
||
One open check before the rewrite: does `TaskStop` on a backgrounded Git Bash call kill the `claude.exe` grandchild on Windows? If it does not, keep `kill --job`. Either way the helper should kill its own child on SIGTERM or SIGBREAK.
|
||
|
||
## 6. Usable right now?
|
||
|
||
**Yes, on Windows, for the happy path.** I ran it from the worktree root and in a scratch git repo at `C:\Users\g4b\AppData\Local\Temp\claude\G--dev-projects-reliquary\42c87f82-…\scratchpad\verdict-smoke`, following SKILL.md's call sequence.
|
||
|
||
| Step | Result |
|
||
|---|---|
|
||
| `where` | Exit 0 in 0.25 s, `ok: true`. Platform windows (supported), binary 2.1.299 found through `CLAUDE_CODE_EXECPATH`, uv 0.5.13, git found. 13 "unknown" `CLAUDE_*` vars listed, no warnings. |
|
||
| `init --model claude-sonnet-5-5 --cwd <scratch>` | Exit 0. The model was accepted (`model_admission: "no availableModels list"`). Effort high, permission mode `dontAsk`, empty strict MCP config, preamble hashed. |
|
||
| `train --base B1 --phase 1 --brief <one line>` | Exit 0 in 0.6 s; returned job `j0001` and a session id. |
|
||
| `wait --all` (Bash timeout 600000) | Exit 0 after 2 s. `status: ok`, 5.3 s wall, 2 turns, `models_used: [claude-sonnet-5-5]`, 16,731 tokens written to the 1h cache, $0.0706. |
|
||
| `spawn --base B1 --block 1.1 --phase 1 --brief <one line>` | Exit 0. `base_state: warm`, no rebase or re-train due. |
|
||
| `wait --all` | Exit 0 after 12 s. `status: ok`, **`cache: hit`** (69,943 tokens read from cache, 1,530 written), 10.6 s wall, $0.0195. `notes/1.1.md` was written in the right format, stamped `claude-sonnet-5-5`. |
|
||
| `usage` (json) and `usage --phase 1 --format md` | Both correct. Total $0.0902, cache-hit ratio 1/1, 0 cold starts. The md table renders well. |
|
||
| `list` | B1 active, `warm_until` 1 h out, nothing running. |
|
||
| `expire --all` | Exit 0: `retired: [B1]`, `killed: []`, `ok: true`. |
|
||
| Process check afterwards | No `_supervise`, `--fork-session` or `run-gremlins` processes left. |
|
||
|
||
Nothing broke. Minor findings:
|
||
|
||
- **The default commit rule conflicts with a no-commit brief.** The spawn template's default rule (sequential) told the gremlin to commit, while the brief said not to. The gremlin flagged the conflict in its note and followed the brief.
|
||
- **Rounding differs between ledger fields.** `cost_cumulative_usd` (0.0706285) and `cost_delta_usd` (0.070628) are rounded differently in the ledger.
|
||
- **Paths not exercised.** `kill`, pings, `continue`, `rebase`, failure paths and long runs over the Bash timeout were not run. `expire` had nothing to kill.
|
||
- **Platforms.** The tool works only on Windows; Linux and macOS are gated off by design.
|