fix(providers/claude): reject directory paths and expand npm package dirs (#1723)#1737
Merged
Conversation
…dirs The Claude binary resolver validated configured paths with existsSync, which returns true for directories. Users on Windows who installed Claude Code via npm and configured claudeBinaryPath to the npm platform-package directory (e.g. ...\@Anthropic-AI\claude-code-win32-x64) hit a confusing SDK-side ReferenceError ("Claude Code native binary not found at <path>") because the SDK's child_process.spawn(directory) failed with ENOENT. Replace the existence-only check with a pathKind() helper that distinguishes file / directory / missing, and transparently expand a configured directory to the platform-appropriate child executable (claude.exe on Windows, claude on Unix) when present. A directory without the expected binary now produces a directory-specific error that tells the user what to fix. The autodetect branch already targets a file path directly and is unchanged. Fixes #1723
📝 WalkthroughWalkthroughStat-based path classification ( ChangesPath classification and directory expansion
Sequence DiagramsequenceDiagram
participant Test
participant resolveClaudeBinaryPath
participant validateAndExpand
participant pathKind
Test->>resolveClaudeBinaryPath: call with env/config override
resolveClaudeBinaryPath->>validateAndExpand: validate override path
validateAndExpand->>pathKind: classify provided path
pathKind-->>validateAndExpand: 'file' / 'directory' / 'missing'
alt directory
validateAndExpand->>validateAndExpand: expand to claude/claude.exe
validateAndExpand->>pathKind: classify expanded path
pathKind-->>validateAndExpand: 'file' or 'missing'
alt executable found
validateAndExpand-->>resolveClaudeBinaryPath: return executable path
else executable missing
validateAndExpand-->>resolveClaudeBinaryPath: throw directory error
end
else missing
validateAndExpand-->>resolveClaudeBinaryPath: throw missing error
else file
validateAndExpand-->>resolveClaudeBinaryPath: return path as-is
end
resolveClaudeBinaryPath-->>Test: resolved path or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…dex TODO - Add a regression test for pathKind() returning 'missing' on a broken symlink (uses a real tmp symlink so the statSync ENOENT path is actually exercised, not mocked). - Add a TODO marker in the Codex resolver pointing at #1723. The Codex resolver has the identical existsSync-on-directory gap; left unfixed in this PR to avoid scope creep but now discoverable from the file itself when a Codex bug report lands or someone does a deliberate parity pass.
Collaborator
Author
Automated Self-ReviewRan Strengths
Findings + follow-ups (addressed in
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/providers/src/claude/binary-resolver.ts`:
- Around line 40-48: pathKind currently swallows all errors from _statSync and
returns 'missing', masking permission/access errors (EACCES/EPERM) so
validateAndExpand misreports them; update pathKind(path) to only treat ENOENT
(and maybe ENOTDIR) as 'missing' and rethrow any other errors from _statSync (or
return a distinct result that validateAndExpand handles) so callers like
validateAndExpand can surface permission failures—update the function around the
_statSync call and the catch block accordingly, referencing pathKind and its use
by validateAndExpand.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 344254ce-b2f9-467d-82af-8417f665836f
📒 Files selected for processing (3)
packages/providers/src/claude/binary-resolver-dev.test.tspackages/providers/src/claude/binary-resolver.test.tspackages/providers/src/claude/binary-resolver.ts
Comment on lines
+40
to
+48
| export function pathKind(path: string): 'file' | 'directory' | 'missing' { | ||
| try { | ||
| const stat = _statSync(path); | ||
| if (stat.isFile()) return 'file'; | ||
| if (stat.isDirectory()) return 'directory'; | ||
| return 'missing'; | ||
| } catch { | ||
| return 'missing'; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/providers/src/claude/binary-resolver.ts"
echo "== Lines around pathKind() =="
nl -ba "$FILE" | sed -n '1,140p'
echo "== Search for validateAndExpand() usage and error messages =="
rg -n "validateAndExpand|pathKind\\(" "$FILE"
echo "== Lines around validateAndExpand() =="
nl -ba "$FILE" | sed -n '140,260p'Repository: coleam00/Archon
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/providers/src/claude/binary-resolver.ts"
echo "== Lines around pathKind() =="
sed -n '1,160p' "$FILE" | cat -n
echo "== Occurrences of validateAndExpand() and pathKind() in file =="
rg -n "validateAndExpand|pathKind\\(" "$FILE" || true
echo "== Lines around validateAndExpand() =="
# Print a wider range to ensure we capture function definition
sed -n '160,320p' "$FILE" | cat -nRepository: coleam00/Archon
Length of output: 9362
Don’t collapse all statSync failures into "missing"
packages/providers/src/claude/binary-resolver.ts pathKind() catches all _statSync errors and returns 'missing', so validateAndExpand() can incorrectly report permission/access failures (e.g., EACCES/EPERM) as “file does not exist”.
Suggested fix
export function pathKind(path: string): 'file' | 'directory' | 'missing' {
try {
const stat = _statSync(path);
if (stat.isFile()) return 'file';
if (stat.isDirectory()) return 'directory';
return 'missing';
- } catch {
- return 'missing';
+ } catch (error: unknown) {
+ const code = (error as NodeJS.ErrnoException).code;
+ if (code === 'ENOENT' || code === 'ENOTDIR') return 'missing';
+ throw error;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function pathKind(path: string): 'file' | 'directory' | 'missing' { | |
| try { | |
| const stat = _statSync(path); | |
| if (stat.isFile()) return 'file'; | |
| if (stat.isDirectory()) return 'directory'; | |
| return 'missing'; | |
| } catch { | |
| return 'missing'; | |
| } | |
| export function pathKind(path: string): 'file' | 'directory' | 'missing' { | |
| try { | |
| const stat = _statSync(path); | |
| if (stat.isFile()) return 'file'; | |
| if (stat.isDirectory()) return 'directory'; | |
| return 'missing'; | |
| } catch (error: unknown) { | |
| const code = (error as NodeJS.ErrnoException).code; | |
| if (code === 'ENOENT' || code === 'ENOTDIR') return 'missing'; | |
| throw error; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/providers/src/claude/binary-resolver.ts` around lines 40 - 48,
pathKind currently swallows all errors from _statSync and returns 'missing',
masking permission/access errors (EACCES/EPERM) so validateAndExpand misreports
them; update pathKind(path) to only treat ENOENT (and maybe ENOTDIR) as
'missing' and rethrow any other errors from _statSync (or return a distinct
result that validateAndExpand handles) so callers like validateAndExpand can
surface permission failures—update the function around the _statSync call and
the catch block accordingly, referencing pathKind and its use by
validateAndExpand.
Collaborator
Author
PR Review Summary — Multi-AgentSeven specialist agents reviewed the diff. The PR fixes the core bug correctly and CI is green on Ubuntu + Windows + Docker. The main finding is that the fix is incomplete on the autodetect branch — flagged independently by four agents. Critical (must address)
Suggested fix: Replace Important
Suggestions
Documentation Issues
Suggested wording: "…or the npm platform-package directory (e.g. Strengths
VerdictNEEDS FIXES — one critical (autodetect branch parity), four important (EACCES swallowing, misleading comment, test type hygiene, dev-mode test gap, Recommended Actions
|
…adcrumb, doc updates Extends #1723 fix per multi-agent PR review: - Autodetect branch now uses pathKind === 'file' instead of fileExists so a directory at ~/.local/bin/claude no longer slips past validation and crashes the SDK as ENOENT (matches the env/config branches). - pathKind catches now distinguish ENOENT/ENOTDIR from other stat errors (EACCES, ELOOP, etc.) and emit a WARN log line with the error code so operators have a triage breadcrumb for permission issues that would otherwise surface as the misleading "file does not exist". - Extract CLAUDE_BINARY_NAME constant (was duplicated 7 times across source + tests) and export PathKind type so test mockReturnValue calls are type-checked against the union rather than being unknown strings. - Inline expandDirectoryToExecutable into validateAndExpand — single caller, body shorter than its JSDoc. Drop the WHAT-restating first sentence of validateAndExpand's docstring. - Strip the "Wrapped for spyOn parity" clause from pathKind's JSDoc — contradicted the accurate first sentence and implied the design was testability-driven rather than classification-driven. - Align spy declarations to `| undefined` in binary-resolver.test.ts to match the dev-mode file. Drop the now-unused fileExistsSpy. - Add pathKind happy-path tests (real file → 'file', real dir → 'directory'). Without these, a typo like isFile() → isDirectory() would pass every existing test because all resolver tests spy through pathKind and never exercise the real statSync logic. - Add two dev-mode tests for CLAUDE_BIN_PATH-as-directory. The env branch runs validateAndExpand before the BUNDLED_IS_BINARY guard, so dev users get expansion too; pin the contract. - Add a Windows autodetect-rejects-directory regression test. Docs: surface the new directory-accepting behavior so Windows users who install via npm can discover it without re-reading the source.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/providers/src/claude/binary-resolver.ts`:
- Around line 54-58: The catch block in binary-resolver.ts currently logs the
full error and absolute path (variables err and path) which may contain PII;
change the logging in the catch handler for stat failures (inside the try/catch
around fs.stat usage) to avoid emitting the raw err and path — instead extract
and log only non-sensitive fields such as the errno/code and syscall from (err
as NodeJS.ErrnoException) and, if helpful, a redacted hint (e.g., path basename
or a boolean indicating existence) rather than the full absolute path; update
the getLog().warn invocation accordingly so it only includes safe keys like {
code, syscall, redactedPathHint } and the existing message
'claude.path_stat_failed'.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bddd9572-b909-4c03-9e75-322d43fb6462
📒 Files selected for processing (8)
.env.exampleCLAUDE.mdpackages/docs-web/src/content/docs/getting-started/ai-assistants.mdpackages/docs-web/src/content/docs/getting-started/configuration.mdpackages/docs-web/src/content/docs/reference/configuration.mdpackages/providers/src/claude/binary-resolver-dev.test.tspackages/providers/src/claude/binary-resolver.test.tspackages/providers/src/claude/binary-resolver.ts
✅ Files skipped from review due to trivial changes (5)
- CLAUDE.md
- .env.example
- packages/docs-web/src/content/docs/getting-started/configuration.md
- packages/docs-web/src/content/docs/getting-started/ai-assistants.md
- packages/docs-web/src/content/docs/reference/configuration.md
Comment on lines
+54
to
+58
| } catch (err) { | ||
| const code = (err as NodeJS.ErrnoException).code; | ||
| if (code !== 'ENOENT' && code !== 'ENOTDIR') { | ||
| getLog().warn({ err, path, code }, 'claude.path_stat_failed'); | ||
| } |
There was a problem hiding this comment.
Avoid logging full filesystem paths in stat-failure warnings.
Line 57 logs path and raw err; absolute paths often embed usernames/home dirs and can leak PII. Log only non-sensitive diagnostics (e.g., code, syscall) or a redacted hint.
Proposed minimal redaction diff
} catch (err) {
const code = (err as NodeJS.ErrnoException).code;
if (code !== 'ENOENT' && code !== 'ENOTDIR') {
- getLog().warn({ err, path, code }, 'claude.path_stat_failed');
+ const fsErr = err as NodeJS.ErrnoException;
+ getLog().warn(
+ { code, syscall: fsErr.syscall },
+ 'claude.path_stat_failed'
+ );
}
return 'missing';
}As per coding guidelines, "Never log API keys, tokens (mask as token.slice(0, 8) + '...'), user message content, or PII".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (err) { | |
| const code = (err as NodeJS.ErrnoException).code; | |
| if (code !== 'ENOENT' && code !== 'ENOTDIR') { | |
| getLog().warn({ err, path, code }, 'claude.path_stat_failed'); | |
| } | |
| } catch (err) { | |
| const code = (err as NodeJS.ErrnoException).code; | |
| if (code !== 'ENOENT' && code !== 'ENOTDIR') { | |
| const fsErr = err as NodeJS.ErrnoException; | |
| getLog().warn( | |
| { code, syscall: fsErr.syscall }, | |
| 'claude.path_stat_failed' | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/providers/src/claude/binary-resolver.ts` around lines 54 - 58, The
catch block in binary-resolver.ts currently logs the full error and absolute
path (variables err and path) which may contain PII; change the logging in the
catch handler for stat failures (inside the try/catch around fs.stat usage) to
avoid emitting the raw err and path — instead extract and log only non-sensitive
fields such as the errno/code and syscall from (err as NodeJS.ErrnoException)
and, if helpful, a redacted hint (e.g., path basename or a boolean indicating
existence) rather than the full absolute path; update the getLog().warn
invocation accordingly so it only includes safe keys like { code, syscall,
redactedPathHint } and the existing message 'claude.path_stat_failed'.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
assistants.claude.claudeBinaryPathto the npm-distributed platform-package directory (e.g....\@anthropic-ai\claude-code-win32-x64) made the Claude Agent SDK crash withClaude Code native binary not found at <path>. The resolver validated withexistsSync, which returnstruefor directories; the SDK thenspawned the directory and got ENOENT.claude.exe) was not discoverable from the misleading SDK-side error.pathKind()helper distinguishes file / directory / missing, plusexpandDirectoryToExecutable()that transparently maps a configured directory to itsclaude/claude.exechild when present. Env and config branches now share avalidateAndExpand()helper that throws a directory-specific error when the directory is empty (names the expected child filename).existsSyncpattern exists inpackages/providers/src/codex/binary-resolver.ts:18-25but there is no current bug report; deliberately left for a follow-up to honor YAGNI. Public exports ofresolveClaudeBinaryPathandfileExistsare unchanged;pathKindis added.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
resolveClaudeBinaryPathfileExistsresolveClaudeBinaryPathvalidateAndExpandvalidateAndExpandpathKindvalidateAndExpandexpandDirectoryToExecutableexpandDirectoryToExecutablepathKindprovider.ts:512(pathToClaudeCodeExecutable)resolveClaudeBinaryPathstring | undefined)Label Snapshot
risk: lowsize: Score(specificallyproviders)providers:claudeChange Metadata
bugcore(providers)Linked Issue
claudeBinaryPathresolved from config but SDK fails with "native binary not found" #1723Validation Evidence (required)
CLAUDE_BIN_PATHandclaudeBinaryPath).bun run validatereports 3 pre-existing failures in@archon/adapters > telegram-markdown > blockquotesthat reproduce ondev(verified by checking out the file fromdevand re-running the adapter tests — failures persist independent of this diff). Those tests were last touched byd1feab07 fix(adapters): bump telegramify-markdown to 1.3.3; not investigated as part of this PR.Security Impact (required)
NoNoNoNo—statSyncis added in the same code path that already calledexistsSyncon the same configured paths. No new directories or files are stat'd that weren't being existence-checked before.Compatibility / Migration
Yes— every previously-accepted path (a file) still resolves to the same value. The only behavioral change is that a directory now either expands transparently (success path that previously crashed inside the SDK) or throws a clearer Archon-side error (previously a confusing SDK-side error).NoNoHuman Verification (required)
bun --filter @archon/providers testfor both binary-mode and dev-mode resolver test suites — all pass. The directory-expansion tests dynamically pickclaude(Unix) vsclaude.exe(Windows) so they exercise the real branch on this host.fileExists) is unchanged — its target path is built viajoin(homedir(), '.local', 'bin', 'claude{.exe}'), which is always a file by construction, so the directory-case bug cannot arise there.CLAUDE_BIN_PATH=(still falls through to undefined in dev mode — pinned by existing test). Directory with no inner binary (new directory-specific error). Symlink behavior:statSyncfollows symlinks by default, so a symlink-to-file resolves as'file', and a broken symlink resolves as'missing'— matches user expectations....\@anthropic-ai\claude-code-win32-x64). The author does not have a Windows host; verification depends on @wikimatt or another Windows user retesting after merge. The unit tests are platform-conditional but the directory→claude.exeexpansion logic was specifically motivated by the Windows scenario.Side Effects / Blast Radius (required)
resolveClaudeBinaryPath()callers — currently a single caller atpackages/providers/src/claude/provider.ts:862. The output contract is unchanged (Promise<string \| undefined>). The expanded path is a child of the user-supplied directory, so it remains inside whatever filesystem region the user already granted.claudein some unrelated directory and pointed at that directory expecting something else would now see Archon attempt to spawn thatclaude. This is so unlikely (the file would have to be namedclaudeexactly) that it's a deliberately accepted edge case — and the previous behavior in that scenario was already broken (SDK ENOENT on the directory).claude.binary_resolvedlog line now records the expanded path. Operators triaging spawn issues can grep for the actual path the SDK was given.Rollback Plan (required)
git revert <merge-commit>— single-commit revert is safe; no shared-state mutation, no migration.claude.binary_resolvedlog line missing followed byvalidateAndExpanderror. Easy to spot.Risks and Mitigations
statSyncis added to the hot path; a hostile filesystem (e.g. dangling network mount) could now throw whereexistsSyncwould have returnedfalse.pathKind()wrapsstatSyncintry/catchand returns'missing'on any throw — behavior is conservatively a superset of the previousexistsSyncpath.claudebinary (e.g. some unrelated executable namedclaudeon Unix)..claude/PRPs/issues/issue-1723.md) and intentionally deferred. No current bug report; YAGNI.Summary by CodeRabbit
Bug Fixes
Improvements
Tests
Documentation