feat(plugins): add typed observational lifecycle contract - #40
Conversation
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
✅ Target branch corrected This pull request now targets The |
|
Warning Review limit reached
Next review available in: 36 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: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds a typed plugin registry and request sessions. Sessions normalize and freeze metadata, summarize adapter events, enforce hook timeouts, isolate failures, preserve event streams, and dispatch completion or error hooks. ChangesPlugin system
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PluginRequestSession
participant AdapterEvent
participant Plugin
Client->>PluginRequestSession: create request session
PluginRequestSession->>Plugin: dispatch request hook
AdapterEvent->>PluginRequestSession: provide adapter event
PluginRequestSession->>Plugin: dispatch sanitized event summary
PluginRequestSession-->>Client: yield original event
Client->>PluginRequestSession: complete or report error
PluginRequestSession->>Plugin: dispatch classified result hook
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
abca252 to
a5ba2a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/plugins/index.ts`:
- Around line 141-157: Normalize all usage counts in summarizeUsage—including
inputTokens, outputTokens, totalTokens, and optional cache/reasoning
fields—through nonNegativeInteger. In src/plugins/index.ts lines 385-411,
replace Math.trunc for result.status and error.status with nonNegativeInteger;
add a test covering non-finite token and status values and assert every numeric
delivered-payload field is an integer.
- Around line 141-157: Update summarizeUsage to normalize inputTokens and
outputTokens with the existing nonNegativeInteger helper before including them
in the PluginUsageSummary. Derive totalTokens from the normalized values when
usage.totalTokens is absent, and normalize usage.totalTokens as well when
provided, preserving the existing optional-field handling and estimated flag.
- Around line 179-238: Add an exhaustive never-check and an explicit runtime
fallback after the switch in summarizeAdapterEvent. Ensure every current
AdapterEvent variant remains handled, while future variants trigger a clear
runtime error instead of returning undefined and propagating an invalid
payload.event.
- Around line 133-135: Update byteLength() to use the node:buffer
Buffer.byteLength API with UTF-8 semantics instead of encoder.encode(), adding
the required Buffer import. In summarizeAdapterEvent() or the surrounding
observeAdapterEvents()/PluginRequestSession.dispatch() flow, skip constructing
AfterAdapterEventPayload when PluginRegistry().plugins is empty, while
preserving dispatch behavior for registered plugins.
- Around line 284-289: Update register to preserve prototype-defined hooks and
their instance context for class-based plugins. Replace the spread-copy
construction in register with the existing bind(plugin) behavior, ensuring all
known hook methods remain callable by dispatch and are bound to the original
plugin instance while retaining the current validation and duplicate checks.
In `@tests/plugins.test.ts`:
- Around line 77-106: Add a focused assertion to the existing “disables only the
failing plugin for the rest of the request” test that creates a second session
from the same PluginRegistry after the first session completes, invokes the
broken plugin’s hook, and verifies it runs again while failure state remains
limited to the original PluginRequestSession.
- Around line 69-75: Extend the table-driven test around PluginRegistry.register
to cover the plugin-id regex boundaries: reject an empty id, ids beginning or
ending with ., -, or _, accept interior separators, and verify the 64-character
maximum while rejecting longer ids. Keep the existing duplicate-id and
invalid-id assertions, and ensure each case identifies whether register should
accept or reject it.
- Around line 185-193: Move the Object.isFrozen checks out of the
afterAdapterEvent hook in the observer registration and record each payload’s
frozen state in collections instead. After collect drains all events, assert the
recorded payload, context, and event states in the test body so failures are
reported directly without dispatch failure isolation swallowing them.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 691f5b1b-307f-4c44-b9f0-7695c6f47c76
📒 Files selected for processing (2)
src/plugins/index.tstests/plugins.test.ts
|
Review follow-up (
Every release path runs |
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
Summary
Implements lane 1 of
structure/plugin-metrics-ratelimit-benchmarks.mdas an internal, compile-time plugin contract.beforeAdapterRequest,afterAdapterEvent,onRequestComplete, andonRequestError.Security and privacy boundary
This PR intentionally does not add:
Plugin failures report only
{pluginId, hook, reason}and never include the thrown error or hook payload.Tests
Focused coverage in
tests/plugins.test.ts:Validation
bun run typecheckbun test tests/plugins.test.tsbun run testbun run privacy:scanPR #39 is merged; this PR now targets
devdirectly.Summary by CodeRabbit
Greptile Summary
Adds a typed lifecycle contract for observational plugins, including request-scoped hook dispatch, timeout isolation, immutable redacted payloads, and deterministic registration order.
A class-based plugin can be accepted and listed as registered while its prototype lifecycle methods are silently omitted during registration. As a result, those plugins receive no lifecycle events and no failure notification. This must be corrected before merging.
Confidence Score: 4/5
What T-Rex did
Comments Outside Diff (1)
General comment
OcxPluginclass instance whosebeforeAdapterRequestmethod is defined on its prototype is registered by ID but its lifecycle hook is never invoked. The plugin is neither reported failed nor disabled because dispatch sees no handler.registerstoresObject.freeze({ ...plugin })atsrc/plugins/index.ts:287. Object spread copies only enumerable own properties, excluding class prototype methods. Dispatch readsplugin[hook]at lines 326-329 and therefore skips the absent hook before reaching the unbound call at line 331.OcxPluginclass with a prototype hook that accesses instance state.Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "test(plugins): cover redaction order and..." | Re-trigger Greptile
Context used: