fix(coding-agent): resolve external-dir paths without opening them (fixes #1416) - #1418
fix(coding-agent): resolve external-dir paths without opening them (fixes #1416)#1418MoerAI wants to merge 2 commits into
Conversation
…ixes code-yeongyu#1416) The permission-system tool_call hook classifies every bash/monitor path token through extractExternalPaths -> isExternalPath -> normalizePath, which walked a non-existent path up to its nearest existing ancestor with fs.realpathSync. Bun implements realpathSync (and realpathSync.native) with open(2)/F_GETPATH instead of lstat/readlink, so classifying a command that merely mentions an autofs trigger such as /home forces an automount. On a wedged auto_home map the open never returns, and because the hook runs on the host main thread the whole TUI, every timer and the codemode bridge stop. normalizePath now resolves symlinks with a realpath(3)-style component walker built on lstat + readlink, so no component is ever opened. Non-existent trailing components are kept verbatim after the last resolvable one and non-ENOENT failures fall back to the unresolved path, preserving the previous observable classification, including symlinked-cwd handling.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fabb45eb0f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const MAX_SYMLINK_HOPS = 40; | ||
|
|
||
| function splitSegments(value: string): string[] { | ||
| return value.split(/[\\/]+/).filter((segment) => segment.length > 0); |
There was a problem hiding this comment.
On POSIX, backslash is a valid filename character rather than a separator, so an external path such as /tmp/project\\outside/file with cwd /tmp/project is rewritten by this helper as /tmp/project/outside/file. isExternalPath() consequently returns false, causing tools such as write to omit the external_directory request and bypass the workspace preset's required approval for this sibling directory. Use platform-specific separator handling so backslashes remain literal on POSIX.
AGENTS.md reference: packages/coding-agent/src/core/extensions/builtin/permission-system/AGENTS.md:L58-L60
Useful? React with 👍 / 👎.
The component walker split on /[\\/]+/, which treats a backslash as a separator. On posix a backslash is a legal filename character, so '/tmp/project\outside/file' was rewritten to '/tmp/project/outside/file' and isExternalPath() reported it as inside '/tmp/project'. Tools such as write then skipped the external_directory request and bypassed the workspace preset's approval for a sibling directory. Every value reaching the splitter is already path.normalize()d, which folds '/' into '\\' on win32 and leaves backslashes untouched on posix, so splitting on path.sep is correct on both platforms. Reported by Codex review on code-yeongyu#1418.
1 participant
Summary
normalizePath()in the permission system's external-directory classifier resolved symlinks withfs.realpathSync. Under Bun that is implemented withopen(2)/F_GETPATH, so classifying a command that merely mentions an autofs trigger (/home,/net), a stalled network mount, or a FIFO forces an automount and can block forever. The hook runs on the host main thread, so the whole TUI freezes.This replaces that walk with a
realpath(3)-style component walker built onlstat+readlink, which never opens the entry it inspects.Fixes #1416.
Root cause
The
tool_callhook classifies everybash/monitorpath token throughextractExternalPaths()->isExternalPath()->normalizePath(). For a path that does not exist,normalizePath()walked up to the nearest existing ancestor, callingfs.realpathSyncat each level:/home/user/work/poll-fdl.sh-> ENOENT ->/home/user/work-> ENOENT ->/home/user-> ENOENT ->realpathSync("/home")lstatandreadlinkare enough to implementrealpath(3)and never open a component, so the classifier no longer touches what it classifies.Changes
packages/coding-agent/src/core/extensions/builtin/permission-system/external-dir.tsnormalizePath()resolves symlinks with anlstat+readlinkcomponent walker (40-hop cycle guard) instead offs.realpathSyncpackages/coding-agent/test/permission/external-dir.test.tsrealpathSync/statSync/existsSyncduring classification, plus symlink, symlink-chain, symlink-cycle and non-existent-tail behaviourpackages/coding-agent/CHANGELOG.md[Unreleased] > FixedentryObservable classification is unchanged by design: non-existent trailing components are kept verbatim after the last resolvable component, and any non-ENOENT failure falls back to the unresolved path, exactly as the previous
realpathSyncversion did. The existing symlinked-cwd case still passes.Reproduction (before fix)
node:fsis swapped for a stub whoserealpathSyncblocks forever for/home*— a stand-in for the wedgedauto_homemap in the issue, since Bun'srealpathSyncnever returns there.The unit seam shows the same thing — one classification issued 15
realpathSynccalls:Verification (after fix)
Real-CLI QA (
.agents/skills/senpi-qa), each asserting~/.senpi/agent/auth.jsonis unchanged:mock-loop --with-toolis the relevant end-to-end channel: it drives a real agent turn through thebashtool, which is exactly the hook that calls this classifier.Evidence captured under
local-ignore/qa-evidence/20260907-1416-external-dir-lstat-walker/(gitignored).Test
packages/coding-agent/test/permission/external-dir.test.ts(5 new cases)bun run --cwd packages/coding-agent test test/permission— 485 passbun run check— cleanNot in scope
permission-system/parsers.tshas a siblingrealpathSync(dirname(resolve(cwd, path)))on themonitorparser. It is the same class of landmine, but it backs the authoritative canonical-parent check, so changing it is a security-semantics decision rather than a mechanical swap. Left untouched here; happy to follow up if you want it moved to the same walker.Summary by cubic
Fixes the external-directory classifier freezing the TUI when a command merely mentions an autofs trigger like
/home, a stalled network mount, or a FIFO.normalizePath()now resolves symlinks with anlstat+readlinkcomponent walker instead offs.realpathSync, so classification never opens the path it inspects. Observable classification behavior is unchanged: non-existent trailing components stay verbatim and non-ENOENT failures fall back to the unresolved path.Bug Fixes
path.seponly, so a backslash in a posix path is treated as a filename character and no longer lets paths like/tmp/project\outside/filebe classified as inside/tmp/project.realpathSync,statSync, orexistsSynccalls during classification.coding-agentchangelog with aFixedentry.Written for commit 80c3380. Summary will update on new commits.