📝 Gremlins MVP phase review

2026-10-16 23:49:03 +02:00
parent 0493f03c98
commit cfeb1bf111
1 changed files with 82 additions and 0 deletions
@@ -0,0 +1,82 @@
 # Gremlins MVP: phase review
 - Reviewer model: **claude-opus-5-5[1m]** · Date: 2026-10-16 · Branch `gremlins-mvp` @ `0493f03` · Timebox 15 min
 - Scope: `plugins/spire/skills/gremlins/` against the [spec](../plans/2026-10-17-00-gremlins-mvp-spec.md) and the [build report](2026-10-17-gremlins-mvp-build.md)
 - Gate: `uv run --no-project --python 3.12 python -m unittest discover -s tests` gave **24 tests, OK, 7.0 s**, the same as the build report. No live `claude` calls in this review.
 ## 1. Boxes
 | Box | Verdict | Evidence | Verified by |
 |---|---|---|---|
 | B1 | ticked | `cli.py:226-247` train with `--session-id` (`launch.py:172`); fingerprint stored once in `run.json`, overwrite refused `cli.py:202`; `launch.py:139-169` | live smoke + `test_cli` happy/missing_binary/train_failures, `test_stream` |
 | B2 | ticked | `launch.py:176-177` stored argv + `--resume --fork-session --session-id`; `cli.py:250-269`; `cache_verdict` `stream.py:178` | live smoke (`cache: hit`) + `test_launch`, `test_cli` happy/unknown_base/cache_miss |
 | B3 | ticked | no detach/supervisor code anywhere; one job file per job `cli.py:134`; ledger append `store.py:85` | live (two `run_in_background` spawns) + `test_parallel_spawns`, `test_store` |
 | B4 | partly | no `kill` command; handler `call.py:53-66` (SIGTERM/SIGINT/SIGBREAK, second signal kills) | unit only (`test_call` SIGTERM, `test_cli` CTRL_BREAK). Spec §5 step 4 live `TaskStop` on an MVP spawn **not run** |
 | B5 | ticked | `stream.py:141-175` (models by usage, six token columns, delta rounded once); ledger line `cli.py:174-181` | live smoke (two ledger lines) + `test_stream`, `test_usage_models_and_cost` |
 | B6 | ticked | `_warmth` `cli.py:119-126` derived from ledger `hit_at`; `stream.py:206-215`; no ping code | live `status` (warm, ttl_left 3566) + `test_stream` cold/5m/expiring, `test_status_cold_without_hit` |
 | B7 | partly | `cli.py:272-298`, `launch.py:180-182` (no fork, no `--session-id`), session id from result `cli.py:294`, running-jobs warning `cli.py:278-280` | unit only (fake). Not run live; the cumulative-cost basis after a plain resume is an open assumption |
 | B8 | partly | only portable `Popen.terminate/kill` + `getattr(signal,"SIGBREAK")` `call.py:11`; scrub `launch.py:88-103` | `test_launch` scrub test. Spec §5 step 6 non-Windows gate **not run** |
 ## 2. Unrequested surface (list only; J = the build report justifies it)
 | # | Where | What | J |
 |---|---|---|---|
 | 1 | `store.py:10,25-30` | `SUBDIRS` + `init_state` | J |
 | 2 | `store.py:10`, `cli.py:136` | `briefs/<J>.md`: full prompt written per job | - |
 | 3 | `store.py:91-97` | ledger torn-last-line repair before append | - |
 | 4 | `store.py:14,98` | in-process `threading.Lock` around ledger append (spec: "without locks") | - |
 | 5 | `launch.py:10`, `cli.py:211` | `MODEL_RE`: aliases refused at `init` | - |
 | 6 | `launch.py:11`, `cli.py:330` | `EFFORTS` choices; default effort `high` | - |
 | 7 | `launch.py:13-15,158-161` | fixed tool list, `allowed_tools`, `disallowed_tools` (incl. `Bash(claude:*)`, `git push/rebase/reset --hard`) | J (fields); deny list itself not |
 | 8 | `launch.py:41` | `SPIRE_GREMLIN_CHILD=1` still pinned into the child env although the refusal was dropped | partly (dev. 11) |
 | 9 | `launch.py:36-40,65` | `SCRUB_KEEP`/`"keep"` class: no behavioural effect (keep and unlisted both pass) | ported per §3 |
 | 10 | `stream.py:13-27` | `iso`/`parse_iso` in stream | J |
 | 11 | `stream.py:43,56-58,127-128` | `refused` + `model_refused` status case (not in spec's `classify` tuple) | - |
 | 12 | `stream.py:34,36-37,44,73-75` | `init`, `msg_ids`, `_ids`, `events`, `turns`: tracked, never read | - |
 | 13 | `stream.py:165-167,174` | `mismatch` computed, never read | J |
 | 14 | `stream.py:200-203` | `wrote_only_5m`: listed in §3, never called | (spec) |
 | 15 | `cli.py:180` | ledger field `hit_at` | J |
 | 16 | `cli.py:16,87-90` | `NAME_RE` validation of base/job names | - |
 | 17 | `cli.py:17,163-167` | note detection by file mtime with 2 s slack | - |
 | 18 | `cli.py:22-32,355-357` | `Refusal` / JSON argparse errors; `ValueError`/`OSError` mapped to exit 2 | J |
 | 19 | `cli.py:359` | exit codes 0/1/2 | J |
 | 20 | `cli.py:231,255` | train refuses existing base; spawn refuses existing job | J |
 | 21 | `cli.py:281,291-293` | rebase job id `<B>-rebase<n>`; `rebases[]` entries `{job, at, from_commit, from_session}` | J (ids) |
 | 22 | `cli.py:258,269` | spawn output `base_state`, computed before the call | - |
 | 23 | `cli.py:208` | `init` defaults cwd to the git toplevel | - |
 | 24 | `cli.py:214-217` | frozen preamble copy + `mcp-empty.json` in the state dir | J (mcp) |
 | 25 | `call.py:48` | stderr to `raw/<J>.err` | - |
 | 26 | `call.py:10,83-85` | reader join timeout 10 s (orphan holding stdout) | - |
 | 27 | `call.py:56-57` | second signal escalates to `kill()` | J |
 | 28 | `call.py:42` | `ss=None` injection parameter, used by no caller | - |
 | 29 | `SKILL.md` | the skill file | J |
 | 30 | `tests/test_call.py:24` | `test_tee_and_parse` | J |
 **30 items, 15 justified (J or partly) by the build report, 15 not.** None looks like a bug. Most are a few lines; #2, #3, #11, #12, #17 are the ones that add real behaviour.
 ## 3. The 972 vs 610 gap
 Measured with `ast` (docstrings), blank, comment and import lines:
 | Module | Est. | Actual | Blank | Doc | Cmt | Import | Logic | Where the logic surplus goes |
 |---|---|---|---|---|---|---|---|---|
 | `store.py` | 70 | 116 | 23 | 8 | 0 | 7 | 78 | `init_state`, torn-line repair, lock (#1, #3, #4) |
 | `launch.py` | 110 | 182 | 31 | 9 | 6 | 7 | 129 | scrub constants as estimated; plus tool lists, `binary_version`, `build_fingerprint` re-validating what `cmd_init` already checks (`launch.py:141-150` vs `cli.py:205-212`) |
 | `stream.py` | 170 | 215 | 28 | 17 | 0 | 4 | 166 | on estimate; dead `wrote_only_5m`/`turns`/`mismatch` (~12) offset by a compact `classify` |
 | `call.py` | 60 | 86 | 9 | 4 | 0 | 6 | 67 | two threads, kill escalation, join timeout |
 | `cli.py` | 200 | 359 | 52 | 15 | 0 | 11 | 281 | multi-line dict literals for job/ledger/base (~35); `delta_header` + git helpers (~35, spec counted it "as is"); `_execute` hit/note/excerpt logic (~15); duplicated init validation (~8); `Refusal`/`_Parser` (~12, not the ~30 the build report says) |
 | `gremlins.py` | (cli) | 14 | 3 | 1 | 4 | 3 | 3 | PEP 723 header |
 | **Total** | **610** | **972** | 146 | 54 | 10 | 38 | **724** | |
 Form (blank, docstring, comment, import) is **248** of the 362 extra lines. Logic is 724 against 610, so +114 (+19%), and 81 of that is in `cli.py`. That surplus is mostly verbose layout (dicts written over several lines, duplicated validation) plus the small additions in §2. Only `call.py`'s threads and `_execute`'s hit/note logic are real logic the spec underestimated. **Verdict: mostly form.** The substance is within the verdict's 600–800 band.
 ## Handoff flags answered
 - Dev. 1 (SKILL.md): accepted; B3/B4 need the orchestrator to be told. Dev. 2 (rebase.md wording): correct, the old text contradicted B7. Dev. 3: per §3.
 - Dev. 4–6, 8, 9: consistent with the code and tests (`test_cli` happy checks the unrounded basis to 6 places). Dev. 7, 10: as implemented. Dev. 11: the refusal is gone but the env var is still set (#8). Dev. 12: justified, and the only test that covers stdin and the tee.
 - Open items (live B4/B7/B8, Windows append atomicity): still open. These three boxes are "partly", not failures.
 - Weakened checks / tests that cannot fail: none found. On Windows `test_stop_signal` sends CTRL_BREAK to a process group that includes the fake child, so it cannot tell the helper's `terminate()` apart from the child dying on its own. `test_call.test_sigterm_stops_child` (in-process `raise_signal`) does isolate the handler path.
 ## Bug (one line)
 No real bug noticed. The nearest is harmless duplication: `init` validates model, binary and cwd twice (`cli.py:205-212`, `launch.py:141-150`).