fix(noema): bound model runtime to 900 seconds - #1890
Merged
Conversation
|
Warning Review limit reachedNext included review available in 39 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
enabled auto-merge (squash)
September 5, 2026 04:42
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 was referenced Sep 6, 2026
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.
Summary
Runtime evidence
33922765894remained inPrepare Noema model verdictfor more than 30 minutes on exact PR ⚡ Bolt: JSON 디코딩 성능 최적화 #1872 head evidencetimeout-minutes, allowing a stalled call to retain a shared runnerVerification
uv run pytest -q tests/test_noema_orchestrator_workflow_contract.py tests/test_noema_reviewer_token_lifetime.py(14 passed)actionlint .github/workflows/noema-review.ymlgit diff --check