feat: support custom Claude CLI path and config directory#138
feat: support custom Claude CLI path and config directory#138JingkaiTang wants to merge 3 commits intoop7418:mainfrom
Conversation
op7418
left a comment
There was a problem hiding this comment.
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.tswithexpandTilde(),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
-
require()at function call time: Several files useconst { getClaudeConfigDir } = require("@/lib/cli-config")inside functions instead of top-levelimport. 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. -
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. -
expandTilde()duplication:expandTilde()appears both incli-config.tsand insrc/app/api/settings/app/route.ts. Consider importing fromcli-config.tsto avoid duplication. -
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.
-
getClaudeUserConfigPath(): The PR description mentions support for~/.claude-internal, but I'd want to verify thatgetClaudeUserConfigPath()correctly derives.claude-internal.jsonfrom a config dir of~/.claude-internal/. Edge case: what if the config dir doesn't end with a pattern that maps cleanly to a.jsonfile?
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.
|
Thanks for the thorough review! All concerns have been addressed in commit 1.
|
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
op7418
left a comment
There was a problem hiding this comment.
结构性问题
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.ts和settings/app/route.ts中都有实现,应统一引用
建议的后续步骤
- 创建功能分支(如
feat/custom-cli-path),只包含功能性改动 - 去掉纯格式化变更(引号风格等),如需统一格式可单独提 PR
- 将
require()改为顶层import - 统一
expandTilde()的实现位置
功能设计本身很好,集中化的 cli-config.ts 模块是正确的方向。修正上述问题后可以顺利合并。
|
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. |
|
New PR: #224 |
Summary
cli-config.tsmodule to support custom Claude CLI binary path and config directory (e.g.claude-internal/~/.claude-internal)~/.claudereferences (15 files) with configurable helpers~tilde expansion in all path settingsMotivation
For teams using internal forks of Claude Code (e.g.
claude-internalwith~/.claude-internalconfig dir), there was no way to configure CodePilot to use a different CLI binary or config directory. All paths were hardcoded toclaudeand~/.claude.Changes
New file
src/lib/cli-config.ts— Centralized config module withexpandTilde(),getClaudeConfigDir(),getClaudeBinaryName(), and path helpers for commands/skills/projects/settings/plugins/bin directoriesSettings UI
claude~/.claudeReplaced hardcoded references (15 files)
src/lib/platform.ts— bin dir, binary namesrc/lib/claude-client.ts— CLI path resolutionsrc/lib/claude-session-parser.ts— projects dirsrc/app/api/settings/route.ts— settings.json pathsrc/app/api/settings/app/route.ts— validation logicsrc/app/api/plugins/route.ts,[id]/route.ts,mcp/route.ts,mcp/[name]/route.ts— config dirsrc/app/api/skills/route.ts,[name]/route.ts— commands/plugins/skills dirssrc/components/settings/GeneralSection.tsx— new UI cardssrc/i18n/en.ts,src/i18n/zh.ts— translation keysValidation
fs.statSync().isFile())fs.statSync().isDirectory())Test plan
claude-internalbinary → new chats use that binary~/.claude-internal→ skills, plugins, settings read from that directory~/.claude~expansion: input~/bin/claude→ resolves to/Users/<user>/bin/claude🤖 Generated with Claude Code