fix(noema): harden truncated structured verdict publication - #1606
fix(noema): harden truncated structured verdict publication#1606seonghobae wants to merge 49 commits into
Conversation
Preserve current Strix and SBOM control-plane changes while carrying only the verified Noema structured-completion production source, regression contract, and changelog forward. The completed one-shot workflow and repair driver remain absent.
|
Warning Review limit reachedNext included review available in 45 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 (1)
📝 WalkthroughWalkthroughNoema가 OpenAI 호환 완료 메타데이터를 보존하고, JSON verdict의 출력 범위를 검증하며, ChangesNoema 완료 복구
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The change hardens structured verdict handling but also includes a validation workflow that retains write-capable repository credentials during tests, creating a potential unintended-ref update risk; malformed error paths may also pass the new checks. Merge should wait for the credential handling issue to be fixed and the assertions tightened. Sequence Diagram(s)sequenceDiagram
participant ReviewGate as call_llm
participant Provider as LLM provider
participant Parser as extract_llm_completion
participant Validator as validate_verdict_output_bounds
ReviewGate->>Provider: JSON verdict request with 4096-token budget
Provider-->>ReviewGate: completion envelope
ReviewGate->>Parser: Parse content and bounded metadata
Parser-->>ReviewGate: LLMCompletion
ReviewGate->>Validator: Validate verdict fields and bounds
Validator-->>ReviewGate: Valid verdict or typed error
ReviewGate->>Provider: Compact repair retry after finish_reason=length
Provider-->>ReviewGate: Repaired completion
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 79.31% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 29 functions across 2 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches 💡 3📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
) QUEUE_SATURATION_CHICKEN_EGG: this exact one-line test repair matches protected production's three-argument GITHUB_ENV publication contract. The stale protected-main assertion is independently proven as the sole failure after 2,344 passing tests in #1606's exact-head Strix quality run. Current-head Devin/CodeRabbit statuses are success, there are zero review threads, and all required workflows are queued in a 894-run saturated Actions fleet. No substantive product, security, provenance, or review defect is bypassed.
Preserve the protected-main Strix installer-test repair while carrying the verified three-file Noema structured-verdict hardening forward. Temporary repair workflow/driver files remain absent; no force push or unrelated tree replacement.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🔍 Focused tests need installed tooling
Local execution lacks pytest, so only compilation and whitespace checks ran. Hosted results remain the available behavioral test evidence.
Was this helpful? React with 👍 or 👎 to provide feedback.
seonghobae
added
documentation
ChatGPT Codex Connector
| except RuntimeError as exc: | ||
| raise InvalidCompletionError(str(exc)) from exc |
There was a problem hiding this comment.
🔴 Oversized JSON integers crash reviews
When completion JSON exceeds Python's integer digit limit, json.loads raises ValueError, bypassing both retry wrappers. The required review exits with a traceback.
Prompt for agents
Handle ValueError raised by Python's JSON decoder for integer literals exceeding the configured digit limit. Both extract_llm_completion and extract_json_object can raise it, and call_llm currently catches only RuntimeError around those calls. Convert this parser failure into InvalidCompletionError so the first failure receives one repair attempt and a repeated failure produces the existing bounded invalid_json_after_retry diagnostic. Add envelope-level and verdict-content regression tests using an integer longer than sys.get_int_max_str_digits().
Was this helpful? React with 👍 or 👎 to provide feedback.
| if isinstance(exc, InvalidCompletionError): | ||
| raise RuntimeError( | ||
| f"Noema LLM response invalid_json_after_retry: {exc}" | ||
| ) from exc |
There was a problem hiding this comment.
🔍 Envelope failures get JSON label
Repeated unsupported finish reasons and malformed metadata receive invalid_json_after_retry. Operational diagnostics therefore misidentify valid JSON envelopes as malformed JSON.
Was this helpful? React with 👍 or 👎 to provide feedback.
| "Noema LLM response adversarial probe class_evidence must be an object" | ||
| ) | ||
| if len(class_evidence) > NOEMA_MAX_CLASS_EVIDENCE_FIELDS: | ||
| raise RuntimeError( | ||
| "Noema LLM response adversarial probe class_evidence " | ||
| f"exceeds {NOEMA_MAX_CLASS_EVIDENCE_FIELDS} fields" | ||
| ) | ||
| for value in class_evidence.values(): | ||
| _bounded_text( | ||
| value, | ||
| "adversarial_validation.probes.class_evidence", |
There was a problem hiding this comment.
📝 Info: Class evidence keys stay private
Unbounded class_evidence keys do not reach review Markdown or diagnostics. Cardinality and rendered values remain bounded, so publication size stays controlled.
Was this helpful? React with 👍 or 👎 to provide feedback.
Merge-conflict triage: genuine architectural divergence in
|
The defect is still live on
|
| this PR | main today |
|
|---|---|---|
| repair ownership | caller-side retry (is_retry, truncated_after_retry, stale-head check before retry) |
gateway-owned — caller attempts=1 (gateway owns repair/failover) |
| content extraction | extract_llm_completion → LLMCompletion carrying finish_reason |
extract_llm_message_content (content only) |
| error taxonomy | TruncatedCompletionError / InvalidCompletionError / InvalidVerdictError |
NoemaModelOutputError + active_phase tracking |
| observability | — | _extract_served_model, _extract_http_error_telemetry, _stable_failure_diagnostic |
Of the five conflicted hunks, three are benign — hunk 1 is two complementary constant sets (mine bound verdict fields, main's add MAX_ALLOWED_LOCATIONS_JSON_BYTES and MAX_HTTP_ERROR_BODY_BYTES; both are wanted), and hunks 2–3 are pure additions where main has nothing. Hunks 4 and 5 are the problem: they are the LLM call path itself, rewritten on both sides toward opposite designs. Resolving them means choosing between caller-side and gateway-owned repair — a decision, not a conflict resolution. Taking my side would silently reintroduce caller-side retry against main's explicit contract.
3. What I propose instead
Do not merge this PR as-is. The valuable part is one check that fits main's architecture without touching repair ownership at all:
- extend
main's content extraction to readchoices[0].finish_reasonalongside the message content; - raise
NoemaModelOutputErrorwhen it islength(and when it is anything outside{"", "stop"}); - no retry, no new exception hierarchy, no change to who owns repair — the gateway keeps that.
That is a small, additive change in main's own idiom and its own error type, and it closes case B above. This PR's bounded-field validators (validate_verdict_output_bounds and the NOEMA_MAX_* constants) are independently useful and could follow as their own change.
I have not written that patch, because #1921 proposes narrowing the declared verdict scope in the same file and the two interact — both are about a verdict that overstates what was reviewed. Sequencing them deliberately beats racing them into the same function.
Happy to implement the minimal finish_reason check as a fresh PR against current main on a word from whoever owns this gate. Not pushing anything to this branch meanwhile.
Generated by Claude Code
|
I am working through this repository's What the conflict looks like
Block 5 is the blocker. Why block 5 has no correct pickMain's side of block 5 is the only place that assigns Taking main's side compiles and goes green — and silently deletes this PR's feature: 180 lines of dead code with a fully green I then grafted the branch's checks into main's structure ( Those contracts require the retry-and-repair loop: fail once, resend with a repair prompt, and only classify as What I suggestThe four tractable blocks above are stated precisely enough to apply directly; block 5 needs the retry loop rebuilt against main's current Nothing was pushed to this branch — it is exactly as you left it ( 🤖 Generated with Claude Code |
|
Scheduled review-feedback autofix for this PR head.
|
1 participant
Current-main Ready successor to closed #1602. The branch was non-destructively reconciled onto protected
main@5d1b9b2109991689d02301fb3577a4d79dbe386f; the effective delta is exactlyCHANGELOG.md,scripts/ci/noema_review_gate.py, andtests/test_noema_truncated_completion_contract.py. Temporary one-shot workflow/driver files were removed after verification.This preserves #1602's bounded structured-completion recovery and also closes the exact current review findings: comment verdicts can no longer smuggle non-string rendered evidence into GitHub Markdown, every rendered reviewed/probe/finding field is typed and bounded, line values must be positive integers, decoded-but-schema-invalid verdicts receive a typed
InvalidVerdictErrorrepair retry and stableinvalid_verdict_after_retryterminal diagnostic, and completion-envelope decoding remains fail closed.TDD evidence from hosted one-shot run
33510393580: the three new regressions were required to fail before production repair; the repair then produced9 passedin the focused structured-completion contract and2327 passed, 1 skipped, 21 subtests passedin the full repository suite, followed by compileall andgit diff --check. The run itself concluded success. No reviewer identity, provider route, branch protection, security threshold, merge authority, or direct-provider fallback changed.Fresh exact-head review/check evidence on this successor is authoritative; predecessor PR reviews/checks do not transfer. If the unchanged current head remains mechanically mergeable with no substantive finding and required workflows alone remain queued behind the saturated central Actions fleet, apply the documented
QUEUE_SATURATION_CHICKEN_EGGboundary only after re-fetching live state.Summary by CodeRabbit