fix(vscode): make the extension's send path work, and close the security holes - #5987
Conversation
Replace the attach-only scaffold with a working agent chat extension: - Chat sidebar (codewhale.chat): create/switch/resume threads, live streaming over the replayable SSE contract, inline tool approvals, clarification questions, steer, and interrupt - Editor context chips (selection / active file / diagnostics) assembled into the prompt; CodeWhale: Ask Codewhale command, ctrl+alt+c keybinding, and editor context menu entry - Safe Markdown rendering (escaped subset) with per-block Copy and Insert-at-cursor actions - Runtime bearer tokens move to VS Code SecretStorage (CodeWhale: Set Runtime Token) with settings migration - Pure /v1 client (api.ts), SSE parser (sse.ts), and renderer (markdown.ts) split out of VS Code for direct testing The runtime remains the single turn/event owner; the extension only renders and routes. Full-feature flows (diff review, model switching, account sign-in) stay with the runtime's embedded browser client until their contracts land. Proof: extensions/vscode npm test -> 25 passed, 0 failed; packaged codewhale-vscode-0.10.0.vsix (95 KB, tests excluded); compiled client smoke-tested against the release runtime binary (health, auth-required detection, thread summaries, create, detail hydration, snapshots) with no model turns executed.
…ity holes
The extension had never successfully started a turn. `startTurn` accepted
only HTTP 200/202 while the runtime's `start_thread_turn`
(crates/tui/src/runtime_api.rs:4613-4632) returns `StatusCode::CREATED` as
its ONLY success path, so every send failed. `git log -- src/api.ts` is a
single commit: this was never a regression, it shipped that way and was
never run end to end.
api.ts (send path):
- Status handling now tests a RANGE (`isOk`: >= 200 && < 300) through one
`ensureOk` helper routed through every call site, rather than enumerating
codes at eleven of them. 201 is accepted because it is 2xx, not because it
is special-cased — the same shape the embedded web client already used at
crates/tui/src/runtime_web/app.mjs:873, which is why that client worked
against the same runtime this one choked on.
- The runtime's JSON `error.message` is surfaced on every route; previously
only startTurn passed it through.
- 409 is typed: a second send while a turn is live is "already running", and
interrupting when nothing streams is "nothing to stop", not an error.
Security (extension.ts, runtime.ts, secrets.ts):
- SecretStorage now wins over the settings token, matching what secrets.ts,
the manifest and the README all already promised. Previously a repo-local
.vscode/settings.json could supply a bearer AND retarget `runtimeHost`,
and the token rode every request — opening a repo was enough.
- The runtime token is passed to the terminal via environment instead of
`--auth-token` in argv, which was visible in shell history and `ps`.
- status.ts nonce uses a CSPRNG, matching chat.ts.
Chat correctness and accessibility (chat.ts, transcript.ts):
- Transcript prefers `detail` over the 280-char `summary`, so reload shows
the reply instead of a stub.
- `operation_key` is reused on retry, so a timeout and resend no longer
creates two turns; the dead SSE stream is cleared so reconnect can fire.
- The composer keeps its text until the turn is accepted.
- Tool paths are parsed out of `metadata.tool_input` and treated as
untrusted. The durable fix is runtime-side and is NOT taken here.
- Focus styling and roles/labels added; there were none.
Chrome (package.json, extension.ts):
- Chat is contributed to the secondary sidebar with an activity-bar
fallback, gated on `codewhale.noSecondarySidebar`, which is set at
activation from `vscode.version` (>= 1.106). One ChatView instance serves
both view ids and `reveal()` focuses whichever resolved.
- `engines.vscode` stays `^1.96.2` and the threshold is enforced at runtime,
matching the shipping Codex extension; raising the floor would have cut
off 1.90-1.105 users and made the fallback unreachable for nothing.
CI and dev loop (ci.yml, .gitignore, .vscode/):
- CI runs the extension tests. Nothing ran them before, which is how a send
path that could not work stayed green.
- launch.json/tasks.json give a working F5 host. The root `.gitignore`'s
bare `.vscode/` silently swallowed them, so a negation was added — without
it these files exist locally and vanish on commit.
Gates:
cd extensions/vscode && npx tsc --noEmit -> clean
cd extensions/vscode && npm test -> tests 42, pass 42, fail 0
(baseline before this was 25)
manifest/provider coherence: every declared view id has a provider;
no provider lacks a manifest entry; context key set at activation.
Not done, deliberately: the Runtime view still exists, so this is not yet a
single-view Agents panel. Removing it spans extension.ts, status.ts and two
commands; a half-removal is worse than either state. Gate item 5 — a human
driving one real turn in an Extension Development Host — remains unproven.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D4rk4NXwyy6wmvii9Lp84P
Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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_4ebed471-8eaf-417b-ae82-0f8da24ffcf0) |
|
Claude finished @Hmbown's task in 2m 37s —— View job Review of PR #5987
Note: SummaryThis is a careful, well-scoped fix. I traced the core claims against the diff and they hold up:
Minor / non-blocking
Nothing found here rises to a correctness bug, a security hole, or a provider/model-routing concern (this extension doesn't do any provider/model-prefix inference — model info is just displayed, not chosen from). The security-hardening claims in the PR description are backed by the diff, and the untrusted-input handling (tool paths, markdown, SSE frames) is appropriately defensive and tested. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| async selectThread(threadId: string): Promise<void> { | ||
| this.closeStream(); | ||
| this.streamingTurnId = undefined; | ||
| this.interruptRequested = false; |
There was a problem hiding this comment.
🔴 Running threads lose their controls
Selecting an existing thread clears streamingTurnId without restoring the snapshot’s active turn. Stop and steer disappear while that turn continues running.
Prompt for agents
Update ChatView.selectThread in extensions/vscode/src/chat.ts to derive streamingTurnId from the loaded ThreadDetail. Find the latest turn whose status is queued or in_progress and preserve its ID so resumed threads expose Stop and Steer. Ensure terminal snapshots leave streamingTurnId unset, and cover selecting a thread that was already running before the VS Code view loaded.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const selected = new Set(); | ||
| const answer = (label) => vscode.postMessage({ | ||
| command: "answerInput", inputId: input.id, | ||
| answers: [{ id: question.id, label: label, value: label }], | ||
| }); |
There was a problem hiding this comment.
🔴 Multi-question replies lose later answers
Choosing one option submits only that question. submit_user_input consumes the entire request, so later questions cannot be answered.
Prompt for agents
Rework userInputCard in extensions/vscode/src/chat.ts to collect answers across every question in a PendingUserInput and submit once. Preserve single-select and multi-select rules, require an answer for each question, and send one answers array to answerInput. The Runtime's submit_user_input consumes the whole pending request on the first POST, so per-question submission cannot work for requests containing multiple questions.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Accepted: only now is it safe to drop the composer text and chips. | ||
| this.pendingSend = undefined; | ||
| this.chips = []; | ||
| this.streamingTurnId = result.turn.id; | ||
| this.interruptRequested = false; | ||
| this.addLocalUserMessage(prompt); | ||
| this.post({ type: "composerResult", ok: true }); |
There was a problem hiding this comment.
🟡 Every sent prompt appears twice
Every accepted send adds a local user item. emit_claimed_turn_started already publishes the persisted item, so the transcript renders both copies.
Prompt for agents
Remove or reconcile the synthetic local user message in ChatView.sendPrompt. The Runtime emits item.started and item.completed for the persisted user item before start_turn returns, and the existing SSE stream or replay will deliver it. Keep optimistic rendering only if it uses an identity that can be replaced by the canonical item without duplication.
Was this helpful? React with 👍 or 👎 to provide feedback.
| this.items.clear(); | ||
| this.itemOrder = []; | ||
| this.activeThreadId = threadId; | ||
| this.postSync(); |
There was a problem hiding this comment.
🟡 Thread switches expose stale approvals
Switching threads keeps the previous activeDetail during loading. Its approval cards remain actionable under the newly selected thread until the request finishes.
| this.items.clear(); | |
| this.itemOrder = []; | |
| this.activeThreadId = threadId; | |
| this.postSync(); | |
| this.items.clear(); | |
| this.itemOrder = []; | |
| this.activeDetail = undefined; | |
| this.lastSeq = 0; | |
| this.activeThreadId = threadId; | |
| this.postSync(); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (info.statusCode === 401) { | ||
| return { kind: "auth-required", detail: "Runtime info requires a token." }; | ||
| } |
There was a problem hiding this comment.
🟡 Runtime information failures look connected
A 404 or 500 from the information endpoint falls through to connected. The extension reports a healthy Runtime although its API is unavailable.
| if (info.statusCode === 401) { | |
| return { kind: "auth-required", detail: "Runtime info requires a token." }; | |
| } | |
| if (info.statusCode === 401) { | |
| return { kind: "auth-required", detail: "Runtime info requires a token." }; | |
| } | |
| if (!isOk(info.statusCode)) { | |
| return { kind: "error", detail: `Runtime info returned HTTP ${info.statusCode}.` }; | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| try { | ||
| await interruptTurn(await this.configProvider(), this.activeThreadId, this.streamingTurnId); | ||
| this.output.appendLine(`Interrupt requested for turn ${this.streamingTurnId}`); |
There was a problem hiding this comment.
🟡 Idle stop leaves controls stuck
When interruptTurn returns not-running, the chat retains streamingTurnId. Stop and steer remain visible although the turn has already ended.
| try { | |
| await interruptTurn(await this.configProvider(), this.activeThreadId, this.streamingTurnId); | |
| this.output.appendLine(`Interrupt requested for turn ${this.streamingTurnId}`); | |
| const result = await interruptTurn(await this.configProvider(), this.activeThreadId, this.streamingTurnId); | |
| if (result === "not-running") { | |
| this.streamingTurnId = undefined; | |
| this.interruptRequested = false; | |
| this.postSync(); | |
| } | |
| this.output.appendLine(`Interrupt requested for turn ${this.streamingTurnId}`); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| case "connected": | ||
| updateStatus("$(check) CodeWhale", state.detail); | ||
| await refreshAgentViewDetails(false); | ||
| await chatView.refreshThreads(); | ||
| if (becameConnected) { | ||
| await chatView.resyncAfterConnection(); | ||
| } |
There was a problem hiding this comment.
🟡 Runtime panel stays permanently stale
Successful checks refresh only chatView. The retained Runtime panel never receives thread summaries or snapshots, and its refresh button cannot update them.
Prompt for agents
Restore RuntimeStatusView data updates in extensions/vscode/src/extension.ts. On a connected check, load thread summaries and snapshots through the authenticated ApiConfig and call statusView.updateThreads and statusView.updateSnapshots. Ensure codewhale.refreshAgentView updates the Runtime panel rather than only ChatView, while avoiding duplicate network requests where practical.
Was this helpful? React with 👍 or 👎 to provide feedback.
| private async openStream(threadId: string, sinceSeq: number): Promise<void> { | ||
| this.closeStream(); | ||
| this.reconnectAttempt = 0; | ||
| const config = await this.configProvider(); |
There was a problem hiding this comment.
🔍 Reconnect backoff never advances
openStream resets reconnectAttempt before each attempt. Persistent failures therefore retry every second instead of reaching the five-second cap.
Was this helpful? React with 👍 or 👎 to provide feedback.
| private handleStreamEvent(event: RuntimeEvent): void { | ||
| if (event.seq <= this.lastSeq) { | ||
| return; // duplicate or stale replay | ||
| } | ||
| this.lastSeq = event.seq; | ||
| this.reconnectAttempt = 0; |
There was a problem hiding this comment.
🔍 Event gaps lack recovery
The client ignores previousSeq and accepts any newer sequence. The embedded client detects gaps and reloads its snapshot.
Was this helpful? React with 👍 or 👎 to provide feedback.
| constructor( | ||
| private readonly extensionContext: vscode.ExtensionContext, | ||
| private readonly configProvider: () => Promise<ApiConfig>, | ||
| private readonly output: vscode.OutputChannel, | ||
| ) {} |
There was a problem hiding this comment.
🔍 Unused context dependency
ChatView stores extensionContext but never reads it. Remove the parameter to keep the view’s dependency boundary minimal.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
🟡 Changes recommended
There are a few concrete correctness issues in new/updated logic (selection line-range computation, interrupt UI state, and redundant refresh behavior) that should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR upgrades the extensions/vscode integration from a runtime/status scaffold into a functional chat sidebar client for the Codewhale Runtime API, while hardening token handling and webview safety and ensuring the send/stream paths match the runtime’s actual HTTP/SSE contracts.
Changes:
- Adds a VS Code–independent Runtime HTTP/SSE client, SSE parser, transcript projection, and safe Markdown rendering with node-based unit tests.
- Introduces a full chat webview (threads, streaming turns, approvals/user-input, context chips, stop/steer) and wires it into extension activation + status refresh.
- Improves security posture (SecretStorage token precedence/migration, machine-scoped settings, runtime token passed via env, CSP nonces via CSPRNG) and adds CI coverage for extension tests.
File summaries
| File | Description |
|---|---|
| extensions/vscode/src/transcript.ts | Pure transcript projection + defensive file-path extraction/containment helpers. |
| extensions/vscode/src/test/transcript.test.ts | Unit tests for transcript projection, file-path parsing, containment, and SSE status mapping. |
| extensions/vscode/src/test/sse.test.ts | Unit tests for SSE frame parsing across chunking and CRLF boundaries. |
| extensions/vscode/src/test/markdown.test.ts | Unit tests validating safe Markdown subset rendering (escaping, links, code actions). |
| extensions/vscode/src/test/api.test.ts | Unit tests for HTTP status handling (2xx), error surfacing, SSE streaming, and typed 409 conflicts. |
| extensions/vscode/src/status.ts | Switches CSP nonce generation to CSPRNG. |
| extensions/vscode/src/sse.ts | Adds minimal dependency-free SSE parser producing runtime envelope events. |
| extensions/vscode/src/secrets.ts | Implements SecretStorage-first token resolution with one-way migration from deprecated settings and workspace-scope defense. |
| extensions/vscode/src/runtime.ts | Removes token-from-settings usage and starts runtime with token via env instead of argv. |
| extensions/vscode/src/markdown.ts | Adds dependency-free safe Markdown renderer returning HTML + extracted code blocks. |
| extensions/vscode/src/extension.ts | Activates chat view, routes connection state into chat/status views, adds commands, secondary-sidebar gating, and refresh loop updates. |
| extensions/vscode/src/context.ts | Adds context-chip capture (selection/file/diagnostics) and prompt assembly for attaching editor context. |
| extensions/vscode/src/chat.ts | Adds chat webview implementation (threads, streaming SSE, approvals/inputs, context chips, safe interactions). |
| extensions/vscode/src/api.ts | Adds VS Code–free Runtime API client with centralized 2xx handling and typed errors. |
| extensions/vscode/README.md | Updates extension documentation to reflect chat sidebar + security posture + local dev loop. |
| extensions/vscode/package.json | Adds chat commands, menus/keybinding, secondary sidebar contribution with runtime gating, settings hardening, and test script. |
| extensions/vscode/media/codewhale.svg | Updates the contributed icon geometry and documentation comment. |
| extensions/vscode/.vscodeignore | Excludes sources/tests/dev artifacts from the packaged VSIX. |
| extensions/vscode/.vscode/tasks.json | Adds compile/watch tasks for F5 Extension Development Host. |
| extensions/vscode/.vscode/launch.json | Adds launch configs for running the extension and its node-based tests. |
| .gitignore | Un-ignores the extension’s committed .vscode/ dev-loop configuration directory. |
| .github/workflows/ci.yml | Adds a CI job to run the extension’s npm test suite. |
Review details
- Files reviewed: 20/23 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| private async interrupt(): Promise<void> { | ||
| if (!this.activeThreadId || !this.streamingTurnId) { | ||
| return; | ||
| } | ||
| try { | ||
| await interruptTurn(await this.configProvider(), this.activeThreadId, this.streamingTurnId); | ||
| this.output.appendLine(`Interrupt requested for turn ${this.streamingTurnId}`); | ||
| } catch (error) { | ||
| this.handleError("Interrupt failed", error); | ||
| } | ||
| } |
| const endLine = selection.end.line + 1; | ||
| const lines = endLine - startLine + 1; |
| await chatView.refreshThreads(); | ||
| if (becameConnected) { | ||
| await chatView.resyncAfterConnection(); | ||
| } |
| id: `chip-${++chipCounter}`, | ||
| kind: "diagnostics", | ||
| label: "Problems", | ||
| detail: `${entries.length} entrie${entries.length === 1 ? "" : "s"}`, |
![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: 6482b58af2
ℹ️ 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".
| this.items.clear(); | ||
| this.itemOrder = []; | ||
| this.activeThreadId = threadId; | ||
| this.postSync(); |
There was a problem hiding this comment.
When switching threads, this sync publishes the new activeThreadId while activeDetail still belongs to the previous thread. Until the detail request finishes—and indefinitely if it fails—the new thread displays the old thread's approval and user-input cards; clicking an old approval calls the globally addressed /v1/approvals/{id} endpoint and can authorize work in the previous thread. Clear activeDetail before posting the transitional state.
Useful? React with 👍 / 👎.
| this.chips = []; | ||
| this.streamingTurnId = result.turn.id; | ||
| this.interruptRequested = false; | ||
| this.addLocalUserMessage(prompt); |
There was a problem hiding this comment.
Every accepted send adds a new local-* user item even though the Runtime emits durable item.started and item.completed events for its persisted user item before startTurn returns (RuntimeThreadManager::emit_claimed_turn_started). The stream is then reopened from the pre-send cursor, so even if the prior stream missed those events they are replayed under the durable item ID; because nothing removes or reconciles the local ID, normal sends display two “You” bubbles until the thread is reloaded. The steer path has the same duplication.
Useful? React with 👍 / 👎.
| if (event.seq <= this.lastSeq) { | ||
| return; // duplicate or stale replay | ||
| } | ||
| this.lastSeq = event.seq; |
There was a problem hiding this comment.
For a newer event this code advances lastSeq without checking event.previousSeq. The Runtime contract in docs/RUNTIME_API.md:1095-1100 explicitly requires comparing previous_seq with the accepted per-thread cursor, and the existing browser client re-snapshots on a mismatch. If a frame is lost, malformed, or unavailable during replay, this client instead accepts later events permanently, which can omit transcript text, a completion, or a pending approval/user-input request; detect the mismatch and refresh the thread snapshot before continuing.
Useful? React with 👍 / 👎.
| const answer = (label) => vscode.postMessage({ | ||
| command: "answerInput", inputId: input.id, | ||
| answers: [{ id: question.id, label: label, value: label }], | ||
| }); |
There was a problem hiding this comment.
When a request_user_input request contains two or three questions, each option click immediately posts an answer containing only that one question. The Runtime settles and removes the entire pending request after the first POST, so the remaining questions cannot be answered and the model receives an incomplete response. The existing web client collects every question into one answers array and exposes a single submit action; this card needs the same request-level submission behavior.
Useful? React with 👍 / 👎.
| this.activeDetail = detail; | ||
| this.lastSeq = detail.latestSeq; |
There was a problem hiding this comment.
Selecting a thread always clears streamingTurnId, but loading its detail never derives the currently in-progress turn from detail.thread.latestTurnId and detail.turns. If the user switches away while a turn runs and then returns—or reconnects while it is running—the transcript continues receiving events, but streaming remains false, Stop and Steer stay hidden, and terminal events cannot match the tracked turn. Restore the in-progress turn ID from the snapshot.
Useful? React with 👍 / 👎.
|
|
||
| private async openStream(threadId: string, sinceSeq: number): Promise<void> { | ||
| this.closeStream(); | ||
| this.reconnectAttempt = 0; |
There was a problem hiding this comment.
Every reconnect enters openStream and resets reconnectAttempt before the connection has produced an event. During a sustained outage, the subsequent error therefore always increments from zero and schedules another retry after one second, so the advertised capped backoff never reaches two through five seconds and the extension continually hammers the unavailable endpoint. Reset the counter only after a successfully accepted event, as handleStreamEvent already does.
Useful? React with 👍 / 👎.
| box.value = ""; | ||
| vscode.postMessage({ command: "steer", text }); |
There was a problem hiding this comment.
The steer composer clears its value before the extension host has acknowledged the request and has no result message analogous to composerResult. If the Runtime rejects the steer because the turn just ended, the token expired, or the connection drops, ChatView.steer only displays an error and the user's steering instruction is irretrievably lost. Keep the value until success or restore it on failure.
Useful? React with 👍 / 👎.
| })); | ||
| card.appendChild(confirm); | ||
| } | ||
| if (question.allowFreeText) { |
There was a problem hiding this comment.
This hides the free-text response whenever allowFreeText is false or omitted, but the repository's terminal and browser surfaces intentionally keep an “Other” answer available for every clarification (crates/tui/src/tui/user_input.rs:637-651 and crates/tui/src/runtime_web/app.mjs:1676-1695). Since older/model-generated requests commonly omit the flag, a user whose answer is not among the suggested options cannot respond from VS Code even though the other supported clients can.
Useful? React with 👍 / 👎.
| const stream = openEventStream(config, threadId, sinceSeq, new SseParser()); | ||
| stream.onEvent = (event) => this.handleStreamEvent(event); | ||
| stream.onError = (error) => this.handleStreamError(threadId, error); | ||
| this.stream = stream; |
There was a problem hiding this comment.
The error handler is keyed only by threadId, so it cannot distinguish the current stream from an older stream closed by openStream. Destroying an active Node HTTP response emits an ECONNRESET response error; if that callback runs after the replacement is assigned, handleStreamError closes this.stream—the new stream—and schedules another reconnect. This occurs on same-thread stream replacement after a send and can cascade into repeated disconnects; capture the stream identity or detach its handlers before closing it.
Useful? React with 👍 / 👎.
| (response) => { | ||
| let raw = ""; | ||
| response.setEncoding("utf8"); | ||
| response.on("data", (chunk: string) => { | ||
| raw += chunk; | ||
| }); | ||
| response.on("end", () => { | ||
| resolve({ statusCode: response.statusCode ?? 0, body: parseJson(raw) }); |
There was a problem hiding this comment.
The promise listens only for the response's end event. If the Runtime restarts, a proxy resets the socket, or the peer otherwise aborts after sending headers or a partial body, Node emits aborted/error on the response and closes the request without emitting end or the request timeout; this promise then remains pending forever. Affected sends leave the composer busy indefinitely, and an affected health check leaves autoRefreshInFlight stuck, so reject on response abort/error as well.
Useful? React with 👍 / 👎.
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
This is a large, focused VS Code extension rework that fixes the send path (2xx status handling, 201 Created), moves tokens to SecretStorage, adds a real Chat sidebar with SSE streaming, and introduces CI for extension tests. The core direction is sound and the tests are a big improvement, but several correctness and security edge cases remain.
Findings
- [WARNING] Streamed agent text can be replaced by the truncated summary on item.completed (
extensions/vscode/src/transcript.ts:71)
In transcript.ts, a terminal agent message computes its text asdetail || item.summary || streamText || existing?.summary. The runtime may omitdetailand still send the 280-charactersummaryon completion. During a live stream this discards the full streamed text and renders the truncated stub, which contradicts the PR's stated transcript behavior. - [WARNING] checkConnection can report connected when /v1/runtime/info returns a non-2xx other than 401 (
extensions/vscode/src/api.ts:141)
After a successful /health call, checkConnection only checks whether the info request returned 401. If the info request times out (statusCode 0), returns 500, or otherwise fails, the code falls through toconnectedwith an empty version. Add anisOkguard for the info response. - [WARNING] Runtime view thread summaries and restore points are no longer populated automatically (
extensions/vscode/src/extension.ts:91)
The oldrefreshAgentViewDetailspopulatedstatusView.updateThreadsandstatusView.updateSnapshotson connection and auto-refresh. The newcheckAndRefreshRuntimeonly callsstatusView.update(state)andchatView.refreshThreads(); snapshots refresh only when the explicitcodewhale.refreshSnapshotscommand runs. The Runtime view remains visible, so its thread/snapshot panes can be empty or stale, contrary to the README's description of that view. - [WARNING] 409 ConflictError is typed but the chat send path still shows a generic failure (
extensions/vscode/src/chat.ts)
startTurnthrows a typed ConflictError for a second send while a turn is live, butsendPromptcatches all errors and callshandleError, which shows a genericSend failedmessage. The PR description says a second send while a turn is live is reported as 'already running', but that user-facing translation is not implemented for the send path. - [WARNING] File path containment is lexical only and can be bypassed by workspace symlinks (
extensions/vscode/src/transcript.ts)
isInsideRootresolves paths lexically but does not resolve symlinks. A repo-local symlink inside the workspace can point outside the workspace, so a model-supplied relative path could open an editor outside the workspace without triggering the explicit outside-workspace confirmation. - [INFO] Security-critical token precedence and migration have no automated coverage (
extensions/vscode/src/secrets.ts)
The newsecrets.tsbehavior is critical to the security fix: SecretStorage wins over settings, workspace-level tokens are ignored, and the legacy user token is migrated once. None of that behavior is unit tested. Given the PR's security focus, this should be pinned with tests; the chat/send state machine is also untested. - [INFO] Packaged extension includes dev-only .vscode launch/tasks configuration (
extensions/vscode/.vscodeignore:1)
.vscodeignoreexcludes source and tests but does not exclude.vscode/**.vsce packagewill therefore includelaunch.jsonandtasks.jsonin the VSIX, even though they are only for the local F5 development loop.
Suggestions
-
extensions/vscode/src/transcript.ts:71— Prefer the already streamed full text over a completion payload's 280-char summary when a live agent message finishes.const text = detail || streamText || item.summary || existing?.summary || ""; -
extensions/vscode/src/api.ts:141— Check all non-2xx info responses so a successful health check followed by an info timeout or 5xx cannot be reported as connected.if (info.statusCode === 401) { return { kind: "auth-required", detail: "Runtime info requires a token." }; } if (!isOk(info.statusCode)) { return { kind: info.statusCode === 0 ? "offline" : "error", detail: info.statusCode === 0 ? "Runtime info could not be reached." : `Runtime info returned HTTP ${info.statusCode}.`, }; } -
extensions/vscode/.vscodeignore:1— Exclude the dev-loop .vscode directory from the packaged VSIX.src/** .vscode/**
Assessment
Approve with changes requested. The primary send-path fix and token storage migration are valuable and well-tested in the API layer, but the streamed transcript truncation, runtime info status guard, Runtime view population regression, and remaining symlink path containment gap should be addressed before merge. The package should also exclude the dev-only .vscode configuration.
Advisory review by Codewhale (codewhale review --pr 5987 --post, head 6482b58af2203067050cc500737359038df08d59). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
|
|
||
| if (item.kind === "agent_message") { | ||
| // `detail` carries the full reply; `summary` is a 280-char stub on reload. | ||
| const text = detail || item.summary || streamText || existing?.summary || ""; |
There was a problem hiding this comment.
[WARNING] Streamed agent text can be replaced by the truncated summary on item.completed
In transcript.ts, a terminal agent message computes its text as detail || item.summary || streamText || existing?.summary. The runtime may omit detail and still send the 280-character summary on completion. During a live stream this discards the full streamed text and renders the truncated stub, which contradicts the PR's stated transcript behavior.
| const info = await requestJson(`${config.baseUrl}/v1/runtime/info`, config, { | ||
| timeoutMs: HEALTH_TIMEOUT_MS, | ||
| }); | ||
| if (info.statusCode === 401) { |
There was a problem hiding this comment.
[WARNING] checkConnection can report connected when /v1/runtime/info returns a non-2xx other than 401
After a successful /health call, checkConnection only checks whether the info request returned 401. If the info request times out (statusCode 0), returns 500, or otherwise fails, the code falls through to connected with an empty version. Add an isOk guard for the info response.
| case "connected": | ||
| updateStatus("$(check) CodeWhale", state.detail); | ||
| await refreshAgentViewDetails(false); | ||
| await chatView.refreshThreads(); |
There was a problem hiding this comment.
[WARNING] Runtime view thread summaries and restore points are no longer populated automatically
The old refreshAgentViewDetails populated statusView.updateThreads and statusView.updateSnapshots on connection and auto-refresh. The new checkAndRefreshRuntime only calls statusView.update(state) and chatView.refreshThreads(); snapshots refresh only when the explicit codewhale.refreshSnapshots command runs. The Runtime view remains visible, so its thread/snapshot panes can be empty or stale, contrary to the README's description of that view.
| @@ -0,0 +1,6 @@ | |||
| src/** | |||
There was a problem hiding this comment.
[INFO] Packaged extension includes dev-only .vscode launch/tasks configuration
.vscodeignore excludes source and tests but does not exclude .vscode/**. vsce package will therefore include launch.json and tasks.json in the VSIX, even though they are only for the local F5 development loop.
|
|
||
| if (item.kind === "agent_message") { | ||
| // `detail` carries the full reply; `summary` is a 280-char stub on reload. | ||
| const text = detail || item.summary || streamText || existing?.summary || ""; |
There was a problem hiding this comment.
Prefer the already streamed full text over a completion payload's 280-char summary when a live agent message finishes.
| const text = detail || item.summary || streamText || existing?.summary || ""; | |
| const text = detail || streamText || item.summary || existing?.summary || ""; |
| if (info.statusCode === 401) { | ||
| return { kind: "auth-required", detail: "Runtime info requires a token." }; | ||
| } |
There was a problem hiding this comment.
Check all non-2xx info responses so a successful health check followed by an info timeout or 5xx cannot be reported as connected.
| if (info.statusCode === 401) { | |
| return { kind: "auth-required", detail: "Runtime info requires a token." }; | |
| } | |
| if (info.statusCode === 401) { | |
| return { kind: "auth-required", detail: "Runtime info requires a token." }; | |
| } | |
| if (!isOk(info.statusCode)) { | |
| return { | |
| kind: info.statusCode === 0 ? "offline" : "error", | |
| detail: | |
| info.statusCode === 0 | |
| ? "Runtime info could not be reached." | |
| : `Runtime info returned HTTP ${info.statusCode}.`, | |
| }; | |
| } |
| @@ -0,0 +1,6 @@ | |||
| src/** | |||
There was a problem hiding this comment.
Exclude the dev-loop .vscode directory from the packaged VSIX.
| src/** | |
| src/** | |
| .vscode/** |
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_8559b3b0-3b41-4fe2-90a1-e66502e39088) |
|
Claude finished @Hmbown's task in 2m 19s —— View job Review complete ✅
SummaryThis PR is a large, well-scoped improvement: the send-path fix ( That said, I traced several of this PR's own stated correctness claims against the server side ( Confirmed correctness bugs
Two smaller, previously-noted issues I independently verified as real:
Reuse / simplification
Test coverage
Security
|
There was a problem hiding this comment.
Codewhale review
Large VS Code extension rewrite adding a chat sidebar over the local runtime. Core API client, SSE parser, Markdown renderer, and security hardening are covered by new tests, but review found correctness and security gaps around connection status, legacy plaintext token cleanup, and the Runtime view's thread summaries.
Findings
- [WARNING] checkConnection can report connected when runtime info fails (
extensions/vscode/src/api.ts)
In api.ts checkConnection, after/healthsucceeds, it requests/v1/runtime/infobut only special-cases 401. If that request returns statusCode 0 (network failure), 500, or any other non-2xx,readBoolean(readBody(info.body).auth_required)is false andreadString(readBody(info.body).version)is undefined, so the function falls through and returns{ kind: "connected" }. The status bar and chat header then claim the runtime is reachable even though data endpoints are unavailable. - [WARNING] Deprecated plaintext runtime token can remain after migration (
extensions/vscode/src/secrets.ts)
resolveTokenreturns the SecretStorage value immediately and never clears the deprecated globalcodewhale.runtimeTokensetting.promptForTokenalso stores the new secret without clearing that setting. Since the setting is still machine-scoped plaintext, a user who sets a token through the command or already has a secret can retain a plaintext bearer copy in settings and possibly Settings Sync, weakening the stated security posture. - [WARNING] Runtime view no longer receives thread summaries (
extensions/vscode/src/extension.ts)
The old refreshAgentView code calledstatusView.updateThreadswithlistThreadSummaries. The new connected branch incheckAndRefreshRuntimeonly callschatView.refreshThreads(), and thecodewhale.refreshAgentViewcommand does the same.RuntimeStatusViewstill exposesupdateThreads, but no code path calls it, so the Runtime view no longer shows recent thread summaries despite the README and PR description promising them. - [INFO] Security-sensitive token and runtime paths lack direct tests (
extensions/vscode/src/test/api.test.ts)
Added tests cover api, markdown, sse, and transcript. There are no unit tests for secrets.ts (SecretStorage precedence, workspace token ignored, migration clearing) or runtime.ts (no--auth-tokenin argv). These are security-sensitive behaviors that can regress without failing CI.
Suggestions
extensions/vscode/src/api.ts— After requesting/v1/runtime/info, checkisOk(info.statusCode)andinfo.statusCode !== 0before interpretingauth_requiredorversion; return anerrororofflinestate on non-2xx/status 0 instead of falling through to connected.extensions/vscode/src/secrets.ts— When SecretStorage already holds a token, clear the deprecated globalcodewhale.runtimeTokensetting. Also clear it afterstoreTokeninpromptForToken, so the plaintext copy is removed once the secret is authoritative.extensions/vscode/src/extension.ts— On a connected runtime, fetchlistThreadSummariesand pass the result tostatusView.updateThreads, as the old implementation did, so the Runtime view actually shows recent thread summaries.
Assessment
Substantial and well-tested client rewrite, but three warning-level gaps should be addressed: connection state can report false positives, the plaintext legacy token can linger after migration, and Runtime view thread summaries are no longer populated. Security-sensitive token/terminal paths also deserve direct tests.
Advisory review by Codewhale (codewhale review --pr 5987 --post, head dc6d8d5281e8b60275d11ce38f02553bcaff6679). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
`prune_older_than_keeps_the_newest_and_drops_only_the_old_tail` fails
intermittently on windows-latest with
assertion `left == right` failed: only the old tail should be removed
left: 3
right: 2
The fixture builds two old snapshots, sleeps 8s, then two new ones 1.1s
apart, and cuts at a hardcoded 6s. That assumes `repo.snapshot()` is fast:
`new:0` is only ~1.2s plus one git subprocess older than prune time, so on
a loaded Windows runner that subprocess alone carries it past the 6s line
and it is pruned with the old pair.
The existing fixture guard could not catch it — it asserts on `before[0]`
and `before[2]`, and `before[1]` is the entry that drifts.
The cut is now computed from the timestamps the repo actually recorded:
aim at the midpoint of the gap between the oldest survivor and the newest
victim, which leaves ~4s of slack in both directions instead of depending
on wall-clock luck. The gap itself is asserted first, so a fixture that
collapsed says so plainly rather than failing later as a count mismatch.
Behaviour under test is unchanged: two removed, `new:1` and `new:0`
survive. No production code is touched.
cargo clippy -p codewhale-tui --lib -> 0 errors
cargo test -p codewhale-tui --lib -- prune_older_than
-> test result: ok. 3 passed; 0 failed
Found when it failed the windows leg of Hmbown#5987, a PR containing zero Rust
files (TypeScript, CI config and .gitignore only), so it cannot have been
caused there.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D4rk4NXwyy6wmvii9Lp84P
Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Closes #5834
The extension had never successfully started a turn
startTurnaccepted only HTTP 200/202 while the runtime'sstart_thread_turn(crates/tui/src/runtime_api.rs:4613-4632) returnsStatusCode::CREATEDas its only success path.git log -- extensions/vscode/src/api.tsis a single commit: this was never a regression — it shipped that way and was never run end to end.Two commits: the original chat sidebar, then the fixes.
Send path (
api.ts)isOk:>= 200 && < 300) through oneensureOkhelper routed through every call site, instead of enumerating codes at eleven of them. 201 is accepted because it is 2xx, not because it is special-cased — the shape the embedded web client already used atcrates/tui/src/runtime_web/app.mjs:873, which is why that client worked against the same runtime this one choked on.error.messageis surfaced on every route; previously onlystartTurnpassed it through.Security
secrets.ts, the manifest and the README all already promised. Previously a repo-local.vscode/settings.jsoncould supply a bearer and retargetruntimeHost— opening a repo was enough. Settings values are now a one-time migration source, andruntimeHost/runtimePort/runtimeToken/commandPathare"scope": "machine".--auth-tokenin argv, which was visible in shell history andps.status.tsnonce uses a CSPRNG, matchingchat.ts.Chat correctness and accessibility
detailover the 280-charsummary, so reload shows the reply instead of a stub.operation_keyis reused on retry, so a timeout and resend no longer creates two turns; the dead SSE stream is cleared so reconnect can fire.metadata.tool_inputand treated as untrusted. The durable fix is runtime-side (runtime_threads.rs:8818-8822) and is deliberately not taken here.Chrome
Chat is contributed to the secondary sidebar with an activity-bar fallback, gated on
codewhale.noSecondarySidebar, set at activation fromvscode.version(>= 1.106). OneChatViewinstance serves both view ids andreveal()focuses whichever resolved.engines.vscodestays^1.96.2with the threshold enforced at runtime, matching the shipping Codex extension — raising the floor would cut off 1.90-1.105 users and make the fallback unreachable for nothing.CI and dev loop
launch.json/tasks.jsongive a working F5 host. The root.gitignore's bare.vscode/silently swallowed them, so a negation was added — without it these files exist locally and vanish on commit.Evidence
Manifest/provider coherence checked by hand: every declared view id has a provider, no provider lacks a manifest entry, context key set at activation. A green typecheck cannot see either of those — an earlier pass had all six units reporting "complete" with
tscclean and no UI at all.Not done, deliberately
The Runtime view still exists, so this is not yet a single-view Agents panel — removing it spans
extension.ts,status.tsand two commands, and a half-removal is worse than either state.A human driving one real turn in an Extension Development Host remains unproven. F5 now works, which it never did before.
Extension version stays 0.9.12 to match
Cargo.toml;prepare-release.shbumps it when 0.9.13 is cut.🤖 Generated with Claude Code
https://claude.ai/code/session_01D4rk4NXwyy6wmvii9Lp84P
Note
Medium Risk
Large new surface area (webview + local HTTP to agent runtime) with thoughtful token and path guards, but auth and untrusted model output remain sensitive; core send-path and API behavior are now covered by CI tests.
Overview
Turns the VS Code extension from a runtime-status scaffold into a Codewhale chat sidebar (activity bar or secondary sidebar on VS Code ≥1.106) that talks to the local Engine Runtime over HTTP/SSE: threads, streaming turns, steer/stop, inline approvals and clarification prompts, editor context chips, and safe Markdown rendering with copy/insert on code blocks. A separate Runtime webview keeps connection helpers, thread summaries, and restore points.
Runtime client layer is new and VS Code–free (
api.ts,sse.ts): health/info, thread CRUD, turns withoperation_key, approvals, user input, snapshots, and replayable event streams. Send path fix: success is any 2xx (so 201 CREATED onPOST …/turnsworks); errors surface runtime messages; 409 is typed for conflicts and “nothing to stop” on interrupt.Security hardening: bearer tokens live in SecretStorage (user setting only migrates once; workspace tokens are ignored); host/port/command are machine-scoped;
servegets the token viaCODEWHALE_RUNTIME_TOKENinstead of--auth-tokenin argv; webview CSP, HTML escape-before-markdown, and guarded file opens from model-influenced paths.CI/dev: new
vscode-extensionjob runsnpm test(compile + unit tests); F5launch.json/tasks.jsonare tracked via a.gitignorenegation; packaging uses.vscodeignoreand bumps manifest to 0.9.12 /^1.96.2engines with runtime sidebar gating.Reviewed by Cursor Bugbot for commit dc6d8d5. Bugbot is set up for automated code reviews on this repo. Configure here.