feat: provider extension surface — dialogs, dynamic commands, rich parts, provider registry - #131
Conversation
…rts, provider registry Wire-additive groundwork for non-Claude backends to expose their full feature set through codeoid (pi harness lands on top of this): - session.ui_request/ui_response/ui_resolved: generic provider-initiated dialogs (select/confirm/input/editor) with daemon-enforced timeouts, attach re-delivery, first-answer-wins, interrupt cancellation, and stall-watchdog integration; ui.dialogs capability + web UiRequestBar - session.commands: provider slash-command catalogs; clients pass catalogued verbs through as prompt text (parseSlash isProviderCommand) - custom_message provider events + PartsView: ContentPart[] finally rendered in the web UI; session.part_action gives ButtonPart its return path (validated against the real message parts) - tool_start.patchableKeys: provider-declared approval-form whitelist generalizing the hardcoded AskUserQuestion case - ProviderRegistry (factories) wired into session creation, replacing the hardcoded provider switch; unknown ids still fall back for resume All additive: PROTOCOL_VERSION unchanged, legacy clients unaffected.
|
Warning Review limit reached
Next review available in: 30 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 (7)
📝 WalkthroughWalkthroughThis PR adds a provider extension surface: provider-initiated UI dialogs, dynamic slash-command discovery, rich renderable content parts with button actions, and provider-declared patchable approval keys. A new factory-based ProviderRegistry replaces hardcoded backend construction, wired through SessionManager/Session, with matching web-side state and components. ChangesProvider Extension Surface
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Daemon
participant Session
participant Provider
Provider->>Session: requestUserInput(UiRequest)
Session->>Client: session.ui_request (capable clients only)
Client->>Daemon: session.ui_response
Daemon->>Session: resolveUiRequestFromClient
Session->>Provider: settle UiResponse
Session->>Client: session.ui_resolved
sequenceDiagram
participant Client
participant Daemon
participant Session
participant Provider
Client->>Daemon: session.part_action(messageId, action, data)
Daemon->>Session: dispatchPartAction
Session->>Provider: handlePartAction(action, data)
Provider-->>Session: result
Session-->>Daemon: ok / not_found / invalid_request
Daemon-->>Client: response
Possibly related issues
Possibly related PRs
🚥 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #131 +/- ##
==========================================
+ Coverage 79.49% 80.15% +0.66%
==========================================
Files 90 90
Lines 15251 15587 +336
==========================================
+ Hits 12124 12494 +370
+ Misses 3127 3093 -34
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![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 (3)
src/tests/provider-registry.test.ts (1)
31-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the fixture to actually prove which field feeds
backingSessionId.
makeInit()uses the same value ("s1") for bothsessionIdandinitialBackingId, so this test can't distinguish whether the gemini/openai factories wirebackingSessionIdfrominit.sessionIdorinit.initialBackingId— a future refactor could swap the field silently and this test would still pass.♻️ Proposed fix
function makeInit(store: Store): ProviderSessionInit { return { sessionId: "s1", workspaceId: "ws1", model: null, - initialBackingId: "s1", + initialBackingId: "backing-s1", store, }; }Also applies to: 83-100
🤖 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/tests/provider-registry.test.ts` around lines 31 - 39, The `makeInit` fixture in `provider-registry.test.ts` currently uses the same value for `sessionId` and `initialBackingId`, so the gemini/openai factory assertions can’t verify which field actually populates `backingSessionId`. Update `makeInit()` to use distinct values for `sessionId` and `initialBackingId`, then adjust the related expectations in the provider factory tests so they explicitly prove `backingSessionId` comes from the intended field in the `gemini` and `openai` factory paths.src/daemon/providers/registry.ts (1)
102-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog level mismatch:
resolve()usesconsole.errorfor an expected fallback, but the docstring calls it a "warn".The comment states unknown provider ids "warn and fall back to the default rather than throw" — this is normal forward-compat behavior (e.g. resume against an older
defaultId), not a real error. Usingconsole.errorhere risks false-positive alerting in production log-monitoring pipelines that key off stderr/error-level output for real incidents.♻️ Proposed fix
resolve(id: string | undefined, logContext: string): ProviderFactory { const requested = id ?? this.defaultId; const factory = this.#factories.get(requested); if (factory) return factory; - console.error( + console.warn( `[codeoid/${logContext}] unknown provider "${requested}" — falling back to ${this.defaultId}`, ); return this.getOrThrow(this.defaultId); }🤖 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/daemon/providers/registry.ts` around lines 102 - 115, `resolve()` in `Registry` is treating the expected unknown-provider fallback as an error by logging with `console.error`, which conflicts with the docstring’s warn-and-fallback behavior. Change the logging in `resolve(id, logContext)` to a warning-level log for the unknown provider case, keeping the same fallback to `this.getOrThrow(this.defaultId)`, and preserve the existing context/message format so forward-compat resume behavior still works without triggering error monitoring.web/src/state/commands.ts (1)
27-59: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTransient failures are cached as permanently-empty, same as "unsupported".
.catch()caches[]unconditionally on any rejection — network blip, WS timeout (timeoutMs: 8_000), or an older daemon rejecting the verb are all treated identically. Since nothing besidesinvalidateCommands()(only called today after rotate) clears the cache, a transient failure on the first fetch permanently disables provider-command passthrough for that session until the user rotates.Consider not caching on transient/timeout-classified failures so the next
ensureCommandscall (e.g. next focus) retries, while still caching[]for a definitive "unsupported verb" rejection.🤖 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 `@web/src/state/commands.ts` around lines 27 - 59, The ensureCommands flow is caching every rejection as an empty command list, which makes transient failures look like unsupported sessions. Update the catch handling in ensureCommands so only a definitive unsupported-verb rejection stores [] in state.bySession, while timeouts/network/transient errors simply clear inflight and allow a later ensureCommands call to retry. Use the existing ensureCommands, getClient, and invalidateCommands paths to keep the retry behavior aligned with session focus changes.
🤖 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/protocol/src/schemas.ts`:
- Around line 120-128: The session.ui_response schema currently allows value,
confirmed, and cancelled to appear together or all be missing, which breaks the
“exactly one payload field” contract. Update sessionUiResponseSchema in
schemas.ts by adding a refine/superRefine that enforces exactly one of those
three optional fields is present. Keep the validation tied to
sessionUiResponseSchema so SessionUiResponseMsg consumers get the
mutual-exclusivity check automatically.
In `@src/daemon/session.ts`:
- Around line 2683-2703: The custom_message handler is letting provider metadata
override the fixed provider message event tag. In session.ts within the switch
case for custom_message, update the object passed to this.#makeMessage so
event.metadata is spread before the fixed event: "provider.message" field,
keeping that tag authoritative regardless of metadata contents.
---
Nitpick comments:
In `@src/daemon/providers/registry.ts`:
- Around line 102-115: `resolve()` in `Registry` is treating the expected
unknown-provider fallback as an error by logging with `console.error`, which
conflicts with the docstring’s warn-and-fallback behavior. Change the logging in
`resolve(id, logContext)` to a warning-level log for the unknown provider case,
keeping the same fallback to `this.getOrThrow(this.defaultId)`, and preserve the
existing context/message format so forward-compat resume behavior still works
without triggering error monitoring.
In `@src/tests/provider-registry.test.ts`:
- Around line 31-39: The `makeInit` fixture in `provider-registry.test.ts`
currently uses the same value for `sessionId` and `initialBackingId`, so the
gemini/openai factory assertions can’t verify which field actually populates
`backingSessionId`. Update `makeInit()` to use distinct values for `sessionId`
and `initialBackingId`, then adjust the related expectations in the provider
factory tests so they explicitly prove `backingSessionId` comes from the
intended field in the `gemini` and `openai` factory paths.
In `@web/src/state/commands.ts`:
- Around line 27-59: The ensureCommands flow is caching every rejection as an
empty command list, which makes transient failures look like unsupported
sessions. Update the catch handling in ensureCommands so only a definitive
unsupported-verb rejection stores [] in state.bySession, while
timeouts/network/transient errors simply clear inflight and allow a later
ensureCommands call to retry. Use the existing ensureCommands, getClient, and
invalidateCommands paths to keep the retry behavior aligned with session focus
changes.
🪄 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: 9da57bfa-90c9-4b77-9097-23bb4b91aa4f
📒 Files selected for processing (29)
CHANGELOG.mdpackages/core/src/slash.test.tspackages/core/src/slash.tspackages/protocol/src/schemas.test.tspackages/protocol/src/schemas.tspackages/protocol/src/types.tssrc/daemon/providers/interface.tssrc/daemon/providers/mock/session-provider.tssrc/daemon/providers/registry.tssrc/daemon/scrollback.tssrc/daemon/server.tssrc/daemon/session-manager.tssrc/daemon/session.tssrc/tests/protocol.test.tssrc/tests/provider-registry.test.tssrc/tests/session-extension-surface.test.tssrc/tests/session-manager-extension-verbs.test.tsweb/src/components/CenterPane.tsxweb/src/components/prompt/PromptBox.tsxweb/src/components/transcript/MessageRow.tsxweb/src/components/transcript/PartsView.test.tsxweb/src/components/transcript/PartsView.tsxweb/src/components/transcript/UiRequestBar.test.tsxweb/src/components/transcript/UiRequestBar.tsxweb/src/state/commands.test.tsweb/src/state/commands.tsweb/src/state/connection.tsweb/src/state/ui-requests.test.tsweb/src/state/ui-requests.ts
…horitative event tag, transient command-fetch retry - session.ui_response schema now enforces exactly one of value/confirmed/ cancelled (ambiguous payloads rejected with invalid_request) - custom_message: spread provider metadata BEFORE the fixed event=provider.message tag so it can't be overridden - web commands store: only permanent rejections (unknown verb / missing scope) cache as empty; transient failures retry on next focus - registry.resolve logs the expected fallback at warn, not error - provider-registry test fixture differentiates sessionId from initialBackingId to prove which feeds backingSessionId
1 participant
What
Wire-additive groundwork for supporting additional agent backends (the pi harness is next) at full fidelity. Four gaps closed, one refactor:
1. Provider-initiated dialogs (
session.ui_request/session.ui_response/session.ui_resolved)A provider (or one of its extensions) can now ask the user something that is not a tool approval — confirm gates, pick-one lists, free text, multi-line editors. Semantics:
timeoutMs(auto-cancel); clients only display the countdownsession.attach(a dialog raised while nobody watched still gets answered)session.ui_resolveddismisses every other copyinterrupt()/destroy()cancel pending dialogs; the turn-stall watchdog pauses while a dialog is pending (same rule as tool approvals)ui.dialogscapability — the daemon only targets clients that declared itUiRequestBarabove the prompt (same slot family asApprovalBar)TurnOpts.requestUserInput(optional, so existing providers/tests compile unchanged)2. Dynamic provider commands (
session.commands)Providers can expose their slash-command catalog (extension commands, prompt templates, skills) via a new optional
AgentProvider.listCommands(). Invocation needs no new verb: clients send"/name args"as plain prompt text and the provider expands it.parseSlashgains anisProviderCommandpassthrough option (built-ins always win); the web UI prefetches the catalog on session focus. Gated on thecommands.dynamiccapability.3. Rich parts, actually rendered +
ButtonPartreturn pathcustom_messageprovider event: standalone provider-authored messages withContentPart[](plain-text fallback incontent), persisted + replayed like any messageparts[](PartsView: code, diff, table, tree, progress, image, anchor, button) — previously the parts system was transported but never rendered anywheresession.part_actionverb activates aButtonPart: the daemon validates the button exists on the real message (clients can't mint arbitrary provider calls), then forwards to the provider's optionalhandlePartAction4. Provider-declared approval forms (
tool_start.patchableKeys)Generalizes the hardcoded AskUserQuestion
updatedInputwhitelist: any backend can declare which input keys a client may patch on approval. The built-in AskUserQuestion rule remains the fallback; undeclared keys are still dropped.5. ProviderRegistry wired in
Session backends now come from a factory registry built once at daemon startup — the previous
ProviderRegistrywas dead code next to a hardcodedswitchinSession. Adding a backend is oneregister()call. Unknown provider ids keep the warn-and-fall-back behavior so resume survives metas written by newer codeoids.Compatibility
Everything is additive:
PROTOCOL_VERSIONunchanged, new message kinds/fields follow the ignore-unknown discipline, new behaviors are capability-gated (ui.dialogs,commands.dynamic), and legacy clients (Telegram, older TUIs) are untouched. The companion codeoid-ui (Rust TUI) PR consumes the same surface.Tests
session-extension-surface.test.ts— dialog lifecycle (broadcast gating, answer, timeout, interrupt, attach re-delivery),custom_messagepersistence,patchableKeyssanitization (declared keys pass, foreign keys dropped, legacy fallback intact), part-action validation, command listingsession-manager-extension-verbs.test.ts— scope enforcement + wire shapes for the three new verbsprovider-registry.test.ts— registration invariants, resolve fallback, default catalogUiRequestBar,PartsView(incl.javascript:anchor sanitization and button dispatch),ui-requests+commandsstores🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes