mirror of
https://github.com/Continuum-AI-Corp/Orca-Code-Review.git
synced 2026-09-29 21:48:00 +00:00
e6a65c48cc* Sync the product name to "OrcaCode Review", accept all four trigger spellings
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 `<!-- orca-code-review-* -->` 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.
- `<!-- orca-cr-summary:start/end -->`, 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
`<!-- orca-cr-state: … -->`.
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.
* Fix three metering defects found in review
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.
53 lines
2.7 KiB
53 lines
2.7 KiB