Repository navigation
feat(environments): expose Project Environments on the API, SDK, and CLI - #3425
Conversation
Project Environments existed in the backend and the hosted UI but had no
public surface, and the v1 eval-run route actively rejected `environmentId`.
This adds the whole surface and makes environment runs work end to end.
- v1 routes (`server/routes/v1/environments.ts`): list (with
`?includeArchived`), get, create, patch, archive, restore, and a `/resolve`
launch preview. Archive/restore are explicit sub-actions rather than a
DELETE because the row is kept and restore has real semantics. Off the
guest allowlist; reads need project membership, writes need project admin.
- Eval runs: lifted the `environmentId` rejection. The route now resolves the
environment before building the manager, connects its closed server set
(including servers contributed by pinned plugin versions), and hands the
resolution to `prepareEvalRun` so the run is pinned to the revision whose
tools were captured.
- SDK: `PlatformEnvironment*` types, `PlatformApiClient` methods, and seven
MCP operations.
- CLI: `mcpjam environments {list,get,resolve,create,update,archive,restore}`.
- OpenAPI + docs, including the revision workflow and the pin constraints.
Two contracts are surfaced rather than smoothed over:
`expectedRevision` is required on every mutation instead of being re-read
server-side. Hiding it would convert the backend's safe concurrent-edit
rejection into a last-write-wins clobber.
PATCH keeps the backend's tri-state: an omitted field is unchanged, an
explicit `null` clears. Empty arrays stay rejected — they are not a clear.
Adds `CONFLICT` (409) to the v1 public error contract, which had no code for
"well-formed request, wrong resource state". This also fixes an existing bug:
`ENVIRONMENT_REVISION_CONFLICT` was missing from `INTERNAL_TO_V1_CODE`, so a
run-start revision conflict reached API callers as a 500. The byte-identical
backend copy of the contract is updated in a paired mcpjam-backend commit;
both repos' golden fixtures move together.
Verified: 9,734 tests pass; server typecheck adds zero errors over the
pre-existing baseline; client and CLI typechecks clean; openapi-drift green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GitHub Actions
Inactive
dosubot
Bot
added
size:XXL
✅ 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. |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d5a06e769
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .refine( | ||
| (body) => | ||
| body.suiteId || body.environmentId || (body.serverIds?.length ?? 0) > 0, | ||
| { | ||
| message: | ||
| "serverIds are required when creating a new suite without an environment", | ||
| } |
There was a problem hiding this comment.
This new branch changes server selection, manager priming, resolution-error mapping, and revision handoff, but the added tests cover only the environment CRUD/resolve routes; a repo-wide search finds no eval-route test that submits environmentId. Add happy-path and resolution/conflict coverage so regressions such as reconnecting the suite servers or dropping resolvedEnvironment are detected.
AGENTS.md reference: mcpjam-inspector/AGENTS.md:L53-L55
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b62b4fb — the gap was real, write-routes.test.ts had no eval-run test that sent environmentId. Added an environment-backed runs block covering exactly the four things that could silently regress:
- the manager connects the resolved closed set including plugin-contributed servers (
servers[].serverId, live-healed), caller-suppliedserverIdsare ignored, andtestSuites:getSuiteRunServerSelectionis never consulted; - the resolution object is handed to
prepareEvalRunasresolvedEnvironment, so the run pins the revision whose servers were just connected rather than resolving a second time; - a launch-resolution failure maps to 409
CONFLICTwith theENV_*reason indetails.code(and the manager is never built); ENV_NOT_FOUNDmaps to 404, not a conflict.
Plus a new-suite-with-environmentId-and-no-serverIds case for the lifted schema refinement.
| "environmentId": { | ||
| "type": "string", | ||
| "description": "Run against a project environment. The environment supplies the closed server set (so `serverIds` is not required and is ignored if sent), and the run is pinned to the revision resolved at launch \u2014 if the environment changes in between, the run is rejected with 409 rather than executing against a different configuration." |
There was a problem hiding this comment.
When creating a new suite with suiteName, inline tests, and environmentId but no serverIds, the route now explicitly accepts the request because the environment supplies its servers. However, EvalRunCreateRequest.anyOf still requires serverIds for every new-suite shape, so OpenAPI-generated clients and request validators reject this newly supported request despite the added environmentId property. Add an environment-backed new-suite alternative and update the stale request/server descriptions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b62b4fb. Added a third anyOf alternative (suiteName + tests + environmentId) so the environment-backed new-suite body validates, and un-staled the descriptions: the request-level one now names all three shapes, and serverIds says it is not required when environmentId is given (and is ignored if sent). The public-api.mdx endpoint table row got the same note.
| const page = await client.listEnvironments( | ||
| { projectId: project.id, includeArchived: true }, | ||
| { signal } |
There was a problem hiding this comment.
Because archiving explicitly frees a name for reuse, a project can contain an archived Staging and a new live Staging. Listing both here makes resolveByIdOrName report that name as ambiguous, so name-based get, resolve, update, and archive operations all fail even though live environment names are unique. Restrict normal operations to live rows and include archived rows only when resolving a restore target, or otherwise prefer the live match.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b62b4fb. You are right — assertNameAvailable skips archived rows, so a live and an archived Staging can coexist and the flat lookup called that name ambiguous.
resolveEnvironmentSelector now takes a prefer side: an exact ID still wins outright, then a name is resolved against the live rows for every operation except restore_project_environment (whose target is archived by definition), falling back to the full listing when the preferred side has no name match — so "get an archived env by name" still works and the not-found message still enumerates everything. A name that is ambiguous within the preferred side (two archived Stagings for a restore) still reports as ambiguous, because there it genuinely is. Covered by sdk/tests/platform/operations-environments.test.ts.
Internal previewPreview URL: https://mcp-inspector-pr-3425.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.
7 issues found across 20 files
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="cli/src/commands/environments.ts">
<violation number="1" location="cli/src/commands/environments.ts:47">
P3: Platform command timeout/error handling is now duplicated across four command modules, so future changes can make command behavior drift. A shared CLI platform-command helper would keep all surfaces on one implementation.</violation>
<violation number="2" location="cli/src/commands/environments.ts:410">
P3: Restore help currently suggests an invalid command: `environments get` has no `--include-archived` option. Point users to `environments get` (which already finds archived environments) or to `environments list --include-archived`.</violation>
</file>
<file name="mcpjam-inspector/server/routes/v1/environments.ts">
<violation number="1" location="mcpjam-inspector/server/routes/v1/environments.ts:228">
P2: Non-member create/update/archive/restore requests return `404 NOT_FOUND` whenever Convex emits its normal `FORBIDDEN` message without “admin”, contrary to the documented 403 write contract. Preserve 404 for `NOT_FOUND`, but map structured `FORBIDDEN` writes to `403 FORBIDDEN` instead of inferring authorization state from message text.</violation>
<violation number="2" location="mcpjam-inspector/server/routes/v1/environments.ts:343">
P2: The `description` field in `updateEnvironmentSchema` uses only `.optional()` (no `.nullable()`), so callers cannot clear it with explicit `null` — they'd get a 400. All other clearable fields (`serverAttachmentId`, `skillSelection`, `pluginVersionIds`) use `.nullable().optional()` and correctly support `null`-clearing per the contract. If `description` is meant to be clearable (it's optional at creation, so clearing it to "no description" is a natural use case), add `.nullable()`. If it's intentionally set-only, the inconsistency with the rest of the schema should be documented so callers know they must send an empty string `""` instead of `null`.
If clearing via empty string is the intended design, `description` schema needs no change. But the discrepancy means a caller following the contract statement "explicit `null` = clear" will get a confusing 400 instead of having their description cleared — the mismatch should at minimum be documented so callers know `description` uses `""` instead of `null` for clearing.</violation>
</file>
<file name="mcpjam-inspector/server/routes/v1/evals.ts">
<violation number="1" location="mcpjam-inspector/server/routes/v1/evals.ts:296">
P2: The runtime validation now accepts a new-suite request with `environmentId` and no `serverIds` (the environment supplies its servers), but the OpenAPI `EvalRunCreateRequest.anyOf` definition still requires `serverIds` for every new-suite branch. OpenAPI-generated clients and request validators will reject this newly supported request shape. Consider adding an environment-backed new-suite alternative to the `anyOf` and updating the affected request/server descriptions.</violation>
<violation number="2" location="mcpjam-inspector/server/routes/v1/evals.ts:556">
P2: The `translateEnvironmentResolveError` function in evals.ts rebuilds the `details` object as only `{ code }` when returning a 409, while the equivalent `translateResolveError` in environments.ts spreads the full `data?.details` object. This means additional structured fields that the Convex backend includes (like `{ reason: "disabled" }`) are surfaced to callers of the environment resolve route but silently dropped when the same error occurs on an eval-run. If both routes serve the same public surface, the error detail should be preserved consistently across both.</violation>
<violation number="3" location="mcpjam-inspector/server/routes/v1/evals.ts:1427">
P2: The environment-backed eval run path (resolving the launch, overriding server selection, error mapping, and passing `resolvedEnvironment` to pin the revision) is untested. The new `environments.test.ts` covers CRUD and resolve routes, but no test submits `environmentId` to the eval-run endpoint. Adding at least a happy-path and a resolution-conflict test would catch regressions in server reconnection logic or a dropped `resolvedEnvironment`.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| if (code === "NOT_FOUND") { | ||
| return new WebRouteError(404, ErrorCode.NOT_FOUND, "Environment not found"); | ||
| } | ||
| if (code === "FORBIDDEN") { |
There was a problem hiding this comment.
P2: Non-member create/update/archive/restore requests return 404 NOT_FOUND whenever Convex emits its normal FORBIDDEN message without “admin”, contrary to the documented 403 write contract. Preserve 404 for NOT_FOUND, but map structured FORBIDDEN writes to 403 FORBIDDEN instead of inferring authorization state from message text.
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/environments.ts, line 228:
<comment>Non-member create/update/archive/restore requests return `404 NOT_FOUND` whenever Convex emits its normal `FORBIDDEN` message without “admin”, contrary to the documented 403 write contract. Preserve 404 for `NOT_FOUND`, but map structured `FORBIDDEN` writes to `403 FORBIDDEN` instead of inferring authorization state from message text.</comment>
<file context>
@@ -0,0 +1,579 @@
+ if (code === "NOT_FOUND") {
+ return new WebRouteError(404, ErrorCode.NOT_FOUND, "Environment not found");
+ }
+ if (code === "FORBIDDEN") {
+ if (structuredMessage && /admin/i.test(structuredMessage)) {
+ return new WebRouteError(403, ErrorCode.FORBIDDEN, structuredMessage);
</file context>
|
|
||
| const createEnvironmentSchema = z.strictObject({ | ||
| name: z.string().trim().min(1), | ||
| description: z.string().optional(), |
There was a problem hiding this comment.
P2: The description field in updateEnvironmentSchema uses only .optional() (no .nullable()), so callers cannot clear it with explicit null — they'd get a 400. All other clearable fields (serverAttachmentId, skillSelection, pluginVersionIds) use .nullable().optional() and correctly support null-clearing per the contract. If description is meant to be clearable (it's optional at creation, so clearing it to "no description" is a natural use case), add .nullable(). If it's intentionally set-only, the inconsistency with the rest of the schema should be documented so callers know they must send an empty string "" instead of null.
If clearing via empty string is the intended design, description schema needs no change. But the discrepancy means a caller following the contract statement "explicit null = clear" will get a confusing 400 instead of having their description cleared — the mismatch should at minimum be documented so callers know description uses "" instead of null for clearing.
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/environments.ts, line 343:
<comment>The `description` field in `updateEnvironmentSchema` uses only `.optional()` (no `.nullable()`), so callers cannot clear it with explicit `null` — they'd get a 400. All other clearable fields (`serverAttachmentId`, `skillSelection`, `pluginVersionIds`) use `.nullable().optional()` and correctly support `null`-clearing per the contract. If `description` is meant to be clearable (it's optional at creation, so clearing it to "no description" is a natural use case), add `.nullable()`. If it's intentionally set-only, the inconsistency with the rest of the schema should be documented so callers know they must send an empty string `""` instead of `null`.
If clearing via empty string is the intended design, `description` schema needs no change. But the discrepancy means a caller following the contract statement "explicit `null` = clear" will get a confusing 400 instead of having their description cleared — the mismatch should at minimum be documented so callers know `description` uses `""` instead of `null` for clearing.</comment>
<file context>
@@ -0,0 +1,579 @@
+
+const createEnvironmentSchema = z.strictObject({
+ name: z.string().trim().min(1),
+ description: z.string().optional(),
+ hostId: z.string().trim().min(1),
+ serverAttachmentId: z.string().trim().min(1).optional(),
</file context>
| description: z.string().optional(), | |
| description: z.string().nullable().optional(), |
| const message = (data as { message?: unknown }).message; | ||
| if (typeof code === "string" && code.startsWith("ENV_")) { | ||
| if (code === "ENV_NOT_FOUND" || code === "ENV_CROSS_PROJECT") { | ||
| return new WebRouteError( |
There was a problem hiding this comment.
P2: The translateEnvironmentResolveError function in evals.ts rebuilds the details object as only { code } when returning a 409, while the equivalent translateResolveError in environments.ts spreads the full data?.details object. This means additional structured fields that the Convex backend includes (like { reason: "disabled" }) are surfaced to callers of the environment resolve route but silently dropped when the same error occurs on an eval-run. If both routes serve the same public surface, the error detail should be preserved consistently across both.
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/evals.ts, line 556:
<comment>The `translateEnvironmentResolveError` function in evals.ts rebuilds the `details` object as only `{ code }` when returning a 409, while the equivalent `translateResolveError` in environments.ts spreads the full `data?.details` object. This means additional structured fields that the Convex backend includes (like `{ reason: "disabled" }`) are surfaced to callers of the environment resolve route but silently dropped when the same error occurs on an eval-run. If both routes serve the same public surface, the error detail should be preserved consistently across both.</comment>
<file context>
@@ -533,6 +537,41 @@ function isConvexNotVisibleError(error: unknown): boolean {
+ const message = (data as { message?: unknown }).message;
+ if (typeof code === "string" && code.startsWith("ENV_")) {
+ if (code === "ENV_NOT_FOUND" || code === "ENV_CROSS_PROJECT") {
+ return new WebRouteError(
+ 404,
+ ErrorCode.NOT_FOUND,
</file context>
| // | ||
| // Any caller-supplied `serverIds` are deliberately ignored for an | ||
| // environment run: the closed set is the point. | ||
| let environmentLaunch: ResolvedEnvironmentForLaunch | undefined; |
There was a problem hiding this comment.
P2: The environment-backed eval run path (resolving the launch, overriding server selection, error mapping, and passing resolvedEnvironment to pin the revision) is untested. The new environments.test.ts covers CRUD and resolve routes, but no test submits environmentId to the eval-run endpoint. Adding at least a happy-path and a resolution-conflict test would catch regressions in server reconnection logic or a dropped resolvedEnvironment.
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/evals.ts, line 1427:
<comment>The environment-backed eval run path (resolving the launch, overriding server selection, error mapping, and passing `resolvedEnvironment` to pin the revision) is untested. The new `environments.test.ts` covers CRUD and resolve routes, but no test submits `environmentId` to the eval-run endpoint. Adding at least a happy-path and a resolution-conflict test would catch regressions in server reconnection logic or a dropped `resolvedEnvironment`.</comment>
<file context>
@@ -1375,12 +1414,35 @@ evals.post("/projects/:projectId/eval-runs", async (c) => {
+ //
+ // Any caller-supplied `serverIds` are deliberately ignored for an
+ // environment run: the closed set is the point.
+ let environmentLaunch: ResolvedEnvironmentForLaunch | undefined;
let serverIds = body.serverIds ?? [];
let serverNames = body.serverNames;
</file context>
| // run legitimately sends zero serverIds. | ||
| .refine( | ||
| (body) => | ||
| body.suiteId || body.environmentId || (body.serverIds?.length ?? 0) > 0, |
There was a problem hiding this comment.
P2: The runtime validation now accepts a new-suite request with environmentId and no serverIds (the environment supplies its servers), but the OpenAPI EvalRunCreateRequest.anyOf definition still requires serverIds for every new-suite branch. OpenAPI-generated clients and request validators will reject this newly supported request shape. Consider adding an environment-backed new-suite alternative to the anyOf and updating the affected request/server descriptions.
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/evals.ts, line 296:
<comment>The runtime validation now accepts a new-suite request with `environmentId` and no `serverIds` (the environment supplies its servers), but the OpenAPI `EvalRunCreateRequest.anyOf` definition still requires `serverIds` for every new-suite branch. OpenAPI-generated clients and request validators will reject this newly supported request shape. Consider adding an environment-backed new-suite alternative to the `anyOf` and updating the affected request/server descriptions.</comment>
<file context>
@@ -282,19 +288,17 @@ const createEvalRunSchema = RunEvalsRequestSchema.omit({
+ // run legitimately sends zero serverIds.
+ .refine(
+ (body) =>
+ body.suiteId || body.environmentId || (body.serverIds?.length ?? 0) > 0,
+ {
+ message:
</file context>
| ); | ||
| } | ||
|
|
||
| async function runPlatformCommand<TOutput>( |
There was a problem hiding this comment.
P3: Platform command timeout/error handling is now duplicated across four command modules, so future changes can make command behavior drift. A shared CLI platform-command helper would keep all surfaces on one implementation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cli/src/commands/environments.ts, line 47:
<comment>Platform command timeout/error handling is now duplicated across four command modules, so future changes can make command behavior drift. A shared CLI platform-command helper would keep all surfaces on one implementation.</comment>
<file context>
@@ -0,0 +1,439 @@
+ );
+}
+
+async function runPlatformCommand<TOutput>(
+ options: PlatformOptions,
+ timeoutMs: number,
</file context>
| .requiredOption("--environment <id-or-name>", "Environment name or ID") | ||
| .requiredOption( | ||
| "--expected-revision <n>", | ||
| "The revision you last read (from `environments get --include-archived` via list)" |
There was a problem hiding this comment.
P3: Restore help currently suggests an invalid command: environments get has no --include-archived option. Point users to environments get (which already finds archived environments) or to environments list --include-archived.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cli/src/commands/environments.ts, line 410:
<comment>Restore help currently suggests an invalid command: `environments get` has no `--include-archived` option. Point users to `environments get` (which already finds archived environments) or to `environments list --include-archived`.</comment>
<file context>
@@ -0,0 +1,439 @@
+ .requiredOption("--environment <id-or-name>", "Environment name or ID")
+ .requiredOption(
+ "--expected-revision <n>",
+ "The revision you last read (from `environments get --include-archived` via list)"
+ )
+ .option("--project <id-or-name>", "Project name or ID")
</file context>
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
WalkthroughAdds project environment management across the v1 API, SDK, CLI, and documentation. The API supports listing, reading, resolving, creating, updating, archiving, and restoring environments with strict validation, project scoping, authorization, and revision conflicts. Eval runs can resolve and use an environment configuration. The CLI exposes matching commands and input modes, while OpenAPI, public API, error mappings, and route tests are updated. 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: 5
🧹 Nitpick comments (4)
docs/reference/openapi.json (1)
2186-2188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLet the cancel-run 409 speak the same language as the new
Conflictresponse.This PR introduces
#/components/responses/Conflictwith a schema and example; leaving this sibling 409 as a bare description means generated clients get no error body type for it. A$refcosts nothing and makes the contract uniform.♻️ Suggested alignment
"409": { - "description": "The run already finished and cannot be cancelled." + "$ref": "`#/components/responses/Conflict`" },🤖 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 `@docs/reference/openapi.json` around lines 2186 - 2188, Update the cancel-run 409 response definition to reference the existing `#/components/responses/Conflict` response instead of retaining only an inline description, ensuring it uses the shared schema and example consistently.mcpjam-inspector/server/routes/v1/__tests__/environments.test.ts (1)
158-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset the Convex mock behaviours, not just their call records.
vi.clearAllMocks()wipes call history but leaves implementations intact, so amockRejectedValueset in one test (e.g. Line 278, Line 543) survives into every later test until something overwrites it. Today that is harmless only because each subsequent test happens to re-stub the mock it exercises; a future test that reads without stubbing would inherit a stale rejection. Seeding a neutral default per test makes the suite order-independent.♻️ Suggested hardening of the per-test baseline
beforeEach(() => { vi.clearAllMocks(); + convexQueryMock.mockReset(); + convexMutationMock.mockReset(); + convexQueryMock.mockResolvedValue(null); process.env.CONVEX_URL = "https://convex.example.com"; validateGuestTokenMock.mockResolvedValue({ valid: false }); });🤖 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/__tests__/environments.test.ts` around lines 158 - 167, Update the beforeEach setup in the environments test suite to reset Convex mock implementations, not only call history. Seed validateGuestTokenMock and any other Convex mocks with neutral default behavior for each test so prior mockRejectedValue or other overrides cannot persist across tests, while preserving the existing environment-variable reset logic.cli/src/commands/environments.ts (1)
154-439: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSeven subcommands repeat the same dispatch boilerplate.
Each of
list/get/resolve/create/update/archive/restorerepeats the identicalgetGlobalOptions→runPlatformCommand→writeResultshape, differing only in the operation and its input object. Worth folding into a small generic dispatcher to cut ~250 lines of repetition and reduce the risk that a future subcommand forgets a step (e.g.,signalpassthrough orwriteResult(result, format)).♻️ Sketch of a shared dispatch helper
async function dispatch<TInput, TOutput>( op: PlatformOperation<TInput, TOutput>, options: PlatformOptions, command: Command, buildInput: () => TInput ): Promise<void> { const globalOptions = getGlobalOptions(command); const result = await runPlatformCommand( options, globalOptions.timeout, ({ client, signal }) => op.execute(buildInput(), { client, signal }) ); writeResult(result, globalOptions.format); }🤖 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 `@cli/src/commands/environments.ts` around lines 154 - 439, Extract the repeated getGlobalOptions → runPlatformCommand → writeResult flow in registerEnvironmentsCommands into a generic dispatch helper that accepts the operation, PlatformOptions, command, and an input builder, while preserving client/signal passthrough, timeout handling, and output formatting. Replace all seven subcommand action handlers’ duplicated dispatch blocks with this helper, keeping each command’s existing input construction and validation unchanged.mcpjam-inspector/server/routes/v1/environments.ts (1)
169-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
ENV_*→ HTTP status mapping across two files. BothtranslateResolveErrorandtranslateEnvironmentResolveErrorindependently classify the same ConvexENV_*codes into 404 (not-found codes) vs 409 CONFLICT (everything else);evals.ts's docstring even states it mirrors theenvironments.tsroute, but the mapping logic itself is copy-pasted rather than shared. A futureENV_*code added to only one side would make these documented "mirrored" behaviors quietly diverge.
mcpjam-inspector/server/routes/v1/environments.ts#L169-L189: extract this ENV_* classification (theRESOLVE_NOT_FOUND_CODESset + status/message mapping) into a small shared helper (e.g. inweb/errors.tsorservices/evals/environment-launch.ts).mcpjam-inspector/server/routes/v1/evals.ts#L548-L573: replace the inlined duplicate logic intranslateEnvironmentResolveErrorwith a call to the extracted shared helper.🤖 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/environments.ts` around lines 169 - 189, Extract the shared ENV_* error classification, including RESOLVE_NOT_FOUND_CODES and the 404/409 status and message mapping, into a reusable helper; update translateResolveError in mcpjam-inspector/server/routes/v1/environments.ts:169-189 to use it. Replace the duplicated logic in translateEnvironmentResolveError in mcpjam-inspector/server/routes/v1/evals.ts:548-573 with the same helper, preserving existing WebRouteError handling and fallback translation.
🤖 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 108-119: The --file "-" path in the environment command currently
uses synchronous fd 0 access, which can fail for piped or delayed stdin. Update
the options.file handling to read stdin asynchronously through process.stdin,
while retaining synchronous file reads for regular paths; validate the collected
stdin payload before command execution and convert read failures into the
existing usageError format.
In `@docs/reference/openapi.json`:
- Around line 6097-6119: Update serverAttachmentId, skillSelection, and
pluginVersionIds to express nullability as OpenAPI 3.1 type unions including
{"type":"null"}, removing nullable: true. For skillSelection, replace the allOf
wrapper around EnvironmentSkillSelection with an anyOf/schema union containing
the $ref and null schema, while preserving each field’s existing descriptions
and constraints.
In `@mcpjam-inspector/server/routes/v1/environments.ts`:
- Around line 68-83: The resolve DTO currently requires hostName and
hostConfigId and unconditionally serializes them. Update ResolvedEnvironmentRow
and toResolvedDto so both fields are optional and only included when the backend
provides values, matching the ResolvedEnvironmentForLaunch contract and
preserving their absence otherwise.
- Around line 191-237: Update translateConvexError to classify FORBIDDEN
failures using structured backend sub-codes, such as FORBIDDEN_NOT_ADMIN for the
403 response and FORBIDDEN_NOT_MEMBER for the 404 response, instead of testing
structuredMessage with /admin/i. Preserve the existing fallback behavior for
unrecognized forbidden sub-codes.
In `@mcpjam-inspector/server/routes/v1/evals.ts`:
- Around line 540-573: Update translateEnvironmentResolveError so its fallback
delegates non-WebRouteError, non-ENV_* failures to mapRuntimeError instead of
returning the raw error. Import mapRuntimeError from the existing web errors
module and preserve the current ENV_* mappings and WebRouteError passthrough.
---
Nitpick comments:
In `@cli/src/commands/environments.ts`:
- Around line 154-439: Extract the repeated getGlobalOptions →
runPlatformCommand → writeResult flow in registerEnvironmentsCommands into a
generic dispatch helper that accepts the operation, PlatformOptions, command,
and an input builder, while preserving client/signal passthrough, timeout
handling, and output formatting. Replace all seven subcommand action handlers’
duplicated dispatch blocks with this helper, keeping each command’s existing
input construction and validation unchanged.
In `@docs/reference/openapi.json`:
- Around line 2186-2188: Update the cancel-run 409 response definition to
reference the existing `#/components/responses/Conflict` response instead of
retaining only an inline description, ensuring it uses the shared schema and
example consistently.
In `@mcpjam-inspector/server/routes/v1/__tests__/environments.test.ts`:
- Around line 158-167: Update the beforeEach setup in the environments test
suite to reset Convex mock implementations, not only call history. Seed
validateGuestTokenMock and any other Convex mocks with neutral default behavior
for each test so prior mockRejectedValue or other overrides cannot persist
across tests, while preserving the existing environment-variable reset logic.
In `@mcpjam-inspector/server/routes/v1/environments.ts`:
- Around line 169-189: Extract the shared ENV_* error classification, including
RESOLVE_NOT_FOUND_CODES and the 404/409 status and message mapping, into a
reusable helper; update translateResolveError in
mcpjam-inspector/server/routes/v1/environments.ts:169-189 to use it. Replace the
duplicated logic in translateEnvironmentResolveError in
mcpjam-inspector/server/routes/v1/evals.ts:548-573 with the same helper,
preserving existing WebRouteError handling and fallback translation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0aaeb9b6-8c7d-4880-94dd-2849dd42f3fa
📒 Files selected for processing (20)
cli/src/commands/environments.tscli/src/index.tsdocs/cli/overview.mdxdocs/cli/reference.mdxdocs/reference/openapi.jsondocs/reference/public-api.mdxmcpjam-inspector/server/routes/v1/__fixtures__/error-codes.jsonmcpjam-inspector/server/routes/v1/__fixtures__/error-status.jsonmcpjam-inspector/server/routes/v1/__fixtures__/internal-to-v1-code.jsonmcpjam-inspector/server/routes/v1/__tests__/environments.test.tsmcpjam-inspector/server/routes/v1/environments.tsmcpjam-inspector/server/routes/v1/evals.tsmcpjam-inspector/server/routes/v1/index.tsmcpjam-inspector/server/routes/web/errors.tssdk/src/platform/client.tssdk/src/platform/errors.tssdk/src/platform/index.tssdk/src/platform/operations.tssdk/src/platform/types.tssdk/src/public-api/index.ts
| if (options.file !== undefined) { | ||
| let text: string; | ||
| try { | ||
| text = | ||
| options.file === "-" | ||
| ? readFileSync(0, "utf8") | ||
| : readFileSync(options.file, "utf8"); | ||
| } catch (error) { | ||
| throw usageError(`Failed to read --file "${options.file}".`, { | ||
| source: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Node.js fs.readFileSync(0) reliably read piped stdin on macOS, Linux, and Windows in current LTS versions, or does it still throw EAGAIN/EOF errors?
💡 Result:
Using fs.readFileSync(0) to read piped stdin in Node.js is not universally reliable across platforms and execution contexts, even in current LTS versions [1][2]. While it often works for simple piping, it remains susceptible to EAGAIN and other runtime errors depending on the state of the stdin stream [1][3][2]. Key considerations: EAGAIN Errors: On macOS and Linux, fs.readFileSync(0) can throw an EAGAIN error if the stdin file descriptor has been set to non-blocking mode [1][2]. This frequently occurs in environments where hooks, plugins, or third-party wrappers manage stdin or when multiple piped commands interact with Node.js [1][4][2]. Node.js synchronous file system APIs do not automatically retry operations when the kernel returns EAGAIN, causing the application to crash if not specifically handled [1][3]. Windows Compatibility: While historical issues causing EISDIR or other exceptions when reading from empty stdin on Windows have been largely addressed in modern LTS versions (e.g., v20.x and later) [5][6], the use of raw file descriptors remains less idiomatic and potentially fragile compared to using asynchronous stream-based APIs [7][6]. Reliability: Because of these platform-specific behaviors and the potential for non-blocking stream states, relying on fs.readFileSync(0) is generally discouraged for robust applications [1][3]. Developers encountering EAGAIN errors often implement custom retry logic (typically involving a loop with a small delay) [1][3] or prefer using asynchronous Readable streams (e.g., process.stdin) to avoid blocking and state-related errors [7][8]. For production-grade applications that must handle stdin reliably across all environments, using the asynchronous stream interface (process.stdin) or a dedicated library for synchronous input reading is recommended [7][8].
Citations:
- 1: EAGAIN crash in hook scripts: readFileSync(0) fails when stdin is non-blocking openai/codex-plugin-cc#120
- 2: https://stackoverflow.com/questions/71998849/consecutive-pipes-in-node-js-fail-using-process-stdin-and-process-stdout
- 3: https://stackoverflow.com/questions/40362369/stdin-read-fails-on-some-input
- 4: [QUESTION] How to prevent EAGAIN: resource temporarily unavailable when trying to read from stdin with
fs.readFileSync(0)after using read? npm/read#39 - 5: Accessing stdin with file descriptor 0 on Windows causes exception if there's no input / if stdin is closed nodejs/node#19831
- 6: Accessing stdin with file descriptor 0 on Windows causes exception if there's no input / if stdin is closed nodejs/node#19831
- 7: fs.readSync throws EOF Error instead of returning 0 (Windows, if STDIN is a pipe) nodejs/node#35997
- 8: https://stackoverflow.com/questions/8452957/synchronously-reading-stdin-in-windows
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- environments.ts relevant section ---\n'
sed -n '85,140p' cli/src/commands/environments.ts
printf '\n--- usages of loadJsonObject/options.json/json ---\n'
rg -n "loadJsonObject|options\.json|--json|stdin|readFile" cli/src/commands/environments.ts
printf '\n--- command declarations for create/update relevant flags ---\n'
rg -n "program\.command|create\(|update\(|json|stdin|file" cli/src/commands/environments.tsRepository: MCPJam/inspector
Length of output: 245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- environments.ts relevant section ---'
sed -n '85,140p' cli/src/commands/environments.ts
printf '%s\n' ''
printf '%s\n' '--- usages of loadJsonObject/options.json/json stdin read ---'
rg -n "loadJsonObject|options\.json|--json|stdin|readFile" cli/src/commands/environments.ts
printf '%s\n' ''
printf '%s\n' '--- command declarations for relevant flags ---'
rg -n "program\.command|create\(|update\(|json|stdin|file" cli/src/commands/environments.tsRepository: MCPJam/inspector
Length of output: 5130
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const { readFileSync, openSync, readSync, closeSync, F_OK } = require('node:fs');
const { constants } = require('node:constants');
const { spawnSync } = require('node:child_process');
console.log('process.stdin.isTTY:', process.stdin.isTTY);
console.log('readFileSync(0, utf-8) directly:', typeof readFileSync(0, 'utf8') === 'string' ? 'readable' : '!readable');
// Probe stdin FD flags on non-Windows without reading a payload.
const stdout = spawnSync('sh', ['-c', 'echo ${E2BIG:-x}' && '[ -r /dev/stdin ] && echo readable || echo unreadable'], { encoding: 'utf8' });
console.log('/dev/stdin probe:', stdout.stdout.trim(), 'stderr:', stdout.stderr.trim());
console.log('readFileSync(0, utf-8) after direct probe:', typeof readFileSync(0, 'utf8') === 'string' ? 'readable' : '!readable');
JSRepository: MCPJam/inspector
Length of output: 484
Avoid using readFileSync(0) for --file - stdin input.
This CLI path advertises -, --file -, and --json -, but reading fd 0 synchronously is platform-fragile: non-blocking stdin such as delayed or chained pipes can surface EAGAIN/other Node fs exceptions instead of the intended clean usage error. Consider reading through process.stdin asynchronously and validating the resulting payload before executing the command.
🤖 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 `@cli/src/commands/environments.ts` around lines 108 - 119, The --file "-" path
in the environment command currently uses synchronous fd 0 access, which can
fail for piped or delayed stdin. Update the options.file handling to read stdin
asynchronously through process.stdin, while retaining synchronous file reads for
regular paths; validate the collected stdin payload before command execution and
convert read failures into the existing usageError format.
| "serverAttachmentId": { | ||
| "type": "string", | ||
| "nullable": true, | ||
| "description": "`null` clears the pin and falls back to the host config's servers." | ||
| }, | ||
| "skillSelection": { | ||
| "allOf": [ | ||
| { | ||
| "$ref": "#/components/schemas/EnvironmentSkillSelection" | ||
| } | ||
| ], | ||
| "nullable": true, | ||
| "description": "`null` clears the pinned skill selection." | ||
| }, | ||
| "pluginVersionIds": { | ||
| "type": "array", | ||
| "minItems": 1, | ||
| "items": { | ||
| "type": "string" | ||
| }, | ||
| "nullable": true, | ||
| "description": "Pinned plugin VERSION IDs. Narrow by design: the plugin must be installed and enabled, the version must be `ready`, at most one version per plugin may be pinned, and none of its skills may carry supporting files. Not a general-purpose plugin list. `null` clears all pins." | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the declared OpenAPI version and inventory the mixed nullability idioms.
fd -t f 'openapi.json' | while read -r f; do
echo "== $f"
jq -r '.openapi // "NO openapi FIELD"' "$f"
echo "-- occurrences of 3.0-only \"nullable\":"
jq '[paths as $p | getpath($p) | select(type=="object" and has("nullable"))] | length' "$f"
echo "-- occurrences of 3.1-only \"const\":"
jq '[paths as $p | getpath($p) | select(type=="object" and has("const"))] | length' "$f"
echo "-- occurrences of union type arrays (3.1-only):"
jq '[paths as $p | getpath($p) | select(type=="object" and (.type|type=="array"))] | length' "$f"
done
# Locate any spec-drift/lint check that would have caught this.
rg -n --iglob '*.{json,yml,yaml,ts,js,mjs,sh}' -C2 'openapi.*(drift|lint|validate)|redocly|spectral' | head -50Repository: MCPJam/inspector
Length of output: 321
🌐 Web query:
Is the nullable keyword valid in OpenAPI 3.1 or replaced by type null
💡 Result:
Yes, the nullable keyword has been removed in OpenAPI 3.1 [1][2][3]. In its place, OpenAPI 3.1 utilizes standard JSON Schema functionality, which allows for type arrays to define nullable values [4][3]. To specify that a field can be a particular type or null, you use the type keyword with an array containing the desired type and the null type [1][4]. For example, a property that accepts either a string or null should be defined as follows [4]: type: - "string" - "null" This approach is part of the deeper integration of JSON Schema into OpenAPI 3.1, which avoids adding custom, redundant keywords like nullable [4][3].
Citations:
- 1: Remove "nullable" in OAS 3.1? OAI/OpenAPI-Specification#2244
- 2: Remove "nullable" entirely OAI/OpenAPI-Specification#2246
- 3: Update Proposal 003 "Clarify Nullable" to Promoted status OAI/OpenAPI-Specification#2529
- 4: https://learn.openapis.org/upgrading/v3.0-to-v3.1.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="docs/reference/openapi.json"
echo "== declared version =="
jq -r '.openapi' "$file"
echo "== surrounding pins (context lines) =="
sed -n '6088,6125p' "$file" | cat -n
echo "== referenced components =="
jq '{"EnvironmentSkillSelection": .components.schemas.EnvironmentSkillSelection}' "$file"
echo "== exact occurrences and line numbers for nullable fields =="
python3 - <<'PY'
import json
from pathlib import Path
file=Path("docs/reference/openapi.json")
data=json.loads(file.read_text())
for i, p in enumerate(data.get("paths", {}).items(), 1):
pass
matches=[]
def walk(node, path=()):
if isinstance(node, dict):
if "nullable" in node:
matches.append((*path, node))
for k,v in node.items():
walk(v, path+(k,))
elif isinstance(node, list):
for idx,v in enumerate(node):
walk(v, path+(idx,))
walk(data)
ctx_paths=sorted(set("/".join(str(p) for p in m[:-1]) for m in matches))
for cp in ctx_paths:
print(f"\n# {cp}")
PY
echo "== tooling references around openapi =="
rg -n --iglob '*.{json,yml,yaml,ts,js,mjs,sh}' -C2 'openapi|redocly|spectral|drift|lint|validate' | head -80 || trueRepository: MCPJam/inspector
Length of output: 2996
Express the documented null clears as OpenAPI 3.1 unions
This spec declares 3.1.0, so nullable should not be used. The documented null clears are invisible to validators/clients, while the same document already uses const and type unions for nullable semantics. For skillSelection, also avoid wrapping the optional $ref in allOf; use an anyOf/schema union with {"type": "null"}.
🤖 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 `@docs/reference/openapi.json` around lines 6097 - 6119, Update
serverAttachmentId, skillSelection, and pluginVersionIds to express nullability
as OpenAPI 3.1 type unions including {"type":"null"}, removing nullable: true.
For skillSelection, replace the allOf wrapper around EnvironmentSkillSelection
with an anyOf/schema union containing the $ref and null schema, while preserving
each field’s existing descriptions and constraints.
| type ResolvedEnvironmentRow = { | ||
| environmentRef: { environmentId: string; name: string; revision: number }; | ||
| hostId: string; | ||
| hostName: string; | ||
| hostConfigId: string; | ||
| serverAttachmentId?: string; | ||
| selectedServerIds: string[]; | ||
| effectiveServerIds: string[]; | ||
| pluginVersions: Array<{ | ||
| pluginId: string; | ||
| pluginVersionId: string; | ||
| name: string; | ||
| bundleHash: string; | ||
| }>; | ||
| servers: Array<{ serverId: string; name: string }>; | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any shared or canonical definition of the resolveEnvironmentForLaunch return shape.
rg -n "resolveEnvironmentForLaunch" --type=ts -C3
rg -n "hostConfigId|hostName" mcpjam-inspector/server/services/evals/environment-launch.ts mcpjam-inspector/server/routes/v1/environments.tsRepository: MCPJam/inspector
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -u
echo "Files:"
git ls-files | rg 'mcpjam-inspector/server/services/evals/environment-launch\.ts|mcpjam-inspector/server/routes/v1/environments\.ts|convex|api|schema|query' || true
echo
echo "Search identifiers broadly:"
rg -n "resolveEnvironmentForLaunch|ResolvedEnvironmentForLaunch|ResolvedEnvironmentRow|hostConfigId|hostName|ResolvedEnviron" . || trueRepository: MCPJam/inspector
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -u
echo "Exact tracked files:"
for f in \
"mcpjam-inspector/server/services/evals/environment-launch.ts" \
"mcpjam-inspector/server/routes/v1/environments.ts"
do
if git ls-files --error-unmatch "$f" >/dev/null 2>&1; then
echo "OK $f"
else
echo "MISSING $f"
fi
done
echo
echo "Target symbols in tracked files only:"
rg -n --glob '!node_modules/**' 'resolveEnvironmentForLaunch|ResolvedEnvironmentForLaunch|ResolvedEnvironmentRow|hostConfigId|hostName|toResolvedDto' \
mcpjam-inspector/server || true
echo
echo "Show relevant slices if present:"
for f in \
"mcpjam-inspector/server/services/evals/environment-launch.ts" \
"mcpjam-inspector/server/routes/v1/environments.ts"
do
if [ -f "$f" ]; then
echo "--- $f ---"
wc -l "$f"
sed -n '1,180p' "$f"
fi
doneRepository: MCPJam/inspector
Length of output: 20698
🏁 Script executed:
#!/bin/bash
set -u
echo "Locate Convex environment function definitions:"
rg -n --glob '!node_modules/**' 'resolveEnvironmentForLaunch|projectEnvironments' . | rg '\.[tj]s$|\.ts$' | sed -n '1,220p'
echo
echo "Candidate files from rg (tracked):"
rg -l --glob '!node_modules/**' 'resolveEnvironmentForLaunch|projectEnvironments' . | sed -n '1,200p'Repository: MCPJam/inspector
Length of output: 1346
🏁 Script executed:
#!/bin/bash
set -u
echo "Locate convex/projectEnvironments:"
fd -a 'projectEnvironments|environments' . | sed -n '1,160p'
echo
echo "Search backend-style convex references broadly:"
rg -n --glob '!node_modules/**' 'projectEnvironments:|projectEnvironments\.|import .*environment-launch|ResolvedEnvironmentRow|ResolvedEnvironmentForLaunch|resolveEnvironmentForLaunch' . | sed -n '1,240p'
echo
echo "If candidate convex files exist, show relevant slices:"
for f in $(fd 'projectEnvironments|environment' . | sed -n '1,40p'); do
echo "--- $f ---"
wc -l "$f" 2>/dev/null || true
rg -n -C 8 'resolveEnvironmentForLaunch|ResolvedEnvironmentRow|ResolvedEnvironmentForLaunch|hostName|hostConfigId' "$f" 2>/dev/null | sed -n '1,220p' || true
doneRepository: MCPJam/inspector
Length of output: 25499
Align the resolve DTO with the launch resolver contract.
ResolvedEnvironmentRow treats hostName and hostConfigId as required, but the shared ResolvedEnvironmentForLaunch type — and its test — allow them to be omitted. Since toResolvedDto writes both fields unconditionally, an omitted backend value can be serialized as undefined or null instead of remaining absent. Mark those fields optional and conditionally emit them to match the launch resolver contract.
🤖 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/environments.ts` around lines 68 - 83, The
resolve DTO currently requires hostName and hostConfigId and unconditionally
serializes them. Update ResolvedEnvironmentRow and toResolvedDto so both fields
are optional and only included when the backend provides values, matching the
ResolvedEnvironmentForLaunch contract and preserving their absence otherwise.
| /** | ||
| * CRUD failures. The backend throws structured `ConvexError` data with a | ||
| * `code`, so branch on that first and fall back to message sniffing only for | ||
| * errors raised outside the module's own `fail()` helper. | ||
| * | ||
| * FORBIDDEN is deliberately split. "Not a project member" becomes 404 so the | ||
| * surface never confirms that a project id exists to someone outside it; the | ||
| * admin gate becomes a true 403, because the caller demonstrably CAN see the | ||
| * project and needs to know the block is their role, not a bad id. | ||
| */ | ||
| function translateConvexError( | ||
| error: unknown, | ||
| fallbackMessage = "Environment write rejected by the platform" | ||
| ): WebRouteError { | ||
| if (error instanceof WebRouteError) return error; | ||
| const data = convexErrorData(error); | ||
| const code = typeof data?.code === "string" ? data.code : undefined; | ||
| const structuredMessage = | ||
| typeof data?.message === "string" ? data.message : undefined; | ||
|
|
||
| if (code === "CONFLICT") { | ||
| return new WebRouteError( | ||
| 409, | ||
| ErrorCode.CONFLICT, | ||
| structuredMessage ?? "Environment changed since you loaded it." | ||
| ); | ||
| } | ||
| if (code === "VALIDATION") { | ||
| return new WebRouteError( | ||
| 400, | ||
| ErrorCode.VALIDATION_ERROR, | ||
| structuredMessage ?? fallbackMessage | ||
| ); | ||
| } | ||
| if (code === "NOT_FOUND") { | ||
| return new WebRouteError(404, ErrorCode.NOT_FOUND, "Environment not found"); | ||
| } | ||
| if (code === "FORBIDDEN") { | ||
| if (structuredMessage && /admin/i.test(structuredMessage)) { | ||
| return new WebRouteError(403, ErrorCode.FORBIDDEN, structuredMessage); | ||
| } | ||
| return new WebRouteError( | ||
| 404, | ||
| ErrorCode.NOT_FOUND, | ||
| "Environment or project not found, or you do not have access to it." | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate environments.ts =="
fd -a 'environments\.ts$' . | sed 's#^\./##'
echo
echo "== relevant file slice =="
file="$(fd 'environments\.ts$' . | head -n1)"
wc -l "$file"
sed -n '1,280p' "$file"
echo
echo "== convex-error helper definitions/usages =="
rg -n "convexErrorData|failure|fail\(|ConvexError|FORBIDDEN|NOT_MEMBER|NOT_ADMIN|admin" -S .Repository: MCPJam/inspector
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="mcpjam-inspector/server/routes/v1/environments.ts"
echo "== environments.ts translateConvexError slice =="
sed -n '146,260p' "$file"
echo
echo "== ConvexError construction in mcpjam workspace =="
rg -n "ConvexError|fail\\(|failures\\(\\)|\\.failures\\(" -g '*.{ts,tsx,js}' mcpjam-inspector/server mcpjam-inspector/src | head -n 120
echo
echo "== relevant tests for environments ConvexError mapping =="
test_file="mcpjam-inspector/server/routes/v1/__tests__/environments.test.ts"
if [ -f "$test_file" ]; then
sed -n '120,350p' "$test_file"
fi
echo
echo "== focused backend files likely referencing environments fail/admin =="
rg -n "environments|environment|admin|member|owner|FORBIDDEN|failures" -g '*.{ts,tsx,js}' mcpjam-inspector/server | rg -n "environment|environments|FORBIDDEN|failures|admin|member|owner" | head -n 200Repository: MCPJam/inspector
Length of output: 42070
Use structured FORBIDDEN sub-codes instead of message matching.
The current split between 404 and 403 relies on /admin/i.test(message), so a backend wording change can turn an admin-rejection into a misleading 404 even though the caller has project access. Emit backend sub-codes such as FORBIDDEN_NOT_MEMBER and FORBIDDEN_NOT_ADMIN to keep the route classification stable.
🤖 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/environments.ts` around lines 191 - 237,
Update translateConvexError to classify FORBIDDEN failures using structured
backend sub-codes, such as FORBIDDEN_NOT_ADMIN for the 403 response and
FORBIDDEN_NOT_MEMBER for the 404 response, instead of testing structuredMessage
with /admin/i. Preserve the existing fallback behavior for unrecognized
forbidden sub-codes.
| /** | ||
| * Map a launch-resolution failure onto the public envelope. The environment | ||
| * exists and is readable, but cannot currently produce a runnable | ||
| * configuration (a pinned plugin was disabled, the host was deleted, the | ||
| * closed server set came out empty) — that is a 409 conflict, not bad input, | ||
| * and the machine-readable `ENV_*` code rides along in `details` so callers can | ||
| * branch on the reason. Mirrors `/v1/projects/:p/environments/:e/resolve`. | ||
| */ | ||
| function translateEnvironmentResolveError(error: unknown): unknown { | ||
| if (error instanceof WebRouteError) return error; | ||
| const data = (error as { data?: unknown } | null)?.data; | ||
| if (data && typeof data === "object" && !Array.isArray(data)) { | ||
| const code = (data as { code?: unknown }).code; | ||
| const message = (data as { message?: unknown }).message; | ||
| if (typeof code === "string" && code.startsWith("ENV_")) { | ||
| if (code === "ENV_NOT_FOUND" || code === "ENV_CROSS_PROJECT") { | ||
| return new WebRouteError( | ||
| 404, | ||
| ErrorCode.NOT_FOUND, | ||
| "Environment not found" | ||
| ); | ||
| } | ||
| return new WebRouteError( | ||
| 409, | ||
| ErrorCode.CONFLICT, | ||
| typeof message === "string" | ||
| ? message | ||
| : "Environment cannot be launched right now.", | ||
| { code } | ||
| ); | ||
| } | ||
| } | ||
| return error; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Fallback path skips runtime-error classification, unlike the route it mirrors.
The docstring says this "Mirrors /v1/projects/:p/environments/:e/resolve", but the fallback differs: environments.ts's translateResolveError delegates non-ENV_* errors to translateConvexError, which applies mapRuntimeError for timeout/network patterns (yielding 504/502). Here, Line 572 (return error;) returns the raw error unchanged for any non-WebRouteError, non-ENV_* failure — e.g. a transient Convex network error during resolution would propagate unclassified instead of surfacing as a proper 5xx/502/504.
🔧 Proposed fix to restore parity with the mirrored route
return error;
}
+
+// Fallback for callers of this function's `return error` path — see call site.Simplest fix: replace the final return error; with return mapRuntimeError(error); (importing mapRuntimeError from ../web/errors.js) so an unclassified failure gets the same 502/504/500 treatment as elsewhere in the file.
🤖 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/evals.ts` around lines 540 - 573, Update
translateEnvironmentResolveError so its fallback delegates non-WebRouteError,
non-ENV_* failures to mapRuntimeError instead of returning the raw error. Import
mapRuntimeError from the existing web errors module and preserve the current
ENV_* mappings and WebRouteError passthrough.
…, OpenAPI shape Review follow-ups on the Project Environments public surface. Archiving frees the name, so a project can hold an archived "Staging" and a live "Staging" at once. `resolveEnvironmentSelector` listed both and handed the flat list to `resolveByIdOrName`, which called the name ambiguous — breaking name-based get/resolve/update/archive even though live names are unique. It now resolves a name against the side the operation is for (live for everything, archived for restore, which targets an archived row by definition), falling back to the whole listing so an archived-only name still resolves and the not-found message still enumerates everything. An exact ID still wins, and a name that is ambiguous within the preferred side still reports as ambiguous. `EvalRunCreateRequest.anyOf` still required `serverIds` on every new-suite shape, so generated clients and request validators rejected the newly supported `suiteName` + `tests` + `environmentId` body. Added that alternative and un-staled the `serverIds` / request descriptions. Added the missing eval-route coverage for environment runs: the closed set is connected (plugin-contributed servers included) and caller `serverIds` ignored, the suite's saved selection is never consulted, the resolution is handed to `prepareEvalRun` so the run pins that revision, a launch-resolution failure maps to 409 with its `ENV_*` reason, and an unreadable environment maps to 404. Plus SDK tests for the selector behavior above. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 5 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/__tests__/write-routes.test.ts">
<violation number="1" location="mcpjam-inspector/server/routes/v1/__tests__/write-routes.test.ts:461">
P2: `convexQueryMock.mockResolvedValue(RESOLVED_ENVIRONMENT)` in "accepts a new suite with an environment and no serverIds" makes ALL Convex queries resolve to the environment object. If the route handler or a service it calls makes additional queries (e.g., inside `resolveEnvironmentForLaunch`), they silently get `RESOLVED_ENVIRONMENT` too, masking misrouted or missing queries. The other test in this block uses `mockImplementation` with a discriminator, which is the safer pattern.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| it("accepts a new suite with an environment and no serverIds", async () => { | ||
| mockHappyCreate(); | ||
| convexQueryMock.mockResolvedValue(RESOLVED_ENVIRONMENT); |
There was a problem hiding this comment.
P2: convexQueryMock.mockResolvedValue(RESOLVED_ENVIRONMENT) in "accepts a new suite with an environment and no serverIds" makes ALL Convex queries resolve to the environment object. If the route handler or a service it calls makes additional queries (e.g., inside resolveEnvironmentForLaunch), they silently get RESOLVED_ENVIRONMENT too, masking misrouted or missing queries. The other test in this block uses mockImplementation with a discriminator, which is the safer pattern.
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/__tests__/write-routes.test.ts, line 461:
<comment>`convexQueryMock.mockResolvedValue(RESOLVED_ENVIRONMENT)` in "accepts a new suite with an environment and no serverIds" makes ALL Convex queries resolve to the environment object. If the route handler or a service it calls makes additional queries (e.g., inside `resolveEnvironmentForLaunch`), they silently get `RESOLVED_ENVIRONMENT` too, masking misrouted or missing queries. The other test in this block uses `mockImplementation` with a discriminator, which is the safer pattern.</comment>
<file context>
@@ -371,6 +371,163 @@ describe("v1 write routes", () => {
+
+ it("accepts a new suite with an environment and no serverIds", async () => {
+ mockHappyCreate();
+ convexQueryMock.mockResolvedValue(RESOLVED_ENVIRONMENT);
+
+ const res = await request(
</file context>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b62b4fba66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .strictObject({ | ||
| expectedRevision: z.number().int().nonnegative(), | ||
| name: z.string().trim().min(1).optional(), | ||
| description: z.string().optional(), |
There was a problem hiding this comment.
When an existing description needs to be removed, this schema rejects description: null with a 400, even though the established backend-facing editor sends exactly that clear value (ProjectEnvironmentEditor.tsx lines 189-190) and its mutation type explicitly permits it (useProjectEnvironments.ts line 106). The documented empty string is forwarded unchanged rather than translated to the backend's clear sentinel, so API and SDK callers cannot reliably clear this field; make description nullable and cover this null case alongside the other clearable fields.
AGENTS.md reference: mcpjam-inspector/AGENTS.md:L72-L74
Useful? React with 👍 / 👎.
This branch was successfully deployed
1 participant
Project Environments have existed in the backend and the hosted UI since #3405, but had zero public surface — and the v1 eval-run route actively rejected
environmentId. This adds the whole surface and makes environment runs work end to end.What a Project Environment is
A named, project-scoped, live-editable execution bundle that eval suites and journeys run against: one host + optional standalone server group + optional pinned skill selection + optional pinned plugin versions. Revisioned for optimistic concurrency.
Not a Computer sandbox image (
/images, renamed in #3418) and not the eval-suiteenvironmentservers bag. Nothing namedimages/PlatformImage*/computerEnvironments:*was touched.Changes
v1 routes — new
server/routes/v1/environments.ts:GET /projects/:p/environments?includeArchived=true— needed to find something to restorePOST /projects/:p/environmentsGET/PATCH…/:environmentIdexpectedRevisionPOST …/:environmentId/archive|/restoreDELETEGET …/:environmentId/resolveArchive/restore aren't a
DELETEbecause the backend keeps the row and restore has real semantics — it re-checks the name and drops irrecoverably-dead plugin pins. Routes stay off the guest allowlist. Reads need project membership; every write needs project admin, mapped to a real 403 (while non-membership stays a 404, so project existence never leaks).Eval runs now accept
environmentId. This was the gap that mattered most — without it the API could create environments it couldn't use. The route resolves the environment before building the manager, connects its closed server set (including servers contributed by pinned plugin versions), and passes that same resolution toprepareEvalRunso the run is pinned to the revision whose tool snapshot was captured.SDK —
PlatformEnvironment*types,PlatformApiClientmethods, and seven MCP operations (list_project_environments,get_,resolve_,create_,update_,archive_,restore_). Not added toPLATFORM_CATALOG_OPERATIONS(the guest worker surface), matching hosts and images.CLI —
mcpjam environments {list,get,resolve,create,update,archive,restore}.OpenAPI + docs — full spec entries (the
openapi-drifttest requires them), CLI reference, overview, andpublic-api.mdx.Two contracts surfaced rather than smoothed over
expectedRevisionis required on every mutation, not re-read server-side. Silently supplying the current revision would convert the backend's safe concurrent-edit rejection into a last-write-wins clobber.PATCH keeps the tri-state: omitted = unchanged, explicit
null= clear, value = set. Empty arrays stay rejected — they are not a way to clear. Both directions are covered by tests.Error mapping
Errors branch on the structured
ConvexError.data.coderather than sniffing messages. NoteCONFLICTis overloaded backend-side — stale revision, duplicate live name, edit-archived, already-archived, not-archived — so all map to 409 and the backend's own message is surfaced instead of us asserting a cause. The/resolveendpoint maps theENV_*family, keeping the specific code indetails.code.This required adding
CONFLICT(409) to the v1 public contract, which had no code for "well-formed request, wrong resource state." That also fixes a live bug:ENVIRONMENT_REVISION_CONFLICTwas missing fromINTERNAL_TO_V1_CODE, so run-start revision conflicts were already reaching API callers as a 500.Verification
typecheck:clientandtsc -p cli/tsconfig.json: clean.openapi-drift: green.environments.test.ts: 29 tests — guest denial, DTO shape (noenvironmentIdleak),includeArchivedforwarding, tri-state PATCH, revision 409, admin-403 vs member-404, archive/restore preconditions, andENV_*resolve mapping.Pre-existing Prettier violations in
sdk/src/platform/errors.ts,show-servers.ts, and one block ofv1/evals.tswere left alone; only my own lines are formatted.Not in this PR
Journeys still have no public run path — the swarm runner is web-only and takes no
environmentId. Worth a follow-up.🤖 Generated with Claude Code
Note
Low Risk
CLI and documentation only; delegates to existing SDK platform operations with no server or auth logic changes in this diff.
Overview
Adds
mcpjam environmentsso hosted projects can be managed from the terminal: list, get, resolve, create, update, archive, and restore project environments (host + optional servers, skills, and plugin pins — distinct from Computerimages).The new command module follows the same platform CLI pattern as
hosts:--api-key/--api-url, SDK operation execution with timeouts, Zod validation, and--file/--jsonbodies where flags do not cover structured fields. Mutations require--expected-revisionfromget, with local validation so bad revision strings fail before the API. Create/update merge JSON with CLI flags (flags win); update documents clearing pins via explicitnullin JSON.cli/src/index.tsregisters the command group. Docs add theenvironmentsrow to the CLI overview and a full reference section (revision workflow, admin-gated writes, pinning rules).Reviewed by Cursor Bugbot for commit b62b4fb. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Expose Project Environments on the public surface (v1 API,
@mcpjam/sdk, and@mcpjam/cli) and allow eval runs to target anenvironmentId. Also fixes name-based selection across live vs archived environments and updates OpenAPI so environment-backed runs validate correctly.New Features
/resolve; supports?includeArchived; archive/restore are sub-actions.environmentId, resolve environment and connect its closed server set, pin run to the resolved revision.@mcpjam/sdk):PlatformEnvironment*types and operations (list/get/resolve/create/update/archive/restore).@mcpjam/cli):mcpjam environments {list,get,resolve,create,update,archive,restore}.Bug Fixes
CONFLICT(409) to the v1 error contract; mapENVIRONMENT_REVISION_CONFLICTto 409 instead of 500. Enforce optimistic concurrency: all mutations requireexpectedRevision; PATCH keeps omit/null/set tri-state.environmentId: ignore callerserverIds, connect the environment’s closed set (plugin-contributed servers included), pin the resolved revision, map launch-resolution failures to 409 withENV_*indetails, and return 404 for unreadable environments.suiteName+tests+environmentId(noserverIds), and refresh request descriptions.Written for commit b62b4fb. Summary will update on new commits.