feat(chatbox): chatbox bash runs on a per-conversation ephemeral sandbox (Phase 4) - #3613
Conversation
An env-backed chatbox turn's `bash` bound to the ACTING MEMBER'S PERSISTENT PERSONAL COMPUTER: every chatbox conversation that member opened shared one box, with their project files already in it, and the boot image came from the member's computer row rather than from the environment the chatbox is bound to. Consumes mcpjam-backend #827 (deploys first). - chat-v2 reads the backend's `computerSandbox` state marker and, on `ephemeral`, get-or-creates the conversation's box and passes it as `ctx.sandboxBinding`. No release: the box lives for the conversation and the backend's idle reaper owns teardown. - Marker ABSENCE keeps today's personal-computer behaviour — malformed reads as absent too, so deploy skew can neither strip a working shell nor provision against a policy nobody stated. - Provision failure (503 at capacity, 409 no image) degrades to a turn with NO bash. The personal computer is never the fallback; that is the behaviour being replaced. Same for a turn with no `chatSessionId` — the conversation id IS the isolation boundary. - The blueprint image context is SUPPRESSED on an ephemeral binding: it is derived from the acting member's computer row and would describe a filesystem this turn's bash cannot see. A prompt that confidently describes the wrong machine is worse than no prompt. - `data-sandbox-notice` (mirroring `data-harness-reset`) surfaces the backend's two one-time notices as toasts. Exactly-once is the backend's job, so there is no client dedupe to desync. Also fixes an incomplete `vi.mock` in chat-v2.environment.test.ts that stubbed the whole chatbox-runtime-config module and left its pure readers undefined. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GitHub Actions
Inactive
dosubot
Bot
added
size:L
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_098fff24-4e60-4589-833d-20bc3163f9b4) |
📄 Knowledge reviewDosu skipped reviewing this PR because your organization has used its
|
✅ 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-3613.up.railway.app |
![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: e8170918a3
ℹ️ 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".
| if ( | ||
| computerSandboxMode === "ephemeral" && | ||
| chatboxId && | ||
| (resolvedExecution.builtInToolIds ?? []).includes(BASH_TOOL_NAME) |
There was a problem hiding this comment.
When an environment-backed chatbox also has resolvedExecution.harness, this condition still provisions an ephemeral sandbox and passes its bash through prepare.builtInTools; however, the harness path in run-harness-turn.ts separately calls resolveHarnessSandbox and runs its native Bash/Edit/Write tools on the attached personal computer. Such a turn therefore exposes two unrelated filesystems—and still exposes the personal computer despite the advertised isolation—so files created by one shell are invisible to the other. Gate this phase out for harness hosts, as the commit declares them out of scope, or bind the harness itself to the conversation sandbox before enabling it.
Useful? React with 👍 / 👎.
| const notices = (provisioned.value.notices ?? []).filter( | ||
| isSandboxNoticeReason | ||
| ); | ||
| if (notices.length > 0) sandboxNotices = notices; |
There was a problem hiding this comment.
When provisioning returns a notice but any downstream operation fails before onStreamWriterReady—for example MCP authorization/connection, message conversion, or tool preparation—the control plane has already marked the notice consumed here, yet no SSE part was sent. A retry then succeeds without the sandbox_reset warning, leaving the user unaware that prior files disappeared. The notice needs a durable handoff or an acknowledgment after it is actually written rather than being consumed during this early provisioning call.
Useful? React with 👍 / 👎.
WalkthroughThe change adds server-resolved sandbox modes and authenticated per-conversation sandbox provisioning. The chat route binds Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
mcpjam-inspector/server/routes/web/__tests__/chat-v2.chatbox-sandbox.test.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: ESLint configuration in --config is invalid:
mcpjam-inspector/server/routes/web/chat-v2.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: ESLint configuration in --config is invalid:
mcpjam-inspector/server/utils/computers/environment-context.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: ESLint configuration in --config is invalid:
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: 1
🧹 Nitpick comments (1)
mcpjam-inspector/server/utils/computers/control-plane-client.ts (1)
266-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared
SandboxNoticeReasontype instead of a parallel union.
ChatboxSandboxNoticeduplicates the literal union already defined asSandboxNoticeReasoninshared/sandbox-notice.ts. Two independent unions for the same concept can drift: add a third notice reason to one and the other stays stale, silently narrowingnoticesto the wrong set at the type level.Derive
ChatboxSandboxNoticefrom the shared type instead.♻️ Proposed fix
+import type { SandboxNoticeReason } from "`@/shared/sandbox-notice`"; + /** A one-time, user-visible fact about a chatbox conversation's sandbox. */ -export type ChatboxSandboxNotice = "sandbox_reset" | "stale_image"; +export type ChatboxSandboxNotice = SandboxNoticeReason;🤖 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/utils/computers/control-plane-client.ts` around lines 266 - 267, Update the ChatboxSandboxNotice declaration in control-plane-client.ts to reuse the shared SandboxNoticeReason type from shared/sandbox-notice.ts instead of defining a parallel literal union. Preserve the existing public alias name while deriving it directly from the shared type so future notice reasons remain synchronized.
🤖 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/web/chat-v2.ts`:
- Around line 685-743: Extend the sandbox handling around computerSandboxMode so
"unavailable" explicitly sets suppressComputerResource to true, preventing bash
from being advertised or falling back to the personal computer even when
hostRuntimeConfig still includes computer. Preserve the existing "ephemeral"
provisioning and failure behavior unchanged.
---
Nitpick comments:
In `@mcpjam-inspector/server/utils/computers/control-plane-client.ts`:
- Around line 266-267: Update the ChatboxSandboxNotice declaration in
control-plane-client.ts to reuse the shared SandboxNoticeReason type from
shared/sandbox-notice.ts instead of defining a parallel literal union. Preserve
the existing public alias name while deriving it directly from the shared type
so future notice reasons remain synchronized.
🪄 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: 948d9c06-cb29-4f61-804c-f84ad28bf418
📒 Files selected for processing (10)
mcpjam-inspector/client/src/hooks/use-chat-session.tsmcpjam-inspector/server/routes/web/__tests__/chat-v2.chatbox-sandbox.test.tsmcpjam-inspector/server/routes/web/__tests__/chat-v2.environment.test.tsmcpjam-inspector/server/routes/web/chat-v2.tsmcpjam-inspector/server/utils/__tests__/computer-sandbox-marker.test.tsmcpjam-inspector/server/utils/chatbox-runtime-config.tsmcpjam-inspector/server/utils/computers/control-plane-client.tsmcpjam-inspector/server/utils/web-chat-turn.tsmcpjam-inspector/shared/__tests__/sandbox-notice.test.tsmcpjam-inspector/shared/sandbox-notice.ts
![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 10 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- Exclude HARNESS turns from the ephemeral path. `run-harness-turn.ts` resolves its own machine via `resolveHarnessSandbox` (the member's personal computer) and receives `prepare.builtInTools` verbatim, so provisioning here produced a MIXED-MACHINE turn: the model's bash on the ephemeral box, the harness's Shell and edits on the personal one. It also suppressed the image context that IS correct for the harness's machine. Harness-on-chatbox is Phase 6; until then, hands off. - Treat an `unavailable` marker as authoritative. `suppressComputerResource` now starts from the marker, so a payload that ever carried both an `unavailable` marker and a `computer` resource still cannot expose the member's personal shell to a share-link-reachable chatbox turn. The backend's drop is its promise, not ours to depend on. - Provision AFTER the body validations and the manager authorization. Provisioning is the step that spends money, so a turn that is going to be rejected must be rejected before it can acquire a paid box. Nothing between the new position and `streamWebChatTurn` reads `builtInTools` or `effectiveSystemPrompt`, which is what makes the move free. - Give the chatbox caller honest tool copy. `buildSandboxBashTool` grew a `lifetime` discriminator; the default `run` wording is unchanged for evals and swarms, and the chatbox binding says the box PERSISTS across turns. A model told its files vanish won't build work up across turns — the whole point of a conversation-scoped shell. The notice-delivery window (consumed at provision, emitted at `onStreamWriterReady`) is narrowed by the reordering above but not closed; closing it needs a backend `peek`/`ack` split. Documented at the call site as a tracked follow-up rather than papered over with an in-memory replay buffer, which would lose notices on restart AND re-emit them on retry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dosubot
Bot
added
size:XL
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_e3da276c-e462-49cc-92b4-3d199ba0dff3) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49f1a32078
ℹ️ 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".
| const notices = (provisioned.value.notices ?? []).filter( | ||
| isSandboxNoticeReason | ||
| ); | ||
| if (notices.length > 0) sandboxNotices = notices; |
There was a problem hiding this comment.
When an idle conversation receives sandbox_reset, this code only forwards the notice as a transient SSE part for the client toast; it never adds the reset fact to effectiveSystemPrompt or the model messages. The model therefore receives the old transcript—which may say it created files or installed packages—without learning that those artifacts are gone, so it can continue reasoning or answering from nonexistent state even though the user was warned. Inject the reset notice into the current model context as well as emitting the client notification.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 85da59cae. Correct, and it's the more important half of the feature — I'd wired the human's toast and left the model reading a transcript that says it created files, against a filesystem that no longer contains them. That's the exact confabulation the notice exists to prevent, just relocated from the user to the model.
appendSandboxNoticeContext now injects model-facing copy into the turn's system prompt. Deliberately not the same string as the toast: the user needs to know what happened, the model needs to know what to do about it —
This conversation's sandbox was RESET since the last turn — it is a fresh machine. Any files you created, packages you installed, or processes you started earlier in this conversation are GONE, even though the transcript above says you made them. Do not assume anything from earlier turns still exists on disk: re-check with bash before relying on it, and tell the user plainly if work has to be redone.
Turn-injected, never persisted — persist.systemPrompt keeps the raw host prompt, so a resumed turn doesn't replay "your sandbox was reset" long after the fact (same rule as the blueprint image block). Tests: injects on reset (fails with the fix reverted), stays out of the persisted resume config, and no block at all when there are no notices.
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Closes the delivery gap the previous push only narrowed. Consumes mcpjam-backend #829 (deploys first). The control plane used to mark a notice delivered at PROVISION time, but the SSE writer does not exist until the engine starts streaming — so a throw in between (tool prep, engine dispatch, a dropped connection) destroyed it. Reordering shrank that window; it could not close it, and the correlation made the residue nasty: flaky setup and "the box was idle long enough to be reaped" share a cause, so the notice went missing exactly in the runs where a reset had actually happened. - `provisionChatboxSandbox` now declares `noticeAckVersion`, switching the backend to PEEK (hand over, consume nothing). - `emitSandboxNotices` acks after writing, and acks ONLY the chunks whose write succeeded — a chunk that threw was never delivered and must come back next turn. - `noticeAckPending: false` (a backend predating #829) leaves the ack callback unset, so that deploy behaves exactly as before. Unacked ⇒ re-delivered next turn. Duplicate display is now the worst case instead of silent loss, which is the right way round for "earlier files are gone": a repeated toast is noise, a missing one makes the model confabulate about a filesystem it cannot see. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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_727e2195-bb14-4907-8fb9-60962aa76ba1) |
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_a6b9cd5e-611c-49d3-ac23-3b0f17d9fef6) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b487b52555
ℹ️ 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".
| data: { reason }, | ||
| transient: true, | ||
| } as unknown as UIMessageChunk); | ||
| delivered.push(reason); |
There was a problem hiding this comment.
On the MCPJam/emulated path, onStreamWriterReady receives safeWriter from mcpjam-stream-handler.ts, whose write method catches an underlying controller failure—or returns immediately once streamClosed—without reporting failure. If the client disconnects before this initial notice is written, this line therefore still marks it delivered and the subsequent callback ACKs it, permanently consuming a notice that never reached the browser. Fresh evidence after the peek/ack change is this mismatch between emitSandboxNotices' no-throw assumption and the actual writer supplied by that engine; the writer must expose whether the chunk was accepted, or the notice must be ACKed through a path that does not swallow the write failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 85da59cae — and thank you, this was the sharpest catch on the PR.
You're right that the peek/ack change introduced the unsoundness. emitSandboxNotices inferred delivery from write() returning normally, but the emulated engine hands it safeWriter, which is deliberately no-throw:
write: (chunk) => {
lastWriteAt = Date.now();
if (streamClosed) return; // silent no-op
try { writer.write(chunk); } catch { streamClosed = true; /* never rethrows */ }
}So on a client disconnect the write "succeeded", we acked, and the notice was permanently consumed without ever reaching the browser — undercutting the fix on the one path it mattered most.
safeWriter now exposes isClosed(), and delivery is checked before and after each write (the write that discovers the stream is gone is the one that didn't land), plus abortSignal.aborted. Un-acked notices stay pending and re-deliver, so bailing early costs a duplicate at worst.
Two regression tests, both confirmed to fail with the fix reverted: "does NOT ack a notice written into a closed stream" and "does not ack the chunk whose write closed the stream".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mcpjam-inspector/server/routes/web/__tests__/chat-v2.chatbox-sandbox.test.ts (1)
471-502: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso assert that a failed notice write does not kill the turn.
The test proves the ack list is correct. It stays silent on the more consequential guarantee: a notice chunk that throws must not take the whole response down with it. A sandbox notice is decoration; the assistant's reply is the product.
Add a status assertion so a future refactor cannot let
socket closedescape the notice-emission path.🧪 Proposed additional assertion
const { app, token } = createWebTestApp(); - await postJson(app, "/api/web/chat-v2", BASE_BODY, token); + const response = await postJson(app, "/api/web/chat-v2", BASE_BODY, token); + // A notice is decoration. A write that throws must not fail the turn. + expect(response.status).toBe(200); expect(ackChatboxSandboxNoticesMock).toHaveBeenCalledWith( expect.objectContaining({ notices: ["sandbox_reset"] }) );🤖 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/web/__tests__/chat-v2.chatbox-sandbox.test.ts` around lines 471 - 502, Update the test “acks only the notices whose write succeeded” to assert that the POST request still returns a successful status after the second notice write throws. Capture the response from postJson and add a status assertion, preserving the existing acknowledgement assertion.
🤖 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.
Nitpick comments:
In
`@mcpjam-inspector/server/routes/web/__tests__/chat-v2.chatbox-sandbox.test.ts`:
- Around line 471-502: Update the test “acks only the notices whose write
succeeded” to assert that the POST request still returns a successful status
after the second notice write throws. Capture the response from postJson and add
a status assertion, preserving the existing acknowledgement assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b366a42d-aa12-41c9-bfdb-f0e293fcd959
📒 Files selected for processing (4)
mcpjam-inspector/server/routes/web/__tests__/chat-v2.chatbox-sandbox.test.tsmcpjam-inspector/server/routes/web/chat-v2.tsmcpjam-inspector/server/utils/computers/control-plane-client.tsmcpjam-inspector/server/utils/web-chat-turn.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- mcpjam-inspector/server/routes/web/chat-v2.ts
- mcpjam-inspector/server/utils/web-chat-turn.ts
There was a problem hiding this comment.
1 issue found across 4 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/web/chat-v2.ts">
<violation number="1" location="mcpjam-inspector/server/routes/web/chat-v2.ts:1142">
P2: A client disconnect after provisioning can consume a reset/stale-image notice without delivering it. `onStreamWriterReady` receives a write-error-swallowing writer before the engine checks `aborted`, so this callback ACKs a failed write; preserve the notice when the stream is already closed/aborted.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // behave exactly as before. | ||
| const sandboxRowId = provisioned.value.sandboxRowId; | ||
| if (provisioned.value.noticeAckPending && notices.length > 0) { | ||
| ackSandboxNotices = (delivered) => { |
There was a problem hiding this comment.
P2: A client disconnect after provisioning can consume a reset/stale-image notice without delivering it. onStreamWriterReady receives a write-error-swallowing writer before the engine checks aborted, so this callback ACKs a failed write; preserve the notice when the stream is already closed/aborted.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/server/routes/web/chat-v2.ts, line 1142:
<comment>A client disconnect after provisioning can consume a reset/stale-image notice without delivering it. `onStreamWriterReady` receives a write-error-swallowing writer before the engine checks `aborted`, so this callback ACKs a failed write; preserve the notice when the stream is already closed/aborted.</comment>
<file context>
@@ -1112,20 +1121,32 @@ chatV2.post("/", async (c) => {
+ // behave exactly as before.
+ const sandboxRowId = provisioned.value.sandboxRowId;
+ if (provisioned.value.noticeAckPending && notices.length > 0) {
+ ackSandboxNotices = (delivered) => {
+ void ackChatboxSandboxNotices({
+ bearer: bearerToken,
</file context>
There was a problem hiding this comment.
Fixed in 85da59cae — see the detailed reply on the web-chat-turn.ts:415 thread.
Short version: safeWriter now exposes isClosed(), and emitSandboxNotices consults it (plus abortSignal.aborted) before and after each write instead of trusting a clean return from a writer that is deliberately no-throw. An un-acked notice stays pending server-side and re-delivers next turn, so the failure direction is a duplicate toast rather than a lost reset warning.
…ed notices Two live review findings on the current head, both real, each reported independently by two reviewers. 1. The reset fact only ever reached the USER's toast. The model kept receiving the old transcript — in which it says it wrote files and installed packages — with no indication the filesystem had been wiped, so it went on reasoning against state that no longer exists. That is precisely the confabulation the notice exists to prevent, just relocated from the user to the model; warning only the human is half a fix. `appendSandboxNoticeContext` now injects model-facing copy (with an instruction, not just a status) into the turn's system prompt. Turn-injected, never persisted, same rule as the blueprint image block. 2. `emitSandboxNotices` inferred delivery from `write()` returning normally. On the emulated path that writer is `safeWriter`, which is deliberately no-throw: it swallows controller failures and silently no-ops once the stream is closed, so a client disconnect let us ack — and therefore permanently consume — a notice the browser never saw. That undercut the peek/ack fix on the exact path it mattered most. `safeWriter` now exposes `isClosed()`, and delivery is checked before AND after each write (the write that discovers the stream is gone did not land), plus the abort signal. Also spreads `...actual` into this suite's environment-context mock; a bare factory left the new pure formatter undefined and 500'd the route. Both new behaviours were confirmed to fail with the fix reverted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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_68581c1d-24f0-4d7b-afff-1666743dfee4) |
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_925c3a11-02fa-4c5c-a586-28c40b5ae559) |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcpjam-inspector/server/routes/web/chat-v2.ts (1)
1144-1152: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winAdd error handling to the fire-and-forget notice-ack call.
ackChatboxSandboxNoticesis invoked withvoidand no.catch(). A rejected control-plane response can still leave an unhandled promise rejection, and this server runs on Node 22 where unhandled rejections throw uncaught exceptions. Add a.catch()that logs the failure with the centralized logger. The notice remains pending and is re-delivered next turn, so the failure should not crash the process.🤖 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/web/chat-v2.ts` around lines 1144 - 1152, Update the ackSandboxNotices callback to attach a catch handler to the fire-and-forget ackChatboxSandboxNotices call. Log the rejection through the centralized logger while preserving the pending-notice behavior and preventing the error from propagating as an unhandled rejection.
🤖 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.
Outside diff comments:
In `@mcpjam-inspector/server/routes/web/chat-v2.ts`:
- Around line 1144-1152: Update the ackSandboxNotices callback to attach a catch
handler to the fire-and-forget ackChatboxSandboxNotices call. Log the rejection
through the centralized logger while preserving the pending-notice behavior and
preventing the error from propagating as an unhandled rejection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 35d63da1-bdd7-409e-8977-c39a3ee60db5
📒 Files selected for processing (5)
mcpjam-inspector/server/routes/web/__tests__/chat-v2.chatbox-sandbox.test.tsmcpjam-inspector/server/routes/web/chat-v2.tsmcpjam-inspector/server/utils/computers/environment-context.tsmcpjam-inspector/server/utils/mcpjam-stream-handler.tsmcpjam-inspector/server/utils/web-chat-turn.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- mcpjam-inspector/server/utils/web-chat-turn.ts
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/utils/web-chat-turn.ts">
<violation number="1" location="mcpjam-inspector/server/utils/web-chat-turn.ts:431">
P3: When a notice write throws on the non-sandbox writer paths (local/hosted org models, which pass writers without `isClosed`), the `catch` only logs and continues, so all remaining notices get written into the now-dead stream and each emits its own `[chat] sandbox notice stream write failed` warning. The new `closed()` logic only breaks for writers that expose `isClosed`. Consider breaking out of the loop in the catch as well (treating a throwing write as evidence the stream is gone), so only one warning is logged and the remaining pending notices are simply left to re-deliver next turn — matching the at-least-once intent.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| const closed = () => writer.isClosed?.() === true || abortSignal?.aborted; | ||
| const delivered: SandboxNoticeReason[] = []; | ||
| for (const reason of notices) { | ||
| if (closed()) break; |
There was a problem hiding this comment.
P3: When a notice write throws on the non-sandbox writer paths (local/hosted org models, which pass writers without isClosed), the catch only logs and continues, so all remaining notices get written into the now-dead stream and each emits its own [chat] sandbox notice stream write failed warning. The new closed() logic only breaks for writers that expose isClosed. Consider breaking out of the loop in the catch as well (treating a throwing write as evidence the stream is gone), so only one warning is logged and the remaining pending notices are simply left to re-deliver next turn — matching the at-least-once intent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/server/utils/web-chat-turn.ts, line 431:
<comment>When a notice write throws on the non-sandbox writer paths (local/hosted org models, which pass writers without `isClosed`), the `catch` only logs and continues, so all remaining notices get written into the now-dead stream and each emits its own `[chat] sandbox notice stream write failed` warning. The new `closed()` logic only breaks for writers that expose `isClosed`. Consider breaking out of the loop in the catch as well (treating a throwing write as evidence the stream is gone), so only one warning is logged and the remaining pending notices are simply left to re-deliver next turn — matching the at-least-once intent.</comment>
<file context>
@@ -385,33 +385,59 @@ export interface WebChatTurnRuntime {
+ const closed = () => writer.isClosed?.() === true || abortSignal?.aborted;
const delivered: SandboxNoticeReason[] = [];
for (const reason of notices) {
+ if (closed()) break;
try {
writer.write({
</file context>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1638791bb
ℹ️ 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".
| ); | ||
| suppressComputerResource = true; | ||
| } else { | ||
| const provisioned = await provisionChatboxSandbox({ |
There was a problem hiding this comment.
For an ephemeral chatbox using an Anthropic-compatible or Bedrock model, an MCP server that exposes an invalid tool name causes prepareChatV2 to return a 400, but this provisioning call has already booted the conversation's paid sandbox. Because each fresh conversation has a distinct sandbox and the box remains allocated until the idle reaper runs, repeated attempts against such a misconfigured chatbox consume capacity and incur cost without ever starting a model turn. Validate message conversion and the effective tool set before acquiring the sandbox, or otherwise release a newly provisioned box when preparation rejects the turn.
Useful? React with 👍 / 👎.
1 participant
What
An environment-backed chatbox's
bashnow runs on a per-conversation ephemeral sandbox booted from the environment's pinned image, instead of the acting member's persistent personal computer.Consumes the backend contract in MCPJam/mcpjam-backend#827.
Chatbox
bashworks today — on the acting member's personal project computer. Once both PRs are live, an env-backed chatbox turn runs in a disposable box:Host-backed chatboxes, Playground/host-bound turns, and env-backed chatboxes that pin no computer image are untouched.
Deploy ordering
Backend #827 deploys FIRST. This PR is inert until it does: the
computerSandboxmarker is absent on an older backend, and absence means "keep today's behaviour". Shipping this first is safe; shipping it never is also safe.Design notes
The binding is out-of-band. It rides
ctx.sandboxBinding(the seam B-isolation added), neverconfig.computer.narrowHostComputerruns at the top ofresolveHostToolsand rejects anything that isn'tpersonal, so a union on the config would be rejected — or, worse, wire-forgeable. The value can only be constructed in-process by a caller that just provisioned.Absence and malformation are the same third state.
readComputerSandboxModereturnsnullfor both. Reading absence asunavailablewould strip bash from every chatbox on any deploy skew; reading a malformed marker asephemeralwould provision a paid box against a policy nobody stated.Provision failure never falls back to the personal computer. A 503 (at capacity, or a sibling call still booting) or a 409 (no image) drops the
computerresource for the turn, so bash simply isn't advertised. Falling back is the bug this removes. Same for a turn that carries nochatSessionId: the conversation id is the isolation boundary (scopeKey = chatbox:<chatSessionId>), and binding without one would put every such session on one shared box.No release. The box belongs to the conversation, not the turn —
cd, write a file, run it three turns later. Releasing per turn would break every workflow a shell exists for. The backend's idle reaper owns teardown.The image-context prompt is suppressed, not rewritten.
maybeAppendEnvironmentContextderives its block from the acting member's computer row. On an ephemeral box that describes a different machine booted from a different image — it would invent packages, paths and maintenance commands the box does not have. A prompt that confidently describes the wrong filesystem is worse than no prompt, so the block is dropped whenever a binding is in play. It still appends on the personal path, where it is true.Notices ride a typed transient data part,
data-sandbox-notice, mirroringdata-harness-reset— the existing transport for "your shell state is gone, and here is why". Exactly-once delivery is the backend's job (it marks each notice consumed in the same transaction that hands it over), so the client renders whatever arrives and adds no dedupe of its own that a reconnect could desync. Unknown codes are dropped by the guard rather than rendering an empty toast.Drive-by
server/routes/web/__tests__/chat-v2.environment.test.tsstubbed the wholechatbox-runtime-configmodule with a bare factory, leaving its pure readersundefined. The suite passed only because the environment-target path never called them. Spread...actualso it stubs the network fetch and nothing else.Out of scope
Guests (no computer today, and the backend route refuses
GUEST_ISSUER). Harness on chatbox is Phase 6.Verification
New coverage (16 tests): bash binds to the conversation's sandbox and
buildBashToolis never reached; the same box across turns; a different box for a different conversation; provision failure ⇒ no bash and no error, and no personal fallback;unavailablemarker ⇒ no bash; missingchatSessionId⇒ no bash; absent marker ⇒ today's personal-computer behaviour, image context still appended; notices forwarded verbatim to the stream; unknown notice codes dropped; marker narrowing (readComputerSandboxMode) and the data-part guard.Note: one full-suite run exited 1 on a post-teardown
window is not definedoriginating inclient/src/components/oauth/__tests__/OAuthDebugCallback.test.tsx— every test file passed, it did not reproduce on re-run, and it is unrelated to anything here.🤖 Generated with Claude Code
Note
High Risk
Changes where chatbox shell runs, provisions paid sandboxes, and alters model/system prompt behavior; incorrect fallback or notice ack could cause wrong filesystem assumptions or lost reset warnings.
Overview
Phase 4 moves env-backed chatbox
bashoff the acting member's personal computer onto a per-conversation ephemeral sandbox from the environment image, keyed bychatSessionId. ThecomputerSandboxmarker (ephemeral/unavailable/ absent) is read viareadComputerSandboxMode; absent or malformed markers keep today's personal-computer path.When
ephemeralapplies (non-harness chatbox turns),chat-v2provisions after auth/validation, bindsbashthroughctx.sandboxBindingwithlifetime: "conversation", drops member image-context prompt, and on failure or missing session id runs without bash—no personal fallback. Harness chatbox turns are excluded so model bash and harness Shell stay on one machine.Notices (
sandbox_reset,stale_image) use peek/ack (noticeAckVersion: 1): transientdata-sandbox-noticetoasts, turn-only model context viaappendSandboxNoticeContext(not persisted), and ack only after writes that actually reach an open stream (safeWriter.isClosed). The client shows mapped copy inuse-chat-session.Extensive route and unit tests lock provisioning order, notice semantics, and backward compatibility.
Reviewed by Cursor Bugbot for commit d163879. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Env-backed chatbox
bashnow runs in a per-conversation ephemeral sandbox booted from the environment’s pinned image, replacing the member’s personal computer. Harness turns are excluded; sandbox notices reach both the user and the model with reliable peek/ack delivery.New Features
sandbox-bashto a conversation-scoped box withlifetime: "conversation"and suppress the personal image-context prompt on this path.data-sandbox-notice(sandbox_reset,stale_image) and inject turn-only model context; use peek/ack (noticeAckVersion: 1) and ack only notices actually delivered (checked via writer state). Legacy backends skip ack.computerSandbox.mode: "unavailable"as authoritative; provision after validation/authorization; on provision failure or missingchatSessionId, run withoutbashand never fall back to the personal box.Migration
MCPJam/mcpjam-backend#827and#829; deploy backend first. Older backends keep personal-computer behavior and legacy notice consumption (no ack path).Written for commit d163879. Summary will update on new commits.
Bot review response (2nd push)
Merged
origin/main(picks up theMCPJAM_SWARM_EPHEMERAL_BASHremoval, #3612).1 · [P1] Harness chatboxes must not enter the ephemeral path — FIXED. Verified real:
web-chat-turn.tsforwardsprepare.builtInToolsto the harness path explicitly, whilerun-harness-turn.tsseparately callsresolveHarnessSandboxfor the member's personal computer. That is a mixed-machine turn — model bash on one filesystem, harness Shell and file edits on another — and it also suppressed the image context that is correct for the harness's machine. Harness turns are now excluded from the marker read entirely, so today's behaviour is preserved end to end. Test added.2 · [P1]
unavailablemarker is now authoritative — FIXED.suppressComputerResourceinitialises from the marker instead of only being set inside theephemeralbranch. The backend does dropcomputerin that state, but that is its promise, not the inspector's to depend on. Test added that sends both anunavailablemarker and acomputerresource and asserts no bash of either kind.3 · [P1] No paid box for a turn that gets rejected — FIXED. The whole Phase 4 block (provision →
resolveHostTools→maybeAppendEnvironmentContext) moved below the body validations andcreateAuthorizedManager. Nothing between the new position and thestreamWebChatTurncall readsbuiltInToolsoreffectiveSystemPrompt, so the move is free. Test added: a malformedappToolsbody 400s with zero provision calls.4 · [P2] Notices consumed before a writer exists — NARROWED in the 2nd push, then fully CLOSED in the 3rd (see below). Original disposition: The reorder in #3 shrinks the window from "everything the route does" to "tool prep + engine dispatch", which is as far as an inspector-only change reaches. Closing it properly needs the backend to split provision into
peek+ackso delivery stays retryable until emission succeeds — and #827 shipped them fused, so that is a new backend PR. Documented at the call site with the reasoning, including why the tempting workaround is worse: an in-memory replay buffer would lose notices on restart and re-emit them on retry, which is strictly worse than the durable store already in place. Happy to open that backend PR if you'd like it before merge.5 · [P3] Tool description lifetime — FIXED.
buildSandboxBashToolgrew alifetime: "run" | "conversation"discriminator.runkeeps the exact existing copy (evals and swarms unchanged); the chatbox binding passesconversationand gets copy saying the box persists across turns, is private to the conversation, and is reclaimed after a long idle gap. Agreed this is behavioural, not cosmetic — a model that believes its files vanish won't build work up over several turns. Tests added for both variants.Verification
Net +6 tests (14 in the chatbox-sandbox route suite, 12 in the sandbox-bash tool suite).
3rd push — notice delivery gap CLOSED
Finding #4 above is no longer a documented gap. MCPJam/mcpjam-backend#829 splits provisioning into
peek+ack, and this push wires the inspector to it.Deploy ordering: backend#829 first, then this. Both directions are safe in the meantime — this build sends
noticeAckVersion: 1, which an older backend ignores (it consumes at provision as before and reportsnoticeAckPending: false, so no ack is attempted); and #829 is inert until a client declares that flag.What changed here
provisionChatboxSandboxdeclaresnoticeAckVersion, switching the backend to peek — hand the notice over, consume nothing.emitSandboxNoticesacks immediately after writing, and acks only the chunks whose write succeeded. A chunk that threw was never delivered, so acking it would consume a notice the user never saw.noticeAckPending: false(or absent, i.e. a deploy predating New IPC for OAuth banner #829) leaves the ack callback unset and the turn behaves exactly as it does today.Why it mattered enough for a second backend PR. The reordering in the previous push shrank the window from "everything the route does" to "tool prep + dispatch", but the residue was badly correlated: flaky setup and "the box was idle long enough to be reaped" share a cause, so the notice went missing precisely in the runs where a reset had actually happened. And the notice is "this conversation's sandbox was reset — earlier files are gone." Dropping it is the exact failure the two-phase handshake existed to prevent.
Semantics are now at-least-once, deliberately. Unacked ⇒ re-delivered on the next peek (no lease, no timer — the notices are idempotent by content). The worst case flips from silently losing a notice to occasionally showing one twice. A repeated toast is noise; a missing one makes the model reason confidently about a filesystem it can no longer see.
Verification
Chatbox-sandbox route suite 14 → 18. New: acks only after the write, and in that order; no ack when the turn dies before a stream writer exists (the gap itself); acks only the notices whose write succeeded; never acks against a pre-#829 backend while still displaying the notice.
4th push — two live findings on the current head
Both were reported independently by two reviewers, both were real, and both are fixed in
85da59cae. Individual replies are on the threads; summary here.A. The reset fact never reached the MODEL, only the user's toast. The SSE part warned the human while the model kept receiving a transcript in which it wrote files and installed packages, with no indication the filesystem had been wiped — the exact confabulation the notice exists to prevent, relocated from the user to the model.
appendSandboxNoticeContextnow injects model-facing copy (an instruction, not just a status) into the turn's system prompt. Turn-injected, never persisted, same rule as the blueprint image block.B.
emitSandboxNoticeswas acking notices that never reached the browser. It inferred delivery fromwrite()returning normally, but the emulated engine hands itsafeWriter, which is deliberately no-throw — it swallows controller failures and silently no-ops oncestreamClosed. On a client disconnect we therefore acked, and permanently consumed, a notice nobody saw. That undercut the peek/ack fix on the path where it mattered most.safeWriternow exposesisClosed(); delivery is checked before and after each write (the write that discovers the stream is gone did not land), plusabortSignal.aborted. Un-acked ⇒ still pending ⇒ re-delivered.Also: this suite's
environment-contextmock was a bare factory, which left the new pure formatterundefinedand 500'd the route — the same trap as thechatbox-runtime-configmock earlier in this PR. Spread...actual.The five cubic comments timestamped 03:23 are duplicates of findings 1–5, filed against the pre-fix head; GitHub has marked them outdated and each has a reply pointing at the resolving code.
Verification
Chatbox-sandbox route suite 18 → 23. Every new behaviour was confirmed to FAIL with its fix reverted, so none of them pass vacuously.