mirror of
https://github.com/Continuum-AI-Corp/Orca-Code-Review.git
synced 2026-09-27 09:29:03 +00:00
ff658d1e92* 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>
212 lines
8.7 KiB
212 lines
8.7 KiB