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 000000000..0b1d0b7c7 --- /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/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 000000000..db5e14ef4 --- /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/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 000000000..942429289 --- /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/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 000000000..ec3889889 --- /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/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 000000000..6594ac759 --- /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/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 000000000..dc396d58c --- /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/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 000000000..8dc376793 --- /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/knip.json b/knip.json index 67c9e9a74..a3382bd2c 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/providers/openai.ts b/packages/types/src/providers/openai.ts index 81a022c66..4f04af0a6 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/packages/types/src/terminal.ts b/packages/types/src/terminal.ts index 34f7a74e2..3a32866cd 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/packages/types/src/vscode.ts b/packages/types/src/vscode.ts index fd28f2e99..ca6232dce 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 f13bf8314..1111211a9 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 9f4ac34c4..432f0d1ed 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/api/providers/__tests__/base-openai-compatible-provider.spec.ts b/src/api/providers/__tests__/base-openai-compatible-provider.spec.ts index a7b4cb3c5..c1753bc45 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 9f3a641cb..df0e8b152 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/__tests__/openai-native.spec.ts b/src/api/providers/__tests__/openai-native.spec.ts index 6887da4d2..e37696b0f 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({ diff --git a/src/api/providers/base-openai-compatible-provider.ts b/src/api/providers/base-openai-compatible-provider.ts index fc3d769ae..9ae605f50 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 cf8d16a11..0b79433f3 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 000000000..71edad4df --- /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 000000000..698f68c86 --- /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 +} diff --git a/src/core/auto-approval/__tests__/commands.spec.ts b/src/core/auto-approval/__tests__/commands.spec.ts index 90d2808ba..fa5762cb5 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 d9e88c7ba..82b937e66 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/prompts/sections/__tests__/custom-instructions.spec.ts b/src/core/prompts/sections/__tests__/custom-instructions.spec.ts index 68fa2d37f..20ad8d17a 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 46cf1bf1f..0d72e568b 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) diff --git a/src/core/tools/ExecuteCommandTool.ts b/src/core/tools/ExecuteCommandTool.ts index cd80a6c16..ba838e32d 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/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index d83990baf..52dbb3e73 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 47c320306..40a4d8088 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/src/esbuild.mjs b/src/esbuild.mjs index 6089ac330..8a23090a0 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 bbd268566..2e62d030e 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 b8f688dd8..fac4d22f8 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 2f6157399..cad422173 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 9efe9366c..dafde8388 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 8237a851f..75ade57b3 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 e04b117ba..eee792cfc 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 397dfe8ee..011e8f0e4 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 d4cac6bb2..a40aaefeb 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 283660c50..904693d2c 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 2346e9a03..add7a8411 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 30fafeee3..c699c7109 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 c1a1c394a..3052213b8 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 3eb150117..20abd0f65 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 dcd59910b..c538bfbc1 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 6089f81d4..369b2c55b 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 e68886f3b..6d46434a1 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 d8c2f62c2..286908a2a 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 8218c5b0a..66fcec602 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 c52ce333d..6d31ebe62 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/glob/__tests__/list-files-limit.spec.ts b/src/services/glob/__tests__/list-files-limit.spec.ts index 8eaf73c18..6f0ad8518 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 3207f692b..71a35ca41 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 5366bbb84..6374c7c48 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) diff --git a/src/services/ripgrep/__tests__/diagnostic.spec.ts b/src/services/ripgrep/__tests__/diagnostic.spec.ts new file mode 100644 index 000000000..b41905259 --- /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 000000000..e74800fac --- /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 59cdb03cf..99d91faad 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 000000000..b1e0d6fcc --- /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), + } + } +} diff --git a/src/shared/__tests__/parse-command.spec.ts b/src/shared/__tests__/parse-command.spec.ts new file mode 100644 index 000000000..fa41d31c6 --- /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 a8d87b76a..6b2fb4d26 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/ChatView.tsx b/webview-ui/src/components/chat/ChatView.tsx index ec542a060..bef3fb275 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(() => { // 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__/ChatView.spec.tsx b/webview-ui/src/components/chat/__tests__/ChatView.spec.tsx index 63e71c9bd..892884857 100644 --- a/webview-ui/src/components/chat/__tests__/ChatView.spec.tsx +++ b/webview-ui/src/components/chat/__tests__/ChatView.spec.tsx @@ -9,6 +9,14 @@ import { vscode } from "@src/utils/vscode" import ChatView, { ChatViewProps } from "../ChatView" +const mockVirtuosoState = vi.hoisted(() => ({ + 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()) diff --git a/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx b/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx index f40987d26..2d8b29b1d 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 f34361b3f..2f55688a1 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 ef7be52db..8c071b08c 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 dce6555dd..8f5caab5c 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 74e569016..f258c0e5e 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 6a71eb6f4..f3e56c2a7 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 590339915..12081aba7 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 94ae38d7e..6f6992410 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 bd0020fe3..4de9064e1 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 b96e2f40c..6d2b19b14 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 d8d3a9349..99fedad60 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 1ed0db402..20a95e9c8 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 757013ae9..6088658e5 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 7ab75a269..b0fd630c0 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 1717872ee..86ee9ede8 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 b73860558..efaed4f21 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 5d34595f3..8fa939c70 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 0a57b725e..28a74df77 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 f35bb84a2..927689f54 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 ede8ebe87..0316e2b32 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 bce464f9a..0cf0dcb4a 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 {