Skip to content

feat(plugins): add typed observational lifecycle contract - #40

Merged
OnlineChef merged 6 commits into
devfrom
feat/plugin-contract
Aug 2, 2026
Merged

feat(plugins): add typed observational lifecycle contract#40
OnlineChef merged 6 commits into
devfrom
feat/plugin-contract

Conversation

@OnlineChef

@OnlineChef OnlineChef commented Aug 2, 2026

Copy link
Copy Markdown

Summary

Implements lane 1 of structure/plugin-metrics-ratelimit-benchmarks.md as an internal, compile-time plugin contract.

  • Adds typed observational hooks: beforeAdapterRequest, afterAdapterEvent, onRequestComplete, and onRequestError.
  • Preserves deterministic registration order and rejects duplicate or invalid plugin IDs.
  • Creates request-scoped sessions: a hook error or timeout disables only that plugin for the remainder of that request.
  • Applies a bounded hook timeout and keeps diagnostics payload-free.
  • Exposes structural adapter-event summaries only; text, reasoning, signatures, tool names/arguments, IDs, search queries/sources, URLs, and error messages stay outside the plugin boundary.
  • Freezes all hook payloads and never allows hooks to replace, veto, mutate, reorder, or cancel requests/events.
  • Leaves the global registry empty by default; this PR does not dynamically load code or change existing request behavior.

Security and privacy boundary

This PR intentionally does not add:

  • arbitrary filesystem/package plugin loading;
  • prompt rewriting or an injection-string sanitizer;
  • headers, credentials, raw bodies, model IDs, provider account IDs, request IDs, or conversation IDs in hook payloads;
  • cost estimation in plugins.

Plugin failures report only {pluginId, hook, reason} and never include the thrown error or hook payload.

Tests

Focused coverage in tests/plugins.test.ts:

  • deterministic registration order;
  • invalid/duplicate ID rejection;
  • synchronous failure isolation;
  • bounded timeout isolation;
  • redaction for text, tool-call, search, and terminal-error events;
  • frozen payloads;
  • unchanged adapter event identity and ordering;
  • completion/error metadata classification.

Validation

  • bun run typecheck
  • bun test tests/plugins.test.ts
  • bun run test
  • bun run privacy:scan

PR #39 is merged; this PR now targets dev directly.

Summary by CodeRabbit

  • New Features
    • Added a plugin system with request lifecycle hooks and configurable plugin registration.
    • Added request and event metadata summaries, including HTTP method and completion status details.
    • Added per-request plugin sessions with deterministic execution order.
    • Added safeguards that isolate plugin failures, enforce timeouts, and prevent payload modification.

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

T-Rex T-Rex Logs

What T-Rex did

  • T-Rex produced a proof for a posted P1 finding and linked it to the review comment.
  • T-Rex produced a second proof for another posted P1 finding and linked it to its review comment.
  • T-Rex produced a general-contract-validation-proof that verifies the plugin omits inherited methods and that registry dispatch finds no handler for the prototype hook.
  • T-Rex organized and surfaced the related test sources and run outputs as artifacts to help reviewers inspect both the control and registry paths.

View all artifacts

T-Rex Ran code and verified through T-Rex

Comments Outside Diff (1)

  1. General comment

    P1 PluginRegistry silently drops class-instance prototype lifecycle hooks

    • Bug
      • A structurally valid OcxPlugin class instance whose beforeAdapterRequest method 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.
    • Cause
      • register stores Object.freeze({ ...plugin }) at src/plugins/index.ts:287. Object spread copies only enumerable own properties, excluding class prototype methods. Dispatch reads plugin[hook] at lines 326-329 and therefore skips the absent hook before reaching the unbound call at line 331.
    • Fix
      • Retain the original plugin object (freezing it only if mutation protection is required), or explicitly preserve/bind lifecycle hooks when constructing the stored snapshot. Add regression coverage using an OcxPlugin class with a prototype hook that accesses instance state.

    T-Rex Ran code and verified through T-Rex

Fix All in Cursor Fix All in Codex Fix All in Claude Code Fix All in Conductor

Prompt To Fix All With AI
### Issue 1
src/plugins/index.ts:287
**Prototype lifecycle hooks are discarded**

Object spread copies only enumerable own properties, so registering a valid class-instance `OcxPlugin` drops lifecycle methods declared on its prototype. The plugin remains registered, but dispatch finds no handler and silently skips the hook without reporting or disabling it. Preserve the registered instance or explicitly retain and bind its lifecycle hooks when creating the registry entry.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (2): Last reviewed commit: "test(plugins): cover redaction order and..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

Context used:

  • Context used - Focking gretig zijn en niet stoppen tot perfectie.... (source)

@qodo-code-review

Copy link
Copy Markdown

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown

Target branch corrected

This pull request now targets dev.

The [WRONG BRANCH] title prefix has been removed. The pull request has been marked ready for review again.

@github-actions github-actions Bot changed the title feat(plugins): add typed observational lifecycle contract [WRONG BRANCH] feat(plugins): add typed observational lifecycle contract Aug 2, 2026
@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@OnlineChef, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 36 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 2c65f64f-04f6-4b2b-ac3e-465681a97837

📥 Commits

Reviewing files that changed from the base of the PR and between 861157c and 24d3850.

📒 Files selected for processing (2)
  • src/plugins/index.ts
  • tests/plugins.test.ts
📝 Walkthrough

Walkthrough

The 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.

Changes

Plugin system

Layer / File(s) Summary
Plugin contracts and metadata normalization
src/plugins/index.ts, tests/plugins.test.ts
src/plugins/index.ts:1-120 defines exported plugin types and hook payloads. src/plugins/index.ts:122-238 normalizes usage, classifies HTTP methods, and removes sensitive event data. Tests at tests/plugins.test.ts:141-180 verify metadata privacy and event classification.
Registry and request session safety
src/plugins/index.ts, tests/plugins.test.ts
src/plugins/index.ts:240-346 adds deep freezing, timeout enforcement, plugin validation, registration snapshots, sequential dispatch, and failure isolation. Tests at tests/plugins.test.ts:29-138 cover ordering, validation, timeouts, and failed-plugin handling.
Request and event observation
src/plugins/index.ts, tests/plugins.test.ts
src/plugins/index.ts:348-377 observes normalized requests and adapter events, assigns sequence numbers, freezes payloads, and yields original events. Tests at tests/plugins.test.ts:182-222 verify immutability, ordering, stream identity, and usage totals.
Completion and error notifications
src/plugins/index.ts, tests/plugins.test.ts
src/plugins/index.ts:379-415 adds completion and error notifications plus the default registry. Tests at tests/plugins.test.ts:224-256 verify classified metadata without exposing error messages.

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
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the new typed observational plugin lifecycle contract implemented by the pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/plugin-contract

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@OnlineChef
OnlineChef changed the base branch from plan/plugin-metrics-ratelimit-benchmarks to dev August 2, 2026 05:33
@github-actions github-actions Bot changed the title [WRONG BRANCH] feat(plugins): add typed observational lifecycle contract feat(plugins): add typed observational lifecycle contract Aug 2, 2026
@github-actions github-actions Bot added the enhancement New feature or request label Aug 2, 2026
@OnlineChef OnlineChef closed this Aug 2, 2026
@OnlineChef
OnlineChef force-pushed the feat/plugin-contract branch from abca252 to a5ba2a4 Compare August 2, 2026 05:34
@OnlineChef OnlineChef reopened this Aug 2, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between a5ba2a4 and 861157c.

📒 Files selected for processing (2)
  • src/plugins/index.ts
  • tests/plugins.test.ts

Comment thread src/plugins/index.ts
Comment thread src/plugins/index.ts
Comment thread src/plugins/index.ts
Comment thread src/plugins/index.ts
Comment thread tests/plugins.test.ts Outdated
Comment thread tests/plugins.test.ts
Comment thread tests/plugins.test.ts
Comment thread src/plugins/index.ts Outdated

OnlineChef commented Aug 2, 2026

Copy link
Copy Markdown
Author

Review follow-up (d57ae14, plus Codesmith follow-up b5d5d00):

  • Fixed prototype/class hooks by explicitly binding every lifecycle method to the original plugin instance, with a stateful class regression test.
  • Replaced TextEncoder allocation with Buffer.byteLength(..., "utf8") and skip all payload construction for empty request-session plugin snapshots.
  • Normalized every numeric hook field to a non-negative safe integer, including token counts and HTTP statuses, with NaN/Infinity/fractional regression coverage.
  • Added plugin-ID syntax/length boundary coverage and proved failure disabling resets for each new request session.
  • Moved freeze assertions outside hook failure isolation so assertion failures cannot be swallowed.
  • Added an exhaustive default guard for AdapterEvent: const exhaustive: never = event preserves the strict compile-time failure for future union variants, while the structural return remains safe for malformed runtime input. This is the acceptable form of the requested runtime fallback.

Every release path runs bun run typecheck; the current head is being revalidated by the full cross-platform matrix.

@OnlineChef
OnlineChef merged commit 77a3dcf into dev Aug 2, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant