feat(ide): add /ide slash command with live IDE binding#652
feat(ide): add /ide slash command with live IDE binding#652edwinhu wants to merge 29 commits intoThe-Vibe-Company:mainfrom
Conversation
…+ opus alias The Claude CLI rejects `<family>-latest` compound forms (e.g. `claude-opus-latest`). Valid model IDs are either short aliases (`opus`, `sonnet`, `haiku`) or full version strings (`claude-opus-4-7`, `claude-sonnet-4-6`). Replaces the broken `claude-opus-latest` entry (added in e723252) with: - `claude-opus-4-7` — pinned latest Opus - `opus` — floating alias that auto-follows Anthropic's current Opus (preserves the original intent of e723252's fallback option) Adds a two-sided regression guard asserting `claude-opus-latest` is never re-introduced. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Claude Opus 4.7 returns thinking blocks with empty `thinking` text and a non-empty `signature` by design — the Anthropic API defaults `thinking.display` to `"omitted"` on 4.7, returning reasoning only as an opaque encrypted blob. The previous fallback "No thinking text captured." surfaced this as UI noise on every 4.7 turn. ThinkingBlock now returns null when the thinking text trims to empty, keeping the UI clean for 4.7 while preserving full behavior for models that return plaintext thinking (e.g. Opus 4.6). Upstream: the Claude Code CLI 2.1.112 does not expose a knob to override the display mode (tested --settings, env vars, --betas); getting readable thinking on 4.7 requires upstream CLI support for `thinking.display: "summarized"`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tures Claude Opus 4.7 removed `temperature`/`top_p`/`top_k` — sending any of them returns 400. Two Companion backend callers hit the Anthropic API directly (auto-namer for session titles, ai-validator for permission scoring) and unconditionally set `temperature`. Since `anthropicModel` is a free-text field in SettingsPage, a user configuring `claude-opus-4-7` (or the `opus` short alias) would get 400s on every auto-name and AI-validation call. - Add `supportsSamplingParams(model)` helper in settings-manager that treats `claude-opus-4-7` and `opus` as unsupported. - Gate `temperature` behind the helper in auto-namer.ts and ai-validator.ts. - Add regression tests for the helper and for auto-namer's request body. Also bumps stale model IDs in test fixtures and the Playground mock (`claude-opus-4-20250514` → `claude-opus-4-7`, `claude-sonnet-4-5` → `claude-sonnet-4-6`) for consistency with the active model set. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Adds an opt-in IDE integration: Companion discovers running Claude IDE
lockfiles under ~/.claude/ide/, exposes them via REST, and binds a
selected IDE to a live session by reusing the CLI's own mcp_set_servers
control channel. Users type /ide in the composer to open a picker; the
CLI never receives the slash command — it is intercepted client-side.
Server:
- ide-discovery: fs.watch + PID liveness pruning; emits ide:added/removed/changed
- ide-matcher: longest-prefix workspace match, mtime tiebreak
- ws-bridge.bindIde/unbindIde: sends mcp_set_servers with {ide: ws-ide|sse-ide};
auto-unbinds on ide:removed (BIND-04)
- REST: GET /api/ide/available, POST/DELETE /api/sessions/:id/ide
- session-store sanitizer strips authToken before disk persist (BIND-03)
- broadcasts {type: ide_list_changed} so open pickers live-refresh (DISC-03)
Frontend:
- Composer injects /ide via client-side overlay (no session.slash_commands
mutation), intercepts selection AND manual-typed submission; CLI never
sees /ide (BIND-06). Codex sessions get the row disabled with tooltip.
- IdePicker modal (createPortal, focus trap, axe-clean): 5 states covering
empty, single, many-with-best-match, currently-bound, bind-failed.
- ChatView renders IdeDisconnectBanner on binding non-null → null with the
exact BIND-05 copy ("IDE disconnected — rebind via /ide"), per-session
dismissal, no toast/modal.
- Playground registers the 5 picker states + banner for visual/axe coverage.
Includes scripts/probe-ide.ts for local diagnostics and an "IDE Channel"
appendix to WEBSOCKET_PROTOCOL_REVERSED.md documenting the ws-ide/sse-ide
MCP server type, lockfile schema, and bind mechanism (probed against
Claude CLI v2.1.112).
+111 tests (4948 → 5059), typecheck clean, full suite green.
Implemented by AI agent (Claude). Human review: no.
Co-Authored-By: Claude <noreply@anthropic.com>
|
@edwinhu is attempting to deploy a commit to the The Vibe Company Team on Vercel. A member of the Team first needs to authorize it. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
11 issues found across 52 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="web/src/components/ChatView.tsx">
<violation number="1" location="web/src/components/ChatView.tsx:45">
P2: IDE picker open state is not cleared on session change, allowing modal visibility/actions to leak into a different session context.</violation>
</file>
<file name="web/server/session-types.ts">
<violation number="1" location="web/server/session-types.ts:402">
P1: IDE auth token is included in browser-facing session payloads, creating a credential leakage path.</violation>
</file>
<file name="web/server/ide-matcher.ts">
<violation number="1" location="web/server/ide-matcher.ts:36">
P2: `isPathPrefix` uses an overbroad `startsWith('..')` check that falsely rejects valid descendants like `..cache`, leading to incorrect IDE candidate ranking.</violation>
</file>
<file name="web/server/ide-discovery.ts">
<violation number="1" location="web/server/ide-discovery.ts:118">
P2: Synchronous busy-wait retry blocks the Node event loop and can cause avoidable latency spikes during lockfile scans.</violation>
</file>
<file name="web/server/ide-matcher.test.ts">
<violation number="1" location="web/server/ide-matcher.test.ts:126">
P2: The boundary regression test is too weak: it only asserts that the exact match stays first, so a buggy partial-prefix match can still pass.</violation>
</file>
<file name="scripts/probe-ide.ts">
<violation number="1" location="scripts/probe-ide.ts:77">
P2: `parseLockfile` accepts non-positive PIDs, which can make `isPidAlive` return true due to process-group semantics of `process.kill(pid, 0)` and misclassify invalid lockfiles as live.</violation>
</file>
<file name="web/server/ws-bridge.ts">
<violation number="1" location="web/server/ws-bridge.ts:1069">
P2: `bindIde` marks IDE as bound even when backend is absent, causing persisted/UI binding state to diverge from actual CLI MCP configuration.</violation>
</file>
<file name="web/src/components/IdePicker.test.tsx">
<violation number="1" location="web/src/components/IdePicker.test.tsx:333">
P2: This test is a tautology and does not verify the claimed store-isolation contract, so it cannot catch regressions where IdePicker starts touching the store.</violation>
</file>
<file name="web/src/components/IdePicker.tsx">
<violation number="1" location="web/src/components/IdePicker.tsx:154">
P2: Global Enter handler overrides native button activation inside the dialog, causing incorrect action (bind) when keyboard users press Enter on focused controls.</violation>
<violation number="2" location="web/src/components/IdePicker.tsx:174">
P2: Busy-state protection is bypassed by keyboard actions, allowing concurrent bind/unbind requests.</violation>
<violation number="3" location="web/src/components/IdePicker.tsx:201">
P2: Disconnect failures reuse bind-error UI, so the visible Retry action triggers bind retry instead of retrying disconnect.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| /** Transport advertised in the lockfile. */ | ||
| transport: "ws-ide" | "sse-ide"; | ||
| /** Auth token from lockfile — runtime only, never persisted. */ | ||
| authToken?: string; |
There was a problem hiding this comment.
P1: IDE auth token is included in browser-facing session payloads, creating a credential leakage path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/server/session-types.ts, line 402:
<comment>IDE auth token is included in browser-facing session payloads, creating a credential leakage path.</comment>
<file context>
@@ -375,6 +378,34 @@ export interface BufferedBrowserEvent {
+ /** Transport advertised in the lockfile. */
+ transport: "ws-ide" | "sse-ide";
+ /** Auth token from lockfile — runtime only, never persisted. */
+ authToken?: string;
+ /** Epoch milliseconds when the binding was established. */
+ boundAt: number;
</file context>
Three issues surfaced when testing the merged feature on a live session: 1. Discovery never started — `startIdeDiscovery()` was defined but not invoked from the server bootstrap, so no lockfiles were ever watched. 2. Container sessions couldn't reach the host IDE — `bindIde()` hardcoded `127.0.0.1`, which is the container's own loopback. Use `host.docker.internal` when the session is containerized. 3. Only 1 of 10 IDE tools were exposed to the model (BIND-07). The Claude Code CLI binary contains a hardcoded filter (`_35`) that drops every `mcp__ide__*` tool except `getDiagnostics` and `executeCode`. Naming the MCP server "ide" triggered this prefix; renaming to the sanitized ideName (e.g. `neovim`) causes tools to be prefixed `mcp__neovim__*` which bypasses the filter. All 10 tools now reach the model. Live recording confirms the fix end-to-end. Regression test BIND-07 pins the sanitized-key contract so a future edit that reverts to "ide" fails loudly. BIND-01 and BIND-06 assertions were updated to match the new key shape. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Round 4–7 adversarial review fixes for PR The-Vibe-Company#652 /ide integration: - Track session.dynamicMcpServers so bindIde merges on Claude (full-replace wire) and unbindIde preserves user's other dynamic MCP servers via deleteKeys on Codex (per-key upsert). - Short-circuit bindIde when sanitized ideName is empty (prevents servers[""] orphan on the CLI). - Pre-drain session.pendingMessages in bindIde/unbindIde before direct adapter.send so a stale queued mcp_set_servers cannot replay and clobber the IDE entry. - Keep the dynamicMcpServers mirror in sync during flushQueuedBrowserMessages so the restore-from-disk path (pendingMessages persisted, dynamicMcpServers not) stays consistent. - Mirror-safe unbind payload shape: Claude drops IDE via omission, Codex drops IDE via deleteKeys. Also address earlier greptile/vercel/cubic review comments: - host.docker.internal for containerized sessions in bindIde URL. - auth token stripped from ide-session-routes HTTP responses and from broadcast session_update payloads (BIND-03). - ide-discovery: async sleep, scan generation counter, sync initial snapshot, post-watch catch-up, known map cleared on restart. - ide-matcher: proper prefix check (rel === ".." handling). - probe-ide: reject non-integer / non-positive pids. - IdePicker: busyRef flipped before await; lastFailedOp retry; Enter guard widened to all interactive elements. - ChatView: close IdePicker on session switch. - routes/ide-session-routes: map bind/unbind errors to HTTP 409. All 5095 tests pass; typecheck clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@cubic-dev-ai please re-review — the latest commit
Additional hardening from codex rounds 4–7:
5095/5095 tests pass, typecheck clean. |
@edwinhu I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
4 issues found across 61 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="web/src/ws.ts">
<violation number="1" location="web/src/ws.ts:1253">
P2: Global `ide_list_changed` event is emitted per-session message, causing duplicate IDE-list refetches when multiple session sockets are connected.</violation>
</file>
<file name="web/src/components/IdePicker.tsx">
<violation number="1" location="web/src/components/IdePicker.tsx:98">
P2: Concurrent `loadList` calls can resolve out of order and overwrite newer IDE list state with stale results.</violation>
</file>
<file name="scripts/probe-ide.ts">
<violation number="1" location="scripts/probe-ide.ts:132">
P2: Port derivation is filename-only and unvalidated, allowing invalid (`NaN`) bind URLs and inaccurate diagnostic payload output.</violation>
</file>
<file name="web/server/ws-bridge.ts">
<violation number="1" location="web/server/ws-bridge.ts:1547">
P1: `mcp_set_servers` from browser is sent to Claude without re-merging active IDE entry, so full-replace semantics can silently drop IDE tools and desync UI/backend state.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Four cubic-ai issues on commit 95666bd: - ws-bridge: browser mcp_set_servers on Claude now preserves the active IDE entry by re-merging session.dynamicMcpServers[ideKey] into the outbound payload (full-replace semantics would otherwise drop IDE tools when the user edits MCP via McpPanel while bound). deleteKeys honoring the IDE key still drops it. - ws (client): dedupe ide_list_changed on a monotonic generation from ide-discovery.scanGeneration instead of a 100ms time window, so fast add+remove sequences aren't silently dropped. - IdePicker: concurrent loadList calls now use an AbortController per call (aborts the prior in-flight request on supersede and on unmount) plus an epoch counter as defense-in-depth. AbortError is swallowed on both DOMException and Error shapes. api.getAvailableIdes gains an optional signal parameter. - probe-ide: port is validated as a positive integer < 65536 before use; malformed filenames (e.g. notanumber.lock) are skipped with reason. Tests: 5113 passing (+18 new: browser mcp_set_servers merge on Claude, generation-based dedupe across two sockets, abort-on-unmount + abort-on- supersede, api signal threading, probe() integration skip). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
2 issues found across 14 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="web/src/ws.ts">
<violation number="1" location="web/src/ws.ts:30">
P2: Generation-based dedupe can suppress legitimate IDE refresh events after server/discovery restart because client generation state is never reset.</violation>
</file>
<file name="scripts/probe-ide.ts">
<violation number="1" location="scripts/probe-ide.ts:156">
P2: New port-range gate conflicts with documented Neovim `<pid>.lock` naming and can skip live Neovim lockfiles when PID stem exceeds 65535.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- probe-ide: prefer JSON raw.port as authoritative over filename stem (Neovim names lockfiles by pid, not port); validate final port is integer in 1..65535. - ws.ts: reset generation-dedupe high-water mark on ws.onopen so server restarts don't suppress legitimate ide_list_changed events. - ws-bridge: drop unused ideRemovedUnsubscribe field (TS6133 in CI). - ide-routes.test: cover 409/500 error paths (coverage 78% -> 93%). - Test gaps: probe integration test for JSON-over-stem precedence; assert ideBinding stays null on POST-500 invalid-name path. All 5124 tests pass; typecheck clean; deadcode:check clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@cubic-dev-ai please re-review |
@edwinhu I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
3 issues found across 61 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="web/server/ws-bridge.ts">
<violation number="1" location="web/server/ws-bridge.ts:1166">
P1: IDE server key shares user MCP namespace, allowing key collisions that overwrite and later delete unrelated user MCP servers.</violation>
</file>
<file name="web/src/api.ts">
<violation number="1" location="web/src/api.ts:91">
P2: Expected fetch cancellations (AbortError) are being logged as API failures/exceptions after adding abortable GET, causing noisy and misleading telemetry.</violation>
</file>
<file name="web/src/components/ChatView.tsx">
<violation number="1" location="web/src/components/ChatView.tsx:102">
P2: IDE disconnect banner state can remain visible after rebind because visibility is not gated by current binding state and disconnect id is not cleared on reconnect.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Addresses cubic round-3 review on PR The-Vibe-Company#652. ws-bridge (P1 namespace collision): - Reserve `companion-ide-*` prefix for IDE-bound MCP server keys. The separator characters (`-`) are stripped by our ideName sanitization (`[^a-z0-9]`), so our keyspace is structurally disjoint from any key our sanitizer can emit (addresses codex round-4 BLOCK: `companionide` concatenation only moved the collision point). - Defend reserved namespace at every mirror write site: - routeBrowserMessage: strip `companion-ide-*` from user servers + deleteKeys before merge/mirror/send. - flushQueuedBrowserMessages: re-strip on replay so mirror protection is structural, not accidental (codex round-5 CONDITIONAL-GO fix). - bindIde/unbindIde bypass strip — they own the reserved key. - Tests: BIND-08a/b/c (namespace preservation), BIND-08d (legacy-name collision), BIND-08e/f/g (reserved-key stripping + prefix vs substring), BIND-08h (replay invariant), MIGRATE-01/02 (dynamicMcpServers not persisted). api.ts (P2 AbortError noise): - Add `isAbortError()` helper; gate trackApiFailure in post/get/put/ patch/del + raw bindIde fetch path. Abort-01/02/03 cover plain Error, non-abort network error, DOMException branches. ChatView.tsx (P2 stale disconnect banner): - Clear disconnectedBindingId + dismissedForBinding on null → non-null ideBinding transition so rebind removes the banner. BIND-09 pins. All 5138 tests pass; typecheck clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@cubic-dev-ai please re-review |
@edwinhu I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
3 issues found across 61 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="web/server/ws-bridge.ts">
<violation number="1" location="web/server/ws-bridge.ts:223">
P2: Auto-unbind on `ide:removed` silently drops failures when backend is disconnected, leaving stale `ideBinding` state with no retry.</violation>
</file>
<file name="web/server/ide-discovery.ts">
<violation number="1" location="web/server/ide-discovery.ts:122">
P2: Persistent `readdirSync` failures skip reconciliation, allowing stale IDE entries to remain visible indefinitely.</violation>
<violation number="2" location="web/server/ide-discovery.ts:207">
P1: Transient lockfile parse/read failures are treated as removals, causing spurious `ide:removed` events and downstream auto-unbind churn.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…ures Addresses cubic round-4 review on PR The-Vibe-Company#652. ide-discovery (P1): - Distinguish missing vs transient lockfile read failures via discriminated ReadResult union. On transient errors for a previously-known path, carry the prior entry forward instead of spuriously emitting ide:removed (which caused auto-unbind churn). - Bound carry-forward via per-path transientCounts counter (MAX_CONSECUTIVE_TRANSIENT_READS = 5). After the threshold, the entry is evicted so stale credentials can't be served indefinitely. Counter resets on any ok/missing read and on reconcile eviction. - Counter reset on ok-reads happens BEFORE PID/port validation (structural hardening — functionally equivalent under current control flow since reconcile clears on remove, but future-proofs against refactors). ide-discovery (P2): - Track consecutive readdirSync failures via readdirFailureStreak. After READDIR_FAILURE_THRESHOLD (3) persistent failures, evict all known entries so stale IDEs can't linger when the dir becomes unreachable. Streak resets on successful readdir. ws-bridge (P2): - Auto-unbind on ide:removed now awaits unbindIde; on { ok: false } (e.g. backend disconnected) invokes new private forceClearDeadIdeBinding that clears ideBinding, purges companion-ide-* from dynamicMcpServers, persists, and broadcasts session_update — without touching the adapter. Session state reflects reality even when the backend is dead. Tests: DISC-05a/b/c/d/e/f/g/h (9 in new ide-discovery-resilience.test.ts) + DISC-06a/b/c + BIND-10a/b. 5151/5151 pass; typecheck clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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="web/server/ide-discovery.ts">
<violation number="1" location="web/server/ide-discovery.ts:261">
P2: `readdirFailureStreak` is not reset on normal stop/start, so a new discovery session can inherit old failure history and evict IDEs too early.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Addresses cubic round-5 P2: module-level readdirFailureStreak inherited old failure history across discovery restarts, causing premature eviction when a new session hit its first readdir failure. Reset at three points: stopCurrent(), startIdeDiscovery() entry, and resetIdeDiscoveryForTests(). Added _getReaddirFailureStreakForTests accessor. DISC-06d pins the invariant. 5152/5152 tests pass; typecheck clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@cubic-dev-ai please re-review |
@edwinhu I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 62 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="web/src/components/Composer.tsx">
<violation number="1" location="web/src/components/Composer.tsx:192">
P2: Raw-text `/ide` interception hijacks any real backend slash command named `ide`, preventing it from being sent.</violation>
</file>
<file name="web/server/ide-discovery.ts">
<violation number="1" location="web/server/ide-discovery.ts:577">
P2: `listAvailableIdes()` leaks mutable references to internal `known` entries instead of returning an immutable/copy snapshot, allowing callers to corrupt discovery state and diffing.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@cubic-dev-ai please re-review —
Typecheck clean, 5165/5165 tests pass. One codex P2 remains (session rehydration of |
@edwinhu I have started the AI code review. It will take a few minutes to complete. |
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="web/server/claude-adapter.ts">
<violation number="1" location="web/server/claude-adapter.ts:428">
P2: `applyMcpSetServers` times out locally but does not unregister its pending control-request callback, which can leave stale entries in `pendingControlRequests` when no response arrives.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…meout Cubic P2 (identified by cubic) on 966db2b: `applyMcpSetServers` sets a 10s local timeout but did not remove the pending entry from `pendingControlRequests` when the timeout fired. A non-responding CLI would leak entries until the map reset on disconnect. Change `sendControlRequest` to return the generated request ID so callers with local timeouts can unregister the pending entry. The timeout closure now captures the id and `.delete()`s it before resolving `{ok: false}`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@cubic-dev-ai please re-review —
Typecheck clean, 5165/5165 tests pass. |
@edwinhu I have started the AI code review. It will take a few minutes to complete. |
The FolderPicker manual input tests waited only for the pencil button to render, not for the async loadDirs to resolve. On Ubuntu CI the browsePath was still empty when clicked, causing toHaveValue to fail. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@cubic-dev Please review this PR. |
@edwinhu I have started the AI code review. It will take a few minutes to complete. |
Adversarial Code Review -- PR #652Scope: ~11,900 lines added across 67 files. Adds IDE lockfile discovery, session-IDE binding/unbinding, live picker UI, and Verdict: WARNING -- No blocking security issues found. The authToken handling is thorough and correct. Several medium-priority issues warrant attention before or shortly after merge. CRITICAL IssuesNone found. The security posture is solid -- see Security section below. Security Assessment (PASS)The PR handles IDE
No hardcoded credentials, no injection risks, no path traversal -- the MCP server key namespace isolation ( HIGH PriorityH1. The 10-second timeout in Recommendation: Add an assertion or guard: H2. Race between
Mitigation: The risk is low in practice (requires a user editing MCP servers in the exact same event-loop tick as a bind), but consider adding a per-session mutex or "bind-in-progress" flag that makes MEDIUM PriorityM1.
M2.
M3. The regex M4. No rate limiting on M5.
M6. The function checks for SUGGESTIONS (Low Priority)S1. Large file: S2. Large file: S3. S4. Test-only exports gated by runtime check, not build-time S5. Magic numbers
These are well-commented but would benefit from being configurable via environment variables (matching the pattern of Test Coverage AssessmentExceptionally thorough. The PR includes:
One gap: no test for the H2 race (concurrent Summary
The security story is excellent -- the authToken handling is the most carefully defended credential flow I have seen in a PR of this size. The architecture (lockfile discovery -> event bus -> ws-bridge bind/unbind -> adapter -> CLI) is clean and well-documented. The two HIGH items are edge cases rather than exploitable bugs. This PR can merge with the H1 guard added and H2 documented as a known limitation. |
H1: Guard applyMcpSetServers against sendControlRequest returning undefined in a future refactor — early-return with error instead of leaking pendingControlRequests entries. H2: Add ideBindInProgress flag to prevent concurrent browser mcp_set_servers from racing bindIde/unbindIde. While the flag is set, routeBrowserMessage queues mcp_set_servers messages instead of forwarding them. Queued messages are flushed after the bind/unbind completes. Prevents split-brain on Claude's full-replace wire semantics. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@cubic-dev Please re-review — fixed both HIGH issues (H1: applyMcpSetServers guard, H2: ideBindInProgress race prevention). |
@edwinhu I have started the AI code review. It will take a few minutes to complete. |
Adversarial Review -- Round 2 (H1/H2 Fixes)Commit reviewed: [CRITICAL] C1: Postdrain flush of queued
|
| ID | Severity | Status | Description |
|---|---|---|---|
| C1 | CRITICAL | Must fix | Postdrain flush drops IDE entry on Claude full-replace wire |
| H1 | HIGH | Should fix | Dead-code guard on infallible sendControlRequest return |
| M1 | MEDIUM | Should fix | Silent tail-drop instead of enqueuePendingMessage |
| M2 | MEDIUM | Should fix | Zero test coverage for ideBindInProgress mechanism |
| L1 | LOW | Informational | forceClearDeadIdeBinding concurrent interaction |
Verdict: BLOCK -- C1 is a real data-loss path on Claude backends. The H2 mechanism correctly identifies the race window but the fix is incomplete because the queued message bypasses the IDE re-injection that routeBrowserMessage performs. The simplest fix is to move the queueing guard to after the IDE injection + stripping logic, so the serialized message is already complete when queued.
There was a problem hiding this comment.
1 issue found across 3 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="web/server/ws-bridge.ts">
<violation number="1" location="web/server/ws-bridge.ts:1802">
P2: `mcp_set_servers` updates can be silently dropped during IDE bind/unbind when pending queue is full.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…e position Container sessions now keep bypassPermissions — the container IS the sandbox. Only host sessions running as root are downgraded. Move the ideBindInProgress queueing guard in routeBrowserMessage to AFTER IDE entry injection and mirror update. Previously the guard queued the raw user message before injection, so flushed messages on Claude's full-replace wire would be missing the IDE entry (C1 from adversarial review round 2). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@cubic-dev Please re-review — fixed C1 (queueing guard position) and removed container permission downgrade (containers should have bypass by default). |
@edwinhu I have started the AI code review. It will take a few minutes to complete. |
Adversarial Review -- Round 3Reviewer: Claude Opus 4.6 (adversarial) CRITICAL: C2 --
|
flushQueuedBrowserMessages runs stripReservedIdeKeys on queued mcp_set_servers messages, which removes bridge-injected IDE entries. On Claude's full-replace wire this causes the CLI to silently drop the IDE MCP server. Add IDE entry re-injection in the flush path, mirroring the logic from routeBrowserMessage. The injection reads session.state.ideBinding at flush time, so it correctly handles both successful binds (entry re-injected) and failed binds (no injection, no ideBinding set). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@cubic-dev Please re-review — fixed C2 (IDE entry re-injection in flush path). |
@edwinhu I have started the AI code review. It will take a few minutes to complete. |
Adversarial Review -- Round 4 (C2 Fix)Commit: Verdict: APPROVE with one suggestionThe fix is correct and complete. It addresses the C2 split-brain scenario where What I verified1. Injection logic correctness vs The flush-path injection (lines 2052-2065) correctly mirrors the 2. Edge cases
3. Mirror consistency
4. No remaining split-brain scenarios The two paths that send Suggestion (LOW priority)Missing test for the core C2 scenario. The existing BIND-08h test (line 6850) covers stripping of spoofed keys during flush, but the C2 injection doesn't fire in that test because A test for this would:
Without this test, the C2 fix is only validated by manual reasoning (which checks out), not by automated regression coverage. Review provenance: Adversarial review by AI agent (Claude Opus 4.6). No human review. |
Re: Cubic P2 — mcp_set_servers dropped when queue full during bindThis is handled by
The bind window is at most ~10s (the No code change needed. |
|
@cubic-dev-ai Please re-review the latest commit (802d1b7 — C2 fix: IDE entry re-injection in flush path). |
@edwinhu I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
5 issues found across 68 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="web/server/ide-discovery-resilience.test.ts">
<violation number="1" location="web/server/ide-discovery-resilience.test.ts:1">
P2: DISC-05g/05h are non-red regression tests: the asserted counter cleanup is masked by reconcile cleanup, so the targeted transient-counter bug can regress without failing the test.</violation>
</file>
<file name="web/server/ide-discovery.ts">
<violation number="1" location="web/server/ide-discovery.ts:405">
P2: Transport-only lockfile updates are not treated as changes, so `ide:changed` is not emitted and clients may keep stale IDE transport mode.</violation>
</file>
<file name="web/server/codex-adapter.ts">
<violation number="1" location="web/server/codex-adapter.ts:1574">
P2: `applyMcpSetServers` can return `{ok:true}` even when requested server deletions fail, because delete errors are caught and not propagated in `runMcpSetServers`.</violation>
</file>
<file name="web/src/components/IdePicker.tsx">
<violation number="1" location="web/src/components/IdePicker.tsx:253">
P2: Retry after a disconnect failure can become a no-op when `currentBinding` turns null asynchronously, leaving a persistent error with a non-functional action.</violation>
</file>
<file name="web/server/claude-adapter.ts">
<violation number="1" location="web/server/claude-adapter.ts:442">
P2: `applyMcpSetServers` cannot surface explicit CLI control errors because `handleControlResponse` swallows `subtype === "error"` responses, causing a 10s timeout and misleading error message.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
1. DISC-05g/05h test isolation: restructured tests so transient
counter cleanup is not masked by reconcile eviction cleanup.
2. Transport-only lockfile changes now emit ide:changed so clients
see updated transport mode.
3. codex-adapter: applyMcpSetServers now returns {ok:false} when
server deletions fail instead of silently swallowing errors.
4. IdePicker: retry after disconnect failure works even when
currentBinding becomes null asynchronously.
5. claude-adapter: handleControlResponse now resolves pending
promises on error responses instead of swallowing them,
surfacing actual CLI errors instead of misleading timeouts.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@cubic-dev-ai Please re-review — all 5 P2 issues from your latest review are fixed in commit adcb543. |
@edwinhu I have started the AI code review. It will take a few minutes to complete. |
1 participant
Summary
/ideslash command to Companion: discovers running Claude IDE lockfiles (~/.claude/ide/*.lock), exposes them via REST, and binds a selected IDE to a live session by reusing the CLI'smcp_set_serverscontrol channel.Composer(primary inselectCommand, secondary inhandleSend) ensures the CLI never receives/ide. Codex sessions get the row disabled with a tooltip.ChatViewwith exact spec copy and per-binding dismissal (no toast/modal).Why
HANDOFF phases 0+1+2: users want in-session IDE context/diff/selection without manually editing MCP config or relaunching the CLI.
Testing
IdePicker(19),IdeDisconnectBanner,Composer(+8 IDE tests),ChatView(+8),Playgroundregistrationide-lockfile-contract.test.ts) pins lockfile schema against observed Claude CLI v2.1.112Review provenance
/workflows:dev