fix(noema): raise call_llm HTTP timeout from 120s to org policy - #1509
fix(noema): raise call_llm HTTP timeout from 120s to org policy#1509seonghobae wants to merge 5 commits into
Conversation
noema-review's required check failed with an identical TimeoutError at call_llm (scripts/ci/noema_review_gate.py:656) across 4-5 check runs on 3+ PRs (contextual-orchestrator#965, #958 twice, #960) inside ~2 hours. The 120-second literal was three orders of magnitude short of this org's own recorded policy (docs/product-goal-directive.md: "central OpenCode, Strix, and Noema may take over two hours per model, and the org accepts this"). The sidecar's own preflight smoke test already went through this exact bug class once for its tiny "reply OK" probe (docs/adr/0005-sidecar-preflight-token-budget.md, 30s->120s), but that reasoning was never extended to call_llm's much larger real review request (up to MAX_DIFF_CHARS + MAX_REVIEW_CONTEXT_CHARS of prompt), which inherited the same 120s literal by copy, not by sizing decision. noema-review.yml's job has no timeout-minutes (GitHub's 360-minute default applies), so there was no outer-bound reason to keep the inner timeout short. Replace the hardcoded 120 with LLM_REQUEST_TIMEOUT_SECONDS = 3600, reusing this org's own existing precedent for one model-call attempt (OPENCODE_RUN_TIMEOUT_SECONDS's default in run_opencode_review_model_pool.sh) rather than inventing a new number. call_llm recurses at most once (one repair retry), so one review's worst case is 3600*2 = 7200s (2 hours), matching the org's stated per-model policy and OpenCode's own OPENCODE_LARGE_CHANGE_TOTAL_BUDGET_SECONDS=7200 precedent for the same reason -- well within the job's 360-minute default ceiling. Update the two tests that pinned the old literal and add a dedicated regression test pinning the constant and the two-attempt worst-case arithmetic against the policy, plus a dated gap-baseline entry recording the investigation and evidence trail. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
|
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: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughNoema 리뷰 게이트가 LLM 리뷰와 판정 제출을 분리합니다. 리뷰 결과를 상태 파일에 저장합니다. 제출 전에 GitHub App 또는 OIDC 자격 증명을 새로 발급합니다. 제출자는 리뷰 생성자와 일치해야 합니다. ChangesNoema 리뷰 흐름
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The PR allows slower model reviews and uses a split review-submission flow, but it can submit results for an outdated commit, exceed its intended request budget, or expose OIDC credentials through an insufficiently restricted exchange endpoint. These risks can cause stale reviews, failed or overlong checks, or credential exposure, so the PR is not merge-ready until they are fixed or explicitly accepted. Sequence Diagram(s)sequenceDiagram
participant Workflow as noema-review.yml
participant ReviewGate as noema_review_gate.py
participant State as review-state.json
participant Credentials as GitHub App/OIDC
participant GitHub as GitHub API
Workflow->>ReviewGate: --phase review
ReviewGate->>GitHub: LLM 리뷰 및 PR 정보 조회
ReviewGate->>State: 리뷰 결과 저장
Workflow->>Credentials: 제출용 자격 증명 재발급
Workflow->>ReviewGate: --phase submit
ReviewGate->>State: 리뷰 결과 로드
ReviewGate->>GitHub: current_actor() 신원 검증
ReviewGate->>GitHub: 판정 제출
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 5 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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.
…on token Devin Review found two real bugs in the prior LLM_REQUEST_TIMEOUT_SECONDS=3600 fix on this same PR: 1. The org's "over two hours per model call" policy applies per attempt, not split across the at-most-one repair retry. The repair retry only fires on a content-validation failure, never because the HTTP call ran long, and it resends the same full diff/context as the original attempt (verified by reading call_llm's repair-retry prompt construction), so it deserves the same budget, not half of it. Raised LLM_REQUEST_TIMEOUT_SECONDS to 7200 (the full policy bound) and added LLM_REQUEST_TOTAL_BUDGET_SECONDS=14400 to document the two-attempt worst case explicitly. Added an explicit timeout-minutes: 300 to noema-review.yml's job (previously relying on GitHub Actions' implicit 360-minute default), sized above the recomputed ~252-minute worst case while staying under the 360-minute hosted-runner hard ceiling. 2. noema-review.yml minted its GitHub App/OIDC submission credential once at job start and reused it, unchanged, through call_llm and the final submit_review POST -- a credential with an ~hour lifetime reused after a call that can now legitimately run up to four hours. Split noema_review_gate.py's inspect_and_review into run_review_phase (through call_llm, persists JSON state) and submit_pending_verdict (submits under the current credential, re-verifying the reviewer identity against a fresh credential rather than trusting the identity recorded when the verdict was computed). noema-review.yml now mints a fresh token between the review and submit steps, gated on whether a verdict was actually produced. Added six new tests for the phase-split and identity re-verification, plus evidence assertions for the repair-retry sizing decision. Updated two workflow-contract tests for the renamed step. 2134 tests pass (2133 + 1 pre-existing skip); 100% coverage and 100% docstring coverage on scripts/ci/. Documented in docs/product-technical-gap-baseline.md (2026-08-31 entry). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…mit; bound call_llm's total read time CodeRabbit reviewed this PR and found three real issues, all confirmed against the current code before fixing: 1. Neither OIDC app-token exchange step in noema-review.yml validated that TOKEN_EXCHANGE_URL (a repo variable) actually used https:// before POSTing the freshly-minted OIDC identity token to it. A misconfigured http:// value would have sent that token in cleartext. Added a `case ... https://*` guard using the step's existing fail_unavailable() helper, before either curl request, in both the initial and the post-call_llm submission exchange steps. 2. submit_pending_verdict re-verified reviewer identity against the fresh submission credential (the prior fix on this PR) but never re-fetched the PR, so it always submitted against state["pr"]'s now-possibly-stale headRefOid. A commit landing during call_llm's now-multi-hour window would attach the review to an outdated commit_id, undermining this org's exact-head evidence model. submit_pending_verdict now calls fetch_pr again immediately before submit_review and aborts (bounded RuntimeError) on a head mismatch. 3. call_llm's `opener.open(..., timeout=LLM_REQUEST_TIMEOUT_SECONDS)` only bounds the connection phase and each individual socket read, not the total time response.read() can spend looping over many such reads to reach EOF (confirmed empirically with a real trickling local HTTP server). A slow or pathological server could keep one attempt's read phase alive indefinitely, blowing past LLM_REQUEST_TOTAL_BUDGET_SECONDS even with a "healthy" connection. call_llm now threads a shared time.monotonic() deadline across the original attempt and its repair retry; the response body is read through a new _read_response_body_within_deadline helper that arms a watchdog timer to force-close the socket's read side once the deadline passes, converting a still-blocked or interrupted read into the same bounded, fail-closed RuntimeError this file already uses elsewhere. Regression coverage: a real local HTTP server trickling chunks under a tiny monkeypatched budget (proving the deadline fires mid-read, not just once per attempt), four focused unit tests on the new read helper's edge branches, one on call_llm's own pre-attempt budget check, and one proving submit_review is never called when the head changed between phases. 2141 tests pass (2140 plus one pre-existing skip); 100% coverage and 100% docstring coverage on scripts/ci/. noema-review.yml still parses under PyYAML with its original 12 steps and timeout-minutes: 300; all `run: |` blocks pass bash -n. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
|
Fresh unchanged downstream acceptance canary from
This is not an Orgmetra source/test failure and there is no correct consumer-side shim: widening/removing the trusted reviewer transport bound belongs here in the central owner. Post-integration acceptance: rerun this same unchanged consumer head against the protected central source. The review must no longer terminate at the 120-second inner timeout; it must either complete and publish a valid exact-head Reviews API verdict, or fail closed for a different, explicitly typed substantive cause. A status-only success, predecessor verdict, or timeout hidden behind a green wrapper is non-passing.
|
Devin Review (#1509) found that call_llm's shared LLM_REQUEST_TOTAL_BUDGET_SECONDS deadline was being passed directly to _read_response_body_within_deadline as the read watchdog for BOTH the original attempt and the repair retry, instead of each attempt getting its own fresh attempt_start_time + LLM_REQUEST_TIMEOUT_SECONDS bound. A slow-but-healthy original attempt could therefore run for up to the full 4h shared budget before being cut off, starving a subsequent repair retry of its fair 2h share under the org's per-model-call policy. call_llm now captures attempt_start_time fresh on every call (original and repair-retry recursion alike) and derives effective_deadline = min(attempt_start_time + LLM_REQUEST_TIMEOUT_SECONDS, deadline), using it for both the connection-level attempt_timeout and the response-read watchdog. Neither attempt can now individually exceed its two-hour allowance, while the pair still respects the four-hour outer backstop threaded through the deadline parameter. Adds a regression test mirroring the existing trickling-response test but with the two constants swapped (tiny per-attempt bound, generous total budget), proving the original attempt is cut off at its own bound rather than riding out the shared budget. Leaves the informational write_review_state atomicity note from the same review round untouched per this PR's scope discipline. Validation: coverage run -m pytest tests (2141 passed, 1 skipped) + coverage report --show-missing (100% on scripts/ci/) + interrogate (100% docstring coverage). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…s can't bypass the deadline Devin Review found that call_llm's per-attempt deadline enforcement only protected response.read() -- opener.open() itself (connect, TLS handshake, request transmission, and status-line/header receipt) had no watchdog, so a provider trickling response HEADER bytes slowly could keep it blocked well past effective_deadline (#1509). Extracts the existing body-read watchdog's arm/expire logic into a shared _arm_deadline_watchdog helper, then reuses it one phase earlier: a new _deadline_guarded_connection builds an HTTPConnection/HTTPSConnection subclass that arms the same watchdog on its own socket the instant connect() returns, swapped in via two thin handler classes (_DeadlineHTTPHandler/_DeadlineHTTPSHandler) so build_opener's usual HTTPHandler/HTTPSHandler defaults are replaced rather than duplicated. _open_response_within_deadline wraps opener.open() itself, mirroring the body-read watchdog's three outcomes for the header phase. Assessed and documented the residual gap this does not close: the TLS handshake performed inside connect() for https:// targets, which neither watchdog can reach because ssl's SSLContext.wrap_socket() detaches the pre-wrap socket's file descriptor before the handshake I/O actually runs. Adds a real local-http.server byte-at-a-time header-trickle regression test mirroring the existing body-trickle test, plus 8 isolated unit tests for the new helpers following the existing fake-double pattern. 2150 tests pass (up from 2141); 100% coverage and 100% docstring coverage on scripts/ci/. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
|
Standing down on further timeout/deadline hardening in this PR after this round (commit This PR has now gone through several rounds addressing real, distinct gaps in The latest fix's own investigation found one more residual gap, documented in code ( I'm treating this as the stopping point rather than dispatching another round, for two reasons:
Recommend a separate follow-up if the org wants defense-in-depth against a compromised/adversarial TLS endpoint specifically; not blocking this PR. Generated by Claude Code |
|
Flagging cross-PR overlap so we don't duplicate more work on this: #1415, #1511, and this PR are all independently fixing the same #1415 is currently the most mature of the three (many additional, unrelated fixes already reviewed across dozens of Devin/CodeRabbit rounds: batched route preflight, family/account cap fixes, startup watchdog timing) and just landed its own fix for this exact bug class: Your Before that port, though: CodeRabbit's pre-merge check on this PR flags real, unresolved risk here — submitting results for an outdated commit, exceeding the intended request budget, and a possibly-insufficiently-restricted OIDC exchange endpoint. Those need to be resolved (here or in whatever absorbs this work) before the credential-refresh mechanism is safe to carry over — I'm not treating this PR as ready to merge as-is. Given the overlap, I'd like to close #1511 as superseded by whichever of #1415/this PR ends up serving as the timeout fix (its Generated by Claude Code |
|
Consumer canary evidence from BandScope confirms this exact causal boundary on the current central protected workflow, without a BandScope-local source failure.
Exact job evidence: credential mint, trusted-source materialization, repository visibility, sidecar health/provider-route preflight, and gateway I triggered one bounded exact-job rerun on unchanged |
|
Closing as superseded by #1507. Both PRs fix the same root cause ( This PR's Generated by Claude Code |
Pull request was closed
2 participants
Summary
noema-review's required check failed with an identicalTimeoutErroratcall_llm(scripts/ci/noema_review_gate.py:656) across at least 4-5 separate check runs on 3+ PRs (ContextualWisdomLab/contextual-orchestrator#965,#958twice,#960) inside roughly two hours. Investigated whether this was transient infra flakiness or a genuine policy/timeout mismatch.Confirmed: this is a real policy/timeout mismatch bug, not infra flakiness.
call_llm's HTTP request timeout was a hardcoded literaltimeout=120(seconds) — three orders of magnitude short of this org's own recorded policy (docs/product-goal-directive.mdline 65): "중앙 OpenCode, Strix, Noema는 모델당 두 시간 이상 걸릴 수 있음을 수용한다" — central OpenCode, Strix, and Noema may legitimately take over two hours per model, and the org explicitly accepts this.call_llm's real review-completion request — carrying up toMAX_DIFF_CHARS(60000) +MAX_REVIEW_CONTEXT_CHARS(24000) chars of prompt and requesting a structured multi-part JSON verdict — not the sidecar's own tiny "reply with just 'OK'" preflight smoke test. For the job to even reachcall_llm, the sidecar's own/healthzwait and virtual-pool smoke request must already have succeeded, so aTimeoutErrorhere means the gateway was already proven reachable; the much larger real request is what ran past the bound.docs/adr/0005-sidecar-preflight-token-budget.mdalready reasoned through this exact bug class once, for the sidecar's own smoke-test call (raised 30s → 120s after live reproduction, citing this same policy) — but that reasoning was never extended tocall_llm's much larger real review request, which apparently inherited the same120literal by copy rather than by its own sizing decision.noema-review.yml's job carries notimeout-minutesat all (confirmed viagit log -p— never set), so the effective outer bound is GitHub Actions' 360-minute default. There was no outer-bound reason to keep the inner timeout short.Fix
Replaced the hardcoded
120with a named module-level constant,LLM_REQUEST_TIMEOUT_SECONDS = 3600, reusing this org's own already-codified precedent for one model-call attempt rather than inventing a new number:OPENCODE_RUN_TIMEOUT_SECONDS's default of3600inscripts/ci/run_opencode_review_model_pool.sh(OpenCode's own per-model-attempt timeout, under the same policy sentence that explicitly names OpenCode too).call_llmmay recurse exactly once (one repair attempt whenvalidate_substantive_verdictrejects the first verdict), so one review's worst case is two attempts at this bound:3600 × 2 = 7200s(2 hours) — matching the org's stated per-model policy exactly, and matching OpenCode's own analogousOPENCODE_LARGE_CHANGE_TOTAL_BUDGET_SECONDSdefault of7200for the same reason. This worst case still leaves generous headroom under the job's 360-minute default ceiling, so no change tonoema-review.yml'stimeout-minuteswas needed or made.Developer experience
120literal (test_noema_review_gate.py::test_call_llm_repairs_one_rejected_changed_line_verdict,test_repository_branch_coverage_review_schedulers.py::test_noema_public_dns_result_reaches_valid_model_response) to assert againstnoema.LLM_REQUEST_TIMEOUT_SECONDS.test_noema_review_gate.py::test_llm_request_timeout_matches_org_two_hour_per_model_policy) pinning both the constant's value and the two-attempt worst-case arithmetic against the org's stated policy.docs/product-technical-gap-baseline.mdentry (2026-08-31) recording the investigation, evidence, and fix.coverage run -m pytest tests -q && coverage report --show-missing: 2127 passed, 1 skipped, 21 subtests passed; 100% coverage onscripts/ci/.interrogate: 100% docstring coverage.User experience
No user-facing behavior change for a healthy review — this only widens how long a legitimately slow model completion is allowed to take before
noema-reviewgives up and fails the check, consistent with the org's own stated "accuracy over speed" policy for this exact reviewer.Test plan
coverage run -m pytest tests -q && coverage report --show-missing— 2127 passed, 1 skipped, 100% coverage onscripts/ci/interrogate— 100% docstring coverage🤖 Generated with Claude Code
https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
Generated by Claude Code
Summary by CodeRabbit
개선 사항
문서