refactor(approvals): extract a shared tool-approval registry - #1141
refactor(approvals): extract a shared tool-approval registry#1141materemias wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe PR adds a shared tool-approval registry with session listing, decision resolution, expiry cancellation, and timer cleanup. The Claude provider now delegates approval state management to this registry. Tests cover lifecycle, metadata, filtering, timestamp handling, and resolver behavior. ChangesTool approval registry
Sequence Diagram(s)sequenceDiagram
participant ClaudeProvider
participant ApprovalRegistry
participant ApprovalResolver
participant SessionClient
ClaudeProvider->>ApprovalRegistry: Register pending approval
SessionClient->>ApprovalRegistry: List approvals for session
ClaudeProvider->>ApprovalRegistry: Resolve approval decision
ApprovalRegistry->>ApprovalResolver: Deliver decision or cancellation
ApprovalRegistry-->>SessionClient: Return pending approval views
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Pull request overview
This PR extracts the pending tool-approval registry into a shared backend utility (server/shared/tool-approval-registry.ts) so provider runtimes can route interactive approvals through a single chat.permission-response → resolveToolApproval path, and fixes a hang by ensuring expired approvals cancel the waiting resolver.
Changes:
- Added a provider-agnostic approval registry with registration, lookup, resolution, and an unref’d expiry sweep interval.
- Updated the Claude runtime to use the shared registry rather than a runtime-local
Map. - Added unit tests covering resolution behavior, expiry cancellation, and the interval
unrefbehavior; updated ESLint boundary config for the new shared file.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| server/shared/tool-approval-registry.ts | New shared registry for pending tool approvals, including expiry sweeping + unref’d timer. |
| server/shared/tests/tool-approval-registry.test.ts | Unit tests for approval lifecycle, expiry cancellation, and interval unref behavior. |
| server/modules/providers/list/claude/claude-runtime.provider.js | Switch Claude runtime from private map to shared approval registry. |
| eslint.config.js | Marks the new shared registry file as an explicit backend boundary element. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| sessionId, | ||
| provider, | ||
| meta, | ||
| receivedAt: meta.receivedAt || meta._receivedAt || new Date(), |
There was a problem hiding this comment.
Fixed in 241a35b — coerced at registration.
You're right that the declared Date buys nothing here: meta arrives from claude-runtime.provider.js, which is still JavaScript, so a string genuinely reaches this line. The consequence was worse than the broken view contract — instanceof Date failing in the sweep made the entry read as unknown age and therefore immortal, which is exactly the hang the sweep exists to prevent.
Registration now pins a real timestamp, rejecting an unparseable Date too (new Date('nope') is an instance whose getTime() is NaN):
function coerceReceivedAt(value: unknown): Date {
const time = value instanceof Date ? value.getTime() : Number.NaN;
return Number.isFinite(time) ? (value as Date) : new Date();
}Covered by "a non-Date receivedAt is pinned to a real timestamp and still expires", which asserts the replayed view carries a real Date and that the entry still expires.
| entry.resolver(decision); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Fixed in 241a35b — both points.
resolveToolApproval now consumes the entry before settling it, and swallows a throwing resolver the way the sweep does:
unregisterApproval(requestId);
try { entry.resolver(decision); } catch { /* resolver already settled */ }
return true;Consuming first is the ordering that matters: a duplicate or late chat.permission-response, or an abort racing the user's click, would otherwise hand a second decision to a resolver that already settled. Both providers' own cleanup stays correct because deleting an absent key is a no-op — Claude unregisters in its finally, and the omp runtime unregisters inside its resolver.
Two tests: "resolveToolApproval settles a request exactly once" (second call returns false, one outcome delivered, and the approval stops being replayed to a resubscribing client) and "a resolver that throws still consumes its approval".
![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 (1)
server/shared/tool-approval-registry.ts (1)
108-114: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftDefine a collision-safe approval identity.
Only Claude currently registers approvals and generates UUID request IDs. Before another provider uses this registry, namespace
requestIdby provider or enforce and test process-wide uniqueness. Otherwise,pendingApprovals.set(requestId, ...)replaces an existing resolver, while the runtime dispatches responses byrequestIdto every provider.🤖 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 `@server/shared/tool-approval-registry.ts` around lines 108 - 114, Update the approval registration around pendingApprovals.set in the tool-approval registry to use a collision-safe identity that includes provider context, or enforce process-wide uniqueness for requestId before insertion. Ensure lookup and response dispatch use the same identity so registering one approval cannot replace another provider’s resolver, and add coverage for cross-provider requestId collisions.
🤖 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 `@server/shared/tool-approval-registry.ts`:
- Around line 44-53: Update sweepExpiredApprovals to validate receivedAt as a
finite timestamp rather than using truthiness, so Date(0) is treated as valid
and expired approvals are resolved and removed. Preserve the existing
cancellation and deletion behavior for all valid timestamps.
---
Nitpick comments:
In `@server/shared/tool-approval-registry.ts`:
- Around line 108-114: Update the approval registration around
pendingApprovals.set in the tool-approval registry to use a collision-safe
identity that includes provider context, or enforce process-wide uniqueness for
requestId before insertion. Ensure lookup and response dispatch use the same
identity so registering one approval cannot replace another provider’s resolver,
and add coverage for cross-provider requestId collisions.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a94ac109-a6b4-4007-9155-6cf0d31864a0
📒 Files selected for processing (4)
eslint.config.jsserver/modules/providers/list/claude/claude-runtime.provider.jsserver/shared/tests/tool-approval-registry.test.tsserver/shared/tool-approval-registry.ts
8c49d08 to
cc314dc
Compare
|
Worked through the automated review. Two real bugs, both fixed in
Tests went 11 → 15. Each new assertion is mutation-checked: reverting the coercion fails exactly the two age tests, reverting the consume fails exactly the two resolution tests, and nothing else moves. Declining the Verification on this branch: typecheck, lint and build clean, 277/277 server tests serialized. Note |
The in-process map of pending tool approvals lived inside claude-runtime.provider.js, so every other provider that wants interactive approvals would have to grow its own copy of the same `chat.permission-response` -> resolver plumbing. Move it to server/shared/tool-approval-registry.ts and let the Claude runtime import register/unregister/resolve/listPending from there. The Claude runtime keeps its own per-request timeout, abort handling and metadata, and still re-exports resolveToolApproval and getPendingApprovalsForSession, so the WebSocket wiring and its behavior are unchanged. Pending rows now also carry the owning provider, which is additive: every consumer types the list as `unknown[]` and forwards it verbatim. Two things the extracted registry fixes while it is being written down: - An expiry sweep now settles the entry as cancelled before deleting it. A decision arriving after the 30-minute window previously left the awaiting turn hanging on a resolver nothing would ever call. - Sweeping only on registration left the last stalled approval pending forever, because nothing else ran the sweep. An unref'd interval bounds that wait and stops itself once no approvals are outstanding, so it never holds the event loop open.
cc314dc to
241a35b
Compare
Extracts the in-process tool-approval map out of the Claude SDK adapter into a provider-agnostic
server/shared/tool-approval-registry.ts, so every provider can route interactive tool approvals through the samechat.permission-response→resolveToolApprovalpath.This is a prerequisite for #1143 (adding
ompas a provider), but it stands on its own: it is the piece that v1.37.0'sruntime.permissionscontract already implies, and it fixes one real hang.What changes
server/shared/tool-approval-registry.ts—registerApproval/unregisterApproval/resolveToolApproval/getPendingApprovalsForSession, plus the 30-minute expiry sweep whose timer isunref'd so it never keeps the process alive.claude-runtime.provider.jsimports those instead of owning a privateMap, and re-exportsresolveToolApproval/getPendingApprovalsForSessionunchanged, so the WebSocket wiring and Claude's behavior are untouched.eslint.config.jsdeclares the new shared file as abackend-shared-type-contractboundary element, which is what keeps the barrel/shared-file discipline mechanical rather than conventional.Bug fixed
The old expiry sweep deleted an expired approval without settling its resolver. A decision arriving after the 30-minute window therefore left the awaiting turn parked forever. The sweep now resolves the entry as
{ cancelled: true }before deleting it, so the turn always unblocks.Tests
server/shared/tests/tool-approval-registry.test.ts— 15 tests, no network, no paid calls:UnknownTool)unregisterApprovaldrops the entry without settling its resolver (the runtime's own cleanup path stays idempotent)DatereceivedAtis pinned to a real timestamp and still expires, and an approval received at the Unix epoch expires like any otherresolveToolApprovalsettles a request exactly once, stops replaying it to a resubscribing client, and survives a resolver that throwsEvery load-bearing assertion is mutation-tested — reverting a fix, or removing the
unref, fails exactly the test that covers it and nothing else.The last four cases came out of this PR's automated review, which found two real bugs in the new registry (both amended into the commit rather than stacked on top, since there is no released behaviour to bisect):
receivedAtwas not guaranteed to be aDate, andinstanceof Datethen failing in the sweep made the entry read as unknown age and therefore immortal — the exact hang the sweep exists to prevent; andresolveToolApprovalneither consumed the entry nor guarded the resolver, so a duplicate or latechat.permission-responsecould deliver a second decision to a resolver that already settled.Verification
npm run typecheck— cleannpm run lint— 0 errorsnpm run build— passesnpx tsx --tsconfig server/tsconfig.json --test --test-concurrency=1 "server/**/*.test.ts" "server/**/*.test.js"One caveat, pre-existing on
main: with the default parallel test workers,server/modules/agent/tests/agent.routes.test.tsdies with Node'sUnable to deserialize cloned data due to invalid or unsupported version. It reproduces identically on a pristinemaincheckout (262/263 there), so I ran the suite serialized rather than papering over it here.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests