From 8bddbe0e7ac4e1207e70d08c2739230221e0d8f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Dre=C5=BCewski?= Date: Wed, 17 Jun 2026 21:58:19 +0200 Subject: [PATCH 1/7] feat(openai): add gpt-5.5 model support (zoo #537) Port of Zoo-Code PR #537. Adds the gpt-5.5 model definition to the OpenAI native provider registry with matching tests. Co-authored-by: Slava Co-authored-by: edelauna <54631123+edelauna@users.noreply.github.com> --- .../2026-06-17_zoo-537-add-gpt-5-5-support.md | 220 ++++++++++++++++++ packages/types/src/providers/openai.ts | 26 +++ .../providers/__tests__/openai-native.spec.ts | 65 ++++++ 3 files changed, 311 insertions(+) create mode 100644 ai_plans/2026-06-17_zoo-537-add-gpt-5-5-support.md diff --git a/ai_plans/2026-06-17_zoo-537-add-gpt-5-5-support.md b/ai_plans/2026-06-17_zoo-537-add-gpt-5-5-support.md new file mode 100644 index 0000000000..dc396d58c1 --- /dev/null +++ b/ai_plans/2026-06-17_zoo-537-add-gpt-5-5-support.md @@ -0,0 +1,220 @@ +# Port plan — Zoo PR #537 → `feature/zoo-537-add-gpt-5-5-support` + +> **For the executor (read first).** Do the steps **in order**. Do **not** +> improvise, refactor beyond what is written, or add anything not listed +> (YAGNI). Every code block below is **already adapted to this repo** — paste it +> as-is unless a step says otherwise. If any precondition is false or a step +> doesn't behave as described, **STOP and report** — do not guess. This repo is +> **Tumble Code**: never introduce the strings "Roo" or "Zoo" in user-facing +> text. Placeholders are written as `{{like this}}` — replace every one. + +--- + +## 0. Context (read once, write no code) + +- **Upstream:** Zoo PR #537 — "feat: add gpt-5.5 support" (commit `97ee48a7f`). +- **What it does, one paragraph:** Adds the `gpt-5.5` model definition to the OpenAI native model registry (`packages/types/src/providers/openai.ts`) with associated test coverage in `src/api/providers/__tests__/openai-native.spec.ts`. The model supports 1.05M context window, image inputs, prompt caching, verbosity control, reasoning effort (including "none"), and long-context pricing tiers (flex and priority). +- **Why we want it, with evidence in OUR code:** Our `openAiNativeModels` registry at `packages/types/src/providers/openai.ts:8-591` currently lists models from gpt-5.1-codex-max through gpt-4.x and older o-series, but gpt-5.5 is missing. When users select gpt-5.5 as their API model ID, the handler would fall back to the default model or fail. This port keeps our model list in sync with upstream. +- **What we deliberately leave out (YAGNI):** Nothing — this is a straightforward model definition addition. No UI, branding, or routing changes. +- **Original author(s) — credit them.** Slava and edelauna. When you create the port commit (only if asked), include these trailers, one per line, at the end of the commit message: + + ```text + Co-authored-by: Slava + Co-authored-by: edelauna <54631123+edelauna@users.noreply.github.com> + ``` + +## 1. Preconditions — verify before touching anything + +- [ ] Current branch is `feature/zoo-537-add-gpt-5-5-support`, created off `main`. +- [ ] These files exist (the edits below depend on them): + - `packages/types/src/providers/openai.ts` + - `src/api/providers/__tests__/openai-native.spec.ts` +- [ ] The code we will change still looks like this (quote it; if it differs, + STOP — the plan is stale): + +In `packages/types/src/providers/openai.ts`, lines 24-27 (the closing of gpt-5.1-codex-max and start of gpt-5.4): + +```ts + description: + "GPT-5.1 Codex Max: Our most intelligent coding model optimized for long-horizon, agentic coding tasks", + }, + "gpt-5.4": { +``` + +In `src/api/providers/__tests__/openai-native.spec.ts`, around line 264-267 (after the gpt-5.4 getModel test, before gpt-5.4-mini): + +```ts + expect(modelInfo.info.reasoningEffort).toBe("none") + }) + + it("should return GPT-5.4 Mini model info when selected", () => { +``` + +## 2. Write the failing test FIRST (TDD) + +- **File:** `src/api/providers/__tests__/openai-native.spec.ts` (existing). +- Add two tests: one for `getModel` (model info) and one for streaming (Responses API). + +### Test 2A — getModel for gpt-5.5 + +Insert after line 265 (after the gpt-5.4 getModel test closing `})`): + +```ts +it("should return GPT-5.5 model info when selected", () => { + const gpt55Handler = new OpenAiNativeHandler({ + ...mockOptions, + apiModelId: "gpt-5.5", + }) + + const modelInfo = gpt55Handler.getModel() + expect(modelInfo.id).toBe("gpt-5.5") + expect(modelInfo.info.maxTokens).toBe(128000) + expect(modelInfo.info.contextWindow).toBe(1_050_000) + expect(modelInfo.info.supportsVerbosity).toBe(true) + expect(modelInfo.info.supportsReasoningEffort).toEqual(["none", "low", "medium", "high", "xhigh"]) + expect(modelInfo.info.reasoningEffort).toBe("medium") +}) +``` + +### Test 2B — Responses API streaming for gpt-5.5 + +Insert in the "GPT-5 models" describe block, after the gpt-5.4 streaming test (after its closing `})`): + +```ts +it("should handle GPT-5.5 model with Responses API", async () => { + const mockFetch = vitest.fn().mockResolvedValue({ + ok: true, + body: new ReadableStream({ + start(controller) { + controller.enqueue( + new TextEncoder().encode( + 'data: {"type":"response.output_item.added","item":{"type":"text","text":"GPT-5.5 reply"}}\n\n', + ), + ) + controller.enqueue(new TextEncoder().encode("data: [DONE]\n\n")) + controller.close() + }, + }), + }) + global.fetch = mockFetch as any + + mockResponsesCreate.mockRejectedValue(new Error("SDK not available")) + + handler = new OpenAiNativeHandler({ + ...mockOptions, + apiModelId: "gpt-5.5", + }) + + const stream = handler.createMessage(systemPrompt, messages) + const chunks: any[] = [] + for await (const chunk of stream) { + chunks.push(chunk) + } + + expect(mockFetch).toHaveBeenCalledWith( + "https://api.openai.com/v1/responses", + expect.objectContaining({ + body: expect.any(String), + }), + ) + const body = (mockFetch.mock.calls[0][1] as any).body as string + const parsedBody = JSON.parse(body) + expect(parsedBody.model).toBe("gpt-5.5") + expect(parsedBody.max_output_tokens).toBe(128000) + expect(parsedBody.temperature).toBeUndefined() + expect(parsedBody.include).toEqual(["reasoning.encrypted_content"]) + expect(parsedBody.reasoning?.effort).toBe("medium") + expect(parsedBody.text?.verbosity).toBe("medium") + + const textChunks = chunks.filter((chunk) => chunk.type === "text") + expect(textChunks).toHaveLength(1) + expect(textChunks[0].text).toBe("GPT-5.5 reply") +}) +``` + +- **Run:** `cd src && npx vitest run api/providers/__tests__/openai-native.spec.ts` +- **Expect it to FAIL** because `"gpt-5.5"` is not a key in `openAiNativeModels`, so `getModel()` will return the default model or an undefined info, causing assertion failures. +- If it **passes already**, STOP — the model is likely already present; report back. + +## 3. Implement — minimal change to make the test pass + +### Edit 1 — `packages/types/src/providers/openai.ts` + +Replace (lines 24-27): + +```ts + description: + "GPT-5.1 Codex Max: Our most intelligent coding model optimized for long-horizon, agentic coding tasks", + }, + "gpt-5.4": { +``` + +With: + +```ts + description: + "GPT-5.1 Codex Max: Our most intelligent coding model optimized for long-horizon, agentic coding tasks", + }, + "gpt-5.5": { + maxTokens: 128000, + contextWindow: 1_050_000, + includedTools: ["apply_patch"], + excludedTools: ["apply_diff", "write_to_file"], + supportsImages: true, + supportsPromptCache: true, + supportsReasoningEffort: ["none", "low", "medium", "high", "xhigh"], + reasoningEffort: "medium", + inputPrice: 5.0, + outputPrice: 30.0, + cacheReadsPrice: 0.5, + longContextPricing: { + thresholdTokens: 272_000, + inputPriceMultiplier: 2, + outputPriceMultiplier: 1.5, + appliesToServiceTiers: ["default", "flex"], + }, + supportsVerbosity: true, + supportsTemperature: false, + tiers: [ + { name: "flex", contextWindow: 1_050_000, inputPrice: 2.5, outputPrice: 15.0, cacheReadsPrice: 0.25 }, + { name: "priority", contextWindow: 1_050_000, inputPrice: 12.5, outputPrice: 75.0, cacheReadsPrice: 1.25 }, + ], + description: "GPT-5.5: A new class of intelligence for coding and professional work", + }, + "gpt-5.4": { +``` + +## 4. Out of scope — do NOT do these + +- Do **not** re-add or re-wire: **TTS**, the **router / cloud provider**, **cloud upsell** UI, or **Roo/Zoo branding** — all removed from this fork on purpose. +- Do **not** rename internal ids (those stay `Roo-Code`); only user-facing strings are "Tumble". +- Do **not** change the default model ID or touch any other model definitions. + +## 5. Verify — paste real output, don't claim success without it + +- `cd src && npx vitest run api/providers/__tests__/openai-native.spec.ts` → all tests pass. +- `pnpm --filter @roo-code/types check-types` (or equivalent) → clean. +- `cd src && npx tsc --noEmit -p tsconfig.json` (or equivalent) → clean. + +## 6. Acceptance criteria (binary — all must hold) + +- [ ] The §2 tests pass; the surrounding suite is green. +- [ ] Only `packages/types/src/providers/openai.ts` and `src/api/providers/__tests__/openai-native.spec.ts` changed (`git status` confirms). +- [ ] No new "Roo" or "Zoo" user-facing strings introduced. +- [ ] No removed feature (TTS / router / cloud) was reintroduced. + +## 7. Record in the ledger + +The SKILL's `port` stage (step 3) already records this PR as `ported` once the +plan file exists. If — and only if — that has not been done yet, run it now +(re-running is a harmless idempotent upsert, so when in doubt run it once): + +```bash +node .claude/skills/zoo-port/scripts/zoo-prs.mjs record \ + --pr 537 --status ported \ + --branch feature/zoo-537-add-gpt-5-5-support \ + --plan ai_plans/2026-06-17_zoo-537-add-gpt-5-5-support.md +``` + +When you commit (only if asked), append the `Co-authored-by:` trailer(s) from §0 +to the commit message. Then summarize what landed and let the user review. diff --git a/packages/types/src/providers/openai.ts b/packages/types/src/providers/openai.ts index 81a022c663..4f04af0a63 100644 --- a/packages/types/src/providers/openai.ts +++ b/packages/types/src/providers/openai.ts @@ -24,6 +24,32 @@ export const openAiNativeModels = { description: "GPT-5.1 Codex Max: Our most intelligent coding model optimized for long-horizon, agentic coding tasks", }, + "gpt-5.5": { + maxTokens: 128000, + contextWindow: 1_050_000, + includedTools: ["apply_patch"], + excludedTools: ["apply_diff", "write_to_file"], + supportsImages: true, + supportsPromptCache: true, + supportsReasoningEffort: ["none", "low", "medium", "high", "xhigh"], + reasoningEffort: "medium", + inputPrice: 5.0, + outputPrice: 30.0, + cacheReadsPrice: 0.5, + longContextPricing: { + thresholdTokens: 272_000, + inputPriceMultiplier: 2, + outputPriceMultiplier: 1.5, + appliesToServiceTiers: ["default", "flex"], + }, + supportsVerbosity: true, + supportsTemperature: false, + tiers: [ + { name: "flex", contextWindow: 1_050_000, inputPrice: 2.5, outputPrice: 15.0, cacheReadsPrice: 0.25 }, + { name: "priority", contextWindow: 1_050_000, inputPrice: 12.5, outputPrice: 75.0, cacheReadsPrice: 1.25 }, + ], + description: "GPT-5.5: A new class of intelligence for coding and professional work", + }, "gpt-5.4": { maxTokens: 128000, contextWindow: 1_050_000, diff --git a/src/api/providers/__tests__/openai-native.spec.ts b/src/api/providers/__tests__/openai-native.spec.ts index 6887da4d20..e37696b0f8 100644 --- a/src/api/providers/__tests__/openai-native.spec.ts +++ b/src/api/providers/__tests__/openai-native.spec.ts @@ -264,6 +264,21 @@ describe("OpenAiNativeHandler", () => { expect(modelInfo.info.reasoningEffort).toBe("none") }) + it("should return GPT-5.5 model info when selected", () => { + const gpt55Handler = new OpenAiNativeHandler({ + ...mockOptions, + apiModelId: "gpt-5.5", + }) + + const modelInfo = gpt55Handler.getModel() + expect(modelInfo.id).toBe("gpt-5.5") + expect(modelInfo.info.maxTokens).toBe(128000) + expect(modelInfo.info.contextWindow).toBe(1_050_000) + expect(modelInfo.info.supportsVerbosity).toBe(true) + expect(modelInfo.info.supportsReasoningEffort).toEqual(["none", "low", "medium", "high", "xhigh"]) + expect(modelInfo.info.reasoningEffort).toBe("medium") + }) + it("should return GPT-5.4 Mini model info when selected", () => { const gpt54MiniHandler = new OpenAiNativeHandler({ ...mockOptions, @@ -462,6 +477,56 @@ describe("OpenAiNativeHandler", () => { expect(textChunks[0].text).toBe("GPT-5.4 reply") }) + it("should handle GPT-5.5 model with Responses API", async () => { + const mockFetch = vitest.fn().mockResolvedValue({ + ok: true, + body: new ReadableStream({ + start(controller) { + controller.enqueue( + new TextEncoder().encode( + 'data: {"type":"response.output_item.added","item":{"type":"text","text":"GPT-5.5 reply"}}\n\n', + ), + ) + controller.enqueue(new TextEncoder().encode("data: [DONE]\n\n")) + controller.close() + }, + }), + }) + global.fetch = mockFetch as any + + mockResponsesCreate.mockRejectedValue(new Error("SDK not available")) + + handler = new OpenAiNativeHandler({ + ...mockOptions, + apiModelId: "gpt-5.5", + }) + + const stream = handler.createMessage(systemPrompt, messages) + const chunks: any[] = [] + for await (const chunk of stream) { + chunks.push(chunk) + } + + expect(mockFetch).toHaveBeenCalledWith( + "https://api.openai.com/v1/responses", + expect.objectContaining({ + body: expect.any(String), + }), + ) + const body = (mockFetch.mock.calls[0][1] as any).body as string + const parsedBody = JSON.parse(body) + expect(parsedBody.model).toBe("gpt-5.5") + expect(parsedBody.max_output_tokens).toBe(128000) + expect(parsedBody.temperature).toBeUndefined() + expect(parsedBody.include).toEqual(["reasoning.encrypted_content"]) + expect(parsedBody.reasoning?.effort).toBe("medium") + expect(parsedBody.text?.verbosity).toBe("medium") + + const textChunks = chunks.filter((chunk) => chunk.type === "text") + expect(textChunks).toHaveLength(1) + expect(textChunks[0].text).toBe("GPT-5.5 reply") + }) + it("should handle GPT-5.3 Chat model with Responses API", async () => { // Mock fetch for Responses API const mockFetch = vitest.fn().mockResolvedValue({ From 0673d408a78f444773f98ed62da15a989d52539b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Dre=C5=BCewski?= Date: Wed, 17 Jun 2026 22:12:40 +0200 Subject: [PATCH 2/7] feat(ripgrep): add Show Ripgrep Diagnostic command (zoo #281) Port of Zoo-Code PR #281. Adds a user-triggerable diagnostic command that runs ripgrep binary resolution and writes a verbose report to a dedicated output channel + clipboard. Command id and titles adapted to Tumble Code; registered via getCommand() with showRipgrepDiagnostic added to the CommandId union. Co-authored-by: 0xMink <260166390+0xMink@users.noreply.github.com> Co-authored-by: edelauna <54631123+edelauna@users.noreply.github.com> Co-authored-by: Elliott de Launay --- ...6-17_zoo-281-ripgrep-diagnostic-command.md | 172 +++++++++ knip.json | 3 +- packages/types/src/vscode.ts | 2 + .../__tests__/registerCommands.spec.ts | 36 +- src/activate/registerCommands.ts | 21 +- src/esbuild.mjs | 2 +- src/package.json | 5 + src/package.nls.ca.json | 1 + src/package.nls.de.json | 1 + src/package.nls.es.json | 1 + src/package.nls.fr.json | 1 + src/package.nls.hi.json | 1 + src/package.nls.id.json | 1 + src/package.nls.it.json | 1 + src/package.nls.ja.json | 1 + src/package.nls.json | 1 + src/package.nls.ko.json | 1 + src/package.nls.nl.json | 1 + src/package.nls.pl.json | 1 + src/package.nls.pt-BR.json | 1 + src/package.nls.ru.json | 1 + src/package.nls.tr.json | 1 + src/package.nls.vi.json | 1 + src/package.nls.zh-CN.json | 1 + src/package.nls.zh-TW.json | 1 + .../ripgrep/__tests__/diagnostic.spec.ts | 365 ++++++++++++++++++ src/services/ripgrep/diagnostic.ts | 144 +++++++ src/services/ripgrep/index.ts | 35 +- src/services/ripgrep/internal/loadRipgrep.ts | 21 + 29 files changed, 808 insertions(+), 16 deletions(-) create mode 100644 ai_plans/2026-06-17_zoo-281-ripgrep-diagnostic-command.md create mode 100644 src/services/ripgrep/__tests__/diagnostic.spec.ts create mode 100644 src/services/ripgrep/diagnostic.ts create mode 100644 src/services/ripgrep/internal/loadRipgrep.ts diff --git a/ai_plans/2026-06-17_zoo-281-ripgrep-diagnostic-command.md b/ai_plans/2026-06-17_zoo-281-ripgrep-diagnostic-command.md new file mode 100644 index 0000000000..db5e14ef48 --- /dev/null +++ b/ai_plans/2026-06-17_zoo-281-ripgrep-diagnostic-command.md @@ -0,0 +1,172 @@ +# Port plan — Zoo PR #281 → `feature/zoo-281-ripgrep-diagnostic-command` + +## §0 Context & credit + +- **Upstream:** Zoo-Code PR #281 `feat(ripgrep): add Show Ripgrep Diagnostic +command` (squashed commit `d29520b5c`). +- **Authors (credit on commit):** + - `Co-authored-by: 0xMink <260166390+0xMink@users.noreply.github.com>` + - `Co-authored-by: edelauna <54631123+edelauna@users.noreply.github.com>` + - `Co-authored-by: Elliott de Launay ` +- **Canonical source diff:** `git -C ../Zoo-Code show d29520b5c` (28 files). + +## §1 What it does + +Adds a user-triggerable VS Code command (`showRipgrepDiagnostic`) that runs the +same hybrid ripgrep-resolution logic our search path uses and writes a verbose +report to a dedicated output channel + the clipboard. The report covers three +steps: + +1. `require("@vscode/ripgrep")` via a testable `loadRipgrep()` wrapper (surfaces + the `.asar → .asar.unpacked` rewrite and whether the file exists). +2. A path probe of every known `appRoot`-relative candidate path, marking each + `✓`/`✗`. +3. A `rg --version` spawn (5 s timeout) on the path `getBinPath()` selects — + directly catching the "file exists but spawn fails" failure mode. + +Motivated by the #248 ripgrep-universal layout bug (already ported to our fork): +debugging it required a custom instrumented VSIX. With this command shipped, +users hitting ripgrep-resolution weirdness can paste the diagnostic into a bug +report instead. + +The PR refactors `getBinPath` to share its candidate list with the diagnostic via +a new exported `ripgrepCandidatePaths(appRoot)` helper, and reintroduces the +`@vscode/ripgrep` esbuild `external` entry (so the `require()` resolves at runtime +against VS Code's own `node_modules` rather than being bundled). + +## §2 Fork adaptations (the judgment calls) + +This PR is Zoo-branded; our fork is **Tumble Code**. Decisions made: + +1. **Command id — routed through `getCommand()`, never hardcoded.** Upstream + registers `zoo-code.showRipgrepDiagnostic`. Our `getCommand(id)` in + `src/utils/commands.ts` produces `` `${Package.name}.${id}` `` → + `tumble-code.showRipgrepDiagnostic`. `diagnostic.ts` calls + `getCommand("showRipgrepDiagnostic")`. `"showRipgrepDiagnostic"` added to the + `CommandId` union in `packages/types/src/vscode.ts` (matching the existing + blank-line-separated style, appended after `toggleAutoApprove`). + +2. **`package.json` command contribution** uses literal command id + `tumble-code.showRipgrepDiagnostic` (matching siblings like + `tumble-code.plusButtonClicked`) with `"title": +"%command.showRipgrepDiagnostic.title%"` and `"category": +"%configuration.title%"`, inserted between the `acceptInput` and + `toggleAutoApprove` entries (mirroring the NLS insert position). + +3. **NLS title VALUE — brand-neutral.** Inspected our existing command titles in + `src/package.nls.json`: they are brand-neutral ("New Task", "Fix Code", "Accept + Input/Suggestion"), NOT prefixed with "Tumble Code:". So the English value is + **"Show Ripgrep Diagnostic"** (no brand prefix), matching upstream's English + value exactly (upstream's en value also had no brand prefix; only the + diagnostic report text and toast carry the brand). For the 17 non-English + locales I mirrored upstream's per-locale localized string verbatim — none of + upstream's title values contained "Zoo"/"Roo", so no brand-token swap was + needed in any locale title. Inserted between `command.acceptInput.title` and + `command.toggleAutoApprove.title` in every file. + +4. **Brand tokens INSIDE `diagnostic.ts` (user-facing report/channel/toast)** + swapped Zoo→Tumble: + + - OutputChannel name: `"Zoo Code Ripgrep Diagnostic"` → `"Tumble Code Ripgrep +Diagnostic"`. + - Report header line: `"Zoo Code Ripgrep Diagnostic (…)"` → `"Tumble Code +Ripgrep Diagnostic (…)"`. + - Toast: `"Zoo Code: ripgrep diagnostic copied to clipboard."` → `"Tumble + Code: ripgrep diagnostic copied to clipboard."`. + The test expectations were adapted to the Tumble strings accordingly. + +5. **`@vscode/ripgrep` dependency — NOT re-added to `package.json`.** Verified it + is absent from BOTH our `src/package.json` AND upstream's post-#281 + `src/package.json` (`@vscode/ripgrep` is a VS Code internal package resolved at + runtime, not an npm devDep in either tree). Only the two things genuinely + needed for the `require()` to compile+resolve were added: + + - `src/esbuild.mjs`: `"@vscode/ripgrep"` appended to the `external` array. + - `knip.json`: `"@vscode/ripgrep"` added to `ignore` so knip doesn't flag the + unlisted dependency. + (Upstream's commit message says "reintroduces the devDep", but the actual diff + never touched `package.json` deps — only `knip.json` + `esbuild.mjs`. We match + the real diff, not the prose.) + +6. **`registerCommands.ts` adaptation.** Our fork's pre-state used + `Record` and had no separate diagnostic registration. Applied + upstream's intent: added the import, the + `context.subscriptions.push(registerRipgrepDiagnosticCommand())` call, the + `CommandCallback` alias, and changed the map type to + `Record, CommandCallback>` so the + diagnostic's separate registration owns the OutputChannel lifecycle. + +## §3 How our `index.ts` differed from upstream pre-state + +Our fork already ported #248 (the `@vscode/ripgrep-universal` layout). So our +pre-#281 `getBinPath` was a 6-branch `checkPath(...)||...` chain that ALREADY +included the two `ripgrep-universal` candidates — upstream's pre-#281 had only the +4 classic candidates plus its own universal addition. Both converge on the same +6-candidate ordered list. The #281 refactor (extract `ripgrepCandidatePaths()`, +loop in `getBinPath`) applied cleanly on top of our 6-candidate version: the +extracted helper lists exactly our 6 candidates in the same order, and `getBinPath` +became a `for` loop returning the first existing path. No behavioral change to +resolution order. + +Confirmed `src/services/ripgrep/diagnostic.ts` and +`src/services/ripgrep/internal/loadRipgrep.ts` did NOT pre-exist (new files). + +## §4 Out of scope — do NOT do these + +- No TTS / router / cloud / Roo/Zoo branding reintroduced. +- No `package.json` dependency addition (§2.5). +- Internal ids stay (`@roo-code/types`, `Package.name` resolves to `tumble-code`). + +## §5 Files changed (execution checklist) + +### Types + +- `packages/types/src/vscode.ts`: add `"showRipgrepDiagnostic"` to `commandIds`. + +### New ripgrep modules + +- `src/services/ripgrep/internal/loadRipgrep.ts` (NEW): `loadRipgrep()` require + wrapper + `LoadRipgrepResult` type. +- `src/services/ripgrep/diagnostic.ts` (NEW): `trySpawnRipgrep`, + `getRipgrepDiagnostic` (data fn), `registerRipgrepDiagnosticCommand` (cmd + wrapper) — Tumble-branded strings. +- `src/services/ripgrep/index.ts`: extract `ripgrepCandidatePaths()` (exported), + rewrite `getBinPath` as a loop over it. + +### Wiring + +- `src/activate/registerCommands.ts`: import + register the diagnostic command; + exclude `showRipgrepDiagnostic` from `getCommandsMap`; `CommandCallback` alias. +- `src/package.json`: `contributes.commands` entry for + `tumble-code.showRipgrepDiagnostic`. +- `src/package.nls.json` + 17 locale files: `command.showRipgrepDiagnostic.title`. +- `src/esbuild.mjs`: `@vscode/ripgrep` external. +- `knip.json`: `@vscode/ripgrep` ignore. + +### Tests + +- `src/services/ripgrep/__tests__/diagnostic.spec.ts` (NEW): 21 data-fn + + registration tests, expectations adapted to `tumble-code.showRipgrepDiagnostic` + and the Tumble channel/toast strings. +- `src/activate/__tests__/registerCommands.spec.ts`: mock the diagnostic module, + add `commands.registerCommand` to the vscode mock, add a `registerCommands` + describe verifying the disposable lands in `context.subscriptions`. + +## §6 Verification (binary acceptance) + +- `cd src && npx vitest run services/ripgrep/__tests__/diagnostic.spec.ts +activate/__tests__/registerCommands.spec.ts` → 25/25 GREEN. +- `pnpm --filter @roo-code/types check-types` → clean. +- `cd src && pnpm check-types` (`tsc --noEmit`) → clean. +- No new "Zoo"/"Roo" user-facing strings introduced. + +## §7 Record in ledger + +```bash +node .claude/skills/zoo-port/scripts/zoo-prs.mjs record \ + --pr 281 --status ported \ + --branch feature/zoo-281-ripgrep-diagnostic-command \ + --plan ai_plans/2026-06-17_zoo-281-ripgrep-diagnostic-command.md +``` + +Commit (only if asked) with the three `Co-authored-by:` trailers from §0. diff --git a/knip.json b/knip.json index 67c9e9a746..a3382bd2c1 100644 --- a/knip.json +++ b/knip.json @@ -8,7 +8,8 @@ "src/workers/countTokens.ts", "src/extension.ts", "scripts/**", - "apps/cli/scripts/**" + "apps/cli/scripts/**", + "@vscode/ripgrep" ], "workspaces": { "src": { diff --git a/packages/types/src/vscode.ts b/packages/types/src/vscode.ts index fd28f2e994..ca6232dce6 100644 --- a/packages/types/src/vscode.ts +++ b/packages/types/src/vscode.ts @@ -47,6 +47,8 @@ export const commandIds = [ "acceptInput", "focusPanel", "toggleAutoApprove", + + "showRipgrepDiagnostic", ] as const export type CommandId = (typeof commandIds)[number] diff --git a/src/activate/__tests__/registerCommands.spec.ts b/src/activate/__tests__/registerCommands.spec.ts index f13bf83142..1111211a9a 100644 --- a/src/activate/__tests__/registerCommands.spec.ts +++ b/src/activate/__tests__/registerCommands.spec.ts @@ -2,7 +2,7 @@ import type { Mock } from "vitest" import * as vscode from "vscode" import { ClineProvider } from "../../core/webview/ClineProvider" -import { getVisibleProviderOrLog } from "../registerCommands" +import { getVisibleProviderOrLog, registerCommands } from "../registerCommands" vi.mock("execa", () => ({ execa: vi.fn(), @@ -13,6 +13,9 @@ vi.mock("vscode", () => ({ QuickFix: { value: "quickfix" }, RefactorRewrite: { value: "refactor.rewrite" }, }, + commands: { + registerCommand: vi.fn().mockReturnValue({ dispose: vi.fn() }), + }, window: { createTextEditorDecorationType: vi.fn().mockReturnValue({ dispose: vi.fn() }), }, @@ -29,6 +32,10 @@ vi.mock("vscode", () => ({ vi.mock("../../core/webview/ClineProvider") +vi.mock("../../services/ripgrep/diagnostic", () => ({ + registerRipgrepDiagnosticCommand: vi.fn().mockReturnValue({ dispose: vi.fn() }), +})) + describe("getVisibleProviderOrLog", () => { let mockOutputChannel: vscode.OutputChannel @@ -65,3 +72,30 @@ describe("getVisibleProviderOrLog", () => { expect(mockOutputChannel.appendLine).toHaveBeenCalledWith("Cannot find any visible Tumble Code instances.") }) }) + +describe("registerCommands", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it("registers the ripgrep diagnostic command and stores its disposable in context.subscriptions", async () => { + const { registerRipgrepDiagnosticCommand } = await import("../../services/ripgrep/diagnostic") + + const mockContext = { + subscriptions: [] as { dispose: () => void }[], + } as unknown as vscode.ExtensionContext + const mockOutputChannel = { appendLine: vi.fn() } as unknown as vscode.OutputChannel + const mockProvider = {} as ClineProvider + + registerCommands({ + context: mockContext, + outputChannel: mockOutputChannel, + provider: mockProvider, + }) + + const mock = vi.mocked(registerRipgrepDiagnosticCommand) + const disposable = mock.mock.results[0]?.value + expect(mock).toHaveBeenCalled() + expect(mockContext.subscriptions).toContain(disposable) + }) +}) diff --git a/src/activate/registerCommands.ts b/src/activate/registerCommands.ts index 9f4ac34c4e..432f0d1edf 100644 --- a/src/activate/registerCommands.ts +++ b/src/activate/registerCommands.ts @@ -13,6 +13,7 @@ import { handleNewTask } from "./handleTask" import { CodeIndexManager } from "../services/code-index/manager" import { importSettingsWithFeedback } from "../core/config/importExport" import { MdmService } from "../services/mdm/MdmService" +import { registerRipgrepDiagnosticCommand } from "../services/ripgrep/diagnostic" import { t } from "../i18n" /** @@ -68,9 +69,27 @@ export const registerCommands = (options: RegisterCommandOptions) => { const command = getCommand(id as CommandId) context.subscriptions.push(vscode.commands.registerCommand(command, callback)) } + + context.subscriptions.push(registerRipgrepDiagnosticCommand()) } -const getCommandsMap = ({ context, outputChannel, provider }: RegisterCommandOptions): Record => ({ +// `showRipgrepDiagnostic` is registered separately by +// `registerRipgrepDiagnosticCommand` (above), which owns the OutputChannel +// lifecycle alongside the command registration, so it's intentionally +// excluded from this map. +// +// Callback shape mirrors VS Code's own `commands.registerCommand` signature +// (`(...args: any[]) => any`), with the return narrowed to `unknown` so +// callers must inspect before using. `any[]` for args is unavoidable: the +// callbacks here are heterogeneous (`importSettings` takes an optional +// `filePath?: string`, others take none) and VS Code dispatches positional +// args dynamically. +type CommandCallback = (...args: any[]) => unknown +const getCommandsMap = ({ + context, + outputChannel, + provider, +}: RegisterCommandOptions): Record, CommandCallback> => ({ activationCompleted: () => {}, cloudButtonClicked: () => { const visibleProvider = getVisibleProviderOrLog(outputChannel) diff --git a/src/esbuild.mjs b/src/esbuild.mjs index 6089ac3306..8a23090a0e 100644 --- a/src/esbuild.mjs +++ b/src/esbuild.mjs @@ -121,7 +121,7 @@ async function main() { // global-agent must be external because it dynamically patches Node.js http/https modules // which breaks when bundled. It needs access to the actual Node.js module instances. // undici must be bundled because our VSIX is packaged with `--no-dependencies`. - external: ["vscode", "esbuild", "global-agent"], + external: ["vscode", "esbuild", "global-agent", "@vscode/ripgrep"], } /** diff --git a/src/package.json b/src/package.json index bbd268566d..2e62d030e7 100644 --- a/src/package.json +++ b/src/package.json @@ -166,6 +166,11 @@ "title": "%command.acceptInput.title%", "category": "%configuration.title%" }, + { + "command": "tumble-code.showRipgrepDiagnostic", + "title": "%command.showRipgrepDiagnostic.title%", + "category": "%configuration.title%" + }, { "command": "tumble-code.toggleAutoApprove", "title": "%command.toggleAutoApprove.title%", diff --git a/src/package.nls.ca.json b/src/package.nls.ca.json index b8f688dd83..fac4d22f85 100644 --- a/src/package.nls.ca.json +++ b/src/package.nls.ca.json @@ -14,6 +14,7 @@ "command.terminal.fixCommand.title": "Corregir Aquesta Ordre", "command.terminal.explainCommand.title": "Explicar Aquesta Ordre", "command.acceptInput.title": "Acceptar Entrada/Suggeriment", + "command.showRipgrepDiagnostic.title": "Mostra el diagnòstic de Ripgrep", "command.toggleAutoApprove.title": "Alternar Auto-Aprovació", "views.activitybar.title": "Tumble Code", "views.contextMenu.label": "Tumble Code", diff --git a/src/package.nls.de.json b/src/package.nls.de.json index 2f61573991..cad422173a 100644 --- a/src/package.nls.de.json +++ b/src/package.nls.de.json @@ -14,6 +14,7 @@ "command.terminal.fixCommand.title": "Diesen Befehl Reparieren", "command.terminal.explainCommand.title": "Diesen Befehl Erklären", "command.acceptInput.title": "Eingabe/Vorschlag Akzeptieren", + "command.showRipgrepDiagnostic.title": "Ripgrep-Diagnose anzeigen", "command.toggleAutoApprove.title": "Auto-Genehmigung Umschalten", "views.activitybar.title": "Tumble Code", "views.contextMenu.label": "Tumble Code", diff --git a/src/package.nls.es.json b/src/package.nls.es.json index 9efe9366ca..dafde83884 100644 --- a/src/package.nls.es.json +++ b/src/package.nls.es.json @@ -14,6 +14,7 @@ "command.terminal.fixCommand.title": "Corregir Este Comando", "command.terminal.explainCommand.title": "Explicar Este Comando", "command.acceptInput.title": "Aceptar Entrada/Sugerencia", + "command.showRipgrepDiagnostic.title": "Mostrar diagnóstico de Ripgrep", "command.toggleAutoApprove.title": "Alternar Auto-Aprobación", "views.activitybar.title": "Tumble Code", "views.contextMenu.label": "Tumble Code", diff --git a/src/package.nls.fr.json b/src/package.nls.fr.json index 8237a851f9..75ade57b30 100644 --- a/src/package.nls.fr.json +++ b/src/package.nls.fr.json @@ -14,6 +14,7 @@ "command.terminal.fixCommand.title": "Corriger cette Commande", "command.terminal.explainCommand.title": "Expliquer cette Commande", "command.acceptInput.title": "Accepter l'Entrée/Suggestion", + "command.showRipgrepDiagnostic.title": "Afficher le diagnostic Ripgrep", "command.toggleAutoApprove.title": "Basculer Auto-Approbation", "views.activitybar.title": "Tumble Code", "views.contextMenu.label": "Tumble Code", diff --git a/src/package.nls.hi.json b/src/package.nls.hi.json index e04b117ba8..eee792cfc1 100644 --- a/src/package.nls.hi.json +++ b/src/package.nls.hi.json @@ -14,6 +14,7 @@ "command.terminal.fixCommand.title": "यह कमांड ठीक करें", "command.terminal.explainCommand.title": "यह कमांड समझाएं", "command.acceptInput.title": "इनपुट/सुझाव स्वीकारें", + "command.showRipgrepDiagnostic.title": "Ripgrep डायग्नोस्टिक दिखाएं", "command.toggleAutoApprove.title": "ऑटो-अनुमोदन टॉगल करें", "views.activitybar.title": "Tumble Code", "views.contextMenu.label": "Tumble Code", diff --git a/src/package.nls.id.json b/src/package.nls.id.json index 397dfe8ee9..011e8f0e48 100644 --- a/src/package.nls.id.json +++ b/src/package.nls.id.json @@ -24,6 +24,7 @@ "command.terminal.fixCommand.title": "Perbaiki Perintah Ini", "command.terminal.explainCommand.title": "Jelaskan Perintah Ini", "command.acceptInput.title": "Terima Input/Saran", + "command.showRipgrepDiagnostic.title": "Tampilkan Diagnostik Ripgrep", "command.toggleAutoApprove.title": "Alihkan Persetujuan Otomatis", "configuration.title": "Tumble Code", "commands.allowedCommands.description": "Perintah yang dapat dijalankan secara otomatis ketika 'Selalu setujui operasi eksekusi' diaktifkan", diff --git a/src/package.nls.it.json b/src/package.nls.it.json index d4cac6bb2b..a40aaefeb0 100644 --- a/src/package.nls.it.json +++ b/src/package.nls.it.json @@ -14,6 +14,7 @@ "command.terminal.fixCommand.title": "Correggi Questo Comando", "command.terminal.explainCommand.title": "Spiega Questo Comando", "command.acceptInput.title": "Accetta Input/Suggerimento", + "command.showRipgrepDiagnostic.title": "Mostra diagnostica Ripgrep", "command.toggleAutoApprove.title": "Attiva/Disattiva Auto-Approvazione", "views.activitybar.title": "Tumble Code", "views.contextMenu.label": "Tumble Code", diff --git a/src/package.nls.ja.json b/src/package.nls.ja.json index 283660c504..904693d2c7 100644 --- a/src/package.nls.ja.json +++ b/src/package.nls.ja.json @@ -24,6 +24,7 @@ "command.terminal.fixCommand.title": "このコマンドを修正", "command.terminal.explainCommand.title": "このコマンドを説明", "command.acceptInput.title": "入力/提案を承認", + "command.showRipgrepDiagnostic.title": "Ripgrep 診断を表示", "command.toggleAutoApprove.title": "自動承認を切替", "configuration.title": "Tumble Code", "commands.allowedCommands.description": "'常に実行操作を承認する'が有効な場合に自動実行できるコマンド", diff --git a/src/package.nls.json b/src/package.nls.json index 2346e9a030..add7a84113 100644 --- a/src/package.nls.json +++ b/src/package.nls.json @@ -24,6 +24,7 @@ "command.terminal.fixCommand.title": "Fix This Command", "command.terminal.explainCommand.title": "Explain This Command", "command.acceptInput.title": "Accept Input/Suggestion", + "command.showRipgrepDiagnostic.title": "Show Ripgrep Diagnostic", "command.toggleAutoApprove.title": "Toggle Auto-Approve", "configuration.title": "Tumble Code", "commands.allowedCommands.description": "Commands that can be auto-executed when 'Always approve execute operations' is enabled", diff --git a/src/package.nls.ko.json b/src/package.nls.ko.json index 30fafeee39..c699c7109a 100644 --- a/src/package.nls.ko.json +++ b/src/package.nls.ko.json @@ -14,6 +14,7 @@ "command.terminal.fixCommand.title": "이 명령어 수정", "command.terminal.explainCommand.title": "이 명령어 설명", "command.acceptInput.title": "입력/제안 수락", + "command.showRipgrepDiagnostic.title": "Ripgrep 진단 표시", "command.toggleAutoApprove.title": "자동 승인 전환", "views.activitybar.title": "Tumble Code", "views.contextMenu.label": "Tumble Code", diff --git a/src/package.nls.nl.json b/src/package.nls.nl.json index c1a1c394af..3052213b8b 100644 --- a/src/package.nls.nl.json +++ b/src/package.nls.nl.json @@ -24,6 +24,7 @@ "command.terminal.fixCommand.title": "Repareer Dit Commando", "command.terminal.explainCommand.title": "Leg Dit Commando Uit", "command.acceptInput.title": "Invoer/Suggestie Accepteren", + "command.showRipgrepDiagnostic.title": "Ripgrep-diagnose weergeven", "command.toggleAutoApprove.title": "Auto-Goedkeuring Schakelen", "configuration.title": "Tumble Code", "commands.allowedCommands.description": "Commando's die automatisch kunnen worden uitgevoerd wanneer 'Altijd goedkeuren uitvoerbewerkingen' is ingeschakeld", diff --git a/src/package.nls.pl.json b/src/package.nls.pl.json index 3eb1501171..20abd0f656 100644 --- a/src/package.nls.pl.json +++ b/src/package.nls.pl.json @@ -14,6 +14,7 @@ "command.terminal.fixCommand.title": "Napraw tę Komendę", "command.terminal.explainCommand.title": "Wyjaśnij tę Komendę", "command.acceptInput.title": "Akceptuj Wprowadzanie/Sugestię", + "command.showRipgrepDiagnostic.title": "Pokaż diagnostykę Ripgrep", "command.toggleAutoApprove.title": "Przełącz Auto-Zatwierdzanie", "views.activitybar.title": "Tumble Code", "views.contextMenu.label": "Tumble Code", diff --git a/src/package.nls.pt-BR.json b/src/package.nls.pt-BR.json index dcd59910b8..c538bfbc1d 100644 --- a/src/package.nls.pt-BR.json +++ b/src/package.nls.pt-BR.json @@ -14,6 +14,7 @@ "command.terminal.fixCommand.title": "Corrigir Este Comando", "command.terminal.explainCommand.title": "Explicar Este Comando", "command.acceptInput.title": "Aceitar Entrada/Sugestão", + "command.showRipgrepDiagnostic.title": "Mostrar diagnóstico do Ripgrep", "command.toggleAutoApprove.title": "Alternar Auto-Aprovação", "views.activitybar.title": "Tumble Code", "views.contextMenu.label": "Tumble Code", diff --git a/src/package.nls.ru.json b/src/package.nls.ru.json index 6089f81d4e..369b2c55b8 100644 --- a/src/package.nls.ru.json +++ b/src/package.nls.ru.json @@ -24,6 +24,7 @@ "command.terminal.fixCommand.title": "Исправить эту команду", "command.terminal.explainCommand.title": "Объяснить эту команду", "command.acceptInput.title": "Принять ввод/предложение", + "command.showRipgrepDiagnostic.title": "Показать диагностику Ripgrep", "command.toggleAutoApprove.title": "Переключить Авто-Подтверждение", "configuration.title": "Tumble Code", "commands.allowedCommands.description": "Команды, которые могут быть автоматически выполнены, когда включена опция 'Всегда подтверждать операции выполнения'", diff --git a/src/package.nls.tr.json b/src/package.nls.tr.json index e68886f3b3..6d46434a1b 100644 --- a/src/package.nls.tr.json +++ b/src/package.nls.tr.json @@ -14,6 +14,7 @@ "command.terminal.fixCommand.title": "Bu Komutu Düzelt", "command.terminal.explainCommand.title": "Bu Komutu Açıkla", "command.acceptInput.title": "Girişi/Öneriyi Kabul Et", + "command.showRipgrepDiagnostic.title": "Ripgrep Tanılamasını Göster", "command.toggleAutoApprove.title": "Otomatik Onayı Değiştir", "views.activitybar.title": "Tumble Code", "views.contextMenu.label": "Tumble Code", diff --git a/src/package.nls.vi.json b/src/package.nls.vi.json index d8c2f62c2e..286908a2a2 100644 --- a/src/package.nls.vi.json +++ b/src/package.nls.vi.json @@ -14,6 +14,7 @@ "command.terminal.fixCommand.title": "Sửa Lệnh Này", "command.terminal.explainCommand.title": "Giải Thích Lệnh Này", "command.acceptInput.title": "Chấp Nhận Đầu Vào/Gợi Ý", + "command.showRipgrepDiagnostic.title": "Hiển thị chẩn đoán Ripgrep", "command.toggleAutoApprove.title": "Bật/Tắt Tự Động Phê Duyệt", "views.activitybar.title": "Tumble Code", "views.contextMenu.label": "Tumble Code", diff --git a/src/package.nls.zh-CN.json b/src/package.nls.zh-CN.json index 8218c5b0a8..66fcec6023 100644 --- a/src/package.nls.zh-CN.json +++ b/src/package.nls.zh-CN.json @@ -14,6 +14,7 @@ "command.terminal.fixCommand.title": "修复此命令", "command.terminal.explainCommand.title": "解释此命令", "command.acceptInput.title": "接受输入/建议", + "command.showRipgrepDiagnostic.title": "显示 Ripgrep 诊断", "command.toggleAutoApprove.title": "切换自动批准", "views.activitybar.title": "Tumble Code", "views.contextMenu.label": "Tumble Code", diff --git a/src/package.nls.zh-TW.json b/src/package.nls.zh-TW.json index c52ce333d0..6d31ebe624 100644 --- a/src/package.nls.zh-TW.json +++ b/src/package.nls.zh-TW.json @@ -14,6 +14,7 @@ "command.terminal.fixCommand.title": "修復此命令", "command.terminal.explainCommand.title": "解釋此命令", "command.acceptInput.title": "接受輸入/建議", + "command.showRipgrepDiagnostic.title": "顯示 Ripgrep 診斷", "command.toggleAutoApprove.title": "切換自動批准", "views.activitybar.title": "Tumble Code", "views.contextMenu.label": "Tumble Code", diff --git a/src/services/ripgrep/__tests__/diagnostic.spec.ts b/src/services/ripgrep/__tests__/diagnostic.spec.ts new file mode 100644 index 0000000000..b41905259a --- /dev/null +++ b/src/services/ripgrep/__tests__/diagnostic.spec.ts @@ -0,0 +1,365 @@ +// npx vitest run src/services/ripgrep/__tests__/diagnostic.spec.ts + +import * as path from "path" +import { EventEmitter } from "events" + +import { vi, describe, it, expect, beforeEach } from "vitest" +import * as vscode from "vscode" +import * as childProcess from "child_process" + +import { getRipgrepDiagnostic, registerRipgrepDiagnosticCommand, trySpawnRipgrep } from "../diagnostic" + +const ripgrepMock = vi.hoisted(() => ({ + value: undefined as { rgPath?: string; loadError?: string } | undefined, +})) + +const fsMock = vi.hoisted(() => ({ + existing: new Set(), +})) + +const getBinPathMock = vi.hoisted(() => ({ value: undefined as string | undefined })) + +type SpawnResult = Awaited> +const spawnMock = vi.hoisted(() => ({ + result: { stdout: "ripgrep 14.1.0", exitCode: 0 } as SpawnResult, +})) + +function makeFakeProc(result: SpawnResult) { + const proc = new EventEmitter() as EventEmitter & { + stdout: EventEmitter + stderr: EventEmitter + kill: () => void + } + proc.stdout = new EventEmitter() + proc.stderr = new EventEmitter() + proc.kill = vi.fn() + setImmediate(() => { + if (result.spawnError) { + proc.emit("error", new Error(result.spawnError)) + } else if (result.timedOut) { + proc.emit("close", null, "SIGTERM") + } else { + if (result.stdout) proc.stdout.emit("data", Buffer.from(result.stdout)) + if (result.stderr) proc.stderr.emit("data", Buffer.from(result.stderr)) + proc.emit("close", result.exitCode ?? 0, null) + } + }) + return proc +} + +vi.mock("../internal/loadRipgrep", () => ({ + loadRipgrep: () => ripgrepMock.value, +})) + +vi.mock("../../../utils/fs", () => ({ + fileExistsAtPath: (p: string) => Promise.resolve(fsMock.existing.has(p)), +})) + +vi.mock("../index", async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + getBinPath: () => Promise.resolve(getBinPathMock.value), + } +}) + +vi.mock("child_process", async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, spawn: vi.fn() } +}) + +const mockChannel = { + clear: vi.fn(), + appendLine: vi.fn(), + show: vi.fn(), + dispose: vi.fn(), +} + +const mockCommand = { dispose: vi.fn() } + +vi.mock("vscode", () => ({ + version: "1.99.0", + env: { appRoot: "/mock/appRoot", clipboard: { writeText: vi.fn() } }, + window: { + createOutputChannel: vi.fn(() => mockChannel), + showInformationMessage: vi.fn(), + }, + commands: { registerCommand: vi.fn(() => mockCommand) }, + Disposable: { + from: vi.fn((...items: { dispose(): void }[]) => ({ dispose: () => items.forEach((d) => d.dispose()) })), + }, +})) + +const APP_ROOT = "/app" + +const binName = process.platform.startsWith("win") ? "rg.exe" : "rg" +const universalRelBin = `bin/${process.platform}-${process.arch}/${binName}` + +const expectedCandidates = [ + path.join(APP_ROOT, "node_modules", "@vscode", "ripgrep", "bin", binName), + path.join(APP_ROOT, "node_modules", "vscode-ripgrep", "bin", binName), + path.join(APP_ROOT, "node_modules.asar.unpacked", "vscode-ripgrep", "bin", binName), + path.join(APP_ROOT, "node_modules.asar.unpacked", "@vscode", "ripgrep", "bin", binName), + path.join(APP_ROOT, "node_modules", "@vscode", "ripgrep-universal", ...universalRelBin.split("/")), + path.join(APP_ROOT, "node_modules.asar.unpacked", "@vscode", "ripgrep-universal", ...universalRelBin.split("/")), +] + +describe("getRipgrepDiagnostic", () => { + beforeEach(() => { + ripgrepMock.value = undefined + fsMock.existing = new Set() + getBinPathMock.value = undefined + spawnMock.result = { stdout: "ripgrep 14.1.0", exitCode: 0 } + vi.mocked(childProcess.spawn).mockImplementation(() => makeFakeProc(spawnMock.result) as any) + }) + + it("includes rgPath and fileExistsAtPath: true when loadRipgrep returns an existing path", async () => { + const rgPath = "/some/path" + ripgrepMock.value = { rgPath } + fsMock.existing = new Set([rgPath]) + + const report = await getRipgrepDiagnostic(APP_ROOT) + + expect(report).toContain("rgPath: /some/path") + expect(report).toContain("fileExistsAtPath: true") + expect(report).toContain("after .asar→.asar.unpacked: /some/path") + }) + + it("rewrites node_modules.asar to node_modules.asar.unpacked in the report", async () => { + const rgPath = "/app/node_modules.asar/foo/rg" + const substituted = "/app/node_modules.asar.unpacked/foo/rg" + ripgrepMock.value = { rgPath } + fsMock.existing = new Set([substituted]) + + const report = await getRipgrepDiagnostic(APP_ROOT) + + expect(report).toContain(`after .asar→.asar.unpacked: ${substituted}`) + expect(report).toContain("fileExistsAtPath: true") + }) + + it("does not double-substitute when rgPath already contains node_modules.asar.unpacked", async () => { + // A previous `\b` regex matched the `r`/`.` boundary inside + // `node_modules.asar.unpacked`, producing `.unpacked.unpacked`. + const rgPath = "/app/node_modules.asar.unpacked/foo/rg" + ripgrepMock.value = { rgPath } + fsMock.existing = new Set([rgPath]) + + const report = await getRipgrepDiagnostic(APP_ROOT) + + expect(report).toContain(`after .asar→.asar.unpacked: ${rgPath}`) + expect(report).not.toContain("node_modules.asar.unpacked.unpacked") + expect(report).toContain("fileExistsAtPath: true") + }) + + it("reports require failure when loadRipgrep returns undefined", async () => { + ripgrepMock.value = undefined + + const report = await getRipgrepDiagnostic(APP_ROOT) + + expect(report).toContain("loadRipgrep() returned undefined (require threw)") + }) + + it("reports the loadError when loadRipgrep returns a loadError field", async () => { + ripgrepMock.value = { loadError: "Cannot find module '@vscode/ripgrep'" } + + const report = await getRipgrepDiagnostic(APP_ROOT) + + expect(report).toContain("loadRipgrep() returned loadError: Cannot find module '@vscode/ripgrep'") + // Should not also push the success-branch lines. + expect(report).not.toContain("after .asar→.asar.unpacked:") + expect(report).not.toContain("rgPath:") + }) + + it("reports rgPath: (undefined) when loadRipgrep returns an object without rgPath", async () => { + ripgrepMock.value = {} + + const report = await getRipgrepDiagnostic(APP_ROOT) + + expect(report).toContain("rgPath: (undefined)") + expect(report).not.toContain("after .asar→.asar.unpacked:") + }) + + it("marks only the first probe candidate as found when only it exists", async () => { + fsMock.existing = new Set([expectedCandidates[0]]) + + const report = await getRipgrepDiagnostic(APP_ROOT) + + const found = expectedCandidates.filter((c) => report.includes(`✓ ${c}`)) + const missing = expectedCandidates.filter((c) => report.includes(`✗ ${c}`)) + + expect(found).toEqual([expectedCandidates[0]]) + expect(missing).toEqual(expectedCandidates.slice(1)) + }) + + it("marks all probe candidates as missing when none exist", async () => { + fsMock.existing = new Set() + + const report = await getRipgrepDiagnostic(APP_ROOT) + + for (const candidate of expectedCandidates) { + expect(report).toContain(`✗ ${candidate}`) + } + expect(report).not.toContain("✓ ") + }) + + it("rewrites node_modules.asar to node_modules.asar.unpacked on Windows paths (backslash separator)", async () => { + // Pure string-substitution test: the literal `win32-x64` segment is + // not derived from process.platform/arch, so this runs on any host. + const rgPath = "C:\\app\\node_modules.asar\\@vscode\\ripgrep-universal\\bin\\win32-x64\\rg.exe" + const substituted = "C:\\app\\node_modules.asar.unpacked\\@vscode\\ripgrep-universal\\bin\\win32-x64\\rg.exe" + ripgrepMock.value = { rgPath } + fsMock.existing = new Set([substituted]) + + const report = await getRipgrepDiagnostic("C:\\app") + + expect(report).toContain(`after .asar→.asar.unpacked: ${substituted}`) + expect(report).toContain("fileExistsAtPath: true") + }) + + it("returns an explanatory report when vscode.env.appRoot is empty", async () => { + const report = await getRipgrepDiagnostic("") + + expect(report).toContain("vscode.env.appRoot: (empty)") + expect(report).toContain("Cannot probe paths: vscode.env.appRoot is empty.") + // step 1 should still run + expect(report).toContain('--- step 1: require("@vscode/ripgrep") via loadRipgrep ---') + // path probe and spawn test should NOT have run + expect(report).not.toContain("--- step 2: path probe under appRoot ---") + expect(report).not.toContain("--- step 3: spawn rg --version on selected path ---") + }) + + it("reports getBinPath() undefined when no candidate exists", async () => { + getBinPathMock.value = undefined + + const report = await getRipgrepDiagnostic(APP_ROOT) + + expect(report).toContain("--- step 3: spawn rg --version on selected path ---") + expect(report).toContain("getBinPath() returned undefined") + }) + + it("reports spawn success with exit code and stdout", async () => { + getBinPathMock.value = expectedCandidates[4] + fsMock.existing = new Set([expectedCandidates[4]]) + spawnMock.result = { stdout: "ripgrep 14.1.0", exitCode: 0 } + + const report = await getRipgrepDiagnostic(APP_ROOT) + + expect(report).toContain(`getBinPath() selected: ${expectedCandidates[4]}`) + expect(report).toContain(`selectedPath JSON: ${JSON.stringify(expectedCandidates[4])}`) + expect(report).toContain("exit code: 0") + expect(report).toContain("stdout: ripgrep 14.1.0") + }) + + it("reports spawn ENOENT — the file-exists-but-spawn-fails failure mode", async () => { + getBinPathMock.value = expectedCandidates[4] + fsMock.existing = new Set([expectedCandidates[4]]) + spawnMock.result = { spawnError: "spawn ENOENT" } + + const report = await getRipgrepDiagnostic(APP_ROOT) + + expect(report).toContain("spawn error: spawn ENOENT") + expect(report).not.toContain("exit code:") + }) + + it("passes the selected path, ['--version'], and timeout option to spawn", async () => { + getBinPathMock.value = expectedCandidates[4] + fsMock.existing = new Set([expectedCandidates[4]]) + + await getRipgrepDiagnostic(APP_ROOT) + + expect(vi.mocked(childProcess.spawn)).toHaveBeenCalledWith(expectedCandidates[4], ["--version"], { + timeout: 5_000, + }) + }) + + it("reports timed out when the process is killed with SIGTERM", async () => { + getBinPathMock.value = expectedCandidates[4] + fsMock.existing = new Set([expectedCandidates[4]]) + spawnMock.result = { timedOut: true } + + const report = await getRipgrepDiagnostic(APP_ROOT) + + expect(report).toContain("timed out after 5000ms") + expect(report).not.toContain("exit code:") + }) + + it("reports stderr when the process exits with a non-zero code", async () => { + getBinPathMock.value = expectedCandidates[4] + fsMock.existing = new Set([expectedCandidates[4]]) + spawnMock.result = { exitCode: 1, stderr: "error: unrecognised flag" } + + const report = await getRipgrepDiagnostic(APP_ROOT) + + expect(report).toContain("exit code: 1") + expect(report).toContain("stderr: error: unrecognised flag") + }) + + it("omits the stderr line when stderr is empty", async () => { + getBinPathMock.value = expectedCandidates[4] + fsMock.existing = new Set([expectedCandidates[4]]) + spawnMock.result = { stdout: "ripgrep 14.1.0", exitCode: 0, stderr: "" } + + const report = await getRipgrepDiagnostic(APP_ROOT) + + expect(report).not.toContain("stderr:") + }) + + it("includes JSON-escaped path for the rgPath from loadRipgrep", async () => { + ripgrepMock.value = { rgPath: "/path/with spaces/rg" } + fsMock.existing = new Set(["/path/with spaces/rg"]) + + const report = await getRipgrepDiagnostic(APP_ROOT) + + expect(report).toContain(`rgPath JSON: ${JSON.stringify("/path/with spaces/rg")}`) + }) +}) + +describe("registerRipgrepDiagnosticCommand", () => { + beforeEach(() => { + vi.clearAllMocks() + ripgrepMock.value = undefined + fsMock.existing = new Set() + }) + + it("creates an output channel and registers the command", () => { + registerRipgrepDiagnosticCommand() + + expect(vscode.window.createOutputChannel).toHaveBeenCalledWith("Tumble Code Ripgrep Diagnostic") + expect(vscode.commands.registerCommand).toHaveBeenCalledWith( + expect.stringContaining("showRipgrepDiagnostic"), + expect.any(Function), + ) + }) + + it("returns a Disposable that disposes both the command and channel", () => { + const disposable = registerRipgrepDiagnosticCommand() + disposable.dispose() + + expect(vscode.Disposable.from).toHaveBeenCalledWith(mockCommand, mockChannel) + }) + + it("clears, appends, and shows the channel when the command runs", async () => { + registerRipgrepDiagnosticCommand() + + const [[, handler]] = vi.mocked(vscode.commands.registerCommand).mock.calls + await handler() + + expect(mockChannel.clear).toHaveBeenCalled() + expect(mockChannel.appendLine).toHaveBeenCalledWith(expect.stringContaining("Tumble Code Ripgrep Diagnostic")) + expect(mockChannel.show).toHaveBeenCalledWith(true) + }) + + it("writes the report to the clipboard and shows a toast when the command runs", async () => { + registerRipgrepDiagnosticCommand() + + const [[, handler]] = vi.mocked(vscode.commands.registerCommand).mock.calls + await handler() + + expect(vscode.env.clipboard.writeText).toHaveBeenCalledWith( + expect.stringContaining("Tumble Code Ripgrep Diagnostic"), + ) + expect(vscode.window.showInformationMessage).toHaveBeenCalledWith( + "Tumble Code: ripgrep diagnostic copied to clipboard.", + ) + }) +}) diff --git a/src/services/ripgrep/diagnostic.ts b/src/services/ripgrep/diagnostic.ts new file mode 100644 index 0000000000..e74800face --- /dev/null +++ b/src/services/ripgrep/diagnostic.ts @@ -0,0 +1,144 @@ +import * as childProcess from "child_process" +import * as vscode from "vscode" + +import { fileExistsAtPath } from "../../utils/fs" +import { getCommand } from "../../utils/commands" +import { loadRipgrep } from "./internal/loadRipgrep" +import { getBinPath, ripgrepCandidatePaths } from "./index" + +const SPAWN_TIMEOUT_MS = 5_000 + +/** + * Attempts `rg --version` with a 5 s timeout. + * Returns { stdout, stderr, exitCode } on normal exit, { timedOut } on timeout, + * or { spawnError } when spawn itself fails (e.g. ENOENT). + */ +export function trySpawnRipgrep(rgPath: string): Promise<{ + stdout?: string + stderr?: string + exitCode?: number + timedOut?: true + spawnError?: string +}> { + return new Promise((resolve) => { + let proc: childProcess.ChildProcess + try { + proc = childProcess.spawn(rgPath, ["--version"], { timeout: SPAWN_TIMEOUT_MS }) + } catch (err) { + resolve({ spawnError: err instanceof Error ? err.message : String(err) }) + return + } + + let stdout = "" + let stderr = "" + proc.stdout?.on("data", (d: Buffer) => (stdout += d.toString())) + proc.stderr?.on("data", (d: Buffer) => (stderr += d.toString())) + + proc.on("error", (err) => resolve({ spawnError: err.message })) + proc.on("close", (code, signal) => { + if (signal === "SIGTERM" && code === null) { + resolve({ timedOut: true }) + } else { + resolve({ stdout: stdout.trim(), stderr: stderr.trim(), exitCode: code ?? -1 }) + } + }) + }) +} + +/** + * Produces a textual diagnostic report of how ripgrep would be resolved + * for the given VS Code installation. Pure data function — no UI side + * effects — so it's fully unit-testable. + * + * Step 1 tries `loadRipgrep()` (CommonJS require, hits VS Code's + * extHost interceptor on builds that have completed the + * `@vscode/ripgrep` → `@vscode/ripgrep-universal` migration). + * Step 2 probes every known `vscode.env.appRoot`-relative path and + * reports which ones exist on disk. Step 2 is skipped when appRoot is empty. + * Step 3 runs `rg --version` on the path getBinPath() would select, directly + * testing whether spawn succeeds (the failure mode Naved reported). + */ +export async function getRipgrepDiagnostic(vscodeAppRoot: string): Promise { + const appRootEmpty = !vscodeAppRoot || vscodeAppRoot.trim() === "" + const lines: string[] = [ + `Tumble Code Ripgrep Diagnostic (${new Date().toISOString()})`, + `vscode.version: ${vscode.version}`, + `vscode.env.appRoot: ${appRootEmpty ? "(empty)" : vscodeAppRoot}`, + ...(appRootEmpty ? [] : [`process.platform/arch: ${process.platform}/${process.arch}`]), + ``, + `--- step 1: require("@vscode/ripgrep") via loadRipgrep ---`, + ] + + const m = loadRipgrep() + if (!m) { + lines.push(`loadRipgrep() returned undefined (require threw)`) + } else if (m.loadError) { + lines.push(`loadRipgrep() returned loadError: ${m.loadError}`) + } else { + const keys = Object.keys(m).join(",") || "(none)" + lines.push(`loadRipgrep() returned object. keys: ${keys}`) + lines.push(`rgPath: ${m.rgPath ?? "(undefined)"}`) + lines.push(`rgPath JSON: ${JSON.stringify(m.rgPath)}`) + if (m.rgPath) { + // Path-separator lookahead instead of `\b` — `\b` matches at the `r`/`.` boundary + // inside `node_modules.asar.unpacked` too, producing a double `.unpacked.unpacked`. + const fixed = m.rgPath.replace(/node_modules\.asar(?=[\\/])/, "node_modules.asar.unpacked") + lines.push(`after .asar→.asar.unpacked: ${fixed}`) + lines.push(`fileExistsAtPath: ${await fileExistsAtPath(fixed)}`) + } + } + + if (appRootEmpty) { + lines.push(``) + lines.push(`Cannot probe paths: vscode.env.appRoot is empty.`) + } else { + lines.push(``) + lines.push(`--- step 2: path probe under appRoot ---`) + for (const candidate of ripgrepCandidatePaths(vscodeAppRoot)) { + const exists = await fileExistsAtPath(candidate) + lines.push(` ${exists ? "✓" : "✗"} ${candidate}`) + } + + lines.push(``) + lines.push(`--- step 3: spawn rg --version on selected path ---`) + const selectedPath = await getBinPath(vscodeAppRoot) + if (!selectedPath) { + lines.push(`getBinPath() returned undefined — no candidate path exists`) + } else { + lines.push(`getBinPath() selected: ${selectedPath}`) + lines.push(`selectedPath JSON: ${JSON.stringify(selectedPath)}`) + const result = await trySpawnRipgrep(selectedPath) + if (result.spawnError) { + lines.push(`spawn error: ${result.spawnError}`) + } else if (result.timedOut) { + lines.push(`timed out after ${SPAWN_TIMEOUT_MS}ms`) + } else { + lines.push(`exit code: ${result.exitCode}`) + lines.push(`stdout: ${result.stdout || "(empty)"}`) + if (result.stderr) lines.push(`stderr: ${result.stderr}`) + } + } + } + + return lines.join("\n") +} + +/** + * Registers the `tumble-code.showRipgrepDiagnostic` command. Thin wrapper — + * runs `getRipgrepDiagnostic`, shows the result in an output channel, + * copies it to the clipboard, and shows an info toast. The OutputChannel + * is created once at registration and disposed alongside the command via + * the composite Disposable returned here. + */ +export function registerRipgrepDiagnosticCommand(): vscode.Disposable { + const channel = vscode.window.createOutputChannel("Tumble Code Ripgrep Diagnostic") + const command = vscode.commands.registerCommand(getCommand("showRipgrepDiagnostic"), async () => { + const report = await getRipgrepDiagnostic(vscode.env.appRoot) + channel.clear() + channel.appendLine(report) + channel.show(true) + await vscode.env.clipboard.writeText(report) + await vscode.window.showInformationMessage("Tumble Code: ripgrep diagnostic copied to clipboard.") + }) + return vscode.Disposable.from(command, channel) +} diff --git a/src/services/ripgrep/index.ts b/src/services/ripgrep/index.ts index 59cdb03cf9..99d91faad3 100644 --- a/src/services/ripgrep/index.ts +++ b/src/services/ripgrep/index.ts @@ -84,6 +84,26 @@ const MAX_LINE_LENGTH = 500 export function truncateLine(line: string, maxLength: number = MAX_LINE_LENGTH): string { return line.length > maxLength ? line.substring(0, maxLength) + " [truncated...]" : line } +/** + * Returns the ordered list of absolute candidate paths where ripgrep may + * live under the given VS Code appRoot. Used by both getBinPath (first-match + * resolution) and the diagnostic command (existence report for all paths). + */ +export function ripgrepCandidatePaths(vscodeAppRoot: string): readonly string[] { + return [ + path.join(vscodeAppRoot, "node_modules/@vscode/ripgrep/bin/", binName), + path.join(vscodeAppRoot, "node_modules/vscode-ripgrep/bin", binName), + path.join(vscodeAppRoot, "node_modules.asar.unpacked/vscode-ripgrep/bin/", binName), + path.join(vscodeAppRoot, "node_modules.asar.unpacked/@vscode/ripgrep/bin/", binName), + path.join(vscodeAppRoot, `node_modules/@vscode/ripgrep-universal/${ripgrepUniversalBinDir}`, binName), + path.join( + vscodeAppRoot, + `node_modules.asar.unpacked/@vscode/ripgrep-universal/${ripgrepUniversalBinDir}`, + binName, + ), + ] +} + /** * Get the path to the ripgrep binary shipped inside the VS Code installation. * @@ -94,19 +114,10 @@ export function truncateLine(line: string, maxLength: number = MAX_LINE_LENGTH): * Returns `undefined` when ripgrep cannot be located. */ export async function getBinPath(vscodeAppRoot: string): Promise { - const checkPath = async (pkgFolder: string) => { - const fullPath = path.join(vscodeAppRoot, pkgFolder, binName) - return (await fileExistsAtPath(fullPath)) ? fullPath : undefined + for (const candidate of ripgrepCandidatePaths(vscodeAppRoot)) { + if (await fileExistsAtPath(candidate)) return candidate } - - return ( - (await checkPath("node_modules/@vscode/ripgrep/bin/")) || - (await checkPath("node_modules/vscode-ripgrep/bin")) || - (await checkPath("node_modules.asar.unpacked/vscode-ripgrep/bin/")) || - (await checkPath("node_modules.asar.unpacked/@vscode/ripgrep/bin/")) || - (await checkPath(`node_modules/@vscode/ripgrep-universal/${ripgrepUniversalBinDir}`)) || - (await checkPath(`node_modules.asar.unpacked/@vscode/ripgrep-universal/${ripgrepUniversalBinDir}`)) - ) + return undefined } async function execRipgrep(bin: string, args: string[]): Promise { diff --git a/src/services/ripgrep/internal/loadRipgrep.ts b/src/services/ripgrep/internal/loadRipgrep.ts new file mode 100644 index 0000000000..b1e0d6fcc1 --- /dev/null +++ b/src/services/ripgrep/internal/loadRipgrep.ts @@ -0,0 +1,21 @@ +/** + * Loads `@vscode/ripgrep` via CommonJS `require()`. Lives in its own + * module so unit tests can `vi.mock` the wrapper — vitest's mock registry + * hooks the import graph, not Node's native CJS resolver, and + * `@vscode/ripgrep` resolves through the latter at test time because it's + * a real devDep installed in `node_modules`. + * + * On require failure returns `{ loadError }` so the diagnostic can surface + * the actual error message instead of dropping it. + */ +export type LoadRipgrepResult = { rgPath?: string; loadError?: string } + +export function loadRipgrep(): LoadRipgrepResult | undefined { + try { + return require("@vscode/ripgrep") as LoadRipgrepResult + } catch (error) { + return { + loadError: error instanceof Error ? error.message : String(error), + } + } +} From f2fa6e16cbe3b1a022315cc8189b0bf92eb84133 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Dre=C5=BCewski?= Date: Wed, 17 Jun 2026 22:16:10 +0200 Subject: [PATCH 3/7] fix(list-files): check cwd exists before invoking ripgrep (zoo #558) Port of Zoo-Code PR #558. Guards listFiles against a non-existent cwd so ripgrep isn't spawned against a missing directory. Co-authored-by: edelauna <54631123+edelauna@users.noreply.github.com> --- ...2026-06-17_zoo-558-list-files-check-cwd.md | 220 ++++++++++++++++++ .../glob/__tests__/list-files-limit.spec.ts | 4 + .../glob/__tests__/list-files.spec.ts | 37 +++ src/services/glob/list-files.ts | 5 + 4 files changed, 266 insertions(+) create mode 100644 ai_plans/2026-06-17_zoo-558-list-files-check-cwd.md diff --git a/ai_plans/2026-06-17_zoo-558-list-files-check-cwd.md b/ai_plans/2026-06-17_zoo-558-list-files-check-cwd.md new file mode 100644 index 0000000000..8dc376793e --- /dev/null +++ b/ai_plans/2026-06-17_zoo-558-list-files-check-cwd.md @@ -0,0 +1,220 @@ +# Port plan — Zoo PR #558 → `feature/zoo-558-list-files-check-cwd` + +> **For the executor (read first).** Do the steps **in order**. Do **not** +> improvise, refactor beyond what is written, or add anything not listed +> (YAGNI). Every code block below is **already adapted to this repo** — paste it +> as-is unless a step says otherwise. If any precondition is false or a step +> doesn't behave as described, **STOP and report** — do not guess. This repo is +> **Tumble Code**: never introduce the strings "Roo" or "Zoo" in user-facing +> text. Placeholders are written as `{{like this}}` — replace every one. + +--- + +## 0. Context (read once, write no code) + +- **Upstream:** Zoo PR #558 — "fix(list-files): checking cwd before invoking rg" (commit `991130a5c`). +- **What it does, one paragraph:** Before invoking the ripgrep binary, `listFiles()` now checks whether the target directory actually exists on disk. If it does not, a clear error is thrown (`"Cannot list files: directory does not exist: "`) instead of allowing the downstream `child_process.spawn(rg, ...)` call to fail with a confusing ENOENT that names the ripgrep executable rather than the missing directory. +- **Why we want it, with evidence in OUR code:** In our `src/services/glob/list-files.ts`, the `listFiles` function (line 33) proceeds directly to `getRipgrepPath()` (line 47) and then `listFilesWithRipgrep()` (lines 51, 60) which spawns `child_process.spawn(rgPath, ...)` at line 654. There is **no guard** between the early-return at line 36 and the special-directory handler at line 40. If a nonexistent path is passed, ripgrep is spawned against a missing directory and fails with an ENOENT error message that blames the ripgrep binary, not the absent directory. This is exactly the bug PR #558 fixes. +- **What we deliberately leave out (YAGNI):** None — this is a single-guard fix with accompanying tests. +- **Original author(s) — credit them.** edelauna. When you create the port commit (only if asked), include this trailer at the end of the commit message: + + ```text + Co-authored-by: edelauna <54631123+edelauna@users.noreply.github.com> + ``` + +## 1. Preconditions — verify before touching anything + +- [ ] Current branch is `feature/zoo-558-list-files-check-cwd`, created off `main`. +- [ ] These files exist (the edits below depend on them): + - `src/services/glob/list-files.ts` + - `src/services/glob/__tests__/list-files.spec.ts` + - `src/services/glob/__tests__/list-files-limit.spec.ts` + - `src/services/roo-config/index.ts` (provides `directoryExists`) +- [ ] The code we will change still looks like this (quote it; if it differs, + STOP — the plan is stale): + +```ts +// src/services/glob/list-files.ts lines 33-48 +export async function listFiles(dirPath: string, recursive: boolean, limit: number): Promise<[string[], boolean]> { + // Early return for limit of 0 - no need to scan anything + if (limit === 0) { + return [[], false] + } + + // Handle special directories + const specialResult = await handleSpecialDirectories(dirPath) + + if (specialResult) { + return specialResult + } + + // Get ripgrep path + const rgPath = await getRipgrepPath() +``` + +## 2. Write the failing test FIRST (TDD) + +- **File:** `src/services/glob/__tests__/list-files.spec.ts` +- Add exactly this test (appended after the last existing describe block, before the final `}`): + +```ts +describe("listFiles nonexistent directory", () => { + beforeEach(() => { + vi.clearAllMocks() + resetFsPromiseMocks() + }) + + it("should throw a clear error instead of a misleading ENOENT naming the executable", async () => { + vi.mocked(directoryExists).mockResolvedValue(false) + + await expect(listFiles("/nonexistent/path", true, 100)).rejects.toThrow( + "Cannot list files: directory does not exist:", + ) + + // spawn should never be called when the directory doesn't exist + expect(vi.mocked(childProcess.spawn)).not.toHaveBeenCalled() + }) + + it("should report the missing directory even when ripgrep binary is also unavailable", async () => { + vi.mocked(directoryExists).mockResolvedValue(false) + const { getBinPath } = await import("../../ripgrep") + vi.mocked(getBinPath).mockRejectedValue(new Error("Could not find ripgrep binary")) + + await expect(listFiles("/nonexistent/path", true, 100)).rejects.toThrow( + "Cannot list files: directory does not exist:", + ) + }) +}) +``` + +- Also add the `directoryExists` mock at the top of the file (after the existing `vi.mock("fs")` block), and import it: + +Add near the top (after the imports, before the first `vi.mock`): + +```ts +import { directoryExists } from "../../../services/roo-config" +``` + +Add this mock (after `vi.mock("fs")`): + +```ts +vi.mock("../../../services/roo-config", () => ({ + directoryExists: vi.fn().mockResolvedValue(true), +})) +``` + +Add `stat` mock to the `fs.promises` mock object (inside the `vi.mock("fs", () => ({...}))`): + +```ts + stat: vi.fn().mockResolvedValue({ isDirectory: () => true }), +``` + +Add resets to `resetFsPromiseMocks()`: + +```ts +vi.mocked(fs.promises.stat).mockReset() +vi.mocked(fs.promises.stat).mockResolvedValue({ isDirectory: () => true } as any) +vi.mocked(directoryExists).mockReset() +vi.mocked(directoryExists).mockResolvedValue(true) +``` + +- **Also update `src/services/glob/__tests__/list-files-limit.spec.ts`** — add the same `roo-config` mock: + +```ts +vi.mock("../../../services/roo-config", () => ({ + directoryExists: vi.fn().mockResolvedValue(true), +})) +``` + +Add it after the `vi.mock("../../../utils/path", ...)` block (before the `import * as childProcess` line). + +- **Run:** `cd src && npx vitest run services/glob/__tests__/list-files.spec.ts` +- **Expect it to FAIL** with: `TypeError: directoryExists is not a function` or a similar import/resolve error (because the function doesn't import `directoryExists` yet), OR the test assertion failure if it does import it but doesn't call it. +- If it **passes already**, STOP — the behavior is likely present; report back. + +## 3. Implement — minimal change to make the test pass + +Make only these edits. Each is explicit; do not touch anything else. + +### Edit 1 — `src/services/glob/list-files.ts` + +Add import (after the `getBinPath` import, line 8): + +Replace: + +```ts +import { getBinPath } from "../../services/ripgrep" +import { DIRS_TO_IGNORE } from "./constants" +``` + +With: + +```ts +import { getBinPath } from "../../services/ripgrep" +import { directoryExists } from "../../services/roo-config" +import { DIRS_TO_IGNORE } from "./constants" +``` + +### Edit 2 — `src/services/glob/list-files.ts` + +Add the cwd-existence guard (after the early return for limit === 0, before the special directories handler): + +Replace: + +```ts +// Early return for limit of 0 - no need to scan anything +if (limit === 0) { + return [[], false] +} + +// Handle special directories +``` + +With: + +```ts +// Early return for limit of 0 - no need to scan anything +if (limit === 0) { + return [[], false] +} + +if (!(await directoryExists(path.resolve(dirPath)))) { + throw new Error(`Cannot list files: directory does not exist: ${path.resolve(dirPath)}`) +} + +// Handle special directories +``` + +## 4. Out of scope — do NOT do these + +- Do not refactor `listFiles` beyond adding the single guard. +- Do **not** re-add or re-wire: **TTS**, the **router / cloud provider**, **cloud upsell** UI, or **Roo/Zoo branding** — all removed from this fork on purpose. +- Do **not** rename internal ids (those stay `Roo-Code`); only user-facing strings are "Tumble". + +## 5. Verify — paste real output, don't claim success without it + +- `cd src && npx vitest run services/glob/__tests__/list-files.spec.ts services/glob/__tests__/list-files-limit.spec.ts` → all green. +- `cd src && pnpm check-types` → must be clean. + +## 6. Acceptance criteria (binary — all must hold) + +- [ ] The §2 test passes; the surrounding suite is green. +- [ ] Only the files listed in §3 changed (`git status` confirms). +- [ ] No new "Roo" or "Zoo" user-facing strings introduced. +- [ ] No removed feature (TTS / router / cloud) was reintroduced. + +## 7. Record in the ledger + +The SKILL's `port` stage (step 3) already records this PR as `ported` once the +plan file exists. If — and only if — that has not been done yet, run it now +(re-running is a harmless idempotent upsert, so when in doubt run it once): + +```bash +node .claude/skills/zoo-port/scripts/zoo-prs.mjs record \ + --pr 558 --status ported \ + --branch feature/zoo-558-list-files-check-cwd \ + --plan ai_plans/2026-06-17_zoo-558-list-files-check-cwd.md +``` + +When you commit (only if asked), append the `Co-authored-by:` trailer(s) from §0 +to the commit message. Then summarize what landed and let the user review. diff --git a/src/services/glob/__tests__/list-files-limit.spec.ts b/src/services/glob/__tests__/list-files-limit.spec.ts index 8eaf73c188..6f0ad85184 100644 --- a/src/services/glob/__tests__/list-files-limit.spec.ts +++ b/src/services/glob/__tests__/list-files-limit.spec.ts @@ -23,6 +23,10 @@ vi.mock("../../../utils/path", () => ({ arePathsEqual: vi.fn().mockReturnValue(false), })) +vi.mock("../../../services/roo-config", () => ({ + directoryExists: vi.fn().mockResolvedValue(true), +})) + import * as childProcess from "child_process" const createMockRipgrepProcess = (chunks: string[] = [], exitCode = 0) => ({ diff --git a/src/services/glob/__tests__/list-files.spec.ts b/src/services/glob/__tests__/list-files.spec.ts index 3207f692b2..71a35ca412 100644 --- a/src/services/glob/__tests__/list-files.spec.ts +++ b/src/services/glob/__tests__/list-files.spec.ts @@ -1,9 +1,13 @@ import * as path from "path" import * as childProcess from "child_process" import { listFiles } from "../list-files" +import { directoryExists } from "../../../services/roo-config" vi.mock("child_process") vi.mock("fs") +vi.mock("../../../services/roo-config", () => ({ + directoryExists: vi.fn().mockResolvedValue(true), +})) vi.mock("vscode", () => ({ env: { appRoot: "/mock/vscode/app/root", @@ -48,6 +52,7 @@ vi.mock("fs", () => ({ access: vi.fn().mockRejectedValue(new Error("Not found")), readFile: vi.fn().mockResolvedValue(""), readdir: vi.fn().mockResolvedValue([]), + stat: vi.fn().mockResolvedValue({ isDirectory: () => true }), }, })) @@ -93,6 +98,10 @@ function resetFsPromiseMocks() { vi.mocked(fs.promises.readFile).mockResolvedValue("") vi.mocked(fs.promises.readdir).mockReset() vi.mocked(fs.promises.readdir).mockResolvedValue([]) + vi.mocked(fs.promises.stat).mockReset() + vi.mocked(fs.promises.stat).mockResolvedValue({ isDirectory: () => true } as any) + vi.mocked(directoryExists).mockReset() + vi.mocked(directoryExists).mockResolvedValue(true) } describe("list-files symlink support", () => { @@ -394,3 +403,31 @@ describe("buildRecursiveArgs edge cases", () => { expect(args).not.toContain("--no-ignore") }) }) + +describe("listFiles nonexistent directory", () => { + beforeEach(() => { + vi.clearAllMocks() + resetFsPromiseMocks() + }) + + it("should throw a clear error instead of a misleading ENOENT naming the executable", async () => { + vi.mocked(directoryExists).mockResolvedValue(false) + + await expect(listFiles("/nonexistent/path", true, 100)).rejects.toThrow( + "Cannot list files: directory does not exist:", + ) + + // spawn should never be called when the directory doesn't exist + expect(vi.mocked(childProcess.spawn)).not.toHaveBeenCalled() + }) + + it("should report the missing directory even when ripgrep binary is also unavailable", async () => { + vi.mocked(directoryExists).mockResolvedValue(false) + const { getBinPath } = await import("../../ripgrep") + vi.mocked(getBinPath).mockRejectedValue(new Error("Could not find ripgrep binary")) + + await expect(listFiles("/nonexistent/path", true, 100)).rejects.toThrow( + "Cannot list files: directory does not exist:", + ) + }) +}) diff --git a/src/services/glob/list-files.ts b/src/services/glob/list-files.ts index 5366bbb84b..6374c7c48c 100644 --- a/src/services/glob/list-files.ts +++ b/src/services/glob/list-files.ts @@ -6,6 +6,7 @@ import * as vscode from "vscode" import ignore from "ignore" import { arePathsEqual } from "../../utils/path" import { getBinPath } from "../../services/ripgrep" +import { directoryExists } from "../../services/roo-config" import { DIRS_TO_IGNORE } from "./constants" /** @@ -36,6 +37,10 @@ export async function listFiles(dirPath: string, recursive: boolean, limit: numb return [[], false] } + if (!(await directoryExists(path.resolve(dirPath)))) { + throw new Error(`Cannot list files: directory does not exist: ${path.resolve(dirPath)}`) + } + // Handle special directories const specialResult = await handleSpecialDirectories(dirPath) From a84ac2b096d181843f6f02f5bda141fcbc696ade Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Dre=C5=BCewski?= Date: Wed, 17 Jun 2026 22:25:05 +0200 Subject: [PATCH 4/7] feat(litellm): handle reasoning_content and reasoning fields in streaming (zoo #449) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit LiteLLMHandler.createMessage never read the reasoning_content / reasoning fields from the stream delta, so reasoning output from DeepSeek-R1, QwQ, and other reasoning models routed through LiteLLM was silently dropped. Extract the reasoning-field logic into a shared extractReasoningFromDelta helper, wire it into lite-llm.ts, and replace the inline for-of/break block in base-openai-compatible-provider.ts with it. The helper also fixes two latent bugs in the old inline logic: - the for…break short-circuited as soon as the reasoning_content key existed, so a delta with reasoning_content: null + a populated reasoning field dropped the thinking output instead of falling back; - the .trim() guard discarded whitespace-only chunks (a lone " " or "\n\n"), collapsing word/paragraph boundaries once chunks are concatenated downstream. The helper picks the first field that is a non-empty string (length-based). Co-authored-by: Oh Daewoong --- ...-17_zoo-449-litellm-reasoning-streaming.md | 579 ++++++++++++++++++ .../base-openai-compatible-provider.spec.ts | 12 +- src/api/providers/__tests__/lite-llm.spec.ts | 195 ++++++ .../base-openai-compatible-provider.ts | 14 +- src/api/providers/lite-llm.ts | 6 + .../utils/__tests__/extract-reasoning.spec.ts | 63 ++ src/api/providers/utils/extract-reasoning.ts | 25 + 7 files changed, 880 insertions(+), 14 deletions(-) create mode 100644 ai_plans/2026-06-17_zoo-449-litellm-reasoning-streaming.md create mode 100644 src/api/providers/utils/__tests__/extract-reasoning.spec.ts create mode 100644 src/api/providers/utils/extract-reasoning.ts diff --git a/ai_plans/2026-06-17_zoo-449-litellm-reasoning-streaming.md b/ai_plans/2026-06-17_zoo-449-litellm-reasoning-streaming.md new file mode 100644 index 0000000000..ec3889889d --- /dev/null +++ b/ai_plans/2026-06-17_zoo-449-litellm-reasoning-streaming.md @@ -0,0 +1,579 @@ +# Port plan — Zoo PR #449 → `feature/zoo-449-litellm-reasoning-streaming` + +> **For the executor (read first).** Do the steps **in order**. Do **not** +> improvise, refactor beyond what is written, or add anything not listed +> (YAGNI). Every code block below is **already adapted to this repo** — paste it +> as-is unless a step says otherwise. If any precondition is false or a step +> doesn't behave as described, **STOP and report** — do not guess. This repo is +> **Tumble Code**: never introduce the strings "Roo" or "Zoo" in user-facing +> text. + +--- + +## 0. Context (read once, write no code) + +- **Upstream:** Zoo PR #449 — "feat(litellm): handle reasoning_content and reasoning fields in streaming" (commit `c4531d45b`). +- **What it does, one paragraph:** `LiteLLMHandler.createMessage` never reads the + `reasoning_content` / `reasoning` fields from the streaming delta, so reasoning + output from DeepSeek-R1, QwQ, and other reasoning models routed through LiteLLM + is silently dropped. This PR (a) extracts the reasoning-field logic into a + shared `extractReasoningFromDelta` helper, (b) wires it into `lite-llm.ts`, and + (c) replaces the existing inline `for-of/break` block in + `base-openai-compatible-provider.ts` with the same helper. The helper also fixes + two bugs in the old inline logic: (1) the old `for…break` short-circuited the + moment the `reasoning_content` KEY existed, so a delta with + `reasoning_content: null` + a populated `reasoning` field dropped the thinking + output instead of falling back; (2) the old `.trim()` guard discarded + whitespace-only chunks (a lone `" "` or `"\n\n"`), collapsing word/paragraph + boundaries once chunks are concatenated downstream. The helper picks the first + field that is a **non-empty string** (length-based, not trim-based). +- **Why we want it, with evidence in OUR code:** [lite-llm.ts:230-242](src/api/providers/lite-llm.ts#L230-L242) + — our `createMessage` loop yields `delta.content` and `delta.tool_calls` but has + no branch for `reasoning_content` / `reasoning`, so reasoning is dropped today. + And [base-openai-compatible-provider.ts:151-161](src/api/providers/base-openai-compatible-provider.ts#L151-L161) + still has the exact old `for (const key of ["reasoning_content","reasoning"])` + block with both bugs. +- **What we deliberately leave out (YAGNI):** nothing — all 6 files port directly; + our before-state matches upstream's before-state exactly. +- **Original author(s) — credit them.** Oh Daewoong (commit author `daewoong` = + GitHub `daewoongoh`, same person). When you create the port commit, include this + trailer at the end of the commit message (drop the `Claude Opus 4.7` AI-assistant + trailer per fork policy): + + ```text + Co-authored-by: Oh Daewoong + ``` + +## 1. Preconditions — verify before touching anything + +- [ ] Current branch is `feature/zoo-449-litellm-reasoning-streaming`, created off `main`. +- [ ] These files exist: + - `src/api/providers/lite-llm.ts` + - `src/api/providers/base-openai-compatible-provider.ts` + - `src/api/providers/__tests__/lite-llm.spec.ts` + - `src/api/providers/__tests__/base-openai-compatible-provider.spec.ts` +- [ ] These files do NOT yet exist (you will create them): + - `src/api/providers/utils/extract-reasoning.ts` + - `src/api/providers/utils/__tests__/extract-reasoning.spec.ts` +- [ ] In `base-openai-compatible-provider.ts` the reasoning block still looks + EXACTLY like this (if it differs, STOP — the plan is stale): + +```ts + if (delta) { + for (const key of ["reasoning_content", "reasoning"] as const) { + if (key in delta) { + const reasoning_content = ((delta as any)[key] as string | undefined) || "" + if (reasoning_content?.trim()) { + yield { type: "reasoning", text: reasoning_content } + } + break + } + } + } +``` + +## 2. Write the failing test FIRST (TDD) + +- **File:** `src/api/providers/utils/__tests__/extract-reasoning.spec.ts` (create it). +- Add exactly this test: + +```ts +// npx vitest run api/providers/utils/__tests__/extract-reasoning.spec.ts + +import { extractReasoningFromDelta } from "../extract-reasoning" + +describe("extractReasoningFromDelta", () => { + it("returns reasoning_content when present and non-blank", () => { + expect(extractReasoningFromDelta({ reasoning_content: "thinking..." })).toBe("thinking...") + }) + + it("returns reasoning when reasoning_content is missing", () => { + expect(extractReasoningFromDelta({ reasoning: "analyzing" })).toBe("analyzing") + }) + + it("prefers reasoning_content over reasoning when both are non-blank", () => { + expect( + extractReasoningFromDelta({ + reasoning_content: "from_content", + reasoning: "from_reasoning", + }), + ).toBe("from_content") + }) + + it("falls back to reasoning when reasoning_content is null on the same delta", () => { + expect( + extractReasoningFromDelta({ + reasoning_content: null, + reasoning: "fallback", + }), + ).toBe("fallback") + }) + + it("falls back to reasoning when reasoning_content is empty string", () => { + expect( + extractReasoningFromDelta({ + reasoning_content: "", + reasoning: "fallback", + }), + ).toBe("fallback") + }) + + it("preserves whitespace-only payloads so streamed chunks keep word and paragraph boundaries", () => { + expect(extractReasoningFromDelta({ reasoning_content: " " })).toBe(" ") + expect(extractReasoningFromDelta({ reasoning: "\n\n" })).toBe("\n\n") + }) + + it("falls back to reasoning when reasoning_content is an empty string but does not skip whitespace", () => { + expect( + extractReasoningFromDelta({ + reasoning_content: "", + reasoning: "\n\n", + }), + ).toBe("\n\n") + }) + + it("returns undefined when neither field is present", () => { + expect(extractReasoningFromDelta({ content: "hi" })).toBeUndefined() + }) + + it("returns undefined for nullish input", () => { + expect(extractReasoningFromDelta(null)).toBeUndefined() + expect(extractReasoningFromDelta(undefined)).toBeUndefined() + }) +}) +``` + +- **Run:** `cd src && npx vitest run api/providers/utils/__tests__/extract-reasoning.spec.ts` +- **Expect it to FAIL** with a module-not-found / import error (the helper file + `../extract-reasoning` does not exist yet). + +## 3. Implement — minimal change to make the test pass + +Make only these edits. Each is explicit; do not touch anything else. + +### Edit 1 — CREATE `src/api/providers/utils/extract-reasoning.ts` + +Full contents of the new file: + +```ts +/** + * Extracts reasoning text from a streaming delta object. + * + * Prefers `reasoning_content` (DeepSeek-R1 / QwQ style) and falls back to + * `reasoning` (OpenRouter style). Whitespace-only payloads (e.g. a lone " " + * or "\n\n" between paragraphs) are preserved so streamed reasoning keeps + * word and paragraph boundaries once chunks are concatenated downstream. + * + * The fallback only fires when the current field is missing, non-string, + * or an empty string — a delta with `reasoning_content: null` and a + * populated `reasoning` still resolves to the populated field. + */ +export function extractReasoningFromDelta(delta: unknown): string | undefined { + if (!delta) return undefined + + const d = delta as { reasoning_content?: unknown; reasoning?: unknown } + + if (typeof d.reasoning_content === "string" && d.reasoning_content.length > 0) { + return d.reasoning_content + } + if (typeof d.reasoning === "string" && d.reasoning.length > 0) { + return d.reasoning + } + return undefined +} +``` + +Now re-run the §2 test — it must PASS (9 tests green). + +### Edit 2 — `src/api/providers/lite-llm.ts` + +After the import of `RouterProvider`, add the helper import. Replace: + +```ts +import type { SingleCompletionHandler, ApiHandlerCreateMessageMetadata } from "../index" +import { RouterProvider } from "./router-provider" +``` + +With: + +```ts +import type { SingleCompletionHandler, ApiHandlerCreateMessageMetadata } from "../index" +import { RouterProvider } from "./router-provider" +import { extractReasoningFromDelta } from "./utils/extract-reasoning" +``` + +Then, inside the `for await (const chunk of completion)` streaming loop, add a +reasoning branch right after the `delta?.content` block. Replace: + +```ts + if (delta?.content) { + yield { type: "text", text: delta.content } + } + + // Handle tool calls in stream - emit partial chunks for NativeToolCallParser + if (delta?.tool_calls) { +``` + +With: + +```ts + if (delta?.content) { + yield { type: "text", text: delta.content } + } + + const reasoningText = extractReasoningFromDelta(delta) + if (reasoningText) { + yield { type: "reasoning", text: reasoningText } + } + + // Handle tool calls in stream - emit partial chunks for NativeToolCallParser + if (delta?.tool_calls) { +``` + +### Edit 3 — `src/api/providers/base-openai-compatible-provider.ts` + +Add the helper import. Replace: + +```ts +import { getApiRequestTimeout } from "./utils/timeout-config" +``` + +With: + +```ts +import { getApiRequestTimeout } from "./utils/timeout-config" +import { extractReasoningFromDelta } from "./utils/extract-reasoning" +``` + +Then replace the inline reasoning block (quoted in §1) entirely. Replace: + +```ts + if (delta) { + for (const key of ["reasoning_content", "reasoning"] as const) { + if (key in delta) { + const reasoning_content = ((delta as any)[key] as string | undefined) || "" + if (reasoning_content?.trim()) { + yield { type: "reasoning", text: reasoning_content } + } + break + } + } + } +``` + +With: + +```ts + const reasoningText = extractReasoningFromDelta(delta) + if (reasoningText) { + yield { type: "reasoning", text: reasoningText } + } +``` + +### Edit 4 — `src/api/providers/__tests__/lite-llm.spec.ts` + +Insert a new `describe("reasoning field handling", …)` block **immediately +before** the existing `describe("tool ID normalization", …)` block (it currently +starts at roughly line 722). Find this line: + +```ts + describe("tool ID normalization", () => { +``` + +Insert the following block right before it (keep the existing `tool ID +normalization` describe intact after it): + +```ts +describe("reasoning field handling", () => { + it("should yield reasoning chunks from reasoning_content delta", async () => { + const mockStream = { + async *[Symbol.asyncIterator]() { + yield { + choices: [{ delta: { reasoning_content: "Let me think..." } }], + usage: null, + } + yield { + choices: [{ delta: { content: "The answer is 42." } }], + usage: { prompt_tokens: 20, completion_tokens: 10 }, + } + }, + } + + mockCreate.mockReturnValue({ + withResponse: vi.fn().mockResolvedValue({ data: mockStream }), + }) + + const generator = handler.createMessage("system", [{ role: "user", content: "What is the answer?" }]) + const results = [] + for await (const chunk of generator) { + results.push(chunk) + } + + const reasoningChunk = results.find((c) => c.type === "reasoning") + expect(reasoningChunk).toBeDefined() + expect(reasoningChunk).toMatchObject({ type: "reasoning", text: "Let me think..." }) + + const textChunk = results.find((c) => c.type === "text") + expect(textChunk).toMatchObject({ type: "text", text: "The answer is 42." }) + }) + + it("should yield reasoning chunks from reasoning delta field", async () => { + const mockStream = { + async *[Symbol.asyncIterator]() { + yield { + choices: [{ delta: { reasoning: "Analyzing the problem..." } }], + usage: null, + } + yield { + choices: [{ delta: { content: "Done." } }], + usage: { prompt_tokens: 10, completion_tokens: 5 }, + } + }, + } + + mockCreate.mockReturnValue({ + withResponse: vi.fn().mockResolvedValue({ data: mockStream }), + }) + + const generator = handler.createMessage("system", [{ role: "user", content: "Solve this." }]) + const results = [] + for await (const chunk of generator) { + results.push(chunk) + } + + const reasoningChunk = results.find((c) => c.type === "reasoning") + expect(reasoningChunk).toBeDefined() + expect(reasoningChunk).toMatchObject({ type: "reasoning", text: "Analyzing the problem..." }) + }) + + it("should prefer reasoning_content over reasoning when both are present", async () => { + const mockStream = { + async *[Symbol.asyncIterator]() { + yield { + choices: [{ delta: { reasoning_content: "from_reasoning_content", reasoning: "from_reasoning" } }], + usage: { prompt_tokens: 5, completion_tokens: 5 }, + } + }, + } + + mockCreate.mockReturnValue({ + withResponse: vi.fn().mockResolvedValue({ data: mockStream }), + }) + + const generator = handler.createMessage("system", [{ role: "user", content: "Test." }]) + const results = [] + for await (const chunk of generator) { + results.push(chunk) + } + + const reasoningChunks = results.filter((c) => c.type === "reasoning") + expect(reasoningChunks).toHaveLength(1) + expect(reasoningChunks[0]).toMatchObject({ type: "reasoning", text: "from_reasoning_content" }) + }) + + it("should not yield reasoning chunk when reasoning field is present but falsy", async () => { + const mockStream = { + async *[Symbol.asyncIterator]() { + yield { + choices: [{ delta: { reasoning_content: undefined } }], + usage: null, + } + yield { + choices: [{ delta: { reasoning: "" } }], + usage: null, + } + yield { + choices: [{ delta: { content: "Hello" } }], + usage: { prompt_tokens: 5, completion_tokens: 5 }, + } + }, + } + + mockCreate.mockReturnValue({ + withResponse: vi.fn().mockResolvedValue({ data: mockStream }), + }) + + const generator = handler.createMessage("system", [{ role: "user", content: "Hi" }]) + const results = [] + for await (const chunk of generator) { + results.push(chunk) + } + + const reasoningChunks = results.filter((c) => c.type === "reasoning") + expect(reasoningChunks).toHaveLength(0) + }) + + it("should preserve whitespace-only reasoning chunks so streamed boundaries survive concatenation", async () => { + const mockStream = { + async *[Symbol.asyncIterator]() { + yield { + choices: [{ delta: { reasoning_content: "Let's" } }], + usage: null, + } + yield { + choices: [{ delta: { reasoning_content: " " } }], + usage: null, + } + yield { + choices: [{ delta: { reasoning_content: "think" } }], + usage: null, + } + yield { + choices: [{ delta: { reasoning_content: "\n\n" } }], + usage: null, + } + yield { + choices: [{ delta: { reasoning_content: "next" } }], + usage: null, + } + yield { + choices: [{ delta: { content: "Hello" } }], + usage: { prompt_tokens: 5, completion_tokens: 5 }, + } + }, + } + + mockCreate.mockReturnValue({ + withResponse: vi.fn().mockResolvedValue({ data: mockStream }), + }) + + const generator = handler.createMessage("system", [{ role: "user", content: "Hi" }]) + const results = [] + for await (const chunk of generator) { + results.push(chunk) + } + + const reasoningChunks = results.filter((c) => c.type === "reasoning") + expect(reasoningChunks.map((c) => (c as { text: string }).text).join("")).toBe("Let's think\n\nnext") + }) + + it("should fall back to reasoning when reasoning_content is null on the same delta", async () => { + const mockStream = { + async *[Symbol.asyncIterator]() { + yield { + choices: [{ delta: { reasoning_content: null, reasoning: "fallback thinking" } }], + usage: null, + } + yield { + choices: [{ delta: { content: "Answer." } }], + usage: { prompt_tokens: 5, completion_tokens: 5 }, + } + }, + } + + mockCreate.mockReturnValue({ + withResponse: vi.fn().mockResolvedValue({ data: mockStream }), + }) + + const generator = handler.createMessage("system", [{ role: "user", content: "Test." }]) + const results = [] + for await (const chunk of generator) { + results.push(chunk) + } + + const reasoningChunks = results.filter((c) => c.type === "reasoning") + expect(reasoningChunks).toHaveLength(1) + expect(reasoningChunks[0]).toMatchObject({ type: "reasoning", text: "fallback thinking" }) + }) +}) +``` + +### Edit 5 — `src/api/providers/__tests__/base-openai-compatible-provider.spec.ts` + +Two existing tests change because whitespace-only reasoning is now preserved. + +**5a.** Replace this test title line: + +```ts + it("should filter out whitespace-only reasoning_content", async () => { +``` + +With: + +```ts + it("should preserve whitespace-only reasoning_content so streamed boundaries survive concatenation", async () => { +``` + +**5b.** In that same test, replace the assertion block: + +```ts +// Should only have the regular content, not the whitespace-only reasoning +expect(chunks).toEqual([{ type: "text", text: "Regular content" }]) +``` + +With: + +```ts +expect(chunks).toEqual([ + { type: "reasoning", text: "\n" }, + { type: "reasoning", text: " " }, + { type: "reasoning", text: "\t\n " }, + { type: "text", text: "Regular content" }, +]) +``` + +**5c.** In the `"should yield non-empty reasoning_content"` test, replace the +assertion block: + +```ts +// Should only yield the non-empty reasoning content +expect(chunks).toEqual([ + { type: "reasoning", text: "Thinking step 1" }, + { type: "reasoning", text: "Thinking step 2" }, +]) +``` + +With: + +```ts +expect(chunks).toEqual([ + { type: "reasoning", text: "Thinking step 1" }, + { type: "reasoning", text: "\n" }, + { type: "reasoning", text: "Thinking step 2" }, +]) +``` + +> Note: there is a third test in that describe block, `"should handle +reasoning_content with leading/trailing whitespace"` — leave it untouched; with +> the new helper a `" content with spaces "` payload (length > 0) still flows +> through verbatim, which is what it already asserts. + +## 4. Out of scope — do NOT do these + +- Do not change any other provider, or the `TagMatcher`/`think`-tag handling. +- Do **not** re-add or re-wire: **TTS**, the **router / cloud provider**, **cloud + upsell** UI, or **Roo/Zoo branding**. +- Do **not** rename internal ids (those stay `Roo-Code`). + +## 5. Verify — paste real output, don't claim success without it + +Run from the repo root: + +- `cd src && npx vitest run api/providers/utils/__tests__/extract-reasoning.spec.ts` → 9 pass. +- `cd src && npx vitest run api/providers/__tests__/lite-llm.spec.ts` → all pass (incl. 6 new reasoning tests). +- `cd src && npx vitest run api/providers/__tests__/base-openai-compatible-provider.spec.ts` → all pass. +- `cd src && pnpm check-types` → clean. + +## 6. Acceptance criteria (binary — all must hold) + +- [ ] The §2 helper test (9 cases) passes. +- [ ] `lite-llm.spec.ts` reasoning block (6 cases) passes; rest of suite green. +- [ ] `base-openai-compatible-provider.spec.ts` updated 2 tests pass; rest green. +- [ ] Only these 6 files changed (`git status` confirms): the 2 new util files, + `lite-llm.ts`, `base-openai-compatible-provider.ts`, and the 2 spec files. +- [ ] `pnpm check-types` clean. +- [ ] No new "Roo" or "Zoo" user-facing strings; no removed feature reintroduced. + +## 7. Record in the ledger + +Already recorded by the orchestrator after the plan file is written. The commit +(done by the orchestrator) will carry: + +```text +Co-authored-by: Oh Daewoong +``` + +> **Cross-port note for later:** PR #588 ("extractReasoningFromDelta helper") in +> the same port batch concerns this very helper. Since #449 introduces it here, +> #588 may already be satisfied or only need a small follow-up — re-diff #588 +> against this branch/main when its turn comes. diff --git a/src/api/providers/__tests__/base-openai-compatible-provider.spec.ts b/src/api/providers/__tests__/base-openai-compatible-provider.spec.ts index a7b4cb3c5d..c1753bc458 100644 --- a/src/api/providers/__tests__/base-openai-compatible-provider.spec.ts +++ b/src/api/providers/__tests__/base-openai-compatible-provider.spec.ts @@ -229,7 +229,7 @@ describe("BaseOpenAiCompatibleProvider", () => { }) describe("reasoning_content field", () => { - it("should filter out whitespace-only reasoning_content", async () => { + it("should preserve whitespace-only reasoning_content so streamed boundaries survive concatenation", async () => { mockCreate.mockImplementationOnce(() => { return { [Symbol.asyncIterator]: () => ({ @@ -262,8 +262,12 @@ describe("BaseOpenAiCompatibleProvider", () => { chunks.push(chunk) } - // Should only have the regular content, not the whitespace-only reasoning - expect(chunks).toEqual([{ type: "text", text: "Regular content" }]) + expect(chunks).toEqual([ + { type: "reasoning", text: "\n" }, + { type: "reasoning", text: " " }, + { type: "reasoning", text: "\t\n " }, + { type: "text", text: "Regular content" }, + ]) }) it("should yield non-empty reasoning_content", async () => { @@ -295,9 +299,9 @@ describe("BaseOpenAiCompatibleProvider", () => { chunks.push(chunk) } - // Should only yield the non-empty reasoning content expect(chunks).toEqual([ { type: "reasoning", text: "Thinking step 1" }, + { type: "reasoning", text: "\n" }, { type: "reasoning", text: "Thinking step 2" }, ]) }) diff --git a/src/api/providers/__tests__/lite-llm.spec.ts b/src/api/providers/__tests__/lite-llm.spec.ts index 9f3a641cb3..df0e8b152d 100644 --- a/src/api/providers/__tests__/lite-llm.spec.ts +++ b/src/api/providers/__tests__/lite-llm.spec.ts @@ -719,6 +719,201 @@ describe("LiteLLMHandler", () => { }) }) + describe("reasoning field handling", () => { + it("should yield reasoning chunks from reasoning_content delta", async () => { + const mockStream = { + async *[Symbol.asyncIterator]() { + yield { + choices: [{ delta: { reasoning_content: "Let me think..." } }], + usage: null, + } + yield { + choices: [{ delta: { content: "The answer is 42." } }], + usage: { prompt_tokens: 20, completion_tokens: 10 }, + } + }, + } + + mockCreate.mockReturnValue({ + withResponse: vi.fn().mockResolvedValue({ data: mockStream }), + }) + + const generator = handler.createMessage("system", [{ role: "user", content: "What is the answer?" }]) + const results = [] + for await (const chunk of generator) { + results.push(chunk) + } + + const reasoningChunk = results.find((c) => c.type === "reasoning") + expect(reasoningChunk).toBeDefined() + expect(reasoningChunk).toMatchObject({ type: "reasoning", text: "Let me think..." }) + + const textChunk = results.find((c) => c.type === "text") + expect(textChunk).toMatchObject({ type: "text", text: "The answer is 42." }) + }) + + it("should yield reasoning chunks from reasoning delta field", async () => { + const mockStream = { + async *[Symbol.asyncIterator]() { + yield { + choices: [{ delta: { reasoning: "Analyzing the problem..." } }], + usage: null, + } + yield { + choices: [{ delta: { content: "Done." } }], + usage: { prompt_tokens: 10, completion_tokens: 5 }, + } + }, + } + + mockCreate.mockReturnValue({ + withResponse: vi.fn().mockResolvedValue({ data: mockStream }), + }) + + const generator = handler.createMessage("system", [{ role: "user", content: "Solve this." }]) + const results = [] + for await (const chunk of generator) { + results.push(chunk) + } + + const reasoningChunk = results.find((c) => c.type === "reasoning") + expect(reasoningChunk).toBeDefined() + expect(reasoningChunk).toMatchObject({ type: "reasoning", text: "Analyzing the problem..." }) + }) + + it("should prefer reasoning_content over reasoning when both are present", async () => { + const mockStream = { + async *[Symbol.asyncIterator]() { + yield { + choices: [ + { delta: { reasoning_content: "from_reasoning_content", reasoning: "from_reasoning" } }, + ], + usage: { prompt_tokens: 5, completion_tokens: 5 }, + } + }, + } + + mockCreate.mockReturnValue({ + withResponse: vi.fn().mockResolvedValue({ data: mockStream }), + }) + + const generator = handler.createMessage("system", [{ role: "user", content: "Test." }]) + const results = [] + for await (const chunk of generator) { + results.push(chunk) + } + + const reasoningChunks = results.filter((c) => c.type === "reasoning") + expect(reasoningChunks).toHaveLength(1) + expect(reasoningChunks[0]).toMatchObject({ type: "reasoning", text: "from_reasoning_content" }) + }) + + it("should not yield reasoning chunk when reasoning field is present but falsy", async () => { + const mockStream = { + async *[Symbol.asyncIterator]() { + yield { + choices: [{ delta: { reasoning_content: undefined } }], + usage: null, + } + yield { + choices: [{ delta: { reasoning: "" } }], + usage: null, + } + yield { + choices: [{ delta: { content: "Hello" } }], + usage: { prompt_tokens: 5, completion_tokens: 5 }, + } + }, + } + + mockCreate.mockReturnValue({ + withResponse: vi.fn().mockResolvedValue({ data: mockStream }), + }) + + const generator = handler.createMessage("system", [{ role: "user", content: "Hi" }]) + const results = [] + for await (const chunk of generator) { + results.push(chunk) + } + + const reasoningChunks = results.filter((c) => c.type === "reasoning") + expect(reasoningChunks).toHaveLength(0) + }) + + it("should preserve whitespace-only reasoning chunks so streamed boundaries survive concatenation", async () => { + const mockStream = { + async *[Symbol.asyncIterator]() { + yield { + choices: [{ delta: { reasoning_content: "Let's" } }], + usage: null, + } + yield { + choices: [{ delta: { reasoning_content: " " } }], + usage: null, + } + yield { + choices: [{ delta: { reasoning_content: "think" } }], + usage: null, + } + yield { + choices: [{ delta: { reasoning_content: "\n\n" } }], + usage: null, + } + yield { + choices: [{ delta: { reasoning_content: "next" } }], + usage: null, + } + yield { + choices: [{ delta: { content: "Hello" } }], + usage: { prompt_tokens: 5, completion_tokens: 5 }, + } + }, + } + + mockCreate.mockReturnValue({ + withResponse: vi.fn().mockResolvedValue({ data: mockStream }), + }) + + const generator = handler.createMessage("system", [{ role: "user", content: "Hi" }]) + const results = [] + for await (const chunk of generator) { + results.push(chunk) + } + + const reasoningChunks = results.filter((c) => c.type === "reasoning") + expect(reasoningChunks.map((c) => (c as { text: string }).text).join("")).toBe("Let's think\n\nnext") + }) + + it("should fall back to reasoning when reasoning_content is null on the same delta", async () => { + const mockStream = { + async *[Symbol.asyncIterator]() { + yield { + choices: [{ delta: { reasoning_content: null, reasoning: "fallback thinking" } }], + usage: null, + } + yield { + choices: [{ delta: { content: "Answer." } }], + usage: { prompt_tokens: 5, completion_tokens: 5 }, + } + }, + } + + mockCreate.mockReturnValue({ + withResponse: vi.fn().mockResolvedValue({ data: mockStream }), + }) + + const generator = handler.createMessage("system", [{ role: "user", content: "Test." }]) + const results = [] + for await (const chunk of generator) { + results.push(chunk) + } + + const reasoningChunks = results.filter((c) => c.type === "reasoning") + expect(reasoningChunks).toHaveLength(1) + expect(reasoningChunks[0]).toMatchObject({ type: "reasoning", text: "fallback thinking" }) + }) + }) + describe("tool ID normalization", () => { it("should truncate tool IDs longer than 64 characters", async () => { const optionsWithBedrock: ApiHandlerOptions = { diff --git a/src/api/providers/base-openai-compatible-provider.ts b/src/api/providers/base-openai-compatible-provider.ts index fc3d769ae2..9ae605f507 100644 --- a/src/api/providers/base-openai-compatible-provider.ts +++ b/src/api/providers/base-openai-compatible-provider.ts @@ -14,6 +14,7 @@ import { BaseProvider } from "./base-provider" import { handleOpenAIError } from "./utils/openai-error-handler" import { calculateApiCostOpenAI } from "../../shared/cost" import { getApiRequestTimeout } from "./utils/timeout-config" +import { extractReasoningFromDelta } from "./utils/extract-reasoning" type BaseOpenAiCompatibleProviderOptions = ApiHandlerOptions & { providerName: string @@ -147,16 +148,9 @@ export abstract class BaseOpenAiCompatibleProvider } } - if (delta) { - for (const key of ["reasoning_content", "reasoning"] as const) { - if (key in delta) { - const reasoning_content = ((delta as any)[key] as string | undefined) || "" - if (reasoning_content?.trim()) { - yield { type: "reasoning", text: reasoning_content } - } - break - } - } + const reasoningText = extractReasoningFromDelta(delta) + if (reasoningText) { + yield { type: "reasoning", text: reasoningText } } // Emit raw tool call chunks - NativeToolCallParser handles state management diff --git a/src/api/providers/lite-llm.ts b/src/api/providers/lite-llm.ts index cf8d16a112..0b79433f35 100644 --- a/src/api/providers/lite-llm.ts +++ b/src/api/providers/lite-llm.ts @@ -13,6 +13,7 @@ import { sanitizeOpenAiCallId } from "../../utils/tool-id" import type { SingleCompletionHandler, ApiHandlerCreateMessageMetadata } from "../index" import { RouterProvider } from "./router-provider" +import { extractReasoningFromDelta } from "./utils/extract-reasoning" /** * LiteLLM provider handler @@ -235,6 +236,11 @@ export class LiteLLMHandler extends RouterProvider implements SingleCompletionHa yield { type: "text", text: delta.content } } + const reasoningText = extractReasoningFromDelta(delta) + if (reasoningText) { + yield { type: "reasoning", text: reasoningText } + } + // Handle tool calls in stream - emit partial chunks for NativeToolCallParser if (delta?.tool_calls) { for (const toolCall of delta.tool_calls) { diff --git a/src/api/providers/utils/__tests__/extract-reasoning.spec.ts b/src/api/providers/utils/__tests__/extract-reasoning.spec.ts new file mode 100644 index 0000000000..71edad4dfd --- /dev/null +++ b/src/api/providers/utils/__tests__/extract-reasoning.spec.ts @@ -0,0 +1,63 @@ +// npx vitest run api/providers/utils/__tests__/extract-reasoning.spec.ts + +import { extractReasoningFromDelta } from "../extract-reasoning" + +describe("extractReasoningFromDelta", () => { + it("returns reasoning_content when present and non-blank", () => { + expect(extractReasoningFromDelta({ reasoning_content: "thinking..." })).toBe("thinking...") + }) + + it("returns reasoning when reasoning_content is missing", () => { + expect(extractReasoningFromDelta({ reasoning: "analyzing" })).toBe("analyzing") + }) + + it("prefers reasoning_content over reasoning when both are non-blank", () => { + expect( + extractReasoningFromDelta({ + reasoning_content: "from_content", + reasoning: "from_reasoning", + }), + ).toBe("from_content") + }) + + it("falls back to reasoning when reasoning_content is null on the same delta", () => { + expect( + extractReasoningFromDelta({ + reasoning_content: null, + reasoning: "fallback", + }), + ).toBe("fallback") + }) + + it("falls back to reasoning when reasoning_content is empty string", () => { + expect( + extractReasoningFromDelta({ + reasoning_content: "", + reasoning: "fallback", + }), + ).toBe("fallback") + }) + + it("preserves whitespace-only payloads so streamed chunks keep word and paragraph boundaries", () => { + expect(extractReasoningFromDelta({ reasoning_content: " " })).toBe(" ") + expect(extractReasoningFromDelta({ reasoning: "\n\n" })).toBe("\n\n") + }) + + it("falls back to reasoning when reasoning_content is an empty string but does not skip whitespace", () => { + expect( + extractReasoningFromDelta({ + reasoning_content: "", + reasoning: "\n\n", + }), + ).toBe("\n\n") + }) + + it("returns undefined when neither field is present", () => { + expect(extractReasoningFromDelta({ content: "hi" })).toBeUndefined() + }) + + it("returns undefined for nullish input", () => { + expect(extractReasoningFromDelta(null)).toBeUndefined() + expect(extractReasoningFromDelta(undefined)).toBeUndefined() + }) +}) diff --git a/src/api/providers/utils/extract-reasoning.ts b/src/api/providers/utils/extract-reasoning.ts new file mode 100644 index 0000000000..698f68c86b --- /dev/null +++ b/src/api/providers/utils/extract-reasoning.ts @@ -0,0 +1,25 @@ +/** + * Extracts reasoning text from a streaming delta object. + * + * Prefers `reasoning_content` (DeepSeek-R1 / QwQ style) and falls back to + * `reasoning` (OpenRouter style). Whitespace-only payloads (e.g. a lone " " + * or "\n\n" between paragraphs) are preserved so streamed reasoning keeps + * word and paragraph boundaries once chunks are concatenated downstream. + * + * The fallback only fires when the current field is missing, non-string, + * or an empty string — a delta with `reasoning_content: null` and a + * populated `reasoning` still resolves to the populated field. + */ +export function extractReasoningFromDelta(delta: unknown): string | undefined { + if (!delta) return undefined + + const d = delta as { reasoning_content?: unknown; reasoning?: unknown } + + if (typeof d.reasoning_content === "string" && d.reasoning_content.length > 0) { + return d.reasoning_content + } + if (typeof d.reasoning === "string" && d.reasoning.length > 0) { + return d.reasoning + } + return undefined +} From ffa34ec4f11f801c9ec6ef4ba0165dedc4d15457 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Dre=C5=BCewski?= Date: Wed, 17 Jun 2026 22:31:20 +0200 Subject: [PATCH 5/7] fix(commands): correct multi-line quoted command parsing and surface malformed commands (zoo #483) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parseCommand split on every newline before any quote handling, so newlines inside a quoted argument (sh -c '...'), a heredoc body, or an unterminated quote were treated as separate sub-commands — defeating allowlist auto-approval and producing spurious entries in the command-pattern UI. Rewrite the parser around a single scanTopLevelQuotes state machine that masks quoted regions (single / double / ANSI-C $'...' / locale $"..." / heredocs / herestrings) before splitting on genuine unquoted newlines, and detects unterminated quotes/heredocs as shell syntax errors. parseCommand now returns ParseResult { commands, parseError }; getCommandDecision returns a new malformed_command decision, ExecuteCommandTool surfaces the syntax error to the agent as a tool error, and the CommandExecution card renders an error state. The webview pattern extractor pre-splits via parseCommand so heredoc/quoted fragments no longer leak into the allow/deny selector. Co-authored-by: Andrew Schmeder <149117631+awschmeder@users.noreply.github.com> --- ...oo-483-multiline-quoted-command-parsing.md | 125 ++++ packages/types/src/terminal.ts | 5 + .../auto-approval/__tests__/commands.spec.ts | 60 ++ src/core/auto-approval/commands.ts | 18 +- src/core/tools/ExecuteCommandTool.ts | 20 + src/shared/__tests__/parse-command.spec.ts | 507 +++++++++++++++ src/shared/parse-command.ts | 578 +++++++++++++++++- .../src/components/chat/CommandExecution.tsx | 16 +- .../chat/__tests__/CommandExecution.spec.tsx | 60 ++ webview-ui/src/i18n/locales/ca/chat.json | 1 + webview-ui/src/i18n/locales/de/chat.json | 1 + webview-ui/src/i18n/locales/en/chat.json | 1 + webview-ui/src/i18n/locales/es/chat.json | 1 + webview-ui/src/i18n/locales/fr/chat.json | 1 + webview-ui/src/i18n/locales/hi/chat.json | 1 + webview-ui/src/i18n/locales/id/chat.json | 1 + webview-ui/src/i18n/locales/it/chat.json | 1 + webview-ui/src/i18n/locales/ja/chat.json | 1 + webview-ui/src/i18n/locales/ko/chat.json | 1 + webview-ui/src/i18n/locales/nl/chat.json | 1 + webview-ui/src/i18n/locales/pl/chat.json | 1 + webview-ui/src/i18n/locales/pt-BR/chat.json | 1 + webview-ui/src/i18n/locales/ru/chat.json | 1 + webview-ui/src/i18n/locales/tr/chat.json | 1 + webview-ui/src/i18n/locales/vi/chat.json | 1 + webview-ui/src/i18n/locales/zh-CN/chat.json | 1 + webview-ui/src/i18n/locales/zh-TW/chat.json | 1 + .../utils/__tests__/command-parser.spec.ts | 42 ++ webview-ui/src/utils/command-parser.ts | 57 +- 29 files changed, 1457 insertions(+), 49 deletions(-) create mode 100644 ai_plans/2026-06-17_zoo-483-multiline-quoted-command-parsing.md create mode 100644 src/shared/__tests__/parse-command.spec.ts diff --git a/ai_plans/2026-06-17_zoo-483-multiline-quoted-command-parsing.md b/ai_plans/2026-06-17_zoo-483-multiline-quoted-command-parsing.md new file mode 100644 index 0000000000..6594ac7593 --- /dev/null +++ b/ai_plans/2026-06-17_zoo-483-multiline-quoted-command-parsing.md @@ -0,0 +1,125 @@ +# Port plan — Zoo PR #483 → `feature/zoo-483-multiline-quoted-command-parsing` + +> **For the executor (read first).** Do the steps **in order**. Do **not** +> improvise or hand-edit the parser — this port lands as a verified-clean +> `git apply` of the upstream diff with a single file excluded. If any +> `git apply --check` reports a conflict, **STOP and report** — do not force it +> or hand-merge. This repo is **Tumble Code**: never introduce the strings "Roo" +> or "Zoo" in user-facing text. + +--- + +## 0. Context (read once, write no code) + +- **Upstream:** Zoo PR #483 — "fix(commands): correct multi-line quoted command parsing, auto-approval, and malformed-command error surfacing" (commit `d7bc9f687`). +- **What it does, one paragraph:** Our shared command parser [parse-command.ts](src/shared/parse-command.ts) + splits on every newline _before_ any quote handling, so newlines inside a + quoted argument (e.g. a multi-line script in `sh -c '…'`), inside a heredoc + body, or inside an unterminated quote are wrongly treated as separate + sub-commands. That defeats allowlist auto-approval and produces noisy, + spurious entries in the command-pattern breakdown UI. This PR rewrites the + parser to mask quoted regions (single / double / ANSI-C `$'…'` / locale + `$"…"` / heredocs / herestrings) via a single `scanTopLevelQuotes` state + machine before splitting, detects unterminated quotes/heredocs as shell syntax + errors, and changes `parseCommand`'s return type from `string[]` to + `ParseResult { commands: string[]; parseError: UnterminatedQuote | null }`. + Callers (`getCommandDecision`, `ExecuteCommandTool`, the webview + `CommandExecution` card and `extractPatternsFromCommand`) are updated to read + the new shape; a new `malformed_command` decision and a CommandExecutionStatus + `error` status surface the syntax error to the agent and the UI. +- **Why we want it, with evidence in OUR code:** [parse-command.ts:22-33](src/shared/parse-command.ts#L22-L33) + — our `parseCommand` does `command.split(/\r\n|\r|\n/)` first, then + `parseCommandLine` per line, so a quoted/heredoc newline splits one command + into bogus sub-commands. [commands.ts:266](src/core/auto-approval/commands.ts#L266) + consumes that split for auto-approval, so a fragment of a malformed command can + be independently auto-approved today. +- **Verified before-state:** Our working tree matches upstream's pre-#483 state + for **all 27 code/i18n files** — confirmed by `git apply --check` passing on + the core group, the webview group, and the full diff minus the changeset. +- **What we deliberately leave out (YAGNI):** the changeset file + `.changeset/fix-multiline-quoted-command-parsing.md` — it carries the + `"zoo-code"` package id (Zoo branding) and is Zoo release-prep; our fork does + not keep per-PR changesets. Excluded from the apply. +- **Original author — credit.** Andrew Schmeder. Commit trailer: + + ```text + Co-authored-by: Andrew Schmeder <149117631+awschmeder@users.noreply.github.com> + ``` + +## 1. Preconditions — verify before touching anything + +- [ ] Current branch is `feature/zoo-483-multiline-quoted-command-parsing`, created off `main`. +- [ ] Working tree clean (`git status --short` empty). +- [ ] The Zoo clone exists at `/home/krzych/Projekty/QUB-IT/Zoo-Code`. + +## 2. Regenerate the diff and confirm it applies (no code yet) + +```bash +cd /home/krzych/Projekty/QUB-IT/Roo-Code +git -C /home/krzych/Projekty/QUB-IT/Zoo-Code show d7bc9f687 > /tmp/zoo-483.diff +git apply --check --exclude='.changeset/*' /tmp/zoo-483.diff && echo "APPLIES CLEANLY" +``` + +- **Expect:** `APPLIES CLEANLY`. If `git apply --check` prints any error, **STOP + and report** — the before-state has drifted and this plan is stale. + +## 3. Apply the diff (excluding the Zoo changeset) + +```bash +cd /home/krzych/Projekty/QUB-IT/Roo-Code +git apply --exclude='.changeset/*' /tmp/zoo-483.diff +git status --short +``` + +- **Expect exactly these 28 files changed** (the 29th, `.changeset/…md`, is + intentionally excluded): `packages/types/src/terminal.ts`, + `src/core/auto-approval/commands.ts`, + `src/core/auto-approval/__tests__/commands.spec.ts`, + `src/core/tools/ExecuteCommandTool.ts`, `src/shared/parse-command.ts`, + `src/shared/__tests__/parse-command.spec.ts`, + `webview-ui/src/components/chat/CommandExecution.tsx`, + `webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx`, + `webview-ui/src/utils/command-parser.ts`, + `webview-ui/src/utils/__tests__/command-parser.spec.ts`, and the 18 i18n + `chat.json` files (`en` + 17 locales). +- If `git status` shows the `.changeset` file, delete it: + `rm -f .changeset/fix-multiline-quoted-command-parsing.md`. + +## 4. Out of scope — do NOT do these + +- Do **not** keep the `.changeset/fix-multiline-quoted-command-parsing.md` file + (contains `"zoo-code"`; Zoo release-prep). +- Do **not** hand-edit the parser or "improve" the applied code. +- Do **not** re-add or re-wire: **TTS**, the **router / cloud provider**, **cloud + upsell** UI, or **Roo/Zoo branding**. +- Do **not** rename internal ids (those stay `Roo-Code`). + +## 5. Verify — paste real output, don't claim success without it + +Run from the repo root: + +- `cd src && npx vitest run shared/__tests__/parse-command.spec.ts` → all pass. +- `cd src && npx vitest run core/auto-approval/__tests__/commands.spec.ts` → all pass. +- `cd src && pnpm check-types` → clean. +- `cd webview-ui && npx vitest run src/utils/__tests__/command-parser.spec.ts src/components/chat/__tests__/CommandExecution.spec.tsx` → all pass. +- `cd webview-ui && npx vitest run` is overkill; the two suites above cover the change. + +## 6. Acceptance criteria (binary — all must hold) + +- [ ] `parse-command.spec.ts`, `commands.spec.ts`, `command-parser.spec.ts`, + `CommandExecution.spec.tsx` all green. +- [ ] `pnpm check-types` clean (the `parseCommand` return-type change touches + `commands.ts`, `ExecuteCommandTool.ts`, `CommandExecution.tsx`, + `command-parser.ts` — all updated by the diff). +- [ ] `git status` shows the 28 expected files (NOT the `.changeset` file). +- [ ] English i18n value is "Malformed command: shell syntax error"; no new + "Roo"/"Zoo" strings; no removed feature reintroduced. + +## 7. Record in the ledger + +Already recorded by the orchestrator after the plan file is written. The commit +(done by the orchestrator) will carry: + +```text +Co-authored-by: Andrew Schmeder <149117631+awschmeder@users.noreply.github.com> +``` diff --git a/packages/types/src/terminal.ts b/packages/types/src/terminal.ts index 34f7a74e24..3a32866cdb 100644 --- a/packages/types/src/terminal.ts +++ b/packages/types/src/terminal.ts @@ -29,6 +29,11 @@ export const commandExecutionStatusSchema = z.discriminatedUnion("status", [ executionId: z.string(), status: z.literal("timeout"), }), + z.object({ + executionId: z.string(), + status: z.literal("error"), + message: z.string().optional(), + }), ]) export type CommandExecutionStatus = z.infer diff --git a/src/core/auto-approval/__tests__/commands.spec.ts b/src/core/auto-approval/__tests__/commands.spec.ts index 90d2808ba9..fa5762cb56 100644 --- a/src/core/auto-approval/__tests__/commands.spec.ts +++ b/src/core/auto-approval/__tests__/commands.spec.ts @@ -99,3 +99,63 @@ describe("getCommandDecision — integration with dangerous substitution checks" expect(getCommandDecision('echo "${var@P}"', allowedCommands)).toBe("ask_user") }) }) + +describe("getCommandDecision — multi-line script wrapped in a quoted argument", () => { + // A wrapper command (e.g. sh -c '...') carrying a multi-line script as a + // single quoted argument must be treated as one command. The embedded + // newlines belong to the quoted string and must not be mistaken for a + // multi-statement sequence that would defeat allowlist auto-approval. + const wrappedSingleQuoted = [ + `sh -c 'kubectl exec pod -- python3 -c "`, + `import urllib.request`, + `url = \\"http://127.0.0.1:49527/\\"`, + `try:`, + ` with urllib.request.urlopen(url, timeout=10) as r:`, + ` print(r.status)`, + `except Exception as e:`, + ` print(\\"fetch failed:\\", e)`, + `"'`, + ].join("\n") + + it("auto-approves a multi-line single-quoted script when the wrapper prefix is allowed", () => { + expect(getCommandDecision(wrappedSingleQuoted, ["sh"])).toBe("auto_approve") + }) + + it("auto-approves a multi-line double-quoted script when the wrapper prefix is allowed", () => { + const wrappedDoubleQuoted = 'sh -c "echo line1\necho line2"' + expect(getCommandDecision(wrappedDoubleQuoted, ["sh"])).toBe("auto_approve") + }) + + it("asks user when the wrapper prefix is not in the allowlist", () => { + expect(getCommandDecision(wrappedSingleQuoted, ["git"])).toBe("ask_user") + }) + + it("still splits genuine multi-statement scripts and asks when a statement is not allowed", () => { + // Real unquoted newlines separate independent statements; each must be on + // the allowlist for auto-approval to engage. + const multiStatement = "echo hello\nrm -rf /tmp/x" + expect(getCommandDecision(multiStatement, ["echo"])).toBe("ask_user") + }) + + it("auto-approves a genuine multi-statement script when every statement is allowed", () => { + const multiStatement = "echo hello\nls -la" + expect(getCommandDecision(multiStatement, ["echo", "ls"])).toBe("auto_approve") + }) + + it("auto-approves an ANSI-C quoted ($'...') multi-line argument when the wrapper prefix is allowed", () => { + const ansiC = "sh -c $'echo 1\necho 2'" + expect(getCommandDecision(ansiC, ["sh"])).toBe("auto_approve") + }) + + it("returns malformed_command for a command with an unterminated quote regardless of allowlist", () => { + // An unterminated quote is a shell syntax error. Even if the leading word + // is on the allowlist (or the allowlist is the wildcard), or the exact + // full command string is listed, the command must not be auto-approved -- + // the shell would report a syntax error and any prefix that ran before + // the error could have unintended side effects. + const malformed = "sh -c 'echo a\necho b" + expect(getCommandDecision(malformed, ["sh"])).toBe("malformed_command") + expect(getCommandDecision(malformed, ["*"])).toBe("malformed_command") + expect(getCommandDecision(malformed, [malformed])).toBe("malformed_command") + }) +}) diff --git a/src/core/auto-approval/commands.ts b/src/core/auto-approval/commands.ts index d9e88c7ba2..82b937e66f 100644 --- a/src/core/auto-approval/commands.ts +++ b/src/core/auto-approval/commands.ts @@ -204,7 +204,7 @@ export function isAutoDeniedSingleCommand( /** * Command approval decision types */ -export type CommandDecision = "auto_approve" | "auto_deny" | "ask_user" +export type CommandDecision = "auto_approve" | "auto_deny" | "ask_user" | "malformed_command" /** * Unified command validation that implements the longest prefix match rule. @@ -224,6 +224,7 @@ export type CommandDecision = "auto_approve" | "auto_deny" | "ask_user" * - `"auto_approve"`: All sub-commands are explicitly allowed and no dangerous patterns detected * - `"auto_deny"`: At least one sub-command is explicitly denied * - `"ask_user"`: Mixed or no matches found, requires user decision, or contains dangerous patterns + * - `"malformed_command"`: Command contains an unterminated quote -- a shell syntax error that must not be auto-approved * * **Examples:** * ```typescript @@ -262,8 +263,19 @@ export function getCommandDecision( return "auto_approve" } - // Parse into sub-commands (split by &&, ||, ;, |) - const subCommands = parseCommand(command) + // Parse into sub-commands (split by &&, ||, ;, |). parseCommand also + // detects shell syntax errors (unterminated quotes, unclosed heredocs) and + // returns a non-null parseError in that case. + const { commands: subCommands, parseError } = parseCommand(command) + + // Reject commands with a shell syntax error. An unterminated quote means + // the shell would report a parse error; in a compound command it may + // partially execute the well-formed prefix before aborting. Returning a + // distinct decision lets callers surface a useful message to the agent + // rather than silently presenting the command for user approval. + if (parseError !== null) { + return "malformed_command" + } // Check each sub-command and collect decisions const decisions: CommandDecision[] = subCommands.map((cmd) => { diff --git a/src/core/tools/ExecuteCommandTool.ts b/src/core/tools/ExecuteCommandTool.ts index cd80a6c165..ba838e32d8 100644 --- a/src/core/tools/ExecuteCommandTool.ts +++ b/src/core/tools/ExecuteCommandTool.ts @@ -12,6 +12,7 @@ import { Task } from "../task/Task" import { ToolUse, ToolResponse } from "../../shared/tools" import { formatResponse } from "../prompts/responses" import { unescapeHtmlEntities } from "../../utils/text-normalization" +import { parseCommand } from "../../shared/parse-command" import { ExitCodeDetails, RooTerminalCallbacks, @@ -86,6 +87,25 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> { task.consecutiveMistakeCount = 0 + // Detect shell syntax errors (unterminated quotes, unclosed heredocs) before + // presenting the command for approval. Surfacing this as a tool error gives + // the agent a precise, actionable message so it can retry with a corrected + // command, rather than receiving a generic denial from the approval dialog. + const { parseError } = parseCommand(canonicalCommand) + if (parseError !== null) { + const executionId = task.lastMessageTs?.toString() ?? Date.now().toString() + const provider = await task.providerRef.deref() + const errorStatus: CommandExecutionStatus = { + executionId, + status: "error", + message: parseError.message, + } + provider?.postMessageToWebview({ type: "commandExecutionStatus", text: JSON.stringify(errorStatus) }) + task.didToolFailInCurrentTurn = true + pushToolResult(formatResponse.toolError(parseError.message)) + return + } + const didApprove = await askApproval("command", canonicalCommand) if (!didApprove) { diff --git a/src/shared/__tests__/parse-command.spec.ts b/src/shared/__tests__/parse-command.spec.ts new file mode 100644 index 0000000000..fa41d31c6c --- /dev/null +++ b/src/shared/__tests__/parse-command.spec.ts @@ -0,0 +1,507 @@ +import { findUnterminatedQuote, parseCommand } from "../parse-command" + +// Toggle that lets a single test force shell-quote's parse() to throw so the +// parser's fallback branch can be exercised deterministically. +let forceShellQuoteFailure = false + +vi.mock("shell-quote", async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + parse: (...args: Parameters) => { + if (forceShellQuoteFailure) { + throw new Error("forced parse failure") + } + return actual.parse(...args) + }, + } +}) + +describe("parseCommand", () => { + describe("basic chaining", () => { + it("returns empty array for empty input", () => { + expect(parseCommand("").commands).toEqual([]) + expect(parseCommand(" ").commands).toEqual([]) + }) + + it("returns a single command unchanged", () => { + expect(parseCommand("git status").commands).toEqual(["git status"]) + }) + + it("splits on &&", () => { + expect(parseCommand("git add . && git commit").commands).toEqual(["git add .", "git commit"]) + }) + + it("splits on ||, ;, and |", () => { + expect(parseCommand("a || b").commands).toEqual(["a", "b"]) + expect(parseCommand("a ; b").commands).toEqual(["a", "b"]) + expect(parseCommand("a | b").commands).toEqual(["a", "b"]) + }) + }) + + describe("genuine multi-statement scripts (unquoted newlines)", () => { + it("splits unquoted newlines into separate sub-commands", () => { + const input = "echo a\necho b\necho c" + expect(parseCommand(input).commands).toEqual(["echo a", "echo b", "echo c"]) + }) + + it("handles Windows and old-Mac line endings", () => { + expect(parseCommand("echo a\r\necho b").commands).toEqual(["echo a", "echo b"]) + expect(parseCommand("echo a\recho b").commands).toEqual(["echo a", "echo b"]) + }) + + it("ignores blank lines", () => { + expect(parseCommand("echo a\n\n\necho b").commands).toEqual(["echo a", "echo b"]) + }) + }) + + describe("newlines inside single-quoted strings", () => { + it("treats a multi-line single-quoted argument as one command", () => { + const input = "sh -c 'echo a\necho b'" + expect(parseCommand(input).commands).toEqual(["sh -c 'echo a\necho b'"]) + }) + + it("does not split operators that appear inside single quotes", () => { + const input = "sh -c 'echo a && echo b | grep x'" + expect(parseCommand(input).commands).toEqual(["sh -c 'echo a && echo b | grep x'"]) + }) + + it("preserves the embedded newline in the restored command", () => { + const input = "sh -c 'echo 1\necho 2'" + const { commands: result } = parseCommand(input) + expect(result).toEqual(["sh -c 'echo 1\necho 2'"]) + expect(result[0]).toContain("\n") + }) + }) + + describe("ANSI-C quoting ($'...')", () => { + it("does not leak a placeholder for a $'...' multi-line argument", () => { + const input = "sh -c $'echo 1\necho 2'" + const { commands: result } = parseCommand(input) + // The placeholder used internally must never appear in the output. + expect(result.join(" ")).not.toContain("SQUOTE") + expect(result.join(" ")).not.toContain("__") + }) + + // An escaped apostrophe inside an ANSI-C string must not terminate the + // quoted region early; otherwise a following newline would leak out and + // split the single command into bogus sub-commands. + it("treats a $'...' argument with an escaped apostrophe and newline as one command", () => { + const input = "sh -c $'echo \\'1\\'\necho 2'" + const { commands: result } = parseCommand(input) + expect(result).toEqual([input]) + expect(result.join(" ")).not.toContain("SQUOTE") + expect(result.join(" ")).not.toContain("__") + }) + }) + + describe("newlines inside double-quoted strings", () => { + it("treats a multi-line double-quoted argument as one command", () => { + const input = 'sh -c "echo a\necho b"' + expect(parseCommand(input).commands).toEqual(['sh -c "echo a\necho b"']) + }) + + it("does not split operators that appear inside double quotes", () => { + const input = 'sh -c "echo a && echo b | grep x"' + expect(parseCommand(input).commands).toEqual(['sh -c "echo a && echo b | grep x"']) + }) + + it("handles escaped quotes inside a double-quoted string", () => { + const input = 'sh -c "echo \\"hello world\\""' + expect(parseCommand(input).commands).toEqual(['sh -c "echo \\"hello world\\""']) + }) + }) + + describe("mixed quote styles", () => { + it("does not let an apostrophe inside double quotes start a single-quoted region", () => { + const input = `echo "don't" && echo ok` + expect(parseCommand(input).commands).toEqual([`echo "don't"`, "echo ok"]) + }) + + it("does not let a double quote inside single quotes start a double-quoted region", () => { + const input = `echo 'a " b' && echo ok` + expect(parseCommand(input).commands).toEqual([`echo 'a " b'`, "echo ok"]) + }) + }) + + describe("comment lines containing quote characters", () => { + // A quote character inside a # comment is not shell quoting, so it must + // not be paired with a quote on a later line. The newline that ends the + // comment line stays a command separator and the following command is + // parsed independently. + it("does not let a quote inside a # comment hide the newline before the next command", () => { + // The unmatched single quote in the comment must not absorb the + // newline; line 2 must surface as its own sub-command. + const input = "echo hello # it's a comment\necho world" + const { commands: result } = parseCommand(input) + expect(result).toHaveLength(2) + expect(result[result.length - 1]).toBe("echo world") + }) + + it("does not let a double-quote inside a # comment hide the newline before the next command", () => { + // The unmatched double quote in the comment must not absorb the newline. + const input = `echo hello # say "hi\necho world` + const { commands: result } = parseCommand(input) + expect(result).toHaveLength(2) + expect(result[result.length - 1]).toBe("echo world") + }) + + // A comment quote that could otherwise pair with a quote several lines + // down must not swallow the intervening newlines. Every command stays + // separate so each is evaluated on its own, and the comment tail itself + // is discarded (it is not part of any executable command). + it("keeps every command separate when an earlier comment contains a quote", () => { + const input = "echo first # what's here\necho middle\necho 'done'" + const { commands: result } = parseCommand(input) + expect(result).toEqual(["echo first", "echo middle", "echo 'done'"]) + }) + }) + + describe("real-world wrapped multi-line script (regression)", () => { + it("treats a wrapper command with an embedded multi-line script as a single command", () => { + const input = [ + `sh -c 'kubectl exec pod -- python3 -c "`, + `import urllib.request`, + `url = \\"http://127.0.0.1:49527/\\"`, + `try:`, + ` with urllib.request.urlopen(url, timeout=10) as r:`, + ` for k, v in r.headers.items():`, + ` print(f\\"{k}: {v}\\")`, + `except Exception as e:`, + ` print(\\"fetch failed:\\", type(e).__name__, e)`, + `"'`, + ].join("\n") + + const { commands: result } = parseCommand(input) + expect(result).toHaveLength(1) + expect(result[0]).toBe(input) + }) + }) + + describe("subshells still split", () => { + it("extracts subshell content as separate commands", () => { + const { commands: result } = parseCommand("echo $(whoami)") + expect(result).toContain("whoami") + }) + }) + + describe("shell-quote parse-failure fallback", () => { + afterEach(() => { + forceShellQuoteFailure = false + }) + + // When shell-quote throws, the parser falls back to a crude operator + // split and must still restore every masked placeholder -- including the + // ANSI-C single-quote bucket -- so callers never see internal markers. + it("restores ANSI-C quoted placeholders in the fallback path", () => { + forceShellQuoteFailure = true + + const { commands: result } = parseCommand("sh -c $'echo hi' && echo done") + expect(result).toEqual(["sh -c $'echo hi'", "echo done"]) + expect(result.join(" ")).not.toContain("SQUOTE") + expect(result.join(" ")).not.toContain("__") + }) + }) + + describe("malformed input with unterminated quotes", () => { + // A command with an unterminated quote is a shell syntax error. The + // parseError field is non-null and commands contains the raw input as a + // single opaque token so no embedded line can be auto-approved alone. + it("returns an unclosed single-quoted command as one opaque token with a parseError", () => { + const input = "sh -c 'echo test" + const { commands, parseError } = parseCommand(input) + expect(commands).toEqual([input]) + expect(parseError).not.toBeNull() + expect(parseError?.quoteType).toBe("posix-single") + }) + + it("does not split an unclosed single quote on an embedded newline", () => { + const input = "sh -c 'echo a\necho b" + // Without the guard, the second line would surface as a standalone + // `echo b` even though it was meant to live inside the unclosed quote. + const { commands, parseError } = parseCommand(input) + expect(commands).toEqual([input]) + expect(parseError).not.toBeNull() + }) + + it("returns an unclosed double-quoted command as one opaque token with a parseError", () => { + const input = 'sh -c "echo a\necho b' + const { commands, parseError } = parseCommand(input) + expect(commands).toEqual([input]) + expect(parseError?.quoteType).toBe("double") + }) + + it("returns an unclosed ANSI-C quoted command as one opaque token with a parseError", () => { + const input = "sh -c $'echo a\necho b" + const { commands, parseError } = parseCommand(input) + expect(commands).toEqual([input]) + expect(parseError?.quoteType).toBe("ansi-c") + }) + + it("treats odd-quote improper quoting as a single opaque token with a parseError", () => { + const input = "sh -c 'cmd ''\\\n" + const { commands, parseError } = parseCommand(input) + expect(commands).toEqual([input]) + expect(parseError).not.toBeNull() + }) + }) + + describe("heredoc quoting", () => { + // The entire heredoc -- opener line, body, and terminator -- must be + // treated as a single opaque token so body lines are never split into + // independent sub-commands for auto-approval evaluation. + + it("treats an unquoted-delimiter heredoc as one command", () => { + const input = "sh -c bash << EOF\necho hello\nEOF" + expect(parseCommand(input).commands).toEqual([input]) + }) + + it("treats a single-quoted-delimiter heredoc as one command", () => { + const input = "sh -c bash << 'EOF'\necho hello\nEOF" + expect(parseCommand(input).commands).toEqual([input]) + }) + + it("treats a double-quoted-delimiter heredoc as one command", () => { + const input = 'sh -c bash << "EOF"\necho hello\nEOF' + expect(parseCommand(input).commands).toEqual([input]) + }) + + it("treats a backslash-escaped-delimiter heredoc as one command", () => { + const input = "sh -c bash << \\EOF\necho hello\nEOF" + expect(parseCommand(input).commands).toEqual([input]) + }) + + it("treats a <<- heredoc (strip leading tabs) as one command", () => { + // <<- allows the terminator to be indented with tabs. + const input = "sh -c bash <<-EOF\n\techo hello\n\tEOF" + expect(parseCommand(input).commands).toEqual([input]) + }) + + it("does not split body lines of a multi-line heredoc", () => { + const input = ["sh -c bash << 'EOF'", "echo line1", "echo line2", "echo line3", "EOF"].join("\n") + expect(parseCommand(input).commands).toEqual([input]) + }) + + it("splits a command that follows the heredoc terminator", () => { + const input = "sh -c bash << EOF\necho hello\nEOF\necho done" + const { commands: result } = parseCommand(input) + expect(result).toHaveLength(2) + expect(result[0]).toBe("sh -c bash << EOF\necho hello\nEOF") + expect(result[1]).toBe("echo done") + }) + + it("treats a heredoc with a missing terminator as one opaque token", () => { + // An unterminated heredoc is a syntax error; the whole input must be + // returned as a single token so no body line can be auto-approved alone. + const input = "sh -c bash << EOF\necho hello" + expect(parseCommand(input).commands).toEqual([input]) + }) + + it("treats the real-world sh heredoc pattern as one command", () => { + const input = [ + "sh -c bash << 'EOF'", + "echo line1 > /tmp/test.txt", + "echo line2 \\", + " --flag value \\", + " --other value", + "EOF", + ].join("\n") + expect(parseCommand(input).commands).toEqual([input]) + }) + it("does not treat a # comment inside a heredoc body as a command separator", () => { + // A '#' inside a heredoc body is literal text, not a shell comment. + // The body must not be split and the comment line must be preserved. + const input = "sh -c bash << 'EOF'\n# this is a comment\necho hello\nEOF" + expect(parseCommand(input).commands).toEqual([input]) + }) + + it("does not mistake << inside a # comment as a heredoc opener", () => { + // A heredoc opener that appears inside a # comment must be ignored; + // the following lines must still be treated as separate commands. + const input = "echo hi # << EOF\necho world" + const { commands: result } = parseCommand(input) + expect(result).toHaveLength(2) + expect(result[result.length - 1]).toBe("echo world") + }) + }) + + describe("herestring (<<<)", () => { + // A herestring (<<<) feeds a single word as stdin -- it has no body region + // or terminator. The word to the right is a normal shell word (possibly + // quoted) and must NOT be treated as a heredoc body. + + it("treats a simple herestring as one command", () => { + const input = `sh <<< "echo hello"` + expect(parseCommand(input).commands).toEqual([input]) + }) + + it("does not scan for a heredoc terminator after <<<", () => { + // Without the <<< guard the parser would treat <<< as << followed by + // delimiter "<", then scan for a line exactly equal to "<" -- consuming + // the rest of input and returning it as an unterminated heredoc. + const input = "sh <<< 'echo hello'\necho done" + const { commands: result } = parseCommand(input) + expect(result).toHaveLength(2) + expect(result[0]).toBe("sh <<< 'echo hello'") + expect(result[1]).toBe("echo done") + }) + + it("treats a herestring with a single-quoted multiline word as one command", () => { + // The quoted word after <<< may contain embedded newlines via quoting; + // those newlines are handled by the single-quote masker, not the heredoc + // scanner. The whole construct is one command. + const input = "sh <<< 'echo line1\necho line2'" + expect(parseCommand(input).commands).toEqual([input]) + }) + + it("treats a herestring with an ANSI-C quoted multiline word as one command", () => { + const input = "sh <<< $'echo line1\\necho line2'" + expect(parseCommand(input).commands).toEqual([input]) + }) + + it("does not report an unterminated quote for a balanced herestring", () => { + expect(findUnterminatedQuote(`sh <<< "echo hello"`)).toBeNull() + expect(findUnterminatedQuote("sh <<< 'echo hello'")).toBeNull() + }) + }) + + describe('locale quoting ($"...")', () => { + // Locale quoting $"..." behaves like double quotes for delimiter purposes + // but the $ prefix is part of the token and must be preserved verbatim. + + it("treats a locale-quoted argument as one command", () => { + const input = `echo $"hello world"` + expect(parseCommand(input).commands).toEqual([input]) + }) + + it("does not split on operators inside a locale-quoted string", () => { + const input = `echo $"hello && world"` + expect(parseCommand(input).commands).toEqual([input]) + }) + + it("does not split on a newline inside a locale-quoted string", () => { + const input = `echo $"hello\nworld"` + expect(parseCommand(input).commands).toEqual([input]) + }) + + it("preserves the $ prefix in the output without leaking placeholders", () => { + const input = `echo $"greeting" && echo done` + const { commands: result } = parseCommand(input) + expect(result[0]).toContain('$"greeting"') + expect(result.join(" ")).not.toContain("__") + }) + + it("returns an unterminated locale-quoted string as one opaque token with a parseError", () => { + const input = `echo $"hello` + const { commands, parseError } = parseCommand(input) + expect(commands).toEqual([input]) + expect(parseError?.quoteType).toBe("locale") + }) + }) + + describe("placeholder collision guard", () => { + // If the raw command literally contains the internal placeholder tokens + // (e.g. __QUOTE_0__) the pre-escape/post-unescape round-trip must preserve + // them verbatim instead of corrupting them during the masking phase. + it("round-trips a command that contains every internal placeholder token", () => { + const input = + "echo __QUOTE_0__ __SQUOTE_0__ __REDIR_0__ __ARITH_0__ __PARAM_0__ __VAR_0__ __SUBSH_0__ __TOPLEVEL_QUOTE_0__" + expect(parseCommand(input).commands).toEqual([input]) + }) + }) + + describe("findUnterminatedQuote", () => { + it("returns null for balanced and quote-free input", () => { + expect(findUnterminatedQuote("git status")).toBeNull() + expect(findUnterminatedQuote("echo 'hello'")).toBeNull() + expect(findUnterminatedQuote('echo "hello"')).toBeNull() + expect(findUnterminatedQuote("sh -c 'echo a\necho b'")).toBeNull() + expect(findUnterminatedQuote("sh -c $'echo a\necho b'")).toBeNull() + }) + + it("detects an unterminated single quote and reports its opening index", () => { + expect(findUnterminatedQuote("echo 'hello")).toEqual( + expect.objectContaining({ quoteType: "posix-single", openIndex: 5 }), + ) + }) + + it("detects an unterminated double quote and reports its opening index", () => { + expect(findUnterminatedQuote('echo "hello')).toEqual( + expect.objectContaining({ quoteType: "double", openIndex: 5 }), + ) + }) + + it("reports ansi-c style and the $ position for an unterminated ANSI-C quote", () => { + expect(findUnterminatedQuote("echo $'hello")).toEqual( + expect.objectContaining({ quoteType: "ansi-c", openIndex: 5 }), + ) + }) + + it("treats a backslash-escaped quote outside a string as literal", () => { + // `echo \'` is a literal quote, not an unterminated region. + expect(findUnterminatedQuote("echo \\'")).toBeNull() + expect(findUnterminatedQuote('echo \\"')).toBeNull() + }) + + it("ignores a quote that appears inside a comment", () => { + expect(findUnterminatedQuote("echo hi # it's fine")).toBeNull() + }) + + it("ignores an apostrophe inside a # comment that follows a closed quoted argument", () => { + // A well-formed quoted argument followed by a comment whose content + // contains a quote character. The comment apostrophe must not open a + // new region. + expect(findUnterminatedQuote("echo 'hello' # it's a comment")).toBeNull() + expect(findUnterminatedQuote('echo "hello" # it\'s a comment')).toBeNull() + }) + + it("treats # attached to a word as an ordinary character", () => { + expect(findUnterminatedQuote("echo foo#'bar'")).toBeNull() + expect(findUnterminatedQuote("echo foo#'bar")).toEqual( + expect.objectContaining({ quoteType: "posix-single", openIndex: 9 }), + ) + }) + + it("does not let an apostrophe inside double quotes open a region", () => { + expect(findUnterminatedQuote(`echo "don't"`)).toBeNull() + }) + + it("does not let a double quote inside single quotes open a region", () => { + expect(findUnterminatedQuote(`echo 'a " b'`)).toBeNull() + }) + + it("honors backslash escapes inside double quotes", () => { + // The escaped quote does not close the region; the final quote does. + expect(findUnterminatedQuote('echo "a \\" b"')).toBeNull() + expect(findUnterminatedQuote('echo "a \\"')?.quoteType).toBe("double") + }) + + it("honors backslash escapes inside ANSI-C quotes", () => { + expect(findUnterminatedQuote("echo $'it\\'s ok'")).toBeNull() + expect(findUnterminatedQuote("echo $'it\\'s ok")?.quoteType).toBe("ansi-c") + }) + + it("treats POSIX single quotes as opaque to backslashes", () => { + // Inside a POSIX single quote a backslash is literal, so the quote + // closes at the next apostrophe regardless of any preceding backslash. + expect(findUnterminatedQuote("echo 'a\\'")).toBeNull() + }) + + it("reports locale style and the $ position for an unterminated locale quote", () => { + expect(findUnterminatedQuote('echo $"hello')).toEqual( + expect.objectContaining({ quoteType: "locale", openIndex: 5 }), + ) + }) + + it("returns null for a balanced locale-quoted string", () => { + expect(findUnterminatedQuote('echo $"hello world"')).toBeNull() + }) + + it("returns null for a balanced <<- heredoc with an indented terminator", () => { + // stripTabs must be true for <<- so the indented terminator line is + // recognized; without the fix every terminator line was unconditionally + // stripped of tabs, making <<- and << behave identically. + expect(findUnterminatedQuote("sh <<-EOF\n\techo hello\n\tEOF")).toBeNull() + }) + }) +}) diff --git a/src/shared/parse-command.ts b/src/shared/parse-command.ts index a8d87b76ac..6b2fb4d267 100644 --- a/src/shared/parse-command.ts +++ b/src/shared/parse-command.ts @@ -2,39 +2,538 @@ import { parse } from "shell-quote" export type ShellToken = string | { op: string } | { command: string } +/** + * The style of quoting that opened a region. + * + * - `posix-single` and `ansi-c` share the `'` delimiter but follow different + * escaping rules, so they must be distinguished. + * - `locale` and `double` share the `"` delimiter but `locale` is prefixed with + * `$` (like ANSI-C), making `$"..."` a distinct token for accurate restoration. + * - `heredoc` covers `< = { + "posix-single": "single quote (')", + "ansi-c": "ANSI-C quote ($')", + double: 'double quote (")', + locale: 'locale quote ($")', + heredoc: "heredoc (<<)", + } + const snippetStart = Math.max(0, openIndex - 10) + const snippetEnd = Math.min(command.length, openIndex + 20) + const prefix = snippetStart > 0 ? "..." : "" + const suffix = snippetEnd < command.length ? "..." : "" + const excerpt = prefix + command.slice(snippetStart, snippetEnd).replace(/\r?\n/g, "\\n") + suffix + return `Malformed command: unterminated ${labels[quoteType]} at position ${openIndex} -- near: \`${excerpt}\`. ` +} + +/** + * Scan a command string left-to-right with a small state machine and report the + * first quoted region that is never closed, or `null` when every quoted region + * is properly terminated. + * + * A regex cannot reliably answer "is this command well-quoted?" because quoting + * is context-sensitive: a backslash escapes the next character outside single + * quotes, `#` may begin a comment that should be ignored, and an apostrophe + * inside double quotes (or vice versa) is literal text rather than a delimiter. + * This walk mirrors how a POSIX shell tokenizes quoting so that legitimate + * multi-line quoted arguments are accepted while genuinely unterminated quotes + * (a shell syntax error) are detected. + * + * Rules implemented: + * - Outside any quote, a backslash escapes the following character, so `\'` and + * `\"` are literal and do not open a region. + * - Outside any quote, `#` begins a comment when it is at the start of the input + * or preceded by whitespace; the remainder of that line is ignored. A `#` that + * is attached to a word (e.g. `foo#bar`) is an ordinary character. + * - Single quotes are opaque: no escapes apply, and the region ends only at the + * next `'`. + * - Double quotes honor backslash escapes, so `\"` does not close the region. + * - ANSI-C quoting ($'...') behaves like a single-quoted region for delimiter + * purposes but honors backslash escapes, so `\'` does not close it. + * - Locale quoting ($"...") behaves like double quotes but is opened by `$"` so + * the leading `$` is part of the token (same pattern as ANSI-C). + * - Heredoc (`< 0) { + let found = false + while (i < command.length) { + const lineStart = i + while (i < command.length && command[i] !== "\n" && command[i] !== "\r") { + i++ + } + // Strip leading tabs only for <<- heredocs. + const rawLine = command.slice(lineStart, i) + const line = stripTabs ? rawLine.replace(/^\t*/, "") : rawLine + // Do NOT advance past the terminator's newline -- leave it as a + // separator for any command that follows the heredoc. + if (line === delimiter) { + found = true + break + } + if (i < command.length) i++ // consume newline of a body line + } + if (!found) { + return { + spans, + unterminatedQuote: { + quoteType: "heredoc", + openIndex: start, + message: unterminatedQuoteMessage("heredoc", start, command), + }, + } + } + } + spans.push({ start, end: i, quoteType: "heredoc" }) + continue + } + + // ANSI-C quoting: $'...', escape-aware. + if (char === "$" && command[i + 1] === "'") { + const start = i + i += 2 // skip $' + let closed = false + while (i < command.length) { + if (command[i] === "\\") { + i += 2 // skip escaped char + } else if (command[i] === "'") { + i++ // consume closing ' + closed = true + break + } else { + i++ + } + } + if (!closed) { + return { + spans, + unterminatedQuote: { + quoteType: "ansi-c", + openIndex: start, + message: unterminatedQuoteMessage("ansi-c", start, command), + }, + } + } + spans.push({ start, end: i, quoteType: "ansi-c" }) + continue + } + + // Locale quoting: $"...", escape-aware like double quotes. + if (char === "$" && command[i + 1] === '"') { + const start = i + i += 2 // skip $" + let closed = false + while (i < command.length) { + if (command[i] === "\\") { + i += 2 // skip escaped char + } else if (command[i] === '"') { + i++ // consume closing " + closed = true + break + } else { + i++ + } + } + if (!closed) { + return { + spans, + unterminatedQuote: { + quoteType: "locale", + openIndex: start, + message: unterminatedQuoteMessage("locale", start, command), + }, + } + } + spans.push({ start, end: i, quoteType: "locale" }) + continue + } + + // POSIX single quote: fully opaque, ends at the next literal '. + if (char === "'") { + const start = i + i++ // skip opening ' + while (i < command.length && command[i] !== "'") { + i++ + } + if (i >= command.length) { + // No closing quote found. + return { + spans, + unterminatedQuote: { + quoteType: "posix-single", + openIndex: start, + message: unterminatedQuoteMessage("posix-single", start, command), + }, + } + } + i++ // consume closing ' + spans.push({ start, end: i, quoteType: "posix-single" }) + continue + } + + // Double quote: escape-aware, ends at the next unescaped ". + if (char === '"') { + const start = i + i++ // skip opening " + let closed = false + while (i < command.length) { + if (command[i] === "\\") { + i += 2 // skip escaped char + } else if (command[i] === '"') { + i++ // consume closing " + closed = true + break + } else { + i++ + } + } + if (!closed) { + return { + spans, + unterminatedQuote: { + quoteType: "double", + openIndex: start, + message: unterminatedQuoteMessage("double", start, command), + }, + } + } + spans.push({ start, end: i, quoteType: "double" }) + continue + } + + // Ordinary character -- advance. + i++ + } + + return { spans, unterminatedQuote: null } +} + +/** + * Walk `command` and replace every top-level quoted region with a placeholder + * token. Returns the masked string and the array of original quoted substrings + * so callers can restore them later. + * + * Delegates all state-machine logic to `scanTopLevelQuotes`, ensuring + * consistent quoting rules with `findUnterminatedQuote`. An unterminated quote + * is treated as a span that runs to the end of the string so the masked output + * is always well-formed and the subsequent split never exposes a fragment of + * a malformed command. + */ +function maskTopLevelQuotes(command: string): { masked: string; quotes: string[] } { + const { spans, unterminatedQuote } = scanTopLevelQuotes(command) + + // If the command has an unterminated quote, treat the unclosed region as a + // span running to the end of the string. This is safe because parseCommand + // already guards against unterminated quotes and returns the raw command as + // a single token before calling maskTopLevelQuotes -- but the fallback + // ensures maskTopLevelQuotes is robust even when called directly. + const effectiveSpans: QuoteSpan[] = + unterminatedQuote !== null + ? [ + ...spans, + { start: unterminatedQuote.openIndex, end: command.length, quoteType: unterminatedQuote.quoteType }, + ] + : spans + + const quotes: string[] = [] + let result = "" + let pos = 0 + + for (const span of effectiveSpans) { + // Copy the unquoted text between the previous span end and this span start. + result += command.slice(pos, span.start) + // Replace the quoted span with a placeholder. + quotes.push(command.slice(span.start, span.end)) + result += `__TOPLEVEL_QUOTE_${quotes.length - 1}__` + pos = span.end + } + + // Copy any remaining text after the last span (or the whole string if no spans). + result += command.slice(pos) + + return { masked: result, quotes } +} + /** * Split a command string into individual sub-commands by - * chaining operators (&&, ||, ;, |, or &) and newlines. + * chaining operators (&&, ||, ;, |, or &) and unquoted newlines. * * Uses shell-quote to properly handle: - * - Quoted strings (preserves quotes) + * - Quoted strings (preserves quotes, including multi-line quoted strings) * - Subshell commands ($(cmd), `cmd`, <(cmd), >(cmd)) * - PowerShell redirections (2>&1) * - Chain operators (&&, ||, ;, |, &) - * - Newlines as command separators + * - Newlines as command separators (only when outside quoted strings) + * + * Key invariant: newlines that appear inside a quoted string (single or double) + * are part of that string argument and must NOT be treated as command separators. + * Only unquoted newlines split commands. For example: + * + * sh -c 'python3 -c " + * import sys + * print(sys.version) + * "' + * + * ...is a single command, not multiple commands split at each newline. + * + * Returns a `ParseResult` containing the list of sub-commands and an optional + * `parseError` that is non-null when the input contains a shell syntax error + * (currently: an unterminated quote or heredoc). When `parseError` is set the + * `commands` array contains the raw input as a single opaque token; callers + * should surface the error rather than proceeding with auto-approval. */ -export function parseCommand(command: string): string[] { +export function parseCommand(command: string): ParseResult { if (!command?.trim()) { - return [] + return { commands: [], parseError: null } } - // Split by newlines first (handle different line ending formats) - // This regex splits on \r\n (Windows), \n (Unix), or \r (old Mac) - const lines = command.split(/\r\n|\r|\n/) + // Run the shared state-machine scan once. It gives us both the list of + // top-level quoted spans (used by maskTopLevelQuotes) and the parse-error + // descriptor (used here to detect malformed input) in a single pass. + const { unterminatedQuote } = scanTopLevelQuotes(command) + + // Reject syntactically malformed input -- an unterminated quote is a shell + // syntax error. Return the raw input as a single opaque token so callers + // can surface the error to the agent rather than splitting unsafe fragments + // that might be auto-approved in isolation. + if (unterminatedQuote !== null) { + return { commands: [command], parseError: unterminatedQuote } + } + + // Pre-escape any literal __ sequences present in the raw command so they + // cannot collide with the internal placeholder tokens (e.g. __QUOTE_0__) + // used during masking and splitting. \x00 (the null byte, U+0000) is the + // sentinel: it encodes end-of-string at the C/OS level, so the OS terminates + // any command string at the first \x00 -- meaning a real shell command can + // never contain one. It therefore cannot appear in any command text the + // parser receives and will never match a placeholder regex. The post-unescape + // step at the return converts \x00 back to __ in every output command. + const escapedCommand = command.replace(/__/g, "\x00") + + // Mask quoted strings before splitting on newlines so that newlines embedded + // inside a quoted argument are not mistaken for command separators. The + // masker delegates to scanTopLevelQuotes internally, ensuring identical + // quoting rules with the check above. + const { masked, quotes: topLevelQuotes } = maskTopLevelQuotes(escapedCommand) + + // Split on unquoted newlines (all line-ending formats). + const lines = masked.split(/\r\n|\r|\n/) const allCommands: string[] = [] for (const line of lines) { - // Skip empty lines if (!line.trim()) { continue } - // Process each line through the existing parsing logic - const lineCommands = parseCommandLine(line) + // Restore top-level quote placeholders before per-line parsing so that + // parseCommandLine sees the original quoted content and can apply its own + // masking for operator splitting. + const restoredLine = line.replace(/__TOPLEVEL_QUOTE_(\d+)__/g, (_, i) => topLevelQuotes[parseInt(i)]) + + // If the restored line contains embedded newlines it means a top-level + // quote (e.g. a heredoc) spanned multiple lines. The entire restored + // string is a single atomic command -- passing it through parseCommandLine + // would let shell-quote split on the embedded newlines and << operators. + if (restoredLine.includes("\n")) { + allCommands.push(restoredLine) + continue + } + + const lineCommands = parseCommandLine(restoredLine) allCommands.push(...lineCommands) } - return allCommands + // Unescape \x00 back to __ in every output command. This reverses the + // pre-escape applied above to prevent literal __ tokens in the input from + // colliding with internal placeholder names. split/join is used instead of + // a regex literal to avoid triggering the no-control-regex lint rule. + return { commands: allCommands.map((cmd) => cmd.split("\x00").join("__")), parseError: null } } /** @@ -47,7 +546,7 @@ function parseCommandLine(command: string): string[] { const redirections: string[] = [] const subshells: string[] = [] const quotes: string[] = [] - const arrayIndexing: string[] = [] + const singleQuotes: string[] = [] const arithmeticExpressions: string[] = [] const variables: string[] = [] const parameterExpansions: string[] = [] @@ -84,6 +583,27 @@ function parseCommandLine(command: string): string[] { return `__SUBSH_${subshells.length - 1}__` }) + // Handle locale quoting: $"...". This must run before variable masking for + // the same reason as ANSI-C: without it the generic double-quote masker would + // strip the outer quotes leaving a bare $ that the variable regex absorbs, + // corrupting the placeholder. The whole token (including $") is stored in the + // double-quote bucket so it is restored verbatim. + processedCommand = processedCommand.replace(/\$"(?:[^"\\]|\\.)*"/g, (match) => { + quotes.push(match) + return `__QUOTE_${quotes.length - 1}__` + }) + + // Handle ANSI-C quoting: $'...'. This must run before variable masking so the + // leading $ is captured as part of the quoted unit rather than being treated + // as a variable expansion (which would corrupt a following placeholder). + // ANSI-C strings interpret backslash escapes, so the pattern is escape-aware. + // The whole token (including the $ and quotes) is preserved in the single- + // quote bucket so it is restored verbatim. + processedCommand = processedCommand.replace(/\$'(?:[^'\\]|\\.)*'/g, (match) => { + singleQuotes.push(match) + return `__SQUOTE_${singleQuotes.length - 1}__` + }) + // Handle simple variable references: $varname pattern // This prevents shell-quote from splitting $count into separate tokens processedCommand = processedCommand.replace(/\$[a-zA-Z_][a-zA-Z0-9_]*/g, (match) => { @@ -108,8 +628,22 @@ function parseCommandLine(command: string): string[] { return `__SUBSH_${subshells.length - 1}__` }) - // Then handle quoted strings - processedCommand = processedCommand.replace(/"[^"]*"/g, (match) => { + // Mask quoted strings (single and double) so their contents -- including + // operators like &&, |, ; and any embedded newlines -- are not treated as + // command separators. A single left-to-right scan with an alternation is used + // so that whichever quote opens first wins, preventing a quote of one style + // inside a string of the other style from starting a spurious match. + // + // Single quotes are matched literally (POSIX single quotes are fully opaque, + // no escaping inside them). Double quotes use an escape-aware pattern so that + // an escaped quote (\") does not prematurely terminate the match. Negated + // character classes match newlines, so multi-line quoted strings are captured + // as a single token. + processedCommand = processedCommand.replace(/'[^']*'|"(?:[^"\\]|\\.)*"/g, (match) => { + if (match.startsWith("'")) { + singleQuotes.push(match) + return `__SQUOTE_${singleQuotes.length - 1}__` + } quotes.push(match) return `__QUOTE_${quotes.length - 1}__` }) @@ -132,8 +666,8 @@ function parseCommandLine(command: string): string[] { restorePlaceholders( cmd, quotes, + singleQuotes, redirections, - arrayIndexing, arithmeticExpressions, parameterExpansions, variables, @@ -177,13 +711,13 @@ function parseCommandLine(command: string): string[] { commands.push(currentCommand.join(" ")) } - // Restore quotes and redirections + // Restore all placeholders return commands.map((cmd) => restorePlaceholders( cmd, quotes, + singleQuotes, redirections, - arrayIndexing, arithmeticExpressions, parameterExpansions, variables, @@ -198,20 +732,20 @@ function parseCommandLine(command: string): string[] { function restorePlaceholders( command: string, quotes: string[], + singleQuotes: string[], redirections: string[], - arrayIndexing: string[], arithmeticExpressions: string[], parameterExpansions: string[], variables: string[], subshells: string[], ): string { let result = command - // Restore quotes + // Restore double-quoted strings result = result.replace(/__QUOTE_(\d+)__/g, (_, i) => quotes[parseInt(i)]) + // Restore single-quoted strings + result = result.replace(/__SQUOTE_(\d+)__/g, (_, i) => singleQuotes[parseInt(i)]) // Restore redirections result = result.replace(/__REDIR_(\d+)__/g, (_, i) => redirections[parseInt(i)]) - // Restore array indexing expressions - result = result.replace(/__ARRAY_(\d+)__/g, (_, i) => arrayIndexing[parseInt(i)]) // Restore arithmetic expressions result = result.replace(/__ARITH_(\d+)__/g, (_, i) => arithmeticExpressions[parseInt(i)]) // Restore parameter expansions diff --git a/webview-ui/src/components/chat/CommandExecution.tsx b/webview-ui/src/components/chat/CommandExecution.tsx index af1d72c6a5..7ba6aa7e76 100644 --- a/webview-ui/src/components/chat/CommandExecution.tsx +++ b/webview-ui/src/components/chat/CommandExecution.tsx @@ -57,14 +57,17 @@ export const CommandExecution = ({ executionId, text, icon, title }: CommandExec // Extract command patterns from the actual command that was executed const commandPatterns = useMemo(() => { // First get all individual commands (including subshell commands) using parseCommand - const allCommands = parseCommand(command) + const { commands: allCommands } = parseCommand(command) // Then extract patterns from each command using the existing pattern extraction logic const allPatterns = new Set() - // Add all individual commands first + // Add all individual commands first. Multi-line patterns (e.g. heredocs, + // unterminated quotes) are opaque tokens and must not be added verbatim -- + // their body lines would surface as approvable patterns. Only add + // single-line commands; multi-line tokens are covered by pattern extraction. allCommands.forEach((cmd) => { - if (cmd.trim()) { + if (cmd.trim() && !cmd.includes("\n")) { allPatterns.add(cmd.trim()) } }) @@ -164,6 +167,13 @@ export const CommandExecution = ({ executionId, text, icon, title }: CommandExec )} + {status?.status === "error" && ( +
+ +
+ +
+ )}
diff --git a/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx b/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx index f40987d269..2d8b29b1d0 100644 --- a/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx +++ b/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx @@ -605,4 +605,64 @@ Output: expect(terminalOutput).toHaveTextContent("0 total") }) }) + + describe("multi-line script wrapped in a quoted argument", () => { + // A wrapper command carrying a multi-line script inside a single quoted + // argument must be treated as one command. The pattern breakdown must not + // surface stray fragments from the embedded script lines, which would both + // clutter the UI and defeat allowlist auto-approval. + const wrappedCommand = [ + `sh -c 'kubectl exec pod -- python3 -c "`, + `import urllib.request`, + `url = \\"http://127.0.0.1:49527/\\"`, + `print(url)`, + `"'`, + ].join("\n") + + it("does not surface stray script-line fragments in the pattern selector", () => { + render( + + + , + ) + + const selector = screen.getByTestId("command-pattern-selector") + + // The wrapper command should be present as a single pattern. + expect(selector.textContent).toContain("sh") + + // Each script line is rendered as its own only if it was split + // into a separate command. Assert no span exactly equals a script-line + // fragment that would indicate erroneous splitting. + const fragments = Array.from(selector.querySelectorAll("span")).map((s) => s.textContent ?? "") + expect(fragments).not.toContain("import") + expect(fragments).not.toContain("import urllib.request") + expect(fragments).not.toContain("url") + expect(fragments).not.toContain("print") + }) + }) + + describe("heredoc command in pattern selector", () => { + // An unterminated heredoc is returned as one opaque token by parseCommand. + // The pattern selector must show only the leading command word (sh) and + // must not surface EOF, body-line words, or other heredoc internals. + it("shows only the leading command for an unterminated heredoc", () => { + const heredocCommand = "sh << EOF\necho hello" + + render( + + + , + ) + + const selector = screen.getByTestId("command-pattern-selector") + + expect(selector.textContent).toContain("sh") + + const fragments = Array.from(selector.querySelectorAll("span")).map((s) => s.textContent ?? "") + expect(fragments.some((f) => f.includes("EOF"))).toBe(false) + expect(fragments.some((f) => f.includes("echo"))).toBe(false) + expect(fragments.some((f) => f.includes("hello"))).toBe(false) + }) + }) }) diff --git a/webview-ui/src/i18n/locales/ca/chat.json b/webview-ui/src/i18n/locales/ca/chat.json index f34361b3f6..2f55688a1b 100644 --- a/webview-ui/src/i18n/locales/ca/chat.json +++ b/webview-ui/src/i18n/locales/ca/chat.json @@ -241,6 +241,7 @@ "running": "En execució", "pid": "PID: {{pid}}", "exitStatus": "S'ha sortit amb l'estat {{exitCode}}", + "malformedCommand": "Ordre mal formada: error de sintaxi del shell", "manageCommands": "Comandes aprovades automàticament", "addToAllowed": "Afegeix a la llista permesa", "removeFromAllowed": "Elimina de la llista permesa", diff --git a/webview-ui/src/i18n/locales/de/chat.json b/webview-ui/src/i18n/locales/de/chat.json index ef7be52dbe..8c071b08c6 100644 --- a/webview-ui/src/i18n/locales/de/chat.json +++ b/webview-ui/src/i18n/locales/de/chat.json @@ -241,6 +241,7 @@ "running": "Wird ausgeführt", "pid": "PID: {{pid}}", "exitStatus": "Beendet mit Status {{exitCode}}", + "malformedCommand": "Fehlerhafter Befehl: Shell-Syntaxfehler", "manageCommands": "Automatisch genehmigte Befehle", "addToAllowed": "Zur erlaubten Liste hinzufügen", "removeFromAllowed": "Von der erlaubten Liste entfernen", diff --git a/webview-ui/src/i18n/locales/en/chat.json b/webview-ui/src/i18n/locales/en/chat.json index dce6555ddc..8f5caab5c9 100644 --- a/webview-ui/src/i18n/locales/en/chat.json +++ b/webview-ui/src/i18n/locales/en/chat.json @@ -284,6 +284,7 @@ "running": "Running", "pid": "PID: {{pid}}", "exitStatus": "Exited with status {{exitCode}}", + "malformedCommand": "Malformed command: shell syntax error", "manageCommands": "Auto-approved commands", "addToAllowed": "Add to allowed list", "removeFromAllowed": "Remove from allowed list", diff --git a/webview-ui/src/i18n/locales/es/chat.json b/webview-ui/src/i18n/locales/es/chat.json index 74e5690164..f258c0e5e4 100644 --- a/webview-ui/src/i18n/locales/es/chat.json +++ b/webview-ui/src/i18n/locales/es/chat.json @@ -241,6 +241,7 @@ "running": "Ejecutando", "pid": "PID: {{pid}}", "exitStatus": "Salió con el estado {{exitCode}}", + "malformedCommand": "Comando mal formado: error de sintaxis del shell", "manageCommands": "Comandos aprobados automáticamente", "addToAllowed": "Agregar a la lista de permitidos", "removeFromAllowed": "Eliminar de la lista de permitidos", diff --git a/webview-ui/src/i18n/locales/fr/chat.json b/webview-ui/src/i18n/locales/fr/chat.json index 6a71eb6f4a..f3e56c2a75 100644 --- a/webview-ui/src/i18n/locales/fr/chat.json +++ b/webview-ui/src/i18n/locales/fr/chat.json @@ -241,6 +241,7 @@ "running": "En cours d'exécution", "pid": "PID : {{pid}}", "exitStatus": "Terminé avec le statut {{exitCode}}", + "malformedCommand": "Commande mal formée : erreur de syntaxe shell", "manageCommands": "Commandes approuvées automatiquement", "addToAllowed": "Ajouter à la liste autorisée", "removeFromAllowed": "Retirer de la liste autorisée", diff --git a/webview-ui/src/i18n/locales/hi/chat.json b/webview-ui/src/i18n/locales/hi/chat.json index 5903399153..12081aba76 100644 --- a/webview-ui/src/i18n/locales/hi/chat.json +++ b/webview-ui/src/i18n/locales/hi/chat.json @@ -241,6 +241,7 @@ "running": "चल रहा है", "pid": "पीआईडी: {{pid}}", "exitStatus": "{{exitCode}} स्थिति के साथ बाहर निकल गया", + "malformedCommand": "गलत कमांड: shell सिंटैक्स त्रुटि", "manageCommands": "स्वतः-अनुमोदित कमांड", "addToAllowed": "अनुमत सूची में जोड़ें", "removeFromAllowed": "अनुमत सूची से हटाएँ", diff --git a/webview-ui/src/i18n/locales/id/chat.json b/webview-ui/src/i18n/locales/id/chat.json index 94ae38d7eb..6f69924102 100644 --- a/webview-ui/src/i18n/locales/id/chat.json +++ b/webview-ui/src/i18n/locales/id/chat.json @@ -292,6 +292,7 @@ "running": "Menjalankan", "pid": "PID: {{pid}}", "exitStatus": "Keluar dengan status {{exitCode}}", + "malformedCommand": "Perintah tidak valid: kesalahan sintaks shell", "manageCommands": "Perintah yang disetujui secara otomatis", "addToAllowed": "Tambahkan ke daftar yang diizinkan", "removeFromAllowed": "Hapus dari daftar yang diizinkan", diff --git a/webview-ui/src/i18n/locales/it/chat.json b/webview-ui/src/i18n/locales/it/chat.json index bd0020fe34..4de9064e12 100644 --- a/webview-ui/src/i18n/locales/it/chat.json +++ b/webview-ui/src/i18n/locales/it/chat.json @@ -241,6 +241,7 @@ "running": "In esecuzione", "pid": "PID: {{pid}}", "exitStatus": "Uscito con stato {{exitCode}}", + "malformedCommand": "Comando non valido: errore di sintassi della shell", "manageCommands": "Comandi approvati automaticamente", "addToAllowed": "Aggiungi alla lista dei permessi", "removeFromAllowed": "Rimuovi dalla lista dei permessi", diff --git a/webview-ui/src/i18n/locales/ja/chat.json b/webview-ui/src/i18n/locales/ja/chat.json index b96e2f40c6..6d2b19b144 100644 --- a/webview-ui/src/i18n/locales/ja/chat.json +++ b/webview-ui/src/i18n/locales/ja/chat.json @@ -241,6 +241,7 @@ "running": "実行中", "pid": "PID: {{pid}}", "exitStatus": "ステータス {{exitCode}} で終了しました", + "malformedCommand": "不正なコマンド: シェル構文エラー", "manageCommands": "自動承認されたコマンド", "addToAllowed": "許可リストに追加", "removeFromAllowed": "許可リストから削除", diff --git a/webview-ui/src/i18n/locales/ko/chat.json b/webview-ui/src/i18n/locales/ko/chat.json index d8d3a93499..99fedad600 100644 --- a/webview-ui/src/i18n/locales/ko/chat.json +++ b/webview-ui/src/i18n/locales/ko/chat.json @@ -241,6 +241,7 @@ "running": "실행 중", "pid": "PID: {{pid}}", "exitStatus": "상태 {{exitCode}}(으)로 종료됨", + "malformedCommand": "잘못된 명령: shell 구문 오류", "manageCommands": "자동 승인된 명령", "addToAllowed": "허용 목록에 추가", "removeFromAllowed": "허용 목록에서 제거", diff --git a/webview-ui/src/i18n/locales/nl/chat.json b/webview-ui/src/i18n/locales/nl/chat.json index 1ed0db4024..20a95e9c83 100644 --- a/webview-ui/src/i18n/locales/nl/chat.json +++ b/webview-ui/src/i18n/locales/nl/chat.json @@ -236,6 +236,7 @@ "abort": "Afbreken", "pid": "PID: {{pid}}", "exitStatus": "Afgesloten met status {{exitCode}}", + "malformedCommand": "Ongeldig commando: shell syntaxisfout", "manageCommands": "Automatisch goedgekeurde commando's", "addToAllowed": "Toevoegen aan toegestane lijst", "removeFromAllowed": "Verwijderen van toegestane lijst", diff --git a/webview-ui/src/i18n/locales/pl/chat.json b/webview-ui/src/i18n/locales/pl/chat.json index 757013ae92..6088658e55 100644 --- a/webview-ui/src/i18n/locales/pl/chat.json +++ b/webview-ui/src/i18n/locales/pl/chat.json @@ -241,6 +241,7 @@ "abort": "Przerwij", "pid": "PID: {{pid}}", "exitStatus": "Zakończono ze statusem {{exitCode}}", + "malformedCommand": "Nieprawidlowe polecenie: blad skladni shell", "manageCommands": "Polecenia zatwierdzone automatycznie", "addToAllowed": "Dodaj do listy dozwolonych", "removeFromAllowed": "Usuń z listy dozwolonych", diff --git a/webview-ui/src/i18n/locales/pt-BR/chat.json b/webview-ui/src/i18n/locales/pt-BR/chat.json index 7ab75a269b..b0fd630c06 100644 --- a/webview-ui/src/i18n/locales/pt-BR/chat.json +++ b/webview-ui/src/i18n/locales/pt-BR/chat.json @@ -241,6 +241,7 @@ "abort": "Interromper", "pid": "PID: {{pid}}", "exitStatus": "Saiu com o status {{exitCode}}", + "malformedCommand": "Comando malformado: erro de sintaxe do shell", "manageCommands": "Comandos aprovados automaticamente", "addToAllowed": "Adicionar à lista de permitidos", "removeFromAllowed": "Remover da lista de permitidos", diff --git a/webview-ui/src/i18n/locales/ru/chat.json b/webview-ui/src/i18n/locales/ru/chat.json index 1717872ee4..86ee9ede80 100644 --- a/webview-ui/src/i18n/locales/ru/chat.json +++ b/webview-ui/src/i18n/locales/ru/chat.json @@ -236,6 +236,7 @@ "abort": "Прервать", "pid": "PID: {{pid}}", "exitStatus": "Завершено со статусом {{exitCode}}", + "malformedCommand": "Некорректная команда: синтаксическая ошибка shell", "manageCommands": "Управление разрешениями команд", "commandManagementDescription": "Управляйте разрешениями команд: Нажмите ✓, чтобы разрешить автоматическое выполнение, ✗, чтобы запретить выполнение. Шаблоны можно включать/выключать или удалять из списков. Просмотреть все настройки", "addToAllowed": "Добавить в список разрешенных", diff --git a/webview-ui/src/i18n/locales/tr/chat.json b/webview-ui/src/i18n/locales/tr/chat.json index b738605581..efaed4f21d 100644 --- a/webview-ui/src/i18n/locales/tr/chat.json +++ b/webview-ui/src/i18n/locales/tr/chat.json @@ -241,6 +241,7 @@ "abort": "İptal Et", "pid": "PID: {{pid}}", "exitStatus": "{{exitCode}} durumuyla çıkıldı", + "malformedCommand": "Hatalı komut: shell sozdizimi hatasi", "manageCommands": "Komut İzinlerini Yönet", "commandManagementDescription": "Komut izinlerini yönetin: Otomatik yürütmeye izin vermek için ✓'e, yürütmeyi reddetmek için ✗'e tıklayın. Desenler açılıp kapatılabilir veya listelerden kaldırılabilir. Tüm ayarları görüntüle", "addToAllowed": "İzin verilenler listesine ekle", diff --git a/webview-ui/src/i18n/locales/vi/chat.json b/webview-ui/src/i18n/locales/vi/chat.json index 5d34595f31..8fa939c70c 100644 --- a/webview-ui/src/i18n/locales/vi/chat.json +++ b/webview-ui/src/i18n/locales/vi/chat.json @@ -241,6 +241,7 @@ "abort": "Hủy bỏ", "pid": "PID: {{pid}}", "exitStatus": "Đã thoát với trạng thái {{exitCode}}", + "malformedCommand": "Lệnh không hợp lệ: lỗi cú pháp shell", "manageCommands": "Quản lý quyền lệnh", "commandManagementDescription": "Quản lý quyền lệnh: Nhấp vào ✓ để cho phép tự động thực thi, ✗ để từ chối thực thi. Các mẫu có thể được bật/tắt hoặc xóa khỏi danh sách. Xem tất cả cài đặt", "addToAllowed": "Thêm vào danh sách cho phép", diff --git a/webview-ui/src/i18n/locales/zh-CN/chat.json b/webview-ui/src/i18n/locales/zh-CN/chat.json index 0a57b725e5..28a74df775 100644 --- a/webview-ui/src/i18n/locales/zh-CN/chat.json +++ b/webview-ui/src/i18n/locales/zh-CN/chat.json @@ -241,6 +241,7 @@ "abort": "中止", "pid": "PID: {{pid}}", "exitStatus": "已退出，状态码 {{exitCode}}", + "malformedCommand": "命令格式错误：shell 语法错误", "manageCommands": "管理命令权限", "commandManagementDescription": "管理命令权限：点击 ✓ 允许自动执行，点击 ✗ 拒绝执行。可以打开/关闭模式或从列表中删除。查看所有设置", "addToAllowed": "添加到允许列表", diff --git a/webview-ui/src/i18n/locales/zh-TW/chat.json b/webview-ui/src/i18n/locales/zh-TW/chat.json index f35bb84a28..927689f545 100644 --- a/webview-ui/src/i18n/locales/zh-TW/chat.json +++ b/webview-ui/src/i18n/locales/zh-TW/chat.json @@ -287,6 +287,7 @@ "running": "執行中", "pid": "PID: {{pid}}", "exitStatus": "已結束，狀態碼 {{exitCode}}", + "malformedCommand": "命令格式錯誤：shell 語法錯誤", "manageCommands": "自動核准的命令", "addToAllowed": "新增至允許清單", "removeFromAllowed": "從允許清單中移除", diff --git a/webview-ui/src/utils/__tests__/command-parser.spec.ts b/webview-ui/src/utils/__tests__/command-parser.spec.ts index ede8ebe874..0316e2b328 100644 --- a/webview-ui/src/utils/__tests__/command-parser.spec.ts +++ b/webview-ui/src/utils/__tests__/command-parser.spec.ts @@ -133,4 +133,46 @@ describe("extractPatternsFromCommand", () => { expect(patterns).toEqual(["docker", "docker run"]) // Stops at -it flag }) + + describe("heredoc handling", () => { + // A terminated heredoc is a single opaque token from parseCommand. + // extractPatternsFromCommand must extract only the leading command word + // and not feed the raw heredoc body to shell-quote (which would split on + // << and embedded newlines, producing garbage tokens). + it("extracts only the leading command from a terminated heredoc", () => { + const heredoc = "sh << EOF\necho hello\nEOF" + const patterns = extractPatternsFromCommand(heredoc) + expect(patterns).toEqual(["sh"]) + expect(patterns).not.toContain("EOF") + expect(patterns).not.toContain("echo") + }) + + it("extracts only the leading command from an unterminated heredoc", () => { + // An unterminated heredoc is returned as one opaque token by parseCommand. + // The pattern extractor must not split it into components interior to the heredoc + const heredoc = "sh << EOF\necho hello" + const patterns = extractPatternsFromCommand(heredoc) + expect(patterns).toEqual(["sh"]) + expect(patterns).not.toContain("EOF") + expect(patterns).not.toContain("echo") + expect(patterns).not.toContain("hello") + }) + it("does not produce tokens from heredoc body lines", () => { + const heredoc = "sh << EOF\necho body-line\nEOF" + const patterns = extractPatternsFromCommand(heredoc) + // body commands must not surface as auto-approvable patterns + expect(patterns).not.toContain("echo") + expect(patterns).toEqual(["sh"]) + }) + + it("handles a heredoc followed by another command", () => { + // parseCommand splits this into the heredoc token and 'echo done' + const input = "sh << EOF\necho hello\nEOF\necho done" + const patterns = extractPatternsFromCommand(input) + expect(patterns).toContain("sh") + expect(patterns).toContain("echo") + expect(patterns).not.toContain("EOF") + expect(patterns).not.toContain("hello") + }) + }) }) diff --git a/webview-ui/src/utils/command-parser.ts b/webview-ui/src/utils/command-parser.ts index bce464f9ad..0cf0dcb4a6 100644 --- a/webview-ui/src/utils/command-parser.ts +++ b/webview-ui/src/utils/command-parser.ts @@ -1,44 +1,59 @@ import { parse } from "shell-quote" +import { parseCommand } from "@roo/parse-command" /** * Extract command patterns from a command string. * Returns at most 3 levels: base command, command + first argument, and command + first two arguments. * Stops at flags (-), paths (/\~), file extensions (.ext), or special characters (:). + * + * Uses parseCommand (heredoc- and unterminated-quote-aware) to split the input + * into safe sub-commands before tokenizing with shell-quote, so that heredoc + * bodies and multi-line opaque tokens are never fed raw to shell-quote. */ export function extractPatternsFromCommand(command: string): string[] { if (!command?.trim()) return [] const patterns = new Set() + // Split into sub-commands using the heredoc- and quote-aware parser so that + // constructs like heredocs or unterminated quotes are not broken up by + // shell-quote's operator splitting. + const { commands: subCommands } = parseCommand(command) + + for (const subCmd of subCommands) { + extractPatternsFromSingleCommand(subCmd, patterns) + } + + return Array.from(patterns).sort() +} + +/** + * Extract patterns from a single sub-command (no chaining operators) using + * shell-quote for word tokenization. Called only after parseCommand has already + * ensured the input is a safe, single-line sub-command. + */ +function extractPatternsFromSingleCommand(command: string, patterns: Set): void { + if (!command?.trim()) return + + // A sub-command that still contains a newline is a multi-line opaque token + // (heredoc body, unterminated quote). shell-quote cannot safely tokenize it, + // so extract only the leading word and stop. + if (command.includes("\n")) { + const firstWord = command.trim().split(/\s+/)[0] + if (firstWord && !/^\d+$/.test(firstWord)) patterns.add(firstWord) + return + } + try { const parsed = parse(command) - const commandSeparators = new Set(["|", "&&", "||", ";"]) - let currentTokens: string[] = [] - - for (const token of parsed) { - if (typeof token === "object" && "op" in token && commandSeparators.has(token.op)) { - // Process accumulated tokens as a command - if (currentTokens.length > 0) { - extractFromTokens(currentTokens, patterns) - currentTokens = [] - } - } else if (typeof token === "string") { - currentTokens.push(token) - } - } - - // Process any remaining tokens - if (currentTokens.length > 0) { - extractFromTokens(currentTokens, patterns) - } + const tokens = parsed.filter((t): t is string => typeof t === "string") + extractFromTokens(tokens, patterns) } catch (error) { console.warn("Failed to parse command:", error) // Fallback: just extract the first word const firstWord = command.trim().split(/\s+/)[0] if (firstWord) patterns.add(firstWord) } - - return Array.from(patterns).sort() } function extractFromTokens(tokens: string[], patterns: Set): void { From 677d85a9809defc0990ff75764d7a99e2c0370de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Dre=C5=BCewski?= Date: Wed, 17 Jun 2026 22:39:56 +0200 Subject: [PATCH 6/7] fix(chat): cap Virtuoso pre-render to stop OOM on large transcripts (#153) Shrink the chat list pre-render buffer from {top:3000,bottom:1000} to {top:600,bottom:800}, give Virtuoso a defaultItemHeight (180) and a stable computeItemKey so off-screen rows are recycled instead of all mounted at once. On long tasks the old buffer ballooned memory until the webview greyed out. Also add lightweight diagnostics: when the webview is hidden during an active task, ClineProvider logs taskId / messageCount / stackDepth / timestamp to the output channel as a breadcrumb for the grey-screen report. Ported from Zoo-Code PR #153 (ed868c675); diagnostics string rebranded to Tumble Code. Co-authored-by: Toray Altas <6816042+taltas@users.noreply.github.com> Co-authored-by: edelauna <54631123+edelauna@users.noreply.github.com> Co-authored-by: Elliott de Launay --- ...06-17_zoo-153-chat-oom-large-transcript.md | 160 ++++++++++++++++++ src/core/webview/ClineProvider.ts | 19 +++ .../webview/__tests__/ClineProvider.spec.ts | 58 ++++++- webview-ui/src/components/chat/ChatView.tsx | 14 +- .../chat/__tests__/ChatView.spec.tsx | 61 ++++++- 5 files changed, 302 insertions(+), 10 deletions(-) create mode 100644 ai_plans/2026-06-17_zoo-153-chat-oom-large-transcript.md diff --git a/ai_plans/2026-06-17_zoo-153-chat-oom-large-transcript.md b/ai_plans/2026-06-17_zoo-153-chat-oom-large-transcript.md new file mode 100644 index 0000000000..0b1d0b7c7b --- /dev/null +++ b/ai_plans/2026-06-17_zoo-153-chat-oom-large-transcript.md @@ -0,0 +1,160 @@ +# Port plan — Zoo PR #153 → `feature/zoo-153-chat-oom-large-transcript` + +> **For the executor (read first).** Do the steps **in order**. This port lands +> as a verified-clean `git apply` of the upstream diff, **followed by a small +> post-apply rebrand** of two user-facing strings in `ClineProvider.ts`. Do +> **not** improvise or hand-edit anything else. If any `git apply --check` +> reports a conflict, **STOP and report** — do not force it or hand-merge. This +> repo is **Tumble Code**: never leave the strings "Roo" or "Zoo" in user-facing +> text. + +--- + +## 0. Context (read once, write no code) + +- **Upstream:** Zoo PR #153 — "[Fix] Chat window runs out of memory when transcript grows large" (commit `ed868c675`). +- **What it does, one paragraph:** When a chat transcript grows very large, the + Virtuoso virtual list in [ChatView.tsx](webview-ui/src/components/chat/ChatView.tsx) + pre-renders an enormous viewport buffer (`increaseViewportBy={{ top: 3_000, bottom: 1000 }}`), + forcing thousands of pixels of off-screen message rows to mount at once. On long + tasks this balloons memory until the webview crashes / greys out. This PR shrinks + the pre-render buffer to `{ top: 600, bottom: 800 }`, gives Virtuoso a + `defaultItemHeight` (180) so it can estimate scroll extents without mounting + every row, and a stable `computeItemKey` (`${ts}-${index}`) so rows are recycled + instead of re-created. It also adds lightweight diagnostics: when the webview + becomes hidden during an active task, `ClineProvider` logs the task id, message + count, stack depth and a timestamp to the output channel — a breadcrumb for + debugging the "grey screen" report. +- **Why we want it, with evidence in OUR code:** + [ChatView.tsx:1665](webview-ui/src/components/chat/ChatView.tsx#L1665) still + carries the original `increaseViewportBy={{ top: 3_000, bottom: 1000 }}`, with + no `defaultItemHeight` and no `computeItemKey` — so our build over-renders the + same way and is subject to the same OOM on long transcripts. +- **What we deliberately leave out (YAGNI):** nothing structural — all 4 files + apply cleanly. The only adaptation is the **rebrand** in step 4: the upstream + diagnostics string is Zoo-branded (`[Zoo Code]`, `support@zoocode.dev`) and must + be made neutral / Tumble. +- **Original authors — credit them.** (commit author is `roomote[bot]`, dropped; + `T ` deduped into `Toray Altas` — same email). Commit trailers: + + ```text + Co-authored-by: Toray Altas <6816042+taltas@users.noreply.github.com> + Co-authored-by: edelauna <54631123+edelauna@users.noreply.github.com> + Co-authored-by: Elliott de Launay + ``` + +## 1. Preconditions — verify before touching anything + +- [ ] Current branch is `feature/zoo-153-chat-oom-large-transcript`, created off `main`. +- [ ] Working tree clean (`git status --short` empty). +- [ ] The Zoo clone exists at `/home/krzych/Projekty/QUB-IT/Zoo-Code`. +- [ ] These 4 files exist: `webview-ui/src/components/chat/ChatView.tsx`, + `webview-ui/src/components/chat/__tests__/ChatView.spec.tsx`, + `src/core/webview/ClineProvider.ts`, + `src/core/webview/__tests__/ClineProvider.spec.ts`. + +## 2. Regenerate the diff and confirm it applies (no code yet) + +```bash +cd /home/krzych/Projekty/QUB-IT/Roo-Code +git -C /home/krzych/Projekty/QUB-IT/Zoo-Code show ed868c675 > /tmp/zoo-153.diff +git apply --check /tmp/zoo-153.diff && echo "APPLIES CLEANLY" +``` + +- **Expect:** `APPLIES CLEANLY`. If `git apply --check` prints any error, **STOP + and report** — the before-state has drifted and this plan is stale. + +## 3. Apply the diff + +```bash +cd /home/krzych/Projekty/QUB-IT/Roo-Code +git apply /tmp/zoo-153.diff +git status --short +``` + +- **Expect exactly these 4 files changed:** + `webview-ui/src/components/chat/ChatView.tsx`, + `webview-ui/src/components/chat/__tests__/ChatView.spec.tsx`, + `src/core/webview/ClineProvider.ts`, + `src/core/webview/__tests__/ClineProvider.spec.ts`. + +## 4. Rebrand the two Zoo strings (MANDATORY — Tumble Code, no Zoo) + +The applied `logWebviewHiddenDiagnostics` method in +[ClineProvider.ts](src/core/webview/ClineProvider.ts) contains Zoo branding. Make +exactly these two edits in that method, nothing else. + +### Edit 1 — the log prefix + +Replace: + +```ts + this.log( + `[Zoo Code] Webview hidden during active task.\n` + +``` + +With: + +```ts + this.log( + `[Tumble Code] Webview hidden during active task.\n` + +``` + +### Edit 2 — the support line (drop the Zoo support email) + +Replace: + +```ts + ` timestamp: ${new Date().toISOString()}\n` + + `If the panel appears gray after this, share this log with support@zoocode.dev`, +``` + +With: + +```ts + ` timestamp: ${new Date().toISOString()}\n` + + `If the panel appears gray after this, include this log when reporting the issue.`, +``` + +- **Why this is safe:** the spec only asserts `expect.stringContaining("running-task")` + (the taskId) and `not.toHaveBeenCalled()` for the abort/abandoned cases — it + never references the branding or the support sentence. Confirmed in the §2 diff + of `ClineProvider.spec.ts`. + +## 5. Out of scope — do NOT do these + +- Do **not** hand-edit the Virtuoso tuning or "improve" the applied numbers. +- Do **not** re-add or re-wire: **TTS**, the **router / cloud provider**, **cloud + upsell** UI, or **Roo/Zoo branding**. +- Do **not** rename internal ids (those stay `Roo-Code`). +- Do **not** leave `Zoo`, `zoocode`, or `support@zoocode.dev` anywhere. + +## 6. Verify — paste real output, don't claim success without it + +```bash +cd /home/krzych/Projekty/QUB-IT/Roo-Code/src && npx vitest run core/webview/__tests__/ClineProvider.spec.ts +cd /home/krzych/Projekty/QUB-IT/Roo-Code/webview-ui && npx vitest run src/components/chat/__tests__/ChatView.spec.tsx +cd /home/krzych/Projekty/QUB-IT/Roo-Code/src && npx tsc --noEmit -p . 2>&1 | tail -5 # or: pnpm check-types +grep -rIn "Zoo\|zoocode" webview-ui/src/components/chat/ChatView.tsx src/core/webview/ClineProvider.ts || echo "NO ZOO STRINGS" +``` + +## 7. Acceptance criteria (binary — all must hold) + +- [ ] `ClineProvider.spec.ts` (incl. the new `logWebviewHiddenDiagnostics` describe) green. +- [ ] `ChatView.spec.tsx` green. +- [ ] `check-types` clean. +- [ ] `git status` shows exactly the 4 expected files. +- [ ] `grep` confirms **no** `Zoo` / `zoocode` strings remain in the two production files. +- [ ] `ChatView.tsx` shows `increaseViewportBy={CHAT_VIEWPORT_BUFFER}` (not the old `3_000/1000`), + `defaultItemHeight={CHAT_DEFAULT_ITEM_HEIGHT}`, and `computeItemKey={computeMessageKey}`. + +## 8. Record in the ledger + +Already recorded by the orchestrator after the plan file is written. The commit +(done by the orchestrator) will carry: + +```text +Co-authored-by: Toray Altas <6816042+taltas@users.noreply.github.com> +Co-authored-by: edelauna <54631123+edelauna@users.noreply.github.com> +Co-authored-by: Elliott de Launay +``` diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index d83990bafd..52dbb3e73a 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -913,6 +913,8 @@ export class ClineProvider const viewStateDisposable = webviewView.onDidChangeViewState(() => { if (this.view?.visible) { this.postMessageToWebview({ type: "action", action: "didBecomeVisible" }) + } else { + this.logWebviewHiddenDiagnostics() } }) @@ -922,6 +924,8 @@ export class ClineProvider const visibilityDisposable = webviewView.onDidChangeVisibility(() => { if (this.view?.visible) { this.postMessageToWebview({ type: "action", action: "didBecomeVisible" }) + } else { + this.logWebviewHiddenDiagnostics() } }) @@ -2892,6 +2896,21 @@ export class ClineProvider return this.clineStack[this.clineStack.length - 1] } + private logWebviewHiddenDiagnostics(): void { + const task = this.getCurrentTask() + if (!task || task.abort || task.abandoned) { + return + } + this.log( + `[Tumble Code] Webview hidden during active task.\n` + + ` taskId: ${task.taskId}\n` + + ` messageCount: ${task.clineMessages.length}\n` + + ` stackDepth: ${this.clineStack.length}\n` + + ` timestamp: ${new Date().toISOString()}\n` + + `If the panel appears gray after this, include this log when reporting the issue.`, + ) + } + public getRecentTasks(): string[] { if (this.recentTasksCache) { return this.recentTasksCache diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 47c3203065..40a4d80884 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -353,7 +353,7 @@ describe("ClineProvider", () => { let provider: ClineProvider let mockContext: vscode.ExtensionContext let mockOutputChannel: vscode.OutputChannel - let mockWebviewView: vscode.WebviewView + let mockWebviewView: any let mockPostMessage: any let updateGlobalStateSpy: any @@ -432,7 +432,7 @@ describe("ClineProvider", () => { return { dispose: vi.fn() } }), onDidChangeVisibility: vi.fn().mockImplementation(() => ({ dispose: vi.fn() })), - } as unknown as vscode.WebviewView + } provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) @@ -523,6 +523,48 @@ describe("ClineProvider", () => { expect(mockWebviewView.webview.html).toContain("") }) + describe("logWebviewHiddenDiagnostics", () => { + let visibilityCallback: () => void + + beforeEach(async () => { + // Capture the visibility callback registered during resolveWebviewView + mockWebviewView.onDidChangeVisibility = vi.fn().mockImplementation((cb: () => void) => { + visibilityCallback = cb + return { dispose: vi.fn() } + }) + // @ts-ignore - accessing private property for testing + provider.view = mockWebviewView + await provider.resolveWebviewView(mockWebviewView) + ;(mockOutputChannel.appendLine as ReturnType).mockClear() + }) + + test("does not log when no task is active", () => { + // view becomes hidden with no task on the stack + Object.defineProperty(mockWebviewView, "visible", { value: false, configurable: true }) + visibilityCallback() + expect(mockOutputChannel.appendLine).not.toHaveBeenCalled() + }) + + test("does not log when the active task is aborted", async () => { + const task = new Task(defaultTaskOptions) + Object.defineProperty(task, "taskId", { value: "aborted-task", writable: true }) + task.abort = true + await provider.addClineToStack(task) + Object.defineProperty(mockWebviewView, "visible", { value: false, configurable: true }) + visibilityCallback() + expect(mockOutputChannel.appendLine).not.toHaveBeenCalled() + }) + + test("logs task state to output channel when an active task is running", async () => { + const task = new Task(defaultTaskOptions) + Object.defineProperty(task, "taskId", { value: "running-task", writable: true }) + await provider.addClineToStack(task) + Object.defineProperty(mockWebviewView, "visible", { value: false, configurable: true }) + visibilityCallback() + expect(mockOutputChannel.appendLine).toHaveBeenCalledWith(expect.stringContaining("running-task")) + }) + }) + test("resolveWebviewView sets up webview correctly in development mode even if local server is not running", async () => { provider = new ClineProvider( { ...mockContext, extensionMode: vscode.ExtensionMode.Development }, @@ -2043,7 +2085,7 @@ describe("Project MCP Settings", () => { let provider: ClineProvider let mockContext: vscode.ExtensionContext let mockOutputChannel: vscode.OutputChannel - let mockWebviewView: vscode.WebviewView + let mockWebviewView: any let mockPostMessage: any beforeEach(async () => { @@ -2097,7 +2139,7 @@ describe("Project MCP Settings", () => { visible: true, onDidDispose: vi.fn(), onDidChangeVisibility: vi.fn(), - } as unknown as vscode.WebviewView + } ;(vscode.window as any).activeTextEditor = undefined ;(vscode.workspace.getWorkspaceFolder as any).mockReset() provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) @@ -2360,7 +2402,7 @@ describe("ClineProvider - Router Models", () => { let provider: ClineProvider let mockContext: vscode.ExtensionContext let mockOutputChannel: vscode.OutputChannel - let mockWebviewView: vscode.WebviewView + let mockWebviewView: any let mockPostMessage: any beforeEach(() => { @@ -2419,7 +2461,7 @@ describe("ClineProvider - Router Models", () => { return { dispose: vi.fn() } }), onDidChangeVisibility: vi.fn().mockImplementation(() => ({ dispose: vi.fn() })), - } as unknown as vscode.WebviewView + } if (!TelemetryService.hasInstance()) { TelemetryService.createInstance([]) @@ -2664,7 +2706,7 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { let provider: ClineProvider let mockContext: vscode.ExtensionContext let mockOutputChannel: vscode.OutputChannel - let mockWebviewView: vscode.WebviewView + let mockWebviewView: any let mockPostMessage: any let defaultTaskOptions: TaskOptions @@ -2733,7 +2775,7 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { return { dispose: vi.fn() } }), onDidChangeVisibility: vi.fn().mockImplementation(() => ({ dispose: vi.fn() })), - } as unknown as vscode.WebviewView + } provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) diff --git a/webview-ui/src/components/chat/ChatView.tsx b/webview-ui/src/components/chat/ChatView.tsx index ec542a060e..bef3fb2754 100644 --- a/webview-ui/src/components/chat/ChatView.tsx +++ b/webview-ui/src/components/chat/ChatView.tsx @@ -61,6 +61,11 @@ export interface ChatViewRef { } export const MAX_IMAGES_PER_MESSAGE = 20 // This is the Anthropic limit. +const CHAT_DEFAULT_ITEM_HEIGHT = 180 +const CHAT_VIEWPORT_BUFFER = { + top: 600, + bottom: 800, +} as const const isMac = navigator.platform.toUpperCase().indexOf("MAC") >= 0 @@ -1459,6 +1464,11 @@ const ChatViewComponent: React.ForwardRefRenderFunction `${messageOrGroup.ts}-${index}`, + [], + ) + // Function to handle mode switching const switchToNextMode = useCallback(() => { const allModes = getAllModes(customModes) @@ -1635,7 +1645,9 @@ const ChatViewComponent: React.ForwardRefRenderFunction ({ + lastConfig: null as { + computeItemKey?: (index: number, item: ClineMessage) => React.Key + defaultItemHeight?: number + increaseViewportBy?: number | { top?: number; bottom?: number } + } | null, +})) + // Define minimal types needed for testing interface ClineMessage { type: "say" | "ask" @@ -61,14 +69,26 @@ vi.mock("react-virtuoso", () => ({ Virtuoso: function MockVirtuoso({ data, itemContent, + computeItemKey, + defaultItemHeight, + increaseViewportBy, }: { data: ClineMessage[] itemContent: (index: number, item: ClineMessage) => React.ReactNode + computeItemKey?: (index: number, item: ClineMessage) => React.Key + defaultItemHeight?: number + increaseViewportBy?: number | { top?: number; bottom?: number } }) { + mockVirtuosoState.lastConfig = { + computeItemKey, + defaultItemHeight, + increaseViewportBy, + } + return (
{data.map((item, index) => ( -
+
{itemContent(index, item)}
))} @@ -461,6 +481,45 @@ describe("ChatView - Sound Playing Tests", () => { }) }) +describe("ChatView - Virtualization Configuration", () => { + beforeEach(() => { + vi.clearAllMocks() + mockVirtuosoState.lastConfig = null + }) + + it("keeps the off-screen render buffer tight for chat rows", async () => { + renderChatView() + + const taskTs = Date.now() - 100 + const rowTs = Date.now() + + mockPostMessage({ + clineMessages: [ + { + type: "say", + say: "task", + ts: taskTs, + text: "Initial task", + }, + { + type: "say", + say: "text", + ts: rowTs, + text: "Visible row", + }, + ], + }) + + await waitFor(() => { + expect(mockVirtuosoState.lastConfig).not.toBeNull() + }) + + expect(mockVirtuosoState.lastConfig?.defaultItemHeight).toBe(180) + expect(mockVirtuosoState.lastConfig?.increaseViewportBy).toEqual({ top: 600, bottom: 800 }) + expect(mockVirtuosoState.lastConfig?.computeItemKey?.(1, { type: "say", ts: rowTs })).toBe(`${rowTs}-1`) + }) +}) + describe("ChatView - Focus Grabbing Tests", () => { beforeEach(() => vi.clearAllMocks()) From 0b3f4d678524097f24d9500c4c14cf69bb235185 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Dre=C5=BCewski?= Date: Wed, 17 Jun 2026 22:42:58 +0200 Subject: [PATCH 7/7] fix(rules): resolve relative symlinks via realpath of parent dir (#442) When a .roo/rules directory is itself a symlink to an external directory and a file inside is a relative symlink (e.g. ../1-project.txt), resolveSymLink resolved the target against the access path instead of the symlink's real location, pointing at the wrong file. Call fs.realpath() on the symlink's parent directory before resolving the relative target, matching how the OS resolves relative symlinks; fall back to the raw dir if realpath fails. Ported from Zoo-Code PR #442 (4c91a8f24). Co-authored-by: Povilas Kanapickas --- ...-442-resolve-relative-symlinks-in-rules.md | 99 ++++++++++ .../__tests__/custom-instructions.spec.ts | 169 ++++++++++++++++++ .../prompts/sections/custom-instructions.ts | 15 +- 3 files changed, 281 insertions(+), 2 deletions(-) create mode 100644 ai_plans/2026-06-17_zoo-442-resolve-relative-symlinks-in-rules.md diff --git a/ai_plans/2026-06-17_zoo-442-resolve-relative-symlinks-in-rules.md b/ai_plans/2026-06-17_zoo-442-resolve-relative-symlinks-in-rules.md new file mode 100644 index 0000000000..9424292898 --- /dev/null +++ b/ai_plans/2026-06-17_zoo-442-resolve-relative-symlinks-in-rules.md @@ -0,0 +1,99 @@ +# Port plan — Zoo PR #442 → `feature/zoo-442-resolve-relative-symlinks-in-rules` + +> **For the executor (read first).** Do the steps **in order**. This port lands +> as a verified-clean `git apply` of the upstream diff — **no hand-edits, no +> rebrand needed** (the diff carries no user-facing strings). If any +> `git apply --check` reports a conflict, **STOP and report** — do not force it. +> This repo is **Tumble Code**: never introduce the strings "Roo" or "Zoo" in +> user-facing text (the `.roo` directory is an internal id and stays as-is). + +--- + +## 0. Context (read once, write no code) + +- **Upstream:** Zoo PR #442 — "fix: resolve relative symlinks in rules files using realpath of parent directory" (commit `4c91a8f24`). +- **What it does, one paragraph:** `resolveSymLink` in + [custom-instructions.ts](src/core/prompts/sections/custom-instructions.ts) + resolves a rules-file symlink's target with + `path.resolve(path.dirname(symlinkPath), linkTarget)`. When the `.roo/rules` + directory is itself a symlink to an external directory and a file inside is a + **relative** symlink (e.g. `../1-project.txt`), the OS resolves that relative + target against the symlink's **real** location, but our code resolves it against + the **access path** — pointing at the wrong file (or nothing). The fix calls + `fs.realpath()` on the symlink's parent directory first, then resolves the + relative target against that real path, matching OS semantics. `fs.realpath` + failures fall back to the non-realpath'd dir, so non-symlinked setups are + unaffected. +- **Why we want it, with evidence in OUR code:** + [custom-instructions.ts:84-88](src/core/prompts/sections/custom-instructions.ts#L84-L88) + in our tree still does the old `path.resolve(path.dirname(symlinkPath), linkTarget)` + with no realpath step, so the bug reproduces here for symlinked `.roo/rules` + dirs with relative inner symlinks. +- **What we deliberately leave out (YAGNI):** nothing — the diff is exactly the + fix plus its test; both files exist and apply cleanly. +- **Original author — credit.** Povilas Kanapickas. Commit trailer: + + ```text + Co-authored-by: Povilas Kanapickas + ``` + +## 1. Preconditions — verify before touching anything + +- [ ] Current branch is `feature/zoo-442-resolve-relative-symlinks-in-rules`, created off `main`. +- [ ] Working tree clean (`git status --short` empty). +- [ ] These files exist: `src/core/prompts/sections/custom-instructions.ts`, + `src/core/prompts/sections/__tests__/custom-instructions.spec.ts`. + +## 2. Regenerate the diff and confirm it applies (no code yet) + +```bash +cd /home/krzych/Projekty/QUB-IT/Roo-Code +git -C /home/krzych/Projekty/QUB-IT/Zoo-Code show 4c91a8f24 > /tmp/zoo-442.diff +git apply --check /tmp/zoo-442.diff && echo "APPLIES CLEANLY" +``` + +- **Expect:** `APPLIES CLEANLY`. If it prints any error, **STOP and report** — the + before-state has drifted and this plan is stale. + +## 3. Apply the diff + +```bash +cd /home/krzych/Projekty/QUB-IT/Roo-Code +git apply /tmp/zoo-442.diff +git status --short +``` + +- **Expect exactly these 2 files changed:** + `src/core/prompts/sections/custom-instructions.ts`, + `src/core/prompts/sections/__tests__/custom-instructions.spec.ts`. + +## 4. Out of scope — do NOT do these + +- Do **not** hand-edit the fix or "improve" the realpath logic. +- Do **not** rename `.roo` → `.tumble` or touch any internal id (those stay). +- Do **not** re-add or re-wire: **TTS**, the **router / cloud provider**, **cloud + upsell** UI, or **Roo/Zoo branding**. + +## 5. Verify — paste real output, don't claim success without it + +```bash +cd /home/krzych/Projekty/QUB-IT/Roo-Code/src && npx vitest run core/prompts/sections/__tests__/custom-instructions.spec.ts +cd /home/krzych/Projekty/QUB-IT/Roo-Code/src && pnpm check-types +``` + +## 6. Acceptance criteria (binary — all must hold) + +- [ ] `custom-instructions.spec.ts` (incl. the new realpath/symlink describe) green. +- [ ] `check-types` clean. +- [ ] `git status` shows exactly the 2 expected files. +- [ ] `resolveSymLink` now calls `fs.realpath(symlinkDir)` (with try/catch fallback) + before `path.resolve(realSymlinkDir, linkTarget)`. + +## 7. Record in the ledger + +Already recorded by the orchestrator after the plan file is written. The commit +(done by the orchestrator) will carry: + +```text +Co-authored-by: Povilas Kanapickas +``` diff --git a/src/core/prompts/sections/__tests__/custom-instructions.spec.ts b/src/core/prompts/sections/__tests__/custom-instructions.spec.ts index 68fa2d37f5..20ad8d17ab 100644 --- a/src/core/prompts/sections/__tests__/custom-instructions.spec.ts +++ b/src/core/prompts/sections/__tests__/custom-instructions.spec.ts @@ -3,6 +3,20 @@ // Mock fs/promises vi.mock("fs/promises") +// Mock os.homedir to make global directory path predictable across environments +vi.mock("os", async () => ({ + ...(await vi.importActual("os")), + homedir: vi.fn().mockReturnValue("/mock/home"), +})) + +// Mock roo-config to avoid environment-dependent global directory paths in tests +vi.mock("../../../../services/roo-config", () => ({ + getRooDirectoriesForCwd: vi.fn().mockImplementation((cwd: string) => [`${cwd}/.roo`]), + getAllRooDirectoriesForCwd: vi.fn().mockImplementation((cwd: string) => Promise.resolve([`${cwd}/.roo`])), + getAgentsDirectoriesForCwd: vi.fn().mockImplementation((cwd: string) => Promise.resolve([cwd])), + getGlobalRooDirectory: vi.fn().mockReturnValue("/mock/home/.roo"), +})) + // Mock path.resolve and path.join to be predictable in tests vi.mock("path", async () => ({ ...(await vi.importActual("path")), @@ -64,6 +78,7 @@ const statMock = vi.fn() const readdirMock = vi.fn() const readlinkMock = vi.fn() const lstatMock = vi.fn() +const realpathMock = vi.fn().mockImplementation((p: string) => Promise.resolve(p)) // Replace fs functions with our mocks fs.readFile = readFileMock as any @@ -71,6 +86,7 @@ fs.stat = statMock as any fs.readdir = readdirMock as any fs.readlink = readlinkMock as any fs.lstat = lstatMock as any +fs.realpath = realpathMock as any // Mock process.cwd const originalCwd = process.cwd @@ -1573,6 +1589,159 @@ describe("Rules directory reading", () => { expect(result).toContain("mmm-middle.txt") }) + it.skipIf(process.platform === "win32")( + "should resolve relative symlinks using realpath of parent directory", + async () => { + // This tests the scenario where: + // /project/.roo/rules -> /external/rules (symlinked directory) + // /external/rules/1-project.txt -> ../1-project.txt (relative symlink) + // The relative symlink should resolve to /external/1-project.txt, NOT /project/.roo/1-project.txt + + // Simulate .roo/rules directory exists + statMock.mockResolvedValueOnce({ + isDirectory: vi.fn().mockReturnValue(true), + } as any) + + // Simulate listing files - one relative symlink inside the symlinked directory + readdirMock.mockResolvedValueOnce([ + { + name: "1-project.txt", + isFile: () => false, + isSymbolicLink: () => true, + parentPath: "/project/.roo/rules", + }, + ] as any) + + // readlink returns a relative target + readlinkMock.mockResolvedValueOnce("../1-project.txt") + + // realpath resolves the symlinked parent directory to its real location + realpathMock.mockImplementation((dirPath: string) => { + const normalizedPath = dirPath.toString().replace(/\\/g, "/") + if (normalizedPath === "/project/.roo/rules") { + return Promise.resolve("/external/rules") + } + return Promise.resolve(dirPath) + }) + + // stat mock for the resolved target + statMock.mockImplementation((filePath: string) => { + const normalizedPath = filePath.toString().replace(/\\/g, "/") + if ( + normalizedPath === "/external/rules/../1-project.txt" || + normalizedPath === "/external/1-project.txt" + ) { + return Promise.resolve({ + isFile: vi.fn().mockReturnValue(true), + isDirectory: vi.fn().mockReturnValue(false), + } as any) + } + return Promise.resolve({ + isFile: vi.fn().mockReturnValue(false), + isDirectory: vi.fn().mockReturnValue(false), + } as any) + }) + + readFileMock.mockImplementation((filePath: string) => { + const normalizedPath = filePath.toString().replace(/\\/g, "/") + if ( + normalizedPath === "/external/rules/../1-project.txt" || + normalizedPath === "/external/1-project.txt" + ) { + return Promise.resolve("content from external file") + } + return Promise.reject({ code: "ENOENT" }) + }) + + const result = await loadRuleFiles("/project") + + // Verify realpath was called on the symlink's parent directory + expect(realpathMock).toHaveBeenCalledWith("/project/.roo/rules") + + // Verify the content was read from the correctly resolved path + // (resolved relative to /external/rules, not /project/.roo/rules) + expect(result).toContain("content from external file") + + // Verify readlink was called + expect(readlinkMock).toHaveBeenCalledWith("/project/.roo/rules/1-project.txt") + + const statCalls = statMock.mock.calls.map((call: any[]) => call[0].toString().replace(/\\/g, "/")) + expect(statCalls).toEqual(["/project/.roo/rules", "/external/1-project.txt", "/external/1-project.txt"]) + }, + ) + + it.skipIf(process.platform === "win32")("should fall back to symlink path when realpath fails", async () => { + // Simulate .roo/rules directory exists + statMock.mockResolvedValueOnce({ + isDirectory: vi.fn().mockReturnValue(true), + } as any) + + // Simulate listing files - one relative symlink inside the directory + readdirMock.mockResolvedValueOnce([ + { + name: "1-project.txt", + isFile: () => false, + isSymbolicLink: () => true, + parentPath: "/project/.roo/rules", + }, + ] as any) + + // readlink returns a relative target + readlinkMock.mockResolvedValueOnce("../1-project.txt") + + // realpath fails for the parent directory + realpathMock.mockImplementation((dirPath: string) => { + const normalizedPath = dirPath.toString().replace(/\\/g, "/") + if (normalizedPath === "/project/.roo/rules") { + return Promise.reject({ code: "ENOENT" }) + } + return Promise.resolve(dirPath) + }) + + // stat mock for the resolved target (using fallback path) + statMock.mockImplementation((filePath: string) => { + const normalizedPath = filePath.toString().replace(/\\/g, "/") + if ( + normalizedPath === "/project/.roo/rules/../1-project.txt" || + normalizedPath === "/project/.roo/1-project.txt" + ) { + return Promise.resolve({ + isFile: vi.fn().mockReturnValue(true), + isDirectory: vi.fn().mockReturnValue(false), + } as any) + } + return Promise.resolve({ + isFile: vi.fn().mockReturnValue(false), + isDirectory: vi.fn().mockReturnValue(false), + } as any) + }) + + readFileMock.mockImplementation((filePath: string) => { + const normalizedPath = filePath.toString().replace(/\\/g, "/") + if ( + normalizedPath === "/project/.roo/rules/../1-project.txt" || + normalizedPath === "/project/.roo/1-project.txt" + ) { + return Promise.resolve("content from fallback path") + } + return Promise.reject({ code: "ENOENT" }) + }) + + const result = await loadRuleFiles("/project") + + // Verify realpath was called on the symlink's parent directory + expect(realpathMock).toHaveBeenCalledWith("/project/.roo/rules") + + // Verify the content was read from the fallback path + expect(result).toContain("content from fallback path") + + // Verify readlink was called + expect(readlinkMock).toHaveBeenCalledWith("/project/.roo/rules/1-project.txt") + + const statCalls = statMock.mock.calls.map((call: any[]) => call[0].toString().replace(/\\/g, "/")) + expect(statCalls).toEqual(["/project/.roo/rules", "/project/.roo/1-project.txt", "/project/.roo/1-project.txt"]) + }) + it("should handle empty file list gracefully", async () => { // Simulate .roo/rules directory exists statMock.mockResolvedValueOnce({ diff --git a/src/core/prompts/sections/custom-instructions.ts b/src/core/prompts/sections/custom-instructions.ts index 46cf1bf1f9..0d72e568b8 100644 --- a/src/core/prompts/sections/custom-instructions.ts +++ b/src/core/prompts/sections/custom-instructions.ts @@ -84,8 +84,19 @@ async function resolveSymLink( try { // Get the symlink target const linkTarget = await fs.readlink(symlinkPath) - // Resolve the target path (relative to the symlink location) - const resolvedTarget = path.resolve(path.dirname(symlinkPath), linkTarget) + // Resolve the target path relative to the symlink's real parent directory. + // We must use realpath on the parent directory because the symlink itself + // may be inside a symlinked directory. Relative symlink targets are resolved + // by the filesystem relative to the symlink's actual location, not the path + // used to access it. + const symlinkDir = path.dirname(symlinkPath) + let realSymlinkDir: string + try { + realSymlinkDir = await fs.realpath(symlinkDir) + } catch { + realSymlinkDir = symlinkDir + } + const resolvedTarget = path.resolve(realSymlinkDir, linkTarget) // Check if the target is a file const stats = await fs.stat(resolvedTarget)