[github] feat: model selection — antagonistic review + Cindi + Specialist Agents UI tab (Phase 1) - #4123
Conversation
|
👀 Quinn is reviewing — verdict (PASS / WARN / FAIL) + findings to follow. |
📝 WalkthroughWalkthroughThis PR extends the agent model configuration system to support configurable specialist review models for antagonistic and inline review. It introduces ChangesAntagonistic Review Model Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Comment |
![protoquinn[bot]](https://avatars.githubusercontent.com/in/3282126?s=60&v=4){kind=small}
There was a problem hiding this comment.
QA Audit — PR #4123 | [github] feat: model selection — antagonistic review + Cindi + Specialist Agents UI tab (Phase 1)
VERDICT: WARN (CI not yet terminal)
CI Status
- build: queued
- test: queued
- checks: queued
Diff Review
Adds two new PhaseModelConfig fields (antagonisticReviewModel, inlineReviewModel) to replace a hardcoded 'haiku' model in PlanReviewService. Server side wires getPhaseModelWithOverrides() to resolve the model at call time and passes it to runAntagonisticReview. Defaults to 'protolabs/fast'.
Observations
- Gap:
runAntagonisticReviewis called withmodelandsettingsServiceargs, but the function signature update is not visible in this diff window. If the inner function doesn't accept those params, this is a runtime break. Author should confirm the full function signature accommodates these args (file:line evidence needed). - Gap:
settingsServiceforwarded as optional torunAntagonisticReview— if the inner function doesn't handle null gracefully, silent null-refs could occur at runtime. - Gap: No tests visible in the first 200 lines of diff. CI results will reveal coverage.
- CI not terminal: Will re-evaluate once checks complete. If CI passes cleanly, this is a PASS with the above as advisory notes.
— Quinn, QA Engineer
|
Submitted CI is still queuing — VERDICT is |
There was a problem hiding this comment.
QA Audit — PR #4123 | [github] feat: model selection — antagonistic review + Cindi + Specialist Agents UI tab (Phase 1)
VERDICT: FAIL
CI Status
- ci-complete: failure ❌
- build: failure ❌
- test: success ✅
- checks: failure ❌
Diff Review
Phase 1 of specialist agent model selection. Adds antagonisticReviewModel / inlineReviewModel fields to PhaseModelConfig and wires them through PlanReviewService.verifyPlan() via getPhaseModelWithOverrides. Defaults to protolabs/fast. Clean intent, but two blocking issues.
Observations
CLAWPATCH/GAP: Structural review unavailable — checkout cache rejected refmainfor this repo.CRITICAL:apps/server/src/services/plan-review-service.ts:66—verifyPlanpassesmodel+settingsServiceto_runReview(lines 80–81), but_runReview's updated method signature is absent from the diff. Build failure is consistent with a TypeScript type mismatch at this call site. Author must ensure the method signature was updated end-to-end.HIGH:libs/types/src/agent-settings.ts:227–230— Both new fields are required (no?) onPhaseModelConfig, but existing persisted configs predate this PR and will be missing them. Risk of runtime crashes on existing operators. Recommend making fields optional or adding a migration path.MEDIUM: No test coverage added for the new config surface.
— Quinn, QA Engineer
|
Submitted |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/server/src/services/plan-review-service.ts (2)
74-85:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSignature mismatch:
verifyPlanGoalBackwarddoesn't acceptmodelorsettingsService.The call passes
modelandsettingsService(lines 82-83), but the method signature at lines 136-156 does not include these parameters. This will cause a TypeScript compilation error.🐛 Proposed fix: update verifyPlanGoalBackward signature
private async verifyPlanGoalBackward(params: { featureTitle: string; featureDescription: string; complexity: string; planOutput: string; projectPath: string; structuredPlan: StructuredPlan; + model: string; + settingsService?: SettingsService | null; }): Promise<{Then use
params.modelon line 213 instead ofresolveModelString('haiku').🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/services/plan-review-service.ts` around lines 74 - 85, The call to verifyPlanGoalBackward passes model and settingsService but the method signature for verifyPlanGoalBackward doesn't accept them; update the verifyPlanGoalBackward function signature to include model and settingsService (e.g., add model and settingsService to its params object) and inside verifyPlanGoalBackward replace the hardcoded resolveModelString('haiku') usage with params.model so the passed-in model is used for resolution and settingsService is available for any settings-dependent logic.
136-156:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
verifyPlanGoalBackwardignores the passed model and hardcodes haiku.Even after fixing the signature mismatch, line 213 hardcodes
resolveModelString('haiku')instead of using the configured model. The PR objective is to make this configurable.🔧 Proposed fix
After adding
modelto the params signature:const result = await simpleQuery({ prompt: `You are a critical architect...`, - model: resolveModelString('haiku'), + model: params.model, cwd: projectPath,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/services/plan-review-service.ts` around lines 136 - 156, The verifyPlanGoalBackward function currently ignores the configured model and always calls resolveModelString('haiku'); update the function to accept and use the model parameter passed in params (e.g., add model: string to the params object and destructuring) and replace resolveModelString('haiku') with resolveModelString(model) (or the appropriate variable name) wherever the hardcoded string is used so the configured model is honored; ensure any callers are updated to pass the model through to verifyPlanGoalBackward if they do not already.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/server/src/services/plan-review-service.ts`:
- Around line 59-65: The resolved phase model stored in model (from
getPhaseModelWithOverrides + resolvePhaseModel) is not used; replace hardcoded
resolveModelString('haiku') calls in the standard review path with the
configured model (either pass model directly or call resolveModelString(model)
if model is a string alias) so the phase override is honored; update all
occurrences in the affected block (the standard review path and the similar
branch that spans the other section) to use the resolved model variable instead
of the literal 'haiku'.
- Around line 13-18: The PR uses resolveModelString (called as
resolveModelString('haiku')) but that function isn't imported, causing a runtime
ReferenceError; fix by either adding an import for resolveModelString from
'`@protolabsai/model-resolver`' or — preferred — replace the hardcoded
resolveModelString('haiku') calls with the already-resolved model variable (the
one computed via resolvePhaseModel or named `model` in this file) so all code
uses the same resolved model; update occurrences in the functions that call
resolveModelString to reference the existing resolved model or add the import
and use it consistently.
---
Outside diff comments:
In `@apps/server/src/services/plan-review-service.ts`:
- Around line 74-85: The call to verifyPlanGoalBackward passes model and
settingsService but the method signature for verifyPlanGoalBackward doesn't
accept them; update the verifyPlanGoalBackward function signature to include
model and settingsService (e.g., add model and settingsService to its params
object) and inside verifyPlanGoalBackward replace the hardcoded
resolveModelString('haiku') usage with params.model so the passed-in model is
used for resolution and settingsService is available for any settings-dependent
logic.
- Around line 136-156: The verifyPlanGoalBackward function currently ignores the
configured model and always calls resolveModelString('haiku'); update the
function to accept and use the model parameter passed in params (e.g., add
model: string to the params object and destructuring) and replace
resolveModelString('haiku') with resolveModelString(model) (or the appropriate
variable name) wherever the hardcoded string is used so the configured model is
honored; ensure any callers are updated to pass the model through to
verifyPlanGoalBackward if they do not already.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 268bcafa-80b3-4018-8827-a4906fb5159b
📒 Files selected for processing (2)
apps/server/src/services/plan-review-service.tslibs/types/src/agent-settings.ts
| import { createLogger } from '@protolabsai/utils'; | ||
| import type { StructuredPlan } from '@protolabsai/types'; | ||
| import { resolveModelString } from '@protolabsai/model-resolver'; | ||
| import { resolvePhaseModel } from '@protolabsai/model-resolver'; | ||
| import { simpleQuery } from '../providers/simple-query-service.js'; | ||
| import { getPhaseModelWithOverrides } from '../lib/settings-helpers.js'; | ||
| import type { SettingsService } from './settings-service.js'; |
There was a problem hiding this comment.
Missing import: resolveModelString is used but not imported.
Lines 108 and 213 call resolveModelString('haiku'), but this function is not imported. This will cause a ReferenceError at runtime.
Either import resolveModelString from @protolabsai/model-resolver, or replace the hardcoded calls with the resolved model variable.
🐛 Proposed fix: add missing import or use resolved model
Option A — Import the missing function:
import { resolvePhaseModel } from '`@protolabsai/model-resolver`';
+import { resolveModelString } from '`@protolabsai/model-resolver`';Option B — Use the already-resolved model throughout (preferred, aligns with PR goals):
- model: resolveModelString('haiku'),
+ model,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { createLogger } from '@protolabsai/utils'; | |
| import type { StructuredPlan } from '@protolabsai/types'; | |
| import { resolveModelString } from '@protolabsai/model-resolver'; | |
| import { resolvePhaseModel } from '@protolabsai/model-resolver'; | |
| import { simpleQuery } from '../providers/simple-query-service.js'; | |
| import { getPhaseModelWithOverrides } from '../lib/settings-helpers.js'; | |
| import type { SettingsService } from './settings-service.js'; | |
| import { createLogger } from '`@protolabsai/utils`'; | |
| import type { StructuredPlan } from '`@protolabsai/types`'; | |
| import { resolvePhaseModel } from '`@protolabsai/model-resolver`'; | |
| import { resolveModelString } from '`@protolabsai/model-resolver`'; | |
| import { simpleQuery } from '../providers/simple-query-service.js'; | |
| import { getPhaseModelWithOverrides } from '../lib/settings-helpers.js'; | |
| import type { SettingsService } from './settings-service.js'; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/server/src/services/plan-review-service.ts` around lines 13 - 18, The PR
uses resolveModelString (called as resolveModelString('haiku')) but that
function isn't imported, causing a runtime ReferenceError; fix by either adding
an import for resolveModelString from '`@protolabsai/model-resolver`' or —
preferred — replace the hardcoded resolveModelString('haiku') calls with the
already-resolved model variable (the one computed via resolvePhaseModel or named
`model` in this file) so all code uses the same resolved model; update
occurrences in the functions that call resolveModelString to reference the
existing resolved model or add the import and use it consistently.
| // Resolve model from phase model config | ||
| const { phaseModel } = await getPhaseModelWithOverrides( | ||
| 'antagonisticReviewModel', | ||
| settingsService, | ||
| projectPath | ||
| ); | ||
| const model = resolvePhaseModel(phaseModel).model; |
There was a problem hiding this comment.
Resolved model is not used in the standard review path.
The model variable is resolved from phase config (lines 59-65) but the standard review path (line 108) still hardcodes resolveModelString('haiku'). This defeats the purpose of the configurable model override.
🔧 Proposed fix
const result = await simpleQuery({
prompt: `You are a critical code reviewer...`,
- model: resolveModelString('haiku'),
+ model,
cwd: projectPath,Also applies to: 87-114
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/server/src/services/plan-review-service.ts` around lines 59 - 65, The
resolved phase model stored in model (from getPhaseModelWithOverrides +
resolvePhaseModel) is not used; replace hardcoded resolveModelString('haiku')
calls in the standard review path with the configured model (either pass model
directly or call resolveModelString(model) if model is a string alias) so the
phase override is honored; update all occurrences in the affected block (the
standard review path and the similar branch that spans the other section) to use
the resolved model variable instead of the literal 'haiku'.
![protoreview[bot]](https://avatars.githubusercontent.com/in/4233969?s=60&v=4){kind=small}
There was a problem hiding this comment.
QA panel review — FAIL
code-review · head 3bcf4ac91602 · formal
[review-synthesizer completed: workflow code-review:report]
All three findings survive verification — none refuted, none uncertain — so the array keeps every item, with verdict/note carried verbatim. The verifier's only corrections are cosmetic (the exact TS diagnostic code and the add/delete count), which I've folded into the claim for finding 1 while keeping the note byte-identical.
Overall risk is high: the server package does not build, because the goal-backward call at line 82 passes model/settingsService to verifyPlanGoalBackward, whose unchanged signature accepts neither key, so TypeScript excess-property checking rejects the literal — fix that first by extending the callee's param type or dropping the extra args. Verification confirmed all three findings as-is; the panel's only disagreements were cosmetic (the diagnostic is TS2345/TS2322 at the call site, not TS2353, and the diff is ~+22/−1, not +25/−1), and the PR-head file 404'd but the diff plus base fully determine head content. No coverage gaps: both changed files carry findings, and the heavily-changed plan-review-service.ts has two.
Findings
| Severity | Location | Finding | Verified | |
|---|---|---|---|---|
| 🔴 | blocker | apps/server/src/services/plan-review-service.ts:82 |
The call to verifyPlanGoalBackward now passes model and settingsService, but the callee's parameter type — untouched by this PR, declaring only featureTitl… |
confirmed |
| 🟡 | minor | libs/types/src/agent-settings.ts:228 |
inlineReviewModel is added as a required PhaseModelConfig key with a DEFAULT_PHASE_MODELS entry, but the key is brand-new and no code path in this PR (the on… |
confirmed |
| 🟡 | minor | apps/server/src/services/plan-review-service.ts:60 |
verifyPlan now resolves its review model via getPhaseModelWithOverrides('antagonisticReviewModel', settingsService, projectPath) — a behavior change gated on s… | confirmed |
findings JSON (machine-readable)
[
{
"file": "apps/server/src/services/plan-review-service.ts",
"line": 82,
"severity": "blocker",
"category": "cross-file",
"claim": "The call to verifyPlanGoalBackward now passes `model` and `settingsService`, but the callee's parameter type — untouched by this PR, declaring only featureTitle/featureDescription/complexity/planOutput/projectPath/structuredPlan — accepts neither key, so the fresh object literal fails TypeScript excess-property checking (TS2345/TS2322) and the server package does not build; even past type-checking, the callee never reads either value, so the resolved model is silently dropped on the goal-backward path. Flagged by correctness, removed-behavior, and cross-file review.",
"evidence": "Diff hunk @@ -67,6 +79,8 @@:\n planOutput,\n projectPath,\n structuredPlan,\n+ model,\n+ settingsService,\n });\nagainst the unchanged callee signature from apps/server/src/services/plan-review-service.ts (base ref): `private async verifyPlanGoalBackward(params: {\n featureTitle: string;\n featureDescription: string;\n complexity: string;\n planOutput: string;\n projectPath: string;\n structuredPlan: StructuredPlan;\n })` — no hunk in this PR adds `model` or `settingsService` to it.",
"verdict": "confirmed",
"note": "Diff adds `model`/`settingsService` to the goal-backward call at head line 82 and contains no hunk touching the callee signature; base file confirms the param type lacks both keys, so TS excess-property checking rejects the literal — build-break mechanism real (diagnostic is TS2345/TS2322 at the call site, not TS2353; cosmetic). PR-head file reads 404'd, but diff + base read fully determine the facts."
},
{
"file": "libs/types/src/agent-settings.ts",
"line": 228,
"severity": "minor",
"category": "cross-file",
"claim": "`inlineReviewModel` is added as a required PhaseModelConfig key with a DEFAULT_PHASE_MODELS entry, but the key is brand-new and no code path in this PR (the only new phase-model read is 'antagonisticReviewModel') consumes it — the doc comment names PlanProcessor as its consumer, which this PR never wires — so the advertised setting has no runtime effect while the required field forces every other PhaseModelConfig literal to adopt a key nothing reads. Flagged by both cross-file and conventions review.",
"evidence": "Diff hunk @@ -222,6 +222,12 @@:\n+ /** Model for inline fallback review when PlanReviewService is not wired (PlanProcessor) */\n+ inlineReviewModel: PhaseModelEntry;\nand DEFAULT_PHASE_MODELS gains `inlineReviewModel: { model: 'protolabs/fast' },`; the PR's only consumer change is `getPhaseModelWithOverrides('antagonisticReviewModel', settingsService, projectPath)` in plan-review-service.ts — no hunk reads `inlineReviewModel`.",
"verdict": "confirmed",
"note": "Diff adds required (no `?`) `inlineReviewModel` at head line 228 + default entry; the PR's only phase-model lookup is 'antagonisticReviewModel', so nothing in the PR reads it — a brand-new key read via string literals has no pre-existing consumer."
},
{
"file": "apps/server/src/services/plan-review-service.ts",
"line": 60,
"severity": "minor",
"category": "tests",
"claim": "verifyPlan now resolves its review model via getPhaseModelWithOverrides('antagonisticReviewModel', settingsService, projectPath) — a behavior change gated on settingsService — yet the entire diff (2 files, +25/−1) contains no test changes covering the new resolution path or the new required config fields.",
"evidence": " // Resolve model from phase model config\n const { phaseModel } = await getPhaseModelWithOverrides(\n 'antagonisticReviewModel',\n settingsService,\n projectPath\n );\n const model = resolvePhaseModel(phaseModel).model;",
"verdict": "confirmed",
"note": "Resolution block is at head line 60 (verified by hunk `@@ -50,8 +53,17 @@` arithmetic); diff touches exactly 2 files, neither a test, so the new path and required fields are untested. Claimed +25/−1 ≈ actual +22/−1 — count cosmetic, substance holds."
}
]1 participant
Summary
SPARC PRD: Model Selection — Specialist Agents UI Tab (Phase 1)
Situation
The protoMaker platform uses a
PhaseModelConfigsystem (libs/types/src/agent-settings.ts) allowing operators to configure LLM models per workflow phase (enhancement, spec generation, coding, etc.). The UI exposes these controls via a "Model Defaults" settings tab (apps/ui/src/components/views/settings-view/model-defaults/), and the server resolves models throughgetPhaseModelWithOverrides()(`apps/server/src...Closes #4110
Created automatically by Automaker
Summary by CodeRabbit
Release Notes