Orca-Code-Review

mirror of https://github.com/Continuum-AI-Corp/Orca-Code-Review.git synced 2026-09-29 21:48:00 +00:00

Author	SHA1	Message	Date
Zhenghua Bao	a49ffb5981	chore(summary): drop --fix-first and --held, which nothing reads (#55 ) Both flags survived the cascade that gave them meaning. --held marked a cheap pass withholding a strong review, and --fix-first let the blocking count follow the fix-first set on such a run, because block-on need not contain those severities. There is no withholding and no tier line any more, so the count has followed block-on alone for some time — while the two flags went on being parsed, and --fix-first went on being validated, with neither value read by anything downstream. fix-first REMAINS an action input. It stops the exhaustive loop early (action.yml), which is a different question from what the summary counts; the header now says so, so the name cannot argue its way back into this count later. Removing a flag the script no longer understands is safe to ship on its own: the argument loop has no else branch, so an unrecognised flag and the value after it are skipped one token at a time without shifting the flags that follow. The surviving block-on test now passes --fix-first in the MIDDLE of the argv to hold that property down. Also scopes the action-inputs reference to this action: the hosted App runs a different review engine, so fix-first and exhaustive can behave differently there, and the file now says to answer App questions from the console instead. Test suite is unchanged by this: 558 pass / 9 fail before and after, the failures being a pre-existing CRLF mismatch in the action.yml input tests on a Windows checkout. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-09-21 10:09:40 +08:00
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.	2026-09-09 15:02:50 +08:00
Zhenghua Bao	ff658d1e92	fix: fail closed when the L2 judge is unavailable, and name which gate closed (#52 ) * fix: a transient 5xx should not cost a whole review The judge call had no retry. Every other LLM call in the pipeline retries 5xx four times; this one fetched once and exited on anything that was not 200. A single 502 was enough to take it out — and with precision filtering on, that now takes the review with it. It retries the same statuses, with the same bounds, and reports the body of the LAST failure so the log names the actual upstream reason instead of "attempt 4 failed". L2 is no longer soft-fail, which is the change that makes the retry matter. L1's output is not a shippable review: it is the engine's raw findings with mismatched snippets re-homed, still carrying the duplicate and low-value ones L2 exists to cluster and drop. Publishing it because the judge was unreachable shipped exactly the noise the filter was turned on to remove, and it arrived looking like a finished review. `precision-filter: "false"` remains the supported way to run without a judge — it skips L1 too, which is the honest version of that choice. The input docs say so now. The failure messages were all the same sentence. An engine crash, a partial run, and an unavailable judge printed one "no usable result", so a reader could not tell which gate closed and picked whichever cause was last in the log — which is how a run whose judge logged a 502 just before an unrelated failure got read as the judge throwing the review away. Each cause now names itself in the summary, and check-result tags its four reasons for anyone parsing. The report_on failure default was every severity, on the reasoning that a workspace predating the setting resolved to all four so no single value could match what it normally sees. That premise expired: the gateway's own default is P0,P1 now for any workspace that has not set the field, so all-severities matched nothing and a settings outage published P2/P3 to installs that would never have been shown them. Which is the wrong direction twice over, because the outage that opens this up is the one most likely to have degraded the filtering that keeps those severities useful. Tests: 6 new, covering retry on each retryable status, the bound (exactly 4 attempts, last body reported), and that a 400 is not retried. Mutation-checked by forcing attempts to 1 — five of them fail. Full suite goes 526 -> 533 passing with an identical failure set (10 pre-existing, unrelated). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: withdraw the judge retry — the proxy already owns it The retry added to judge.mjs was wrong, and wrong in a way that would only have shown up on the bill. In production OCR_LLM_URL points at the local fact proxy this action starts, and that proxy already retries 429/502/503/504 four times with backoff. A loop in judge.mjs does not add resilience, it multiplies: four attempts here times four there is sixteen upstream requests for a single judge call. It was also a wider status set than the proxy's, and not by accident on the proxy's part — it excludes 500 precisely because a 500 can mean the completion was produced and billed, so replaying it buys the same bill twice. The outer loop would have re-sent exactly that, bypassing a policy it did not know was there. So the 502 that prompted this was never an unretried call. It was the answer after the proxy had already spent its retries, and no amount of asking again would have changed it. What replaces the code is a comment saying why the retry is deliberately absent, and a test that pins the request count at one — so the next reader with the same good idea meets a red test instead of a merge. Also in this round: - The judge-unavailable marker was written and read but never cleaned up, so a second pass in the same job could inherit the first pass's failure and report a judge outage that had not happened. It now clears with the other per-pass markers in both cleanup paths. - A result carrying `warnings` is already doomed — check-result fails closed on it — so filtering it spent a judge call on output nobody would read, and if that call failed, the summary blamed the judge for a run whose real defect was a partial engine result. That is the exact mis-attribution the reason= tags exist to prevent. Such a result now falls straight through to the check, which reports reason=partial and names the warnings. * fix: read `warnings` the way the check reads it, or the skip publishes unjudged The skip-the-filter gate added last round tested `(r.warnings\|\|[]).length`. check-result.mjs tests `Array.isArray(parsed.warnings) ? parsed.warnings : []`. On any non-array value the two disagree, and they disagree in the one direction that matters. A `warnings` of the string "oops" measures 4, so the gate called the result partial and skipped L1 and L2 — on the reasoning that the check would reject it anyway. The check normalized the same value to `[]` and published. Skip plus publish is the engine's raw findings, unclustered and unjudged, shipped under `precision-filter: true` and arriving as a green review: precisely what the fail-closed judge branch in this PR exists to prevent, reintroduced by the optimization meant to support it. Reproduced before fixing and re-checked after, across every shape a `warnings` field can take — non-empty array, empty array, absent, null, string, number, object. Only the genuine non-empty array now skips, and that is the one the check stops. This also pins the direction the predicate has to fail in. Skipping is the branch that can publish unjudged, so it may only be taken when the result is definitely partial; anything malformed or unrecognized runs the filter. The regression test extracts the real predicate out of action.yml and runs it, rather than restating it, so the two cannot drift apart silently again. Its match is deliberately loose — it does not require the `Array.isArray` spelling — because a test that only detects a missing token would have gone red for the wrong reason. Mutation-checked by restoring the old predicate: the string, number and object cases fail with "filter skipped AND CHECK published". * fix: name the judge failure instead of claiming it exhausted retries "the L2 judge was unavailable after retries" was true for one failure class and false for the rest. The judge sends one request; the proxy behind it retries only 429/502/503/504. A 400, a 401, an unparseable completion envelope and a model that answered in prose all reached that same sentence, which sent the reader to wait out a transient outage that was not happening — the same wrong-cause reading this branch exists to end, one layer up. The reason is now carried instead of asserted. The judge's output is captured rather than only streamed, its first line becomes the marker's content, and the summary quotes the marker the way the wall-clock branch already did — the marker was being written and then ignored in favour of a literal, so that write had no effect at all. First line, not last: the judge prints "judge did not return JSON:" followed by a dump of what the model actually said, so a tail would quote a fragment of the bad completion in place of the reason. Two cases the log alone could not name: - The judge exiting 0 while writing nothing was indistinguishable from a failed call. It now says so. - An unreachable gateway had no named class at all. `fetch` rejects rather than returning a response when it cannot reach the endpoint, so the process died on an unhandled rejection and the first line of stderr was a path inside undici — which is what the summary would have quoted as the failure class. It is caught and named now, with the cause included, because fetch collapses every transport failure into "fetch failed" and hides ECONNREFUSED, DNS failures and header timeouts in `cause`. This one is a regression from withdrawing the retry: the loop's try/catch had been printing a named message, and removing the loop took it with it. Verified by running all five classes, not by reading. Against a stub gateway, each failure names itself on the first line: HTTP 400, HTTP 401, bad completion envelope, judge did not return JSON, and could not reach the LLM (ECONNREFUSED for a closed port). The action's plumbing was then driven with stub judges for each class, and the summary line came out distinct and accurate every time, including "it exited cleanly but wrote no output" and "it exited 9 without a message". Both new tests are mutation-checked: dropping the catch fails the unreachable test on the no-stack-trace assertion, and switching the first-line assertion to last-line semantics fails the class test. * fix: the captured judge log needs cleaning, and a green run must not claim it failed Two misses from the round that added the captured log, both in the same shape: a new artefact was introduced without updating the place that already handles every artefact of its kind. The log itself was never cleaned. The cleanup lists enumerate `result.l1.json`, `result.l2.json` and both `-extra` variants by name — deliberate and exhaustive — and `result.l2.log` was simply not added to either. On a reused self-hosted runner, and especially after a hard cancellation, that leaves judge diagnostics behind for the next job: finding paths, root-cause text, and on a malformed-completion failure up to 600 characters of model output. Verified by running the failure paths and listing what survives: after an HTTP 401 and after a judge that exits 0 without writing, every leftover file is now named in the cleanup list. The second is a false claim rather than a leak. `::error:: … failing closed` was printed by the judge branch itself, but whether a failed judge ends the run belongs to the caller: on the mandatory pass it is fatal, on a best-effort exhaustive pass `run_review` warns, keeps the findings already in hand, and the job goes green. So an exhaustive pass-2 judge failure produced a green run carrying a red annotation announcing a failure that had not happened — the same register of wrong claim this whole pass exists to remove. The branch now states the fact as a plain line inside the group the operator is already reading, and both callers keep owning the consequence. This is what the wall-clock marker directly above it already does. The downgrade also carries the reason now. "exhaustive pass 2 produced no usable result" is what an engine crash reads as too, and the reason is the only thing that tells an operator whether to re-run or fix a key. Verified by extracting the real `run_review` and driving it with a judge that fails on the mandatory pass and then on the extra pass: the first yields `::error:: … failing closed` and a nonzero exit, the second only `:⚠️: … the L2 judge failed (HTTP 401 …) — keeping the findings so far` and exit 0. No `::error::` is emitted on the green path any more. * fix: the body read belongs inside the transport catch too The catch added last round covered `fetch` and stopped there. `fetch` resolves as soon as response headers arrive, so a connection that dies mid-body rejects at `res.text()` instead — outside the catch, from a different undici frame, still an unhandled rejection. Which the summary then quotes as the failure class, because it lifts the first line of the judge's stderr. So the operator is handed `node:internal/deps/undici/undici:12141` as the reason their review failed. Not hypothetical. fact-proxy relays headers first and destroys the connection on a mid-stream upstream failure — "the headers are out, so destroy the connection" is its own comment — and the proxy is what serves this call in production. So this is the transport failure the judge is MOST likely to meet, and it was the one still uncovered. Reproduced against a server that writes headers and then destroys the socket: `TypeError: terminated`, uncaught, first line inside undici. With the body read inside the try it reports `could not complete the LLM request: terminated (UND_ERR_SOCKET)`. The wording moves from "reach" to "complete" because by the time a mid-body drop lands there the request did reach the gateway; the cause code separates the two, ECONNREFUSED never arrived against UND_ERR_SOCKET arrived and was cut off. All six failure classes re-verified end to end after the change: HTTP 400, HTTP 401, bad completion envelope, judge did not return JSON, ECONNREFUSED, and the mid-body drop. Each names itself on the first line. Mutation-checked by moving the body read back outside the try: exactly the new test fails, and the unreachable-endpoint test stays green — they cover different frames, which is the reason the first fix looked complete. * fix: a malformed 200 must name itself, not report a line number The summary lifts the first line of the judge's stderr as the L2 failure reason. A response that parses but has the wrong shape threw outside every handler, so that first line was `judge.mjs:195` — a source location, which names nothing and implies no fix. Two entrances, both reproduced before fixing: - `content` present but not a string. Extraction succeeds and the `.replace` after it throws. Typed now where the raw response is still in hand to quote. - `groups` truthy and not an array. A string iterates by character and an object is not iterable at all, so this threw a few lines down. It fails now rather than falling open: falling open keeps every finding unjudged, the one outcome the judge exists to prevent. The falsy cases are unchanged — absent or null means the judge classified nothing, which the fail-open pass handles deliberately, and a test pins that the new guard does not sweep them up. Enumerating malformed shapes is always one shape short, so there is also a backstop: an uncaught throw or unhandled rejection prints one stable summary line and then the stack. Nothing is lost from the log, because the action echoes all of it and promotes only the first line. Verified against both a synchronous throw and a top-level-await rejection. All five reported shapes now name themselves: content null / number / object, groups string / object / number. Mutation-checked three ways — removing the content guard reds exactly the three content tests, removing the groups guard reds exactly the three groups tests, and removing both plus the backstop reds all six. Also: the compatibility note beside `report_on` said an omitted field "behaves as before". That stopped being true when the default moved from all four severities to P0,P1, and the sentence was left standing. It now states what actually happens, including that the report filter shows report_on UNION block_on and block_on defaults to P0,P1 too, so P2/P3 are not recovered elsewhere. The value is unchanged and deliberate: the gateway sets P0,P1 itself for any workspace that has not chosen one, so an omitted field means "never set", not "server too old to have the field". * fix: name what a valid `groups` IS, because the reject-list had falsy holes The guard added last round tested `parsed.groups && !Array.isArray(...)`. `false`, `0` and `""` are falsy AND non-arrays, so they walked straight past it into `\|\| []`: every finding came out unclassified, the fail-open pass kept all of them, and the judge exited 0. Which means a response that violated the schema published every finding as JUDGED, under `precision-filter: true`, as a green review. That is the exact outcome this branch fails closed to prevent — reintroduced by the guard that looked like it closed it. The test now names what is ALLOWED instead of what is rejected: `groups` may be absent or null, both meaning "the judge classified nothing" and handled by the fail-open pass on purpose, and anything else must be an array. An accept-list cannot have this shape of hole — a value that is neither exemption nor an array has nowhere to go but the failure branch, whatever type someone invents next. That framing is the actual fix here. This is the third time in this branch that enumerating bad values came up one value short, and each time the miss landed on the publishing side. The new tests assert more than the exit code, because the failure mode WAS a successful exit: they also assert that no output file is written. Mutation- checked by restoring the truthiness form, which reds exactly the three falsy cases and nothing else. Audited the sibling of the same shape rather than assuming: the precision gate in action.yml counts `(r.comments\|\|[]).length`, so a string `comments` reads as "has findings". Ran every non-array value through the gate and the check together — string, number, object, boolean, null, absent — and check-result requires `Array.isArray(parsed.comments)`, so all six fail closed. A string spends a judge call before being rejected, which is wasteful and safe; the publishing side is sound and left alone. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-09-09 14:13:25 +08:00
ankaifeng	b3c1229518	chore: keep only the GIFs the README shows The mp4 copies were never linked from the README once the captions went; the GIFs are the demo.	2026-09-03 10:40:08 +08:00
ankaifeng	7fcfb0f748	chore: drop the demo recording tapes The GIFs and mp4s under docs/ are the shipped artefacts; the vhs tapes that produced them were session-specific (a throwaway repo, this account's paths) and not something a contributor can re-run as written.	2026-09-03 10:36:18 +08:00
AnKaifeng	5dae0d66b5	feat: review locally with your own agent as the engine (#51 ) * feat: review locally with your own agent as the engine `review plan` prints everything a reviewer needs — files in scope with the reasons for exclusions, per-language checklists, the P0-P3 rubric, the output shape, the project's conventions — and `review submit` verifies positions, dedupes, applies the gate and reports, in text, markdown or JSON. Nothing in this path talks to OrcaRouter or needs a key; the thinking is the agent's. - `--pr <n>` reviews a pull request by number without checking it out - `--lang` follows the user's language through the plan, findings and report - file selection is Open Code Review's own rules, vendored as data under vendor/open-code-review (Apache-2.0) with a JS port of its matcher — no `ocr` binary to install - `.orcacode-review.json` holds a repo's block_on / language / exclude / rules; `review config` shows what applies and `review config init` writes it; the plan offers to create it once, after the first review - exit 0 for a review that ran; `--fail-on-block` for hooks that want a 1; 2 for an unusable result is never suppressed Skills are renamed to orca-review and orca-review-action; installing the new names retires our old ones beside them. The installer now asks how you will use it — local, Action, or both — and `--mode` answers that for scripts. rules/output-shape.md shows a wrong/right title pair, because a real PR got a sixteen-word imperative title with a semicolon. * docs: three demos — install, Action setup, local review — and a README that shows each Recorded with vhs against a throwaway repo (docs/tapes/README.md says how). Every prompt takes its recommended answer. The old combined demo is retired; the two features get their own GIF, with the 3x-speed mp4 linked beside each. * docs: README talks to the person, not the terminal Drop the '3x · mp4' captions under the GIFs, and the menus of npx subcommands and flags. The product is the two skills; you tell your agent what to review and it drives the CLI. The one command that stays is the install. Flags and the review contract are documented where a scripter looks, in --help and skills/orca-review/references/contract.md. * release: 2.1.0 package.json, plugin.json and marketplace.json move together, as the publish workflow insists. The tarball check now also names the local-review files — harness, selection, config, the vendored rules, both skills — so a files: regression in package.json fails the release instead of shipping a plan with no rubric.	2026-09-03 10:27:13 +08:00
Zhenghua Bao	5ecfbf339c	chore: current weights in the pasteable recipe (#50 ) Reviewer deepseek/deepseek-v4-flash -> deepseek/deepseek-v4-flash-0731, judge deepseek/deepseek-v4-pro -> z-ai/glm-5.3, matching the control plane's provisioning constant. Both verified present and in the `default` group on the live gateway's /api/pricing. The request named "0730"; 0731 is the dated weight. Also unpins two docs from a specific model that is no longer the one used: the judge-model note in action.yml and judge.mjs's usage line, which showed deepseek-v4-pro as its example and so read as a default. The usage line now shows the router alias, which is what the action actually passes. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-27 13:48:45 +08:00
Zhenghua Bao	7b24e7048c	fix: the shipped recipe kept a bare default, so a pasted copy lost the judge (#49 ) The judge rule went into the control plane's provisioning constant and its own pasteable copy, and not into this one — the file a BYO consumer actually pastes. So setup produced a correct recipe while the documented one sent the judge to `default:`, i.e. the reviewer's own model. A judge scoring work its own model produced agrees with it and still reports the pass as successful, so a workspace following the docs got a silently inert L2. Caught by a reader, which is the third time a recipe has drifted that way: the fact contract still said none\|cheap\|strong after one value was ever sent, the per-angle rules outlived the path that could reach them, and now this. The cause is structural and stays: the authority for what setup writes is a Go constant in another repository, and nothing here can see it. Cross-repo agreement is not assertable. So the new suite pins the properties that make a recipe self-consistent instead, which is what each of those three failures actually violated: - every rule names a model, and a default exists - every `when:` keys on a fact the action actually sends, so a rule cannot describe a policy that never applies - the Action's recipe carries a judge rule - that rule does not name the default's model - it carries no per-angle rule, which no Action call can reach - it names the router by the alias action.yml defaults to, so a rename cannot leave the paste target pointing at a name that no longer resolves Verified the way it will be relied on: removing the judge rule turns two of them red. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-27 10:08:40 +08:00
AnKaifeng	7f7e6a191a	docs: RELEASE.md described a withdrawal that did not happen (#48 ) The 1.x section was written between deciding to unpublish the line and finding out npm would not allow it, and it shipped in #44 claiming "1.0.2 through 1.5.0 were published on 25-26 Aug 2026 and unpublished inside npm's 72-hour window." They are all still on the registry. Anyone reading that section would conclude the packument had one version in it and go looking for a bug when it has ten. What actually happened: all nine are deprecated, so they stay installable for anyone pinned and warn on every fresh install, pointing at @latest. That is the outcome the section should have described in the first place — the reasoning it gave for preferring deprecation was already sitting in its own last paragraph. Also records that the unscoped `orcacode-review` deprecation is still undone, which the file has prescribed since the org move without saying it had never been run. It cannot be run from CI: NPM_TOKEN is scoped to this one package, which is exactly the property that makes a leak survivable, so the command needs the personal account that owns the name.	2026-08-26 22:09:46 +08:00