Route agent hooks through project service - #274
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughClaude and Codex hook commands now use project HTTP endpoints instead of CLI hook strings. The metadata server adds ChangesProject hook command migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MetadataServer
participant InteractionRegistry
Client->>MetadataServer: POST /hooks/claude?action=permission-request&sessionId=...
MetadataServer->>InteractionRegistry: register interaction and wait
InteractionRegistry-->>MetadataServer: decision resolved
MetadataServer-->>Client: hookSpecificOutput decision
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/project-hook-command.ts (1)
19-26: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSession id is not URL-encoded while action is.
actionis passed throughencodeURIComponent(Line 16), butsession(from$AIMUX_SESSION_IDor the shell-quoted fallback) is inserted raw into the query string at Line 26. A session id containing&,=,%, or spaces would corrupt the URL or smuggle extra query parameters into the request.🛡️ Proposed fix: URL-encode session server-side or switch to curl's `--data-urlencode`
- `url="$endpoint${route}?action=${action}&sessionId=$session"`, - `printf "%s" "$payload" | curl --silent --show-error --fail --max-time ${timeoutSeconds} -H 'content-type: application/json' --data-binary `@-` "$url" || printf '{}\\n'`, + `url="$endpoint${route}"`, + `printf "%s" "$payload" | curl -G --silent --show-error --fail --max-time ${timeoutSeconds} -H 'content-type: application/json' --data-binary `@-` --data-urlencode "action=${opts.action}" --data-urlencode "sessionId=$session" "$url" || printf '{}\\n'`,Note:
-Gwith--data-binarytogether needs verification against the curl version in use; validate this combination still posts the body while appending encoded query params.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/project-hook-command.ts` around lines 19 - 26, The query string in project-hook-command is building `url` with a raw `sessionId`, while `action` is already encoded, so update the `session` handling in this shell snippet to URL-encode it before concatenating into `url`. Keep the fix localized around the `session`, `endpoint_file`, and `url` construction logic, and preserve the existing `encodeURIComponent` treatment for `action`; if you change the curl invocation, make sure the `curl` flags still append encoded query params correctly with the request body behavior intact.src/metadata-server.ts (1)
1638-1727: 🔒 Security & Privacy | 🔵 TrivialHook endpoints trust any local caller with no additional authentication.
The new
/hooks/claudeand/hooks/codexPOST routes acceptsessionId/session_id/backendSessionId/tool-input payloads from any process able to reach127.0.0.1on this port, with no token/secret check beyond knowing/guessing asessionId.resolveHookSessionIdwill even remap events to a different session if the payload'ssession_idhappens to match another session'sbackendSessionId. This is a comparable trust model to the prior CLI adapter (which also accepted unauthenticated stdin JSON), so it's not a strictly new class of risk, but centralizing this on an always-listening HTTP server (which also setsAccess-Control-Allow-Origin: *elsewhere in this router) widens the blast radius for a malicious local process or browser-based localhost CSRF to spoof permission decisions or fake completion/backend-session data. Worth a deliberate design review now that hooks are network-reachable rather than argv/stdin-scoped.Also applies to: 2978-3004
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/metadata-server.ts` around lines 1638 - 1727, The hook routes currently accept unauthenticated localhost POSTs and trust session identifiers/tool payloads, which allows spoofed events and session remapping. Add an explicit verification step in the hook request flow before `handleClaudeHook` and `handleCodexHook` process payloads, and make `resolveHookSessionId` only remap after the sender is authenticated or the session binding is validated. Update the hook auth model so these endpoints are no longer blindly accepted from any local caller, while preserving the existing event handling paths.src/codex-hooks.ts (1)
50-56: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueTrim the Codex permission-request timeout. This hook only emits telemetry and returns immediately, so the 120s
--max-timelooks unnecessary here; a shorter timeout would fail faster on network issues.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/codex-hooks.ts` around lines 50 - 56, The buildCodexHookCommand function currently gives the "permission-request" action a much longer timeout than needed. Update the timeoutSeconds logic in buildCodexHookCommand so the Codex permission-request path uses a shorter max-time value instead of 120, while keeping the normal timeout for other actions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/project-hook-command.ts`:
- Around line 23-27: The hook command currently swallows every failure path in
the curl request and setup checks, making
SessionStart/Stop/Notification/PermissionRequest diagnostics invisible. Update
the shell snippet in project-hook-command.ts to emit a brief error message to
stderr before returning the fallback JSON when curl is missing, the endpoint
file is unavailable/empty, or the curl call fails. Keep the existing `{}`
fallback behavior, but add failure context around the existing
endpoint/session/url handling and the curl invocation so logs can identify the
broken stage.
---
Nitpick comments:
In `@src/codex-hooks.ts`:
- Around line 50-56: The buildCodexHookCommand function currently gives the
"permission-request" action a much longer timeout than needed. Update the
timeoutSeconds logic in buildCodexHookCommand so the Codex permission-request
path uses a shorter max-time value instead of 120, while keeping the normal
timeout for other actions unchanged.
In `@src/metadata-server.ts`:
- Around line 1638-1727: The hook routes currently accept unauthenticated
localhost POSTs and trust session identifiers/tool payloads, which allows
spoofed events and session remapping. Add an explicit verification step in the
hook request flow before `handleClaudeHook` and `handleCodexHook` process
payloads, and make `resolveHookSessionId` only remap after the sender is
authenticated or the session binding is validated. Update the hook auth model so
these endpoints are no longer blindly accepted from any local caller, while
preserving the existing event handling paths.
In `@src/project-hook-command.ts`:
- Around line 19-26: The query string in project-hook-command is building `url`
with a raw `sessionId`, while `action` is already encoded, so update the
`session` handling in this shell snippet to URL-encode it before concatenating
into `url`. Keep the fix localized around the `session`, `endpoint_file`, and
`url` construction logic, and preserve the existing `encodeURIComponent`
treatment for `action`; if you change the curl invocation, make sure the `curl`
flags still append encoded query params correctly with the request body behavior
intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b788514-d87e-45fe-982d-233d7e83c268
📒 Files selected for processing (13)
src/claude-hooks.test.tssrc/claude-hooks.tssrc/cli-launcher.test.tssrc/codex-hooks.test.tssrc/codex-hooks.tssrc/main.tssrc/metadata-server.interaction.test.tssrc/metadata-server.tssrc/multiplexer/session-launch.test.tssrc/multiplexer/session-launch.tssrc/one-shot-node-inventory.test.tssrc/project-api-contract.tssrc/project-hook-command.ts
💤 Files with no reviewable changes (1)
- src/main.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/codex-hooks.test.ts (1)
19-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale test description.
Test title says "falls back to empty JSON" but the assertions (lines 21-29) no longer verify any fallback behavior — only that the command ends with a trailing quote, per the line-range summary noting
|| echo '{}'fallback was removed. Consider updating theitdescription to match what's actually being asserted (curl-based POST with expected env vars/action, excluding legacy patterns).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/codex-hooks.test.ts` around lines 19 - 30, The test name is stale because it still mentions falling back to empty JSON even though the `buildCodexHookCommand` assertions now only verify the curl-based project service POST and legacy string exclusions. Update the `it(...)` description in `codex-hooks.test.ts` to match the actual behavior being checked, using the `buildCodexHookCommand`-focused assertions and removing any fallback wording that is no longer covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/metadata-server.interaction.test.ts`:
- Around line 277-289: The assertion in the Codex permission hook test is too
weak because optional chaining lets it pass even if the session or derived state
was never recorded. Update the test around loadMetadataState(),
sessions["codex-1"], and derived.attention to first assert the session/derived
object exists, then verify attention is not "needs_response" so the test באמת
confirms telemetry-only behavior without stranded response state.
---
Nitpick comments:
In `@src/codex-hooks.test.ts`:
- Around line 19-30: The test name is stale because it still mentions falling
back to empty JSON even though the `buildCodexHookCommand` assertions now only
verify the curl-based project service POST and legacy string exclusions. Update
the `it(...)` description in `codex-hooks.test.ts` to match the actual behavior
being checked, using the `buildCodexHookCommand`-focused assertions and removing
any fallback wording that is no longer covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 38120d35-f1d8-47f3-ae55-d0be342f5e12
📒 Files selected for processing (4)
src/codex-hooks.test.tssrc/codex-hooks.tssrc/metadata-server.interaction.test.tssrc/metadata-server.ts
💤 Files with no reviewable changes (1)
- src/metadata-server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/codex-hooks.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/project-hook-command.ts`:
- Line 30: The curl hook command uses --url-query in the project-hook-command
path, which breaks on older curl versions and causes the post to fail. Update
the command construction in the hook execution logic to avoid --url-query unless
curl 7.87+ is guaranteed; instead, build the query string into the URL or switch
to --data-urlencode while keeping the existing fail handling and session/action
parameters intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 387ddd74-156d-4e0d-856c-7fe74edeb78b
📒 Files selected for processing (3)
src/codex-hooks.test.tssrc/codex-hooks.tssrc/project-hook-command.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/codex-hooks.test.ts
1 participant
Summary
Verification
Summary by CodeRabbit
metadata-api.txt).curlrequests with action-appropriate timeouts.