Sync the product name to "OrcaCode Review", accept all four trigger spellings - #12
Conversation
…pellings
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.
|
@codex review |
1 similar comment
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8df4a6c52f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| upRes.on("end", () => { | ||
| recordUsage(tail.toString("utf8"), { usageFile, status, retries, seq: mySeq }); |
There was a problem hiding this comment.
For a non-streaming response larger than 64 KiB, only this tail is passed to recordUsage, but OpenAI-shaped JSON places model near the beginning and usage at the end. This is reachable through the non-streaming judge call, which permits up to 32k output tokens; such calls retain their token counts but record model: null, so usage-summary.mjs groups them as (unknown) and cannot calculate their per-model cost. Capture the model from the prefix or while streaming rather than extracting both fields from the bounded tail.
Useful? React with 👍 / 👎.
| # 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::" |
There was a problem hiding this comment.
When the mandatory engine pass times out, produces unusable output, or hits a policy block, run_review exits the shell at lines 918–946 before this block is reached. Those are exactly the costly failure cases that the proxy explicitly records, but the later always() cleanup deletes cr-usage.jsonl, so their accounting is never printed or recoverable. Move this soft-fail summary into an always() step (before cleanup) so metering covers failed runs as advertised.
Useful? React with 👍 / 👎.
| limiter.acquire().then(() => { | ||
| if (clientGone || res.destroyed) return; | ||
| dial(body, retries); | ||
| }); |
There was a problem hiding this comment.
With CR_MAX_RPM enabled, a request that disconnects while waiting remains in the limiter's serialized queue. When its turn eventually arrives, acquire() records a rate-limit slot before this callback notices clientGone, so the dead request sends nothing but still consumes capacity; a burst of timed-out per-file requests can consequently reserve successive minute windows and delay later live requests until the review's wall-clock timeout. Admission needs to be cancellable, or disconnected entries must be removed before they reserve a slot.
Useful? React with 👍 / 👎.
| 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)) |
There was a problem hiding this comment.
When the client advertises gzip or Brotli and the upstream honors it, these chunks contain compressed bytes rather than JSON because the proxy forwards accept-encoding and does not decompress the metering tap. The response still reaches OCR successfully, but every metering field is recorded as null or zero; this can affect ordinary HTTP clients that advertise compression by default. Either request identity encoding upstream or decode a separate copy for extraction while preserving the relayed bytes.
Useful? React with 👍 / 👎.
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.
|
Fixed 3 of 4 in 1. 2. Compressed responses — fixed, and this was the worst of the four. 3. Token accounting on failed runs — fixed as you suggested: its own step with 4. Cancel queued limiter admissions — declined. The mechanism is real; I confirmed Tests: 3 added, each validated by reverting its own fix — the two behavioural tests fail without the fix, and the third (accept-encoding untouched when metering is off) passes either way by design, which is the point. @codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Two concerns, one release since they both touch
action.yml.Naming
The Action called itself
Orca-Code-Reviewwhile the App brands its comments OrcaCode Review — one product signing the same PR two ways. This renames the display name only: actionname, thebranddefault, 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:
<!-- orca-code-review-* -->upsert markers<!-- orca-cr-summary:start/end -->uses:, and the.github/workflows/filenameThe summary heading is safe to change precisely because nothing matches on it — upsert goes through the marker, and the push counter through
<!-- orca-cr-state: … -->. Verified: marker counts are byte-identical tomain, and no line in the diff touches a marker or auses:.Trigger
The example workflow gated on
/orca-code-review— a spelling the App deliberately dropped — so the command this repo documented did nothing on the App path.Now accepts the full cross of both prefixes and both separators, matching the App exactly:
The cross is the point rather than a convenience. A partial set is a trap: the reader who writes the one spelling you left out gets no run, no comment, and no error to explain why.
Metering
This part was already staged in the working tree and is carried here rather than reverted, because it answers a question we currently cannot answer: what a review actually costs.
fact-proxy.mjsgainsCR_USAGE_FILEper-call token accounting andCR_MAX_RPM. It keeps only a bounded tail of each response (usage is last in an OpenAI body) so SSE stays unbuffered, and every extraction and append is soft-fail — metering is observability and must never gate a review.scripts/usage-summary.mjsturns that log into a per-model cost. It resolves the provider-echoed model name against the gateway's vendor-prefixed price keys, and refuses to guess when a basename maps to several vendors — a wrong ratio is worse than an honest gap. Models with nocache_ratioare billed at full input rate and flagged, because that is what the gateway does.concurrency(default24— the engine's own default of 8 was never set, which shows up as per-file timeouts on slower models),max-tools,meter.Testing
settings.test.mjsare a pre-existing libuv crash on Windows + Node 24 (UV_HANDLE_CLOSING, exit0xC0000409) — identical on an unmodified tree, confirmed by stashing.name,branddefault, and the expandedifverified after parse.usage-summary.mjssmoke-tested against a hand-built JSONL: vendor-prefix resolution, snapshot-suffix stripping, missing-cache_ratioflagging, malformed-line skip with exit 0, and the arithmetic checked by hand.Reviewer note
usage-summary.mjsships without a test file, unlike every other script here. It is observability-only, never gates, and exits 0 on malformed input — but that is a real gap, not an argument that it doesn't need one.Separately:
brandis documented as "Name shown on PR comments", yetsummary-comment.mjshardcodes the heading instead of reading it. Pre-existing, left alone here to keep this a rename; worth a follow-up.