From d4b6e869cb66fc423dae272eaa6cb8861451ed54 Mon Sep 17 00:00:00 2001 From: can1357 Date: Sun, 22 Mar 2026 12:38:33 +0100 Subject: [PATCH] feat(coding-agent): implemented pattern-based bash interceptor rules + exclusive range semantics - Changed bash interceptor configuration from boolean flags to customizable pattern-based rules array. - Clarified hashline range replace semantics: end parameter is now strictly exclusive boundary. - Fixed bash interceptor to apply built-in default rules when no custom patterns are configured. - Updated hashline range validation and calculations to enforce exclusive end semantics throughout. --- packages/coding-agent/CHANGELOG.md | 7 +- .../src/config/settings-schema.ts | 48 +++++++--- packages/coding-agent/src/config/settings.ts | 5 +- packages/coding-agent/src/patch/hashline.ts | 40 ++++++--- .../src/prompts/tools/hashline.md | 54 +++++------- .../src/tools/bash-interceptor.ts | 40 +-------- .../coding-agent/test/core/hashline.test.ts | 75 ++++++++++++---- .../session-selector-delete.test.ts | 20 ++--- packages/coding-agent/test/tools.test.ts | 87 ++++++++++++++++++- 9 files changed, 243 insertions(+), 133 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 955fe5505..704a1efde 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -1,7 +1,6 @@ # Changelog ## [Unreleased] - ### Added - Added ACP (Agent Client Protocol) mode for headless agent operation via `--mode acp` @@ -10,11 +9,17 @@ ### Changed +- Updated bash interceptor configuration to use customizable pattern rules instead of individual boolean flags +- Clarified hashline range replace semantics: `end` parameter is now strictly exclusive (the line it points to survives and is not consumed) - Updated ask tool rendering to support markdown formatting in questions and option labels - Refactored hook input and selector components to render titles as markdown for richer text formatting - Changed session collection to include sessions with zero messages, enabling ACP mode to create discoverable sessions immediately - Changed session persistence logic to use atomic file rewrite when flushing unflushed sessions to prevent duplication +### Fixed + +- Fixed bash interceptor to apply built-in default rules when no custom patterns are configured + ## [13.14.0] - 2026-03-20 ### Added diff --git a/packages/coding-agent/src/config/settings-schema.ts b/packages/coding-agent/src/config/settings-schema.ts index 09ceb4274..260855051 100644 --- a/packages/coding-agent/src/config/settings-schema.ts +++ b/packages/coding-agent/src/config/settings-schema.ts @@ -139,6 +139,43 @@ type SettingDef = // under `as const` while still letting SettingValue infer the correct element type. const EMPTY_STRING_ARRAY: string[] = []; const EMPTY_STRING_RECORD: Record = {}; +export const DEFAULT_BASH_INTERCEPTOR_RULES: BashInterceptorRule[] = [ + { + pattern: "^\\s*(cat|head|tail|less|more)\\s+", + tool: "read", + message: "Use the `read` tool instead of cat/head/tail. It provides better context and handles binary files.", + }, + { + pattern: "^\\s*(grep|rg|ripgrep|ag|ack)\\s+", + tool: "grep", + message: "Use the `grep` tool instead of grep/rg. It respects .gitignore and provides structured output.", + }, + { + pattern: "^\\s*(find|fd|locate)\\s+.*(-name|-iname|-type|--type|-glob)", + tool: "find", + message: "Use the `find` tool instead of find/fd. It respects .gitignore and is faster for glob patterns.", + }, + { + pattern: "^\\s*sed\\s+(-i|--in-place)", + tool: "edit", + message: "Use the `edit` tool instead of sed -i. It provides diff preview and fuzzy matching.", + }, + { + pattern: "^\\s*perl\\s+.*-[pn]?i", + tool: "edit", + message: "Use the `edit` tool instead of perl -i. It provides diff preview and fuzzy matching.", + }, + { + pattern: "^\\s*awk\\s+.*-i\\s+inplace", + tool: "edit", + message: "Use the `edit` tool instead of awk -i inplace. It provides diff preview and fuzzy matching.", + }, + { + pattern: "^\\s*(echo|printf|cat\\s*<<)\\s+.*[^|]>\\s*\\S", + tool: "write", + message: "Use the `write` tool instead of echo/cat redirection. It handles encoding and provides confirmation.", + }, +]; export const SETTINGS_SCHEMA = { // ──────────────────────────────────────────────────────────────────────── @@ -943,16 +980,7 @@ export const SETTINGS_SCHEMA = { default: false, ui: { tab: "editing", label: "Bash Interceptor", description: "Block shell commands that have dedicated tools" }, }, - - "bashInterceptor.simpleLs": { - type: "boolean", - default: true, - ui: { - tab: "editing", - label: "Intercept `ls`", - description: "Intercept bare ls commands (when interceptor is enabled)", - }, - }, + "bashInterceptor.patterns": { type: "array", default: DEFAULT_BASH_INTERCEPTOR_RULES }, // Python "python.toolMode": { diff --git a/packages/coding-agent/src/config/settings.ts b/packages/coding-agent/src/config/settings.ts index 1ecb255c3..3740ecc23 100644 --- a/packages/coding-agent/src/config/settings.ts +++ b/packages/coding-agent/src/config/settings.ts @@ -341,10 +341,7 @@ export class Settings { * Get bash interceptor rules (typed accessor for complex array config). */ getBashInterceptorRules(): BashInterceptorRule[] { - const patterns = (this.#merged.bashInterceptor as { patterns?: unknown[] })?.patterns; - if (!Array.isArray(patterns)) return []; - - return patterns.filter((p): p is BashInterceptorRule => typeof p === "object" && p !== null && "pattern" in p); + return this.get("bashInterceptor.patterns"); } /** diff --git a/packages/coding-agent/src/patch/hashline.ts b/packages/coding-agent/src/patch/hashline.ts index 8bd67ddfe..493770dbc 100644 --- a/packages/coding-agent/src/patch/hashline.ts +++ b/packages/coding-agent/src/patch/hashline.ts @@ -15,6 +15,13 @@ import type { HashMismatch } from "./types"; export type Anchor = { line: number; hash: string }; +/** + * Edit operation on hashline-addressed content. + * + * For range replace: `pos` is inclusive (first consumed line), + * `end` is **exclusive** (first surviving line after the range). + * The consumed range is `[pos.line, end.line - 1]`. + */ export type HashlineEdit = | { op: "replace"; pos: Anchor; end?: Anchor; lines: string[] } | { op: "append"; pos?: Anchor; lines: string[] } @@ -470,9 +477,8 @@ function shouldAutocorrect(line: string, otherLine: string): boolean { /** * Apply an array of hashline edits to file content. * - * Each edit operation identifies target lines directly (`replace`, - * `append`, `prepend`). Line references are resolved via {@link parseTag} - * and hashes validated before any mutation. + * For range replace, `end` is **exclusive**: the consumed range is + * `[pos.line, end.line - 1]` and the line at `end` survives. * * Edits are sorted bottom-up (highest effective line first) so earlier * splices don't invalidate later line numbers. @@ -518,8 +524,10 @@ export function applyHashlineEdits( const startValid = validateRef(edit.pos); const endValid = validateRef(edit.end); if (!startValid || !endValid) continue; - if (edit.pos.line > edit.end.line) { - throw new Error(`Range start line ${edit.pos.line} must be <= end line ${edit.end.line}`); + if (edit.pos.line >= edit.end.line) { + throw new Error( + `Range start line ${edit.pos.line} must be < end line ${edit.end.line} (end is exclusive)`, + ); } } else { if (!validateRef(edit.pos)) continue; @@ -597,10 +605,13 @@ export function applyHashlineEdits( case "replace": if (!edit.end) { sortLine = edit.pos.line; + precedence = 0; } else { sortLine = edit.end.line; + // Range replaces must run after edits anchored on the surviving end line, + // so those line-number references still point at the same survivor. + precedence = 3; } - precedence = 0; break; case "append": sortLine = edit.pos ? edit.pos.line : fileLines.length + 1; @@ -634,19 +645,22 @@ export function applyHashlineEdits( fileLines.splice(edit.pos.line - 1, 1, ...newLines); trackFirstChanged(edit.pos.line); } else { - const count = edit.end.line - edit.pos.line + 1; + // end is exclusive: consumed range is [pos.line, end.line - 1] + const count = edit.end.line - edit.pos.line; const newLines = [...edit.lines]; + // The end line itself survives (exclusive). If the model re-emits it + // in lines, that's a duplication mistake — auto-correct by popping. const trailingReplacementLine = newLines[newLines.length - 1]?.trimEnd(); - const nextSurvivingLine = fileLines[edit.end.line]?.trimEnd(); + const nextSurvivingLine = fileLines[edit.end.line - 1]?.trimEnd(); if ( shouldAutocorrect(trailingReplacementLine, nextSurvivingLine) && - // Safety: only correct when end-line content differs from the duplicate. - // If end already points to the boundary, matching next line is coincidence. - fileLines[edit.end.line - 1]?.trimEnd() !== trailingReplacementLine + // Safety: only correct when the last consumed line differs from the duplicate. + // If the last consumed line is the same as the surviving end line, it's coincidence. + fileLines[edit.end.line - 2]?.trimEnd() !== trailingReplacementLine ) { newLines.pop(); warnings.push( - `Auto-corrected range replace ${edit.pos.line}#${edit.pos.hash}-${edit.end.line}#${edit.end.hash}: removed trailing replacement line "${trailingReplacementLine}" that duplicated next surviving line`, + `Auto-corrected range replace ${edit.pos.line}#${edit.pos.hash}..${edit.end.line}#${edit.end.hash}: removed trailing replacement line "${trailingReplacementLine}" that duplicated the surviving end line`, ); } const leadingReplacementLine = newLines[0]?.trimEnd(); @@ -659,7 +673,7 @@ export function applyHashlineEdits( ) { newLines.shift(); warnings.push( - `Auto-corrected range replace ${edit.pos.line}#${edit.pos.hash}-${edit.end.line}#${edit.end.hash}: removed leading replacement line "${leadingReplacementLine}" that duplicated preceding surviving line`, + `Auto-corrected range replace ${edit.pos.line}#${edit.pos.hash}..${edit.end.line}#${edit.end.hash}: removed leading replacement line "${leadingReplacementLine}" that duplicated preceding surviving line`, ); } fileLines.splice(edit.pos.line - 1, count, ...newLines); diff --git a/packages/coding-agent/src/prompts/tools/hashline.md b/packages/coding-agent/src/prompts/tools/hashline.md index 2b7fea2e3..821564d68 100644 --- a/packages/coding-agent/src/prompts/tools/hashline.md +++ b/packages/coding-agent/src/prompts/tools/hashline.md @@ -11,14 +11,13 @@ Read the file first to get fresh tags. Submit one `edit` call per file with all - if `replace`: first line to rewrite - if `prepend`: line to insert new lines **before**; omit for beginning of file - if `append`: line to insert new lines **after**; omit for end of file -**`edits[n].end`** — range replace only. The last line of the range (inclusive). Omit for single-line replace. +**`edits[n].end`** — range replace only. The first line **after** the range (exclusive — this line survives). Omit for single-line replace. **`edits[n].lines`** — the replacement content: - - for `replace`: the exact lines that will replace `[pos, end??pos]` inclusively (or the single `pos` line when `end` is omitted) + - for `replace`: the lines that will replace `[pos, end)`. Everything from `pos` up to (but not including) `end` is removed; `lines` is inserted in its place. - for `prepend`/`append`: the new lines to insert - `[""]` — blank line - `null` or `[]` — delete if replace -- If `lines` contains content that already exists after `end`, those lines **will be duplicated** in the output. -- Keep `lines` to exactly what belongs inside the consumed range. +- **`end` is exclusive — the line it points to stays in the file.** You do not need to re-emit it in `lines`. If you accidentally include it in `lines`, it will be duplicated. - Ops are applied bottom-up. Tags **MUST** be referenced from the most recent `read` output. @@ -71,22 +70,22 @@ Single line — `lines: null` deletes entirely: }] } ``` -Range — remove the legacy block (lines 10–11): +Range — remove the legacy block (lines 10–11). `end` points to line 12 (the line after the range): ``` { path: "util.ts", edits: [{ op: "replace", pos: {{hlineref 10 "\t// TODO: remove after migration"}}, - end: {{hlineref 11 "\tlegacy();"}}, + end: {{hlineref 12 "\ttry {"}}, lines: null }] } ``` - -Replace the catch body with smarter error handling. Shape (a): `pos` is the first body line, `end` is the last body line. The catch header (line 14) and its closer (line 17) are outside the range and stay untouched. + +Replace the catch body with smarter error handling. `pos` is the first body line, `end` is the closer — the closer survives automatically. When changing body content, replace the **entire** body span — not just one line inside it. Patching one line leaves the rest of the body stale. ``` @@ -95,7 +94,7 @@ When changing body content, replace the **entire** body span — not just one li edits: [{ op: "replace", pos: {{hlineref 15 "\t\tconsole.error(err);"}}, - end: {{hlineref 16 "\t\treturn null;"}}, + end: {{hlineref 17 "\t}"}}, lines: [ "\t\tif (isEnoent(err)) return null;", "\t\tthrow err;" @@ -103,28 +102,15 @@ When changing body content, replace the **entire** body span — not just one li }] } ``` +Result: lines 15–16 are replaced. The `\t}` on line 17 stays because `end` is exclusive. - -Simplify `beta()` to a one-liner. Shape (b): `pos`=header, `end`=closer, re-emit all in `lines`. + +Simplify `beta()` to a one-liner. `pos`=header (consumed), `end`=the line **after** the block (survives). -Bad — `end` stops at the inner `\t}` on line 17, so the outer `}` on line 18 survives. Result: two consecutive `}` lines. -``` -{ - path: "util.ts", - edits: [{ - op: "replace", - pos: {{hlineref 9 "function beta() {"}}, - end: {{hlineref 17 "\t}"}}, - lines: [ - "function beta() {", - "\treturn parse(data);", - "}" - ] - }] -} -``` -Good — `end` includes the function's own `}` on line 18, so the old closer is consumed: +Since `}` on line 18 is the last line of `beta()` and we want to consume it, `end` must point to the next line after the block. When line 18 is the last line of the file, omit `end` — single-line replace plus a delete of lines 10–17 first, or use `write` to rewrite the file. + +When there IS a line after the block: ``` { path: "util.ts", @@ -134,12 +120,12 @@ Good — `end` includes the function's own `}` on line 18, so the old closer is end: {{hlineref 18 "}"}}, lines: [ "function beta() {", - "\treturn parse(data);", - "}" + "\treturn parse(data);" ] }] } ``` +Result: lines 9–17 are consumed (replaced). `}` on line 18 survives as beta's closer — no need to re-emit it. @@ -149,7 +135,7 @@ Bad — if you need to change code on both sides of that line, replacing just th Good — choose one of two safe shapes instead: - move inward and replace only body-owned lines -- expand outward and replace one whole owned block, consuming its real closer/separator too +- expand outward and replace one whole owned block @@ -180,9 +166,9 @@ Use a trailing `""` to preserve the blank line between sibling declarations. - For `append`/`prepend`, `lines` **MUST** contain only the newly introduced content. Do not re-emit surrounding content, or terminators that already exist. - When changing existing code near a block tail or closing delimiter, default to `replace` over the owned span instead of inserting around the boundary. - When adding a sibling declaration, default to `prepend` on the next sibling declaration instead of `append` on the previous block's closing brace. -- **Block boundaries travel together.** For a block `{ header / body / closer }`, there are exactly two valid replace shapes: (a) replace only the body — `pos`=first body line, `end`=last body line, leave the header and closer untouched; or (b) replace the whole block — `pos`=header, `end`=closer, re-emit all three in `lines`. Never split them: do not set `end` to the closer while omitting it from `lines` (deletes it), and do not emit the closer in `lines` without including it in `end` (duplicates it). This applies to every block terminator: `}`, `continue`, `break`, `return`, `throw`. -- **Never target shared boundary lines.** Do not use `replace` spans that start, end, or pivot on a line that closes one construct and opens/separates another, such as `},{`, `}),`, `} else {`, or `} catch (err) {`. Those lines are not owned by a single block. Move the range inward to body-only lines, or widen it to consume one whole owned construct including its true trailing delimiter. -- **`lines` must not extend past `end`.** `lines` replaces exactly `pos..end`. Content after `end` survives. If you include lines in `lines` that exist after `end`, they will appear twice. Either extend `end` to cover all lines you are re-emitting, or remove the extra lines from `lines`. +- **`end` is the boundary you want to keep.** Point `end` at the closing delimiter (`}`, `)`, ``) when you want it to survive. Point `end` past it when you want to consume it. Do **not** include the `end` line in `lines` — it survives on its own. +- **Never target shared boundary lines.** Do not use `replace` spans that start, end, or pivot on a line that closes one construct and opens/separates another, such as `},{`, `}),`, `} else {`, or `} catch (err) {`. Those lines are not owned by a single block. Move the range inward to body-only lines, or widen it to consume one whole owned construct. +- **`lines` must not extend past `end`.** `lines` replaces exactly `[pos, end)`. The `end` line and everything after it survives. If you include `end`-line content in `lines`, it will appear twice. - `lines` entries **MUST** be literal file content with indentation copied exactly from the `read` output. If the file uses tabs, use a real tab character. - After any successful `edit` call on a file, the next change to that same file **MUST** start with a fresh `read`. Do not chain a second `edit` call off stale mental state, even if the intended range is nearby. - If you need a second change in the same local region, default to one wider `replace` over the whole owned block instead of a sequence of micro-edits on adjacent lines. Repeated small patches in a moving region are unstable. diff --git a/packages/coding-agent/src/tools/bash-interceptor.ts b/packages/coding-agent/src/tools/bash-interceptor.ts index e78096ba8..8fca9e981 100644 --- a/packages/coding-agent/src/tools/bash-interceptor.ts +++ b/packages/coding-agent/src/tools/bash-interceptor.ts @@ -5,45 +5,7 @@ * this interceptor provides helpful error messages directing them to use * the specialized tools instead. */ -import type { BashInterceptorRule } from "../config/settings-schema"; - -export const DEFAULT_BASH_INTERCEPTOR_RULES: BashInterceptorRule[] = [ - { - pattern: "^\\s*(cat|head|tail|less|more)\\s+", - tool: "read", - message: "Use the `read` tool instead of cat/head/tail. It provides better context and handles binary files.", - }, - { - pattern: "^\\s*(grep|rg|ripgrep|ag|ack)\\s+", - tool: "grep", - message: "Use the `grep` tool instead of grep/rg. It respects .gitignore and provides structured output.", - }, - { - pattern: "^\\s*(find|fd|locate)\\s+.*(-name|-iname|-type|--type|-glob)", - tool: "find", - message: "Use the `find` tool instead of find/fd. It respects .gitignore and is faster for glob patterns.", - }, - { - pattern: "^\\s*sed\\s+(-i|--in-place)", - tool: "edit", - message: "Use the `edit` tool instead of sed -i. It provides diff preview and fuzzy matching.", - }, - { - pattern: "^\\s*perl\\s+.*-[pn]?i", - tool: "edit", - message: "Use the `edit` tool instead of perl -i. It provides diff preview and fuzzy matching.", - }, - { - pattern: "^\\s*awk\\s+.*-i\\s+inplace", - tool: "edit", - message: "Use the `edit` tool instead of awk -i inplace. It provides diff preview and fuzzy matching.", - }, - { - pattern: "^\\s*(echo|printf|cat\\s*<<)\\s+.*[^|]>\\s*\\S", - tool: "write", - message: "Use the `write` tool instead of echo/cat redirection. It handles encoding and provides confirmation.", - }, -]; +import { type BashInterceptorRule, DEFAULT_BASH_INTERCEPTOR_RULES } from "../config/settings-schema"; export interface InterceptionResult { /** If true, the bash command should be blocked */ diff --git a/packages/coding-agent/test/core/hashline.test.ts b/packages/coding-agent/test/core/hashline.test.ts index 0e6794de4..f78f35cd8 100644 --- a/packages/coding-agent/test/core/hashline.test.ts +++ b/packages/coding-agent/test/core/hashline.test.ts @@ -253,18 +253,29 @@ describe("applyHashlineEdits — replace", () => { expect(result.firstChangedLine).toBe(2); }); - it("range replace (shrink)", () => { + it("range replace (shrink) — end is exclusive", () => { const content = "aaa\nbbb\nccc\nddd"; + // end points to "ccc" which survives; only line 2 ("bbb") is consumed const edits: HashlineEdit[] = [{ op: "replace", pos: makeTag(2, "bbb"), end: makeTag(3, "ccc"), lines: ["ONE"] }]; + const result = applyHashlineEdits(content, edits); + expect(result.lines).toBe("aaa\nONE\nccc\nddd"); + }); + + it("range replace consuming multiple lines", () => { + const content = "aaa\nbbb\nccc\nddd"; + // end points to "ddd" which survives; lines 2-3 are consumed + const edits: HashlineEdit[] = [{ op: "replace", pos: makeTag(2, "bbb"), end: makeTag(4, "ddd"), lines: ["ONE"] }]; + const result = applyHashlineEdits(content, edits); expect(result.lines).toBe("aaa\nONE\nddd"); }); it("range replace (same count)", () => { const content = "aaa\nbbb\nccc\nddd"; + // Consume lines 2-3, replace with two lines. "ddd" survives. const edits: HashlineEdit[] = [ - { op: "replace", pos: makeTag(2, "bbb"), end: makeTag(3, "ccc"), lines: ["XXX", "YYY"] }, + { op: "replace", pos: makeTag(2, "bbb"), end: makeTag(4, "ddd"), lines: ["XXX", "YYY"] }, ]; const result = applyHashlineEdits(content, edits); @@ -272,6 +283,28 @@ describe("applyHashlineEdits — replace", () => { expect(result.firstChangedLine).toBe(2); }); + it("applies prepend at the surviving end boundary before the enclosing range replace", () => { + const content = "a\nb\nc\nd"; + const edits: HashlineEdit[] = [ + { op: "replace", pos: makeTag(2, "b"), end: makeTag(4, "d"), lines: ["X"] }, + { op: "prepend", pos: makeTag(4, "d"), lines: ["Y"] }, + ]; + const result = applyHashlineEdits(content, edits); + expect(result.lines).toBe("a\nX\nY\nd"); + expect(result.firstChangedLine).toBe(2); + }); + + it("applies single-line replace at the surviving end boundary before the enclosing range replace", () => { + const content = "a\nb\nc\nd"; + const edits: HashlineEdit[] = [ + { op: "replace", pos: makeTag(2, "b"), end: makeTag(4, "d"), lines: ["X"] }, + { op: "replace", pos: makeTag(4, "d"), lines: ["Y"] }, + ]; + const result = applyHashlineEdits(content, edits); + expect(result.lines).toBe("a\nX\nY"); + expect(result.firstChangedLine).toBe(2); + }); + it("replaces first line", () => { const content = "first\nsecond\nthird"; const edits: HashlineEdit[] = [{ op: "replace", pos: makeTag(1, "first"), lines: ["FIRST"] }]; @@ -305,9 +338,10 @@ describe("applyHashlineEdits — delete", () => { expect(result.firstChangedLine).toBe(2); }); - it("deletes range of lines", () => { + it("deletes range of lines (end exclusive)", () => { const content = "aaa\nbbb\nccc\nddd"; - const edits: HashlineEdit[] = [{ op: "replace", pos: makeTag(2, "bbb"), end: makeTag(3, "ccc"), lines: [] }]; + // end points to "ddd" which survives; lines 2-3 deleted + const edits: HashlineEdit[] = [{ op: "replace", pos: makeTag(2, "bbb"), end: makeTag(4, "ddd"), lines: [] }]; const result = applyHashlineEdits(content, edits); expect(result.lines).toBe("aaa\nddd"); @@ -494,11 +528,12 @@ describe("applyHashlineEdits — heuristics", () => { it("does not override model whitespace choices in replacement content", () => { const content = ["import { foo } from 'x';", "import { bar } from 'y';", "const x = 1;"].join("\n"); + // end points to "const x = 1;" (survives); lines 1-2 are consumed const edits: HashlineEdit[] = [ { op: "replace", pos: makeTag(1, "import { foo } from 'x';"), - end: makeTag(2, "import { bar } from 'y';"), + end: makeTag(3, "const x = 1;"), lines: ["import {foo} from 'x';", "import { bar } from 'y';", "// added"], }, ]; @@ -511,21 +546,21 @@ describe("applyHashlineEdits — heuristics", () => { expect(outLines[3]).toBe("const x = 1;"); }); - it("treats same-line ranges as single-line replacements", () => { + it("rejects same-line range (pos must be < end)", () => { const content = "aaa\nbbb\nccc"; const good = makeTag(2, "bbb"); const edits: HashlineEdit[] = [{ op: "replace", pos: good, end: good, lines: ["BBB"] }]; - const result = applyHashlineEdits(content, edits); - expect(result.lines).toBe("aaa\nBBB\nccc"); + expect(() => applyHashlineEdits(content, edits)).toThrow(/must be < end/); }); - it("auto-corrects off-by-one range end that duplicates a closing brace", () => { + it("auto-corrects when model re-emits the exclusive end line (closing brace)", () => { const content = "if (ok) {\n run();\n}\nafter();"; + // end points to "}" which should survive. Model accidentally includes it in lines. const edits: HashlineEdit[] = [ { op: "replace", pos: makeTag(1, "if (ok) {"), - end: makeTag(2, " run();"), + end: makeTag(3, "}"), lines: ["if (ok) {", " runSafe();", "}"], }, ]; @@ -536,13 +571,14 @@ describe("applyHashlineEdits — heuristics", () => { expect(result.warnings?.[0]).toContain('"}"'); }); - it('auto-corrects off-by-one range end that duplicates a ");" closer', () => { + it('auto-corrects when model re-emits the exclusive end line (");" closer)', () => { const content = "doThing(\n value,\n);\nnext();"; + // end points to ");" which should survive. Model accidentally includes it. const edits: HashlineEdit[] = [ { op: "replace", pos: makeTag(1, "doThing("), - end: makeTag(2, " value,"), + end: makeTag(3, ");"), lines: ["doThing(", " normalize(value),", ");"], }, ]; @@ -552,13 +588,14 @@ describe("applyHashlineEdits — heuristics", () => { expect(result.warnings?.[0]).toContain('");"'); }); - it("auto-corrects duplicated trailing lines when they match the next surviving line", () => { + it("auto-corrects when model re-emits the exclusive end line (generic content)", () => { const content = "start\n oldCall();\nnextCall();\nafter();"; + // end points to "nextCall();" which should survive. Model includes it in lines. const edits: HashlineEdit[] = [ { op: "replace", pos: makeTag(1, "start"), - end: makeTag(2, " oldCall();"), + end: makeTag(3, "nextCall();"), lines: ["start", " newCall();", "nextCall();"], }, ]; @@ -570,15 +607,18 @@ describe("applyHashlineEdits — heuristics", () => { it("auto-corrects off-by-one range start that duplicates a preceding line", () => { const content = "if (x) {\n oldBody();\n}\nafter();"; + // pos is body line 2, end is "}" (exclusive, survives). + // Model includes "if (x) {" in lines, duplicating preceding line. const edits: HashlineEdit[] = [ { op: "replace", pos: makeTag(2, " oldBody();"), end: makeTag(3, "}"), - lines: ["if (x) {", " newBody();", "}"], + lines: ["if (x) {", " newBody();"], }, ]; const result = applyHashlineEdits(content, edits); + // Leading "if (x) {" is popped; "}" survives from exclusive end expect(result.lines).toBe("if (x) {\n newBody();\n}\nafter();"); expect(result.warnings).toHaveLength(1); expect(result.warnings?.[0]).toContain("removed leading replacement line"); @@ -685,11 +725,12 @@ describe("applyHashlineEdits — multiple edits", () => { it("applies non-overlapping edits against original anchors when line counts change", () => { const content = "one\ntwo\nthree\nfour\nfive\nsix"; + // end is exclusive: "four" survives, lines 2-3 are consumed const edits: HashlineEdit[] = [ { op: "replace", pos: makeTag(2, "two"), - end: makeTag(3, "three"), + end: makeTag(4, "four"), lines: ["TWO_THREE"], }, { op: "replace", pos: makeTag(6, "six"), lines: ["SIX"] }, @@ -802,7 +843,7 @@ describe("applyHashlineEdits — errors", () => { expect(() => applyHashlineEdits(content, edits)).toThrow(/does not exist/); }); - it("rejects range with start > end", () => { + it("rejects range with start >= end (exclusive end)", () => { const content = "aaa\nbbb\nccc\nddd\neee"; const edits: HashlineEdit[] = [{ op: "replace", pos: makeTag(5, "eee"), end: makeTag(2, "bbb"), lines: ["X"] }]; diff --git a/packages/coding-agent/test/modes/controllers/session-selector-delete.test.ts b/packages/coding-agent/test/modes/controllers/session-selector-delete.test.ts index fcb22edc8..cb5d8d382 100644 --- a/packages/coding-agent/test/modes/controllers/session-selector-delete.test.ts +++ b/packages/coding-agent/test/modes/controllers/session-selector-delete.test.ts @@ -1,19 +1,11 @@ -import { afterEach, describe, expect, it, mock, vi } from "bun:test"; +import { afterEach, beforeAll, describe, expect, it, vi } from "bun:test"; +import { SessionSelectorComponent } from "../../../src/modes/components/session-selector"; +import { initTheme } from "../../../src/modes/theme/theme"; import type { SessionInfo } from "../../../src/session/session-manager"; -const themeModulePath = new URL("../../../src/modes/theme/theme.ts", import.meta.url).pathname; - -mock.module(themeModulePath, () => ({ - theme: { - fg: (_tone: string, text: string) => text, - bold: (text: string) => text, - nav: { cursor: ">" }, - sep: { dot: "·" }, - boxSharp: { horizontal: "-", vertical: "|" }, - }, -})); - -import { SessionSelectorComponent } from "../../../src/modes/components/session-selector"; +beforeAll(() => { + initTheme(); +}); afterEach(() => { vi.restoreAllMocks(); diff --git a/packages/coding-agent/test/tools.test.ts b/packages/coding-agent/test/tools.test.ts index 354cbeddd..fec75e389 100644 --- a/packages/coding-agent/test/tools.test.ts +++ b/packages/coding-agent/test/tools.test.ts @@ -2,7 +2,7 @@ import { afterEach, beforeEach, describe, expect, it } from "bun:test"; import * as fs from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; -import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; +import { DEFAULT_BASH_INTERCEPTOR_RULES, Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import { EditTool } from "@oh-my-pi/pi-coding-agent/patch"; import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; import { BashTool } from "@oh-my-pi/pi-coding-agent/tools/bash"; @@ -455,6 +455,91 @@ function b() { expect(result.details).toBeUndefined(); }); + it("should expose built-in interceptor defaults truthfully", () => { + const defaultSettings = Settings.isolated({ "bashInterceptor.enabled": true }); + const explicitEmptySettings = Settings.isolated({ + "bashInterceptor.enabled": true, + "bashInterceptor.patterns": [], + }); + + expect(defaultSettings.get("bashInterceptor.patterns")).toEqual(DEFAULT_BASH_INTERCEPTOR_RULES); + expect(defaultSettings.getBashInterceptorRules()).toEqual(DEFAULT_BASH_INTERCEPTOR_RULES); + expect(explicitEmptySettings.get("bashInterceptor.patterns")).toEqual([]); + expect(explicitEmptySettings.getBashInterceptorRules()).toEqual([]); + }); + + it("should block built-in interceptor commands when enabled with default patterns", async () => { + const interceptedBashTool = wrapToolWithMetaNotice( + new BashTool(createTestToolSession(testDir, Settings.isolated({ "bashInterceptor.enabled": true }))), + ); + + await expect( + interceptedBashTool.execute( + "test-call-8-intercept-default", + { command: "cat test.txt" }, + undefined, + undefined, + { toolNames: ["read"] }, + ), + ).rejects.toThrow(/Use the `read` tool instead of cat\/head\/tail/); + }); + + it("should allow an explicit empty interceptor pattern list", async () => { + const allowedFile = path.join(testDir, "allow-empty.txt"); + fs.writeFileSync(allowedFile, "empty means empty\n"); + + const interceptedBashTool = wrapToolWithMetaNotice( + new BashTool( + createTestToolSession( + testDir, + Settings.isolated({ + "bashInterceptor.enabled": true, + "bashInterceptor.patterns": [], + }), + ), + ), + ); + + const result = await interceptedBashTool.execute( + "test-call-8-intercept-empty", + { command: `cat ${allowedFile}` }, + undefined, + undefined, + { toolNames: ["read"] }, + ); + + expect(getTextOutput(result)).toContain("empty means empty"); + }); + + it("should honor custom bash interceptor patterns", async () => { + const interceptedBashTool = wrapToolWithMetaNotice( + new BashTool( + createTestToolSession( + testDir, + Settings.isolated({ + "bashInterceptor.enabled": true, + "bashInterceptor.patterns": [ + { + pattern: "^\\s*customcmd\\s+", + tool: "grep", + message: "Use the `grep` tool for customcmd.", + }, + ], + }), + ), + ), + ); + await expect( + interceptedBashTool.execute( + "test-call-8-intercept-custom", + { command: "customcmd foo" }, + undefined, + undefined, + { toolNames: ["grep"] }, + ), + ).rejects.toThrow(/Use the `grep` tool for customcmd\./); + }); + it("should expose env values without shell re-parsing", async () => { const mermaid = [ "flowchart TD",