fix(runtime): execute configured reviewer models - #10
Conversation
3metaJun
left a comment
There was a problem hiding this comment.
Reviewed first in the #10 → #11 → #12 order. Verified locally on Windows: npm test passes with 103 passing tests and the one expected Unix-only skip — matching the PR description.
The design is sound: reviewer lists validate as non-empty and unique, a Harness override replaces the whole role value, and the --model / --model-index / --all-models conflict check plus the --all-models-requires---read-only guard match the README. The fanout test genuinely exercises concurrency (workers wait on each other through a shared log file), attributed per-worker output, partial-failure exit codes, and a 128 KiB output payload. The Windows leg of that test works because processInvocation wraps the fake CLI in PowerShell, which resolves pi.ps1 on PATH.
Non-blocking nits:
validModelaccepts whitespace-padded names —" gpt-5 "passes validation and is forwarded verbatim as the model CLI argument. Consider trimming (or rejecting) at validation time.--all-modelsis accepted when the resolved role value is a plain string; execution then prints a one-element JSON array where the single-run path prints an object. Harmless, but an undocumented output-shape change — either reject--all-modelsfor string roles or document it.- Repeated switch flags (e.g.
--read-only --read-only) now fail with "Duplicate option" under the shared parser. Fine if intentional; just noting the strictness.
The inherit-parent break (new CLI processes now require --parent-model, or an explicit --model auto) is a deliberate and clearly documented behavior change with tests updated accordingly — no objection.
Merge note (already in the PR body): this conflicts with #11/#12 on the package.json test line, skills/reflect/SKILL.md, and the shared target digests in profiles/upstream-manifest.json; the reflect digest must be recomputed after combining.
[gpt-6] RESPONDING ON BEHALF OF 3metajunVerified all three review notes against the current source.
The subsequent PRs will be rebased in order, preserving both test registrations and recomputing |
Reviewer lists documented by
interrogatecould not be configured, and a fresh CLI process silently treatedinherit-parentas its own default model. Reviewer roles now accept model lists with explicit single-entry selection or concurrent read-only execution. Each worker retains its output and exit status; failed workers make the command fail.CLI inheritance requires a known
--parent-model, whileautoexplicitly selects the CLI default. Setup preserves existing choices and overrides, fallback reviews keep the intended workspace, and model CLI flags reject missing values. Documentation distinguishes the reply-marker smoke check from skill workflow verification.Validation: full suite passed with 103 passing tests and one expected Windows skip. The focused tests additionally exercised concurrent workers, partial failure, large output, explicit inheritance, and malformed arguments. Package and pinned upstream checks passed; independent review found no blockers.
This PR can be reviewed independently of the history and full-tree integrity changes. After combining branches, resolve the
reflectdigest against its final body. npm publication remains deferred.Agent: GPT-6 via Codex