7.8 KiB
7.8 KiB
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 and the build report - Gate:
uv run --no-project --python 3.12 python -m unittest discover -s testsgave 24 tests, OK, 7.0 s, the same as the build report. No liveclaudecalls 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_clihappy 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_signalsends CTRL_BREAK to a process group that includes the fake child, so it cannot tell the helper'sterminate()apart from the child dying on its own.test_call.test_sigterm_stops_child(in-processraise_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).