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..0ecc61b 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,12 @@ 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 # 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 +739,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 +821,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 +994,27 @@ runs: # >1 only in exhaustive mode; the summary comment notes it. echo "passes=$PASSES" >> "$GITHUB_OUTPUT" + # 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 + 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 +1107,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 +1458,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..dca7aa4 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,133 @@ 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; +// ...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 +// 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. +// `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); + 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: (headText ? extractModel(headText) : null) ?? 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,13 +311,32 @@ 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 + // 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); @@ -211,7 +370,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 +404,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 +418,33 @@ 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); + 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 + ? Buffer.from(joined.subarray(-USAGE_TAIL_BYTES)) + : joined; + }); + upRes.on("end", () => { + recordUsage(tail.toString("utf8"), { + usageFile, + status, + retries, + seq: mySeq, + headText: head.toString("utf8"), + }); + }); + } res.writeHead(status, outHeaders); upRes.pipe(res); // stream SSE through unbuffered // pipe() forwards data, not errors — and http.IncomingMessage swallows @@ -276,6 +473,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 +555,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 +571,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 +600,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 +651,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..2fcf7c3 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,504 @@ 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(); + } + }); + + 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 +// 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