Skip to content

Keep Claude hooks scoped to selected config dirs - #6956

Closed
brennanb2025 wants to merge 3 commits into
mainfrom
brennanb2025/fix-claude-usage
Closed

brennanb2025 wants to merge 3 commits into
mainfrom
brennanb2025/fix-claude-usage

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Jun 30, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Keeps Orca-managed Claude hooks in the same Claude config namespace that launch prep selects for the active runtime/account. When Orca launches Claude with a resolved CLAUDE_CONFIG_DIR or WSL config dir, hook settings and scripts are repaired in that same namespace instead of defaulting to ~/.claude.

Boundaries: this does not add a multi-account UI, does not use Claude enterprise managed settings, and does not crawl arbitrary old config dirs. Disable cleanup removes default, ambient, saved-account, recorded WSL system-default, and known persisted WSL default targets only.

Design Approach

  • Parameterized Claude hook settings paths so local hook install/status/remove can target an explicit Claude config dir.
  • Runs account-scoped hook repair after Claude auth prep and before local Claude spawn on both PTY spawn paths.
  • Supports WSL by writing settings through the Windows-accessible config dir while storing Linux-shaped hook commands and importing hook env through WSLENV.
  • Writes a POSIX endpoint file alongside the native endpoint file so long-lived WSL hook scripts can refresh live hook coordinates after an Orca restart.
  • Preserves default no-target hook behavior for settings/status surfaces and OpenClaude.

Review And Verification

  • Design review: completed; confirmed current Claude Code supports config-dir-scoped state and that this should follow the selected runtime config dir rather than introducing a synthetic shared home.
  • Completeness verification: final pass reported implemented with no blockers.
  • Headline behavior status: implemented.
  • Broad app consistency: launch prep remains source of truth; settings/status remain provider-level for default installs; global disable now cleans up known account/runtime namespaces to avoid leaving launch-repaired hooks armed.
  • Perf audit: fixed unnecessary steady-state chmod, avoided the final launch repair status reread, deduped disable cleanup targets, and avoided synchronous WSL discovery during startup cleanup.
  • Code review loop: multiple rounds; final two reviewers returned clean after fixes for WSL inherited env, cleanup lifecycle, endpoint handling, and CLI typecheck boundaries.
  • Merge-confidence validation: final verdict land; Electron launched from this worktree with identity verified, no renderer console errors observed.

Tests

  • pnpm run typecheck
  • pnpm run tc:node
  • pnpm run typecheck:cli
  • pnpm run lint
  • pnpm exec oxlint src/shared/agent-hook-listener.ts src/main/agent-hooks/server.ts src/main/agent-hooks/server.test.ts src/main/agent-hooks/installer-utils.ts src/main/agent-hooks/installer-utils.test.ts src/main/agent-hooks/managed-agent-hook-controls.ts src/main/agent-hooks/managed-agent-hook-controls.test.ts src/main/claude/hook-settings.ts src/main/claude/hook-service.ts src/main/claude/hook-service.test.ts src/main/ipc/pty.ts src/main/ipc/pty.test.ts src/main/providers/local-pty-provider.ts src/main/providers/local-pty-provider.test.ts src/main/daemon/pty-subprocess.ts src/main/daemon/pty-subprocess.test.ts src/main/wsl-env.ts src/main/index.ts src/main/ipc/settings.ts src/main/runtime/orca-runtime.ts
  • pnpm vitest run src/main/agent-hooks/server.test.ts src/main/agent-hooks/installer-utils.test.ts src/main/agent-hooks/managed-agent-hook-controls.test.ts src/main/claude/hook-service.test.ts src/main/ipc/pty.test.ts src/main/providers/local-pty-provider.test.ts src/main/daemon/pty-subprocess.test.ts src/cli/handlers/agent-hooks.test.ts --config config/vitest.config.ts
  • git diff --check

Remaining Gaps

  • No real WSL live-path test was possible on this macOS host.
  • No real Claude account/auth mutation was performed; tests cover the launch prep, settings, script, and env plumbing without touching production Claude accounts.
  • Older Claude builds that ignore CLAUDE_CONFIG_DIR may still degrade; current Claude Code supports this config-dir state model.

Post-PR Stabilization

Completed. CI verify passed on the latest commit. CodeRabbit posted 8 actionable comments on the first pass and 1 actionable follow-up on the second pass; both sets were fixed and the latest CodeRabbit review reports no actionable comments. CodeRabbit still shows a non-blocking repo-wide docstring-coverage warning in its pre-merge summary, but GitHub checks are passing and that warning is unrelated to this PR.

Made with Orca 🐋


Open in Stage

Co-authored-by: Orca <help@stably.ai>
@ghost

ghost commented Jun 30, 2026 •

Copy link
Copy Markdown

@coderabbitai

coderabbitai Bot commented Jun 30, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This PR updates Claude managed-hook installation, status, removal, and target discovery to support host and WSL config paths. It adds POSIX endpoint-file support in the agent hook server and shell-aware endpoint-file writing. WSL environment propagation changes in daemon and local PTY flows now remove stale Claude config imports, add agent-hook keys, and select POSIX endpoint paths when available. PTY launch handling now installs Claude hooks during prepared launches and passes runtime settings into hook-enable updates.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately describes the main change: scoping Claude hooks to selected config dirs.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The PR description covers the main required sections and details, with only minor gaps like an explicit screenshot placeholder and security audit section.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 8

🧹 Nitpick comments (1)
src/main/ipc/pty.test.ts (1)

689-720: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert call order in the “before spawn” tests.

These cases prove hook repair ran, but not that it ran before spawn(). A refactor that moves installation after the launch would still pass.

Suggested assertion
       expect(claudeHookInstallMock).toHaveBeenCalledWith(
         expect.objectContaining({
           configDir: '/tmp/claude-personal',
           runtime: 'host'
         }),
         { verifyStatus: false }
       )
+      expect(claudeHookInstallMock.mock.invocationCallOrder[0]).toBeLessThan(
+        spawnMock.mock.invocationCallOrder[0]
+      )
       expect(spawnMock).toHaveBeenCalled()

Also applies to: 860-899


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 7a1d5e50-bc4e-4044-9630-0036dbdde65b

📥 Commits

Reviewing files that changed from the base of the PR and between bcb381e and 902a0bc.

📒 Files selected for processing (20)
  • src/main/agent-hooks/installer-utils.test.ts
  • src/main/agent-hooks/installer-utils.ts
  • src/main/agent-hooks/managed-agent-hook-controls.test.ts
  • src/main/agent-hooks/managed-agent-hook-controls.ts
  • src/main/agent-hooks/server.test.ts
  • src/main/agent-hooks/server.ts
  • src/main/claude/hook-service.test.ts
  • src/main/claude/hook-service.ts
  • src/main/claude/hook-settings.ts
  • src/main/daemon/pty-subprocess.test.ts
  • src/main/daemon/pty-subprocess.ts
  • src/main/index.ts
  • src/main/ipc/pty.test.ts
  • src/main/ipc/pty.ts
  • src/main/ipc/settings.ts
  • src/main/providers/local-pty-provider.test.ts
  • src/main/providers/local-pty-provider.ts
  • src/main/runtime/orca-runtime.ts
  • src/main/wsl-env.ts
  • src/shared/agent-hook-listener.ts

Comment thread src/main/agent-hooks/installer-utils.ts Outdated
Comment thread src/main/agent-hooks/managed-agent-hook-controls.test.ts
Comment thread src/main/agent-hooks/managed-agent-hook-controls.ts
Comment thread src/main/agent-hooks/managed-agent-hook-controls.ts
Comment thread src/main/daemon/pty-subprocess.test.ts
Comment thread src/main/daemon/pty-subprocess.ts
Comment thread src/main/ipc/pty.ts Outdated
Comment thread src/main/providers/local-pty-provider.test.ts
Co-authored-by: Orca <help@stably.ai>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 97949ffa-b887-4603-969a-f381f2713cdd

📥 Commits

Reviewing files that changed from the base of the PR and between 902a0bc and 378fe7b.

📒 Files selected for processing (9)
  • src/main/agent-hooks/installer-utils.ts
  • src/main/agent-hooks/managed-agent-hook-controls.test.ts
  • src/main/agent-hooks/managed-agent-hook-controls.ts
  • src/main/daemon/pty-subprocess.test.ts
  • src/main/daemon/pty-subprocess.ts
  • src/main/ipc/pty.test.ts
  • src/main/ipc/pty.ts
  • src/main/providers/local-pty-provider.test.ts
  • src/main/providers/local-pty-provider.ts
🚧 Files skipped from review as they are similar to previous changes (8)
  • src/main/providers/local-pty-provider.test.ts
  • src/main/providers/local-pty-provider.ts
  • src/main/daemon/pty-subprocess.test.ts
  • src/main/daemon/pty-subprocess.ts
  • src/main/ipc/pty.test.ts
  • src/main/agent-hooks/managed-agent-hook-controls.test.ts
  • src/main/agent-hooks/managed-agent-hook-controls.ts
  • src/main/ipc/pty.ts

Comment thread src/main/agent-hooks/installer-utils.ts
Co-authored-by: Orca <help@stably.ai>
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Managed account folders now install their own hooks through #26801. The separate case using the user's own configuration folder still needs checking; create a replacement issue before retiring this older repair branch.

The remaining problem is tracked in #27660.

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