From fee177a445588159eba6ff5bba9ebcf459192201 Mon Sep 17 00:00:00 2001 From: roboomp Date: Sat, 4 Jul 2026 17:38:42 +0000 Subject: [PATCH] fix(tool): clarified grep alternation guidance - Documented the supported Rust-style grep alternation and shell grep -E fallback for agents. - Warned agents away from bash when exact pipeline semantics or shell-specific regex behavior matter. - Added regression coverage for the rendered tool descriptions and recommended grep -E pipeline path. Fixes #4540 --- packages/coding-agent/CHANGELOG.md | 1 + .../coding-agent/src/prompts/tools/bash.md | 10 +- .../coding-agent/src/prompts/tools/grep.md | 2 +- .../test/tool-regex-guidance.test.ts | 94 +++++++++++++++++++ 4 files changed, 102 insertions(+), 5 deletions(-) create mode 100644 packages/coding-agent/test/tool-regex-guidance.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 9c2395e3d..9118275d7 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -12,6 +12,7 @@ ### Fixed +- Fixed grep/bash tool guidance so agents use Rust-style `foo|bar` patterns or `grep -E` instead of GNU BRE `\|` assumptions, and avoid bash when exact pipeline semantics are required. ([#4540](https://github.com/can1357/oh-my-pi/issues/4540)) - Fixed raw `read` ranges not contributing to edit seen-line provenance, so re-reading an anchor range with `:raw` now unblocks hashline edits without adding non-raw line prefixes. - Fixed replan-driven session title refresh updating the statusline but not the terminal window title: terminal-title updates now fire from the session-name-changed listener, so every `setSessionName` path (first-input titling, `/rename`, plan seeding, replan refresh) sets the OSC title consistently. - Fixed user-interrupt aborts rendering the persisted `Interrupted by user` label in assistant transcripts; replay and live views now suppress that redundant line again while preserving generic/custom abort labels. diff --git a/packages/coding-agent/src/prompts/tools/bash.md b/packages/coding-agent/src/prompts/tools/bash.md index 0cfdd4238..3e22867ce 100644 --- a/packages/coding-agent/src/prompts/tools/bash.md +++ b/packages/coding-agent/src/prompts/tools/bash.md @@ -1,10 +1,10 @@ -Runs bash in a shell session — terminal ops: git, bun, cargo, python. +Runs commands in the embedded shell — terminal ops: git, bun, cargo, python. # When to use bash — and when not to -Bash invokes **real binaries** with simple args. It is NOT a scripting surface. +The shell invokes **real binaries** with simple args. It is NOT full GNU Bash. -Use bash ONLY for: a single binary call, or one short pipeline that COMPUTES a fact (`wc -l`, `sort | uniq -c`, `comm`, `diff`, a checksum, `git status`). +Use bash ONLY for: a single binary call, or one short pipeline that COMPUTES a fact and does not depend on shell-specific regex/quoting (`wc -l`, `sort | uniq -c`, `comm`, `diff`, a checksum, `git status`). Anything below → `eval` cell, not bash: - Inline interpreter scripts (`-e`/`-c`/`--eval`) when an eval runtime exists for that language @@ -13,6 +13,7 @@ Anything below → `eval` cell, not bash: - Pipelines with more than two stages, or stages that need control flow or quote/JSON escaping - Multiline commands, `&&`-chains mixing control flow - Quote/JSON escaping that fights the shell +- GNU grep BRE extensions are not guaranteed: `\|` / `\b` may not mean alternation/word-boundary. Use `grep -E 'json|tool'`, the built-in `grep` tool with `pattern: "json|tool"`, or `eval` for exact text processing. - `cwd` sets the working dir, not `cd dir && …` @@ -22,13 +23,14 @@ Anything below → `eval` cell, not bash: - `;` only when later commands should run despite earlier failures - Multiple bash calls per message run concurrently. NEVER split order-dependent commands across parallel calls — chain with `&&` in one call. - Internal URIs (`skill://`, `agent://`, …) auto-resolve to FS paths +- Need exact pipeline semantics (`cmd | head`, multi-stage filtering) or output truncation? Prefer `eval` and process the stream directly. {{#if asyncEnabled}} - `async: true` for long-running commands when you don't need immediate output: returns a background job ID; result delivered as a follow-up. {{/if}} -- Bash invokes real binaries with simple args; it is NOT a scripting surface. Loops, conditionals, heredocs, inline interpreter scripts (`-e`/`-c`/`--eval`) when an eval runtime exists, several piped stages, or quote/JSON escaping mean you're writing a program → use `eval` cells: restartable, stateful, and free of shell-quoting traps. +- The embedded shell invokes real binaries with simple args; it is NOT full GNU Bash. Loops, conditionals, heredocs, inline interpreter scripts (`-e`/`-c`/`--eval`) when an eval runtime exists, several piped stages, exact pipeline semantics, or quote/JSON escaping mean you're writing a program → use `eval` cells: restartable, stateful, and free of shell-quoting traps. diff --git a/packages/coding-agent/src/prompts/tools/grep.md b/packages/coding-agent/src/prompts/tools/grep.md index 0518bd77c..448168d6f 100644 --- a/packages/coding-agent/src/prompts/tools/grep.md +++ b/packages/coding-agent/src/prompts/tools/grep.md @@ -1,7 +1,7 @@ Greps files using regex. -- Rust regex (RE2-style) — no lookaround/backreferences; use line anchors or post-filters instead of (?!…)/(? diff --git a/packages/coding-agent/test/tool-regex-guidance.test.ts b/packages/coding-agent/test/tool-regex-guidance.test.ts new file mode 100644 index 000000000..ba469fdca --- /dev/null +++ b/packages/coding-agent/test/tool-regex-guidance.test.ts @@ -0,0 +1,94 @@ +import { afterEach, describe, expect, it } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; +import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; +import { BashTool } from "@oh-my-pi/pi-coding-agent/tools/bash"; +import { GrepTool } from "@oh-my-pi/pi-coding-agent/tools/grep"; +import { removeWithRetries } from "@oh-my-pi/pi-utils"; + +function makeSession(cwd: string): ToolSession { + return { + cwd, + hasUI: false, + skills: [], + getSessionFile: () => null, + settings: Settings.isolated({ + "async.enabled": false, + "bash.autoBackground.enabled": false, + "bash.autoBackground.thresholdMs": 60_000, + "bashInterceptor.enabled": false, + "bash.stripTrailingHeadTail": false, + "astGrep.enabled": true, + "astEdit.enabled": true, + "grep.enabled": true, + "glob.enabled": true, + "edit.mode": "patch", + readLineNumbers: true, + }), + getClientBridge: () => undefined, + } as unknown as ToolSession; +} + +function textOf(result: { content: Array<{ type: string; text?: string }> }): string { + return result.content + .filter(entry => entry.type === "text") + .map(entry => entry.text ?? "") + .join("\n"); +} + +function expectAlternationGuidance(description: string, goodPattern: string, escapedPattern: string): void { + expect(description).toContain(goodPattern); + expect(description).toContain(escapedPattern); + const goodIndex = description.indexOf(goodPattern); + const escapedIndex = description.indexOf(escapedPattern); + const start = Math.max(0, Math.min(goodIndex, escapedIndex) - 160); + const end = Math.min(description.length, Math.max(goodIndex, escapedIndex) + escapedPattern.length + 160); + const localGuidance = description.slice(start, end); + expect(localGuidance).toMatch(/\b(?:not|avoid|rather than|instead of|don't|do not)\b/i); +} +function expectEscapedBreWarning(description: string, escapedToken: string): void { + expect(description).toContain(escapedToken); + const tokenIndex = description.indexOf(escapedToken); + const localGuidance = description.slice(Math.max(0, tokenIndex - 120), tokenIndex + escapedToken.length + 120); + expect(localGuidance).toMatch(/\b(?:not|avoid|rather than|instead of|don't|do not|not guaranteed)\b/i); +} + +describe("tool regex guidance", () => { + const tempDirs: string[] = []; + + afterEach(async () => { + await Promise.all(tempDirs.splice(0).map(dir => removeWithRetries(dir))); + }); + + it("advertises Rust-style alternation for the built-in grep pattern", () => { + const description = new GrepTool(makeSession("/tmp")).description; + + expect(description).toContain("Rust"); + expect(description).toContain("RE2"); + expectAlternationGuidance(description, "foo|bar", String.raw`foo\|bar`); + }); + + it("advertises grep -E for shell pipelines that need alternation", () => { + const description = new BashTool(makeSession("/tmp")).description; + + expect(description).toContain("grep -E 'json|tool'"); + expectEscapedBreWarning(description, String.raw`\|`); + }); + + it("runs an extended-grep pipeline through BashTool and returns the first two matching lines", async () => { + const cwd = await fs.mkdtemp(path.join(os.tmpdir(), "omp-bash-grep-guidance-")); + tempDirs.push(cwd); + await Bun.write(path.join(cwd, "fixture.txt"), "json contract\ntool description\nignored line\njson later\n"); + + const result = await new BashTool(makeSession(cwd)).execute("grep-e-pipeline", { + command: "grep -E 'json|tool' fixture.txt | head -n2", + }); + + expect(result.isError).toBeUndefined(); + const outputLines = textOf(result).split("\n"); + expect(outputLines.slice(0, 2)).toEqual(["json contract", "tool description"]); + expect(outputLines).not.toContain("json later"); + }); +});