Skip to content

feat: support custom Claude CLI path and config directory#138

Closed
JingkaiTang wants to merge 3 commits intoop7418:mainfrom
JingkaiTang:main
Closed

feat: support custom Claude CLI path and config directory#138
JingkaiTang wants to merge 3 commits intoop7418:mainfrom
JingkaiTang:main

Conversation

@JingkaiTang
Copy link

Summary

  • Add centralized cli-config.ts module to support custom Claude CLI binary path and config directory (e.g. claude-internal / ~/.claude-internal)
  • Add two new settings in Settings → General: Claude CLI Path and Claude Config Directory
  • Replace all hardcoded ~/.claude references (15 files) with configurable helpers
  • Add backend path validation on save (file check for CLI path, directory check for config dir) with frontend error/success feedback
  • Support ~ tilde expansion in all path settings
  • Full i18n support (en/zh) for all new UI strings

Motivation

For teams using internal forks of Claude Code (e.g. claude-internal with ~/.claude-internal config dir), there was no way to configure CodePilot to use a different CLI binary or config directory. All paths were hardcoded to claude and ~/.claude.

Changes

New file

  • src/lib/cli-config.ts — Centralized config module with expandTilde(), getClaudeConfigDir(), getClaudeBinaryName(), and path helpers for commands/skills/projects/settings/plugins/bin directories

Settings UI

Setting Description Default
Claude CLI Path Custom CLI executable path Auto-detect claude
Claude Config Directory Custom config directory ~/.claude

Replaced hardcoded references (15 files)

  • src/lib/platform.ts — bin dir, binary name
  • src/lib/claude-client.ts — CLI path resolution
  • src/lib/claude-session-parser.ts — projects dir
  • src/app/api/settings/route.ts — settings.json path
  • src/app/api/settings/app/route.ts — validation logic
  • src/app/api/plugins/route.ts, [id]/route.ts, mcp/route.ts, mcp/[name]/route.ts — config dir
  • src/app/api/skills/route.ts, [name]/route.ts — commands/plugins/skills dirs
  • src/components/settings/GeneralSection.tsx — new UI cards
  • src/i18n/en.ts, src/i18n/zh.ts — translation keys

Validation

  • CLI Path: validates target is a file (fs.statSync().isFile())
  • Config Dir: validates target is a directory (fs.statSync().isDirectory())
  • Empty value = clear setting, fall back to defaults
  • Invalid path: setting is saved but shows red warning; runtime falls back to defaults

Test plan

  • Leave both settings empty → app behaves identically to before (no regression)
  • Set CLI Path to a valid claude-internal binary → new chats use that binary
  • Set CLI Path to invalid path → red error shown, falls back to auto-detect
  • Set Config Dir to ~/.claude-internal → skills, plugins, settings read from that directory
  • Set Config Dir to non-existent path → red error shown, falls back to ~/.claude
  • Test ~ expansion: input ~/bin/claude → resolves to /Users/<user>/bin/claude
  • Verify i18n: switch between English and Chinese, all new strings display correctly

🤖 Generated with Claude Code

Copy link
Owner

@op7418 op7418 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: PR #138 — Custom Claude CLI path and config directory

Overall: Well-architected feature. The centralized cli-config.ts module is a clean abstraction, and replacing 15 hardcoded references is thorough work.

Positives

  • Centralized config: cli-config.ts with expandTilde(), getClaudeConfigDir(), getClaudeBinaryName() is the right pattern. Single source of truth for all path resolution.
  • Graceful fallbacks: Invalid or empty settings fall back to defaults. Good resilience.
  • Validation on save: Backend validates file/directory existence before confirming. Shows error feedback in UI.
  • Full i18n: All new strings have en/zh translations.
  • Comprehensive file list: Touching 15 files is a lot, but the changes in each are mechanical (replace hardcoded path with config helper).

Concerns

  1. require() at function call time: Several files use const { getClaudeConfigDir } = require("@/lib/cli-config") inside functions instead of top-level import. This is presumably to avoid circular dependency or module initialization issues, but it adds runtime overhead on every call and bypasses TypeScript's type checking. Consider using top-level imports where possible.

  2. Quote style changes: The PR changes single quotes to double quotes across many files (e.g., 'utf-8'"utf-8"). These are cosmetic reformatting changes that inflate the diff significantly. If this is a formatter change, it should be a separate commit or PR to keep the functional diff clean.

  3. expandTilde() duplication: expandTilde() appears both in cli-config.ts and in src/app/api/settings/app/route.ts. Consider importing from cli-config.ts to avoid duplication.

  4. Security note: The CLI path setting allows users to specify an arbitrary executable. This is intentional for the use case (internal forks), but worth noting that it could be used to run arbitrary binaries. Since this is a desktop app where the user already has full system access, this is acceptable.

  5. getClaudeUserConfigPath(): The PR description mentions support for ~/.claude-internal, but I'd want to verify that getClaudeUserConfigPath() correctly derives .claude-internal.json from a config dir of ~/.claude-internal/. Edge case: what if the config dir doesn't end with a pattern that maps cleanly to a .json file?

Minor

  • The PR description is excellent — clear motivation, change summary, and test plan. 👍

Verdict: Solid feature work. The require() pattern and quote style changes are the main concerns. Recommend addressing the require() imports and splitting cosmetic changes in a follow-up.

@JingkaiTang
Copy link
Author

Thanks for the thorough review! All concerns have been addressed in commit 234e1d8:

1. require() → top-level import

All 10 runtime require("@/lib/cli-config") calls across 6 files have been replaced with top-level import statements. Redundant wrapper functions (e.g. local getClaudeSkillsDir()) were removed where the imported function can be called directly. Also cleaned up unused os imports.

2. expandTilde() duplication ✅

expandTilde() is now exported from cli-config.ts and imported in api/settings/app/route.ts. The duplicate definition has been removed.

3. Quote style changes

The single-to-double quote changes were applied automatically by the editor's formatter (no Prettier config in the project). Happy to add a .prettierrc with singleQuote: true in a follow-up PR if you'd like to enforce a consistent style across the codebase.

4. Security note (CLI path)

Acknowledged — since this is a desktop app where the user already has full system access, arbitrary executable paths are acceptable by design.

5. getClaudeUserConfigPath() edge case

The implementation derives the JSON filename from the config directory basename: ~/.claude-internal/.claude-internal.json. This works correctly for the intended use case. If the config dir has an unusual name that doesn't map cleanly to a .json file, the user would need to use the standard naming convention.

Add centralized configuration module (cli-config.ts) to support
switching between claude and claude-internal (or any custom fork).

Settings UI:
- Claude CLI Path: custom executable path, auto-detect by default
- Claude Config Directory: custom config dir, defaults to ~/.claude

Changes:
- New src/lib/cli-config.ts: centralized config with expandTilde(),
  getClaudeConfigDir(), getClaudeBinaryName(), and path helpers for
  commands/skills/projects/settings/plugins directories
- Replace 15 files' hardcoded ~/.claude and binary references with
  cli-config.ts helpers
- Backend validation on save: CLI path must be a file, config dir
  must be a directory; returns validation result to frontend
- Frontend shows green "saved" or red "invalid" status with
  descriptive error messages
- i18n support for en/zh on all new UI strings
- Replace all runtime require("@/lib/cli-config") calls with top-level
  imports for proper TypeScript type checking and no per-call overhead
- Export expandTilde() from cli-config.ts and remove duplicate definition
  in api/settings/app/route.ts
- Remove unused `os` imports where no longer needed
- Delete redundant wrapper functions (getClaudeSkillsDir local wrappers)
33 test cases covering:
- expandTilde: tilde expansion (~, ~/, ~\, absolute, relative, edge cases)
- getClaudeConfigDir: default vs custom config directory resolution
- getClaudeBinaryName: binary name derivation, Windows extension stripping
- getCustomCliPath: custom path with tilde expansion
- getClaudeUserConfigPath: .json filename derivation from config dir
- Convenience helpers: all subdirs with default and custom config dir
Copy link
Owner

@op7418 op7418 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

结构性问题

1. 源分支是 main

这个 PR 的源分支是 main,目标分支也是 main(main → main)。请创建一个专门的功能分支(例如 feat/custom-cli-path),然后重新提交 PR。直接在 main 分支上开发会导致无法同步上游更新、无法并行开发多个功能等问题。

2. 代码格式变更混入功能改动

PR 中包含大量引号风格变更(单引号 → 双引号),这些纯格式化改动显著增大了 diff(+2,809 / -1,720 中有相当一部分是格式化变更)。建议:

  • 将格式化变更与功能改动分开,作为独立的 PR 提交
  • 或者在功能 PR 中只包含功能性改动,保持已有代码的格式不变

这样可以让 Reviewer 更容易聚焦在实际的功能变更上。

3. 之前 Review 中提到的技术问题

  • require() 替代 import:多个文件中使用运行时 require() 代替顶层 import,绕过了 TypeScript 类型检查
  • expandTilde() 重复定义:在 cli-config.tssettings/app/route.ts 中都有实现,应统一引用

建议的后续步骤

  1. 创建功能分支(如 feat/custom-cli-path),只包含功能性改动
  2. 去掉纯格式化变更(引号风格等),如需统一格式可单独提 PR
  3. require() 改为顶层 import
  4. 统一 expandTilde() 的实现位置

功能设计本身很好,集中化的 cli-config.ts 模块是正确的方向。修正上述问题后可以顺利合并。

@JingkaiTang
Copy link
Author

Closing in favor of a new PR from a dedicated feature branch (feat/custom-cli-path) as requested in review. The new PR removes all formatting noise and contains only functional changes.

@JingkaiTang
Copy link
Author

New PR: #224

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.

2 participants