-
-
Notifications
You must be signed in to change notification settings - Fork 756
fix: security hook cwd extraction and PATH issues (#555, #556) #587
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
fix: security hook cwd extraction and PATH issues (#555, #556) #587
Conversation
The bash_security_hook was checking context.cwd, but the Claude Agent SDK passes cwd in input_data dict, not context object. This caused the hook to always fall back to os.getcwd() which returns the runner directory (apps/backend/) instead of the project directory. According to Claude Agent SDK docs, PreToolUse hooks receive cwd in input_data, not context. The context parameter is reserved for future use in the Python SDK. Fixes AndyMik90#555 Signed-off-by: Hunter Luisi <[email protected]>
When Electron launches from Finder/Dock on macOS, process.env.PATH is minimal and doesn't include user shell paths. This caused tools like dotnet, cargo, etc. to fail with 'command not found'. Solution: 1. Use getAugmentedEnv() in agent-process.ts instead of raw process.env 2. Add /usr/local/share/dotnet and ~/.dotnet/tools to COMMON_BIN_PATHS getAugmentedEnv() already exists and is used throughout the frontend for Git/GitHub/GitLab operations. It adds common tool directories to PATH based on platform. Fixes AndyMik90#556 Signed-off-by: Hunter Luisi <[email protected]>
Pinning electron version (removing caret) so electron-builder can compute the version from installed modules in monorepo setup. Signed-off-by: Hunter Luisi <[email protected]>
📝 WalkthroughWalkthroughThe changes address tool discovery and security hook issues across the backend and frontend. The security hook now reads the project directory from input data instead of context, while the frontend environment is augmented with common tool paths for .NET and homebrew, and Electron is locked to a specific version. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
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 |
Summary of ChangesHello @hluisi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses two critical bugs related to environment configuration and security hook behavior. It ensures that the security hook correctly identifies the project's working directory and that agent processes have a complete Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 Thanks for your first PR!
A maintainer will review it soon. Please make sure:
- Your branch is synced with
develop - CI checks pass
- You've followed our contribution guide
Welcome to the Auto Claude community!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively addresses two critical bugs related to security hook execution context and environment path resolution for agent processes. The fix for cwd extraction in the security hook is correct and aligns with the Claude Agent SDK's behavior. The use of getAugmentedEnv to resolve PATH issues on macOS is a solid improvement. I've made one suggestion to improve the path ordering for darwin to follow standard conventions and avoid potential tool version conflicts. The version pinning for electron is also a good practice for build stability.
| darwin: [ | ||
| '/opt/homebrew/bin', // Apple Silicon Homebrew | ||
| '/usr/local/bin', // Intel Homebrew / system | ||
| '/usr/local/share/dotnet', // .NET SDK | ||
| '/opt/homebrew/sbin', // Apple Silicon Homebrew sbin | ||
| '/usr/local/sbin', // Intel Homebrew sbin | ||
| '~/.local/bin', // User-local binaries (Claude CLI) | ||
| '~/.dotnet/tools', // .NET global tools | ||
| ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current order of paths gives system-wide directories (like /opt/homebrew/bin and /usr/local/bin) higher priority than user-specific directories (~/.local/bin, ~/.dotnet/tools). This is unconventional and can lead to issues where a user's local installation of a tool is ignored in favor of an older system version.
It's standard practice to prepend user-specific binary paths to the PATH to allow users to override system-wide tools. I suggest reordering the darwin paths to prioritize user directories.
| darwin: [ | |
| '/opt/homebrew/bin', // Apple Silicon Homebrew | |
| '/usr/local/bin', // Intel Homebrew / system | |
| '/usr/local/share/dotnet', // .NET SDK | |
| '/opt/homebrew/sbin', // Apple Silicon Homebrew sbin | |
| '/usr/local/sbin', // Intel Homebrew sbin | |
| '~/.local/bin', // User-local binaries (Claude CLI) | |
| '~/.dotnet/tools', // .NET global tools | |
| ], | |
| darwin: [ | |
| '~/.local/bin', // User-local binaries (Claude CLI) | |
| '~/.dotnet/tools', // .NET global tools | |
| '/opt/homebrew/bin', // Apple Silicon Homebrew | |
| '/usr/local/bin', // Intel Homebrew / system | |
| '/usr/local/share/dotnet', // .NET SDK | |
| '/opt/homebrew/sbin', // Apple Silicon Homebrew sbin | |
| '/usr/local/sbin', // Intel Homebrew sbin | |
| ], |
There was a problem hiding this comment.
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 details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
apps/backend/security/hooks.pyapps/frontend/package.jsonapps/frontend/src/main/agent/agent-process.tsapps/frontend/src/main/env-utils.ts
🧰 Additional context used
📓 Path-based instructions (4)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/agent/agent-process.tsapps/frontend/src/main/env-utils.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/main/agent/agent-process.tsapps/frontend/src/main/env-utils.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/agent/agent-process.tsapps/frontend/src/main/env-utils.ts
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/security/hooks.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/security/hooks.py
🧬 Code graph analysis (1)
apps/frontend/src/main/agent/agent-process.ts (1)
apps/frontend/src/main/env-utils.ts (1)
getAugmentedEnv(97-146)
🔇 Additional comments (3)
apps/frontend/package.json (1)
117-117: LGTM: Electron version pinned for build stability.Removing the caret to pin Electron to an exact version is appropriate for resolving electron-builder version computation issues in the monorepo. This ensures consistent builds across environments.
Note: This prevents automatic security updates, so remember to manually update Electron when security patches are released.
apps/frontend/src/main/agent/agent-process.ts (1)
19-19: LGTM: Environment augmentation correctly implemented.The integration of
getAugmentedEnv()properly ensures that common tool paths (dotnet, Homebrew, etc.) are available to spawned processes, even when the app is launched from Finder/Dock. The merge order is correct:
augmentedEnvprovides the base with augmented PATHextraEnvandprofileEnvcan override as needed- Python-specific variables are set last
This change directly addresses the PR objective of ensuring tools in non-standard locations are discoverable.
Also applies to: 59-63
apps/backend/security/hooks.py (1)
68-69: Verify that the Claude Agent SDK passescwdininput_datato PreToolUse hooks, not just as an agent option.The code assumes the SDK populates
input_datawithcwdfrom the agent options, but the hook's docstring documentsinput_dataas containing onlytool_nameandtool_input. If the SDK doesn't passcwdthroughinput_data, the fallback toos.getcwd()will analyze the runner's directory instead of the target project. Confirm with the SDK documentation or the development team thatcwdis passed ininput_data, or modify the implementation to extractcwdthroughcontextor another mechanism.
| darwin: [ | ||
| '/opt/homebrew/bin', // Apple Silicon Homebrew | ||
| '/usr/local/bin', // Intel Homebrew / system | ||
| '/usr/local/share/dotnet', // .NET SDK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Consider adding .NET paths for Linux.
The .NET SDK and global tools paths are correctly added for macOS. However, .NET is also commonly used on Linux, where it's typically installed at:
/usr/share/dotnet(SDK)~/.dotnet/tools(global tools)
🔎 Proposed change to add Linux .NET support
linux: [
'/usr/local/bin',
'/usr/bin', // System binaries (Python, etc.)
+ '/usr/share/dotnet', // .NET SDK
'/snap/bin', // Snap packages
'~/.local/bin', // User-local binaries
+ '~/.dotnet/tools', // .NET global tools
'/usr/sbin', // System admin binaries
],Also applies to: 71-71
🤖 Prompt for AI Agents
In apps/frontend/src/main/env-utils.ts around lines 67 and 71, the PATH list
includes macOS .NET locations but is missing common Linux .NET install
locations; add '/usr/share/dotnet' for the SDK and the current user's tools
folder (e.g. expand to `${HOME}/.dotnet/tools` or use os.homedir()/path.join) to
the array; if using os.homedir() or path.join, add the necessary imports and
ensure the entry only gets added when process.platform === 'linux' (or add
unconditionally if safe).
|
I have read the CLA Document and I hereby sign the CLA |
|
@coderabbitai - Confirmed with official Claude Agent SDK documentation. According to the Python SDK Reference, PreToolUse hooks receive Hook signature: async def hook(input_data: dict, tool_use_id: str | None, context: HookContext)input_data fields (from SDK docs):
context parameter:
This is how the SDK passes the Tested on macOS with C# project - security hook now correctly analyzes the target project directory instead of falling back to |
|
Tip For best results, initiate chat on the files or code changes. @hluisi Thank you for the thorough documentation and confirmation! This clarifies the SDK's hook interface perfectly. The official SDK reference you provided confirms that This is exactly the behavior needed to fix issue #555, and your documentation reference provides solid evidence for the implementation choice. ✏️ Learnings added
🧠 Learnings used |
Summary
Fixes two critical bugs that were incorrectly closed as "working-as-intended":
Changes
1. Fix security hook cwd extraction (
apps/backend/security/hooks.py)Problem: Hook checked
context.cwdbut Claude Agent SDK passescwdininput_datadict.Fix:
Evidence: Claude Agent SDK Python docs show PreToolUse hooks receive
cwdininput_data, notcontext.2. Fix PATH for tools in agent processes (
apps/frontend/src/main/agent/agent-process.ts)Problem: When Electron launches from Finder/Dock,
process.env.PATHis minimal.Fix: Use existing
getAugmentedEnv()infrastructure:Also added
/usr/local/share/dotnetand~/.dotnet/toolstoCOMMON_BIN_PATHS.3. Pin electron version (
apps/frontend/package.json)Changed
^39.2.7→39.2.7so electron-builder can compute version in monorepo.Testing
Verified on macOS with C# project:
dotnetcommands now allowed by security hookdotnetexecutable found immediately (no more "command not found")Related Issues
Closes #555
Closes #556
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.