fix(noema): restore unbounded model runtime contract - #1891
Merged
Conversation
|
Warning Review limit reachedNext included review available in 14 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 (4)
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 |
seonghobae
marked this pull request as ready for review
September 5, 2026 04:49
Contributor
Author
|
이 PR은 현재 목표와 병합된 #1890을 되돌립니다. 중앙 run 33922765894가 무제한 |
Contributor
Author
|
objective 원문을 재검증한 결과 제가 #1890의 900초 강제를 정당화한 판단이 잘못됐습니다. 원문 일반지침 8.5–8.6은 모델 timeout 기본 null 및 OpenCode·Strix·Noema 모델당 2시간 이상 허용을 명시합니다. 이 PR을 복구하고 최신 main 위에서 #1890의 잘못된 Noema 제한을 제거하는 exact-head 수리로 재검증하겠습니다. |
This was referenced Sep 5, 2026
seonghobae
added a commit
that referenced
this pull request
Sep 5, 2026
…pline (#1909) Two rules from mistakes this session actually made and corrected, per the per-session lane split agreed with the other concurrent sessions (peer 3 took verification discipline in #1907; peer 2 has gate/merge mechanics; host 1 has close-time diff comparison and noema concurrency; host 2 has CI failure diagnosis). - Narrowing a PR does not carry its delta. #1871 was closed in favor of #1877 plus #1879; both successors were green, but neither carried the coverage/docstring delta, leaving main's required 100% gate broken until #1883 recovered it. "Each piece works" and "the pieces together cover the original's scope" are different questions. - Compare content, not ancestry. main mixes squash and merge commits (last 200: 153 single-parent, 47 two-parent, counted directly), so `git merge-base --is-ancestor` gives false negatives for squashed deltas and false positives for reverted merge-commit deltas. - Never endorse a timeout or retry constant on a model-invocation path without reading docs/product-goal-directive.md section 8, which accepts more than two hours per model and states speed is not a core consideration. #1889/#1890/#1892 each capped a model step at 900s on real multi-hour-hang evidence and were all reverted (#1891, #1895). Every PR number, the section-8 quotes, the parent-count distribution, and the 100% gate values were verified against the repository directly. An earlier draft of the timeout bullet cited a section number that does not exist and attributed a sentence to that file which appears only in #1891's PR body; both were caught by grepping rather than trusting the summary that introduced them, and that failure is recorded in the text. Full suite: 2883 passed, 1 skipped. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
seonghobae
added a commit
that referenced
this pull request
Sep 5, 2026
…-20260824) 13 conflict blocks across four files. Both sides hardened the same function independently - this branch at the credential egress boundary, main at output validation and gateway telemetry (#1477) - so the resolution is a union, not a choice, except where the two guards overlap. THE OVERLAP, resolved in the branch's favour with evidence: main's reject_private_llm_url has a fail-OPEN path - 'except socket.gaierror: return' - and does not require HTTPS for non-loopback targets, does not reject userinfo, and denylists special addresses instead of requiring globally routable unicast. The branch's validate_endpoint is stricter on every one of those axes and fails closed on DNS failure, which is this PR's stated subject. So call_llm now calls validate_endpoint only; reject_private_llm_url stays defined because two tests exercise it directly and still pass. That makes main's four call_llm-level 'URL cannot target internal IP addresses' assertions wrong and the branch's replacements right - which is exactly what the branch's side of those four conflict blocks already said. PORTED onto main's structure: - opener gains ProxyHandler({}) + PinnedHTTPHandler/PinnedHTTPSHandler in front of main's NoRedirectHandler - response.read(MAX_LLM_RESPONSE_BYTES + 1) with the over-limit check - post-request DNS identity re-check before decoding - constants and the pinned-connection classes merged alongside main's JSON bound helpers (disjoint symbol sets, so a plain union) NOT PORTED, on policy: The branch also passes timeout=120 to opener.open and its oracle asserts 'timeout == 120'. CLAUDE.md states model-path timeouts are policy-fixed, and #1889/#1890/#1892 each added such a cap and were all reverted (#1891, #1895); main's call_llm docstring says it 'carries no fixed model wall-clock deadline'. The assertion was translated to 'timeout is None' with that citation in a comment. The byte cap is the bound that test really needs. TEST FIXTURES updated because the behaviour changed, not to make red go green: - the branch's oracle called the pre-#1477 five-argument call_llm (11 sites) and did not stub validate_substantive_verdict, which main added; both translated, the latter the same way main's own call_llm tests do it - fail-closed DNS means an unresolvable *.example.test aborts a test before its own assertion, so tests/test_noema_review_gate.py and tests/test_noema_repair_attempt_telemetry.py get an autouse fixture that resolves non-literal hosts and leaves literal addresses to the real resolver, so the internal-address tests keep their meaning - the byte-capped read passes an argument, so nine response doubles became read(self, _limit=None) Evidence: - uvx ruff check --select F821 scripts/ci tests: All checks passed - full suite, branch head 3feb583 (unmerged): 1931 passed, 0 failed - full suite, this merge: 2938 passed, 0 failed - the branch's own oracle tests/test_noema_endpoint_boundary.py: 33 passed (10 before the signature translation) - main's noema tests: 121 passed; telemetry contracts: 23 passed - negative controls: removing the post-request DNS re-check fails test_public_endpoint_requires_https_and_stable_global_dns; removing the byte cap fails test_response_body_is_bounded_before_json_decoding - both by name - coverage: TOTAL 100%; interrogate: PASSED (minimum 100.0%) GAP FOUND, left for the author: removing PinnedHTTPHandler/PinnedHTTPSHandler from the opener changes no test result (33 still pass). The pinned classes are covered only by direct unit tests; nothing asserts they are installed in call_llm's opener, so a future edit could unwire DNS pinning silently. This is pre-existing in the PR, not introduced here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Protected
main@bc59c07c448dcfa1b8fbc64b601550d232697f24merged #1890 with a 15-minutetimeout-minutesonPrepare Noema model verdictand a 30-minute job timeout. That change terminates synchronous model work solely because wall-clock time elapsed. It conflicts with the current CWL LLM contract: production model timeout defaults to null, long-running Noema/OpenCode/Strix work must be allowed, and user cancellation, provider termination, and administrator/runtime policy termination must remain distinguishable instead of being collapsed into an elapsed-time cutoff.The operational symptom behind #1890 is real: a stalled/provider-unfinished model call can retain a shared hosted runner. The fixed inference deadline is not the causal repair for that capacity problem. Runner occupancy should be addressed at the admission/continuation architecture or by an explicit provider/gateway terminal signal without converting elapsed inference time into model failure.
Minimal causal repair
Exact head
9dc149e7f093c6069dc6e85ae55186e4af033923restores the four pre-#1890 owner blobs and nothing else:.github/workflows/noema-review.yml: removes the 15-minute model-step and 30-minute enclosing-job wall-clock caps and restores the explicit no-fixed-inference-timeout rationale;tests/test_noema_orchestrator_workflow_contract.py: restores the fail-closed regression requiring no job-level timeout on the synchronous model body and the documented long-running allowance;docs/product-goal-directive.md: restores the long-running OpenCode/Strix/Noema policy instead of the 900-second policy rewrite;docs/doctoring/autofix-and-noema-review-model-job-timeout-removal.md: removes the supersession note introduced by fix(noema): bound model runtime to 900 seconds #1890.bc59c07... → 9dc149e...is one fast-forward commit and exactly four files; there is no provider/model override, paid fallback, token change, gate weakening, force-push, or unrelated workflow delta.Follow-up architecture boundary
Do not solve shared-runner retention by reintroducing an elapsed-time-only inference deadline. A successor may move long model work behind an event-driven continuation/reconciliation boundary or consume an explicit contextual-orchestrator/provider terminal state, provided exact-head authority is preserved and user-cancel/provider-end/admin-timeout remain distinct outcomes.
Verification / merge gate
The PR is now Ready only to obtain exact-head review/CI evidence; Ready is not a merge-readiness claim. The first Draft-created Security/SAST/CodeQL/Python Security runs were cancelled when the lifecycle changed, and fresh
ready_for_reviewruns have materialized on the unchanged9dc149e...head. Those fresh runs are queued; Agent Review Runtime Quality CI also remains queued. Pre-#1890 evidence and cancelled predecessor runs are lineage only. Merge normally only after terminal required workflows, valid independent review, zero unresolved valid findings, and protected-branch eligibility. No self-approval, administrator bypass, destructive rebase, synthetic status, or blind rerun.