Skip to content

fix(runtime): execute configured reviewer models - #10

Merged
3metaJun merged 2 commits into
mainfrom
fix/model-runtime-selection
Sep 11, 2026
Merged

3metaJun merged 2 commits into
mainfrom
fix/model-runtime-selection

Conversation

@3metaJun

Copy link
Copy Markdown
Owner

Reviewer lists documented by interrogate could not be configured, and a fresh CLI process silently treated inherit-parent as 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, while auto explicitly 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 reflect digest against its final body. npm publication remains deferred.


Agent: GPT-6 via Codex

@3metaJun 3metaJun left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. validModel accepts whitespace-padded names — " gpt-5 " passes validation and is forwarded verbatim as the model CLI argument. Consider trimming (or rejecting) at validation time.
  2. --all-models is 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-models for string roles or document it.
  3. 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.

@3metaJun

Copy link
Copy Markdown
Owner Author

[gpt-6] RESPONDING ON BEHALF OF 3metajun

Verified all three review notes against the current source.

  1. Fixed padded model names in scalar defaults, reviewer lists, Harness overrides, --model, and --parent-model. The commands reject leading or trailing whitespace before creating a worker plan. Both new regressions failed before the fix; all 39 related runtime/configuration/environment tests now pass.
  2. Kept --all-models valid for string roles and documented its one-element array result. The output shape follows the requested execution mode consistently.
  3. Repeated switches remain intentionally rejected by the shared parser. This keeps duplicate arguments explicit instead of silently accepting them.

The subsequent PRs will be rebased in order, preserving both test registrations and recomputing reflect and complete-tree digests from the combined content.

@3metaJun
3metaJun merged commit d93cf7f into main Sep 11, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant