Repository navigation
fix: isolate Claude project hooks from ESM hosts - #3184
Conversation
ECC Tools / Security EvidenceCommit: Security evidence gate passed (success) No security-sensitive scanner-evidence gap detected. Mode: enforce Scanned 4 changed file(s). No missing scanner-evidence signal was detected. Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / PR Risk TaxonomyCommit: PR taxonomy review recommended (neutral) Detected 2 PR taxonomy bucket(s): CI/CD Recommendation, Cost/Token Risk. Scanned 4 changed file(s). Roadmap taxonomy buckets: CI/CD RecommendationCI, dependency, coverage, and contract signals should be routed into follow-up checks or verification work. Signals:
Paths:
Cost/Token RiskAI routing, usage, and token-budget changes should include budget or usage-limit evidence. Signals:
Paths:
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / Reference Set ReadinessCommit: Reference set readiness gaps detected (neutral) Reference evidence present for 0/7 areas (0%) across 4 changed file(s). This check is based on files changed in this PR. Repository-level readiness is still reported by
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / Hosted Promotion ReadinessCommit: Hosted promotion readiness passed (success) No hosted promotion evidence gaps detected across 4 changed file(s); 0 corpus scenarios had matching evidence. This check compares PR file changes against the evaluator/RAG promotion corpus in No evaluator corpus scenarios matched this PR. Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: affaan-m/ECC/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used📓 Path-based instructions (21)Focus on command injection, unsafe subprocess usage, path traversal, SSRF, secret exposure, and missing tests for new CLI behavior.⚙️ CodeRabbit configuration file Files:
Lightweight agents with frequent invocation Pair programming and code generation Worker agents in multi-agent systems Main development work Orchestrating multi-agent workflows Complex coding tasks Complex architectural decisions Maximum rea...📄 CodeRabbit inference engine (.cursor/rules/common-performance.md) Files:
NEVER hardcode secrets in source code - ALWAYS use environment variables or a secret manager📄 CodeRabbit inference engine (.cursor/rules/common-security.md) Files:
No hardcoded secrets (API keys, passwords, tokens) - validate before any commit📄 CodeRabbit inference engine (.cursor/rules/common-security.md) Files:
Package manager detection should support npm, pnpm, yarn, and bun, with configuration via CLAUDE_PACKAGE_MANAGER environment variable or project config.📄 CodeRabbit inference engine (CLAUDE.md) Files:
Always create new objects, never mutate existing ones.📄 CodeRabbit inference engine (.cursor/rules/common-coding-style.md) Files:
Use parameterized queries to prevent SQL injection📄 CodeRabbit inference engine (.cursor/rules/common-security.md) Files:
Implement XSS prevention by sanitizing HTML output📄 CodeRabbit inference engine (.cursor/rules/common-security.md) Files:
All user inputs must be validated Enable CSRF protection on all state-changing endpoints Verify authentication and authorization for all protected endpoints Implement rate limiting on all endpoints to prevent abuse Ensure error messages do...📄 CodeRabbit inference engine (.cursor/rules/common-security.md) Files:
Write tests before implementation (test-driven development); target 80%+ coverage Achieve minimum 80% test coverage across all three layers: Unit, Integration, and E2E Use AAA structure (Arrange / Act / Assert) in tests with descriptive tes...📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Do not hardcode secrets, API keys, passwords, or tokens📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Always create new objects and never mutate in place; return new copies instead Keep files between 200–400 lines typical, with a maximum of 800 lines Extract helpers when a file exceeds 200 lines Handle errors explicitly at every level; neve...📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
HTML output must be sanitized where applicable📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Auto-format JavaScript/TypeScript files using Prettier after edit Warn about `console.log` statements in edited files Check all modified files for `console.log` statements before session ends📄 CodeRabbit inference engine (.cursor/rules/typescript-hooks.md) Files:
Never hardcode secrets; always use environment variables for sensitive credentials like API keys Throw an error when required environment variables are not configured to fail fast and ensure security prerequisites are met📄 CodeRabbit inference engine (.cursor/rules/typescript-security.md) Files:
Use Playwright as the E2E testing framework for critical user flows in TypeScript/JavaScript📄 CodeRabbit inference engine (.cursor/rules/typescript-testing.md) Files:
Use spread operator for immutable updates in TypeScript/JavaScript instead of direct mutation Use async/await with try-catch for error handling in TypeScript/JavaScript Use Zod for schema-based input validation in TypeScript/JavaScript No c...📄 CodeRabbit inference engine (.cursor/rules/typescript-coding-style.md) Files:
Use the ApiResponse interface pattern with generic type parameter: `interface ApiResponse { success: boolean; data?: T; error?: string; meta?: { total: number; page: number; limit: number; } }` Implement custom React hooks following the...📄 CodeRabbit inference engine (.cursor/rules/typescript-patterns.md) Files:
Ensure cross-platform support for Windows, macOS, and Linux via Node.js scripts in the scripts/ directory.📄 CodeRabbit inference engine (CLAUDE.md) Files:
Required environment variables must be validated at startup📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Use parameterized queries for all database writes (no string interpolation) Auth/authz must be checked server-side for every sensitive path Rate limiting must be applied to all public endpoints📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
🪛 ast-grep (0.45.3)tests/scripts/uninstall.test.js[warning] 147-147: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use. (detect-non-literal-fs-filename) [warning] 149-149: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use. (detect-non-literal-fs-filename) [warning] 150-150: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use. (detect-non-literal-fs-filename) [warning] 168-168: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use. (detect-non-literal-fs-filename) [warning] 169-169: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use. (detect-non-literal-fs-filename) [warning] 176-176: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use. (detect-non-literal-fs-filename) [warning] 177-177: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use. (detect-non-literal-fs-filename) tests/scripts/install-apply.test.js[warning] 1070-1070: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use. (detect-non-literal-fs-filename) [warning] 1072-1072: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use. (detect-non-literal-fs-filename) [warning] 1073-1073: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use. (detect-non-literal-fs-filename) [warning] 1080-1080: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use. (detect-non-literal-fs-filename) [warning] 1081-1081: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use. (detect-non-literal-fs-filename) 🔇 Additional comments (1)
📝 SummarySummary by CodeRabbit
WalkthroughThe Claude project installer now writes CommonJS package boundaries for hook and library scripts. Tests verify hook execution in ESM projects, repeat-install state, managed-file removal, and preservation of user-owned Claude configuration. ChangesClaude project hook module boundary
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~15 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Project
participant Installer
participant HookRuntime
participant Filesystem
Project->>Installer: request claude-project install with hooks
Installer->>HookRuntime: plan hooks-runtime operations
HookRuntime->>Filesystem: write CommonJS package boundaries
Filesystem-->>HookRuntime: store managed files and state
Project->>HookRuntime: invoke block-no-verify.js
HookRuntime-->>Project: reject git commit --no-verify
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
ECC Tools / Security EvidenceCommit: Security evidence gate passed (success) No security-sensitive scanner-evidence gap detected. Mode: enforce Scanned 4 changed file(s). No missing scanner-evidence signal was detected. Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / PR Risk TaxonomyCommit: PR taxonomy review recommended (neutral) Detected 2 PR taxonomy bucket(s): CI/CD Recommendation, Cost/Token Risk. Scanned 4 changed file(s). Roadmap taxonomy buckets: CI/CD RecommendationCI, dependency, coverage, and contract signals should be routed into follow-up checks or verification work. Signals:
Paths:
Cost/Token RiskAI routing, usage, and token-budget changes should include budget or usage-limit evidence. Signals:
Paths:
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / Reference Set ReadinessCommit: Reference set readiness gaps detected (neutral) Reference evidence present for 0/7 areas (0%) across 4 changed file(s). This check is based on files changed in this PR. Repository-level readiness is still reported by
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / Hosted Promotion ReadinessCommit: Hosted promotion readiness passed (success) No hosted promotion evidence gaps detected across 4 changed file(s); 0 corpus scenarios had matching evidence. This check compares PR file changes against the evaluator/RAG promotion corpus in No evaluator corpus scenarios matched this PR. Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
What Changed
{"type":"commonjs"}boundaries inside.claude/scripts/hooksand.claude/scripts/libwhen theclaude-projecthooks runtime is selected.claude/package.jsonand.claude/scripts/package.jsonfiles are never overwrittenWhy This Change
Project-scoped Claude hooks are CommonJS files. In a host project with
"type": "module", Node treats those.jsfiles as ESM unless a nearer package boundary exists, causing errors such asmodule is not defined in ES module scope.The boundaries live directly in the installed hooks and library trees, so a user-owned
.claude/scripts/package.jsoncan retain its own module policy without breaking ECC hooks.Closes #3182.
Testing Done
node tests/run-all.js)Latest focused verification:
node tests/scripts/install-apply.test.js— the ESM and user-owned scripts manifest regression passes; 44 passed, 1 unrelated pre-existing failure in theECC_DISABLED_MCPScase (adapter.idis undefined)node tests/scripts/uninstall.test.js— 14 passed, 0 failednode tests/lib/install-targets.test.js— 51 passed, 0 failednode scripts/ci/validate-install-manifests.js— validated 37 modules, 84 components, and 7 profilesnpx eslint scripts/lib/install-targets/claude-project.js tests/scripts/install-apply.test.js tests/scripts/uninstall.test.js— passedgit diff --check— passednpm pack --dry-run --ignore-scripts --json— the CommonJS boundary asset is includedThe full local
npm testrun on the initial fix commit reported 4698 passed and 54 failed. The remaining failures included unavailable WSL/bin/bash, unavailable Python, Windows symlinkEPERM, and the pre-existingECC_DISABLED_MCPSfailure above. The follow-up commit changes only boundary destinations and their regression expectations; focused verification was rerun after that change.Manual ESM fixture verification confirmed that:
.claude/package.jsonand.claude/scripts/package.jsonfiles remain byte-for-byte unchanged across install and reinstallType of Change
fix:Bug fixfeat:New featurerefactor:Code refactoringdocs:Documentationtest:Testschore:Maintenance/toolingci:CI/CD changesSecurity & Quality Checklist
If you changed dependencies or
package.json(bin/files/ deps)Not applicable; no dependency, root
package.json, or lockfile changes.If you added a skill, command, agent, hook, or CLI tool
Not applicable; this PR adds package-scope boundaries for existing hooks, not a new hook or CLI surface.
npm test)Documentation