diff --git a/docs/workpad/reports/2026-10-17-gremlins-mvp-review.md b/docs/workpad/reports/2026-10-17-gremlins-mvp-review.md new file mode 100644 index 0000000..76a3b85 --- /dev/null +++ b/docs/workpad/reports/2026-10-17-gremlins-mvp-review.md @@ -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/.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 `-rebase`; `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/.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`).