fix(noema): reject a completion the provider declares truncated - #1930
fix(noema): reject a completion the provider declares truncated#1930seonghobae wants to merge 3 commits into
Conversation
noema_review_gate.py never read finish_reason -- the field where an
OpenAI-compatible provider declares it stopped at the output-token budget
rather than because the model finished. Verified on main@f25063882:
`grep -c finish_reason` was 0.
That is only sometimes caught downstream. Driving main's own parser:
'{"decision":"approve",...,"findings":[{"severity":"high"'
-> NoemaModelOutputError (unbalanced, fails closed)
'{"decision":"approve","summary":"reviewed","findings":[]}'
-> parsed as a valid verdict (truncation lands on a closed object)
The second case is the dangerous one, and it is not a corner: findings is
emitted last, so the likeliest parseable truncation is an approval with an
empty or short findings list -- a review cut off mid-thought, accepted as
a genuine APPROVE on a required gate. The local repair is deliberately
lossless (trailing commas only) so it does not manufacture this; the
provider does, and the gate simply never checked where the provider says
so.
reject_truncated_completion() reads choices[0].finish_reason on the
decoded body and raises NoemaModelOutputError only for the unambiguous
"length". Missing, empty, or any other value passes untouched, so a
provider reporting a vocabulary this gate does not model cannot be failed
spuriously; malformed envelopes keep being classified by
extract_llm_message_content, which reports their real cause.
Deliberately narrow: no retry, no new exception type, no change to who
owns repair. The gateway keeps that, per the caller attempts=1 contract.
This is the portable part of #1606, which cannot merge as written because
it also adds caller-side retry against that contract.
Same failure family as #1921 -- a reviewer that could not see everything
returning APPROVE with nothing in the output saying so.
Tests assert the payload really would have parsed before the guard, that
every other finish_reason is allowed, that shape errors stay deferred to
the content parser, and that the guard runs ahead of extraction.
Full suite 2905 passed, 1 skipped, 21 subtests; noema_review_gate.py 100%
coverage over 864 statements / 388 branches; interrogate 100%.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
|
Warning Review limit reachedNext included review available in 12 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
CI status note — standing down on the two red checks on head
Not re-running it manually: a re-run dispatches again and enlarges the queue that is causing the failure ( Generated by Claude Code |
…letion Rebinds workflow_sha so the required review runs pick up #1939's round-robin catalog fill; the previous head's Strix and Noema runs failed on the single-upstream gateway stall that #1939 removes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
|
CI note and push: head Two new red checks on the previous head, both the single-upstream gateway stall class and neither in this diff: Local gate on Generated by Claude Code |
|
CI note on head Same class as #1938's failure 20 minutes earlier, read per host 1's source trace on that thread: the gateway did fail over; Corrected tally, since the one I posted on #1938 was wrong: of the post-#1939 Action: not re-running yet. #1938's attempt-2 re-run (queued 23:49Z, same pre-#1944 workflow) is the measurement of whether the rate has moved; at 1 in 4 a re-run here would spend another ~25 minutes of a runner slot in a 234-deep queue at the same odds. I will issue this PR's single re-run once that result is in, or fold it into the next base-merge push, which also binds the artifact-producing workflow. The two CodeQL compatibility failures remain the designed pending state noted earlier. Generated by Claude Code |
|
CI note on head
So at 23:48–00:12Z the free pool was rate-limited on every account at once — OpenRouter at preflight, the primary NVIDIA account at preflight, the sub account on request. #1939's interleave is working (4+4+4 selected); there was simply no free capacity to interleave. The same window produced #1938's and this PR's Action: no re-run now — with one ready route that 429s on contact, a re-run is 25 more minutes of slot at near-zero odds. This PR's single sanctioned re-run stays gated on #1938's queued attempt 2 (or the next base-merge push), same as the Noema check. The lever for the condition itself (a pool with ≤1 ready route that rate-limits on first contact should not cost 25 minutes) sits in the orchestrator's retry policy and the Strix replay policy, neither in this diff; I have handed the artifact to the observability lane. Generated by Claude Code |
|
CI note on head Nothing in this diff is involved, and no push or re-run from this side changes the outcome — a re-run re-dispatches into the same rejected gate, and the dispatch workflow re-runs this exact job by itself once a verdict is published. Standing down on this check until #1929's variable is reconciled; the Strix, Noema, and CodeQL notes above stand. Generated by Claude Code |
…letion Binds the sidecar pin bump to contextual-orchestrator@414f2297 (contextual-orchestrator#1081's retry-stacking fix, .github efb8926) and the #1943/#1944/#1945 workflows to this head's required runs; the previous head's Noema/Strix runs failed on the stalled-route loop that fix removes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
|
CI note and push: head Merges Local gate on Generated by Claude Code |
Lane: jan | session:session_01BV96rXhqoR3tYZ9AeAVur4
The hole
scripts/ci/noema_review_gate.pynever readsfinish_reason— the field where an OpenAI-compatible provider declares it stopped at the output-token budget rather than because the model finished. Onmain@f25063882,grep -c finish_reasonreturns 0.Truncation is only sometimes caught downstream. Driving
main's own parser directly:The second case is not a corner case.
findingsis emitted last, so the likeliest parseable truncation is an approval carrying an empty or short findings list — a review cut off mid-thought, arriving as a genuine-looking APPROVE on a required gate, with nothing anywhere saying it was incomplete.main's local repair is deliberately lossless (_strip_trailing_commas_outside_stringsonly), so it does not manufacture this. The provider does, and the gate simply never looked at the field where the provider says so.This is the same failure family as #1921: a reviewer that could not see everything returns APPROVE, silently. There the cause is allowed-locations starvation; here it is output-budget truncation.
The fix
reject_truncated_completion(raw)readschoices[0].finish_reasonon the decoded body and raisesNoemaModelOutputErrorfor the unambiguous"length". It runs incall_llmimmediately afterdecode_llm_response_body, ahead of content extraction.Deliberately narrow, in three ways:
"length"is rejected. Missing, empty,"stop","tool_calls","end_turn","content_filter"— all pass untouched. A provider reporting a vocabulary this gate does not model must not be failed spuriously, and this gate fronts every repository in the organization.extract_llm_message_content, which reports their actual cause with precise messages. Duplicating that here would attribute the wrong reason to a body this function cannot classify.caller attempts=1 (gateway owns repair/failover)contract already onmain.Relationship to #1606
This is the portable half of #1606. That PR cannot merge as written: alongside this check it adds caller-side retry (
is_retry,truncated_after_retry, a stale-head check before retry) built onextract_llm_completion, whichmainreplaced withextract_llm_message_contentwhen it moved repair ownership to the gateway. Resolving its conflict would mean silently reversing that architectural decision. Detail in #1606.So the defect is landed here in
main's own idiom and its own error type, and #1606's remaining parts — the bounded-field validators andNOEMA_MAX_*constants — stay available as their own change if wanted.Verification
pytest tests/test_noema_review_gate.py -k "truncated_completion or call_llm_consults"→ 15 passedcoverage run -m pytest tests -q→ 2905 passed, 1 skipped, 21 subtests passedcoverage report→scripts/ci/noema_review_gate.py100% (864 statements / 388 branches, 0 missing); TOTAL 100%interrogate→ PASSED (100.0%)git diff --checkcleanorigin/main@f25063882in a fresh worktreeThe first test asserts the truncated payload really would have parsed before the guard — otherwise the test would pass for the wrong reason and prove nothing about the hole being closed. A separate test pins that the guard runs ahead of content extraction, so a future reorder cannot silently move it after the parse.
Developer experience
A truncated review now fails with a message naming the cause (
finish_reason=length) instead of either succeeding wrongly or failing later with a misleading JSON-shape error. The message embeds no part of the untrusted response body, so it stays safe in the publicpull_request_targetjob log.User experience
An APPROVE from
noema-reviewcan no longer be produced by a completion the provider itself declared incomplete. This closes a path by which a required gate could pass a change nobody finished reviewing.🤖 Generated with Claude Code
https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
Generated by Claude Code