diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 2e70c2bd9..cf126b6ed 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Filtered alias definitions brush's whitespace-only expander cannot execute (`(`, `)`, `|`, `&`, `;`, `<`, `>`, `` ` ``) from the bash-tool shell snapshot, so user rc-files containing compound aliases like Fedora's default `which='(alias; declare -f) | /usr/bin/which …'` no longer poison the brush session with `error: command not found: (alias;` ([#3234](https://github.com/can1357/oh-my-pi/issues/3234)). + ## [16.1.14] - 2026-06-22 ### Added diff --git a/packages/coding-agent/src/utils/shell-snapshot.ts b/packages/coding-agent/src/utils/shell-snapshot.ts index 851f67bbb..438daf0c5 100644 --- a/packages/coding-agent/src/utils/shell-snapshot.ts +++ b/packages/coding-agent/src/utils/shell-snapshot.ts @@ -8,11 +8,72 @@ import * as fs from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; -import { postmortem } from "@oh-my-pi/pi-utils"; +import { logger, postmortem } from "@oh-my-pi/pi-utils"; const cachedSnapshotPaths = new Map(); const SNAPSHOT_TIMEOUT_MS = 2_000; +/** + * Characters that force brush's primitive alias expander down a path it does + * not implement. brush-core resolves aliases via `value.split_ascii_whitespace()` + * (`crates/brush-core-vendored/src/interp.rs:1500`, tracking + * https://github.com/reubeno/brush/issues/57): the resulting pieces are dropped + * into argv verbatim instead of going through the shell parser. Any alias body + * containing subshells `(...)`, pipes `|`, redirections `<` `>`, separators + * `;` `&`, or command substitutions `` ` `` turns the first whitespace-split + * piece into the command name and produces `command not found: (alias;` style + * failures (issue #3234, Fedora's default `which` alias is the canonical case). + * + * Until brush implements proper alias parsing we drop these from the snapshot; + * brush then falls through to whatever lives on `PATH`, which is what the user + * actually expected when they invoked `which` / `ls` / etc. + */ +const BRUSH_INCOMPATIBLE_ALIAS_BODY = /[()|&;<>`]/; + +/** Matches `alias -- NAME='VALUE'` lines emitted by `generateSnapshotScript`. */ +const SNAPSHOT_ALIAS_LINE = /^alias -- ([^\s=]+)='(.*)'\s*$/; + +/** + * Strip alias definitions brush's whitespace-only expander cannot execute. + * + * Returns the rewritten snapshot plus the list of dropped alias names so the + * caller can surface them in the debug log. + */ +export function sanitizeSnapshotForBrush(content: string): { content: string; dropped: string[] } { + const dropped: string[] = []; + const lines = content.split("\n"); + const out: string[] = []; + for (const line of lines) { + const m = line.match(SNAPSHOT_ALIAS_LINE); + if (m) { + // Decode the bash-quoting escape `'\''` → `'` so we test the real value. + const value = m[2].replace(/'\\''/g, "'"); + if (BRUSH_INCOMPATIBLE_ALIAS_BODY.test(value)) { + dropped.push(m[1]); + continue; + } + } + out.push(line); + } + return { content: out.join("\n"), dropped }; +} + +/** + * Apply {@link sanitizeSnapshotForBrush} to the freshly generated snapshot + * file. Best-effort: I/O failures here must not poison `getOrCreateSnapshot`. + */ +function scrubSnapshotInPlace(snapshotPath: string): void { + try { + const raw = fs.readFileSync(snapshotPath, "utf8"); + const { content, dropped } = sanitizeSnapshotForBrush(raw); + if (dropped.length === 0) return; + fs.writeFileSync(snapshotPath, content); + logger.debug("shell-snapshot: dropped brush-incompatible aliases", { dropped }); + } catch (err) { + logger.debug("shell-snapshot: scrub failed", { err: String(err) }); + } +} + function sanitizeSnapshotEnv(env: Record): Record { const sanitized = { ...env }; delete sanitized.BASH_ENV; @@ -169,6 +230,7 @@ export async function getOrCreateSnapshot( await child.exited; if (child.exitCode === 0 && fs.existsSync(snapshotPath)) { + scrubSnapshotInPlace(snapshotPath); cachedSnapshotPaths.set(cacheKey, snapshotPath); return snapshotPath; } diff --git a/packages/coding-agent/test/bash-executor.test.ts b/packages/coding-agent/test/bash-executor.test.ts index 7d02a23ee..6e5b344c2 100644 --- a/packages/coding-agent/test/bash-executor.test.ts +++ b/packages/coding-agent/test/bash-executor.test.ts @@ -797,6 +797,54 @@ exit 64 expect(result.output.trim()).toBe("snapshot_ok"); }); + it("survives compound aliases from the user's shell snapshot (issue #3234)", async () => { + if (process.platform === "win32") return; + const bashPath = Bun.env.SHELL?.includes("bash") ? Bun.env.SHELL : "/bin/bash"; + if (!fs.existsSync(bashPath)) return; + + // Pre-seed a snapshot that mirrors Fedora's default `which` alias. + // Without the brush-compat scrub, brush's whitespace-only alias + // expander turns `(alias;` into the command name and `which` fails + // with `command not found: (alias;`. With the scrub, the broken + // alias is dropped and brush falls through to `$PATH`. + const snapshotPath = path.join(tempDir, "snapshot.sh"); + fs.writeFileSync( + snapshotPath, + [ + "unalias -a 2>/dev/null || true", + "alias -- which='(alias; declare -f) | /usr/bin/which --tty-only --read-alias --show-dot --show-tilde'", + "alias -- ll='ls -l'", + "", + ].join("\n"), + ); + const rawSnapshot = fs.readFileSync(snapshotPath, "utf8"); + const { content: scrubbed, dropped } = + shellSnapshot.sanitizeSnapshotForBrush(rawSnapshot); + fs.writeFileSync(snapshotPath, scrubbed); + expect(dropped).toEqual(["which"]); + // Compatible aliases must still be installed in brush. + expect(scrubbed).toContain("alias -- ll='ls -l'"); + + vi.spyOn(Settings.prototype, "getShellConfig").mockReturnValue({ + shell: bashPath, + args: ["-l", "-c"], + env: { PATH: Bun.env.PATH ?? "", HOME: Bun.env.HOME ?? tempDir }, + prefix: undefined, + }); + vi.spyOn(shellSnapshot, "getOrCreateSnapshot").mockResolvedValue(snapshotPath); + + const result = await executeBash("which sh", { + cwd: tempDir, + timeout: 5000, + sessionKey: "brush-compound-alias-which", + }); + + expect(result.cancelled).toBe(false); + expect(result.exitCode).toBe(0); + expect(result.output).not.toContain("command not found"); + expect(result.output.trim()).toMatch(/\/sh$/); + }); + it("does not allow exec to replace the host", async () => { const result = await executeBash("exec echo hi", { cwd: tempDir, timeout: 5000 }); expect(result.cancelled).toBe(false); diff --git a/packages/coding-agent/test/shell-snapshot.test.ts b/packages/coding-agent/test/shell-snapshot.test.ts new file mode 100644 index 000000000..0a74540f9 --- /dev/null +++ b/packages/coding-agent/test/shell-snapshot.test.ts @@ -0,0 +1,80 @@ +import { describe, expect, it } from "bun:test"; +import { sanitizeSnapshotForBrush } from "@oh-my-pi/pi-coding-agent/utils/shell-snapshot"; + +// `sanitizeSnapshotForBrush` is the snapshot-side mitigation for brush's +// whitespace-only alias expander (`crates/brush-core-vendored/src/interp.rs:1500`, +// brush issue reubeno/brush#57). Aliases whose body needs real shell parsing +// must be dropped or brush turns the first whitespace piece into the command +// name and the user sees `error: command not found: (alias;` (issue #3234). + +describe("sanitizeSnapshotForBrush", () => { + it("drops the Fedora `which` alias and reports the dropped name", () => { + const snapshot = [ + "unalias -a 2>/dev/null || true", + "alias -- which='(alias; declare -f) | /usr/bin/which --tty-only --read-alias --show-dot --show-tilde'", + "alias -- ll='ls -l'", + "", + ].join("\n"); + + const result = sanitizeSnapshotForBrush(snapshot); + + expect(result.dropped).toEqual(["which"]); + expect(result.content).not.toContain("alias -- which="); + // Simple aliases must still pass through untouched. + expect(result.content).toContain("alias -- ll='ls -l'"); + }); + + it.each([ + ["subshell", "alias -- a='(echo hi)'"], + ["pipe", "alias -- a='cat /etc/hostname | head -n1'"], + ["semicolon", "alias -- a='echo a; echo b'"], + ["redirect-out", "alias -- a='tee >foo'"], + ["redirect-in", "alias -- a='cat { + const result = sanitizeSnapshotForBrush(line + "\n"); + expect(result.dropped).toEqual(["a"]); + expect(result.content).not.toContain("alias -- a="); + }); + + it.each([ + ["simple", "alias -- ll='ls -l'"], + ["flag-with-equals", "alias -- gc='git --color=auto commit'"], + ["multi-flag", "alias -- la='ls -lAh --group-directories-first'"], + // A plain single quote escape that decodes to a metachar-free body + // must survive — we only ban truly unparseable bodies. + ["embedded-quote", "alias -- say='echo '\\''hello'\\'''"], + ])("preserves aliases brush can handle by whitespace split (%s)", (_label, line) => { + const result = sanitizeSnapshotForBrush(line + "\n"); + expect(result.dropped).toEqual([]); + expect(result.content).toContain(line); + }); + + it("leaves non-alias lines (functions, exports, unalias) untouched", () => { + const snapshot = [ + "# Shell snapshot - generated by omp agent", + "unalias -a 2>/dev/null || true", + "my_fn () { echo hi; }", + "export PATH='/usr/bin:/bin'", + "shopt -s expand_aliases", + "alias -- which='(alias; declare -f) | /usr/bin/which'", // poisoned + ].join("\n"); + + const result = sanitizeSnapshotForBrush(snapshot); + + expect(result.dropped).toEqual(["which"]); + for (const keep of [ + "# Shell snapshot - generated by omp agent", + "unalias -a 2>/dev/null || true", + "my_fn () { echo hi; }", // function body contains `;` but is not an alias line + "export PATH='/usr/bin:/bin'", + "shopt -s expand_aliases", + ]) { + expect(result.content).toContain(keep); + } + }); +});