From 77265de55ef7eeb33842f8c7d3482c7d8b0ff791 Mon Sep 17 00:00:00 2001 From: roboomp Date: Thu, 25 Jun 2026 15:10:59 +0000 Subject: [PATCH] fix(bash): re-exported env vars referenced by snapshotted shell functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit generateSnapshotScript captured the user's shell functions via declare -f / typeset -f and dropped everything except PATH on the export floor. mise activate installs a mise() function whose body expands $__MISE_EXE; the replay shell then ran `command "" "$@"` and died with `command: command not found:` (exit 127). The same shape breaks asdf shims, direnv-style helpers, and any other activation idiom that pairs a shell function with a sidecar env var. The snapshot script now scans captured function bodies for $VAR / ${VAR…} references and re-emits `export NAME='value'` for each name that is currently set and not on a shell-internal denylist (PATH, HOME, BASH_*, LC_*, …). getShellConfigFile also honours env.HOME so callers (and tests) can target a sandboxed home — os.homedir() is cached by Bun and ignores later process.env.HOME mutations. Fixes #3470 --- packages/coding-agent/CHANGELOG.md | 4 + .../src/utils/shell-snapshot-fn-env.sh | 52 +++++++ .../coding-agent/src/utils/shell-snapshot.ts | 54 ++++--- .../coding-agent/test/shell-snapshot.test.ts | 132 +++++++++++++++++- types/assets/index.d.ts | 5 + 5 files changed, 228 insertions(+), 19 deletions(-) create mode 100644 packages/coding-agent/src/utils/shell-snapshot-fn-env.sh diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index d5afd8188..4d92d21e0 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed the bash tool's snapshotted `mise()` shell function dying with `command: command not found:` because `$__MISE_EXE` was empty in the replay shell. `generateSnapshotScript` captured the function via `declare -f`/`typeset -f` but only ever re-exported `PATH`, so every other env var the rc file set (notably the `*_EXE` sidecar `mise activate` exports) was lost; the function body then expanded `command "$__MISE_EXE" "$@"` to `command "" …` and died with exit 127. The snapshot script now scans captured function bodies for `$VAR` / `${VAR…}` references and re-emits `export NAME='value'` for each referenced var that is currently set (with a denylist for shell-internal names like `PATH`/`HOME`/`BASH_*`/`LC_*`), fixing mise, asdf shims, direnv-style helpers, and other activation idioms that pair a function with a helper env var. `getShellConfigFile` now also honours `env.HOME` (falling back to `os.homedir()`) so sandboxed callers can target a non-default rc. ([#3470](https://github.com/can1357/oh-my-pi/issues/3470)) + ## [16.1.19] - 2026-06-25 ### Fixed diff --git a/packages/coding-agent/src/utils/shell-snapshot-fn-env.sh b/packages/coding-agent/src/utils/shell-snapshot-fn-env.sh new file mode 100644 index 000000000..1ed243b6b --- /dev/null +++ b/packages/coding-agent/src/utils/shell-snapshot-fn-env.sh @@ -0,0 +1,52 @@ +# Helpers inlined into `generateSnapshotScript` (shell-snapshot.ts). +# +# Activation idioms like `mise activate` install a shell function whose body +# expands a sidecar env var (e.g. `mise() { command "$__MISE_EXE" "$@"; }`). +# The function survives `declare -f`/`typeset -f` capture, but the helper var +# is set on the rc-sourced shell and lost when only PATH gets re-exported. +# The replay shell then calls `command "" …` and dies with +# `command: command not found:` (issue #3470). +# +# `__omp_emit_referenced_exports` reads captured function bodies from stdin, +# extracts every `$VAR` / `${VAR…}` reference, and emits a single +# `export VAR='value'` line to stdout for each referenced var that +# (a) is currently set in this shell and (b) is not a shell-internal name. +# The script bodies are scanned, not interpreted — over-inclusion (refs inside +# comments / heredocs) is harmless because we only emit names that are set. +# +# Pure POSIX so the same helper works under bash and zsh. + +# Wrap $1 in single quotes, escaping every embedded `'` as `'\''`. +__omp_sq_quote() { + __omp_qbuf=$1 + __omp_qout= + __omp_sq=\' + while case "$__omp_qbuf" in *$__omp_sq*) true ;; *) false ;; esac; do + __omp_qout=$__omp_qout${__omp_qbuf%%$__omp_sq*}"'\\''" + __omp_qbuf=${__omp_qbuf#*$__omp_sq} + done + __omp_qout=$__omp_qout$__omp_qbuf + printf "'%s'" "$__omp_qout" +} + +# Emit `export NAME='value'` for $1 unless the name is a shell-internal we +# must never overwrite or the var is unset. +__omp_emit_export_for() { + case "$1" in + _|PATH|HOME|USER|LOGNAME|PWD|OLDPWD|SHELL|SHLVL|TERM|TERMINFO|TERMCAP|IFS|TMPDIR|TMOUT|LANG|RANDOM|LINENO|SECONDS|FUNCNAME|HISTFILE|HISTSIZE|HISTFILESIZE|HISTCMD|PS1|PS2|PS3|PS4|UID|EUID|GROUPS|HOSTNAME|HOSTTYPE|OSTYPE|MACHTYPE|PIPESTATUS|BASH|ZSH|argv|PROMPT|RPROMPT|RPS1|RPS2|status|pipestatus|COLUMNS|LINES|COLORTERM|FUNCNEST) return ;; + LC_*|BASH_*|ZSH_*) return ;; + esac + eval "[ \"\${$1+x}\" = x ]" 2>/dev/null || return + eval "__omp_xv=\"\${$1}\"" 2>/dev/null || return + printf 'export %s=%s\n' "$1" "$(__omp_sq_quote "$__omp_xv")" +} + +# Read function bodies on stdin, emit dedup'd export lines on stdout. +__omp_emit_referenced_exports() { + grep -oE '\$\{?[A-Za-z_][A-Za-z0-9_]*' \ + | sed -E 's/^\$\{?//' \ + | sort -u \ + | while IFS= read -r __omp_name; do + __omp_emit_export_for "$__omp_name" + done +} diff --git a/packages/coding-agent/src/utils/shell-snapshot.ts b/packages/coding-agent/src/utils/shell-snapshot.ts index 438daf0c5..f8e56c735 100644 --- a/packages/coding-agent/src/utils/shell-snapshot.ts +++ b/packages/coding-agent/src/utils/shell-snapshot.ts @@ -9,6 +9,7 @@ import * as fs from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; import { logger, postmortem } from "@oh-my-pi/pi-utils"; +import fnEnvHelper from "./shell-snapshot-fn-env.sh" with { type: "text" }; const cachedSnapshotPaths = new Map(); const SNAPSHOT_TIMEOUT_MS = 2_000; @@ -83,9 +84,13 @@ function sanitizeSnapshotEnv(env: Record): Record): string { + const home = env.HOME || os.homedir(); if (shell.includes("zsh")) return path.join(home, ".zshrc"); if (shell.includes("bash")) return path.join(home, ".bashrc"); return path.join(home, ".profile"); @@ -105,26 +110,22 @@ function generateSnapshotScript(shell: string, snapshotPath: string, rcFile: str // Escape the snapshot path for shell const escapedPath = snapshotPath.replace(/'/g, "'\\''"); - // Function extraction differs between bash and zsh - const functionScript = isZsh - ? ` -echo "# Functions" >> "$SNAPSHOT_FILE" -# Force autoload all functions first + // Function extraction differs between bash and zsh. Each form prints function + // bodies on stdout so we can both persist them AND scan their bodies for + // referenced env vars (issue #3470). + const functionExtractor = isZsh + ? `# Force autoload all functions first typeset -f > /dev/null 2>&1 # Get user function names - filter system/private ones typeset +f 2>/dev/null | grep -vE '^(_|__)' | grep -vE '${commonToolsRegex}' | while read func; do - typeset -f "$func" >> "$SNAPSHOT_FILE" 2>/dev/null -done -` - : ` -echo "# Functions" >> "$SNAPSHOT_FILE" -# Force autoload all functions first + typeset -f "$func" 2>/dev/null +done` + : `# Force autoload all functions first declare -f > /dev/null 2>&1 # Get user function names - filter system/private ones declare -F 2>/dev/null | cut -d' ' -f3 | grep -vE '^(_|__)' | grep -vE '${commonToolsRegex}' | while read func; do - declare -f "$func" >> "$SNAPSHOT_FILE" 2>/dev/null -done -`; + declare -f "$func" 2>/dev/null +done`; // Shell options extraction const optionsScript = isZsh @@ -151,7 +152,24 @@ echo "# Shell snapshot - generated by omp agent" >| "$SNAPSHOT_FILE" # Unalias everything first to avoid conflicts when sourced echo "unalias -a 2>/dev/null || true" >> "$SNAPSHOT_FILE" -${functionScript} +# Capture function definitions into a variable so we can both persist them +# and scan their bodies for env-var references (issue #3470). +__omp_funcs=$( +${functionExtractor} +) +echo "# Functions" >> "$SNAPSHOT_FILE" +printf '%s\\n' "$__omp_funcs" >> "$SNAPSHOT_FILE" + +# Re-export uppercase identifiers referenced by snapshotted functions whose +# value is set in the rc-sourced shell. Without this, activation idioms like +# mise's \`mise()\` -> \`command "$__MISE_EXE" "$@"\` blow up because the helper +# var is lost (issue #3470). Helper definitions are POSIX-shell so the same +# block works for both bash and zsh. +echo "# Captured function environment" >> "$SNAPSHOT_FILE" +${fnEnvHelper} +printf '%s\\n' "$__omp_funcs" | __omp_emit_referenced_exports >> "$SNAPSHOT_FILE" +unset -f __omp_sq_quote __omp_emit_export_for __omp_emit_referenced_exports 2>/dev/null +unset __omp_funcs __omp_qbuf __omp_qout __omp_sq __omp_xv __omp_name 2>/dev/null ${optionsScript} @@ -198,7 +216,7 @@ export async function getOrCreateSnapshot( return null; } - const rcFile = getShellConfigFile(shell); + const rcFile = getShellConfigFile(shell, env); // Create snapshot directory const snapshotDir = path.join(os.tmpdir(), "omp-shell-snapshots"); diff --git a/packages/coding-agent/test/shell-snapshot.test.ts b/packages/coding-agent/test/shell-snapshot.test.ts index 7806ff335..87349ee13 100644 --- a/packages/coding-agent/test/shell-snapshot.test.ts +++ b/packages/coding-agent/test/shell-snapshot.test.ts @@ -1,5 +1,9 @@ import { describe, expect, it } from "bun:test"; -import { sanitizeSnapshotForBrush } from "@oh-my-pi/pi-coding-agent/utils/shell-snapshot"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { getOrCreateSnapshot, sanitizeSnapshotForBrush } from "@oh-my-pi/pi-coding-agent/utils/shell-snapshot"; +import fnEnvHelper from "../src/utils/shell-snapshot-fn-env.sh" with { type: "text" }; // `sanitizeSnapshotForBrush` is the snapshot-side mitigation for brush's // whitespace-only alias expander (`crates/brush-core-vendored/src/interp.rs:1500`, @@ -78,3 +82,129 @@ describe("sanitizeSnapshotForBrush", () => { } }); }); + +// `__omp_emit_referenced_exports` (shell-snapshot-fn-env.sh) re-exports env +// vars that snapshotted functions reference. mise activates a `mise()` shell +// function whose body expands `$__MISE_EXE`; the snapshot used to persist the +// function but discard the sidecar var, so the replay shell ran +// `command "" "$@"` and died with `command: command not found:` (issue #3470). + +async function readStream(stream: ReadableStream | null): Promise { + if (!stream) return ""; + return await new Response(stream).text(); +} + +describe("shell-snapshot fn-env helper", () => { + it("emits export lines for env vars referenced by captured functions, skips unset and shell-internal names", async () => { + const funcs = [ + `mise () { command "$__MISE_EXE" "$@"; }`, + // biome-ignore lint/suspicious/noTemplateCurlyInString: literal shell parameter expansion `${FOO_TEST_DIR}` + 'my_fn () { echo "$FOO_TEST_DIR ${FOO_TEST_DIR}"; }', + `uses_path () { echo "$PATH"; }`, + `uses_locale () { echo "$LC_ALL"; }`, + `secret () { : "$NEVER_SET_TEST_VAR"; }`, + ``, + ].join("\n"); + + const child = Bun.spawn(["bash", "-c", `${fnEnvHelper}\n__omp_emit_referenced_exports`], { + env: { + PATH: process.env.PATH ?? "/usr/bin:/bin", + __MISE_EXE: "/opt/echo", + FOO_TEST_DIR: "/opt/dir", + LC_ALL: "C", + }, + stdin: "pipe", + stdout: "pipe", + stderr: "pipe", + }); + child.stdin.write(funcs); + await child.stdin.end(); + const out = await readStream(child.stdout as ReadableStream | null); + await child.exited; + expect(child.exitCode).toBe(0); + + expect(out).toContain("export __MISE_EXE='/opt/echo'"); + expect(out).toContain("export FOO_TEST_DIR='/opt/dir'"); + // Shell-internal names must never be re-exported. + expect(out).not.toMatch(/^export PATH=/m); + expect(out).not.toMatch(/^export LC_ALL=/m); + // Unset names produce no line. + expect(out).not.toContain("NEVER_SET_TEST_VAR"); + }); + + it("single-quote-escapes values containing apostrophes and preserves newlines", async () => { + const funcs = `shout () { echo "$TRICKY_VAL $NL_VAL"; }\n`; + const child = Bun.spawn(["bash", "-c", `${fnEnvHelper}\n__omp_emit_referenced_exports`], { + env: { + PATH: process.env.PATH ?? "/usr/bin:/bin", + TRICKY_VAL: "it's 'tricky'", + NL_VAL: "line1\nline2", + }, + stdin: "pipe", + stdout: "pipe", + stderr: "ignore", + }); + child.stdin.write(funcs); + await child.stdin.end(); + const out = await readStream(child.stdout as ReadableStream | null); + await child.exited; + + expect(out).toContain(`export TRICKY_VAL='it'\\''s '\\''tricky'\\'''`); + expect(out).toContain(`export NL_VAL='line1\nline2'`); + + // Eval the emitted lines and verify the round-trip values match. + const round = Bun.spawn( + ["bash", "-c", `eval "$1"; printf '%s\\n' "$TRICKY_VAL"; printf '%s\\n' "$NL_VAL"`, "_", out], + { stdout: "pipe", stderr: "ignore" }, + ); + const echoed = await readStream(round.stdout as ReadableStream | null); + await round.exited; + expect(echoed).toBe("it's 'tricky'\nline1\nline2\n"); + }); +}); + +describe("getOrCreateSnapshot", () => { + it("re-exports env vars referenced by snapshotted functions (issue #3470)", async () => { + const home = await fs.mkdtemp(path.join(os.tmpdir(), "omp-snap-3470-")); + await fs.writeFile( + path.join(home, ".bashrc"), + [ + `export __MISE_EXE=/usr/bin/echo`, + `export FOO_TEST_DIR=/opt/foo`, + `mise () { command "$__MISE_EXE" "$@"; }`, + `shout () { echo "$FOO_TEST_DIR"; }`, + ``, + ].join("\n"), + ); + + // Symlink bash under a unique path so this call misses the process-wide + // snapshot cache (`cachedSnapshotPaths` is keyed on the shell path, and + // sibling tests in the same worker create a cached `/usr/bin/bash` entry + // from a different HOME before this test runs). + const realBash = "/usr/bin/bash"; + const shellLink = path.join(home, "bash-omp-3470"); + await fs.symlink(realBash, shellLink); + + // `getShellConfigFile` honours `env.HOME` so the spawned shell sources + // our fixture bashrc rather than the test runner's home. + const env = { ...process.env, HOME: home }; + const snapshotPath = await getOrCreateSnapshot(shellLink, env); + expect(snapshotPath).not.toBeNull(); + const content = await fs.readFile(snapshotPath!, "utf8"); + + expect(content).toContain(`export __MISE_EXE='/usr/bin/echo'`); + expect(content).toContain(`export FOO_TEST_DIR='/opt/foo'`); + + // Replay the snapshot in a fresh bash with `set -u` and confirm the + // mise() function resolves cleanly instead of dying on the empty var. + const replay = Bun.spawn( + [realBash, "--noprofile", "--norc", "-c", `set -u; source "$1"; mise hello world; shout`, "_", snapshotPath!], + { stdout: "pipe", stderr: "pipe" }, + ); + const stdout = await readStream(replay.stdout as ReadableStream | null); + const stderr = await readStream(replay.stderr as ReadableStream | null); + await replay.exited; + expect({ exitCode: replay.exitCode, stderr }).toEqual({ exitCode: 0, stderr: "" }); + expect(stdout).toBe("hello world\n/opt/foo\n"); + }); +}); diff --git a/types/assets/index.d.ts b/types/assets/index.d.ts index 75b162801..aba649aa9 100644 --- a/types/assets/index.d.ts +++ b/types/assets/index.d.ts @@ -28,6 +28,11 @@ declare module "*.lark" { export default content; } +declare module "*.sh" { + const content: string; + export default content; +} + declare module "*.bdf" { const content: string; export default content;