feat(connector): peek-driven elicitation — delegated consent for connectors (SP3a)#143
Conversation
…one of roots.ts) Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
…chActTool Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
…andler Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
… action, keep <=1-per-turn Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
…n) + enable delegated consent Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
… prevent wedge
When brain.runTurn rejected/timed out while a delegated elicit was still
awaiting a human answer, #pendingElicit was never cleared. Every subsequent
elicit then hit the serialisation guard ('if (this.#pendingElicit) return
cancel') and silently denied all future elicitations until process restart.
The fix: in runLoop's inner finally (the same block that clears
#activeConversationId), check whether #pendingElicit belongs to the ending
turn's conversationId and, if so, resolve it with 'deny' and set
#pendingElicit to undefined. The normal handleConsentResponse path clears
#pendingElicit before resolving, so this never double-resolves.
Adds a regression test that parks runTurn, fires an elicit, then rejects
the turn to simulate a timeout — asserts (a) the abandoned elicit resolves
to 'decline' and (b) a subsequent elicit is NOT blocked.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
… patch) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
|
Warning Review limit reached
Next review available in: 32 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds delegated consent elicitation for ChangesDelegated consent elicitation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@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: 2
🧹 Nitpick comments (1)
packages/connector-core/src/mcp.ts (1)
67-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest seam exposed on the public class surface.
__setRequestHandlerForTestmonkey-patches the client'ssetRequestHandlerand is reachable by any consumer of the compiled package, not just tests. Given this class gates a consent-sensitive flow, consider a constructor-injectable client/factory instead of a production method that can override request dispatch. Low practical risk since only first-party connectors currently import this package.🤖 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 `@packages/connector-core/src/mcp.ts` around lines 67 - 79, The test-only request handler override on the MCP client is exposed on the public class surface, so move this seam out of the production API. Update the Mcp client construction path in mcp.ts to accept an injectable client or factory for tests instead of keeping __setRequestHandlerForTest as an instance method, and keep the request-dispatch override isolated to test helpers. Preserve the existing capabilities() behavior and ensure only test code can swap the underlying setRequestHandler implementation.
🤖 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 `@packages/connector-core/src/runtime.ts`:
- Around line 24-32: The global `#activeConversationId` in Runtime is being shared
across overlapping turns, so one conversation can overwrite another’s state and
cause handleElicit to target the wrong thread or cancel the wrong turn. Update
Runtime to track active state per conversation (or serialize runLoop so only one
turn is active at a time), and make runLoop and handleElicit read/write
conversation-scoped state instead of a single slot. Also ensure the cleanup in
runLoop’s finally only clears the matching conversation’s state, using the
existing `#pendingElicit` and start/onMessage flow as the main entry points.
In `@packages/peek-mcp/src/mcp/elicitation.ts`:
- Around line 16-19: The `elicitInput` call in `server.elicitInput` is not
receiving the request timeout, so the SDK still uses its default budget instead
of the module’s `withTimeout(...)` window. Update the `elicitInput` parameter
shape in `elicitation.ts` to accept `timeoutMs`, and thread it through the call
site as `{ timeout: timeoutMs }` so the SDK and bridge share the same timeout
budget.
---
Nitpick comments:
In `@packages/connector-core/src/mcp.ts`:
- Around line 67-79: The test-only request handler override on the MCP client is
exposed on the public class surface, so move this seam out of the production
API. Update the Mcp client construction path in mcp.ts to accept an injectable
client or factory for tests instead of keeping __setRequestHandlerForTest as an
instance method, and keep the request-dispatch override isolated to test
helpers. Preserve the existing capabilities() behavior and ensure only test code
can swap the underlying setRequestHandler implementation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ad98da25-02c8-4a00-9742-0bd8cabab2fd
📒 Files selected for processing (12)
.changeset/peek-elicit-delegated-consent.mdpackages/connector-core/src/mcp.test.tspackages/connector-core/src/mcp.tspackages/connector-core/src/runtime.test.tspackages/connector-core/src/runtime.tspackages/connector-core/src/sdk-brain.test.tspackages/connector-core/src/sdk-brain.tspackages/connector-slack/src/index.tspackages/peek-mcp/src/mcp/elicitation.test.tspackages/peek-mcp/src/mcp/elicitation.tspackages/peek-mcp/src/mcp/server.tspackages/peek-mcp/test/server.elicit.test.ts
…ize turn processing
Finding 1 (CR major): ElicitCapableServer.elicitInput now accepts an optional
second `options?: { timeout?: number }` arg (structurally loose, matching
roots.ts's minimal-shape convention). elicitConsent passes `{ timeout: timeoutMs }`
at the call site so the SDK and the outer withTimeout race on the same budget;
a valid-but-slow approval between ~60s (SDK default) and 120s is honored rather
than folded to `reason:'error'`. New test asserts the options.timeout value is
threaded through.
Finding 2 (CR major): ConnectorRuntime.onMessage routes each turn's body through
a private `#turnChain` promise chain so turns process strictly one at a time.
handleConsentResponse is NOT routed through the chain (consent responses must
remain able to resolve a #pendingElicit while a turn is parked awaiting consent
— serializing them would deadlock). Existing #activeConversationId / #pendingElicit
/ finally-clear-on-conversation-match SP2 suspend/resume path unchanged. Two new
tests: (a) second turn does not start until first completes; (b) elicit fired
during turn 1 targets conversation 1, not 2. Consent-deadlock trace: turn awaits
Promise inside #turnChain → consent arrives on separate handleConsentResponse call
→ resolves #pendingElicit → turn resumes → #turnChain unblocks.
All tests green: @peekdev/mcp 412 passed (40 files), @peekdev/connector-core 41 passed (5 files).
Both packages typecheck clean under exactOptionalPropertyTypes + noUncheckedIndexedAccess.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
SP3a — peek-driven elicitation (delegated consent for connectors)
Adds a second consent surface for connectors (headless MCP clients like a Slack bot that spawn
@peekdev/mcp): peek-mcp elicits delegated consent from the connector over MCPelicitInput, and the connector relays it to its human via chat. Ordinary MCP clients (Claude Code) are unaffected. Part of the connector-platform arc (SP1 #140, SP2 #142).The loop (end to end)
SdkBrain.delegateActionConsent: true→ the brain runs the action tool-use inline viamcp.callTool('execute_action', …)and stubs sibling tool-uses withis_error.dispatchActToolsees the connector advertisedelicitation.formand calls a defensiveelicitConsenthelper (capability-probe → race a 120 s timeout → degrade, never throws) for theexecute_actiontool; on decline/cancel/timeout it short-circuits to{ verdict:'deny', result:'denied', approver:'user' }before the host bridge runs the action.{capabilities:{elicitation:{form:{}}}}+ registersonElicit; itsConnectorRuntimeposts a Slack consent card and correlates the (conversation-id-less) reverse-request to the active conversation via a single#pendingElicitresolver +#activeConversationId, resolving throughhandleConsentResponse.Design boundaries honored
execute_action— peek does not classify destructive-vs-safe; the extension SW gate + destructive-override remain the backstop. Ride-along tools do not trigger elicitation.bridge.requestexactly as before).delegateActionConsentis false (default); the flag selects the path.Tasks (6 commits, each per-task-reviewed)
d7d3967elicitation.tshelper (clone ofroots.ts's defensive shape) + testsbbf7f89elicitConsentintodispatchActTool(deny short-circuits before bridge) + testsd1bb8afelicitation.form+PeekMcp.onElicit+ testsd0327c8SdkBrain.delegateActionConsent— inline-run + sibling stub (SP2 fallback intact) + tests9c9e160delegateActionConsent:true+ testsf04f0a1#pendingElicitinrunLoopfinally — prevents abandoned-consent wedge + testff41e79@peekdev/mcppatch)Verification
Full local CI green at HEAD: build ✅ · typecheck ✅ (16 pkgs + 2 Astro apps) · lint ✅ (biome, 557 files) · test ✅ (connector-core 39, peek-mcp 411/1-skip, connector-slack 7, all other packages green). A whole-branch opus review confirmed the security-critical invariant: a denied elicitation can never reach
bridge.request;elicitConsentnever throws; no hang on abandoned consent (connector 30 s < peek 120 s + the finally-block deny).Known limitations (intentional, deferred to SP3b)
#activeConversationIdis last-writer-wins; under concurrent conversations a consent card could be mis-targeted (worst case: a human still has to actively approve a mis-addressed card — a stale response can't resolve it; no silent execution).approver:'user', result:'denied'); a dedicatedconnector-elicitapprover + audit-union widening is deferred to SP3b.request_authorizationis largely vestigial for connectors (no side-panel; inline-runs through the bridge — no hang/misbehavior).Design + plan: private repo
docs/specs|plans/2026-07-06-peek-connector-sp3a-elicitation-*.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests