Skip to content

[WRONG BRANCH] chore: sync plugin lane with reviewed plan #41

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
OnlineChef merged 2 commits into from
Aug 2, 2026
Merged

[WRONG BRANCH] chore: sync plugin lane with reviewed plan #41

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
054f4b5
docs(structure): specify hook, metrics, rate-limit, and diagnostics c…
OnlineChef Aug 1, 2026
ed4a090
docs(structure): route rate-limit config through OcxConfig load/save,…
OnlineChef Aug 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 76 additions & 9 deletions structure/plugin-metrics-ratelimit-benchmarks.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,29 @@ Requirements:
- Preserve the adapter event contract, event ordering, terminal events,
cancellation, backpressure, tool calls, image content, and streaming
behavior.
- Hooks are observational in v1: payloads are immutable. Pass each hook a
frozen or defensively copied payload so a plugin cannot mutate the shared
request or a streamed event in place, reorder adapter events, or corrupt an
active stream.
- Each hook has a typed return value. `beforeAdapterRequest`,
`afterAdapterEvent`, `onRequestComplete`, and `onRequestError` return `void`
in v1; plugins cannot replace, veto, or cancel requests or events. Any
future mutation or veto capability requires a new, explicitly typed result
contract and its own security review; it must never be inferred from a
mutated payload object.
- Define the default action for hook timeout and failure per hook: log the
failure through redacted diagnostics, skip the failing plugin for the rest
of that request, and continue processing unchanged. Hook failure never
alters, delays, or cancels the underlying request or stream.
- Expose a redacted request context by default. Prompts, headers, credentials,
OAuth tokens, account identifiers, and raw request bodies are outside the v1
capability boundary.
- The redaction boundary applies to every hook payload, not only the request
context. `afterAdapterEvent` payloads must redact streamed text content,
tool-call arguments, tool results, and image data by default, exposing only
typed structural metadata (event kind, sequence position, byte/token
counts, terminal status). Field-level redaction rules are defined per hook,
and focused tests must cover streaming, tool-call, and image event payloads.
- Enforce deterministic registration order and reject duplicate plugin IDs.
- Apply a bounded per-hook timeout and isolate plugin failures. A plugin failure
must not crash the proxy or corrupt an active stream.
Expand Down Expand Up @@ -88,10 +108,14 @@ Requirements:

- Labels are bounded. Never use request IDs, conversation IDs, account IDs, raw
model IDs, URLs, error messages, or arbitrary caller values as labels.
- `/metrics` follows the existing admission boundary. On exposed hosts it must
require management authorization; loopback behavior must remain safe.
- `/metrics` and `/api/metrics/json` both follow the existing admission
boundary: on exposed hosts each requires management authorization, and the
existing safe loopback policy applies identically to both.
- `/api/metrics/json` is added through the existing management router for GUI
consumption.
consumption and inherits its admission checks; it must not gain a separate,
weaker access path.
- Regression tests must prove unauthenticated requests to `/metrics` and
`/api/metrics/json` are rejected on exposed (non-loopback) listeners.
- Never export prompts, headers, credentials, local paths, identity fields, or
unredacted upstream errors.
- Snapshot generation must be bounded and must not delay request streaming.
Expand All @@ -115,8 +139,8 @@ Policies should cover:

Principal selection order:

1. OpenCodex admission-key identity, represented only by a non-reversible
internal fingerprint;
1. OpenCodex admission-key identity, represented only by a keyed,
non-reversible internal fingerprint;
2. authenticated management principal;
3. remote socket address only where Bun exposes a trustworthy address;
4. a bounded anonymous bucket when no stable principal exists.
Expand All @@ -125,15 +149,43 @@ Requirements:

- Never key buckets by a raw API key, Authorization header, email, account ID,
model, prompt, or other user content.
- Principal fingerprints are computed with an HMAC (for example HMAC-SHA-256)
over the principal using a server-side secret generated at first use, never
a plain unkeyed digest: low-entropy principals such as account identifiers
must not be brute-forceable offline. Use a domain-separation tag per
fingerprint purpose so rate-limit keys cannot be correlated with other
fingerprint uses. Rotating the secret invalidates existing buckets and
starts fresh ones; that is acceptable. The secret is never logged, exported,
or included in diagnostics; fingerprints live only in process memory and are
never persisted, and logs carry at most the bounded fingerprint prefixes
allowed below.
- Do not trust `Origin` as a bypass signal; non-browser clients can set it.
- Loopback bypass or relaxed limits are explicit config choices.
- Use token-bucket semantics with monotonic time, bounded burst, stale-bucket
eviction, and a hard cap on bucket count.
- Token consumption is atomic per bucket: read, refill, decrement, and persist
happen as one synchronous operation on the bucket state, with no `await`
point inside it, so concurrent requests can never observe and spend the same
balance. Any future async or shared-store backend must preserve this
atomicity boundary explicitly.
- Define the hard-cap fallback: when the bucket count is at the cap and no
stale bucket is evictable, fail closed by charging the request against a
bounded shared overflow bucket (or rejecting with the surface's `429`
envelope) instead of allocating a new bucket or waving the request through.
- Return the correct API-specific `429` envelope and integer `Retry-After`.
- Charge WebSocket request limits at handshake. Long-lived stream concurrency is
a separate protection from requests-per-minute accounting.
- Add optional, backward-compatible config through the canonical `OcxConfig`
schema and migration path.
- Bound WebSocket concurrency explicitly: enforce a per-principal and a global
cap on concurrent open connections. Reserve a connection slot before the
handshake completes, roll the reservation back if the handshake fails, and
release the slot on disconnect (including abnormal closes and timeouts) so
long-lived connections cannot exhaust resources without further
request-rate charges.
- Add optional, backward-compatible settings through the canonical `OcxConfig`
Zod schema and the existing config load/save reconciliation flow. There is
no generic migration mechanism to hook into; write a feature-specific
migration only if a persisted representation later requires an actual
Comment on lines +184 to +187

@greptile-apps greptile-apps Bot Aug 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Rate-limit settings bypass reconciliation

The plan routes persisted rate-limit settings through the existing config save flow, but that flow reconciles only the claudeCode subtree. If an operator edits a persisted rate-limit value while the server is running, an unrelated runtime save writes the stale in-memory rate-limit value back to disk and silently discards the operator’s edit. Require rate-limit settings to participate in the live-save baseline/reconciliation policy, with an explicit conflict policy, and add regression coverage for an on-disk rate-limit edit followed by an unrelated save.

Context Used: Focking gretig zijn en niet stoppen tot perfectie.... (source)

Artifacts

Focused rate-limit persistence validation source

  • Bun validation source models a persisted rate-limit edit and calls the actual live guarded save path, proving the exact behavior under review.

Persisted rate-limit setting before unrelated save

  • The executed before run shows the operator's persisted management rate limit is 99 requests per minute, establishing the edited state.

Persisted rate-limit setting after unrelated save

  • The executed after run shows an unrelated save persisted while the rate limit reverted from 99 to stale value 10, proving the edit is overwritten.

Existing narrow config preservation tests

  • The focused Bun test run completed with 16 passing tests and zero failures, confirming the existing Claude-only reconciliation guard remains operational.

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: 184-187

Comment:
**Rate-limit settings bypass reconciliation**

The plan routes persisted rate-limit settings through the existing config save flow, but that flow reconciles only the `claudeCode` subtree. If an operator edits a persisted rate-limit value while the server is running, an unrelated runtime save writes the stale in-memory rate-limit value back to disk and silently discards the operator’s edit. Require rate-limit settings to participate in the live-save baseline/reconciliation policy, with an explicit conflict policy, and add regression coverage for an on-disk rate-limit edit followed by an unrelated save.

**Context Used:** Focking gretig zijn en niet stoppen tot perfectie.... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))

---

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

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

transformation.
- Aggregate statistics may expose counts. Raw principals are never exposed;
fingerprint prefixes are allowed only when necessary for an authenticated
management diagnostic.
Expand All @@ -156,7 +208,17 @@ Candidate diagnostics:

Requirements:

- Preserve `--json` output.
- Preserve `--json` output with a versioned, additive envelope: existing fields
keep their meaning, new diagnostics data lands under new fields (for example
a `diagnostics` array plus a `schemaVersion` marker), and existing consumers
keep parsing without changes.
- Define the severity-to-exit mapping explicitly: structural validation errors
and `error`-severity diagnostics exit nonzero; `warning` and `info`
diagnostics exit zero by default, with an opt-in strict flag to make
warnings fail. Automation must never silently accept an invalid or unsafe
configuration because a diagnostic was demoted to stdout only.
- Add compatibility tests covering the JSON envelope shape and the
severity-to-exit mapping.
- Give every diagnostic a stable code, severity, path, message, and suggested
action.
- Never include secret values.
Expand Down Expand Up @@ -190,11 +252,16 @@ Every runtime PR runs:

```bash
bun run typecheck
bun test tests/<focused-subsystem>.test.ts
bun test tests/FOCUSED_SUBSYSTEM.test.ts # substitute the lane's test file, e.g. tests/plugins.test.ts
bun run test
bun run privacy:scan
```

`FOCUSED_SUBSYSTEM` is a placeholder, not an executable path: each lane
substitutes the focused test file it adds (plugin contract, metrics export,
rate limiting, config diagnostics, or benchmark harness) before running the
command.

Additionally:

- admission, auth/CORS, management endpoint, and config changes require explicit
Expand Down
Loading