← Back to PRs

#15857: fix: recognize safe shell builtins in exec-approvals allowlist

by LucasAIBuilder open 2026-02-14 00:10 View on GitHub →
size: S
## Problem Shell builtins (`cd`, `echo`, `export`, `pwd`, etc.) cannot be matched by the exec-approvals allowlist because they have no binary on disk. `resolveExecutablePath()` does a PATH lookup and returns `undefined` for builtins. `matchAllowlist()` requires a resolved path, so builtins can never match. Since `evaluateSegments()` requires ALL parts of a compound command chain to pass, one unresolvable builtin fails the entire command — e.g., `cd /project && git status` triggers an approval prompt even though `git` is explicitly allowlisted. **Reproduction:** 1. Configure exec-approvals with `security: "allowlist"`, `ask: "on-miss"` 2. Add `/usr/bin/git` to the allowlist 3. Have the agent run: `cd /workspace/project && git status` 4. Expected: auto-approved (git is allowlisted, cd is a harmless builtin) 5. Actual: approval prompt triggered In practice this makes the allowlist nearly useless for compound commands, since most real-world exec usage starts with `cd`. ## Root Cause `resolveExecutablePath()` (in `exec-approvals-analysis.ts`) does a PATH lookup for the command name. Shell builtins like `cd` exist only in shell memory — there's no `/usr/bin/cd` binary. The function returns `undefined`. Downstream in `exec-approvals-allowlist.ts`, `matchAllowlist()` gates on `resolution?.resolvedPath` being truthy — if it's `undefined`, it returns `null`. `isSafeBinUsage()` has the same gate. There's no code path that can auto-approve a builtin. ## Fix Adds a `SAFE_SHELL_BUILTINS` set (`cd`, `pwd`, `echo`, `printf`, `export`, `unset`) and an `UNSAFE_SHELL_BUILTINS` set (`eval`, `exec`, `source`, `.`) in `src/infra/exec-approvals-allowlist.ts`. A new `isSafeBuiltinSegment()` function checks whether a command segment is a safe builtin by examining the raw command token: - The raw executable has no path separators (not a file path) - It's not in the unsafe set - It is in the safe set Classification is based on the raw token alone, not on resolution state. This means builtins are correctly recognized even when an external binary with the same name exists in PATH (e.g., `/usr/bin/echo`), matching how shells actually resolve builtins — builtins always take precedence. This is integrated into `evaluateSegments()` alongside the existing `match`, `safe`, and `skillAllow` checks, and reports `"builtins"` via the `segmentSatisfiedBy` tracking. Also updates the inline type in `buildSafeBinsShellCommand()` in `exec-approvals-analysis.ts` to include the new `"builtins"` variant. **Design choice — minimal safe set:** We intentionally started with a conservative set of 6 builtins that cover the vast majority of real-world compound commands. Other builtins that could be added later if needed: `pushd`, `popd`, `dirs`, `set`, `declare`, `typeset`, `local`, `readonly`, `let`, `true`, `false`, `test`, `[`, `type`, `hash`, `alias`, `unalias`, `command`, `read`, `return`, `exit`, `break`, `continue`, `shift`, `getopts`, `wait`, `trap`, `umask`, `times`. Happy to expand if preferred. ## Testing 10 new test cases in `src/infra/exec-approvals.test.ts`: - Safe builtin + allowlisted command passes (`cd /tmp && git status`) - Safe builtin recognized even when PATH has a same-named external binary (PATH shadowing regression) - Multiple safe builtins in chain (`cd /dir && export FOO=bar && git commit`) - Builtin-only command with no external binaries (`cd /dir && export FOO=bar`) - Rejects chain with safe builtin + non-allowlisted binary - Rejects dangerous builtins: `eval`, `source`, `exec`, `.` Results: - `pnpm build` — pass - `pnpm check` — pass - Targeted: `pnpm test src/infra/exec-approvals.test.ts` — 63/63 pass - Full unit suite: `pnpm vitest run --config vitest.unit.config.ts` — 4861 pass ## AI Disclosure This PR was written with AI assistance. Testing level: fully tested. We understand the code — the fix follows the same pattern as the existing `safeBins` check in `evaluateSegments()`. - **Original implementation:** Codex 5.3-high (implementation), Claude Opus 4.6 (code review + additional test cases) - **Rebase + PATH shadowing fix:** Codex 5.3-high (rebase onto refactored upstream, PATH shadowing fix, regression test), Claude Opus 4.6 (code review) - **Second rebase + segmentSatisfiedBy integration:** Codex 5.3-high (rebase, type integration), Claude Opus 4.6 (code review) - **Third rebase + Windows guard restoration:** Codex (rebase onto latest main), Claude Opus 4.6 (CI failure analysis, code review) ## Agent Prompt *Updated after rebase onto refactored file structure.* Shell builtins like `cd`, `echo`, and `export` have no binary on disk, so `resolveExecutablePath()` in `exec-approvals-analysis.ts` returns `undefined` and `matchAllowlist()` in `exec-approvals-allowlist.ts` requires a resolved path. Any compound command containing a builtin (e.g., `cd /project && git status`) fails the allowlist check even when the actual executables are allowlisted. Builtins that only affect shell state should be recognized as safe during allowlist evaluation in `evaluateSegments()` (in `exec-approvals-allowlist.ts`), classified by raw command token rather than resolution state, so that PATH-shadowed builtins (e.g., `/usr/bin/echo` existing alongside the shell builtin `echo`) are still correctly treated as builtins. The fix should integrate with the `segmentSatisfiedBy` tracking by reporting `"builtins"` as the satisfaction reason, and the inline type in `buildSafeBinsShellCommand()` (`exec-approvals-analysis.ts`) should also include this variant.

Most Similar PRs