Canonical provider classification, forward-client model invariant, and the environment model on the public surface - #3936
Conversation
…ient model invariant Extract the provider-classification rules that four call sites had each re-derived slightly differently into one pure `shared/model-provider.ts`, and hold new hosts to a non-empty model so an environment can always resolve one. - `classifyModelIdProvider` is the single source of truth: blank -> null, `custom:<slug>` -> custom, prefix map (now including the `mistralai -> mistral` alias alongside `meta-llama -> meta` and `x-ai -> xai`), bare Bedrock shapes, everything else -> ollama. - `buildSyntheticModelDefinition`, the chat-session locked-model fallback, and the public eval API's `providerForModelId` all delegate to it. Two intentional behavior changes: a bare unrecognized id no longer becomes `openrouter` in chat sessions, and `mistralai/...` no longer becomes `ollama`. - Parity fixtures live in `shared/__tests__/model-provider-fixtures.ts`, copied into the backend mirror's test so the two implementations cannot drift. - `POST /v1/.../hosts` now refines `config.modelId` as a required non-empty string (templates are guarded, never substituted), and the Behavior tab blocks Save on an edit that CLEARS a pinned model while leaving legacy modelless hosts editable for unrelated changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V5kwSKq4J7Fqi9JBhCZ1Wy
…lity probe Threads `modelId` through the public surface, and gives clients a way to ask whether the deployment in front of them accepts it — the SDK and CLI ship independently of the platform, and an unknown field is a hard validator error there, not a silently ignored one. - v1 environments: `modelId` on create (non-empty) and PATCH (tri-state, `null` clears); the resolve DTO gains `modelId`, `effectiveModelId` and `modelSource`. `ENV_MODEL_REQUIRED` maps to 409 with a branchable `details.reason = "environment_model_required"`. - `GET .../environments/capabilities` proxies the backend probe and answers `false` — never an error — when the backend cannot answer, because ABSENCE is the signal an old deployment gives. Registered above `/:environmentId` so the literal segment is not read as an id. Documented in openapi.json. - SDK: `modelId` on the environment type and both bodies, model fields on the resolved type, `PlatformEnvironmentCapabilities`, `getEnvironmentCapabilities`, and a `get_project_environment_capabilities` operation (excluded from the agent registry — a compatibility probe is not an action). - CLI: `environments create --model`, `environments update --model` / `--clear-model` (mutually exclusive), both preflighting the capability ONLY when model input was supplied. JSON input keeps accepting `"modelId": null`. - Journey launch: 402 maps to `BILLING_LIMIT_REACHED`; the failure message unwraps structured bodies, keeps a plain backend sentence, and refuses HTML, multi-line, or oversized bodies. The swarm create flow stops scheduling on the first 402, reports one billing message for the wave, and the `launchJourney` option type finally declares the `environmentIds` its call site was already passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V5kwSKq4J7Fqi9JBhCZ1Wy
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
GitHub Actions
Inactive
dosubot
Bot
added
size:XXL
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_222431d2-7e04-448c-888d-10c20d92c289) |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Internal previewPreview URL: https://mcp-inspector-pr-3936.up.railway.app |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 29 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds environment-level model overrides with capability discovery, SDK support, CLI flags, validation, and effective-model metadata. It centralizes model-provider classification across inspector services. Host validation now blocks clearing saved models. Journey launches stop scheduling after billing-limit errors and display dedicated partial-launch warnings. Evaluation helpers use iteration snapshots, environment identities, and persisted model metadata. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
mcpjam-inspector/server/routes/v1/agent-op-registry.ts (1)
645-646: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for both operation-surface exclusions.
The new exclusions are correct, but they need tests that prevent future registry drift.
mcpjam-inspector/server/routes/v1/agent-op-registry.ts#L645-L646: assert thatget_project_environment_capabilitiesis excluded from direct and gated agent operations.mcpjam-inspector/server/utils/built-in-tools/mcpjam.ts#L168-L169: assert that the operation is excluded from the workspace tool surface.🤖 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 `@mcpjam-inspector/server/routes/v1/agent-op-registry.ts` around lines 645 - 646, Add regression tests covering both operation-surface exclusions: verify get_project_environment_capabilities is absent from direct and gated agent operations in mcpjam-inspector/server/routes/v1/agent-op-registry.ts at lines 645-646, and absent from the workspace tool surface in mcpjam-inspector/server/utils/built-in-tools/mcpjam.ts at lines 168-169. Anchor the assertions to the relevant registry and workspace-tool construction symbols.Source: Coding guidelines
mcpjam-inspector/server/routes/shared/evals.ts (1)
1605-1620: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftAdd backend-backed regression coverage for the environment rerun bypass.
Cover a model-less prompt with an effective environment model, a missing environment model returning
ENV_MODEL_REQUIRED, and a bare rerun withoutenvironmentIdstill failingassertBareRerunCasesRunnable. Current tests cover the local guard and environment-ID forwarding, but not the complete projection and iteration-recording path.🤖 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 `@mcpjam-inspector/server/routes/shared/evals.ts` around lines 1605 - 1620, Add backend-backed regression tests for the environment rerun flow around assertBareRerunCasesRunnable and startTestSuiteRun: verify a model-less prompt uses the environment’s effective model and records iterations, an environment without a resolvable model fails with ENV_MODEL_REQUIRED, and a rerun without environmentId still invokes the bare-rerun guard and fails. Reuse the existing environment-ID forwarding and local-guard test setup while exercising the complete projection and iteration-recording path.Source: Coding guidelines
mcpjam-inspector/client/src/components/evals/helpers.ts (1)
499-516: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake
aggregateSuitescope explicit.
activeIterationsincludes iterations from multiple suite runs, butbyCaseMapcombines their counts and keeps metadata from the first row. A case rerun with another model can therefore display the wrong model. Either aggregate one run at a time or preserve run/model identity inSuiteAggregate.byCase.Add tests for differing snapshots, mixed legacy and snapshot rows, and null or empty snapshot fields.
🤖 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 `@mcpjam-inspector/client/src/components/evals/helpers.ts` around lines 499 - 516, Update aggregateSuite and SuiteAggregate.byCase so aggregation is scoped to an individual suite run, preventing activeIterations from combining counts or retaining metadata from another run/model. Preserve snapshot precedence with fallback for legacy rows, including correct handling of differing snapshots, mixed legacy and snapshot rows, and null or empty snapshot fields. Add focused tests covering these cases.Source: Coding guidelines
🤖 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 `@cli/src/commands/environments.ts`:
- Around line 119-132: Update assertModelOverridesSupported and the analogous
capability probes near the other referenced locations to pass the effective
target project selector to getEnvironmentCapabilitiesOperation.execute instead
of an empty input. Resolve it from the --project option, falling back to the
JSON project field when present, so capability checks and subsequent commands
use the same project.
In `@docs/reference/openapi.json`:
- Around line 8886-8890: Update both request modelId schema properties to reject
whitespace-only strings by adding the pattern .*\\S.* alongside minLength,
preserving the existing type, description, and minLength declarations.
In `@mcpjam-inspector/client/src/components/swarms/new-swarm-create-flow.tsx`:
- Around line 257-289: Add tests for runWithConcurrency covering a worker
returning "stop", ensuring in-flight workers settle while no additional items
are scheduled. Add launch-flow tests for partial billing blocking and ordinary
partial failure, asserting the billing path displays exactly one billing
warning; include relevant happy-path, validation, error, and empty/null edge
cases where applicable.
In `@mcpjam-inspector/server/routes/v1/__tests__/environments.test.ts`:
- Around line 520-538: Extend the resolve endpoint tests around “carries
effectiveModelId and modelSource through resolve” with a host-derived case: mock
a resolved row without a stored modelId, set effectiveModelId to the
host-derived value and modelSource to “host,” then assert the response preserves
those values and omits or leaves modelId unset as appropriate.
In `@mcpjam-inspector/server/routes/v1/environments.ts`:
- Around line 460-464: Update the capability-fetch error handling around the
Convex query in environments.ts so only the recognized missing-query deploy-skew
error falls back to unsupported capabilities; rethrow or normally translate
authorization, not-found, and other service errors. In
mcpjam-inspector/server/routes/v1/__tests__/environments.test.ts lines 587-600,
preserve the missing-query fallback test and add coverage confirming project
access failures do not return HTTP 200 with unsupported capabilities.
In `@mcpjam-inspector/server/routes/v1/evals.ts`:
- Around line 1311-1318: Update normalizeCreateTestsToRunTests to derive an
omitted provider through the canonical providerForModelId(model) resolver
instead of splitting the model string prefix, preserving correct alias and
custom-ID handling. Add route tests covering suite creation with omitted
providers for both aliased models and custom model IDs.
In `@mcpjam-inspector/server/routes/v1/hosts.ts`:
- Around line 112-123: Add server route tests for the host-creation handler
covering both template and explicit-config inputs: valid requests,
missing/null/empty/whitespace-only config.modelId, empty config, and templates
resolving without a model. Assert successful responses for valid requests and
the expected validation response for every rejected case, including the
WebRouteError raised by hostConfigPinsAModel.
In
`@mcpjam-inspector/server/services/sessionSimulation/__tests__/launch-journey-run.test.ts`:
- Around line 183-193: Extend the parameterized cases in the
launchFailureMessage coverage within launch-journey-run.test.ts to include an
empty body and malformed JSON such as "{". Verify both rejected SwarmAgentError
inputs preserve status 402 and produce the safe credit-limit fallback message
through launchJourneyRun.
In `@mcpjam-inspector/server/services/sessionSimulation/launch-journey-run.ts`:
- Around line 120-144: Update the structured-message handling in the launch
journey reason parser to validate extracted envelope and ConvexError message
values with the same length, newline, and markup policy used by looksLikeProse
before returning them. Preserve the fallback behavior when validation fails, and
add regression coverage for structured multiline, markup-containing, and
oversized messages.
In `@sdk/src/platform/index.ts`:
- Line 66: Update the public type exports in the platform index to include
EnvironmentCapabilitiesInput and EnvironmentCapabilitiesResult alongside
PlatformEnvironmentCapabilities and getEnvironmentCapabilitiesOperation.
Re-export both named types from ./operations.js so consumers can access the
complete capability operation API.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/components/evals/helpers.ts`:
- Around line 499-516: Update aggregateSuite and SuiteAggregate.byCase so
aggregation is scoped to an individual suite run, preventing activeIterations
from combining counts or retaining metadata from another run/model. Preserve
snapshot precedence with fallback for legacy rows, including correct handling of
differing snapshots, mixed legacy and snapshot rows, and null or empty snapshot
fields. Add focused tests covering these cases.
In `@mcpjam-inspector/server/routes/shared/evals.ts`:
- Around line 1605-1620: Add backend-backed regression tests for the environment
rerun flow around assertBareRerunCasesRunnable and startTestSuiteRun: verify a
model-less prompt uses the environment’s effective model and records iterations,
an environment without a resolvable model fails with ENV_MODEL_REQUIRED, and a
rerun without environmentId still invokes the bare-rerun guard and fails. Reuse
the existing environment-ID forwarding and local-guard test setup while
exercising the complete projection and iteration-recording path.
In `@mcpjam-inspector/server/routes/v1/agent-op-registry.ts`:
- Around line 645-646: Add regression tests covering both operation-surface
exclusions: verify get_project_environment_capabilities is absent from direct
and gated agent operations in
mcpjam-inspector/server/routes/v1/agent-op-registry.ts at lines 645-646, and
absent from the workspace tool surface in
mcpjam-inspector/server/utils/built-in-tools/mcpjam.ts at lines 168-169. Anchor
the assertions to the relevant registry and workspace-tool construction symbols.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ed76f0ef-4bb4-442a-b833-0cdab418dcca
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (28)
cli/src/commands/environments.tscli/src/lib/op-bindings.tsdocs/reference/openapi.jsonmcpjam-inspector/client/src/components/evals/helpers.tsmcpjam-inspector/client/src/components/hosts/redesigned/HostBuilderViewRedesigned.tsxmcpjam-inspector/client/src/components/hosts/redesigned/focus/__tests__/useHostDraftValidation.model.test.tsmcpjam-inspector/client/src/components/hosts/redesigned/focus/useHostDraftValidation.tsmcpjam-inspector/client/src/components/swarms/new-swarm-create-flow.tsxmcpjam-inspector/client/src/hooks/use-chat-session.tsmcpjam-inspector/server/routes/shared/evals.tsmcpjam-inspector/server/routes/v1/__tests__/environments.test.tsmcpjam-inspector/server/routes/v1/__tests__/sdk-coverage.test.tsmcpjam-inspector/server/routes/v1/agent-op-registry.tsmcpjam-inspector/server/routes/v1/environments.tsmcpjam-inspector/server/routes/v1/evals.tsmcpjam-inspector/server/routes/v1/hosts.tsmcpjam-inspector/server/services/sessionSimulation/__tests__/launch-journey-run.test.tsmcpjam-inspector/server/services/sessionSimulation/launch-journey-run.tsmcpjam-inspector/server/utils/built-in-tools/mcpjam.tsmcpjam-inspector/server/utils/org-model-config.tsmcpjam-inspector/shared/__tests__/model-provider-fixtures.tsmcpjam-inspector/shared/__tests__/model-provider.test.tsmcpjam-inspector/shared/model-provider.tssdk/src/platform/client.tssdk/src/platform/index.tssdk/src/platform/operations.tssdk/src/platform/types.tssdk/tests/platform/operations.test.ts
| /** | ||
| * Run `worker` over `items`, at most `limit` at a time, and STOP SCHEDULING | ||
| * once `worker` reports the wave is doomed. | ||
| * | ||
| * The stop signal exists for one failure: an organization that hits its credit | ||
| * limit. Every remaining launch in the wave will be rejected for exactly the | ||
| * same reason, so firing them costs a round-trip each and produces N identical | ||
| * banners. A worker returns `"stop"` and no further item is picked up. | ||
| * | ||
| * Requests ALREADY in flight are not cancelled here — a launch is a POST that | ||
| * may have already created a durable run row, and aborting the client half | ||
| * would leave one running with nobody watching. They are allowed to settle; | ||
| * the caller deduplicates their errors so one billing message is shown, not | ||
| * `limit` of them. | ||
| */ | ||
| async function runWithConcurrency<T>( | ||
| items: T[], | ||
| limit: number, | ||
| worker: (item: T) => Promise<void> | ||
| worker: (item: T) => Promise<void | "stop"> | ||
| ): Promise<void> { | ||
| let cursor = 0; | ||
| let stopped = false; | ||
| const runners = Array.from( | ||
| { length: Math.min(limit, items.length) }, | ||
| async () => { | ||
| while (cursor < items.length) { | ||
| if (stopped) return; | ||
| const index = cursor; | ||
| cursor += 1; | ||
| await worker(items[index]); | ||
| if ((await worker(items[index])) === "stop") { | ||
| stopped = true; | ||
| return; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add tests for terminal billing scheduling.
Add client coverage for a worker that returns "stop", including already-running workers settling without scheduling further items. Add launch-flow coverage for a partial billing block and for an ordinary partial failure. Verify that the billing path shows one billing warning.
As per coding guidelines: “All changes should include tests, covering happy paths, validation errors, error handling, and edge cases such as null and empty values.”
🤖 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 `@mcpjam-inspector/client/src/components/swarms/new-swarm-create-flow.tsx`
around lines 257 - 289, Add tests for runWithConcurrency covering a worker
returning "stop", ensuring in-flight workers settle while no additional items
are scheduled. Add launch-flow tests for partial billing blocking and ordinary
partial failure, asserting the billing path displays exactly one billing
warning; include relevant happy-path, validation, error, and empty/null edge
cases where applicable.
Source: Coding guidelines
| // The forward-client invariant applies to the template branch too. Templates | ||
| // carry their OWN model (each is tuned to the client it emulates), so this is | ||
| // a guard, never a substitution — a catalog entry that lost its model is a | ||
| // catalog bug, and minting a modelless host from it would surface as an | ||
| // `ENV_MODEL_REQUIRED` launch refusal much later. | ||
| if (!hostConfigPinsAModel(input)) { | ||
| throw new WebRouteError( | ||
| 400, | ||
| ErrorCode.VALIDATION_ERROR, | ||
| `Host template "${templateId}" does not pin a model; pass an explicit \`config\` instead.` | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add route tests for the new host-creation validation.
No included server test covers these public validation branches. Add tests for valid template and explicit-config requests, missing, null, empty, and whitespace-only config.modelId, an empty config, both input branches, and a template that resolves without a model. Assert the validation response for each rejected request.
As per coding guidelines, “All changes should include tests, covering happy paths, validation errors, error handling, and edge cases such as null and empty values.”
Also applies to: 194-247
🤖 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 `@mcpjam-inspector/server/routes/v1/hosts.ts` around lines 112 - 123, Add
server route tests for the host-creation handler covering both template and
explicit-config inputs: valid requests, missing/null/empty/whitespace-only
config.modelId, empty config, and templates resolving without a model. Assert
successful responses for valid requests and the expected validation response for
every rejected case, including the WebRouteError raised by hostConfigPinsAModel.
Source: Coding guidelines
CI caught a third operation registry I had missed; the rest are review findings
from cubic-dev-ai and CodeRabbit that turned out to be real.
- `mcp/src/tools/platformTools.ts` enforces its own catalog partition, so the
new `get_project_environment_capabilities` operation had to be classified
there too. This was the only genuine CI failure.
- `classifyModelIdProvider` now looks the prefix up with `hasOwnProperty`. A
bare index made `constructor/x` and `toString/x` return a FUNCTION off
`Object.prototype` — truthy, so it was handed back as the provider. Fixture
vectors added on both sides of the mirror.
- `providerForModelId` trims before the catalog lookup, so a padded bare
catalog id resolves to its real vendor instead of the Ollama catch-all; and
`normalizeCreateTestsToRunTests` uses it instead of `model.split("/")[0]`,
which derived `meta-llama`/`mistralai` as providers and refused every
`custom:<slug>:<model>` id.
- `POST /v1/.../hosts` trims `config.modelId` rather than persisting a padded
value that downstream comparison would treat as an unknown model.
- The capabilities route swallows ONLY the missing-function (deploy-skew)
error. Collapsing an authorization failure or an outage into a 200 with both
capabilities false told the caller to upgrade a platform that was already
current instead of fixing their access.
- `launchFailureMessage` applies one policy — bounded, single-line, no markup —
to a message unwrapped from a structured body as well as to a plain one; an
envelope is no more trustworthy about what it carries.
- The CLI capability preflight probes the TARGET project (`--project`, else the
JSON body's `project`), not the caller's default, which could have blocked a
valid write or passed one the target rejects.
- SDK exports `EnvironmentCapabilitiesInput` / `EnvironmentCapabilitiesResult`;
the OpenAPI `modelId` request properties reject whitespace-only strings.
Tests added for the host model invariant (missing/null/empty/whitespace/
non-string, padded, modelless template, and the XOR message), the capability
route's old-deployment vs access-failure split, host-derived resolve, and the
structured/empty/malformed launch-failure bodies.
`aggregateSuite`'s one-row-per-case folding is documented rather than
restructured: it is pre-existing (the live case's `models[0]` was equally
single-valued) and narrowing it means re-keying `byCase`, which changes the
type every caller renders.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V5kwSKq4J7Fqi9JBhCZ1Wy
GitHub Actions
Error
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_4a62f15c-be1f-446e-a1e9-c6b3896c1587) |
|
Pushed CI
Review findings takenA real bug, thank you — the prefix lookup. Also fixed:
New tests: the host model invariant (missing / null / empty / whitespace-only / non-string, the padded-trim case, a modelless template, and that Two I did not take
Forwarding structured billing details on 402. Reasonable, and genuinely useful for upgrade/reset messaging, but it's a new capability rather than a correction, and the fan-out currently only needs the code to decide to stop scheduling. Happy to add it if you'd like it in this PR. VerificationBackend Generated by Claude Code |
MCP worker previewPreview worker |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@mcpjam-inspector/server/routes/v1/__tests__/hosts.test.ts`:
- Around line 422-433: Update the padded-model test case in the POST host
request flow to assert that the response status is 201 before validating
createdHostInput(). Keep the existing trimmed modelId assertion unchanged.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 565826d9-2527-4f2d-b80b-3a7886b5c22f
📒 Files selected for processing (14)
cli/src/commands/environments.tsdocs/reference/openapi.jsonmcp/src/tools/platformTools.tsmcpjam-inspector/client/src/components/evals/helpers.tsmcpjam-inspector/server/routes/v1/__tests__/environments.test.tsmcpjam-inspector/server/routes/v1/__tests__/hosts.test.tsmcpjam-inspector/server/routes/v1/environments.tsmcpjam-inspector/server/routes/v1/evals.tsmcpjam-inspector/server/routes/v1/hosts.tsmcpjam-inspector/server/services/sessionSimulation/__tests__/launch-journey-run.test.tsmcpjam-inspector/server/services/sessionSimulation/launch-journey-run.tsmcpjam-inspector/shared/__tests__/model-provider-fixtures.tsmcpjam-inspector/shared/model-provider.tssdk/src/platform/index.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- mcpjam-inspector/server/routes/v1/hosts.ts
- sdk/src/platform/index.ts
- mcpjam-inspector/shared/tests/model-provider-fixtures.ts
- mcpjam-inspector/server/services/sessionSimulation/launch-journey-run.ts
- mcpjam-inspector/server/services/sessionSimulation/tests/launch-journey-run.test.ts
- mcpjam-inspector/server/routes/v1/evals.ts
- mcpjam-inspector/server/routes/v1/tests/environments.test.ts
- mcpjam-inspector/client/src/components/evals/helpers.ts
- mcpjam-inspector/server/routes/v1/environments.ts
- docs/reference/openapi.json
GitHub Actions
Error
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_9fbe241e-dedd-4956-bac6-c4e65153bdd6) |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@mcpjam-inspector/client/src/hooks/__tests__/use-run-cost-estimate.test.tsx`:
- Around line 493-501: Extend the “sends environment identities” test coverage
around SuiteProbe and queryCalls to verify that undefined and empty
environmentIds omit the field from query arguments, and add a rerender case
confirming that changing the environment list triggers a new query with updated
identities. Preserve the existing non-empty array assertion.
In `@mcpjam-inspector/server/services/evals-runner.ts`:
- Line 2922: Add regression tests covering local success, local failure, and
hosted success execution paths, including validation of null or empty model
values where applicable. Assert in each path that the effective model identifier
is passed to both finalizeEvalIteration and persistEvalTraceFanout, covering the
modelId assignments near the referenced execution flows.
- Line 3721: Update the hosted finalization flow around modelId so it persists
the resolved modelId rather than test.model. Pass modelId through the hosted
finalization calls on both success and failure paths, preserving the canonical
hosted model identifier for each iteration.
In `@mcpjam-inspector/server/services/sessionSimulation/launch-journey-run.ts`:
- Around line 114-126: Update safeLaunchReason to reject all four line
terminators—LF, CR, U+2028, and U+2029—while preserving its existing length,
empty-value, and markup checks. Add or confirm tests covering validation
failures and these line-terminator edge cases.
- Around line 182-203: Update launchFailureDetails to reject array-valued
source.details by requiring it to be a non-null object and not
Array.isArray(details) before spreading it into the returned metadata. Add or
confirm a regression test covering a malformed body with details as an array and
verify WebRouteError.details is not populated from that array.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 37068703-9b2e-4a1d-9c4e-f0fdf93fc782
📒 Files selected for processing (15)
cli/src/commands/environments.tsmcpjam-inspector/client/src/components/evals/__tests__/helpers-explore-sort.test.tsmcpjam-inspector/client/src/components/evals/helpers.tsmcpjam-inspector/client/src/components/evals/run-cost-estimate-hint.tsxmcpjam-inspector/client/src/components/evals/suite-header.tsxmcpjam-inspector/client/src/hooks/__tests__/use-run-cost-estimate.test.tsxmcpjam-inspector/client/src/hooks/use-run-cost-estimate.tsmcpjam-inspector/server/routes/shared/__tests__/evals.test.tsmcpjam-inspector/server/routes/shared/evals.tsmcpjam-inspector/server/services/evals-runner.tsmcpjam-inspector/server/services/evals/finalize-iteration.tsmcpjam-inspector/server/services/evals/recorder.tsmcpjam-inspector/server/services/sessionSimulation/__tests__/launch-journey-run.test.tsmcpjam-inspector/server/services/sessionSimulation/launch-journey-run.tsmcpjam-inspector/shared/model-provider.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- mcpjam-inspector/shared/model-provider.ts
- mcpjam-inspector/server/services/sessionSimulation/tests/launch-journey-run.test.ts
- cli/src/commands/environments.ts
- mcpjam-inspector/server/routes/shared/evals.ts
- mcpjam-inspector/client/src/components/evals/helpers.ts
| evaluation, | ||
| usage: usageFinal, | ||
| messages: acc.conversationMessages, | ||
| modelId: test.model, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add persistence tests for all execution paths.
Add regression tests for local success, local failure, and hosted success. Assert that the effective model identifier reaches finalizeEvalIteration and persistEvalTraceFanout.
As per coding guidelines, “All changes should include tests, covering happy paths, validation errors, error handling, and edge cases such as null and empty values.”
Also applies to: 3090-3090, 3721-3721
🤖 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 `@mcpjam-inspector/server/services/evals-runner.ts` at line 2922, Add
regression tests covering local success, local failure, and hosted success
execution paths, including validation of null or empty model values where
applicable. Assert in each path that the effective model identifier is passed to
both finalizeEvalIteration and persistEvalTraceFanout, covering the modelId
assignments near the referenced execution flows.
Source: Coding guidelines
There was a problem hiding this comment.
1 issue found across 24 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="mcpjam-inspector/server/services/evals/finalize-iteration.ts">
<violation number="1" location="mcpjam-inspector/server/services/evals/finalize-iteration.ts:117">
P2: modelId is forwarded to persistEvalTraceFanout but not to the W1 `updateTestIteration` fallback. When the fanout fails before any turn lands (`useW1Fallback`), the whole transcript is written through `updateTestIteration`, which creates a fresh chatSessions row — so the model attribution is dropped there and falls back to `eval/unknown` (see `args.modelId ?? "eval/unknown"` in persist-eval-trace.ts). This is the same bug class the code documents and fixes for `systemPrompt` in the W1 block ("Mirrors appendEvalTurnTrace.systemPrompt"). Forward modelId there too so the W1 path persists the intended model.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| toolsCalled: evaluation.toolsCalled, | ||
| usage, | ||
| messages, | ||
| ...(modelId ? { modelId } : {}), |
There was a problem hiding this comment.
P2: modelId is forwarded to persistEvalTraceFanout but not to the W1 updateTestIteration fallback. When the fanout fails before any turn lands (useW1Fallback), the whole transcript is written through updateTestIteration, which creates a fresh chatSessions row — so the model attribution is dropped there and falls back to eval/unknown (see args.modelId ?? "eval/unknown" in persist-eval-trace.ts). This is the same bug class the code documents and fixes for systemPrompt in the W1 block ("Mirrors appendEvalTurnTrace.systemPrompt"). Forward modelId there too so the W1 path persists the intended model.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/server/services/evals/finalize-iteration.ts, line 117:
<comment>modelId is forwarded to persistEvalTraceFanout but not to the W1 `updateTestIteration` fallback. When the fanout fails before any turn lands (`useW1Fallback`), the whole transcript is written through `updateTestIteration`, which creates a fresh chatSessions row — so the model attribution is dropped there and falls back to `eval/unknown` (see `args.modelId ?? "eval/unknown"` in persist-eval-trace.ts). This is the same bug class the code documents and fixes for `systemPrompt` in the W1 block ("Mirrors appendEvalTurnTrace.systemPrompt"). Forward modelId there too so the W1 path persists the intended model.</comment>
<file context>
@@ -111,6 +114,7 @@ export function buildIterationFinishParams(args: {
toolsCalled: evaluation.toolsCalled,
usage,
messages,
+ ...(modelId ? { modelId } : {}),
...(systemPrompt ? { systemPrompt } : {}),
...(spans?.length ? { spans } : {}),
</file context>
Snapshots: the runner-parity goldens still expected the pre-fix `eval/unknown`. Local iterations now record the bare id they called; hosted iterations record the canonical id they billed; a model-free case still records nothing (and falls back to `eval/unknown`), because it never ran a model. - evals-runner: the hosted finalization persists the RESOLVED `modelId` — what `executeTestCase` canonicalized and what the `/stream` call actually billed — instead of the authored `test.model`. The local finalization gates `modelId` on `caseNeedsModel`, matching the `model` field beside it: a model-free case arrives with a display-only `pinned-only` sentinel that must not be attributed as a real model. - launch-journey-run: drop the unused `safeLaunchReason`, which duplicated `showableReason`'s policy without a caller. The surviving helper rejects every line terminator (`\r`, U+2028, U+2029), not just `\n` — `trim` strips those at the ends but not the middle, so a body carrying one renders as several lines in a toast. `launchFailureDetails` drops an array-shaped `details` rather than spreading it into index keys. - hosts: apply the model trim on BOTH create branches. The normalization belongs to the write boundary, not to one of the two ways of reaching it — a template carrying a padded id persisted one nothing recognizes. - v1/evals: trim the id that gets STORED, not just the copy used to derive the provider. A padded id previously resolved to the right provider and then matched nothing downstream. - evals helpers: key every iteration when detecting mixed models, not only the snapshot-bearing ones. A case mixing snapshot rows with pre-snapshot rows is still a mix; keying only snapshots stamped the row with one model while the counters folded in another. - tests: assert the create succeeded before reading the mutation args, cover the template trim, the mixed/agreeing pre-snapshot rows, the new line-terminator and array-details rejections, and empty/undefined/ changing `environmentIds` on the suite estimate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V5kwSKq4J7Fqi9JBhCZ1Wy
GitHub Actions
Inactive
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_119a3e43-e6c0-4765-a74d-dca77cc2fc33) |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@mcpjam-inspector/server/routes/v1/evals.ts`:
- Around line 1342-1349: Reject whitespace-only model IDs before persistence by
validating the trimmed value in toPersistedModelEntry, while preserving valid
trimmed model/provider behavior and ensuring explicit providers cannot bypass
the rejection. Apply the validation to case and generated-case inputs, and add
regression tests covering whitespace-only IDs with and without an explicit
provider, alongside existing happy-path validation coverage.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c199939-c2fa-4c5f-97ea-1cd2a3248458
⛔ Files ignored due to path filters (1)
mcpjam-inspector/server/services/evals/__tests__/__snapshots__/runner-parity.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
mcpjam-inspector/client/src/components/evals/__tests__/helpers-explore-sort.test.tsmcpjam-inspector/client/src/components/evals/helpers.tsmcpjam-inspector/client/src/hooks/__tests__/use-run-cost-estimate.test.tsxmcpjam-inspector/server/routes/v1/__tests__/hosts.test.tsmcpjam-inspector/server/routes/v1/evals.tsmcpjam-inspector/server/routes/v1/hosts.tsmcpjam-inspector/server/services/evals-runner.tsmcpjam-inspector/server/services/sessionSimulation/__tests__/launch-journey-run.test.tsmcpjam-inspector/server/services/sessionSimulation/launch-journey-run.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- mcpjam-inspector/server/routes/v1/tests/hosts.test.ts
- mcpjam-inspector/server/services/sessionSimulation/tests/launch-journey-run.test.ts
- mcpjam-inspector/server/services/sessionSimulation/launch-journey-run.ts
- mcpjam-inspector/server/routes/v1/hosts.ts
- mcpjam-inspector/client/src/components/evals/helpers.ts
- mcpjam-inspector/server/services/evals-runner.ts
`defaultCaseModels` tested `modelId.length > 0` and then trimmed, so a whitespace-only suite config id passed the guard. It could not actually reach the return — `providerForModelId` rejects blanks too, so the provider lookup fails first and this falls through to "no default" — but that left the guard here correct only by way of another function's behaviour. Test the value actually returned instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V5kwSKq4J7Fqi9JBhCZ1Wy
GitHub Actions
Inactive
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_baaf717d-402d-4375-a70f-99a56ab22844) |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@mcpjam-inspector/server/routes/v1/__tests__/eval-edit.test.ts`:
- Around line 976-1000: Extend the parameter table in the whitespace-only model
test to include literal empty-string and null model IDs, covering both relevant
provider configurations as appropriate. Preserve the existing POST request and
assertions requiring a 400 response and no testSuites:createTestCase mutation.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9926cb33-b42a-4f8f-b9e2-a17680555778
📒 Files selected for processing (4)
mcpjam-inspector/server/routes/v1/__tests__/eval-edit.test.tsmcpjam-inspector/server/routes/v1/evals.tsmcpjam-inspector/server/services/sessionSimulation/__tests__/launch-journey-run.test.tsmcpjam-inspector/server/services/sessionSimulation/launch-journey-run.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- mcpjam-inspector/server/services/sessionSimulation/tests/launch-journey-run.test.ts
- mcpjam-inspector/server/services/sessionSimulation/launch-journey-run.ts
- mcpjam-inspector/server/routes/v1/evals.ts
… branch
`classifyModelIdProvider` ends in a catch-all, so `providerForModelId`
could only fail to name a provider for a BLANK id — and blank is now
rejected up front as a 400. That left two "cannot derive a provider"
throws unreachable, reading as live guards a maintainer would preserve.
Fold the blank rejection into the resolver and give it a total signature
(`string`, not `string | undefined`). `deriveProvider` collapses to the
explicit-or-derived choice it always was, the create-tests mapper loses
its second `if (!provider)`, and the suite-default call site loses an
undefined case it could no longer produce.
Also widen the blank-id rejection table to literal-empty and null ids.
Those never reach the route helper — `z.string().min(1)` rejects them at
the schema — but pinning them keeps the endpoint's contract one statement
("no usable id is a 400") rather than a fact about which of two layers
catches each shape.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V5kwSKq4J7Fqi9JBhCZ1Wy
GitHub Actions
Inactive
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_8fcc0e02-5084-4c3f-9fb4-7cf2abb4cbf9) |
GitHub Actions
Failure
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_f2eeaa52-2f42-41ee-88d6-a61af3b472d6) |
|
The job died in the very first install step, before touching any of this branch's code: That's the Railway CLI's postinstall fetching its own release tarball from GitHub and having the connection reset. Everything else on Bugbot's Generated by Claude Code |
GitHub Actions
Inactive
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_79187256-50ff-475d-8441-6c162b576da3) |
…anchable model refusal Review follow-ups on this branch. **Nine hosted vendors were being attributed to Ollama.** `providerForModelId` consulted the catalog only for BARE ids, so a qualified id whose vendor is in the catalog but not in the classifier's 13-entry prefix map — `cohere/`, `nvidia/`, `amazon/`, `bytedance/`, `stepfun/` and the rest — fell through to the classifier's `ollama` catch-all. Storing `ollama` then SHORT-CIRCUITS `assertInlineTestModelsValid` (an open namespace validates nothing), so a typo'd hosted id stopped being rejected at create time and was dispatched at a local Ollama instead. The catalog is now consulted for every id, and a qualified id nothing recognizes keeps its own vendor prefix rather than a guess. Split out of that: `attributedProvider` returns undefined where `ollama` would be a guess rather than knowledge, so `defaultCaseModels` goes back to "no default — inherit the suite model at run" for a suite id no catalog knows, instead of durably pinning a generated case to Ollama. **`PATCH /hosts/:hostId` and `/duplicate` bypassed the model invariant.** A config PATCH replaces the config, so one call could mint exactly the modelless host `create` now refuses — and persist a padded id, which `create` trims at what its own comment calls the write boundary. PATCH now applies the Behavior tab's rule (clearing a PINNED model is refused; a legacy modelless host stays editable) and the same trim; duplicate is held to the invariant outright, since copying a legacy row is otherwise a supported way to keep producing the state create refuses. **`ENV_MODEL_REQUIRED` reached eval callers as a bare Convex rejection.** It is now translated at the recorder — the chokepoint every eval surface shares, interactive, scheduled and v1 alike — into the same 409 the v1 environments resolve route emits, `details.reason` included. The swarm launch path now forwards the backend `code` alongside its details, because the message is bounded and prose-shaped by design and is the wrong thing to branch on. Also: a model-free case that THROWS mid-iteration no longer records the display-only sentinel as its model (the success path 170 lines up already gated this); a 402 no longer overwrites an earlier, more actionable target failure; `aggregateSuite` reads its title and its model from the SAME iteration, so a renamed-then-rerun case cannot show an old title beside a new model; the raw NUL byte in the classifier test is escaped, so git stops treating the file as binary; the hosts model requirement is documented in `openapi.json` and enforced in the SDK's `create_host` schema; and three doc comments that had detached from what they describe are reattached. Tests: +7 hosts (PATCH clear/legacy/trim, duplicate both ways), +4 eval-edit (cohere, nvidia, unknown-vendor prefix, unattributable default), +1 recorder, +1 launch. Server 5300, shared 581, SDK 4254, CLI 564, mcp 47, client evals /swarms/hosts 1763. Typecheck clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WaGZq4mTuUAGnXDRXoPpXs
GitHub Actions
Inactive
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_b40fb31e-7de0-4b2a-8485-b1eb59bb00c8) |
|
Pushed Two that block merge
The consequence is worse than a mislabel: Split out of that fix:
PATCH now applies the Behavior tab's rule — clearing a pinned model is refused, a legacy modelless host stays editable — plus the trim. Also fixed
VerificationServer 5300 · shared 581 · SDK 4254 · CLI 564 · New tests: +7 hosts (PATCH clear/legacy/trim, duplicate both ways), +4 eval-edit (cohere, nvidia, unknown-vendor prefix, unattributable suite default), +1 recorder, +1 launch. One existing assertion changed deliberately: the array-shaped- Not addressedThe The ~1/3 of this branch's diff that is Prettier config drift ( Generated by Claude Code |
…del-matrix-b4gyef
There was a problem hiding this comment.
2 issues found across 18 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="mcpjam-inspector/server/routes/v1/hosts.ts">
<violation number="1" location="mcpjam-inspector/server/routes/v1/hosts.ts:373">
P2: When a legacy host is pinned by another write after this preflight read, this PATCH can still replace the config without a model and undo that pin. Enforce the legacy-versus-pinned check atomically in `hosts:updateHost` or add a revision/conditional write to this route.</violation>
<violation number="2" location="mcpjam-inspector/server/routes/v1/hosts.ts:445">
P2: When a legacy host contains a padded model ID, this guard treats it as pinned, but `hosts:duplicateHost` copies the config without the new trimming boundary. The duplicate can therefore retain an unrecognized model ID and fail downstream model resolution; normalize the config in the duplicate write path or reject/repair padded source IDs before duplication.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // Same rule as the Behavior tab: CLEARING a pinned model is refused, while | ||
| // a legacy modelless host stays editable for everything else. That needs | ||
| // the pre-edit model, so the read happens only on the branch that needs it. | ||
| const current = await readHostDetail(token, projectId, hostId); |
There was a problem hiding this comment.
P2: When a legacy host is pinned by another write after this preflight read, this PATCH can still replace the config without a model and undo that pin. Enforce the legacy-versus-pinned check atomically in hosts:updateHost or add a revision/conditional write to this route.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/server/routes/v1/hosts.ts, line 373:
<comment>When a legacy host is pinned by another write after this preflight read, this PATCH can still replace the config without a model and undo that pin. Enforce the legacy-versus-pinned check atomically in `hosts:updateHost` or add a revision/conditional write to this route.</comment>
<file context>
@@ -358,10 +358,32 @@ hosts.patch("/projects/:projectId/hosts/:hostId", async (c) => {
+ // Same rule as the Behavior tab: CLEARING a pinned model is refused, while
+ // a legacy modelless host stays editable for everything else. That needs
+ // the pre-edit model, so the read happens only on the branch that needs it.
+ const current = await readHostDetail(token, projectId, hostId);
+ if (
+ hostConfigPinsAModel(current.config) &&
</file context>
| // edit to a legacy row, nothing is stranded by refusing: the source still | ||
| // exists, and pinning its model makes the copy legal. | ||
| const source = await readHostDetail(token, projectId, hostId); | ||
| if (!hostConfigPinsAModel(source.config)) { |
There was a problem hiding this comment.
P2: When a legacy host contains a padded model ID, this guard treats it as pinned, but hosts:duplicateHost copies the config without the new trimming boundary. The duplicate can therefore retain an unrecognized model ID and fail downstream model resolution; normalize the config in the duplicate write path or reject/repair padded source IDs before duplication.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/server/routes/v1/hosts.ts, line 445:
<comment>When a legacy host contains a padded model ID, this guard treats it as pinned, but `hosts:duplicateHost` copies the config without the new trimming boundary. The duplicate can therefore retain an unrecognized model ID and fail downstream model resolution; normalize the config in the duplicate write path or reject/repair padded source IDs before duplication.</comment>
<file context>
@@ -413,6 +435,21 @@ hosts.post("/projects/:projectId/hosts/:hostId/duplicate", async (c) => {
+ // edit to a legacy row, nothing is stranded by refusing: the source still
+ // exists, and pinning its model makes the copy legal.
+ const source = await readHostDetail(token, projectId, hostId);
+ if (!hostConfigPinsAModel(source.config)) {
+ throw new WebRouteError(
+ 400,
</file context>
Review follow-ups on the launch and host-invariant paths.
- The swarm wave's billing summary rendered `firstError`, so an unrelated
failure that settled before the 402 replaced the credit-limit copy with a
transient reason — the "retry the rest" reading the billing branch exists to
avoid. The 402's message now has its own slot.
- `launchFailureDetails` merged the envelope `code` over the details bag's own,
replacing an actionable domain code with a transport one. Details wins; the
envelope only fills a gap.
- The SDK's create-host schema reported a degenerate `config: {}` as a nested
non-empty-config error while the route reports the XOR. One `superRefine`,
shaped like the route's, so the schema predicts the 400 the caller receives.
- The vendor-prefix fallback test passed an explicit `provider`, which returns
verbatim, so the fallback under test never ran.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V5kwSKq4J7Fqi9JBhCZ1Wy
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_d079af95-2840-4eed-be72-44992d164f70) |
|
Worked the cubic round on Fixed
Not changing: duplicate + padded model id ( The premise doesn't hold — Verification: workspace Generated by Claude Code |
2 participants
Pairs with MCPJam/mcpjam-backend#948. Deploy the backend first — the capability probe and the new resolve fields come from there.
One provider classifier
"Given only this string, which provider serves it?" was answered slightly differently in four places, and the divergences were real bugs:
meta-llama/...came back asmeta-llamafrom one andmetafrom another;mistralai/...fell through to Ollama; a bare unknown id becameopenrouterin chat andollamain the runner.shared/model-provider.tsis now the source of truth: blank →null(never a guess — this is the guard that stops an unpinned host's persisted""becoming a plausible Ollama model),custom:<slug>→ custom, a recognized<prefix>/including the newmistralai → mistralalias, a bare Bedrock shape, everything else →ollama.buildSyntheticModelDefinition, the chat-session locked-model fallback, and the public eval API'sproviderForModelIdall delegate to it.Two intentional behavior changes, both correcting a divergence rather than introducing one: a bare unrecognized id no longer becomes
openrouterin chat sessions, andmistralai/...no longer becomesollama.Parity vectors live in
shared/__tests__/model-provider-fixtures.tsand are mirrored verbatim into the backend, ratcheted by itscheck:mirrors.Forward-client model invariant
An environment resolves its model from its own override, else its client's, else nowhere — and "nowhere" is a hard launch refusal. So a client minted without a model is a client that cannot back a headless environment, and the failure would surface at launch rather than at creation.
POST /v1/.../hostsrefinesconfig.modelIdas a required non-empty string (templates are guarded, never substituted — each keeps its own model). The Behavior tab blocks Save on an edit that clears a pinned model, while a legacy modelless client stays editable for unrelated changes: those rows predate the invariant, are deliberately not auto-backfilled, and stranding every rename behind a model choice would be the wrong trade.Public surface
modelIdon create (non-empty) and PATCH (tri-state,nullclears); the resolve DTO gainsmodelId,effectiveModelId,modelSource.ENV_MODEL_REQUIREDmaps to 409 with a branchabledetails.reason = "environment_model_required".GET .../environments/capabilitiesproxies the backend probe and answersfalse— never an error — when the backend cannot answer, because absence is the signal an old deployment gives. Registered above/:environmentIdso the literal segment is not read as an id. Documented inopenapi.json.PlatformEnvironmentCapabilities;getEnvironmentCapabilities; aget_project_environment_capabilitiesoperation, excluded from the agent registry (a compatibility probe is not an action).environments create --model,environments update --model/--clear-model(mutually exclusive), both preflighting the capability only when model input was supplied — an ordinary create shouldn't pay for a round-trip to guard a field it never sends. JSON input keeps accepting"modelId": null.The backend's own
@mcpjam/sdk@^2.0.1dependency is deliberately not bumped.Journey launch
402 maps to
BILLING_LIMIT_REACHED.launchFailureMessageunwraps structured bodies (v1 envelope, Convex error data), keeps a plain sentence the backend wrote for a human, and refuses HTML, multi-line, or oversized bodies —bodyTextis never passed through unexamined. The swarm create flow stops scheduling on the first 402 (every sibling would be rejected identically) and reports one billing message for the wave; in-flight requests are allowed to settle rather than aborted, because a launch POST may already have created a durable run.The
launchJourneyoption type now declares theenvironmentIdsits call site was already passing — a conditional spread had been defeating excess-property checking, leaving the per-run fan-out one rename away from being silently dropped.Eval history
aggregateSuitelabels a case from the iteration's snapshot rather than the case's current definition, so editing a case no longer relabels finished runs with a model they never used. The bare-rerun precheck no longer rejects model-less prompt cases on an environment-backed run — the backend projects and precreates them, so the premise of that check no longer holds there.Verification
Workspace
npm run typecheck✅ · SDK tests 4254 passed ✅ · CLI tests 564 passed ✅ · Inspector--project server5250 passed ✅ ·--project shared577 passed ✅ ·npm run build:inspector✅--project client: 8617 passed, 2 failed — both pre-existing onmain(ChatboxShareSection"issues the link again once the environment resolves",SwarmsTab.perClientEnvLaunch"refuses the whole launch when one of the two environments is gone"), verified by re-running them stashed.Not in this PR
The eval/swarm composer two-mode UI (
client_defaults/compare, the client × model cross-product, cell-aware collapse guards, draft v2) and the named-environment editor's model controls are not implemented — see the PR discussion for the handoff. Everything they will need is in place and capability-gated, and no user-facing entry point to compare mode ships here.🤖 Generated with Claude Code
https://claude.ai/code/session_01V5kwSKq4J7Fqi9JBhCZ1Wy
Generated by Claude Code
Note
High Risk
Touches public environment/host contracts, model resolution used at launch, and billing/error handling for swarm waves. Deploy backend first — capability probe and resolve fields depend on it.
Overview
Environments can now pin a model override (or inherit the host's) across the public API, SDK, and CLI. Resolve responses add
effectiveModelId/modelSource, andENV_MODEL_REQUIREDbecomes a branchable 409. A new capabilities probe lets clients detect support under version skew; the CLI preflights it only when--model/--clear-model/modelIdis actually supplied.Provider classification is centralized in
shared/model-provider.tsso chat, synthetic models, and the eval API agree (includingmistralai → mistraland bare-id →ollama). New hosts must pin a model; clearing a previously pinned model blocks Save, while legacy modelless hosts stay editable.Also hardens swarm launch: maps 402 to billing, stops scheduling the rest of a wave on credit limit, and sanitizes failure messages. Eval history labels cases from the iteration snapshot so later edits don't rewrite finished runs.
Reviewed by Cursor Bugbot for commit 145b7cb. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Unifies provider classification, enforces a pinned-model host invariant across create/patch/duplicate, and exposes environment model overrides with a capability probe on API, SDK, and CLI. Behavior changes include rejecting blank model ids;
mistralai/...now maps tomistral; chat no longer guessesopenrouter; and known vendor prefixes no longer default toollama.API
modelIdon create and PATCH (nullclears). Resolve addsmodelId,effectiveModelId, andmodelSource. List/Get emit only the stored override.GET /projects/{projectId}/environments/capabilitiesreports support; returnsfalseonly on older deployments. Auth/outage errors propagate.ENV_MODEL_REQUIREDtranslates to 409 withdetails.reason = "environment_model_required"from the resolve route and the eval recorder.config.modelIdon create; PATCH refuses clearing a previously pinned model (legacy modelless hosts remain editable); duplicate requires a pinned model; templates are guarded, not substituted.SDK (
@mcpjam/sdk)modelIdand resolved-model fields; definesPlatformEnvironmentCapabilities; addsgetEnvironmentCapabilities; exportsEnvironmentCapabilitiesInput/Result.create_hostschema enforces XOR oftemplatevsconfigand requires a non-empty model when usingconfig.CLI
environments create --model,environments update --model|--clear-model(mutually exclusive). Capability preflight runs only when a model arg is provided and against the target project.Provider classification
mistralai/... → mistral(alsometa → meta-llama,xai → x-aialiases); bare unknowns →ollama; guardsObject.prototypetraps. Chat, evals, and synthetic models delegate to it. Blank ids are rejected.cohere/,nvidia/,amazon/,bytedance/,stepfun/, etc.) attribute correctly; unknown vendors keep their own prefix instead of defaulting toollama.Evals and cost estimates
environmentIds. Mixed-model detection keys all iterations. Stored ids are trimmed.modelId; local iterations persistmodelIdonly when the case needed a model (including failure paths).Launch handling
BILLING_LIMIT_REACHED; stop scheduling the remaining launches in the wave. The billing message has its own slot and does not get replaced by unrelated earlier failures.details.codeover the envelope transport code; messages unwrap structured bodies, are single-line, no markup, size-bounded.Rollout
modelId; older deployments answerfalse.mistralai/...maps tomistral; qualified hosted vendors no longer attribute toollama; unknown vendors keep their prefix.Written for commit 6791378. Summary will update on new commits.