From 8df4a6c52f05f8f6cc682cd0a27fc63cd5f5538d Mon Sep 17 00:00:00 2001 From: ZhenghuaBao Date: Tue, 18 Aug 2026 16:31:20 +0800 Subject: [PATCH 1/2] Sync the product name to "OrcaCode Review", accept all four trigger spellings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two concerns, one release since they both touch action.yml. NAMING. The Action called itself "Orca-Code-Review" while the App brands its comments "OrcaCode Review", so one product signed the same PR two ways. Renames the display name only: action `name`, the `brand` default, the commit-status description, the summary-comment heading, README/NOTICE /recipe titles, and source header comments. Deliberately NOT renamed, because each is an identity rather than a label: - the four `` upsert markers. The Action finds its own previous comments by these strings; renaming them orphans every comment already posted in every consumer repo and the next run posts a duplicate instead of editing in place. - ``, same reason for the PR-description region. - the repo slug in `uses:` and the `.github/workflows/` filename. Nobody reads a repo slug; the visible names above are what users see, and changing the slug would rewrite every consumer's workflow for nothing. The heading is safe to change precisely because nothing matches on it — upsert goes through the marker and the push counter through ``. TRIGGER. The example workflow gated on `/orca-code-review`, a spelling the App deliberately dropped, so the documented command did nothing on the App path. Now accepts the full cross of both prefixes and both separators — `/orcacode-review`, `/orcacode review`, `@orcacode-review`, `@orcacode review` — matching the App exactly. The cross is the point: a partial set is a trap, because the reader who writes the one spelling you left out gets no run, no comment, and no error to explain it. METERING (was already staged in the tree). `fact-proxy.mjs` gains `CR_USAGE_FILE` per-call token accounting and `CR_MAX_RPM`, plus `scripts/usage-summary.mjs` to turn that log into a per-model cost. It keeps only a bounded tail of each response so SSE stays unbuffered, and every extraction and append is soft-fail — metering is observability and must never gate a review. New inputs: `concurrency` (default 24 — the engine's own default of 8 was never set, which reads as per-file timeouts on slower models), `max-tools`, and `meter`. Tests: 176 pass. The 5 failures in settings.test.mjs are a pre-existing libuv crash on Windows + Node 24, identical on an unmodified tree. --- NOTICE | 2 +- README.md | 29 ++- SECURITY.md | 4 +- action.yml | 71 ++++- recipes/code-review.dsl.yaml | 2 +- scripts/check-result.mjs | 2 +- scripts/diff-guard.mjs | 2 +- scripts/exhaustive-merge.mjs | 2 +- scripts/fact-proxy.mjs | 206 ++++++++++++++- scripts/fact-proxy.test.mjs | 428 ++++++++++++++++++++++++++++++- scripts/gate.mjs | 2 +- scripts/inject-summary.mjs | 2 +- scripts/inject-summary.test.mjs | 4 +- scripts/quiet-filter.mjs | 2 +- scripts/settings.mjs | 2 +- scripts/settings.test.mjs | 2 +- scripts/severity.mjs | 2 +- scripts/summary-comment.mjs | 6 +- scripts/summary-comment.test.mjs | 2 +- scripts/usage-summary.mjs | 221 ++++++++++++++++ workflows/orca-code-review.yml | 25 +- 21 files changed, 959 insertions(+), 59 deletions(-) create mode 100644 scripts/usage-summary.mjs diff --git a/NOTICE b/NOTICE index 7a567f6..9d8b991 100644 --- a/NOTICE +++ b/NOTICE @@ -1,4 +1,4 @@ -Orca-Code-Review +OrcaCode Review Copyright 2026 OrcaRouter This product orchestrates Open Code Review ("ocr"), an open-source code review diff --git a/README.md b/README.md index a28a4e3..905fda0 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,4 @@ -# Orca-Code-Review +# OrcaCode Review AI pull-request review powered by the [OrcaRouter](https://orcarouter.ai) model gateway. A cheap model screens every push; once a push comes back clear of @@ -34,7 +34,10 @@ merge. 4. **Merge gate** — the job fails if any **P0/P1** is found; mark the check "required" in branch protection to block the merge. 5. **Per-commit loop** — `synchronize` re-reviews on every new push; comment - `/orca-code-review` on a PR to re-run on demand. The comment re-run posts + `/orcacode-review` on a PR to re-run on demand. Either prefix (`/` or `@`) + with either separator works — `/orcacode-review`, `/orcacode review`, + `@orcacode-review`, `@orcacode review` — the same set the OrcaCode Review + GitHub App accepts, so no spelling silently does nothing. The comment re-run posts fresh review comments but does **not** update the required merge-gate check: an `issue_comment` run is tied to the default branch, not the PR head, so its pass/fail can't attach to the PR's commit. Push a new commit to refresh the @@ -80,7 +83,7 @@ merge. (copy from [`workflows/orca-code-review.yml`](./workflows/orca-code-review.yml)): ```yaml - name: Orca-Code-Review + name: OrcaCode Review on: pull_request_target: # ready_for_review makes the dashboard's trigger=ready_for_review mode @@ -95,13 +98,17 @@ merge. jobs: review: runs-on: ubuntu-latest - # PR events, or a `/orca-code-review` command from a maintainer — - # otherwise any commenter could spend your quota. + # PR events, or a review command from a maintainer — otherwise any + # commenter could spend your quota. All four spellings are accepted, + # matching the OrcaCode Review GitHub App. if: | github.event_name == 'pull_request_target' || (github.event_name == 'issue_comment' && github.event.issue.pull_request && - startsWith(github.event.comment.body, '/orca-code-review') && + (startsWith(github.event.comment.body, '/orcacode-review') || + startsWith(github.event.comment.body, '/orcacode review') || + startsWith(github.event.comment.body, '@orcacode-review') || + startsWith(github.event.comment.body, '@orcacode review')) && contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.comment.author_association)) steps: - uses: Continuum-AI-Corp/orca-code-review@v1 @@ -123,7 +130,7 @@ All optional — pass as `with:` inputs on the action: |---|---|---| | `orcarouter-api-key` | _(required)_ | OrcaRouter API key | | `orcarouter-url` | `https://api.orcarouter.ai/v1/chat/completions` | Gateway endpoint | -| `brand` | `Orca-Code-Review` | Name shown on PR comments | +| `brand` | `OrcaCode Review` | Name shown on PR comments | | `router` | `orcarouter/code-review` | OrcaRouter router alias whose DSL recipe picks the cheap/strong model per tier (the action names no models) | | `fix-first` | `P0,P1` | Keep the PR on the cheap tier until these are cleared (then it's promoted) | | `block-on` | `P0,P1` | Fail the check (block merge) on one of these | @@ -131,7 +138,7 @@ All optional — pass as `with:` inputs on the action: | `max-diff-files` | `300` | Skip the review (same notice + `on-oversized-diff` outcome) when the diff touches more than this many files | | `on-oversized-diff` | `fail` | What an oversized-diff skip does to the check: `fail` (default) fails it, so a diff padded past the limits can never bypass a required merge gate; `pass` makes skips advisory (notice + green check) | | `settings` | `true` | Fetch per-repo settings from the OrcaRouter dashboard on every run; set `"false"` to skip the fetch and make the workflow file authoritative (inputs/defaults apply as-is, no dashboard override) | -| `auto-review-authors` | `""` (everyone) | Comma-separated author-association allowlist for **automatic** reviews. Empty reviews every PR. On a **public** repo, set e.g. `OWNER,MEMBER,COLLABORATOR,CONTRIBUTOR` so anonymous fork PRs can't drain your wallet with paid cascades (they can still be reviewed on demand via `/orca-code-review`). See [Public repos & spend](SECURITY.md#public-repos--spend). | +| `auto-review-authors` | `""` (everyone) | Comma-separated author-association allowlist for **automatic** reviews. Empty reviews every PR. On a **public** repo, set e.g. `OWNER,MEMBER,COLLABORATOR,CONTRIBUTOR` so anonymous fork PRs can't drain your wallet with paid cascades (they can still be reviewed on demand via `/orcacode-review`). See [Public repos & spend](SECURITY.md#public-repos--spend). | | `report` | `true` | Send a per-run summary (severity counts only — never code) to the OrcaRouter control plane; set `"false"` to disable — see [Run reporting](#run-reporting) | | `github-token` | `${{ github.token }}` | Token used to fetch the PR head, post review comments, and manage the tier label; override only if the default `GITHUB_TOKEN` lacks the needed scopes | | `engine-version` | `1.3.13` | Pinned `@alibaba-group/open-code-review` version (the review engine); bump deliberately after testing — later steps parse its JSON output shape | @@ -153,8 +160,8 @@ OrcaRouter dashboard without touching the workflow: | Setting | Values (default first) | Effect in the Action | |---|---|---| -| `auto_review` | `true` / `false` | `false`: automatic (`pull_request_target`) runs skip the engine, leave one small "automatic review is off" comment, and **pass** the check. `/orca-code-review` comment commands still run. | -| `trigger` | `every_push` / `ready_for_review` / `on_demand` | `every_push`: review every push. `ready_for_review`: skip automatic runs **while the PR is a draft** (add `ready_for_review` to your workflow's `pull_request_target.types` so the review fires when the PR leaves draft). `on_demand`: skip all automatic runs — only `/orca-code-review` comments review. All skips pass the check. | +| `auto_review` | `true` / `false` | `false`: automatic (`pull_request_target`) runs skip the engine, leave one small "automatic review is off" comment, and **pass** the check. `/orcacode-review` comment commands still run. | +| `trigger` | `every_push` / `ready_for_review` / `on_demand` | `every_push`: review every push. `ready_for_review`: skip automatic runs **while the PR is a draft** (add `ready_for_review` to your workflow's `pull_request_target.types` so the review fires when the PR leaves draft). `on_demand`: skip all automatic runs — only `/orcacode-review` comments review. All skips pass the check. | | `exhaustive` | `false` / `true` | Re-run the engine up to **2 extra times on the strong (enforced) tier**, deduplicating findings across passes (one review pass is not exhaustive; a re-run surfaces missed findings). The cheap screening pass never gets extras — its result is either superseded by the same-run strong review or held on fix-first findings anyway. The loop stops early once a pass adds nothing new **or a fix-first (P0/P1) finding is already in hand** (the gate blocks on it regardless of extra depth). **Cost cap: at most 3 engine passes total on the enforced tier.** The summary comment notes `exhaustive: N passes`. | | `quiet` | `false` / `true` | Advisory **P2 comments are not posted inline** — they are muted at the posting step only. The summary keeps the **true** P0/P1/P2 counts with a `quiet mode: P2 shown in summary only` note, and the gate/run report always see the unfiltered counts. | | `fix_first` | `"P0,P1"` | Same meaning as the `fix-first` input — see precedence below. | @@ -225,7 +232,7 @@ repo setting: 4. Save. Now a failing review disables the merge button until it goes green. Re-run the -gate by pushing a new commit (the `/orca-code-review` comment posts a fresh +gate by pushing a new commit (the `/orcacode-review` comment posts a fresh read but can't flip the required check — see the per-commit loop note above). **Merge-gate note — oversized diffs.** An oversized-diff skip (`max-diff-kb` / diff --git a/SECURITY.md b/SECURITY.md index 89c3cd3..94e7903 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -42,7 +42,7 @@ for public repos: - **Restrict auto-review authors** with the `auto-review-authors` input (e.g. `OWNER,MEMBER,COLLABORATOR,CONTRIBUTOR`) so anonymous/first-time PRs are not auto-reviewed. They can still be reviewed on demand by a maintainer's - `/orca-code-review` comment, which is separately gated to + `/orcacode-review` comment, which is separately gated to OWNER/MEMBER/COLLABORATOR. ### `pull_request_target` and secrets @@ -54,7 +54,7 @@ to read. Consumers MUST NOT add build, test, or install steps that execute PR-controlled code to this workflow. If you need to run PR code, do it in a separate `pull_request`-triggered workflow with no secrets. -The comment trigger (`/orca-code-review`) is gated to +The comment trigger (`/orcacode-review`) is gated to OWNER/MEMBER/COLLABORATOR in the shipped workflow so arbitrary commenters cannot spend your quota. diff --git a/action.yml b/action.yml index 3fd0b44..45572fa 100644 --- a/action.yml +++ b/action.yml @@ -1,4 +1,4 @@ -# Orca-Code-Review — reusable composite action. +# OrcaCode Review — reusable composite action. # # Consumers reference this with `uses:` instead of copying the folder; see # workflows/orca-code-review.yml for the ~15-line example workflow. @@ -35,7 +35,7 @@ # (scripts/report.mjs) posts severity counts — never code — to the OrcaRouter # control plane after the gate. -name: "Orca-Code-Review" +name: "OrcaCode Review" description: "AI PR review via the OrcaRouter gateway — cost-tiered cascade with a severity merge gate." author: "OrcaRouter" @@ -58,7 +58,7 @@ inputs: brand: description: "Name shown on PR comments." required: false - default: "Orca-Code-Review" + default: "OrcaCode Review" router: description: >- OrcaRouter router alias (orcarouter/) that owns model selection. @@ -84,7 +84,7 @@ inputs: paid cascades by opening PRs — set e.g. "OWNER,MEMBER,COLLABORATOR,CONTRIBUTOR" to auto-review only known contributors (others can still be reviewed on demand via - /orca-code-review). Also set a wallet budget + alert on the key. + /orcacode-review). Also set a wallet budget + alert on the key. Case-insensitive; values: OWNER, MEMBER, COLLABORATOR, CONTRIBUTOR, FIRST_TIME_CONTRIBUTOR, FIRST_TIMER, MANNEQUIN, NONE. required: false @@ -165,6 +165,29 @@ inputs: raise to be stricter. required: false default: "0.5" + concurrency: + description: >- + Max concurrent file reviews. Raise to shorten wall clock on large diffs; + lower it if your model has a tight per-minute request quota. Empty uses + the engine default. + required: false + default: "24" + max-tools: + description: >- + Max tool-call rounds per file. Lowering it reduces cost and time but can + cost review depth. Empty uses the engine default. + required: false + default: "" + meter: + description: >- + Record per-call token accounting for the run (prompt / completion / + cached tokens and the model the router resolved) and print a totals table + in the job log, so a run's real token cost is visible. Local only — + nothing is uploaded, and the file is removed with the other run files. + Soft-fail: metering never alters or blocks a review. Set "false" to skip + it. + required: false + default: "true" runs: using: "composite" @@ -187,7 +210,7 @@ runs: "$RUNNER_TEMP/cr-facts.json" "$RUNNER_TEMP/proxy.out" "$RUNNER_TEMP/proxy.err" \ "$RUNNER_TEMP/policy-block.json" \ "$RUNNER_TEMP/pr.diff" "$RUNNER_TEMP/diff-guard.json" "$RUNNER_TEMP/prev-summary.md" \ - "$RUNNER_TEMP/wallclock-timeout" \ + "$RUNNER_TEMP/wallclock-timeout" "$RUNNER_TEMP/cr-usage.jsonl" \ "$RUNNER_TEMP/result.l1.json" "$RUNNER_TEMP/result.l2.json" \ "$RUNNER_TEMP/result-extra.l1.json" "$RUNNER_TEMP/result-extra.l2.json" @@ -270,7 +293,7 @@ runs: if [ -n "${PR_NUMBER:-}" ]; then react "repos/$REPO/issues/$PR_NUMBER/reactions" fi - # For a maintainer's /orca-code-review command, also react on that + # For a maintainer's /orcacode-review command, also react on that # trigger comment so the operator sees direct feedback on THEIR comment. if [ "$EVENT_NAME" = "issue_comment" ] && [ -n "${COMMENT_ID:-}" ]; then react "repos/$REPO/issues/comments/$COMMENT_ID/reactions" @@ -331,7 +354,7 @@ runs: # Auto-event gate -> sets DECISION/REASON. Only AUTO runs are gated — # any pull_request* event, so a hand-written plain `pull_request` # workflow honors gating exactly like the documented - # `pull_request_target` one. An explicit `/orca-code-review` comment + # `pull_request_target` one. An explicit `/orcacode-review` comment # command (issue_comment) always proceeds — on-demand mode exists # precisely for those, and a human asking for a review of a # draft/disabled repo is deliberate. Shared by both settings paths so the @@ -461,7 +484,8 @@ runs: `${MARKER}\n## 🐳 ${brand}\n\n` + `⏸️ **Automatic review is off for this PR** (${process.env.REASON}).\n\n` + `The check **passes** — this never blocks your merge. A maintainer can still ` + - `run a review on demand by commenting \`/orca-code-review\`, or change the ` + + `run a review on demand by commenting \`/orcacode-review\` ` + + `(\`@orcacode review\` works too), or change the ` + `repo's review settings in the OrcaRouter dashboard.` + footer; @@ -681,6 +705,13 @@ runs: RESULT_EXTRA: ${{ runner.temp }}/result-extra.json RESULT_MERGED: ${{ runner.temp }}/result-merged.json FACTS_FILE: ${{ runner.temp }}/cr-facts.json + # Per-call token accounting (see the `meter` input). Empty disables the + # tap entirely inside the proxy. + CONCURRENCY: ${{ inputs.concurrency }} + MAX_TOOLS: ${{ inputs.max-tools }} + METER: ${{ inputs.meter }} + USAGE_FILE: ${{ runner.temp }}/cr-usage.jsonl + USAGE_SUMMARY: ${{ github.action_path }}/scripts/usage-summary.mjs # The proxy writes the guardrail/firewall reason here on a block; a later # always()-step posts it to the PR (the cascade itself fails closed). POLICY_BLOCK: ${{ runner.temp }}/policy-block.json @@ -709,6 +740,10 @@ runs: echo '{}' > "$FACTS_FILE" : > "$RUNNER_TEMP/proxy.out" : > "$RUNNER_TEMP/proxy.err" + # Metering is opt-out; an empty CR_USAGE_FILE turns the tap off in the proxy. + CR_USAGE_FILE="" + if [ "$METER" = "true" ]; then CR_USAGE_FILE="$USAGE_FILE"; fi + export CR_USAGE_FILE CR_FACTS_FILE="$FACTS_FILE" CR_POLICY_BLOCK_FILE="$POLICY_BLOCK" node "$PROXY" > "$RUNNER_TEMP/proxy.out" 2> "$RUNNER_TEMP/proxy.err" & PROXY_PID=$! trap 'kill "$PROXY_PID" 2>/dev/null || true' EXIT @@ -787,9 +822,15 @@ runs: # engine ignored the term. Redirections apply to whatever `timeout` # runs — i.e. ocr's own stdout/stderr — so the JSON result and log # end up in the same files as before. + # Engine tuning flags are only passed when set, so an empty input + # leaves the engine's own default in place rather than forcing a value. + ENGINE_FLAGS="" + [ -n "$CONCURRENCY" ] && ENGINE_FLAGS="$ENGINE_FLAGS --concurrency $CONCURRENCY" + [ -n "$MAX_TOOLS" ] && ENGINE_FLAGS="$ENGINE_FLAGS --max-tools $MAX_TOOLS" + # shellcheck disable=SC2086 timeout --kill-after=10s "${TIMEOUT_MIN}m" \ ocr review --from "$BASE" --to "$HEAD" \ - --background "$(cat "$BACKGROUND")" --format json \ + --background "$(cat "$BACKGROUND")" --format json $ENGINE_FLAGS \ > "$1" 2> "$REVIEW_LOG" || rc=$? cat "$REVIEW_LOG" # 124 = GNU timeout tripped (SIGTERM); 137 = 128+9, i.e. --kill-after @@ -954,6 +995,14 @@ runs: # >1 only in exhaustive mode; the summary comment notes it. echo "passes=$PASSES" >> "$GITHUB_OUTPUT" + # Token accounting for this run. Purely informational and soft-fail: + # `|| true` so a metering bug can never turn a completed review red. + if [ "$METER" = "true" ] && [ -s "$USAGE_FILE" ]; then + echo "::group::Token accounting" + node "$USAGE_SUMMARY" "$USAGE_FILE" || true + echo "::endgroup::" + fi + # If a dashboard guardrail/firewall blocked the request, the cascade above # failed closed and the normal review comment never runs. Surface the reason # on the PR so the block is actionable, not just a red X with a log line. @@ -1046,7 +1095,7 @@ runs: try { await github.rest.issues.createLabel({ ...repo, name, color: '0e8a16', - description: 'Orca-Code-Review: promoted to the strong model tier', + description: 'OrcaCode Review: promoted to the strong model tier', }); core.info(`Created repository label ${name}.`); } catch (e2) { @@ -1397,6 +1446,6 @@ runs: "$RUNNER_TEMP/cr-facts.json" "$RUNNER_TEMP/proxy.out" "$RUNNER_TEMP/proxy.err" \ "$RUNNER_TEMP/policy-block.json" \ "$RUNNER_TEMP/pr.diff" "$RUNNER_TEMP/diff-guard.json" "$RUNNER_TEMP/prev-summary.md" \ - "$RUNNER_TEMP/wallclock-timeout" \ + "$RUNNER_TEMP/wallclock-timeout" "$RUNNER_TEMP/cr-usage.jsonl" \ "$RUNNER_TEMP/result.l1.json" "$RUNNER_TEMP/result.l2.json" \ "$RUNNER_TEMP/result-extra.l1.json" "$RUNNER_TEMP/result-extra.l2.json" diff --git a/recipes/code-review.dsl.yaml b/recipes/code-review.dsl.yaml index f15e8dc..f194b8e 100644 --- a/recipes/code-review.dsl.yaml +++ b/recipes/code-review.dsl.yaml @@ -1,4 +1,4 @@ -# Orca-Code-Review — model-selection policy (the real recipe). +# OrcaCode Review — model-selection policy (the real recipe). # # Paste this into the per-workspace router `orcarouter/code-review` # (Dashboard → Routers → code-review → DSL). The Action never names a model; it diff --git a/scripts/check-result.mjs b/scripts/check-result.mjs index 910a3d4..2aaf95c 100644 --- a/scripts/check-result.mjs +++ b/scripts/check-result.mjs @@ -1,5 +1,5 @@ #!/usr/bin/env node -// Availability check for the Orca-Code-Review cascade. +// Availability check for the OrcaCode Review cascade. // // node check-result.mjs // exit 0 -> the engine produced a complete, usable review diff --git a/scripts/diff-guard.mjs b/scripts/diff-guard.mjs index 538d2be..8de9bea 100644 --- a/scripts/diff-guard.mjs +++ b/scripts/diff-guard.mjs @@ -1,5 +1,5 @@ #!/usr/bin/env node -// Oversized-diff guard for the Orca-Code-Review cascade. +// Oversized-diff guard for the OrcaCode Review cascade. // // Reviewing a huge diff is noise: the model truncates context, files get // skipped, and the severity signal collapses — better to skip loudly and let diff --git a/scripts/exhaustive-merge.mjs b/scripts/exhaustive-merge.mjs index 0060c31..2750bec 100644 --- a/scripts/exhaustive-merge.mjs +++ b/scripts/exhaustive-merge.mjs @@ -1,5 +1,5 @@ #!/usr/bin/env node -// Exhaustive-mode merge for the Orca-Code-Review cascade. +// Exhaustive-mode merge for the OrcaCode Review cascade. // // node exhaustive-merge.mjs --base --new --out // diff --git a/scripts/fact-proxy.mjs b/scripts/fact-proxy.mjs index a8cb9e7..c5911d7 100644 --- a/scripts/fact-proxy.mjs +++ b/scripts/fact-proxy.mjs @@ -1,5 +1,5 @@ #!/usr/bin/env node -// In-job fact-injecting proxy for the Orca-Code-Review cascade. +// In-job fact-injecting proxy for the OrcaCode Review cascade. // // OCR can only send the auth header (x-api-key / authorization) — it has no way // to attach custom headers. But the routing DSL routes on `headers[...]`. This @@ -55,19 +55,32 @@ // uncaughtException backstop that logs and keeps serving — one bad client can // never take the proxy down mid-job. // +// Metering (CR_USAGE_FILE): every relayed response gets its `usage` block +// appended as one JSONL record — per-call prompt/completion/cached tokens and +// the model the gateway actually resolved. This is what makes a model's REAL +// per-review cost measurable (unit price alone is misleading: a weaker model +// loops more tool calls, and the prefix-cache hit rate moves the bill more than +// the base rate does). The tap is deliberately non-invasive: it never buffers a +// whole response — it keeps only a bounded TAIL (usage sits at the end of an +// OpenAI-shaped body), so SSE still streams through unbuffered and memory stays +// flat on a multi-MB completion. Extraction and the append are fully soft-fail: +// metering must never alter, delay, or break a review. +// // Env: // ORCAROUTER_URL full upstream chat-completions URL (origin + path forwarded) // CR_FACTS_FILE path to the JSON facts file the driver rewrites per pass // CR_UPSTREAM_TIMEOUT_MS optional per-attempt upstream timeout (ms; default 120000) +// CR_USAGE_FILE optional JSONL path for per-call token accounting (off when unset) +// CR_MAX_RPM optional client-side request ceiling (requests/min; unset = no limit) // On listen it prints `PROXY_URL=http://127.0.0.1:` to // stdout; the driver sets OCR_LLM_URL to that. Auth is forwarded untouched and // never logged. // // Exported for tests: createProxyServer({ upstreamUrl, factsFile, -// policyBlockFile, sleep, maxRetries, maxBufferBytes }) returns an unlistened -// http.Server — `sleep` is the injectable backoff seam and `maxBufferBytes` -// the retry-buffer cap. The CLI entry below keeps the original env-var + -// PROXY_URL contract; action.yml usage is unchanged. +// policyBlockFile, sleep, maxRetries, maxBufferBytes, usageFile }) returns an +// unlistened http.Server — `sleep` is the injectable backoff seam and +// `maxBufferBytes` the retry-buffer cap. The CLI entry below keeps the original +// env-var + PROXY_URL contract; action.yml usage is unchanged. import http from "node:http"; import https from "node:https"; @@ -91,6 +104,123 @@ const MAX_RETRY_BUFFER_BYTES = 8 * 1024 * 1024; // classify the timeout like any other pre/post-response failure. const DEFAULT_UPSTREAM_TIMEOUT_MS = 120_000; const defaultSleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); +// Metering keeps only this much of the END of each response. `usage` is the +// last field of an OpenAI-shaped body (and rides the final SSE frame), so a +// tail is sufficient — and it keeps the proxy's memory flat regardless of how +// large the completion is. +const USAGE_TAIL_BYTES = 64 * 1024; + +// Client-side request-rate ceiling (CR_MAX_RPM). Some models enforce a hard +// per-minute request quota that cannot be raised. The engine fans out per-file +// requests concurrently and has no rate knob, so on a large enough diff it can +// overshoot the quota, the gateway answers 429, and the retry path above turns +// that into backoff sleeps — dead time that counts against the job's +// wall-clock budget. Throttling HERE is the only correct place: the proxy is +// the single choke point every engine request passes through, so it is the +// only spot that can observe the true aggregate rate. +// +// Retries count against the window too — a replayed request consumes quota +// exactly like a fresh one. +export function createRateLimiter({ maxRpm = 0, sleep = defaultSleep, now = () => Date.now() } = {}) { + if (!maxRpm || maxRpm <= 0) return { acquire: () => Promise.resolve() }; + const window = []; + // Serialize admission: concurrent callers must not all observe the same + // pre-admission window and collectively blow through the ceiling. + let queue = Promise.resolve(); + const admit = async () => { + for (;;) { + const t = now(); + while (window.length && t - window[0] >= 60_000) window.shift(); + if (window.length < maxRpm) { + window.push(t); + return; + } + // Wait just past the moment the oldest slot leaves the window. + await sleep(60_000 - (t - window[0]) + 5); + } + }; + return { + acquire() { + // Chain even on rejection so one failure cannot wedge the queue. + queue = queue.then(admit, admit); + return queue; + }, + }; +} + +// Pull the LAST `"usage": { ... }` object out of a response tail. Hand-scanned +// rather than regexed because the object nests (prompt_tokens_details), which a +// non-greedy regex would truncate. Returns null on anything unparseable — a +// missing meter record is always preferable to disturbing the review. +function extractUsage(text) { + const key = text.lastIndexOf('"usage"'); + if (key === -1) return null; + const open = text.indexOf("{", key); + if (open === -1) return null; + let depth = 0; + let inStr = false; + let esc = false; + for (let i = open; i < text.length; i += 1) { + const ch = text[i]; + if (inStr) { + if (esc) esc = false; + else if (ch === "\\") esc = true; + else if (ch === '"') inStr = false; + continue; + } + if (ch === '"') inStr = true; + else if (ch === "{") depth += 1; + else if (ch === "}") { + depth -= 1; + if (depth === 0) { + try { + return JSON.parse(text.slice(open, i + 1)); + } catch { + return null; + } + } + } + } + return null; // truncated by the tail cap — treat as absent +} + +// The resolved model name, so a router-alias run records what the DSL actually +// picked (the request only ever names the alias). +function extractModel(text) { + const m = text.match(/"model"\s*:\s*"([^"]{1,200})"/); + return m ? m[1] : null; +} + +// Append one metering record. Never throws: a full disk or a bad path must not +// fail a review that otherwise succeeded. +function recordUsage(tailText, { usageFile, status, retries, seq, error = null }) { + if (!usageFile) return; + try { + const usage = extractUsage(tailText); + const cached = + usage?.prompt_tokens_details?.cached_tokens ?? + usage?.cached_tokens ?? + usage?.prompt_cache_hit_tokens ?? + 0; + fs.appendFileSync( + usageFile, + JSON.stringify({ + ts: new Date().toISOString(), + seq, + status, + retries, + model: extractModel(tailText), + prompt: usage?.prompt_tokens ?? null, + completion: usage?.completion_tokens ?? null, + total: usage?.total_tokens ?? null, + cached, + ...(error ? { error } : {}), + }) + "\n", + ); + } catch { + /* best-effort: metering is observability, never a failure mode */ + } +} // A guardrail (content policy) or firewall (tool-call policy) block arrives as // HTTP 400 with `error.code = guardrail_blocked|firewall_blocked`. Persist the @@ -171,11 +301,22 @@ export function createProxyServer({ maxRetries = 3, maxBufferBytes = MAX_RETRY_BUFFER_BYTES, upstreamTimeoutMs = DEFAULT_UPSTREAM_TIMEOUT_MS, + usageFile = "", + maxRpm = 0, + now = () => Date.now(), } = {}) { const upstream = new URL(upstreamUrl); const upstreamLib = upstream.protocol === "http:" ? http : https; + // Shared across every in-flight request: the ceiling is per-proxy (i.e. per + // job), not per-connection. + const limiter = createRateLimiter({ maxRpm, sleep, now }); + // Per-proxy call counter, so a metering file reads as the ordered sequence of + // engine calls in one job (how many calls a model needed is itself a cost + // signal — see the header). + let callSeq = 0; return http.createServer((req, res) => { + const mySeq = usageFile ? ++callSeq : 0; const headers = { ...req.headers }; delete headers.host; // must match upstream, not the loopback proxy Object.assign(headers, readFacts(factsFile)); @@ -211,7 +352,14 @@ export function createProxyServer({ // Terminal failure for one client request. Before the relay: answer 502. // After it: the headers are out, so destroy the connection — a truncated // stream must error out fast, not leave OCR waiting until the job timeout. - const failResponse = (retries) => { + const failResponse = (retries, reason) => { + // Meter the failure. Without this, an attempt that never received a + // response is INVISIBLE to the metering file: a run where the gateway + // hung on every call would record zero rows and read as "the model did + // nothing", when in fact the environment failed. Recording it keeps + // those two cases distinguishable — with no token counts, since none + // were reported. + recordUsage("", { usageFile, status: 0, retries, seq: mySeq, error: reason || "no response" }); if (clientGone || res.writableEnded) return; // client already gone — nobody to answer if (res.headersSent) { res.destroy(); @@ -238,6 +386,10 @@ export function createProxyServer({ upRes.on("end", () => { const buf = Buffer.concat(parts); recordPolicyBlock(buf, policyBlockFile); + // A blocked call still consumed the gateway's attention (and shows up + // as a failed pass in the log), so meter it too — an all-blocked run + // must not read as a free run. + recordUsage(buf.toString("utf8"), { usageFile, status, retries, seq: mySeq }); if (clientGone || res.writableEnded) return; // client left during the 400 buffer — nobody to answer res.writeHead(status, outHeaders); res.end(buf); @@ -248,6 +400,22 @@ export function createProxyServer({ }); return; } + // Metering tap: observe a bounded TAIL of the body without buffering it. + // A 'data' listener alongside pipe() sees the same chunks and does not + // consume them, so the relay below is byte-for-byte unchanged. + if (usageFile) { + let tail = Buffer.alloc(0); + upRes.on("data", (c) => { + const joined = tail.length ? Buffer.concat([tail, c]) : c; + tail = + joined.length > USAGE_TAIL_BYTES + ? Buffer.from(joined.subarray(-USAGE_TAIL_BYTES)) + : joined; + }); + upRes.on("end", () => { + recordUsage(tail.toString("utf8"), { usageFile, status, retries, seq: mySeq }); + }); + } res.writeHead(status, outHeaders); upRes.pipe(res); // stream SSE through unbuffered // pipe() forwards data, not errors — and http.IncomingMessage swallows @@ -276,6 +444,17 @@ export function createProxyServer({ // start a second parallel retry chain (two relays -> double writeHead). const attempt = (body, retries) => { if (clientGone || res.destroyed) return; // client gave up — nobody left to answer + // Wait for a rate-limit slot before dialing. No-op when maxRpm is unset. + // Re-check the client afterwards: a throttled request can sit here for + // tens of seconds, and OCR may have hung up in the meantime — dialing + // then would burn quota on a response nobody will read. + limiter.acquire().then(() => { + if (clientGone || res.destroyed) return; + dial(body, retries); + }); + }; + + const dial = (body, retries) => { let settled = false; const settleThisAttempt = () => { if (settled) return false; @@ -347,7 +526,7 @@ export function createProxyServer({ retries < maxRetries ? " (after the request was sent — not retried)" : "" }`, ); - failResponse(retries); + failResponse(retries, e.message); }); upReq.end(body, () => { bodySent = true; @@ -363,6 +542,13 @@ export function createProxyServer({ let streaming = false; let streamReq = null; + // NOTE: the streamed path deliberately does NOT wait on the limiter. The + // client body is already flowing into us and cannot be paused for a + // minute-scale delay without stalling `req` and risking OCR's own timeout, + // and it is a single unreplayable attempt. Oversized bodies are rare (>8 + // MiB) and the engine's per-file requests are far smaller, so this cannot + // meaningfully erode the ceiling — but it is a known, bounded exception + // rather than an oversight. const startStreaming = () => { streaming = true; console.error( @@ -385,7 +571,7 @@ export function createProxyServer({ }); upReq.on("error", (e) => { console.error(`fact-proxy: upstream error (streamed body is unreplayable — not retried): ${e.message}`); - failResponse(0); + failResponse(0, e.message); }); streamReq = upReq; activeUpReq = upReq; @@ -436,6 +622,10 @@ if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) upstreamUrl: upstream.href, factsFile: process.env.CR_FACTS_FILE || "", policyBlockFile: process.env.CR_POLICY_BLOCK_FILE || "", + usageFile: process.env.CR_USAGE_FILE || "", + ...(Number.isFinite(Number(process.env.CR_MAX_RPM)) && Number(process.env.CR_MAX_RPM) > 0 + ? { maxRpm: Number(process.env.CR_MAX_RPM) } + : {}), ...(Number.isFinite(envTimeout) && envTimeout > 0 ? { upstreamTimeoutMs: envTimeout } : {}), }); server.listen(0, "127.0.0.1", () => { diff --git a/scripts/fact-proxy.test.mjs b/scripts/fact-proxy.test.mjs index 1f700bf..0160490 100644 --- a/scripts/fact-proxy.test.mjs +++ b/scripts/fact-proxy.test.mjs @@ -29,7 +29,7 @@ import { fileURLToPath } from "node:url"; import { after, before, describe, test } from "node:test"; import assert from "node:assert/strict"; -import { createProxyServer } from "./fact-proxy.mjs"; +import { createProxyServer, createRateLimiter } from "./fact-proxy.mjs"; const PROXY_SCRIPT = join(dirname(fileURLToPath(import.meta.url)), "fact-proxy.mjs"); let dir; @@ -546,6 +546,432 @@ describe("client disconnect (OCR hangs up mid-relay)", () => { }); }); +// Metering (CR_USAGE_FILE): per-call token accounting is what makes a model's +// real per-review cost measurable, so the records must be accurate — but the +// tap must stay strictly non-invasive. The relayed bytes, the status, and the +// retry header are all part of the contract these tests pin. +describe("usage metering (CR_USAGE_FILE)", () => { + const readMeter = (path) => + readFileSync(path, "utf8") + .trim() + .split("\n") + .filter(Boolean) + .map((l) => JSON.parse(l)); + + const bodyWithUsage = (extra = {}) => + JSON.stringify({ + id: "cmpl-1", + model: "vendor/model-a", + choices: [{ message: { content: "ok" }, finish_reason: "stop" }], + usage: { + prompt_tokens: 8000, + completion_tokens: 120, + total_tokens: 8120, + prompt_tokens_details: { cached_tokens: 6300 }, + ...extra, + }, + }); + + test("each relayed call appends one record with prompt/completion/cached tokens and the RESOLVED model", async () => { + const usageFile = join(dir, "usage-basic.jsonl"); + const upstream = await startUpstream([{ status: 200, body: bodyWithUsage() }]); + const proxy = await startProxy({ + upstreamUrl: `http://127.0.0.1:${upstream.port}/v1/chat/completions`, + usageFile, + }); + try { + // The request names the ALIAS; the meter must record what came back. + const res = await request(proxy.port, { body: '{"model":"orcarouter/code-review"}' }); + assert.equal(res.status, 200); + assert.equal(res.body, bodyWithUsage(), "the relayed body must be byte-identical — the tap only observes"); + + const rows = readMeter(usageFile); + assert.equal(rows.length, 1); + assert.equal(rows[0].prompt, 8000); + assert.equal(rows[0].completion, 120); + assert.equal(rows[0].total, 8120); + assert.equal(rows[0].cached, 6300, "cached_tokens drives the real bill — it must be captured"); + assert.equal(rows[0].model, "vendor/model-a", "records the model the gateway resolved, not the alias"); + assert.equal(rows[0].status, 200); + assert.equal(rows[0].seq, 1); + } finally { + await proxy.close(); + await upstream.close(); + } + }); + + test("calls are numbered in order, so call COUNT (a cost driver for tool-looping models) is recoverable", async () => { + const usageFile = join(dir, "usage-seq.jsonl"); + const upstream = await startUpstream([{ status: 200, body: bodyWithUsage() }]); + const proxy = await startProxy({ + upstreamUrl: `http://127.0.0.1:${upstream.port}/v1/chat/completions`, + usageFile, + }); + try { + for (let i = 0; i < 3; i += 1) await request(proxy.port, { body: '{"model":"m"}' }); + const rows = readMeter(usageFile); + assert.deepEqual( + rows.map((r) => r.seq), + [1, 2, 3], + ); + } finally { + await proxy.close(); + await upstream.close(); + } + }); + + test("alternate cached-token spellings (nested / flat) are all captured", async () => { + const usageFile = join(dir, "usage-alt.jsonl"); + const upstream = await startUpstream([ + { + status: 200, + body: JSON.stringify({ + model: "vendor/model-b", + usage: { prompt_tokens: 100, completion_tokens: 5, prompt_cache_hit_tokens: 64 }, + }), + }, + ]); + const proxy = await startProxy({ + upstreamUrl: `http://127.0.0.1:${upstream.port}/v1/chat/completions`, + usageFile, + }); + try { + await request(proxy.port, { body: "{}" }); + assert.equal(readMeter(usageFile)[0].cached, 64); + } finally { + await proxy.close(); + await upstream.close(); + } + }); + + test("a body with no usage block still records a row (nulls), so a run's call count stays honest", async () => { + const usageFile = join(dir, "usage-none.jsonl"); + const upstream = await startUpstream([{ status: 200, body: '{"model":"m","choices":[]}' }]); + const proxy = await startProxy({ + upstreamUrl: `http://127.0.0.1:${upstream.port}/v1/chat/completions`, + usageFile, + }); + try { + await request(proxy.port, { body: "{}" }); + const rows = readMeter(usageFile); + assert.equal(rows.length, 1); + assert.equal(rows[0].prompt, null); + assert.equal(rows[0].cached, 0); + } finally { + await proxy.close(); + await upstream.close(); + } + }); + + test("an attempt that never gets a response is metered — a hung gateway must not read as an idle model", async () => { + const usageFile = join(dir, "usage-noresp.jsonl"); + // Upstream that accepts the connection then destroys it without answering. + const upstream = await startRawUpstream((req, res) => res.socket.destroy()); + const sleep = fakeSleep(); + const proxy = await startProxy({ + upstreamUrl: `http://127.0.0.1:${upstream.port}/v1/chat/completions`, + usageFile, + sleep: sleep.fn, + }); + try { + const res = await request(proxy.port, { body: "{}" }); + assert.equal(res.status, 502); + const rows = readMeter(usageFile); + assert.equal(rows.length, 1, "the failed call must still produce a row"); + assert.equal(rows[0].status, 0, "status 0 marks 'no response received'"); + assert.equal(rows[0].prompt, null, "no tokens were reported, so none are invented"); + assert.ok(rows[0].error, "the failure reason is recorded"); + } finally { + await proxy.close(); + await upstream.close(); + } + }); + + test("a guardrail-blocked 400 is metered too — an all-blocked run must not read as free", async () => { + const usageFile = join(dir, "usage-block.jsonl"); + const policyBlockFile = join(dir, "usage-block-policy.json"); + const upstream = await startUpstream([ + { + status: 400, + body: JSON.stringify({ + error: { code: "guardrail_blocked", message: 'blocked by guardrail "secrets": a rule' }, + }), + }, + ]); + const proxy = await startProxy({ + upstreamUrl: `http://127.0.0.1:${upstream.port}/v1/chat/completions`, + usageFile, + policyBlockFile, + }); + try { + const res = await request(proxy.port, { body: "{}" }); + assert.equal(res.status, 400); + // The block record must still be written — metering must not displace it. + assert.equal(JSON.parse(readFileSync(policyBlockFile, "utf8")).kind, "guardrail"); + const rows = readMeter(usageFile); + assert.equal(rows.length, 1); + assert.equal(rows[0].status, 400); + } finally { + await proxy.close(); + await upstream.close(); + } + }); + + test("usage is extracted from the TAIL of a body far larger than the tail cap (memory stays flat)", async () => { + const usageFile = join(dir, "usage-big.jsonl"); + // 2 MB of filler ahead of the usage block: a whole-body buffer is exactly + // what the tap must avoid, and a tail scan must still find the numbers. + const filler = "x".repeat(2 * 1024 * 1024); + const upstream = await startUpstream([ + { + status: 200, + body: JSON.stringify({ + model: "big/model", + choices: [{ message: { content: filler } }], + usage: { prompt_tokens: 42, completion_tokens: 7, total_tokens: 49 }, + }), + }, + ]); + const proxy = await startProxy({ + upstreamUrl: `http://127.0.0.1:${upstream.port}/v1/chat/completions`, + usageFile, + }); + try { + const res = await request(proxy.port, { body: "{}" }); + assert.equal(res.status, 200); + assert.ok(res.body.length > 2 * 1024 * 1024, "the full body still reaches the client"); + const rows = readMeter(usageFile); + assert.equal(rows[0].prompt, 42); + assert.equal(rows[0].total, 49); + } finally { + await proxy.close(); + await upstream.close(); + } + }); + + test("nested usage objects parse correctly (a non-greedy regex would truncate them)", async () => { + const usageFile = join(dir, "usage-nested.jsonl"); + const upstream = await startUpstream([ + { + status: 200, + body: JSON.stringify({ + model: "m", + usage: { + prompt_tokens: 10, + prompt_tokens_details: { cached_tokens: 4, audio_tokens: 0 }, + completion_tokens_details: { reasoning_tokens: 3 }, + completion_tokens: 6, + total_tokens: 16, + }, + }), + }, + ]); + const proxy = await startProxy({ + upstreamUrl: `http://127.0.0.1:${upstream.port}/v1/chat/completions`, + usageFile, + }); + try { + await request(proxy.port, { body: "{}" }); + const rows = readMeter(usageFile); + assert.equal(rows[0].completion, 6, "fields AFTER the nested objects must still be read"); + assert.equal(rows[0].cached, 4); + assert.equal(rows[0].total, 16); + } finally { + await proxy.close(); + await upstream.close(); + } + }); + + test("metering off (no CR_USAGE_FILE) writes nothing and changes no behaviour", async () => { + const upstream = await startUpstream([{ status: 200, body: bodyWithUsage() }]); + const proxy = await startProxy({ + upstreamUrl: `http://127.0.0.1:${upstream.port}/v1/chat/completions`, + }); + try { + const res = await request(proxy.port, { body: "{}" }); + assert.equal(res.status, 200); + assert.equal(res.body, bodyWithUsage()); + assert.equal(res.headers["x-cr-retry-count"], "0"); + } finally { + await proxy.close(); + await upstream.close(); + } + }); + + test("an unwritable usage path never fails the review (metering is soft-fail)", async () => { + const upstream = await startUpstream([{ status: 200, body: bodyWithUsage() }]); + const proxy = await startProxy({ + upstreamUrl: `http://127.0.0.1:${upstream.port}/v1/chat/completions`, + // A directory that does not exist — every append throws ENOENT. + usageFile: join(dir, "no-such-dir", "usage.jsonl"), + }); + try { + const res = await request(proxy.port, { body: "{}" }); + assert.equal(res.status, 200, "the review must succeed even though metering cannot write"); + assert.equal(res.body, bodyWithUsage()); + } finally { + await proxy.close(); + await upstream.close(); + } + }); + + test("only the FINAL attempt of a retried call is metered (no double-billing in the record)", async () => { + const usageFile = join(dir, "usage-retry.jsonl"); + const sleep = fakeSleep(); + const upstream = await startUpstream([ + { status: 429 }, + { status: 200, body: bodyWithUsage() }, + ]); + const proxy = await startProxy({ + upstreamUrl: `http://127.0.0.1:${upstream.port}/v1/chat/completions`, + usageFile, + sleep: sleep.fn, + }); + try { + const res = await request(proxy.port, { body: "{}" }); + assert.equal(res.status, 200); + const rows = readMeter(usageFile); + assert.equal(rows.length, 1, "the discarded 429 is drained, not metered"); + assert.equal(rows[0].retries, 1, "but the retry count is recorded"); + assert.equal(rows[0].prompt, 8000); + } finally { + await proxy.close(); + await upstream.close(); + } + }); +}); + +// Rate ceiling (CR_MAX_RPM). Some models enforce a hard, unraisable per-minute +// request quota. Without a client-side ceiling the engine's per-file fan-out +// overshoots it, the gateway 429s, and the retry path converts that into +// backoff sleeps that eat the job's wall-clock budget. +describe("rate limiting (CR_MAX_RPM)", () => { + // Virtual clock: the limiter's waits are minute-scale, so tests must never + // sleep for real. `sleep` advances the clock instead of waiting. + function fakeClock() { + let t = 1_000_000; + return { + now: () => t, + sleep: (ms) => { + t += ms; + return Promise.resolve(); + }, + advance: (ms) => { + t += ms; + }, + at: () => t, + }; + } + + test("admits up to maxRpm within a minute, then defers the next until the window slides", async () => { + const clk = fakeClock(); + const lim = createRateLimiter({ maxRpm: 3, sleep: clk.sleep, now: clk.now }); + const t0 = clk.at(); + for (let i = 0; i < 3; i += 1) await lim.acquire(); + assert.equal(clk.at(), t0, "the first maxRpm requests are admitted with no delay"); + + await lim.acquire(); // 4th must wait for the oldest of the 3 to age out + assert.ok(clk.at() >= t0 + 60_000, `4th request waited past the window (t=${clk.at() - t0}ms)`); + }); + + test("never exceeds maxRpm in any 60s window, even under a concurrent burst", async () => { + const clk = fakeClock(); + const maxRpm = 5; + const lim = createRateLimiter({ maxRpm, sleep: clk.sleep, now: clk.now }); + const admitted = []; + // 20 callers all acquire at once — the real engine fan-out pattern. + await Promise.all( + Array.from({ length: 20 }, () => lim.acquire().then(() => admitted.push(clk.now()))), + ); + assert.equal(admitted.length, 20); + for (const t of admitted) { + const inWindow = admitted.filter((x) => x > t - 60_000 && x <= t).length; + assert.ok(inWindow <= maxRpm, `window ending at ${t} admitted ${inWindow} > ${maxRpm}`); + } + }); + + test("maxRpm unset or 0 disables throttling entirely (zero added latency)", async () => { + const clk = fakeClock(); + for (const maxRpm of [0, undefined]) { + const lim = createRateLimiter({ maxRpm, sleep: clk.sleep, now: clk.now }); + const t0 = clk.at(); + for (let i = 0; i < 500; i += 1) await lim.acquire(); + assert.equal(clk.at(), t0, `maxRpm=${maxRpm} must not delay anything`); + } + }); + + test("a rejected admission does not wedge the queue for later callers", async () => { + const clk = fakeClock(); + let calls = 0; + const flaky = (ms) => { + calls += 1; + if (calls === 1) return Promise.reject(new Error("boom")); + return clk.sleep(ms); + }; + const lim = createRateLimiter({ maxRpm: 1, sleep: flaky, now: clk.now }); + await lim.acquire(); + // Forces a wait, whose first sleep rejects. + await lim.acquire().catch(() => {}); + // The queue must still serve the next caller rather than hanging forever. + await assert.doesNotReject(lim.acquire()); + }); + + test("retries consume quota too — a replayed request is billed like a fresh one", async () => { + // 429 then 200: the proxy must take TWO rate-limit slots for one client + // request, otherwise a 429 storm silently doubles the real request rate. + const clk = fakeClock(); + const upstream = await startUpstream([{ status: 429 }, { status: 200, body: "ok" }]); + const proxy = await startProxy({ + upstreamUrl: `http://127.0.0.1:${upstream.port}/v1/chat/completions`, + sleep: clk.sleep, + now: clk.now, + maxRpm: 1, // only ONE slot per minute, so the retry must wait a window + }); + try { + const res = await request(proxy.port, { body: "{}" }); + assert.equal(res.status, 200); + assert.equal(res.headers["x-cr-retry-count"], "1"); + assert.equal(upstream.seen.length, 2, "both attempts reached upstream"); + // Virtual clock advanced by ~a full window => the retry waited for a slot. + assert.ok( + clk.at() >= 1_000_000 + 60_000, + `the retry acquired a fresh slot rather than bypassing the ceiling (advanced ${clk.at() - 1_000_000}ms)`, + ); + } finally { + await proxy.close(); + await upstream.close(); + } + }); + + test("throttled requests still relay normally end-to-end", async () => { + const clk = fakeClock(); + const upstream = await startUpstream([{ status: 200, body: '{"ok":true}' }]); + const proxy = await startProxy({ + upstreamUrl: `http://127.0.0.1:${upstream.port}/v1/chat/completions`, + sleep: clk.sleep, + now: clk.now, + maxRpm: 2, + }); + try { + for (let i = 0; i < 4; i += 1) { + const res = await request(proxy.port, { body: '{"model":"m"}' }); + assert.equal(res.status, 200); + assert.equal(res.body, '{"ok":true}'); + } + assert.equal(upstream.seen.length, 4); + } finally { + await proxy.close(); + await upstream.close(); + } + }); + + test("the CLI reads CR_MAX_RPM", () => { + const cli = readFileSync(PROXY_SCRIPT, "utf8"); + assert.match(cli, /CR_MAX_RPM/, "CR_MAX_RPM must be wired in the CLI entry"); + assert.match(cli, /maxRpm/, "and mapped onto the maxRpm option"); + }); +}); + describe("action.yml wiring (guardrail / firewall block comment)", () => { const actionYml = () => readFileSync(join(dirname(fileURLToPath(import.meta.url)), "..", "action.yml"), "utf8"); diff --git a/scripts/gate.mjs b/scripts/gate.mjs index b79886b..f1ee5ae 100644 --- a/scripts/gate.mjs +++ b/scripts/gate.mjs @@ -1,5 +1,5 @@ #!/usr/bin/env node -// Severity gate for the Orca-Code-Review cascade. +// Severity gate for the OrcaCode Review cascade. // // Reads an `ocr review --format json` result file, extracts the [P0]/[P1]/[P2]/[P3] // tag the model prefixes onto each comment, and answers one yes/no question: diff --git a/scripts/inject-summary.mjs b/scripts/inject-summary.mjs index d6241b4..5f25436 100644 --- a/scripts/inject-summary.mjs +++ b/scripts/inject-summary.mjs @@ -1,5 +1,5 @@ #!/usr/bin/env node -// Merge the Orca-Code-Review summary into the PR DESCRIPTION body. +// Merge the OrcaCode Review summary into the PR DESCRIPTION body. // // GitHub orders PR issue comments chronologically with no pin-to-top API, so a // summary posted as a comment sinks below the inline findings and is hard to diff --git a/scripts/inject-summary.test.mjs b/scripts/inject-summary.test.mjs index 1b24f23..f923f5f 100644 --- a/scripts/inject-summary.test.mjs +++ b/scripts/inject-summary.test.mjs @@ -13,7 +13,7 @@ import { REGION_END, } from "./inject-summary.mjs"; -const SUMMARY = "\n## Orca-Code-Review — push 1\n\n✅ no blocking findings"; +const SUMMARY = "\n## OrcaCode Review — push 1\n\n✅ no blocking findings"; describe("injectSummary", () => { test("prepends the region to the top on first insert, keeping author text below", () => { @@ -78,7 +78,7 @@ describe("extractRegion", () => { test("recovers the machine-state line the next push reads to number itself", () => { const withState = - "\n\n\n## Orca-Code-Review — push 3"; + "\n\n\n## OrcaCode Review — push 3"; const region = extractRegion(injectSummary("x", withState)); const m = region.match(//); assert.ok(m, "state line survives the round-trip"); diff --git a/scripts/quiet-filter.mjs b/scripts/quiet-filter.mjs index 5be8a5f..73f4ae5 100644 --- a/scripts/quiet-filter.mjs +++ b/scripts/quiet-filter.mjs @@ -1,5 +1,5 @@ #!/usr/bin/env node -// Quiet-mode severity filter for the Orca-Code-Review cascade. +// Quiet-mode severity filter for the OrcaCode Review cascade. // // node quiet-filter.mjs --drop P2 --out // diff --git a/scripts/settings.mjs b/scripts/settings.mjs index 172110e..161d0ad 100644 --- a/scripts/settings.mjs +++ b/scripts/settings.mjs @@ -1,5 +1,5 @@ #!/usr/bin/env node -// Dashboard settings fetch for the Orca-Code-Review cascade. +// Dashboard settings fetch for the OrcaCode Review cascade. // // node settings.mjs --url --repo owner/name --out // (the API key comes from ORCAROUTER_API_KEY in the env, never a flag) diff --git a/scripts/settings.test.mjs b/scripts/settings.test.mjs index ceb8a7c..9c60158 100644 --- a/scripts/settings.test.mjs +++ b/scripts/settings.test.mjs @@ -571,7 +571,7 @@ describe("action.yml: gate_decision — shared auto-event gate (both settings pa }); test("a comment command (issue_comment) still proceeds even for a disallowed author", () => { - // On-demand /orca-code-review is maintainer-gated in the workflow `if:`; + // On-demand /orcacode-review is maintainer-gated in the workflow `if:`; // the settings gate must NOT additionally skip it. assert.equal(disabled({ event: "issue_comment", list: "CONTRIBUTOR", assoc: "NONE" }).decision, "review"); }); diff --git a/scripts/severity.mjs b/scripts/severity.mjs index 397df7f..b4e2d46 100644 --- a/scripts/severity.mjs +++ b/scripts/severity.mjs @@ -1,4 +1,4 @@ -// Shared severity parsing for the Orca-Code-Review scripts. +// Shared severity parsing for the OrcaCode Review scripts. // // One module owns the rule "which severity is this finding?" so the merge gate // (gate.mjs), the control-plane run report (report.mjs), and the PR summary diff --git a/scripts/summary-comment.mjs b/scripts/summary-comment.mjs index 2c1a903..d3e79b2 100644 --- a/scripts/summary-comment.mjs +++ b/scripts/summary-comment.mjs @@ -1,5 +1,5 @@ #!/usr/bin/env node -// Edit-in-place PR summary comment for the Orca-Code-Review cascade. +// Edit-in-place PR summary comment for the OrcaCode Review cascade. // // node summary-comment.mjs --tier cheap|strong --push // --gate pass|blocked [--prev ] @@ -19,7 +19,7 @@ // line 2 // machine state: the NEXT run feeds this body back via --prev for // the Δ column, and reads .push to number itself -// then "## Orca-Code-Review — push N", the severity table (the +// then "## OrcaCode Review — push N", the severity table (the // "Δ vs previous push" column appears only when --prev carries a // parseable state line), a tier-state line, and a gate line. // @@ -126,7 +126,7 @@ const delta = (d) => (d > 0 ? `+${d}` : String(d)); const state = { p0: counts.P0, p1: counts.P1, p2: counts.P2, p3: counts.P3, push }; const lines = [MARKER, ``, ""]; -lines.push(`## Orca-Code-Review — push ${push}`, ""); +lines.push(`## OrcaCode Review — push ${push}`, ""); if (prev) { lines.push("| Severity | Count | Δ vs previous push |", "|---|---|---|"); for (const s of SEVERITIES) { diff --git a/scripts/summary-comment.test.mjs b/scripts/summary-comment.test.mjs index 20dbefb..5204d62 100644 --- a/scripts/summary-comment.test.mjs +++ b/scripts/summary-comment.test.mjs @@ -54,7 +54,7 @@ describe("marker and machine state", () => { test("header names the push number", () => { const out = run([], ["--tier", "strong", "--push", "4", "--gate", "pass"]); - assert.match(out, /## Orca-Code-Review — push 4/); + assert.match(out, /## OrcaCode Review — push 4/); }); }); diff --git a/scripts/usage-summary.mjs b/scripts/usage-summary.mjs new file mode 100644 index 0000000..5a10fdb --- /dev/null +++ b/scripts/usage-summary.mjs @@ -0,0 +1,221 @@ +#!/usr/bin/env node +// Turn fact-proxy's per-call metering (CR_USAGE_FILE) into a cost picture. +// +// node usage-summary.mjs [--pricing pricing.json] [--json] +// +// Why this exists: a reviewer model's LIST price predicts its review cost +// badly. Three effects dominate and only metering exposes them: +// 1. Call amplification — a weaker model burns more tool-call round trips +// per file, so it re-sends the same context more times. Input tokens, not +// price, are the bill. +// 2. Prefix-cache hit rate — the engine re-sends a near-identical prompt per +// file. A model whose gateway entry has NO cache_ratio pays full rate on +// every repeat and can lose to a nominally pricier model that caches. +// 3. Output is a rounding error — a review is overwhelmingly input tokens, +// so a scary-looking completion multiplier barely moves the total. +// +// With --pricing (a dump of the gateway's `GET /api/pricing`) each model's +// ratios are applied to the measured tokens to produce a real per-run cost. +// The gateway bills `QuotaPerUnit = 500 * 1000` quota per USD at ratio 1, i.e. +// ratio 1 == $0.002/1K == $2.00/1M input tokens; output is +// ratio * completion_ratio and cached input is ratio * cache_ratio. +// +// A model with no cache_ratio in the price list gets its cached tokens billed +// at FULL input rate and is flagged, because that is what the gateway does. +// +// Exits 0 even on a malformed line (metering is observability, never a gate). + +import fs from "node:fs"; + +const USD_PER_1M_AT_RATIO_1 = 2.0; + +const argv = process.argv.slice(2); +const file = argv.find((a) => !a.startsWith("--")); +let pricingPath = null; +let asJson = false; +for (let i = 0; i < argv.length; i += 1) { + if (argv[i] === "--pricing") pricingPath = argv[i + 1]; + else if (argv[i] === "--json") asJson = true; +} +if (!file) { + console.error("usage: node usage-summary.mjs [--pricing pricing.json] [--json]"); + process.exit(2); +} + +const rows = []; +let skipped = 0; +for (const line of fs.readFileSync(file, "utf8").split("\n")) { + if (!line.trim()) continue; + try { + rows.push(JSON.parse(line)); + } catch { + skipped += 1; + } +} + +// Price lookup. The name in a metering record is whatever the UPSTREAM provider +// echoed (e.g. "gpt-4o-mini-2024-07-18"), while the price list is keyed by the +// gateway's own vendor-prefixed alias ("openai/gpt-4o-mini-2024-07-18"), so an +// exact-match-only lookup silently prices nothing. Resolution order: +// 1. exact key +// 2. unique match on the part after "/" (vendor prefix dropped) +// 3. unique match after stripping a trailing -YYYY-MM-DD snapshot suffix +// A basename that maps to SEVERAL vendors is left unpriced rather than guessed +// — a wrong ratio is worse than an honest gap in a cost comparison. +let prices = null; +if (pricingPath) { + try { + const data = JSON.parse(fs.readFileSync(pricingPath, "utf8")).data || []; + const exact = new Map(data.map((d) => [d.model_name, d])); + // Basenames are indexed case-INSENSITIVELY: providers echo back their own + // casing, which often differs from the price list's, so a case-sensitive + // index silently prices nothing. + const byBase = new Map(); + for (const d of data) { + const full = String(d.model_name); + const base = full.includes("/") ? full.slice(full.indexOf("/") + 1) : full; + const k = base.toLowerCase(); + if (!byBase.has(k)) byBase.set(k, []); + byBase.get(k).push(d); + } + const unique = (name) => { + const hits = byBase.get(String(name).toLowerCase()); + return hits && hits.length === 1 ? hits[0] : null; + }; + // Suffixes providers append to the echoed name that are not part of the + // price-list key: a dated snapshot, or a release-channel marker such as + // "-preview", which is billed against the unsuffixed entry. + const strip = [ + (s) => s.replace(/-\d{4}-\d{2}-\d{2}$/, ""), + (s) => s.replace(/-(preview|latest|stable)$/i, ""), + ]; + prices = { + get(name) { + if (exact.has(name)) return exact.get(name); + const direct = unique(name); + if (direct) return direct; + for (const f of strip) { + const s = f(String(name)); + if (s === name) continue; + const hit = exact.get(s) || unique(s); + if (hit) return hit; + } + return null; + }, + }; + } catch (e) { + console.error(`usage-summary: could not read pricing (${e.message}) — reporting tokens only`); + } +} + +// Group by resolved model: one run can legitimately span models (the L2 judge +// is a different model from the reviewer, and a router alias can re-resolve). +const byModel = new Map(); +const blank = () => ({ calls: 0, prompt: 0, completion: 0, cached: 0, blocked: 0, retries: 0 }); +for (const r of rows) { + const key = r.model || "(unknown)"; + if (!byModel.has(key)) byModel.set(key, blank()); + const m = byModel.get(key); + m.calls += 1; + m.prompt += r.prompt || 0; + m.completion += r.completion || 0; + m.cached += r.cached || 0; + m.retries += r.retries || 0; + if (r.status && r.status >= 400) m.blocked += 1; +} + +const cost = (model, m) => { + const p = prices ? prices.get(model) : null; + if (!p) return null; + const ratio = p.model_ratio; + if (typeof ratio !== "number") return null; + const inRate = ratio * USD_PER_1M_AT_RATIO_1; + const outRate = ratio * (p.completion_ratio ?? 1) * USD_PER_1M_AT_RATIO_1; + // No cache_ratio on the price entry == no cache discount at this gateway. + const hasCache = typeof p.cache_ratio === "number"; + const cachedRate = hasCache ? inRate * p.cache_ratio : inRate; + const fresh = Math.max(0, m.prompt - m.cached); + const usd = + (fresh / 1e6) * inRate + (m.cached / 1e6) * cachedRate + (m.completion / 1e6) * outRate; + return { usd, inRate, outRate, cachedRate, hasCache }; +}; + +const total = blank(); +for (const m of byModel.values()) { + total.calls += m.calls; + total.prompt += m.prompt; + total.completion += m.completion; + total.cached += m.cached; + total.blocked += m.blocked; + total.retries += m.retries; +} +let totalUsd = 0; +let priced = true; +for (const [model, m] of byModel) { + const c = cost(model, m); + if (c) totalUsd += c.usd; + else priced = false; +} + +const pct = (n, d) => (d > 0 ? `${((100 * n) / d).toFixed(1)}%` : "—"); +const M = (n) => (n / 1e6).toFixed(3); + +if (asJson) { + console.log( + JSON.stringify( + { + calls: total.calls, + prompt_tokens: total.prompt, + completion_tokens: total.completion, + cached_tokens: total.cached, + cache_hit_rate: total.prompt > 0 ? total.cached / total.prompt : null, + blocked_calls: total.blocked, + retries: total.retries, + usd: priced ? Number(totalUsd.toFixed(4)) : null, + by_model: [...byModel].map(([model, m]) => ({ + model, + ...m, + usd: cost(model, m)?.usd ?? null, + })), + malformed_lines: skipped, + }, + null, + 2, + ), + ); + process.exit(0); +} + +console.log( + `calls=${total.calls} prompt=${M(total.prompt)}M completion=${M(total.completion)}M ` + + `cached=${M(total.cached)}M (${pct(total.cached, total.prompt)} hit)` + + (total.retries ? ` retries=${total.retries}` : "") + + (total.blocked ? ` blocked=${total.blocked}` : ""), +); + +for (const [model, m] of byModel) { + const c = cost(model, m); + const money = c + ? ` $${c.usd.toFixed(4)}` + + (c.hasCache ? "" : " [no cache discount at this gateway — repeats billed at full rate]") + : prices + ? " (not in price list)" + : ""; + console.log( + ` ${model}: calls=${m.calls} prompt=${M(m.prompt)}M completion=${M(m.completion)}M ` + + `cached=${pct(m.cached, m.prompt)}${money}`, + ); +} + +if (priced && byModel.size > 0) { + console.log(`TOTAL $${totalUsd.toFixed(4)} for this run`); + // Per-call input is the amplification signal: compare it across models on + // the SAME commit to see which one re-reads context more. + if (total.calls > 0) { + console.log( + ` (${(total.prompt / total.calls).toFixed(0)} input tokens per call — ` + + `compare across models on the same commit to see call amplification)`, + ); + } +} +if (skipped) console.log(`note: ${skipped} malformed line(s) skipped`); diff --git a/workflows/orca-code-review.yml b/workflows/orca-code-review.yml index 0a0f0ae..bc318bb 100644 --- a/workflows/orca-code-review.yml +++ b/workflows/orca-code-review.yml @@ -1,4 +1,4 @@ -# Orca-Code-Review — example consumer workflow. +# OrcaCode Review — example consumer workflow. # # Copy this file into your repo at `.github/workflows/` and add one secret, # `ORCAROUTER_API_KEY`. The review logic lives in the published action, so you @@ -9,7 +9,7 @@ # is clear does the strong model run the final pass. A severity gate can fail the # check to block the merge — mark it "required" in branch protection. -name: Orca-Code-Review +name: OrcaCode Review concurrency: group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.event.issue.number || github.ref }} @@ -34,16 +34,23 @@ permissions: jobs: review: runs-on: ubuntu-latest - # Run on PR events, or when a trusted user comments `/orca-code-review` on a - # PR. The command runs the privileged `pull_request_target` workflow with the - # OrcaRouter secret, so only maintainers (owner/member/collaborator) may - # trigger it — otherwise any participant could burn paid quota and spam - # reviews without pushing new commits. + # Run on PR events, or when a trusted user comments the review command on a + # PR. All four spellings are accepted — either prefix (`/` or `@`) with + # either separator (hyphen or space) — which is the same set the OrcaCode + # Review GitHub App takes. Accept the full cross or you build a trap: a + # reader who writes the one spelling you left out gets no run and no error + # to explain why. The command runs the privileged `pull_request_target` + # workflow with the OrcaRouter secret, so only maintainers + # (owner/member/collaborator) may trigger it — otherwise any participant + # could burn paid quota and spam reviews without pushing new commits. if: | github.event_name == 'pull_request_target' || (github.event_name == 'issue_comment' && github.event.issue.pull_request && - startsWith(github.event.comment.body, '/orca-code-review') && + (startsWith(github.event.comment.body, '/orcacode-review') || + startsWith(github.event.comment.body, '/orcacode review') || + startsWith(github.event.comment.body, '@orcacode-review') || + startsWith(github.event.comment.body, '@orcacode review')) && contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.comment.author_association)) steps: - uses: Continuum-AI-Corp/orca-code-review@v1 @@ -51,7 +58,7 @@ jobs: orcarouter-api-key: ${{ secrets.ORCAROUTER_API_KEY }} # All optional — uncomment to override the defaults: # orcarouter-url: ${{ secrets.ORCAROUTER_URL }} - # brand: "Orca-Code-Review" + # brand: "OrcaCode Review" # router: "orcarouter/code-review" # router whose DSL picks the model per tier # fix-first: "P0,P1" # withhold the strong tier until these are fixed # block-on: "P0" # fail the check (block merge) on these From 9fbab1ff05e0f514986bc0234cd6fa9ba41438a1 Mon Sep 17 00:00:00 2001 From: ZhenghuaBao Date: Tue, 18 Aug 2026 17:38:01 +0800 Subject: [PATCH 2/2] Fix three metering defects found in review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 1 of review follow-ups. Three of four findings were real; the fourth is declined below with its reason. 1. `model` was read from the same bounded TAIL as `usage`, but an OpenAI-shaped body puts `model` near the START. Any non-streaming response larger than the tail therefore recorded `model: null`, and usage-summary groups those under "(unknown)" and cannot price them — which is the entire point of metering. Keep a small bounded head as well and prefer it for the model, falling back to the tail. 2. The proxy forwarded `accept-encoding` untouched, so a gzip or Brotli body reached the metering tap as compressed bytes and every token field came out null. This is the DEFAULT path, not an edge case: the engine is a Go binary and Go's net/http adds `Accept-Encoding: gzip` on its own. Ask upstream for identity while metering. Left alone when metering is off — nothing then justifies giving up compression. 3. Token accounting sat at the end of the review shell, which `exit 1`s on wall-clock timeout, unusable engine output, and policy blocks. Those are exactly the runs whose spend you want to see, since the tokens were spent either way, and the final cleanup deletes cr-usage.jsonl so the numbers were unrecoverable. Moved to its own `always()` step ahead of cleanup. DECLINED — cancel queued limiter admissions for disconnected clients. The mechanism is real: `acquire()` resolves and takes a slot before the callback notices `clientGone`. But `createRateLimiter` returns a no-op acquire when `maxRpm <= 0`, and action.yml never sets `CR_MAX_RPM`, so on the shipped path this code cannot run. Making admission cancellable means restructuring the limiter to buy nothing on any path we ship. Worth revisiting if and when a rate ceiling is actually configured. Tests: 3 added, and each was checked against a reverted fix — the two fact-proxy tests fail without their fix and the third (accept-encoding untouched when metering is off) passes either way by design. fact-proxy.test.mjs is 38/38. --- action.yml | 18 ++++++++-- scripts/fact-proxy.mjs | 35 ++++++++++++++++-- scripts/fact-proxy.test.mjs | 72 +++++++++++++++++++++++++++++++++++++ 3 files changed, 119 insertions(+), 6 deletions(-) diff --git a/action.yml b/action.yml index 45572fa..0ecc61b 100644 --- a/action.yml +++ b/action.yml @@ -711,7 +711,6 @@ runs: MAX_TOOLS: ${{ inputs.max-tools }} METER: ${{ inputs.meter }} USAGE_FILE: ${{ runner.temp }}/cr-usage.jsonl - USAGE_SUMMARY: ${{ github.action_path }}/scripts/usage-summary.mjs # The proxy writes the guardrail/firewall reason here on a block; a later # always()-step posts it to the PR (the cascade itself fails closed). POLICY_BLOCK: ${{ runner.temp }}/policy-block.json @@ -995,8 +994,21 @@ runs: # >1 only in exhaustive mode; the summary comment notes it. echo "passes=$PASSES" >> "$GITHUB_OUTPUT" - # Token accounting for this run. Purely informational and soft-fail: - # `|| true` so a metering bug can never turn a completed review red. + # Token accounting for this run — a separate always() step rather than the + # tail of the review shell above. That shell `exit 1`s on wall-clock timeout, + # unusable engine output, and policy blocks, and those are precisely the runs + # whose token spend you most want to see: the tokens were spent either way. + # The final cleanup step deletes cr-usage.jsonl, so accounting not printed + # here is gone for good. Soft-fail throughout — metering reports on a run's + # verdict, it never changes it. + - name: Token accounting + if: always() + shell: bash + env: + METER: ${{ inputs.meter }} + USAGE_FILE: ${{ runner.temp }}/cr-usage.jsonl + USAGE_SUMMARY: ${{ github.action_path }}/scripts/usage-summary.mjs + run: | if [ "$METER" = "true" ] && [ -s "$USAGE_FILE" ]; then echo "::group::Token accounting" node "$USAGE_SUMMARY" "$USAGE_FILE" || true diff --git a/scripts/fact-proxy.mjs b/scripts/fact-proxy.mjs index c5911d7..dca7aa4 100644 --- a/scripts/fact-proxy.mjs +++ b/scripts/fact-proxy.mjs @@ -109,6 +109,13 @@ const defaultSleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); // tail is sufficient — and it keeps the proxy's memory flat regardless of how // large the completion is. const USAGE_TAIL_BYTES = 64 * 1024; +// ...but `model` sits near the START of that same body, so the tail alone loses +// it once the body outgrows the tail — reachable on a non-streaming call with a +// large completion. A record with model:null is one usage-summary groups under +// "(unknown)" and cannot price, which is the whole point of metering. Keep a +// small head too; `model` appears within the first few hundred bytes, so this +// stays bounded and the proxy's memory stays flat. +const USAGE_HEAD_BYTES = 4 * 1024; // Client-side request-rate ceiling (CR_MAX_RPM). Some models enforce a hard // per-minute request quota that cannot be raised. The engine fans out per-file @@ -193,7 +200,10 @@ function extractModel(text) { // Append one metering record. Never throws: a full disk or a bad path must not // fail a review that otherwise succeeded. -function recordUsage(tailText, { usageFile, status, retries, seq, error = null }) { +// `headText` is the start of the same response when the caller has it (the tap +// keeps one); callers holding the WHOLE body may omit it, since the tail is then +// the whole body too. +function recordUsage(tailText, { usageFile, status, retries, seq, error = null, headText = null }) { if (!usageFile) return; try { const usage = extractUsage(tailText); @@ -209,7 +219,7 @@ function recordUsage(tailText, { usageFile, status, retries, seq, error = null } seq, status, retries, - model: extractModel(tailText), + model: (headText ? extractModel(headText) : null) ?? extractModel(tailText), prompt: usage?.prompt_tokens ?? null, completion: usage?.completion_tokens ?? null, total: usage?.total_tokens ?? null, @@ -319,6 +329,14 @@ export function createProxyServer({ const mySeq = usageFile ? ++callSeq : 0; const headers = { ...req.headers }; delete headers.host; // must match upstream, not the loopback proxy + // The metering tap reads raw response bytes, so a gzip/br body would leave + // every token field null. This is the DEFAULT path rather than an edge case: + // Go's net/http adds `Accept-Encoding: gzip` on its own and decompresses + // transparently, so the engine asks for compression without being told to. + // Ask upstream for identity while metering — the cost is one + // loopback-to-gateway compression win, and the alternative is a meter that + // silently measures nothing. + if (usageFile) headers["accept-encoding"] = "identity"; Object.assign(headers, readFacts(factsFile)); const target = new URL(req.url, upstream); @@ -405,7 +423,12 @@ export function createProxyServer({ // consume them, so the relay below is byte-for-byte unchanged. if (usageFile) { let tail = Buffer.alloc(0); + let head = Buffer.alloc(0); // carries `model`; see USAGE_HEAD_BYTES upRes.on("data", (c) => { + if (head.length < USAGE_HEAD_BYTES) { + // Copy out of the concat so the slice does not retain it. + head = Buffer.from(Buffer.concat([head, c]).subarray(0, USAGE_HEAD_BYTES)); + } const joined = tail.length ? Buffer.concat([tail, c]) : c; tail = joined.length > USAGE_TAIL_BYTES @@ -413,7 +436,13 @@ export function createProxyServer({ : joined; }); upRes.on("end", () => { - recordUsage(tail.toString("utf8"), { usageFile, status, retries, seq: mySeq }); + recordUsage(tail.toString("utf8"), { + usageFile, + status, + retries, + seq: mySeq, + headText: head.toString("utf8"), + }); }); } res.writeHead(status, outHeaders); diff --git a/scripts/fact-proxy.test.mjs b/scripts/fact-proxy.test.mjs index 0160490..2fcf7c3 100644 --- a/scripts/fact-proxy.test.mjs +++ b/scripts/fact-proxy.test.mjs @@ -839,6 +839,78 @@ describe("usage metering (CR_USAGE_FILE)", () => { await upstream.close(); } }); + + test("records the model even when the body outgrows the metering tail", async () => { + const usageFile = join(dir, "usage-bigbody.jsonl"); + // `model` at the top, `usage` at the bottom, and more than the tail's worth + // of content between them — the shape of a non-streaming call with a large + // completion. A tail-only tap keeps the tokens but loses the model, and a + // record usage-summary cannot price is the one thing metering must not emit. + const big = JSON.stringify({ + id: "cmpl-big", + model: "vendor/model-a", + choices: [{ message: { content: "x".repeat(96 * 1024) }, finish_reason: "stop" }], + usage: { prompt_tokens: 8000, completion_tokens: 120, total_tokens: 8120 }, + }); + assert.ok(big.length > 64 * 1024, "fixture must exceed the tail or it proves nothing"); + const upstream = await startUpstream([{ status: 200, body: big }]); + const proxy = await startProxy({ + upstreamUrl: `http://127.0.0.1:${upstream.port}/v1/chat/completions`, + usageFile, + }); + try { + const res = await request(proxy.port, { body: "{}" }); + assert.equal(res.status, 200); + assert.equal(res.body, big, "the relayed body is still byte-identical"); + const rows = readMeter(usageFile); + assert.equal(rows.length, 1); + assert.equal(rows[0].model, "vendor/model-a", "model comes from the head; the tail alone is null here"); + assert.equal(rows[0].prompt, 8000, "usage still comes from the tail"); + } finally { + await proxy.close(); + await upstream.close(); + } + }); + + test("asks upstream for identity encoding while metering — a gzip body would meter nothing", async () => { + const usageFile = join(dir, "usage-identity.jsonl"); + const upstream = await startUpstream([{ status: 200, body: bodyWithUsage() }]); + const proxy = await startProxy({ + upstreamUrl: `http://127.0.0.1:${upstream.port}/v1/chat/completions`, + usageFile, + }); + try { + // Go's net/http adds this header on its own, so the engine asks for + // compression without being configured to — this is the default path. + await request(proxy.port, { body: "{}", headers: { "accept-encoding": "gzip, br" } }); + assert.equal( + upstream.seen[0].headers["accept-encoding"], + "identity", + "the tap reads raw bytes, so a compressed body would leave every field null", + ); + } finally { + await proxy.close(); + await upstream.close(); + } + }); + + test("leaves accept-encoding alone when metering is off", async () => { + const upstream = await startUpstream([{ status: 200, body: bodyWithUsage() }]); + const proxy = await startProxy({ + upstreamUrl: `http://127.0.0.1:${upstream.port}/v1/chat/completions`, + }); + try { + await request(proxy.port, { body: "{}", headers: { "accept-encoding": "gzip, br" } }); + assert.equal( + upstream.seen[0].headers["accept-encoding"], + "gzip, br", + "no tap to feed, so nothing justifies giving up compression", + ); + } finally { + await proxy.close(); + await upstream.close(); + } + }); }); // Rate ceiling (CR_MAX_RPM). Some models enforce a hard, unraisable per-minute