Repository navigation
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe hook settings type now supports Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Hook setup follows Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 2 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation Issue Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (1 skipped: 1 unsupported.)
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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Claude hook config path now follows
CLAUDE_CONFIG_DIR.getConfigPath()routes through a newhookConfigDir(); descriptors declaringconfigDirEnvVar(onlyCLAUDE_HOOK_SETTINGS) resolve viadefaultClaudeConfigDir(), so the managed hooks land in$CLAUDE_CONFIG_DIR/settings.json. The fork CLIs (.openclaude,.qoder,.qoder-cn,.qwen,.codebuddy) declare none and keepjoin(homedir(), configDirName)— unchanged. - New
hook-settings.test.ts. Four resolution cases (default dir, relocated dir, surrounding whitespace, fork unaffected) plus an end-to-endClaudeHookService().install()that pinsHOME/USERPROFILEto a temp dir, points the variable at another, and asserts the managed hooks land in that dir'ssettings.jsonwith nothing left in the default one.
I verified the load-bearing external contract against the official Claude Code docs: CLAUDE_CONFIG_DIR overrides the config directory, and settings/hooks are read from <dir>/settings.json (settings, env-vars). I also checked that the repo's global vitest home guard already clears an inherited CLAUDE_CONFIG_DIR, so existing ~/.claude/settings.json hook tests stay hermetic. The remote (getRemoteConfigPath) and managed-script paths are untouched as the PR describes, and ClaudeRuntimePathResolver pins the same inherited dir for structured sessions, so the default-account case is now consistent. The reported in-settings.json env-block configuration remains a documented, out-of-scope follow-up.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
…eads `getConfigPath` joined `homedir()` with `.claude` unconditionally, so when `CLAUDE_CONFIG_DIR` named another dir the managed hooks were written to `~/.claude/settings.json`, which the CLI never opens. Every hook-driven feature then reads inactive; the reported symptom is "Stay awake while an agent is working" staying Inactive (stablyai#25242). Orca already launches an account whose home is not the CLI default by pinning exactly that variable (`claudeConfigDirEnvPatch`), so those sessions ran no hooks at all. Resolve the dir through `defaultClaudeConfigDir()`, the helper the rest of the app uses for this variable, and only for the descriptors that declare one. The fork CLIs keep their own dir names and declare no variable, so their behavior is unchanged. The managed script keeps its absolute path in the default dir, and `writeHooksJson` still creates the target directory. Verified: with the variable set, an install now writes the managed hooks into that dir's settings.json and leaves no settings file in the default dir. Without the production change the new cases fail with `expected 'C:\Users\celti\.claude\settings.json' to be 'tmp\orca-relocated-claude\settings.json'`.
495ffff to
5da5445
Compare
config/tsconfig.cli.json lists the main-process files the CLI project compiles, so the new import made tc:cli fail with TS6307 until the helper was listed.
|
Rebased onto current The rebase run caught a real omission in the first commit: importing Two body fixes as well:
Finally, for full disclosure: #24894 by @ggbdpq fixes the same issue and is two days older. I have compared them on that PR and I am happy to close this one in its favour — see my comment there. |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Registered
claude-config-dir-pin.tsin the CLI TypeScript project. The newdefaultClaudeConfigDirimport inhook-settings.tsleft the helper outsideconfig/tsconfig.cli.json'sinclude, sotc:clifailed with TS6307; the commit lists it alongside the othersrc/main/claude/*entries.tsconfig.tc.cli.jsonextendstsconfig.cli.json, so this is the correct project, andclaude-config-dir-pin.tsimports onlynode:os/node:path, so nothing further is pulled in. I rantsc --noEmit -p config/tsconfig.tc.cli.jsonand it exits 0.
The underlying hook-path change from the previous review is unchanged, and the prior review found no issues. The reported in-settings.json env-block configuration remains a documented, out-of-scope follow-up.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
|
Closing this in favour of #24894. Two reasons, both of them the point rather than courtesy:
The one thing this branch carried that the other does not is the discriminator: this one declares Nothing here is abandoned: the fix for #24873 is in #24894, and the other half of what I traced — the frame that reaches the chat as |
Author: @paolovalletta_
ELI5
Claude can be told to keep its config in a directory other than
~/.claude. Orca ignored that and kept writing its hooks into~/.claude, where Claude never looks. The hooks therefore never ran, and every feature that depends on them — the reported one is "Stay awake while an agent is working" — saw nothing happening.What Changed
Before: with
CLAUDE_CONFIG_DIRset in the environment Orca runs in, its Claude hooks were written to~/.claude/settings.json. The CLI readssettings.jsonfrom the directory that variable names, so the managed hooks were never loaded and no hook event ever reached Orca.After: the hooks are written to
$CLAUDE_CONFIG_DIR/settings.json, next to the settings the CLI actually reads.Mechanism.
getConfigPath()insrc/main/claude/hook-settings.tsbuilt its path asjoin(homedir(), settings.configDirName, 'settings.json'), hardcoding the default directory. It now resolves throughdefaultClaudeConfigDir(), the helper the rest of the app already uses for this variable (env.CLAUDE_CONFIG_DIR?.trim() || join(homedir(), '.claude')), for descriptors that declare one:CLAUDE_HOOK_SETTINGSdeclaresconfigDirEnvVar: 'CLAUDE_CONFIG_DIR'..openclaude,.qoder,.qoder-cn,.qwen,.codebuddy) declare none, so their behavior is unchanged — I could not establish that they honour this variable, and changing them without evidence would be a guess.Two things I deliberately left alone: the managed script keeps its absolute path under the default dir (the hook command invokes it by absolute path, so a relocated settings file still works and no files move), and
getRemoteConfigPath()for SSH hosts is untouched, since its config dir belongs to the remote home and reading the remote environment here is a different mechanism.Why
The app already treats this variable as the way a Claude home is relocated —
claude-config-dir-pin.tsexists to decide when to pin it, andnative-chat/session-file-resolver.tsdocuments the rule as "Why CLAUDE_CONFIG_DIR and not just homedir()" — so the hook installer was the outlier rather than the norm. ReusingdefaultClaudeConfigDir()keeps one resolution rule instead of adding a second.Alternatives considered:
env.CLAUDE_CONFIG_DIRout of the CLI's ownsettings.jsonand follow that. This is what the reported configuration uses, and I did not implement it: the CLI resolves its config directory before it readssettings.json, so a variable declared in that file'senvblock most likely does not relocate where the CLI looks for its own settings (that block is applied to the sessions the CLI spawns). Implementing it on the assumption that it does would install hooks in a directory the CLI may never read. See Limitations.Linked Issue
Fixes #24873
#25242 is a different case and is left open. #24873 states the same defect on its own — the installer writes
~/.claude/settings.jsonwhileCLAUDE_CONFIG_DIRnames another directory — and this change fixes exactly that. #25242 reports the feature failing under a configuration that declares the variable inside~/.claude/settings.json, which this change does not cover (argument above, and in my comment on that issue). If a maintainer or the reporter can confirm the CLI relocates its own settings lookup from that block, following it is a small follow-up on top of this change.Visual Proof
N/ANo UI change: which file the hook installer writes, and therefore whether the hooks run.
Testing
New
src/main/claude/hook-settings.test.ts— four resolution cases (default dir, relocated dir, surrounding whitespace, a fork descriptor unaffected) plus an end-to-end case that installs throughClaudeHookServicewithHOME/USERPROFILEisolated to a temp dir and the variable pointing at another, then asserts the managed hooks are in that directory'ssettings.jsonand that no settings file is left in the default directory.hook-settings.test.tswith the changehook-settings.test.tswith onlyhook-settings.tsrevertedexpected 'C:\Users\celti\.claude\settings.json' to be 'tmp\orca-relocated-claude\settings.json'src/main/claude/+src/main/agent-hooks/(248 files, 2587 tests)tsc -p config/tsconfig.node.jsonpnpm tc(node + cli + web), macOS / Node 24, rebased onto currentmainhook-settings.test.ts, macOS / Node 24, rebased onto currentmainoxlinton both files,check-changed-code-quality.mjsThose two suites carry 36 Windows-only failures that are identical with and without this change (symlink/junction creation in
installer-utils, plus CRLF-sensitive expectations); I verified them A/B rather than assuming. One file (managed-hook-stdin-lifecycle.test.ts) appeared to differ in the first A/B run; it passes 3/3 in isolation both ways, so that was parallel-run interference, not this change.Platforms tested: Windows only. The change is platform-neutral path composition — it never inspects
process.platform— and the default branch is unchanged when the variable is unset.After rebasing onto current
mainthe same checks were re-run on macOS / Node 24 (pnpm tc,hook-settings.test.ts), because the branch had moved 63 commits. That run caught an omission in the first commit: importingclaude-config-dir-pin.tsfromhook-settings.tsmadetc:clifail with TS6307 until the file was listed inconfig/tsconfig.cli.json, which the second commit does.AI Disclosure
Written with an AI coding agent, session model
deepseek-v4.1-flash(provideropencode-go). All commands and results above were produced on this machine.Review
process.envon Windows, which is how the existing helper already behaves.getRemoteConfigPathand the SFTP path are untouched, so remote installs keep their current behavior.mkdirSync(dir, { recursive: true })the writer already used, the atomic temp+rename write is unchanged, and Orca only ever merges its own managed hook entries (applyManagedHooks) into that file. No new input, network, or credential path. The write does not follow the launch-time pinning rules inclaude-config-dir-pin.ts, which exist to keep a child's credential identity stable — those are about the env given to a launched CLI, not about where its settings live.getConfigPath(hook-service.tsgetStatus/install/one more), and thatwriteHooksJsoncreates the target directory, so a relocated directory that does not exist yet is not an error path.Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
settings.jsonenvblock) is not fixed, for the reason in Linked Issue. I could not test it end to end either: it needs a Claude CLI configured that way, and this machine has no such setup. What I could test — that the installer follows the variable when it is in the environment — is covered end to end.claudeConfigDirEnvPatch), but the hook service is one global instance installed with Orca's own environment, so an account's home still is not where hooks land. That is a separate design question (per-account installs) and I have not touched it.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass —typecheck:node,oxlintand the changed-code gate pass; the full suite cannot run here (ensure-native-runtimeneeds MSVC); CI will cover