Skip to content

fix(claude-code-macos): fails to get response - #346

Merged
lavaman131 merged 1 commit into
mainfrom
lavaman131/hotfix/claude-code-macos
Mar 4, 2026
Merged

fix(claude-code-macos): fails to get response#346
lavaman131 merged 1 commit into
mainfrom
lavaman131/hotfix/claude-code-macos

Conversation

@lavaman131

@lavaman131 lavaman131 commented Mar 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes authentication and response issues when using Claude Code on macOS by improving executable path resolution and config directory handling.

Problem

On macOS, Atomic was failing to get responses from Claude Code due to two authentication-related issues:

  1. Setting CLAUDE_CONFIG_DIR at runtime interfered with Claude's native auth resolution on macOS
  2. Executable path resolution was preferring npm/Bun shims over native installations, preventing auth state reuse

Key Changes

Executable Path Resolution (src/sdk/clients/claude.ts)

  • Added resolveClaudeCodeExecutablePath() function with macOS-specific logic
  • On macOS, now prefers native installations in this order:
    1. Homebrew install (/opt/homebrew/bin/claude)
    2. App bundles (/Applications/Claude Code.app/Contents/MacOS/claude)
    3. User-local binaries (~/.local/bin/claude, ~/bin/claude)
    4. PATH binaries (non-node_modules only)
    5. Falls back to npm/Bun shims if no native install found
  • Added isLikelyNodeModulesClaudePath() to detect and deprioritize package manager shims
  • Added support for ATOMIC_CLAUDE_CODE_EXECUTABLE env var override
  • Includes comprehensive unit tests with fake filesystem

Config Directory Handling (src/utils/claude-config.ts)

  • Removed CLAUDE_CONFIG_DIR runtime override to avoid breaking macOS auth
  • Now only syncs specific directories from ~/.claude: agents, skills, commands
  • Excludes settings, .claude.json, and .git directories to prevent auth conflicts
  • Uses staging directory approach for atomic merges
  • Defaults merged output to ~/.atomic/.claude instead of temp directory

Chat Command (src/commands/chat.ts)

  • Simplified Claude config preparation - now just merges configs without setting env var
  • Added comment explaining why CLAUDE_CONFIG_DIR is no longer set at runtime

Testing

  • Added unit tests for executable path resolution with various scenarios
  • Updated config merging tests to verify selective sync behavior
  • Tests verify .git exclusion and proper precedence handling

Migration Notes

No breaking changes for users. The fix is transparent - users with native Claude Code installs will now automatically have their auth state recognized.

@lavaman131
lavaman131 merged commit c0a32a2 into main Mar 4, 2026
3 checks passed
@lavaman131
lavaman131 deleted the lavaman131/hotfix/claude-code-macos branch March 4, 2026 04:51
@claude

claude Bot commented Mar 4, 2026

Copy link
Copy Markdown

PR Review: fix(claude-code-macos): fails to get response

Summary

This PR addresses macOS authentication issues with Claude Code by:

  1. Preferring native Claude installs over npm/Bun shims on macOS
  2. Removing runtime CLAUDE_CONFIG_DIR override that breaks auth resolution
  3. Limiting config merging to only agents, skills, and commands directories

✅ Strengths

Code Quality

  • Excellent use of dependency injection in resolveClaudeCodeExecutablePath making it highly testable
  • The ClaudeExecutablePathResolutionOptions interface cleanly separates concerns
  • Comprehensive test coverage with 5 well-designed test cases for executable path resolution
  • Good documentation explaining the why behind macOS-specific behavior

Architecture

  • The helper function isLikelyNodeModulesClaudePath is a clean abstraction
  • resolveClaudeExecutableCandidate handles error cases gracefully
  • The environment variable override (ATOMIC_CLAUDE_CODE_EXECUTABLE) provides a useful escape hatch for debugging

⚠️ Concerns

1. Data Loss Risk in claude-config.ts:42-50

When mergedDir defaults to atomicBaseDir (which is the common case), the code performs:

1. Copy atomicBaseDir → stagingDir
2. Merge legacy entries into stagingDir
3. rm -rf mergedDir (= atomicBaseDir!)  ← Destructive
4. Copy stagingDir → mergedDir
5. rm stagingDir

Risk: If the process crashes after step 3 but before step 4 completes, the user's ~/.atomic/.claude directory is lost with no backup.

Suggestion: Consider an atomic rename approach:

const backupDir = join(homeDir, ".atomic", ".tmp", "claude-config-backup");
await rename(mergedDir, backupDir);  // Atomic on same filesystem
try {
  await rename(stagingDir, mergedDir);
  await rm(backupDir, { recursive: true, force: true });
} catch {
  await rename(backupDir, mergedDir);  // Restore on failure
  throw;
}

2. Missing claude-code Binary in macOS Candidates (claude.ts:2569-2585)

The macOS native candidate list doesn't include paths for the claude-code binary name, only claude. However, getBundledClaudeCodePath calls Bun.which("claude-code") as a fallback. Consider adding claude-code variants to native paths for consistency.

3. Test Import Formatting Change (chat.test.ts)

The only change in chat.test.ts is reformatting the import statement. This appears to be an unintentional style change - consider reverting to keep the diff focused.


🔍 Minor Suggestions

  1. claude.ts:2504 - The function isLikelyNodeModulesClaudePath could benefit from a brief JSDoc explaining what constitutes a "node_modules path" (npm, bun, cli.js).

  2. claude-config.ts:6 - CLAUDE_CONFIG_SYNC_ENTRIES uses as const but the type could be more explicit:

    const CLAUDE_CONFIG_SYNC_ENTRIES = ["agents", "skills", "commands"] as const;
    type SyncEntry = typeof CLAUDE_CONFIG_SYNC_ENTRIES[number];
  3. Test Organization - Consider adding a test case for when the resolveRealPath function throws an error to ensure the fallback path is covered.


🧪 Test Coverage Assessment

Area Coverage
Executable path resolution ✅ Comprehensive (5 cases)
Config merging ✅ Good (5 cases including edge cases)
macOS vs Linux behavior ✅ Covered
Error scenarios ⚠️ Partial - missing resolveRealPath error case

📋 Checklist Before Merge

  • Consider adding atomic rename for config directory operations
  • Verify chat.test.ts formatting change is intentional
  • Run bun typecheck and bun lint to verify no regressions
  • Test on actual macOS with both Homebrew and npm-installed Claude

Verdict

Approve with suggestions - The core fix is sound and addresses a real issue with macOS Claude authentication. The refactoring improves testability significantly. The main concern is the data loss risk in the config merging logic which should be addressed before merging to production.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant