feat(computers): host-editor computer toggle + bash capability gating + sign-in links (PR4a)#2561
Merged
Merged
Conversation
…mputers PR1)
Adds `computer?: null | { kind: "personal"; toolset: "bash"; workdir?: string }`
to the hostConfig v2 surface, following the progressiveToolDiscovery pattern:
absent (or null) omits the key entirely, so existing rows hash byte-identically
to pre-feature rows — confirmed by the untouched golden vectors in the
regenerated parity fixture.
- types: HostConfigComputer + input/canonical fields; public HostComputer,
HostJson.computer, HostInit.computer; exported from internal + public barrels
- canonicalize: null → undefined collapse, literal validation, unknown-key
rejection (hash hygiene), workdir trim with empty-after-trim → absent
- Host builder: property, constructor init, setComputer() sugar, round-trip
- eval wire: normalizeSdkEvalHostConfigForWire strips computer on both input
shapes — evals never carry a personal computer (mutable per-user state;
backend run-start rejection lands in PR2)
- tests: canonicalization absent/null/personal/workdir + validation, wire
stripping + hash independence, Host round-trips, new golden parity vector
Plan: mcpjam-backend docs/project-computers.md (MCPJam/mcpjam-backend#485).
https://claude.ai/code/session_01Jf7p3Q1YQN8sKzi3hTzcuh
Resolves the hostConfig v2 conflict with #2511 (builtInToolIds). Both 'computer' (this branch) and 'builtInToolIds' (main) are independent optional dimensions, so the resolution is a union: keep both fields in types.ts / canonicalize.ts, both golden rows in the fixture generator, and both describe blocks in the canonicalize test (the latter two auto-merged). Regenerated the parity fixture (17 vectors) and bumped EXPECTED_INPUT_HASH to the combined value. Added the .changeset entry for 'computer' that PR1 was missing (mirrors #2511's). https://claude.ai/code/session_01Jf7p3Q1YQN8sKzi3hTzcuh
Inspector half of PR3 (mcpjam-backend docs/project-computers.md). The control plane shipped in mcpjam-backend #489/#492/#494; this wires the chat bash tool and the browser terminal to it. - utils/computers/control-plane-client: typed fetch client for the backend's /computers/* routes over CONVEX_HTTP_URL — reserve (user bearer; polled by ensureComputerReady until ready, each poll counts as activity), sandbox-info / commands / terminal-sessions (shared data-plane secret). The vendor sandbox id only ever lives server-side. - utils/computers/terminal-token: verify-only HS256 mirror of the backend's mint (shared COMPUTERS_TERMINAL_TOKEN_SECRET, required purpose claim, fail-closed without the secret). - utils/built-in-tools/bash: the bash chat tool, modeled on exa-web-search. reserve -> sandbox-info -> E2B Sandbox.connect (auto- resumes paused boxes) + commands.run -> idempotent command log keyed on toolCallId. Non-zero exits are results, not errors; device-flow auth URLs (gh auth login etc.) are lifted into a structured authUrls field; needsApproval mirrors the host's requireToolApproval. E2B runner is injectable for tests. - routes/web/computer-terminal + index.ts: @hono/node-ws upgrade at GET /api/web/computers/terminal?token=... — local token verify, sandbox-info exchange, E2B pty.create bridge (binary frames = bytes, text frames = JSON control: resize/ping), session open/close recorded in Convex, PTY killed on socket close. Typed computer.terminal.pty_open_failed event added to the log catalog. - routes/web/chat-v2: merge the bash tool into the turn's built-in tools only when the SERVER-resolved chatbox runtime config carries computer (never the request body) and the actor is not a guest. Backend omits computer for guests and re-rejects them at reserve; this gate is belt and suspenders. - chatbox-runtime-config: carry the optional computer field. - deps: e2b ^2.29.0, @hono/node-ws ^1.3.1. Config (deployment env): E2B_API_KEY, COMPUTERS_TERMINAL_TOKEN_SECRET, COMPUTERS_DATA_PLANE_SECRET (same values as the Convex deployment), CONVEX_HTTP_URL (existing). Unconfigured -> bash tool returns a clean error and the terminal closes 4503; nothing falls open. Tests: 16 new (token verify contract incl. cross-population purpose rejection; bash tool happy path, reserve polling, non-zero exit + auth URLs, unconfigured/denied/provisioning errors; auth-URL detection). Server-scoped suite green except the 11 pre-existing Chromium-dependent mcp-app-browser-harness failures (browser_unavailable in this environment, untouched by this change). https://claude.ai/code/session_01Jf7p3Q1YQN8sKzi3hTzcuh
`computer` becomes the RESOURCE attachment ({ kind: "personal",
workdir? }); the capability the model gets on it moves to builtInToolIds
("bash"), the same catalog list every built-in tool uses — one capability
model, one editor UI, and future toolsets (files, ...) compose without a
parallel `toolset` union. Paired backend PR adds the catalog
requiresComputer flag + write-time invariant.
SDK (@mcpjam/sdk, changeset: minor):
- HostConfigComputer drops `toolset`; new HostConfigComputerInput accepts
the legacy key, and the canonicalizer validates-then-DROPS it, so
legacy input hashes identically to the new shape. setComputer default
is { kind: "personal" }. Eval-wire stripping unchanged.
- Golden parity fixture regenerated (canonical JSON for the computer row
no longer carries toolset; drift-guard hash bumped per the fixture's
own instructions). Hash change is safe: no shipped UI writes the field.
Server — the resolveHostTools refactor (the Layer-2 cleanup):
- built-in-tools/registry.ts is now THE single construction path from
host-config fields to runnable tools: resolveHostTools({ builtInToolIds,
computer }, ctx). All per-tool gates live here — bash requires the id
AND the computer resource AND a non-guest actor, inherits
requireToolApproval via ctx; web_search unchanged. Computer narrowing
(incl. legacy-toolset tolerance) lives here too (narrowHostComputer).
Deliberately thin: no surface flags; eval engines simply never pass
computer.
- web/chat-v2.ts: the hand-spliced bash merge is deleted — one resolver
call. Advertise condition intentionally strengthens from
"computer.toolset === bash" to "computer AND 'bash' id granted"
(no production delta: zero hosts carry computer pre-PR4).
- Mechanical call-site conversion: mcp/chat-v2, mcpjam-agent,
evals-runner x4, sessionSimulation (none pass computer — behavior
unchanged). safeResolveBuiltInTools/resolveBuiltInTools removed.
- chatbox-runtime-config type: computer.toolset optional (tolerated,
ignored — pre-split backend rows still carry it vestigially).
Tests: registry resolver suite (bash needs id+computer, guest skip,
computer-alone does NOT advertise, unknown-id skip, null-ctx, bearer
normalization), narrowHostComputer cases, SDK canonicalize/host/eval
fixtures incl. legacy-input dedupe. SDK suite 1390 green; server suite
green except the 11 pre-existing Chromium-dependent harness tests.
https://claude.ai/code/session_01Jf7p3Q1YQN8sKzi3hTzcuh
Frontend for attaching a personal computer to a host, paired with the
catalog-driven bash capability. Config + chat surfaces only; the live
terminal panel / status chip / delete land in PR4b.
- client-config-v2: `computer?: { kind: 'personal'; workdir? }` on the
input + DTO types, threaded through emptyHostConfigInputV2,
hostConfigDtoToInput (reads only the resource shape — drops the
vestigial `toolset` the pinned-SDK backend still emits), and the
dirty-check equality.
- useBuiltInToolCatalog: catalog entry gains `requiresComputer` (matches
the backend DTO).
- ClientConfigEditor: a "Personal computer" toggle in the built-in tools
section, shown only when the catalog exposes a computer-backed tool
(so it stays hidden until the backend flips the dormant `bash` row to
enabled — no dead toggle pre-launch). Detaching the computer also
strips any computer-backed ids from builtInToolIds so the save can't
fail the backend's requiresComputer invariant.
- BuiltInToolCheckboxList: requiresComputer tools render disabled with a
hint until a computer is attached — the editor can't construct the
exact draft the backend rejects.
- tool-part: render a bash result's `authUrls` as clickable sign-in
links (device-flow logins like `gh auth login`).
Tests: computer dirty-detection + DTO toolset-stripping; checkbox-list
gating (disabled/enabled/click-blocked, web_search unaffected). Client
typecheck clean; full client suite green (3818).
https://claude.ai/code/session_01Jf7p3Q1YQN8sKzi3hTzcuh
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
chelojimenez
temporarily deployed
to
preview-pr-2561
GitHub Actions
Inactive
Contributor
Author
✅ 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. |
dosubot
Bot
added
size:XL
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds a first-class “personal computer” attachment to HostConfig v2 (types, DTO/input mapping, and equality), introduces shared host-config-computer helpers (attach/detach, detection, sanitization), wires a personal-computer toggle into editors and gates built-in tool checkboxes by tool.requiresComputer and eval-suite state, sanitizes eval-suite configs on load/save/reset, and adds safe external URL validation plus rendering of sign-in links from tool output. Most other edits are formatting/dependency-array rewrites and accompanying tests. 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 |
Contributor
…ting, stale repair 1. Untrusted auth URLs are now validated before rendering as links. New lib/safe-external-url filters tool-output `authUrls` to absolute https (http only for loopback) — javascript:/data:/vbscript:/file: and unparseable strings are dropped. tool-part uses it. 2. BehaviorTab (the redesigned host focus editor) is now wired: it gained the Personal computer toggle and passes computerAttached to the checkbox list, so Bash is no longer perma-disabled there. Detach logic shared with ClientConfigEditor via lib/host-config-computer. 3. Eval suites can no longer attach a computer: ClientConfigEditor hides the toggle and marks computer-backed tools disallowed for owner="eval-suite" (they render blocked), matching the backend's eval-run-start guard. 4. Stale invalid state is repairable: a selected-but-blocked computer- backed tool stays togglable (disabled only when NOT selected), so a `bash` id saved without a computer can always be unchecked. Detaching the computer also strips computer-backed ids. Shared helper lib/host-config-computer (attach/detach patch, catalog predicate) keeps the invariant identical across both editors. Tests: safe-external-url (https ok, loopback http ok, dangerous schemes dropped, dedup); checkbox-list stale-removable + eval-disallow; helper detach-strips-ids. Client typecheck clean; full client suite green (3831). https://claude.ai/code/session_01Jf7p3Q1YQN8sKzi3hTzcuh
chelojimenez
temporarily deployed
to
preview-pr-2561
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
mcpjam-inspector/client/src/lib/__tests__/safe-external-url.test.ts (1)
15-19: ⚡ Quick winIPv6 loopback (
[::1]) lacks test coverage.The implementation allowlists
[::1], yet no test exercises it. A single assertion would close the gap:expect(isSafeExternalLinkUrl("http://[::1]:8080/cb")).toBe(true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/lib/__tests__/safe-external-url.test.ts` around lines 15 - 19, Add a unit test asserting the IPv6 loopback is allowed by isSafeExternalLinkUrl: in the existing test case that checks loopback hosts (inside safe-external-url.test.ts) add an assertion calling isSafeExternalLinkUrl("http://[::1]:8080/cb") and expect it to be true so the implementation's allowlist for [::1] is covered.mcpjam-inspector/client/src/lib/safe-external-url.ts (1)
11-11: Remove unreachable"::1"fromLOOPBACK_HOSTS
new URL('http://[::1]/').hostnamenormalizes to"[::1]", whilenew URL('http://::1/')throwsTypeError, sourl.hostnamewill never be"::1"and that set entry is dead code.Suggested fix
-const LOOPBACK_HOSTS = new Set(["localhost", "127.0.0.1", "[::1]", "::1"]); +const LOOPBACK_HOSTS = new Set(["localhost", "127.0.0.1", "[::1]"]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/lib/safe-external-url.ts` at line 11, Remove the unreachable "::1" entry from the LOOPBACK_HOSTS Set constant so it contains only the normalized hostnames; update the declaration of LOOPBACK_HOSTS (the Set defined as LOOPBACK_HOSTS) to exclude "::1" and run/update any related tests or uses of LOOPBACK_HOSTS to ensure behavior remains correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@mcpjam-inspector/client/src/components/client-config/BuiltInToolCheckboxList.tsx`:
- Around line 16-36: Add a component-level read-only gate so parents can enforce
no-interaction: add a readOnly:boolean prop to BuiltInToolCheckboxList and in
the internal toggle handler and any per-checkbox onChange paths (e.g., the
toggle function and the checkbox input handlers around lines ~89-95)
early-return when readOnly is true, avoiding mutating selected or calling
onChange; keep rendering unchanged but ensure disabled visual/state is still
handled by existing computerAttached/computerToolsDisallowed logic.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/lib/__tests__/safe-external-url.test.ts`:
- Around line 15-19: Add a unit test asserting the IPv6 loopback is allowed by
isSafeExternalLinkUrl: in the existing test case that checks loopback hosts
(inside safe-external-url.test.ts) add an assertion calling
isSafeExternalLinkUrl("http://[::1]:8080/cb") and expect it to be true so the
implementation's allowlist for [::1] is covered.
In `@mcpjam-inspector/client/src/lib/safe-external-url.ts`:
- Line 11: Remove the unreachable "::1" entry from the LOOPBACK_HOSTS Set
constant so it contains only the normalized hostnames; update the declaration of
LOOPBACK_HOSTS (the Set defined as LOOPBACK_HOSTS) to exclude "::1" and
run/update any related tests or uses of LOOPBACK_HOSTS to ensure behavior
remains correct.
🪄 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
Run ID: d67f37ca-3c6a-4506-a7f1-a8270bc5e9d5
📒 Files selected for processing (9)
mcpjam-inspector/client/src/components/chat-v2/thread/parts/tool-part.tsxmcpjam-inspector/client/src/components/client-config/BuiltInToolCheckboxList.tsxmcpjam-inspector/client/src/components/client-config/ClientConfigEditor.tsxmcpjam-inspector/client/src/components/client-config/__tests__/BuiltInToolCheckboxList.test.tsxmcpjam-inspector/client/src/components/hosts/redesigned/focus/BehaviorTab.tsxmcpjam-inspector/client/src/lib/__tests__/host-config-computer.test.tsmcpjam-inspector/client/src/lib/__tests__/safe-external-url.test.tsmcpjam-inspector/client/src/lib/host-config-computer.tsmcpjam-inspector/client/src/lib/safe-external-url.ts
✅ Files skipped from review due to trivial changes (1)
- mcpjam-inspector/client/src/lib/tests/host-config-computer.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- mcpjam-inspector/client/src/components/chat-v2/thread/parts/tool-part.tsx
- mcpjam-inspector/client/src/components/client-config/ClientConfigEditor.tsx
…tps-only Addresses re-review of PR4a. Blocker — eval suites could still persist a hidden `computer`. Hiding the toggle didn't clear an existing value, and "Reset to project default" copied the project default (which may carry a computer) straight into the suite config, so the backend then aborted runs with "Computers are excluded from eval runs." New shared `sanitizeHostConfigForEvalSuite` (clears `computer`, strips computer-backed tool ids) is applied at all three boundaries in SuiteExecutionConfigEditor: load seed, reset-to- project-default, and save. The `computer` clear is catalog-independent (the field the backend guard keys on); id-stripping uses the catalog and re-runs when it loads. Returns the same reference when already clean, so no spurious dirty state. Secondary — auth links are now https-only. Dropped the loopback-http allowance: device-flow verification URLs are always https, and a clickable http://localhost link would point at the user's own machine, not the sandbox. Tests: sanitizer unit cases (computer cleared, ids stripped, catalog- independent clear, clean passthrough returns same ref); a SuiteExecutionConfigEditor regression test that mocks Convex and asserts "Reset to project default" with a computer-bearing default writes a config with no computer and no bash; safe-url https-only update. Client typecheck clean; full client suite green (3835). https://claude.ai/code/session_01Jf7p3Q1YQN8sKzi3hTzcuh
chelojimenez
temporarily deployed
to
preview-pr-2561
GitHub Actions
Inactive
| /** Patch that attaches a personal computer (the only MVP resource shape). */ | ||
| export function attachComputerPatch(): Partial<HostConfigInputV2> { | ||
| return { computer: { kind: "personal" } }; | ||
| } |
There was a problem hiding this comment.
Re-attach drops computer workdir
Low Severity
Turning the personal computer switch off and on again applies attachComputerPatch(), which always sets { kind: "personal" } and never carries over a workdir that was loaded from the saved host config. A round-trip toggle silently drops the configured working directory on the next save.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 6a5e6e0. Configure here.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
mcpjam-inspector/client/src/lib/__tests__/safe-external-url.test.ts (1)
59-63: 💤 Low valueConsider adding an explicit empty-array test case.
The implementation correctly returns
[]for an empty input array, but there's no explicit assertion documenting this behavior. A one-liner would round out the edge-case coverage:expect(filterSafeExternalLinkUrls([])).toEqual([]);Fits naturally alongside the existing non-array tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/lib/__tests__/safe-external-url.test.ts` around lines 59 - 63, Add an explicit test asserting that an empty array input returns an empty array: call filterSafeExternalLinkUrls([]) and expect it toEqual([]). Place this one-liner alongside the existing non-array tests in the safe-external-url.test.ts file (same it block) so the behavior for an empty-array edge case is documented and covered.mcpjam-inspector/client/src/lib/__tests__/host-config-computer.test.ts (1)
65-93: ⚡ Quick winConsider edge-case coverage for orphaned computer-backed tool IDs.
The tests comprehensively cover the main scenarios: computer present with/without catalog, and already-clean optimization. One defensive edge case remains untested: a value with no
computerbut has computer-backed tool IDs (e.g.,builtInToolIds: ["bash"]). While the UI prevents this inconsistent state, the sanitizer should strip orphaned IDs and return a new reference.🧪 Suggested test case
+ it("strips orphaned computer-backed ids even without computer", () => { + const value = emptyHostConfigInputV2({ builtInToolIds: ["bash"] }); + const out = sanitizeHostConfigForEvalSuite(value, CATALOG); + expect(out.computer).toBeUndefined(); + expect(out.builtInToolIds).toEqual([]); + expect(out).not.toBe(value); // new reference because value changed + });Note: The inline comment at lines 83–86 helpfully explains why catalog-independent clearing is safe; that documentation clarity is appreciated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/lib/__tests__/host-config-computer.test.ts` around lines 65 - 93, Add a test to cover the edge case where host config has no computer but includes computer-backed builtInToolIds (e.g., ["bash"]) by calling sanitizeHostConfigForEvalSuite(emptyHostConfigInputV2({ builtInToolIds: ["bash"] }), CATALOG) and asserting that the returned object is a new reference (not the same as input) and that builtInToolIds no longer contains the computer-backed id (expect it to equal [] or only non-computer ids); locate this behavior around the existing tests for sanitizeHostConfigForEvalSuite and use the same helpers (emptyHostConfigInputV2, CATALOG) so the sanitizer strips orphaned computer-backed IDs and returns a new object.
🤖 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/client/src/lib/__tests__/host-config-computer.test.ts`:
- Around line 65-93: Add a test to cover the edge case where host config has no
computer but includes computer-backed builtInToolIds (e.g., ["bash"]) by calling
sanitizeHostConfigForEvalSuite(emptyHostConfigInputV2({ builtInToolIds: ["bash"]
}), CATALOG) and asserting that the returned object is a new reference (not the
same as input) and that builtInToolIds no longer contains the computer-backed id
(expect it to equal [] or only non-computer ids); locate this behavior around
the existing tests for sanitizeHostConfigForEvalSuite and use the same helpers
(emptyHostConfigInputV2, CATALOG) so the sanitizer strips orphaned
computer-backed IDs and returns a new object.
In `@mcpjam-inspector/client/src/lib/__tests__/safe-external-url.test.ts`:
- Around line 59-63: Add an explicit test asserting that an empty array input
returns an empty array: call filterSafeExternalLinkUrls([]) and expect it
toEqual([]). Place this one-liner alongside the existing non-array tests in the
safe-external-url.test.ts file (same it block) so the behavior for an
empty-array edge case is documented and covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3c5f8040-38c7-4bce-bde2-247eee57a0c1
📒 Files selected for processing (6)
mcpjam-inspector/client/src/components/evals/__tests__/suite-execution-config-editor.computer.test.tsxmcpjam-inspector/client/src/components/evals/suite-execution-config-editor.tsxmcpjam-inspector/client/src/lib/__tests__/host-config-computer.test.tsmcpjam-inspector/client/src/lib/__tests__/safe-external-url.test.tsmcpjam-inspector/client/src/lib/host-config-computer.tsmcpjam-inspector/client/src/lib/safe-external-url.ts
…list
Two re-review patches on PR4a.
1. An attached computer could be impossible to detach. Both editors only
showed the toggle when the catalog had a requiresComputer tool, so a
config with `computer` set but Bash disabled/not-loaded had no detach
UI. The toggle now also shows when a computer is already attached
(shared shouldShowComputerToggle helper), and the section/FocusBlock
renders for the toggle even when the built-in tools list is hidden
(empty catalog). Still hidden on connection-only and eval surfaces.
2. BehaviorTab readOnly didn't reach the built-in tool checkboxes — the
switches respected it but BuiltInToolCheckboxList had no readOnly prop,
so checkboxes could still fire onChange. Added a readOnly prop (disables
every checkbox, blocks all edits incl. removing a selected tool) and
passed it from BehaviorTab.
Deferred (low priority, per review): toggling the computer off then on
drops `workdir`. There's no workdir input in the UI yet (the toggle writes
`{ kind: 'personal' }`), so it's not user-visible; it belongs with a
future workdir field.
Tests: shouldShowComputerToggle (catalog tool / attached / neither /
disallowed); checkbox-list readOnly disables + blocks edits. Client
typecheck clean; full client suite green (3840).
https://claude.ai/code/session_01Jf7p3Q1YQN8sKzi3hTzcuh
chelojimenez
temporarily deployed
to
preview-pr-2561
GitHub Actions
Inactive
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 03bd00d. Configure here.
Contributor
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 (2)
mcpjam-inspector/client/src/components/client-config/BuiltInToolCheckboxList.tsx (1)
107-110:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t promise an action the read-only state forbids.
When
readOnlyis true, this branch still renders “Uncheck to remove.” for a blocked selected tool, even though the checkbox is disabled. That guidance is false on the surface where it appears.Suggested fix
- {isSelected ? " Uncheck to remove." : ""} + {!readOnly && isSelected ? " Uncheck to remove." : ""}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/components/client-config/BuiltInToolCheckboxList.tsx` around lines 107 - 110, The span that shows the blockReason currently appends " Uncheck to remove." when isSelected is true even if the checkbox is disabled by readOnly; update the JSX in the blocked branch (where blocked, blockReason, isSelected are used) to only append that sentence when isSelected && !readOnly so the UI does not promise an action the read-only state forbids.mcpjam-inspector/client/src/components/client-config/ClientConfigEditor.tsx (1)
377-399:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDetaching before the catalog resolves can emit an invalid draft.
This toggle stays visible when a computer is already attached, but the off path still calls
detachComputerPatch(value, builtInToolCatalog). IfbuiltInToolCatalogis stillundefined, the helper removescomputerand strips zero computer-backed IDs, so a config that still containsbashcan become unsaveable until the catalog finishes loading.A safe fix is to block the detach transition until the catalog is known, or otherwise make the detach path conservative when tool metadata is unavailable.
One minimal guard
+ const detachComputerDisabled = + value.computer !== undefined && builtInToolCatalog === undefined; + {showBuiltInToolsSection || showComputerToggle ? ( <> <section className="space-y-4"> {showComputerToggle ? ( <div className="flex items-start justify-between gap-4"> @@ <Switch id={`${reactId}-computer`} checked={value.computer !== undefined} + disabled={detachComputerDisabled} onCheckedChange={(checked) => update( checked ? attachComputerPatch() : detachComputerPatch(value, builtInToolCatalog)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/components/client-config/ClientConfigEditor.tsx` around lines 377 - 399, The detach path can produce an invalid draft when builtInToolCatalog is undefined; change the onCheckedChange handler to avoid calling detachComputerPatch with an undefined catalog: either block the detach until builtInToolCatalog is available (e.g., ignore the off transition or disable the Switch when builtInToolCatalog is undefined) or call a conservative detach helper (implement detachComputerPatchSafe(value, builtInToolCatalog) or add a guard inside detachComputerPatch) that, when builtInToolCatalog is undefined, only clears the computer field (or returns a no-op patch) and does not strip any tool IDs; update the callsite (the Switch onCheckedChange that currently calls detachComputerPatch(value, builtInToolCatalog)) to use this guard/safe helper and keep attachComputerPatch unchanged.
🤖 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/client/src/components/client-config/BuiltInToolCheckboxList.tsx`:
- Around line 107-110: The span that shows the blockReason currently appends "
Uncheck to remove." when isSelected is true even if the checkbox is disabled by
readOnly; update the JSX in the blocked branch (where blocked, blockReason,
isSelected are used) to only append that sentence when isSelected && !readOnly
so the UI does not promise an action the read-only state forbids.
In `@mcpjam-inspector/client/src/components/client-config/ClientConfigEditor.tsx`:
- Around line 377-399: The detach path can produce an invalid draft when
builtInToolCatalog is undefined; change the onCheckedChange handler to avoid
calling detachComputerPatch with an undefined catalog: either block the detach
until builtInToolCatalog is available (e.g., ignore the off transition or
disable the Switch when builtInToolCatalog is undefined) or call a conservative
detach helper (implement detachComputerPatchSafe(value, builtInToolCatalog) or
add a guard inside detachComputerPatch) that, when builtInToolCatalog is
undefined, only clears the computer field (or returns a no-op patch) and does
not strip any tool IDs; update the callsite (the Switch onCheckedChange that
currently calls detachComputerPatch(value, builtInToolCatalog)) to use this
guard/safe helper and keep attachComputerPatch unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd061d69-766f-4aa5-b284-1a083867c07f
📒 Files selected for processing (6)
mcpjam-inspector/client/src/components/client-config/BuiltInToolCheckboxList.tsxmcpjam-inspector/client/src/components/client-config/ClientConfigEditor.tsxmcpjam-inspector/client/src/components/client-config/__tests__/BuiltInToolCheckboxList.test.tsxmcpjam-inspector/client/src/components/hosts/redesigned/focus/BehaviorTab.tsxmcpjam-inspector/client/src/lib/__tests__/host-config-computer.test.tsmcpjam-inspector/client/src/lib/host-config-computer.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- mcpjam-inspector/client/src/components/client-config/tests/BuiltInToolCheckboxList.test.tsx
- mcpjam-inspector/client/src/components/hosts/redesigned/focus/BehaviorTab.tsx
Bugbot: "Detach leaves bash without catalog." detachComputerPatch (and the eval-suite sanitizer) computed which ids to strip from the LIVE catalog's requiresComputer flag, which is undefined while loading and omits disabled rows — and `bash` ships disabled until launch. So detaching a computer could clear `computer` while leaving `bash` in builtInToolIds, violating the backend requiresComputer invariant with no checkbox to remove it when the built-in tools section is hidden. Fix: give the CLEANUP paths a small catalog-independent floor of known computer-backed ids (["bash"], mirroring the resolver's hardcoded BASH_TOOL_NAME) unioned with the catalog in computerBackedToolIds. Detach and eval-sanitize now strip bash regardless of catalog state. The toggle-visibility predicate (catalogHasComputerBackedTool) stays catalog-only, so a disabled tool can't resurrect a dead pre-launch toggle. Tests: detachComputerPatch strips bash with undefined / bash-less catalog; eval sanitize strips bash pre-catalog-load. Full client suite green (3841). https://claude.ai/code/session_01Jf7p3Q1YQN8sKzi3hTzcuh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Frontend for Project Computers — PR4a: the host-editor surface for attaching a personal computer, paired with the catalog-driven
bashcapability, plus the device-flow sign-in UX in chat. Config + chat only; the live terminal panel + status chip + delete land in PR4b (they need xterm.js + the WS client and are a clean separate slice). Follows the merged control plane (#489/#492/#494), data plane (#2556), and capability/resource split (#2558 + backend #497).What's here
computerin the client host-config model (client-config-v2.ts):computer?: { kind: "personal"; workdir? }on the input + DTO types, threaded throughemptyHostConfigInputV2,hostConfigDtoToInput, and the dirty-check equality. On read it takes only the resource shape — dropping the vestigialtoolsetthe backend still emits while pinned to the published SDK 1.15.The "Personal computer" toggle (
ClientConfigEditor): a Switch in the built-in-tools section that attaches/detaches the resource. Two correctness details:bashrow shipsenabled: falseuntil launch, the toggle stays hidden until that flip. No dead pre-launch control.builtInToolIds, so the editor can never produce the exact draft the backend rejects (requiresComputerwithout the resource).Capability gating (
BuiltInToolCheckboxList+useBuiltInToolCatalog): the catalog entry gainsrequiresComputer; computer-backed tools render disabled with a hint ("Requires a personal computer") until one is attached — mirroring the backend write-time invariant in the UI rather than letting the save fail.Sign-in links in chat (
tool-part.tsx): abashresult'sauthUrls(lifted server-side from device-flow output likegh auth login) render as clickable links instead of being buried in scrollback.How the three layers line up
The same invariant now holds at every layer: a computer-backed capability is only valid with the computer resource. UI disables the checkbox → editor strips ids on detach → backend
ensureHostConfigV2rejects the combination → runtime resolver (resolveHostTools) skips a bash id with no computer. Nothing downstream can be surprised.Tests
computerdirty-detection (attach/detach/workdir) + DTOtoolset-stripping; checkbox-list gating (disabled when unattached, click-blocked, enabled + toggles when attached,web_searchunaffected). Client typecheck clean; full client suite green (3,818 passed).Next (PR4b)
The xterm.js terminal panel speaking #2556's WS protocol (
mintTerminalToken→ connect → binary/JSON frames), a computer status chip (provisioning/ready/waking/hibernating viagetComputerStatus), and manual delete (deleteComputer). Plus, before real users: flip the backendbashcatalog row toenabledand ship the egress policy gated in the plan doc.https://claude.ai/code/session_01Jf7p3Q1YQN8sKzi3hTzcuh
Generated by Claude Code
Note
Medium Risk
Untrusted tool output becomes clickable links (mitigated by strict https-only validation) and host/eval config changes affect which capabilities run and what eval suites can save.
Overview
Adds Project Computers to the host-config UI: a optional
computerresource on v2 host config (attach/detach, dirty-check, DTO read strips legacytoolset).Personal computer toggle in
ClientConfigEditorand host BehaviorTab — shown when the catalog has a computer-backed tool or a computer is already attached (hidden on eval suites). Detaching clearsbash(and otherrequiresComputerids) via sharedhost-config-computerhelpers.Built-in tool checkboxes respect
requiresComputer: blocked until a computer is attached (or on eval suites), with hints; stale selected ids stay removable. Eval suite execution config sanitizes on load, save, and “reset to project default” so computers and computer-backed tools never persist.Chat tool output renders structured
authUrlsfrom bash/device-flow as external links, filtered throughsafe-external-url(https-only, nojavascript:/data:/http:localhost).Catalog type gains
requiresComputer; unit tests cover gating, sanitization, URL filtering, and computer dirty state.Reviewed by Cursor Bugbot for commit b6b1548. Bugbot is set up for automated code reviews on this repo. Configure here.