mirror of
https://github.com/Continuum-AI-Corp/Orca-Code-Review.git
synced 2026-09-29 21:48:00 +00:00
6 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Zhenghua Bao
|
6d253e0399 |
feat: raise the diff limits so a large PR gets reviewed instead of refused (#53)
512 KB / 300 files / 20 minutes becomes 5000 KB / 2000 files / 60 minutes. The old limits were a ceiling on WASTE — chosen so tokens were not spent on diffs that review badly. The side effect was that a large PR could never be reviewed at all, and "never" is worse than "expensive". Size alone was never the right test either: the engine reads the diff through its own tooling rather than taking it as one prompt, so a 2 MB diff is not automatically beyond it. What does bound it is the model's context window, which the action cannot see — it is given a model string, and the actual route is the workspace's recipe. So this trades a clean, cheap refusal for a real attempt that may end in a wall-clock timeout or an aborted pass. That is a deliberate trade and it is written into the input description, including how to get the old behaviour back (lower the number). The timeout had to rise with the limits, not after them. At 20 minutes a review allowed to accept a much larger diff would have been killed halfway — the same non-review as before, with a worse message. It is a per-pass ceiling and `exhaustive` makes up to three passes, which is now worth saying out loud because 3x60 is a different proposition from 3x20. The two limits are counted separately because they describe different shapes: a mechanical rename across 900 files is small and shallow, one 4 MB generated file is large and shallow, and either can be worth reviewing. Every place that states these numbers moved together — the inputs, the guard's own fallbacks, the oversized-diff notice posted on the PR (whose example values were 1024/500, which after this change would have been advice to LOWER the limit), the example workflow, the skill's workflow template, and the skill's input and troubleshooting references. The guard's fallbacks are pinned to the inputs by a test now, because they are one number in two places and the second one is reachable: action.yml passes `--max-kb "$MAX_KB"` quoted, so `max-diff-kb: ""` arrives as an empty argument and lands on the fallback. A stale number there would enforce a limit nobody documented, silently. The test reads the default out of action.yml rather than restating it, and is mutation-checked in both directions — editing only the script reds it, and so does editing only action.yml. That test also turned up a defect in itself, which is worth recording because its failure mode was invisible: it anchored on bare "\n" while this repo has no `.gitattributes` and `core.autocrlf` is the Windows default, so action.yml is CRLF in every Windows checkout. It would have passed on Linux CI forever and failed on every developer machine — and CI green reads as "works". Newlines are normalized on read now, mutation-checked against the CRLF working copy. 567 tests, 558 passing, failure set identical to main's (9 pre-existing and unrelated). The file-count fixture is ~180 KB, far under the size limit, so it still tests the count and not the size. |
||
|
Zhenghua Bao
|
b4fe7ee66b |
feat: one review tier, and a report_on severity filter (#28)
* feat: one review tier, and a report_on severity filter
The recipe routes every branch to one model, and action.yml has hardcoded
PASSES=1 with no promotion for a while now. This drops what was left of the
two-tier machinery and adds the publish-side severity filter the dashboard
already offers.
Single tier
- action.yml: 22 steps -> 21. Gone: the PR-label read (one fewer paginated
API call per run), the promote step, the second per-tier report step, and
TIER_STATE / RESULT_CHEAP / PROMOTE / HELD / strong_ran.
- The tier reported to the control plane is `standard`. It used to be
`strong`, from when a cheap pass could escalate; with one pass the label
described the reviewer rather than the run.
- RESULT_STRONG -> RESULT_FINAL, result-strong.json -> result-final.json.
- summary-comment.mjs: the `Tier:` line is gone, and the blocking count now
always follows --block-on. It used to follow --fix-first on a held run,
which meant the summary and the merge gate could disagree about what
blocks the PR.
report_on
- New scripts/report-filter.mjs, applied between the engine and the quiet
filter: result.json -> result-reported.json -> result-posted.json.
- The published set is report_on UNION block_on, so a severity that blocks
the merge can never be withheld from the timeline.
- Absent report_on (a gateway that predates the field) is pass-through;
an empty value means "only what blocks".
- The gate and the run report keep reading the unfiltered result. Display
filtering never moves enforcement or the numbers.
Docs and template
- README, the workflow template and the recipes describe one review per
push instead of a fast-then-strong escalation.
Tests
- settings.test.mjs: the action.yml wiring assertions now go through a
sliceStep helper that fails when a boundary step is missing. Two of them
had been silently matching most of the file since their end-boundary step
was removed.
- fact-proxy.test.mjs: same fix, same cause.
- report.test.mjs: asserts every report.mjs invocation sits behind the
`report` input, as a count equality rather than a fixed number.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix: an empty report_on is a setting, not a missing one
Review round 1 found four real problems, three of them introduced here.
The report_on step guarded --show on `[ -n "$REPORT_ON" ]`, which sent an
empty value down the no-flag branch — and report-filter.mjs reads an absent
flag as "no setting, pass everything through". So a workspace configured as
"publish only what blocks" got every severity published instead: the exact
inversion the comment above it claimed to be preventing. --show now goes out
unconditionally. It is safe to do that because the settings step always
writes a value (the settings-disabled path writes every severity
explicitly), and if that step fails outright it fails the job rather than
leaving the output empty.
report-filter.mjs shipped with no tests, in a repo where every other script
has a .test.mjs, and the bug above sat exactly in the seam that most needed
one. New report-filter.test.mjs covers the three states of --show, the union
with block-on, the shared severity parsing, and the usage exits.
settings.test.mjs pins the unconditional --show so the guard cannot come
back.
The `fix-first` input description promised the severities were sent to the
reviewer to fix first. They are not, and were not: the only consumer is the
exhaustive early-stop loop, so the input does nothing at all on a default
(non-exhaustive) run. Description rewritten to say that. Its env line on the
summary and gate steps is now dead — those consumers went with the held-run
branch — so both are removed; the Review step keeps its copy.
The startup cleanup lost result-cheap.json and result-strong.json when the
filenames changed, but a reused self-hosted runner hard-cancelled under the
older action still has them, holding finding text and source snippets. That
step exists for exactly the files an earlier cleanup never reached, so the
legacy names go back alongside the new ones.
The recipe still documented x-cr-prev-tier as none | cheap | strong while
inviting workspace owners to write rules on it, and it had grown a second
copy of the fact block. One block now, documenting the one value, and saying
plainly that a rule matching cheap or strong will fall through to the
default.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs: report_on in the README, and detach the retired tier label
Review round 2, four findings, all real.
The README promised P2 → Comment unconditionally and did not mention the
Report severities control at all. With report_on narrowed — and the shipped
default for a new workspace IS narrower than every severity — a P2 finding is
counted in the summary and never posted on the diff, with nothing anywhere
explaining the gap. The severity table now has a "Posted inline" column and a
P3 row, and says what the two settings are: merge policy decides what blocks,
Report severities decides what is posted, a blocking severity is always posted
whatever the display setting says, and the summary always counts everything.
The bundled setup skill still taught the cascade in four places. The installer
copies it and agents treat it as the configuration authority, so it was
actively teaching removed behaviour: `fix-first` described as withholding the
strong tier, `exhaustive` as extra passes on the strong tier, and SKILL.md
calling fix-first an escalation control. Fixing SKILL.md also fixed a
pre-existing platforms.test.mjs failure.
The workflow template's fix-first comment carried the same over-promise the
input description had ("ask for a concrete fix on these first"). It is the
exhaustive early-stop set and nothing else.
An older version created the repository label `orca-review:strong` and attached
it to a PR when it was promoted, to carry the tier between runs. Nothing reads
or writes it now, so a PR promoted before the upgrade keeps a label asserting a
tier that no longer exists — state this action created and would otherwise
abandon on someone's PR. New step detaches it: a blind removeLabel swallowing
404, not list-then-remove, because reading the labels is the paginated call this
change deleted and paying it every run to discover whether a migration artifact
is present costs more than the removal. continue-on-error, because cleanup must
never turn a passing review red. Before the gate, because the gate exits
non-zero on a blocking finding and a blocked PR is exactly the kind that was
promoted. Deletable once no open PR predates the release, and the comment says
so.
Also swept two stale comments the earlier commit missed: summary-comment.mjs
still documented the held-run exception as live, and action.yml called
RESULT_FINAL a per-tier snapshot overwritten by the escalation pass. And the
new step landed between the summary and the gate, so the four test slices that
used the gate as the summary's end boundary now stop at the new step instead —
including one that was still a bare indexOf pair.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Revert the retired-label cleanup
Assume no open PR carries `orca-review:strong`. The step was migration
cleanup for a label an older version attached, and it cost an API call on
every run, for ever, to undo a one-time artifact — not worth carrying in the
action for that.
Reverts the step, its test, and the four slice boundaries that had moved to
it.
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Zhenghua Bao
|
e6a65c48cc |
Sync the product name to "OrcaCode Review", accept all four trigger spellings (#12)
* 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.
|
||
ZhenghuaBao
|
bc28316975 |
rebrand: OrcaRouter Code Review → Orca-Code-Review; drop Open Code Review credit line from README
Renames the product to "Orca-Code-Review" across the action name, brand default, rendered PR-comment header, examples, recipe, and internal comments. Removes the visible "Review engine powered by Open Code Review" line from the README; the Apache-2.0 attribution to Alibaba remains in NOTICE (compliant). LICENSE unchanged. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com> |
||
Claude
|
7b1d04b107 |
fix: adversarial-review fixes — proxy crash/hang/idempotency, merge-gate integrity, settings authority
- fact-proxy: retries can never fire after a response began relaying
(double-writeHead crash); per-attempt settled latch kills duplicate
retry chains; streaming relay destroys the client on upstream error
(no more 6h hangs); connection-error retries only when provably
pre-send (ECONNREFUSED/ENOTFOUND/EAI_AGAIN or pre-flush); bodies over
8MiB stream through with retries disabled
- oversized-diff skip now FAILS the required check by default
(on-oversized-diff: fail|pass) — padding a diff no longer bypasses the
P0/P1 merge gate; diff-guard stats before reading (no more loading a
300MB diff to decide to skip it)
- summary gate line counts against the configured --block-on set instead
of hardcoded P0+P1
- exhaustive-merge.mjs de-binarized (NUL bytes → \u0000 escapes: git can
diff it again); exhaustive extras restricted to the strong tier and
break early once fix-first findings exist
- new settings input ('false' = workflow file authoritative); settings
gate widened to all pull_request* events
- stale-file init step (16 run temp files cleared first) kills false
guardrail-block comments on reused runners; stale skip notices deleted
when reviews resume; proxy-startup sed race fixed
- severity literals import SEVERITIES from severity.mjs
- node:test: 109 pass (94 + 15 new)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AC1sVtj2Jc9cAX7FENZ1hG
|
||
|
Claude
|
f2c72fc88f |
feat: retry/backoff proxy, oversized-diff guard, run report-back, edit-in-place summary comment
- fact-proxy buffers bodies and retries 429/502/503/504 + connection errors (3 attempts, 1s/2s/4s, Retry-After honored ≤30s), x-cr-retry-count header; SSE response streaming unchanged - diff-guard.mjs: review/skip decision (max-diff-kb 512, max-diff-files 300 inputs); skip posts an explaining comment and passes the check fail-open - severity.mjs extracted from gate.mjs (leading-tag + untagged→P1 fail-safe shared, gate contract tests unchanged) - report.mjs: best-effort POST run summary (counts/sha/tier/gate only) to <gateway origin>/api/code_review/report; 5s timeout, one retry, never fails the job; report:false opt-out input; per-tier wiring in action.yml - summary-comment.mjs: single upserted PR comment with severity table, Δ vs previous push via embedded orca-cr-state JSON, tier + gate lines - README: new inputs, run-reporting privacy note - node:test suites: 59 pass Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AC1sVtj2Jc9cAX7FENZ1hG |