Skip to content

docs(structure): plan plugin, metrics, rate-limit, and benchmark lanes - #39

Merged
OnlineChef merged 3 commits into
devfrom
plan/plugin-metrics-ratelimit-benchmarks
Aug 2, 2026
Merged

docs(structure): plan plugin, metrics, rate-limit, and benchmark lanes#39
OnlineChef merged 3 commits into
devfrom
plan/plugin-metrics-ratelimit-benchmarks

Conversation

@OnlineChef

@OnlineChef OnlineChef commented Aug 1, 2026

Copy link
Copy Markdown

Summary

Adds a repository-aware implementation plan for the proposed plugin, observability, rate-limit, config-validation, and benchmark work.

The plan deliberately avoids copying the generated bundle as-is because dev already contains strict TypeScript, canonical ocx config validate, persistent request/usage observability, memory sampling, observation CLI surfaces, and token/provider/model-based cost estimation.

Decisions captured

  • Extend the existing config validator; do not add a duplicate ocx validate command.
  • Reuse canonical usage/cost data; do not estimate cost from duration.
  • Do not ship a default prompt-rewriting “sanitizer”.
  • Protect /metrics through the existing management admission boundary and bound label cardinality.
  • Key rate limits by authenticated principal fingerprints, never raw secrets or spoofable Origin headers.
  • Deliver implementation as five small PR lanes targeting dev: plugin contract, metrics export, rate limiting, diagnostics, then benchmarks.

Test plan

Docs-only change:

  • Confirmed the branch targets dev.
  • Confirmed the document follows the repository's Bun-native, privacy, security-review, and validation requirements.
  • CI markdown/repository policy checks.

Follow-up

Implementation PRs should be stacked from this plan and retargeted to dev as parent lanes land.


View with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is enabled.

Summary by CodeRabbit

  • Documentation
    • Added a staged implementation plan for typed plugins, Prometheus-compatible metrics, admission-aware rate limiting, configuration diagnostics, and benchmarking.
    • Documented lifecycle, privacy, security, validation, performance, and failure-isolation requirements.
    • Defined benchmark coverage, validation commands, and completion criteria.
    • Recorded explicitly rejected approaches, including unauthenticated metrics and Origin-based rate-limit bypasses.

Greptile Summary

This documentation change lays out implementation work for plugin lifecycle handling, bounded metrics, principal-aware rate limits, configuration diagnostics, and benchmarks.

The rate-limit configuration guidance is inaccurate: it directs implementation through a generic migration path that does not exist in this repository. Optional settings should instead use the canonical OcxConfig schema and existing load/save reconciliation.

Confidence Score: 4/5

T-Rex T-Rex Logs

What T-Rex did

  • T-Rex produced a proof for a posted P2 finding.
  • T-Rex checked the parent revision state before adding the documentation plan to confirm the change was introduced by the reviewed commit.
  • T-Rex captured and reviewed the focused Bun validation script source and its validation outputs, including the round-trip and migration inventory.
  • T-Rex performed contract validation, noting that the before/after state shows optional/backward-compatible behavior and lists only targeted migrations.
  • T-Rex uploaded the executable source and its captured source representation for the config-migration validation.

View all artifacts

T-Rex Ran code and verified through T-Rex

Comments Outside Diff (1)

  1. General comment

    P2 Rate-limit plan requires a nonexistent generic config migration path

    • Bug
      • structure/plugin-metrics-ratelimit-benchmarks.md:135-136 says optional backward-compatible rate-limit config must go through the canonical OcxConfig schema “and migration path.” The repository has no generic/canonical configuration migration framework or registration path for a new setting. This can cause implementers to search for or build duplicate infrastructure rather than add an optional field to the canonical schema.
    • Cause
      • The current configuration design is schema-based and forward-compatible: src/config.ts:669-704 declares configSchema with .passthrough(), loadConfig parses it and reconciles defaults at src/config.ts:1121-1167, and saveConfig serializes the resulting OcxConfig at src/config.ts:1291-1303. Existing migration code is explicitly feature-specific, such as Claude auth mode (src/claude/auth-mode-migration.ts:16-32), Alibaba provider region (src/providers/alibaba-region-migration.ts:100-150), and OpenAI tier startup migration—not a reusable config migration path.
    • Fix
      • Revise lines 135-136 to require optional, backward-compatible configuration through the canonical OcxConfig/Zod load-save path, preserving defaults and reconciliation. State that a dedicated, setting-specific migration is needed only if rate-limit configuration must transform a prior persisted representation.

    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
structure/plugin-metrics-ratelimit-benchmarks.md:135-136
**Nonexistent config migration path**

The repository does not provide a generic configuration migration path for new settings. Optional configuration is accepted through the canonical `OcxConfig` Zod schema and preserved by the existing load/save reconciliation flow; current migration modules are feature-specific. This wording can send the rate-limit implementation toward duplicate infrastructure instead of the established configuration path.

Update the plan to require optional, backward-compatible settings through `OcxConfig` and the existing load/save path, reserving a dedicated migration only for a specific persisted representation that actually requires transformation.

---

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

Reviews (1): Last reviewed commit: "docs(structure): plan plugin, metrics, r..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

@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

@coderabbitai

coderabbitai Bot commented Aug 1, 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: 51 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: ddee1463-96e6-426d-abf1-3252d41d347f

📥 Commits

Reviewing files that changed from the base of the PR and between 140d217 and ed4a090.

📒 Files selected for processing (1)
  • structure/plugin-metrics-ratelimit-benchmarks.md
📝 Walkthrough

Walkthrough

The pull request adds a staged implementation plan for typed plugins, Prometheus-compatible metrics, admission-aware rate limiting, config diagnostics, and benchmarks. It also defines validation requirements, security constraints, documentation needs, and completion criteria.

Changes

Platform controls delivery plan

Layer / File(s) Summary
Plugin, metrics, and rate-limit contracts
structure/plugin-metrics-ratelimit-benchmarks.md
The plan defines typed plugin hooks, deterministic registration, timeout and failure isolation, redacted diagnostics, bounded metrics, authenticated metrics access, principal selection, token-bucket behavior, WebSocket charging, and stream concurrency protection.
Diagnostics and benchmark plan
structure/plugin-metrics-ratelimit-benchmarks.md
The plan extends ocx config validate with machine-readable diagnostics and defines post-stabilization benchmarks for plugin dispatch, metrics, rate limiting, and configuration diagnostics.
Validation and completion criteria
structure/plugin-metrics-ratelimit-benchmarks.md
The plan specifies runtime validation commands, security review, documentation, privacy and failure-isolation tests, compatibility checks, and completion requirements.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 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 documentation plan for plugins, metrics, rate limiting, and benchmarks.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch plan/plugin-metrics-ratelimit-benchmarks

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.

@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Aug 1, 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 `@structure/plugin-metrics-ratelimit-benchmarks.md`:
- Around line 187-196: Update the “Required validation” section to replace the
literal focused-test placeholder with executable test commands by listing the
exact test path for each validation lane, or explicitly stating that the
placeholder must be substituted before running. Keep the existing typecheck,
full test, and privacy scan commands unchanged.
- Around line 47-59: Define explicit mutation and isolation semantics for
beforeAdapterRequest and afterAdapterEvent: use immutable payloads or defensive
copies, typed hook return values, and clear rules for replacement, veto, and
cancellation. Specify the default action when a hook times out or fails,
ensuring shared requests, adapter ordering, and active streams remain unchanged
unless an allowed result explicitly requests otherwise.
- Around line 91-94: Update the metrics routing and admission logic so both
/metrics and /api/metrics/json require management authorization and preserve the
safe loopback policy. Ensure the management router applies the same checks to
the JSON endpoint, and add regression coverage verifying unauthenticated
requests are rejected on exposed listeners.
- Around line 49-61: Extend the redaction specification beyond request context
to cover payload fields for every lifecycle hook, especially afterAdapterEvent
data containing streamed content, tool arguments/results, and image data. Define
the field-level redaction rules for each hook and add coverage for streaming,
tool, and image event payloads, without exposing raw sensitive values.
- Around line 124-131: Update the token-bucket requirements to mandate atomic
read, refill, decrement, and persistence per request, with synchronization
covering the complete bucket operation. Define the hard-cap behavior when no
stale bucket is evictable: use a bounded fail-closed response that rejects or
throttles the request without creating another bucket.
- Around line 116-139: Update the principal fingerprint requirements in the
“Principal selection order” section to require a server-secret HMAC rather than
an unspecified digest, including explicit domain separation, secret rotation
behavior, and treatment of stored fingerprints and logs. Ensure raw principals
remain unpersisted and undisclosed, and that any management diagnostic
fingerprint prefixes follow the same keyed, non-reversible contract.
- Around line 157-163: Expand the Requirements section around the `--json`
behavior to define a versioned, additive JSON envelope and the stable diagnostic
fields, including how warnings and informational results affect exit status.
Specify the nonzero exit conditions for unsafe or invalid configurations,
preserve machine-readable output, and add compatibility tests covering the
schema and severity-to-exit mapping.
- Around line 133-134: Update the WebSocket rate-limiting design in the
handshake section to define both per-principal and global concurrent-connection
caps, reserve connection capacity before handshake completion, roll back
reservations when handshakes fail, and release capacity on disconnect. Keep
request-per-minute charging separate from long-lived stream concurrency
accounting.
🪄 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: 319e81a1-1c54-4356-b421-9cd68f7365ce

📥 Commits

Reviewing files that changed from the base of the PR and between 272a431 and 140d217.

📒 Files selected for processing (1)
  • structure/plugin-metrics-ratelimit-benchmarks.md

Comment thread structure/plugin-metrics-ratelimit-benchmarks.md
Comment thread structure/plugin-metrics-ratelimit-benchmarks.md
Comment thread structure/plugin-metrics-ratelimit-benchmarks.md Outdated
Comment thread structure/plugin-metrics-ratelimit-benchmarks.md
Comment thread structure/plugin-metrics-ratelimit-benchmarks.md
Comment thread structure/plugin-metrics-ratelimit-benchmarks.md
Comment thread structure/plugin-metrics-ratelimit-benchmarks.md
Comment thread structure/plugin-metrics-ratelimit-benchmarks.md
…ontracts from review

Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
Comment on lines +135 to +136
- Add optional, backward-compatible config through the canonical `OcxConfig`
schema and migration path.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Nonexistent config migration path

The repository does not provide a generic configuration migration path for new settings. Optional configuration is accepted through the canonical OcxConfig Zod schema and preserved by the existing load/save reconciliation flow; current migration modules are feature-specific. This wording can send the rate-limit implementation toward duplicate infrastructure instead of the established configuration path.

Update the plan to require optional, backward-compatible settings through OcxConfig and the existing load/save path, reserving a dedicated migration only for a specific persisted representation that actually requires transformation.

Artifacts

Parent revision check before the documentation plan was added

  • Ran `git show HEAD^:structure/plugin-metrics-ratelimit-benchmarks.md`; Git reported the file did not exist in the parent revision, showing the reviewed commit introduced the wording.

Focused Bun validation script source

  • The authored Bun script creates an isolated config home, performs a real load/save round trip for optional rate-limit settings, and inventories migration exports.

Captured source of the focused validation script

  • Captured output of the command that materialized the exact focused validation script source, ending with the expected nonzero diff exit code for a newly created file.

Focused configuration round-trip and migration inventory output

  • Executed the focused Bun script; it shows the optional rate-limit object survived load and save, the canonical schema uses passthrough, and discovered migrations are feature-specific.

TypeScript validation after focused configuration check

  • Ran `bun x tsc --noEmit --pretty false` successfully after the focused check, confirming the repository typecheck remains clean.

View artifacts

T-Rex Ran code and verified through T-Rex

Prompt To Fix With AI
This is a comment left during a code review.
Path: structure/plugin-metrics-ratelimit-benchmarks.md
Line: 135-136

Comment:
**Nonexistent config migration path**

The repository does not provide a generic configuration migration path for new settings. Optional configuration is accepted through the canonical `OcxConfig` Zod schema and preserved by the existing load/save reconciliation flow; current migration modules are feature-specific. This wording can send the rate-limit implementation toward duplicate infrastructure instead of the established configuration path.

Update the plan to require optional, backward-compatible settings through `OcxConfig` and the existing load/save path, reserving a dedicated migration only for a specific persisted representation that actually requires transformation.

---

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

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

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

… not a generic migration path

Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
@OnlineChef
OnlineChef merged commit a5ba2a4 into dev Aug 2, 2026
7 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant