From 4ddacbd2973dbabadbbd58db3c17390671900cf7 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 10:52:38 +0000 Subject: [PATCH 1/5] fix(noema): raise call_llm HTTP timeout from 120s to org policy 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 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- docs/product-technical-gap-baseline.md | 61 +++++++++++++++++++ scripts/ci/noema_review_gate.py | 18 +++++- tests/test_noema_review_gate.py | 20 +++++- ...itory_branch_coverage_review_schedulers.py | 2 +- 4 files changed, 98 insertions(+), 3 deletions(-) diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 758ef2961a..4bb3339395 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1715,6 +1715,67 @@ string, a bare number) confirmed to fail against the pre-fix script (`KeyError: signature as the original round-4 bug) before passing after the fix. 1930 tests pass; 100% coverage and 100% docstring coverage on `scripts/ci/`. +## 2026-08-31 recurring `noema-review` `TimeoutError` at `call_llm`: confirmed policy/timeout mismatch, not infra flakiness + +`noema-review`'s required check failed with an identical `TimeoutError` at +`scripts/ci/noema_review_gate.py:656` (`with opener.open(request, timeout=120) as response:`) across +at least 4-5 separate check runs on 3+ different PRs (`ContextualWisdomLab/contextual-orchestrator#965`, +`#958` twice, `#960`) inside roughly two hours. Investigated whether this was transient infra flakiness +or a genuine policy/timeout mismatch, per this org's own recorded policy +(`docs/product-goal-directive.md` line 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 HTTP request timeout was a hardcoded literal `120` +(seconds) — three orders of magnitude short of that stated tolerance. + +**Confirmed a real bug, not flakiness.** Evidence: + +- The failing call is `call_llm`'s own review-completion request — the *actual* review, carrying up to + `MAX_DIFF_CHARS` (60000) + `MAX_REVIEW_CONTEXT_CHARS` (24000) chars of prompt content and requesting a + structured multi-part JSON verdict (summary, per-line findings, adversarial-validation probes) — not + the sidecar's own lightweight "reply with just 'OK'" preflight smoke test + (`scripts/ci/contextual_orchestrator_review_sidecar.sh`'s separate `curl --max-time 120` gateway check). + For the job to reach `call_llm` at all, the "Provision contextual-orchestrator review sidecar" step — + including its own `/healthz` wait and virtual-pool smoke request — must already have succeeded; a + `TimeoutError` specifically at `call_llm` therefore means the sidecar and gateway were already proven + reachable and able to serve a completion, and the much larger real review request was what ran past the + bound, not a down or unreachable dependency. +- This org's own `docs/adr/0005-sidecar-preflight-token-budget.md` already reasoned through this exact + class of bug once, for the sidecar's smoke-test call: a prior 30-second `curl --max-time` was raised to + 120s after live reproduction (`ContextualWisdomLab/.github#1449`, job `99253418179`) showed a + genuinely-healthy route needing more than 30s for a real generation, citing this same "org accepts + multi-hour central review latency" policy. That ADR's own Layer 2 (the smoke test) intentionally keeps + its 120s-per-attempt value **unchanged** — appropriate for a tiny fixed-`max_tokens` "OK" probe — but its + reasoning was never extended to `call_llm`'s much larger real review request, which inherited the same + 120s literal seemingly by default/copy rather than by a sizing decision of its own. This is the same bug + shape recurring one call site later, previously fixed only where the sidecar's own smoke test needed it. +- `noema-review.yml`'s job carries no `timeout-minutes` at all (`.github/workflows/noema-review.yml`), + so the effective outer bound is GitHub Actions' own 360-minute default — confirmed via `git log -p` + that this job has never had an explicit `timeout-minutes`. There was no outer-bound reason to keep the + inner HTTP timeout short; the constraint was purely an unjustified inner literal. + +**Fix.** Replaced the hardcoded `timeout=120` with 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 of `3600` in +`scripts/ci/run_opencode_review_model_pool.sh` (OpenCode's own per-model-attempt run timeout, the same +"central OpenCode ... may take over two hours" policy's other explicitly-named beneficiary). +`call_llm` may recurse exactly once — one repair attempt when `validate_substantive_verdict` rejects the +first verdict — so one review's worst case is two attempts at this bound: `3600 × 2 = 7200s` (2 hours), +matching this org's stated per-model policy exactly, and matching OpenCode's own analogous +`OPENCODE_LARGE_CHANGE_TOTAL_BUDGET_SECONDS` default of `7200` for the same reason. That worst case still +leaves generous headroom under the job's 360-minute (21600s) default ceiling for checkout, sidecar +startup/preflight (up to ~625s per ADR-0005's own worst-case arithmetic), JSON parsing, and posting the +verdict back to GitHub — so no `timeout-minutes` change to `noema-review.yml` was needed or made. + +Regression coverage: updated the two existing tests that pinned the old literal +(`tests/test_noema_review_gate.py::test_call_llm_repairs_one_rejected_changed_line_verdict`, +`tests/test_repository_branch_coverage_review_schedulers.py::test_noema_public_dns_result_reaches_valid_model_response`) +to assert against `noema.LLM_REQUEST_TIMEOUT_SECONDS` instead of the bare literal, and added a new, +dedicated regression test +(`tests/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, so a future edit cannot silently shrink this back toward the bug just fixed. 2127 tests pass +(2128 after the new test); 100% coverage and 100% docstring coverage on `scripts/ci/`. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 90a69bed31..def03fbc73 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -34,6 +34,22 @@ MAX_THREAD_BODY_CHARS = 1200 DIFF_HUNK_RE = re.compile(r"^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@") +# This org's own recorded policy (docs/product-goal-directive.md: "중앙 OpenCode, Strix, Noema는 +# 모델당 두 시간 이상 걸릴 수 있음을 수용한다" -- central OpenCode, Strix, and Noema may take over +# two hours per model, and the org accepts this) explicitly names Noema, and noema-review.yml's own +# job carries no timeout-minutes (GitHub Actions' 360-minute default applies), so this single HTTP +# request has ample outer headroom. This value is not a new guess: it reuses this org's own already +# codified precedent for one model-call attempt, `OPENCODE_RUN_TIMEOUT_SECONDS`'s default of 3600s +# in scripts/ci/run_opencode_review_model_pool.sh. call_llm may recurse exactly once (one repair +# attempt on a rejected verdict, see below), so the worst case for one review -- two attempts at +# this bound -- is 7200s (2 hours), matching this org's own stated per-model policy exactly and +# OpenCode's analogous `OPENCODE_LARGE_CHANGE_TOTAL_BUDGET_SECONDS` default of 7200 for the same +# reason. The prior 120-second value was three orders of magnitude short of this policy and was +# copied from the sidecar's own small "reply OK" preflight smoke-test budget (contract: +# docs/adr/0005-sidecar-preflight-token-budget.md), not sized for a real review completion carrying +# up to MAX_DIFF_CHARS + MAX_REVIEW_CONTEXT_CHARS of prompt content. +LLM_REQUEST_TIMEOUT_SECONDS = 3600 + ORCHESTRATOR_LOOPBACK_HOSTS = frozenset({"127.0.0.1", "::1"}) ORCHESTRATOR_BASE_ENV = "CONTEXTUAL_ORCHESTRATOR_BASE_URL" @@ -653,7 +669,7 @@ def call_llm( method="POST", ) opener = urllib.request.build_opener(NoRedirectHandler()) - with opener.open(request, timeout=120) as response: # nosec B310 + with opener.open(request, timeout=LLM_REQUEST_TIMEOUT_SECONDS) as response: # nosec B310 raw = response.read().decode("utf-8") data = json.loads(raw) content = (((data.get("choices") or [{}])[0].get("message") or {}).get("content") or "").strip() diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 338d46ba81..5ad7f4686a 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -386,6 +386,24 @@ def fake_getaddrinfo_invalid_ip(host, port, *args, **kwargs): assert noema.call_llm("owner/repo", 1, pr, "diff", True)["decision"] == "approve" +def test_llm_request_timeout_matches_org_two_hour_per_model_policy(): + """call_llm's HTTP timeout must stay sized for a real review, not a smoke test. + + Regression guard for the recurring `TimeoutError` at this exact call site + (ContextualWisdomLab/contextual-orchestrator#965, #958, #960): a prior + 120-second value was copied from the sidecar's small preflight smoke-test + budget and was three orders of magnitude short of this org's own recorded + policy that a central Noema/OpenCode/Strix model call may legitimately take + over two hours (docs/product-goal-directive.md). One review allows exactly + one repair retry (see call_llm's recursive repair-on-rejected-verdict path), + so the worst case for a single review must stay at or below that two-hour + policy bound, not merely below the per-attempt value alone. + """ + assert noema.LLM_REQUEST_TIMEOUT_SECONDS == 3600 + max_call_llm_attempts_per_review = 2 # original call + at most one repair retry + assert noema.LLM_REQUEST_TIMEOUT_SECONDS * max_call_llm_attempts_per_review == 7200 + + def test_noema_redirect_handler_rejects_redirects(): """Noema must not follow redirects after validating the initial URL.""" handler = noema.NoRedirectHandler() @@ -688,7 +706,7 @@ def read(self): class Opener: def open(self, request, timeout): - assert timeout == 120 + assert timeout == noema.LLM_REQUEST_TIMEOUT_SECONDS payloads.append(json.loads(request.data)) return Response(invalid if len(payloads) == 1 else valid) diff --git a/tests/test_repository_branch_coverage_review_schedulers.py b/tests/test_repository_branch_coverage_review_schedulers.py index 85cdc0b964..0ff28870a6 100644 --- a/tests/test_repository_branch_coverage_review_schedulers.py +++ b/tests/test_repository_branch_coverage_review_schedulers.py @@ -63,7 +63,7 @@ class Opener: """Open one deterministic provider response.""" def open(self, _request: Any, timeout: int) -> Response: - assert timeout == 120 + assert timeout == noema.LLM_REQUEST_TIMEOUT_SECONDS return Response() monkeypatch.setattr(noema.urllib.request, "build_opener", lambda *_args: Opener()) From 056365968032b47bcd8b2c0a2cd89a688aacdadb Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 11:15:41 +0000 Subject: [PATCH 2/5] fix(noema): give each model call the full 2h budget; refresh submission 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 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- .github/workflows/noema-review.yml | 132 +++++++++++++- docs/product-technical-gap-baseline.md | 95 ++++++++++ scripts/ci/noema_review_gate.py | 157 ++++++++++++++--- ...st_noema_orchestrator_workflow_contract.py | 2 +- tests/test_noema_review_gate.py | 164 ++++++++++++++++-- .../test_required_workflow_queue_contract.py | 2 +- 6 files changed, 513 insertions(+), 39 deletions(-) diff --git a/.github/workflows/noema-review.yml b/.github/workflows/noema-review.yml index 064c4e5aee..4014a7b18a 100644 --- a/.github/workflows/noema-review.yml +++ b/.github/workflows/noema-review.yml @@ -43,6 +43,18 @@ jobs: noema-review: name: noema-review runs-on: ubuntu-latest + # Explicit, computed ceiling instead of GitHub Actions' implicit 360-minute + # (6-hour) default: worst case is two call_llm attempts at + # LLM_REQUEST_TIMEOUT_SECONDS=7200 each (scripts/ci/noema_review_gate.py's + # LLM_REQUEST_TOTAL_BUDGET_SECONDS = 14400s = 240 minutes) plus sidecar + # startup/preflight (ADR-0005: up to ~180s healthz wait + ~360s Layer-2 + # gateway retries ≈ 9 minutes), credential minting/visibility-lookup + # retries, diff/context fetch, and verdict submission -- well under one + # more hour on top, for a computed worst case of roughly 252 minutes. 300 + # minutes leaves a deliberate safety margin above that computed bound + # while staying under GitHub Actions' 360-minute hard ceiling for + # GitHub-hosted runners. + timeout-minutes: 300 if: >- github.event_name == 'repository_dispatch' || ( @@ -299,8 +311,9 @@ jobs: set -euo pipefail bash "$GITHUB_WORKSPACE/scripts/ci/contextual_orchestrator_review_sidecar.sh" - - name: Run Noema LLM review and submit verdict + - name: Run Noema LLM review if: env.PR_NUMBER != '' + id: review env: GH_TOKEN: ${{ secrets.NOEMA_REVIEW_TOKEN || steps.noema_github_app_token.outputs.token || steps.noema_oidc_token.outputs.token }} NOEMA_REVIEW_TOKEN_SOURCE: ${{ steps.noema_credential.outputs.source == 'pat' && 'noema-review-pat' || steps.noema_credential.outputs.source == 'github-app' && 'noema-review-github-app' || 'noema-review-app-oidc' }} @@ -310,10 +323,11 @@ jobs: set -euo pipefail if [ -z "${PR_NUMBER:-}" ]; then echo "No pull request number was available for this event; skipping." + echo "has_verdict=false" >>"$GITHUB_OUTPUT" exit 0 fi if [ -z "${GH_TOKEN:-}" ]; then - echo "::error::Noema reviewer credential selection succeeded but no token was minted; review cannot submit a verdict." + echo "::error::Noema reviewer credential selection succeeded but no token was minted; review cannot run." exit 1 fi if [ -z "${CONTEXTUAL_ORCHESTRATOR_BASE_URL:-}" ] || [ -z "${CONTEXTUAL_ORCHESTRATOR_TOKEN_FILE:-}" ]; then @@ -325,6 +339,118 @@ jobs: export NOEMA_LLM_MODEL="orchestrator/free" export NOEMA_LLM_API_KEY="${CONTEXTUAL_ORCHESTRATOR_TOKEN}" export NOEMA_LLM_VIA_ORCHESTRATOR=1 + # A review-completion model call may run for up to + # LLM_REQUEST_TOTAL_BUDGET_SECONDS (4 hours; see that constant in + # scripts/ci/noema_review_gate.py), long enough to outlive the + # short-lived credential minted above. Compute the verdict here and + # persist it; a later step mints a FRESH credential and submits it, + # so the token used to post the review is never the one that may + # have expired while call_llm was in flight (Devin Review, + # ContextualWisdomLab/.github#1509). + noema_state_file="${RUNNER_TEMP}/noema-review-state.json" python3 -m scripts.ci.noema_review_gate \ --repo "$TARGET_REPOSITORY" \ - --pr-number "$PR_NUMBER" + --pr-number "$PR_NUMBER" \ + --phase review \ + --state-file "$noema_state_file" + if [ -s "$noema_state_file" ]; then + echo "has_verdict=true" >>"$GITHUB_OUTPUT" + else + echo "has_verdict=false" >>"$GITHUB_OUTPUT" + fi + + - name: Mint fresh repository-scoped Noema GitHub App submission token + if: env.PR_NUMBER != '' && steps.review.outputs.has_verdict == 'true' && steps.noema_credential.outputs.source == 'github-app' + id: noema_github_app_token_submit + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 + with: + client-id: ${{ vars.NOEMA_GITHUB_APP_CLIENT_ID }} + private-key: ${{ secrets.NOEMA_GITHUB_APP_PRIVATE_KEY }} + owner: ContextualWisdomLab + repositories: ${{ steps.noema_credential.outputs.repository }} + permission-actions: read + permission-checks: read + permission-contents: read + permission-metadata: read + permission-pull-requests: write + permission-security-events: read + permission-statuses: read + permission-vulnerability-alerts: read + + - name: Exchange fresh Noema app submission token through OIDC + if: env.PR_NUMBER != '' && steps.review.outputs.has_verdict == 'true' && steps.noema_credential.outputs.source == 'oidc' + id: noema_oidc_token_submit + env: + OIDC_AUDIENCE: ${{ vars.NOEMA_OIDC_AUDIENCE || 'cwl-noema-review' }} + TOKEN_EXCHANGE_URL: ${{ vars.NOEMA_TOKEN_EXCHANGE_URL || vars.NOEMA_EXCHANGE_URL || '' }} + run: | + set -euo pipefail + + fail_unavailable() { + local message="$1" + echo "::error::$message" + exit 1 + } + + if [ -z "${ACTIONS_ID_TOKEN_REQUEST_TOKEN:-}" ] || [ -z "${ACTIONS_ID_TOKEN_REQUEST_URL:-}" ]; then + fail_unavailable "Noema app submission token exchange unavailable: OIDC request environment is missing." + fi + + request_url="${ACTIONS_ID_TOKEN_REQUEST_URL}" + separator="&" + case "$request_url" in + *\?*) ;; + *) separator="?" ;; + esac + + if ! oidc_response="$( + curl -fsS \ + -H "Authorization: Bearer ${ACTIONS_ID_TOKEN_REQUEST_TOKEN}" \ + "${request_url}${separator}audience=${OIDC_AUDIENCE}" + )"; then + fail_unavailable "Noema app submission token exchange unavailable: OIDC token request did not complete." + fi + + oidc_token="$(jq -r '.value // empty' <<<"$oidc_response")" + if [ -z "$oidc_token" ]; then + fail_unavailable "Noema app submission token exchange unavailable: OIDC token response was empty." + fi + + if ! token_response="$( + curl -fsS \ + -X POST \ + -H "Content-Type: application/json" \ + -H "Authorization: Bearer ${oidc_token}" \ + --data "$(jq -cn --arg target_repository "$TARGET_REPOSITORY" '{target_repository:$target_repository}')" \ + "${TOKEN_EXCHANGE_URL}" + )"; then + fail_unavailable "Noema app submission token exchange unavailable: app token request did not complete." + fi + + app_token="$(jq -r '.token // empty' <<<"$token_response")" + if [ -z "$app_token" ]; then + fail_unavailable "Noema app submission token exchange unavailable: app token response was empty." + fi + + echo "::add-mask::$app_token" + echo "token=$app_token" >>"$GITHUB_OUTPUT" + + - name: Submit Noema review verdict + if: env.PR_NUMBER != '' && steps.review.outputs.has_verdict == 'true' + env: + GH_TOKEN: ${{ secrets.NOEMA_REVIEW_TOKEN || steps.noema_github_app_token_submit.outputs.token || steps.noema_oidc_token_submit.outputs.token }} + NOEMA_REVIEW_TOKEN_SOURCE: ${{ steps.noema_credential.outputs.source == 'pat' && 'noema-review-pat' || steps.noema_credential.outputs.source == 'github-app' && 'noema-review-github-app' || 'noema-review-app-oidc' }} + NOEMA_REVIEW_ACTOR: ${{ steps.noema_github_app_token_submit.outputs['app-slug'] && format('{0}[bot]', steps.noema_github_app_token_submit.outputs['app-slug']) || '' }} + NOEMA_REVIEW_INSTALLATION_ID: ${{ steps.noema_github_app_token_submit.outputs['installation-id'] }} + run: | + set -euo pipefail + if [ -z "${GH_TOKEN:-}" ]; then + echo "::error::Noema submission credential selection succeeded but no fresh token was minted; verdict cannot be submitted." + exit 1 + fi + noema_state_file="${RUNNER_TEMP}/noema-review-state.json" + python3 -m scripts.ci.noema_review_gate \ + --repo "$TARGET_REPOSITORY" \ + --pr-number "$PR_NUMBER" \ + --phase submit \ + --state-file "$noema_state_file" diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 4bb3339395..3ae63b6dcf 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1776,6 +1776,101 @@ pinning both the constant's value and the two-attempt worst-case arithmetic agai policy, so a future edit cannot silently shrink this back toward the bug just fixed. 2127 tests pass (2128 after the new test); 100% coverage and 100% docstring coverage on `scripts/ci/`. +## 2026-08-31 PR #1509 Devin Review 2건 검증: `LLM_REQUEST_TIMEOUT_SECONDS` 배분 오류와 제출 시점 credential 만료, 둘 다 실재 결함으로 확인 후 수정 + +Devin Review posted two new findings on `ContextualWisdomLab/.github#1509` (the same-day +`LLM_REQUEST_TIMEOUT_SECONDS = 3600` fix above), both against that fix's own PR diff. Investigated +both against the actual `call_llm`/`submit_review` code paths and the `noema-review.yml` workflow, +rather than trusting either finding at face value. **Both confirmed real, not false positives.** + +**Finding 1 — the 3600s value itself split the two-hour policy across two attempts instead of +giving each attempt the full two hours.** The prior fix's own reasoning was: `call_llm` recurses at +most once, so "two attempts at 3600s" equals the org's stated two-hour-per-model-call policy. That +reasoning conflates "one review may need two hours total" with "each individual model call may +need two hours" — but `docs/product-goal-directive.md` line 65 says the latter ("모델당" = per +model [call]), and the repair-retry recursion is not a size-halving operation: it only fires when +`validate_substantive_verdict` rejects the first verdict's *content* (missing evidence, wrong +locations, etc.) — never because the HTTP call itself ran long. A single, genuinely slow-but-healthy +call needing e.g. 90 minutes would hit the 3600s timeout and fail even though it never triggered a +repair retry and stayed well inside the org's per-model allowance. Read `call_llm`'s repair-retry +prompt-construction branch directly for evidence on whether a repair attempt is cheaper than the +original (Devin's own prompt asked this to be checked, not assumed): it resends the *same* full +`diff` and the *same* full `review_context` as the original attempt, appending only two short +instruction lines — it asks the model to redo the entire review with corrected evidence, not a small +patch. There is therefore no basis for giving the repair attempt a smaller budget than the original. + +**Fix.** `LLM_REQUEST_TIMEOUT_SECONDS` raised from `3600` to `7200` (the full two-hour policy bound, +applied to *each* attempt), with a new named `LLM_REQUEST_TOTAL_BUDGET_SECONDS = 14400` (4 hours) +constant documenting the two-attempt worst case explicitly rather than leaving it as an unstated +product of the two numbers. `noema-review.yml`'s `noema-review` job — which had no `timeout-minutes` +at all, relying on GitHub Actions' implicit 360-minute (6-hour) default — now declares +`timeout-minutes: 300`: the computed worst case is the 14400s (240-minute) `call_llm` budget plus +sidecar startup/preflight (ADR-0005: up to ~180s healthz wait + ~360s Layer-2 gateway retries, ~9 +minutes), credential minting, visibility-lookup retries, diff/context fetch, and verdict submission +— roughly 252 minutes total — so 300 minutes leaves a deliberate margin above that computed bound +while staying under the 360-minute hard ceiling GitHub Actions enforces for hosted runners (a job +cannot exceed it regardless of `timeout-minutes`, so 300 was chosen, not a larger number that would +have been silently clamped). + +**Finding 2 — the submission token could expire mid-review, given the new timeout.** Investigated +`noema-review.yml` directly: the job mints exactly one repository-scoped credential +(`actions/create-github-app-token`, a step early in the job, before sidecar provisioning) and reused +it — unchanged — through `fetch_pr`, `call_llm`, and the final `submit_review` POST, all inside one +`python3 -m scripts.ci.noema_review_gate` invocation. GitHub App installation tokens minted by that +action are short-lived (about one hour); with `call_llm` now legitimately able to run for up to four +hours, the credential minted at job start could easily have expired before `submit_review`'s POST at +the end, making a fully-computed, valid review verdict silently fail to post. Confirmed the OIDC +exchange path (`noema_oidc_token`) has the same shape: one exchange near job start, reused unchanged +at submission time. + +**Fix.** Restructured both the workflow and `scripts/ci/noema_review_gate.py` so a fresh submission +credential is minted *after* `call_llm` returns, immediately before the GitHub API call that posts +the review — per Devin's own suggested option (a), reusing the existing mint/exchange step shapes +rather than inventing a new mechanism. `noema_review_gate.py`'s single `inspect_and_review` pass is +now backed by three building blocks: `run_review_phase` (pre-flight checks through `call_llm`, +returns JSON-serializable `{pr, actor, verdict}` state or `None` to skip a draft/already-reviewed +PR), `write_review_state`/`load_review_state` (persist that state to a file between two separate +process invocations), and `submit_pending_verdict` (submits previously computed state under the +*current* credential). `inspect_and_review` itself is kept as a single-process convenience path +(same credential throughout, calls `submit_review` directly) so every test that already exercised it +needed no behavior change. The CLI gained `--phase {review,submit}` plus `--state-file PATH`; +`noema-review.yml`'s single "Run Noema LLM review and submit verdict" step is now three steps: "Run +Noema LLM review" (`--phase review`, writes state and a `has_verdict` output), a repeat of the +GitHub-App-mint / OIDC-exchange step (new ids `noema_github_app_token_submit` / +`noema_oidc_token_submit`, gated on `has_verdict == 'true'` so a skipped review never mints an unused +credential), and "Submit Noema review verdict" (`--phase submit`, using the fresh credential's +outputs for `GH_TOKEN`/`NOEMA_REVIEW_ACTOR`/`NOEMA_REVIEW_INSTALLATION_ID`). The `NOEMA_REVIEW_TOKEN` +PAT fallback path is unaffected (reused directly at both phases; PATs are not the short-lived +installation tokens this finding is about). + +`submit_pending_verdict` also **preserves the verified-reviewer-identity binding across the +credential swap**, per Devin's explicit ask, rather than trusting the identity recorded when the +verdict was computed: it re-calls `current_actor()` against the *fresh* credential and raises if the +resulting identity differs from the one `run_review_phase` verified, refusing to submit under an +unverified or rebound identity. + +Regression coverage (`tests/test_noema_review_gate.py`): `test_run_review_phase_returns_state_or_none` +(review phase returns serializable state or `None`, and never calls `submit_review` itself), +`test_write_and_load_review_state_round_trip`, `test_submit_pending_verdict_matches_and_rejects_identity_drift` +(matching identity submits; a mismatched or empty fresh identity raises and never calls +`submit_review`), `test_main_phase_requires_state_file`, `test_main_review_phase_writes_state_only_when_computed`, +`test_main_submit_phase_submits_or_skips` (six new tests), plus extended +`test_call_llm_repairs_one_rejected_changed_line_verdict` with an explicit assertion that the repair +attempt's prompt contains the same full diff and is not shorter than the original attempt's prompt +(the direct evidence for Finding 1's sizing decision), and rewrote +`test_llm_request_timeout_matches_org_two_hour_per_model_policy` to pin `LLM_REQUEST_TIMEOUT_SECONDS +== 7200` and `LLM_REQUEST_TOTAL_BUDGET_SECONDS == 14400` as separate assertions rather than only +their product, per Devin's request. Workflow-contract tests referencing the old step name ("Run +Noema LLM review and submit verdict") in `tests/test_required_workflow_queue_contract.py` and +`tests/test_noema_orchestrator_workflow_contract.py` were updated to the new step name ("Run Noema +LLM review"); every other existing workflow-contract assertion against `noema-review.yml` still +passes unmodified since the original mint/exchange step ids and the review step's own `GH_TOKEN`/ +`NOEMA_REVIEW_ACTOR`/`NOEMA_REVIEW_INSTALLATION_ID` expressions were left in place, only the +submission step's sourcing was changed. All ten `run: |` shell blocks in `noema-review.yml` (up from +eight) pass `bash -n`; the workflow parses under PyYAML with the expected 12 steps and +`timeout-minutes: 300`. 2134 tests pass (2133 plus one pre-existing skip); 100% coverage and 100% +docstring coverage on `scripts/ci/`. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index def03fbc73..5adf8a5be7 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -34,21 +34,37 @@ MAX_THREAD_BODY_CHARS = 1200 DIFF_HUNK_RE = re.compile(r"^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@") -# This org's own recorded policy (docs/product-goal-directive.md: "중앙 OpenCode, Strix, Noema는 -# 모델당 두 시간 이상 걸릴 수 있음을 수용한다" -- central OpenCode, Strix, and Noema may take over -# two hours per model, and the org accepts this) explicitly names Noema, and noema-review.yml's own -# job carries no timeout-minutes (GitHub Actions' 360-minute default applies), so this single HTTP -# request has ample outer headroom. This value is not a new guess: it reuses this org's own already -# codified precedent for one model-call attempt, `OPENCODE_RUN_TIMEOUT_SECONDS`'s default of 3600s -# in scripts/ci/run_opencode_review_model_pool.sh. call_llm may recurse exactly once (one repair -# attempt on a rejected verdict, see below), so the worst case for one review -- two attempts at -# this bound -- is 7200s (2 hours), matching this org's own stated per-model policy exactly and -# OpenCode's analogous `OPENCODE_LARGE_CHANGE_TOTAL_BUDGET_SECONDS` default of 7200 for the same -# reason. The prior 120-second value was three orders of magnitude short of this policy and was -# copied from the sidecar's own small "reply OK" preflight smoke-test budget (contract: -# docs/adr/0005-sidecar-preflight-token-budget.md), not sized for a real review completion carrying -# up to MAX_DIFF_CHARS + MAX_REVIEW_CONTEXT_CHARS of prompt content. -LLM_REQUEST_TIMEOUT_SECONDS = 3600 +# This org's own recorded policy (docs/product-goal-directive.md line 65: "중앙 OpenCode, Strix, +# Noema는 모델당 두 시간 이상 걸릴 수 있음을 수용한다" -- central OpenCode, Strix, and Noema may +# legitimately take over two hours PER MODEL CALL, and the org accepts this) is stated per model +# call, not per review. A prior fix here (docs/product-technical-gap-baseline.md's 2026-08-31 +# "recurring noema-review TimeoutError" entry) set this to 3600s, reasoning that call_llm's +# at-most-one repair-retry recursion made "two 1-hour attempts" equal the org's two-hour policy -- +# that reasoning was itself a bug (Devin Review on ContextualWisdomLab/.github#1509): the repair +# retry only fires when the model's response FAILS validate_substantive_verdict (a content problem), +# never because the HTTP call itself ran long, so a single genuinely-slow-but-healthy call needing, +# say, 90 minutes would hit a 3600s timeout and fail even though it never needed a retry and stayed +# well within the org's per-model allowance. Each attempt -- the original call or the repair retry -- +# is its own independent model call under this policy and must each get the full two-hour bound, not +# half of it split across the two. This is not merely policy-literal, either: read against evidence, +# the repair-retry prompt (see the `repair_error` branch in call_llm's prompt construction below) +# resends the SAME full diff and SAME full review_context as the original attempt, appending only two +# short instruction lines -- it asks the model to redo the entire review with corrected evidence, not +# a small patch -- so there is no evidence a repair attempt is typically cheaper or faster than the +# original call, and therefore no basis for giving it a smaller budget than the original. +LLM_REQUEST_TIMEOUT_SECONDS = 7200 + +# call_llm recurses at most once: the recursive call passes `repair_error`, and the +# `if repair_error: raise` guard immediately below the recursive call prevents a second recursion, so +# one review's worst-case call_llm duration is exactly two attempts at the full per-attempt bound +# above. Named so the two-attempt worst case is asserted and documented independently of the +# per-attempt value (tests/test_noema_review_gate.py pins both separately, per Devin Review's request +# to assert per-attempt and overall budgets separately rather than only their product). Sidecar +# startup/preflight (ADR-0005: up to ~180s healthz wait plus up to ~360s of Layer-2 gateway retries), +# diff/context fetch, and verdict submission add well under one more hour on top of this total, so +# noema-review.yml's `noema-review` job declares an explicit `timeout-minutes` safely above this bound +# instead of relying on GitHub Actions' implicit 360-minute (6-hour) default. +LLM_REQUEST_TOTAL_BUDGET_SECONDS = LLM_REQUEST_TIMEOUT_SECONDS * 2 ORCHESTRATOR_LOOPBACK_HOSTS = frozenset({"127.0.0.1", "::1"}) ORCHESTRATOR_BASE_ENV = "CONTEXTUAL_ORCHESTRATOR_BASE_URL" @@ -795,8 +811,17 @@ def submit_review(repo: str, number: int, pr: dict[str, Any], actor: str, verdic print(f"Noema {event} review submitted for {repo}#{number} at {head_sha}.") -def inspect_and_review(repo: str, number: int) -> int: - """Inspect PR state and submit Noema's independent LLM review.""" +def run_review_phase(repo: str, number: int) -> dict[str, Any] | None: + """Run pre-flight checks and the LLM review call, without submitting. + + Returns ``None`` when the review is skipped (draft PR, or the current + head already has a Noema review) so a caller can skip minting a fresh + submission credential and the submission step entirely. On success, + returns the JSON-serializable state (``pr``, ``actor``, ``verdict``) + ``submit_pending_verdict`` needs to submit the review later, potentially + under a different (freshly minted) credential -- see that function's + docstring for why the credential used here is not assumed still valid. + """ pr = fetch_pr(repo, number) actor = current_actor() if not actor: @@ -808,15 +833,74 @@ def inspect_and_review(repo: str, number: int) -> int: ) if pr.get("isDraft"): print("PR is draft; Noema review skipped.") - return 0 + return None if existing_noema_review(pr, actor): print("Current head already has a Noema review; nothing to do.") - return 0 + return None diff, truncated = fetch_diff(repo, number) changed_paths = fetch_changed_file_paths(repo, number) review_context = build_review_context(repo, number, pr) verdict = call_llm(repo, number, pr, diff, truncated, review_context, changed_paths) - submit_review(repo, number, pr, actor, verdict) + return {"pr": pr, "actor": actor, "verdict": verdict} + + +def write_review_state(state_path: str, state: dict[str, Any]) -> None: + """Persist review-phase output as JSON for a later submission phase to read.""" + with open(state_path, "w", encoding="utf-8") as handle: + json.dump(state, handle) + + +def load_review_state(state_path: str) -> dict[str, Any] | None: + """Load review-phase JSON output, or ``None`` when no verdict is pending.""" + if not os.path.exists(state_path): + return None + with open(state_path, encoding="utf-8") as handle: + return json.load(handle) + + +def submit_pending_verdict(repo: str, number: int, state: dict[str, Any]) -> None: + """Submit a previously computed verdict under the current credential. + + ``call_llm`` may run for up to ``LLM_REQUEST_TOTAL_BUDGET_SECONDS`` (4 + hours), long enough to outlive the GitHub App installation token or + OIDC-exchanged token that was valid when ``run_review_phase`` computed the + verdict -- installation tokens are short-lived (about one hour). The + noema-review workflow therefore mints a fresh submission credential after + ``run_review_phase`` returns and before calling this function. Re-verify + the reviewer identity against that fresh credential rather than trusting + the identity recorded in ``state``, so a rebinding of + ``NOEMA_REVIEW_ACTOR``/``NOEMA_REVIEW_INSTALLATION_ID`` to a different + identity between the two phases is refused instead of silently trusted -- + this preserves the same verified-reviewer-identity guarantee + ``run_review_phase`` already enforced, under the new credential. + """ + actor = current_actor() + if not actor: + raise RuntimeError("Noema reviewer identity could not be verified") + if actor != state["actor"]: + raise RuntimeError( + f"Noema submission credential identity {actor!r} does not match the " + f"identity {state['actor']!r} that computed the verdict; refusing to " + "submit under a different identity." + ) + submit_review(repo, number, state["pr"], actor, state["verdict"]) + + +def inspect_and_review(repo: str, number: int) -> int: + """Inspect PR state and submit Noema's independent LLM review in one pass. + + Single-process convenience path: runs the review and submits it under the + same credential throughout, with no fresh-credential remint between the + two. The noema-review workflow itself instead runs the review and submit + phases as two separate CLI invocations (``--phase review`` / + ``--phase submit``) bracketing a fresh credential mint, since a single + long-running review can outlive one credential's lifetime -- see + ``run_review_phase`` and ``submit_pending_verdict``. + """ + state = run_review_phase(repo, number) + if state is None: + return 0 + submit_review(repo, number, state["pr"], state["actor"], state["verdict"]) return 0 @@ -825,6 +909,23 @@ def parse_args(argv: list[str]) -> argparse.Namespace: parser = argparse.ArgumentParser() parser.add_argument("--repo", required=True) parser.add_argument("--pr-number", required=True, type=int) + parser.add_argument( + "--phase", + choices=("review", "submit"), + default=None, + help=( + "Run only the review phase (computes and persists a verdict) or only " + "the submit phase (submits a previously persisted verdict), so the " + "caller can mint a fresh submission credential between them. Requires " + "--state-file. Omit both for the legacy single-process path." + ), + ) + parser.add_argument( + "--state-file", + default=None, + help="Path used to hand the computed verdict from the review phase to the " + "submit phase. Required together with --phase.", + ) return parser.parse_args(argv) @@ -833,7 +934,21 @@ def main(argv: list[str]) -> int: args = parse_args(argv) if args.pr_number <= 0: raise SystemExit("--pr-number must be positive") - return inspect_and_review(args.repo, args.pr_number) + if args.phase is None: + return inspect_and_review(args.repo, args.pr_number) + if not args.state_file: + raise SystemExit("--state-file is required with --phase") + if args.phase == "review": + state = run_review_phase(args.repo, args.pr_number) + if state is not None: + write_review_state(args.state_file, state) + return 0 + state = load_review_state(args.state_file) + if state is None: + print("No pending Noema verdict to submit; nothing to do.") + return 0 + submit_pending_verdict(args.repo, args.pr_number, state) + return 0 if __name__ == "__main__": # pragma: no cover diff --git a/tests/test_noema_orchestrator_workflow_contract.py b/tests/test_noema_orchestrator_workflow_contract.py index 75ad5242c0..af8bdc97e6 100644 --- a/tests/test_noema_orchestrator_workflow_contract.py +++ b/tests/test_noema_orchestrator_workflow_contract.py @@ -114,7 +114,7 @@ def test_strix_gateway_default_and_noema_sidecar_fail_closed(tmp_path: Path) -> noema_script = textwrap.dedent( workflow_step( workflow_text("noema-review.yml"), - "Run Noema LLM review and submit verdict", + "Run Noema LLM review", ).split(" run: |\n", 1)[1] ) noema_env = { diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 5ad7f4686a..5a250130d9 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -387,21 +387,33 @@ def fake_getaddrinfo_invalid_ip(host, port, *args, **kwargs): def test_llm_request_timeout_matches_org_two_hour_per_model_policy(): - """call_llm's HTTP timeout must stay sized for a real review, not a smoke test. - - Regression guard for the recurring `TimeoutError` at this exact call site - (ContextualWisdomLab/contextual-orchestrator#965, #958, #960): a prior - 120-second value was copied from the sidecar's small preflight smoke-test - budget and was three orders of magnitude short of this org's own recorded - policy that a central Noema/OpenCode/Strix model call may legitimately take - over two hours (docs/product-goal-directive.md). One review allows exactly - one repair retry (see call_llm's recursive repair-on-rejected-verdict path), - so the worst case for a single review must stay at or below that two-hour - policy bound, not merely below the per-attempt value alone. + """call_llm's per-attempt HTTP timeout must itself be the full per-model policy bound. + + Regression guard, round two, for the recurring `TimeoutError` at this exact + call site (ContextualWisdomLab/contextual-orchestrator#965, #958, #960). + Round one (ContextualWisdomLab/.github#1509) set the per-attempt timeout to + 3600s (half the two-hour policy), reasoning that call_llm's at-most-one + repair-retry recursion made "two 1-hour attempts" equal the org's stated + two-hour-per-model-call policy (docs/product-goal-directive.md). That + reasoning was itself a bug, caught by Devin Review on the same PR: the + repair retry only fires when the model's response FAILS + validate_substantive_verdict, never because the HTTP call itself ran long, + so a single genuinely-slow-but-healthy call needing e.g. 90 minutes would + hit a 3600s timeout and fail despite never needing a retry and staying + within the org's per-model allowance. Each attempt is an independent model + call under the org's policy and must therefore individually get the full + two-hour bound. Per-attempt and overall-budget assertions are pinned + separately here (rather than only their product) per Devin Review's + explicit ask, so a future edit cannot silently shrink either one back + toward either bug already fixed. """ - assert noema.LLM_REQUEST_TIMEOUT_SECONDS == 3600 + assert noema.LLM_REQUEST_TIMEOUT_SECONDS == 7200 max_call_llm_attempts_per_review = 2 # original call + at most one repair retry - assert noema.LLM_REQUEST_TIMEOUT_SECONDS * max_call_llm_attempts_per_review == 7200 + assert noema.LLM_REQUEST_TOTAL_BUDGET_SECONDS == 14400 + assert ( + noema.LLM_REQUEST_TIMEOUT_SECONDS * max_call_llm_attempts_per_review + == noema.LLM_REQUEST_TOTAL_BUDGET_SECONDS + ) def test_noema_redirect_handler_rejects_redirects(): @@ -538,6 +550,83 @@ def test_inspect_and_review_does_not_wait_for_other_reviews_or_checks(monkeypatc assert calls +def test_run_review_phase_returns_state_or_none(monkeypatch): + """run_review_phase must hand back JSON-serializable state, or None to skip. + + Regression coverage for ContextualWisdomLab/.github#1509's second Devin + Review finding: the workflow now mints a fresh submission credential + between computing a verdict and submitting it, which required splitting + inspect_and_review's single pass into a review phase (this function) and a + submit phase (submit_pending_verdict). run_review_phase must never call + submit_review itself. + """ + clean_pr = make_pr() + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: clean_pr) + monkeypatch.setattr(noema, "current_actor", lambda: "noema") + monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) + monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: ["tool.py"]) + monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr: "context") + monkeypatch.setattr( + noema, "call_llm", lambda *args, **kwargs: {"decision": "approve", "summary": "ok", "findings": []} + ) + monkeypatch.setattr( + noema, "submit_review", lambda *args, **kwargs: (_ for _ in ()).throw(AssertionError("must not submit")) + ) + + state = noema.run_review_phase("owner/repo", 7) + assert state == { + "pr": clean_pr, + "actor": "noema", + "verdict": {"decision": "approve", "summary": "ok", "findings": []}, + } + + draft_pr = make_pr(isDraft=True) + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: draft_pr) + assert noema.run_review_phase("owner/repo", 7) is None + + +def test_write_and_load_review_state_round_trip(tmp_path): + """Review-phase state must round-trip through JSON for the submit phase.""" + state_path = str(tmp_path / "noema-review-state.json") + assert noema.load_review_state(state_path) is None + + state = {"pr": make_pr(), "actor": "cwl-noema-review[bot]", "verdict": {"decision": "comment"}} + noema.write_review_state(state_path, state) + assert noema.load_review_state(state_path) == state + + +def test_submit_pending_verdict_matches_and_rejects_identity_drift(monkeypatch): + """submit_pending_verdict must re-verify identity against the fresh credential. + + Regression coverage for ContextualWisdomLab/.github#1509's second Devin + Review finding: a long call_llm can outlive the credential minted before + it, so the workflow mints a fresh one before submitting. This must not + silently trust the identity recorded when the verdict was computed -- + a mismatch (e.g. a misconfigured refresh binding a different identity) + must fail closed rather than post under an unverified identity. + """ + calls = [] + monkeypatch.setattr(noema, "submit_review", lambda *args, **kwargs: calls.append(args)) + monkeypatch.setattr(noema, "current_actor", lambda: "cwl-noema-review[bot]") + state = { + "pr": make_pr(), + "actor": "cwl-noema-review[bot]", + "verdict": {"decision": "approve", "summary": "ok", "findings": []}, + } + noema.submit_pending_verdict("owner/repo", 7, state) + assert calls == [("owner/repo", 7, state["pr"], "cwl-noema-review[bot]", state["verdict"])] + + calls.clear() + monkeypatch.setattr(noema, "current_actor", lambda: "a-different-identity[bot]") + with pytest.raises(RuntimeError, match="does not match the identity"): + noema.submit_pending_verdict("owner/repo", 7, state) + assert calls == [] + + monkeypatch.setattr(noema, "current_actor", lambda: "") + with pytest.raises(RuntimeError, match="identity could not be verified"): + noema.submit_pending_verdict("owner/repo", 7, state) + + def test_call_llm_rejects_empty_review_content(monkeypatch): monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") @@ -715,6 +804,16 @@ def open(self, request, timeout): assert noema.call_llm("owner/repo", 7, make_pr(), diff, False)["decision"] == "approve" assert len(payloads) == 2 assert "trusted validator" in payloads[1]["messages"][1]["content"] + # Evidence for LLM_REQUEST_TIMEOUT_SECONDS applying unchanged to a repair + # retry (ContextualWisdomLab/.github#1509, Devin Review): the repair + # attempt resends the SAME full diff as the original attempt, not a + # smaller patch, so it is not typically a cheaper/faster call and gets no + # smaller a timeout budget than the original. + original_content = payloads[0]["messages"][1]["content"] + repair_content = payloads[1]["messages"][1]["content"] + assert original_content.count(diff) == 1 + assert repair_content.count(diff) == 1 + assert len(repair_content) >= len(original_content) def test_substantive_approve_requires_exact_changed_lines_and_falsified_probes(): @@ -1016,6 +1115,8 @@ def test_parse_args_and_main(monkeypatch): parsed = noema.parse_args(["--repo", "owner/repo", "--pr-number", "9"]) assert parsed.repo == "owner/repo" assert parsed.pr_number == 9 + assert parsed.phase is None + assert parsed.state_file is None seen = [] monkeypatch.setattr(noema, "inspect_and_review", lambda repo, number: seen.append((repo, number)) or 0) @@ -1024,3 +1125,40 @@ def test_parse_args_and_main(monkeypatch): with pytest.raises(SystemExit, match="--pr-number must be positive"): noema.main(["--repo", "owner/repo", "--pr-number", "0"]) + + +def test_main_phase_requires_state_file(monkeypatch): + """--phase without --state-file must fail closed, for either phase value.""" + for phase in ("review", "submit"): + with pytest.raises(SystemExit, match="--state-file is required with --phase"): + noema.main(["--repo", "owner/repo", "--pr-number", "9", "--phase", phase]) + + +def test_main_review_phase_writes_state_only_when_computed(monkeypatch, tmp_path): + """--phase review must persist state on success and write nothing to skip.""" + state_path = str(tmp_path / "state.json") + monkeypatch.setattr(noema, "run_review_phase", lambda repo, number: {"pr": {}, "actor": "noema", "verdict": {}}) + assert noema.main(["--repo", "owner/repo", "--pr-number", "9", "--phase", "review", "--state-file", state_path]) == 0 + assert noema.load_review_state(state_path) == {"pr": {}, "actor": "noema", "verdict": {}} + + skip_path = str(tmp_path / "skip.json") + monkeypatch.setattr(noema, "run_review_phase", lambda repo, number: None) + assert noema.main(["--repo", "owner/repo", "--pr-number", "9", "--phase", "review", "--state-file", skip_path]) == 0 + assert noema.load_review_state(skip_path) is None + + +def test_main_submit_phase_submits_or_skips(monkeypatch, tmp_path): + """--phase submit must submit persisted state, or skip cleanly when absent.""" + missing_path = str(tmp_path / "missing.json") + monkeypatch.setattr( + noema, "submit_pending_verdict", lambda *args, **kwargs: (_ for _ in ()).throw(AssertionError("must not submit")) + ) + assert noema.main(["--repo", "owner/repo", "--pr-number", "9", "--phase", "submit", "--state-file", missing_path]) == 0 + + state_path = str(tmp_path / "state.json") + state = {"pr": {}, "actor": "noema", "verdict": {}} + noema.write_review_state(state_path, state) + seen = [] + monkeypatch.setattr(noema, "submit_pending_verdict", lambda repo, number, s: seen.append((repo, number, s))) + assert noema.main(["--repo", "owner/repo", "--pr-number", "9", "--phase", "submit", "--state-file", state_path]) == 0 + assert seen == [("owner/repo", 9, state)] diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index b3eac37fac..cf03bcedd1 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -604,7 +604,7 @@ def test_strix_gateway_default_and_noema_sidecar_fail_closed( noema_script = textwrap.dedent( workflow_step( workflow_text("noema-review.yml"), - "Run Noema LLM review and submit verdict", + "Run Noema LLM review", ).split(" run: |\n", 1)[1] ) noema_env = { From c9fd9ddc50c837905f7d27b74696ad2464ac8248 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 11:52:21 +0000 Subject: [PATCH 3/5] fix(noema): validate TOKEN_EXCHANGE_URL scheme; re-verify head on submit; 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 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- .github/workflows/noema-review.yml | 14 ++ docs/product-technical-gap-baseline.md | 107 +++++++++++ scripts/ci/noema_review_gate.py | 126 ++++++++++++- tests/test_noema_review_gate.py | 237 ++++++++++++++++++++++++- 4 files changed, 474 insertions(+), 10 deletions(-) diff --git a/.github/workflows/noema-review.yml b/.github/workflows/noema-review.yml index 4014a7b18a..3940356ef1 100644 --- a/.github/workflows/noema-review.yml +++ b/.github/workflows/noema-review.yml @@ -217,6 +217,13 @@ jobs: exit 1 } + case "$TOKEN_EXCHANGE_URL" in + https://*) ;; + *) + fail_unavailable "Noema app token exchange unavailable: TOKEN_EXCHANGE_URL must start with https:// to avoid sending the OIDC token over cleartext (observed ${TOKEN_EXCHANGE_URL:-})." + ;; + esac + if [ -z "${ACTIONS_ID_TOKEN_REQUEST_TOKEN:-}" ] || [ -z "${ACTIONS_ID_TOKEN_REQUEST_URL:-}" ]; then fail_unavailable "Noema app token exchange unavailable: OIDC request environment is missing." fi @@ -392,6 +399,13 @@ jobs: exit 1 } + case "$TOKEN_EXCHANGE_URL" in + https://*) ;; + *) + fail_unavailable "Noema app submission token exchange unavailable: TOKEN_EXCHANGE_URL must start with https:// to avoid sending the OIDC token over cleartext (observed ${TOKEN_EXCHANGE_URL:-})." + ;; + esac + if [ -z "${ACTIONS_ID_TOKEN_REQUEST_TOKEN:-}" ] || [ -z "${ACTIONS_ID_TOKEN_REQUEST_URL:-}" ]; then fail_unavailable "Noema app submission token exchange unavailable: OIDC request environment is missing." fi diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 3ae63b6dcf..42cf3487e5 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1871,6 +1871,113 @@ eight) pass `bash -n`; the workflow parses under PyYAML with the expected 12 ste `timeout-minutes: 300`. 2134 tests pass (2133 plus one pre-existing skip); 100% coverage and 100% docstring coverage on `scripts/ci/`. +## 2026-08-31 PR #1509 CodeRabbit 3건 검증: 모두 실재 결함으로 확인 후 수정 + +CodeRabbit posted a review on `ContextualWisdomLab/.github#1509` (the same PR as the two entries +above) with three "major"-severity findings, each with concrete evidence from the current diff. +Investigated all three directly against the current `noema-review.yml` and `noema_review_gate.py` +content rather than trusting the bot's framing. **All three confirmed real, none were false +positives or already-handled.** + +**Finding 1 — OIDC token exchange over plaintext HTTP was possible.** `TOKEN_EXCHANGE_URL` (`vars. +NOEMA_TOKEN_EXCHANGE_URL || vars.NOEMA_EXCHANGE_URL`) is a repository variable with no scheme +validation before either of the two "Exchange ... Noema app ... token through OIDC" steps POSTed +the freshly-minted OIDC identity token to it. A misconfigured `http://` value (or any non-`https://` +scheme) would have sent that token in cleartext. Confirmed by reading both steps directly: neither +had a scheme check anywhere before their `curl -X POST ... "${TOKEN_EXCHANGE_URL}"` call. + +**Fix.** Added a `case "$TOKEN_EXCHANGE_URL" in https://*) ;; *) fail_unavailable "..."; esac` guard +immediately after each step's existing `fail_unavailable()` helper definition, before either curl +request runs (including the earlier one to `ACTIONS_ID_TOKEN_REQUEST_URL` that fetches the OIDC +identity token in the first place, so a misconfigured target is caught before that token is even +requested, not just before it is sent onward). Both the initial "Exchange Noema app token through +OIDC" step and the post-`call_llm` "Exchange fresh Noema app submission token through OIDC" step got +the same guard, matching this file's `set -euo pipefail` / `fail_unavailable()` style exactly rather +than inventing a new error-handling shape. `TOKEN_EXCHANGE_URL` is a repo-owned config value (already +gated to `source == 'oidc'`, meaning it is guaranteed non-empty by the time this runs), not +attacker-controlled input, so this is defense-in-depth against a misconfiguration, sized as +CodeRabbit itself scoped it ("quick win"). + +**Finding 2 — `submit_pending_verdict` did not re-verify the PR head before submitting.** +Read `submit_pending_verdict` directly: it re-verifies reviewer *identity* against the fresh +submission credential (the fix from the entry above) but never re-called `fetch_pr`, so it always +submitted `state["pr"]` -- the PR snapshot `run_review_phase` fetched before the (now up to ~4-hour) +`call_llm` call -- unconditionally. A new commit landing on the PR during that window would make +`submit_review`'s `commit_id` (from the stale `state["pr"]["headRefOid"]`) point at a commit that is +no longer the PR's head, attaching a review to an outdated diff -- directly contradicting this org's +own exact-head evidence model (`PR_GOVERNANCE_AUDIT.md`: "Old approvals and old checks are not merge +evidence after the head SHA changes"). + +**Fix.** `submit_pending_verdict` now calls `fetch_pr(repo, number)` again immediately before +`submit_review`, compares the freshly-fetched `headRefOid` against `state["pr"]["headRefOid"]`, and +raises a bounded `RuntimeError` ("PR head changed from ... to ...") if they differ -- the same +fail-closed pattern (`main`'s top-level handler turns any `RuntimeError` into a clean non-zero exit +with a `::error::` annotation) this file already uses for the identity-mismatch case right above it. +`submit_review` is never called once a mismatch is detected. Regression test +`test_submit_pending_verdict_rejects_stale_head_between_phases` mocks `fetch_pr` to return a +different `headRefOid` on the (now second) call than the one persisted in `state`, and asserts +`submit_review` is not called. The pre-existing +`test_submit_pending_verdict_matches_and_rejects_identity_drift` was updated to mock `fetch_pr` +returning a matching head, since the happy path now depends on it. + +**Finding 3 — no monotonic total deadline on `call_llm`'s HTTP read.** Read `call_llm` in full +(the single `opener.open(request, timeout=LLM_REQUEST_TIMEOUT_SECONDS)` plus one unconditional +`response.read()`). Confirmed CodeRabbit's cited `urllib.request`/`socket` semantics empirically in +this sandbox (a real local HTTP server trickling small chunks with delays under the socket's +configured timeout kept `response.read()` blocked for the whole trickle regardless of the timeout +value): the `timeout=` argument to `opener.open` bounds the connection phase and each individual +blocking socket read, never the cumulative time `response.read()` spends looping over many such +reads to reach EOF. A server -- pathological or merely very slow -- trickling data at intervals +shorter than `LLM_REQUEST_TIMEOUT_SECONDS` could keep one `call_llm` attempt's read phase alive +indefinitely, and combined with the one possible repair-retry attempt, blow past the declared +`LLM_REQUEST_TOTAL_BUDGET_SECONDS` (14400s) worst-case bound this PR's own tests assert elsewhere. + +**Fix.** `call_llm` now accepts an optional `deadline` (a `time.monotonic()` timestamp), defaulting +to `time.monotonic() + LLM_REQUEST_TOTAL_BUDGET_SECONDS` on the first call and threaded unchanged +through the recursive repair-retry call, so the original attempt and its retry share one end-to-end +budget instead of each implicitly getting a fresh one. Before each attempt, a +`remaining_budget <= 0` check fails closed immediately ("before this attempt could start") rather +than starting a network call that cannot legally complete. The per-attempt connection timeout is now +`min(LLM_REQUEST_TIMEOUT_SECONDS, remaining_budget)` instead of the constant +`LLM_REQUEST_TIMEOUT_SECONDS`. The response body is read through a new +`_read_response_body_within_deadline` helper: since `io.BufferedReader.read()` issues as many +underlying socket reads as it takes to reach EOF (confirmed directly, not assumed -- see the sandbox +probe above) and each individually satisfies a per-read timeout even when their sum does not, +re-checking the deadline only *between* whole `read()` calls would not catch the pathological case +in time. Instead, a `threading.Timer` watchdog is armed for the remaining budget; if it fires before +`response.read()` returns on its own, it force-closes the read side of the socket +(`shutdown(SHUT_RDWR)`, confirmed in this sandbox to reliably interrupt a concurrent blocked read, +unlike `close()`, which does not reliably unblock another thread's in-progress `recv()`). Whether the +interrupted read then raises `OSError` or simply returns a truncated body without one (both observed +empirically depending on timing), the helper raises the same bounded, fail-closed `RuntimeError` +this module already uses elsewhere. An OSError raised for an unrelated reason, before the watchdog +ever fires, still propagates unchanged rather than being misreported as a budget failure. Works on +Python >= 3.10 (this repo's `pyproject.toml` `requires-python`; `scripts/ci/noema_review_gate.py` has +no narrower pin) -- confirmed no APIs used here (`threading.Timer`, `socket.shutdown`, +`time.monotonic`, `response.fp.raw._sock`) require anything newer. + +Regression coverage (`tests/test_noema_review_gate.py`): +`test_call_llm_enforces_monotonic_deadline_on_a_slow_trickling_response` spins up a real local +`http.server.HTTPServer` (routed through the existing loopback sidecar allowlist, not a mock) that +trickles its body in 12 chunks 0.3s apart (3.6s total) against a monkeypatched 1.0s +`LLM_REQUEST_TOTAL_BUDGET_SECONDS`, and asserts `call_llm` raises the bounded `RuntimeError` in well +under the full trickle duration -- proving the deadline is enforced mid-read, not merely once per +attempt. Four focused unit tests on `_read_response_body_within_deadline` directly cover the +already-expired-deadline pre-check, a failing `shutdown()` that must not prevent the timeout from +still being reported, an interrupted read that raises `OSError` after the watchdog fires, and an +unrelated `OSError` raised before the watchdog ever fires (which must propagate unmodified). One more +test exercises `call_llm`'s own pre-attempt budget check directly via the (now public in signature) +`deadline` keyword, asserting no network call is attempted once the shared deadline has already +passed. All pre-existing `call_llm` tests' response-double `read()` methods were widened to accept +(and safely ignore, via a `_read_done` guard returning `b""` on a second call) an optional chunk-size +argument, since production code no longer calls the zero-argument form directly through those doubles +in every path. 2141 tests pass (2140 plus one pre-existing skip; up from the prior entry's 2134 -- +the net of Finding 2's one new regression test and Finding 3's six); 100% coverage and 100% docstring +coverage on `scripts/ci/` (`coverage run -m pytest tests && coverage report --show-missing`, +`interrogate`). `.github/workflows/noema-review.yml`'s twelve `run: |` shell blocks still all pass +`bash -n`; the workflow still parses under PyYAML with the same 12 steps and `timeout-minutes: 300` +(only two `run:` step bodies gained lines; no step was added, removed, or renamed). + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 5adf8a5be7..1baeb5f9f3 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -13,6 +13,8 @@ import socket import subprocess import sys +import threading +import time import urllib.error import urllib.parse import urllib.request @@ -619,6 +621,88 @@ def reject_private_llm_url(api_url: str) -> None: raise ValueError("URL cannot target internal IP addresses") +def _response_raw_socket(response: Any) -> Any | None: + """Return the underlying socket of an open urllib HTTP response, or None. + + Used only to force-interrupt a still-blocked read once the monotonic + deadline passes (see ``_read_response_body_within_deadline``). Not every + response-like object exposes this (test doubles, for instance), so + callers must tolerate ``None`` and fall back to the pre-read deadline + check alone -- the same bound ``call_llm`` already applied before this + helper existed. + """ + try: + return response.fp.raw._sock # noqa: SLF001 + except AttributeError: + return None + + +def _read_response_body_within_deadline(response: Any, deadline: float) -> bytes: + """Read an HTTP response body without exceeding a monotonic deadline. + + ``opener.open(..., timeout=...)`` bounds only the connection phase and + each individual blocking socket read (CPython's documented + ``urllib.request``/``socket`` timeout semantics) -- it does not bound the + total time spent in ``response.read()``. A server that keeps trickling + small amounts of data at intervals shorter than that per-read timeout + could otherwise keep ``response.read()`` blocked well past + ``LLM_REQUEST_TOTAL_BUDGET_SECONDS``: ``io.BufferedReader.read()`` issues + as many underlying socket reads as it takes to reach EOF, and each one + individually satisfies the per-read timeout even though their sum does + not, so re-checking the deadline only *between* whole ``read()`` calls + would never see the pathological case in time. + + Instead, arm a watchdog timer for the remaining budget that force-closes + the read side of the socket (``shutdown(SHUT_RDWR)``, which reliably + unblocks a concurrent blocking read on the same socket, unlike + ``close()``) if it fires before ``response.read()`` returns on its own, + then read the body in one call exactly as before. If the watchdog fired, + raise the same bounded, fail-closed ``RuntimeError`` this module already + uses elsewhere, whether the interrupted read raised an ``OSError`` or + returned a truncated body without one. + """ + remaining = deadline - time.monotonic() + if remaining <= 0: + raise RuntimeError( + "Noema LLM request exceeded LLM_REQUEST_TOTAL_BUDGET_SECONDS " + "before the response body could be read" + ) + raw_socket = _response_raw_socket(response) + timed_out = threading.Event() + watchdog: threading.Timer | None = None + if raw_socket is not None: + + def _expire() -> None: + """Mark the deadline as passed and force-close the read side.""" + timed_out.set() + try: + raw_socket.shutdown(socket.SHUT_RDWR) + except OSError: + pass + + watchdog = threading.Timer(remaining, _expire) + watchdog.daemon = True + watchdog.start() + try: + body = response.read() + except OSError as exc: + if timed_out.is_set(): + raise RuntimeError( + "Noema LLM request exceeded LLM_REQUEST_TOTAL_BUDGET_SECONDS " + "while reading the response body" + ) from exc + raise + finally: + if watchdog is not None: + watchdog.cancel() + if timed_out.is_set(): + raise RuntimeError( + "Noema LLM request exceeded LLM_REQUEST_TOTAL_BUDGET_SECONDS " + "while reading the response body" + ) + return body + + def call_llm( repo: str, number: int, @@ -628,8 +712,24 @@ def call_llm( review_context: str = "", changed_paths: Sequence[str] = (), repair_error: str = "", + deadline: float | None = None, ) -> dict[str, Any]: - """Call the configured OpenAI-compatible LLM endpoint for a review verdict.""" + """Call the configured OpenAI-compatible LLM endpoint for a review verdict. + + ``deadline`` is a ``time.monotonic()`` timestamp shared across the + original attempt and its at-most-one repair retry, so the two together + are held to ``LLM_REQUEST_TOTAL_BUDGET_SECONDS`` end-to-end rather than + each independently getting a fresh full timeout. Callers should leave it + unset; it is set once here on the first (non-retry) call and threaded + through the recursive repair-retry call below. + """ + if deadline is None: + deadline = time.monotonic() + LLM_REQUEST_TOTAL_BUDGET_SECONDS + remaining_budget = deadline - time.monotonic() + if remaining_budget <= 0: + raise RuntimeError( + "Noema LLM request exceeded LLM_REQUEST_TOTAL_BUDGET_SECONDS before this attempt could start" + ) api_url = os.environ.get("NOEMA_LLM_API_URL", "").strip() api_key = os.environ.get("NOEMA_LLM_API_KEY", "").strip() model = os.environ.get("NOEMA_LLM_MODEL", "").strip() or "noema-default" @@ -685,8 +785,9 @@ def call_llm( method="POST", ) opener = urllib.request.build_opener(NoRedirectHandler()) - with opener.open(request, timeout=LLM_REQUEST_TIMEOUT_SECONDS) as response: # nosec B310 - raw = response.read().decode("utf-8") + attempt_timeout = min(LLM_REQUEST_TIMEOUT_SECONDS, remaining_budget) + with opener.open(request, timeout=attempt_timeout) as response: # nosec B310 + raw = _read_response_body_within_deadline(response, deadline).decode("utf-8") data = json.loads(raw) content = (((data.get("choices") or [{}])[0].get("message") or {}).get("content") or "").strip() verdict = extract_json_object(content) @@ -727,6 +828,7 @@ def call_llm( review_context, changed_paths, str(exc), + deadline=deadline, ) return verdict @@ -873,6 +975,16 @@ def submit_pending_verdict(repo: str, number: int, state: dict[str, Any]) -> Non identity between the two phases is refused instead of silently trusted -- this preserves the same verified-reviewer-identity guarantee ``run_review_phase`` already enforced, under the new credential. + + The same multi-hour window means a new commit can land on the PR between + when ``run_review_phase`` persisted ``state["pr"]["headRefOid"]`` and when + this function actually runs. Re-fetch the PR here and compare the fresh + ``headRefOid`` against the persisted one before submitting: this org's own + exact-head evidence model treats a review attached to a commit other than + the one it was computed against as invalid (see + ``PR_GOVERNANCE_AUDIT.md``: "Old approvals and old checks are not merge + evidence after the head SHA changes"), so a head mismatch here must abort + the submission rather than silently post a verdict against a stale diff. """ actor = current_actor() if not actor: @@ -883,6 +995,14 @@ def submit_pending_verdict(repo: str, number: int, state: dict[str, Any]) -> Non f"identity {state['actor']!r} that computed the verdict; refusing to " "submit under a different identity." ) + persisted_head = str((state.get("pr") or {}).get("headRefOid") or "") + current_head = str(fetch_pr(repo, number).get("headRefOid") or "") + if current_head != persisted_head: + raise RuntimeError( + f"Noema PR head changed from {persisted_head!r} to {current_head!r} " + "between the review and submission phases; refusing to submit a " + "verdict computed against a stale commit." + ) submit_review(repo, number, state["pr"], actor, state["verdict"]) diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 5a250130d9..ff4f80db50 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -1,6 +1,9 @@ import base64 +import http.server import json import sys +import threading +import time import pytest @@ -258,8 +261,11 @@ def __exit__(self, *args): """Propagate exceptions from the with-statement body.""" return False - def read(self): - """Return the payload as encoded JSON bytes.""" + def read(self, amt=None): + """Return the payload as encoded JSON bytes, then an empty chunk.""" + if getattr(self, "_read_done", False): + return b"" + self._read_done = True return json.dumps(self.payload).encode("utf-8") @@ -416,6 +422,179 @@ def test_llm_request_timeout_matches_org_two_hour_per_model_policy(): ) +def test_call_llm_enforces_monotonic_deadline_on_a_slow_trickling_response(monkeypatch): + """call_llm must not let a trickling response outlive the total budget. + + Regression coverage for CodeRabbit's finding on ContextualWisdomLab/.github#1509: + ``opener.open(..., timeout=LLM_REQUEST_TIMEOUT_SECONDS)`` only bounds the + connection phase and each individual socket read (confirmed CPython + ``urllib.request``/``socket`` timeout semantics), not the cumulative time + spent in ``response.read()``. A server that keeps trickling small amounts + of data at intervals shorter than that per-read timeout could otherwise + keep a still-alive connection open past ``LLM_REQUEST_TOTAL_BUDGET_SECONDS``. + + This spins up a real local HTTP server (through the loopback sidecar + allowlist, not a mock) that sends its body in small delayed chunks whose + total duration comfortably exceeds a monkeypatched, deliberately tiny + ``LLM_REQUEST_TOTAL_BUDGET_SECONDS``, and asserts call_llm fails closed + with a clear error well before the full trickle would have completed -- + proving the deadline is enforced mid-read, not just once per attempt. + """ + chunk_delay_seconds = 0.3 + chunk_count = 12 # 3.6s of total trickle time, well over the budget below + + class SlowTrickleHandler(http.server.BaseHTTPRequestHandler): + """A local HTTP handler that dribbles its body out in small pieces.""" + + def do_POST(self): + """Consume the request body, then trickle a slow, chunked reply.""" + content_length = int(self.headers.get("Content-Length", "0")) + self.rfile.read(content_length) + self.send_response(200) + self.send_header("Content-Type", "application/json") + self.send_header("Connection", "close") + self.end_headers() + try: + for _ in range(chunk_count): + self.wfile.write(b" ") + self.wfile.flush() + time.sleep(chunk_delay_seconds) + except OSError: + pass # The client is expected to disconnect once its deadline fires. + + def log_message(self, *_args): + """Silence the default per-request stderr logging.""" + + server = http.server.HTTPServer(("127.0.0.1", 0), SlowTrickleHandler) + server_thread = threading.Thread(target=server.serve_forever, daemon=True) + server_thread.start() + try: + origin = f"http://127.0.0.1:{server.server_address[1]}" + monkeypatch.setenv("NOEMA_LLM_API_URL", f"{origin}/v1/chat/completions") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") + # Route through the loopback sidecar allowlist (is_allowed_orchestrator_sidecar_url) + # so reject_private_llm_url permits this 127.0.0.1 target. + monkeypatch.setenv("CONTEXTUAL_ORCHESTRATOR_BASE_URL", origin) + monkeypatch.setattr(noema, "LLM_REQUEST_TOTAL_BUDGET_SECONDS", 1.0) + monkeypatch.setattr(noema, "LLM_REQUEST_TIMEOUT_SECONDS", 10) + + start = time.monotonic() + with pytest.raises(RuntimeError, match="LLM_REQUEST_TOTAL_BUDGET_SECONDS"): + noema.call_llm("owner/repo", 7, make_pr(), "diff", False) + elapsed = time.monotonic() - start + finally: + server.shutdown() + server_thread.join(timeout=5) + + # Must abort close to the 1.0s budget, not after the full ~3.6s trickle + # and not after the (monkeypatched to 10s) per-attempt LLM_REQUEST_TIMEOUT_SECONDS. + assert elapsed < chunk_count * chunk_delay_seconds + + +def test_read_response_body_within_deadline_rejects_already_expired_deadline(): + """An already-passed deadline must fail before any read is attempted.""" + + class _UnreadableResponse: + def read(self): + raise AssertionError("must not be called once the deadline has passed") + + with pytest.raises(RuntimeError, match="before the response body could be read"): + noema._read_response_body_within_deadline(_UnreadableResponse(), time.monotonic() - 1) + + +def test_read_response_body_within_deadline_tolerates_a_failing_shutdown(): + """A watchdog whose shutdown() itself fails must still fail closed on timeout.""" + + class _FailingRawSocket: + def shutdown(self, how): + raise OSError("shutdown not supported by this fake socket") + + class _Raw: + def __init__(self, sock): + self._sock = sock + + class _SlowResponse: + def __init__(self): + self.fp = type("Fp", (), {"raw": _Raw(_FailingRawSocket())})() + + def read(self): + time.sleep(0.2) # long enough for the watchdog to have already fired + return b"partial body despite the failed shutdown" + + with pytest.raises(RuntimeError, match="while reading the response body"): + noema._read_response_body_within_deadline(_SlowResponse(), time.monotonic() + 0.05) + + +def test_read_response_body_within_deadline_converts_reset_after_watchdog_fires(): + """A read() that raises OSError once the watchdog has fired must fail closed.""" + + class _RawSocket: + def __init__(self): + self.shutdown_called = threading.Event() + + def shutdown(self, how): + self.shutdown_called.set() + + class _Raw: + def __init__(self, sock): + self._sock = sock + + class _ResetOnShutdownResponse: + def __init__(self): + self._sock = _RawSocket() + self.fp = type("Fp", (), {"raw": _Raw(self._sock)})() + + def read(self): + self._sock.shutdown_called.wait(timeout=5) + raise OSError("connection reset by the watchdog's shutdown()") + + with pytest.raises(RuntimeError, match="while reading the response body"): + noema._read_response_body_within_deadline(_ResetOnShutdownResponse(), time.monotonic() + 0.05) + + +def test_read_response_body_within_deadline_reraises_unrelated_os_error(): + """An OSError raised before the watchdog ever fires must propagate unchanged.""" + + class _RawSocket: + def shutdown(self, how): + raise AssertionError("must not be called; the read fails before any timeout") + + class _Raw: + def __init__(self, sock): + self._sock = sock + + class _ImmediatelyBrokenResponse: + def __init__(self): + self.fp = type("Fp", (), {"raw": _Raw(_RawSocket())})() + + def read(self): + raise OSError("connection reset by peer, unrelated to the deadline") + + with pytest.raises(OSError, match="unrelated to the deadline"): + noema._read_response_body_within_deadline(_ImmediatelyBrokenResponse(), time.monotonic() + 5) + + +def test_call_llm_rejects_an_already_expired_deadline_before_any_request(monkeypatch): + """call_llm's own top-of-function budget check must fail closed pre-request. + + This also guards the repair-retry recursion: if the original attempt + consumed the entire LLM_REQUEST_TOTAL_BUDGET_SECONDS, the recursive + repair call (which threads the same shared deadline through) must not + silently get a fresh timeout budget instead of failing closed. + """ + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") + + def fail_if_called(*_args, **_kwargs): + raise AssertionError("must not attempt a network call past the deadline") + + monkeypatch.setattr(noema.urllib.request, "build_opener", fail_if_called) + with pytest.raises(RuntimeError, match="before this attempt could start"): + noema.call_llm( + "owner/repo", 7, make_pr(), "diff", False, deadline=time.monotonic() - 1 + ) + + def test_noema_redirect_handler_rejects_redirects(): """Noema must not follow redirects after validating the initial URL.""" handler = noema.NoRedirectHandler() @@ -608,6 +787,7 @@ def test_submit_pending_verdict_matches_and_rejects_identity_drift(monkeypatch): calls = [] monkeypatch.setattr(noema, "submit_review", lambda *args, **kwargs: calls.append(args)) monkeypatch.setattr(noema, "current_actor", lambda: "cwl-noema-review[bot]") + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) state = { "pr": make_pr(), "actor": "cwl-noema-review[bot]", @@ -627,6 +807,33 @@ def test_submit_pending_verdict_matches_and_rejects_identity_drift(monkeypatch): noema.submit_pending_verdict("owner/repo", 7, state) +def test_submit_pending_verdict_rejects_stale_head_between_phases(monkeypatch): + """submit_pending_verdict must refuse to submit against a moved PR head. + + Regression coverage for CodeRabbit's finding on ContextualWisdomLab/.github#1509: + submit_pending_verdict did not re-call fetch_pr before submit_review, so a new + commit landing during the (now up to ~4-hour) window between run_review_phase + persisting state and this phase running would silently submit a verdict + attached to a stale commit_id -- directly undermining this org's exact-head + evidence model (PR_GOVERNANCE_AUDIT.md: "Old approvals and old checks are not + merge evidence after the head SHA changes"). fetch_pr must now be re-called + here, and a headRefOid mismatch must abort before submit_review is ever + called. + """ + calls = [] + monkeypatch.setattr(noema, "submit_review", lambda *args, **kwargs: calls.append(args)) + monkeypatch.setattr(noema, "current_actor", lambda: "cwl-noema-review[bot]") + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr(headRefOid="new-commit-landed")) + state = { + "pr": make_pr(headRefOid="stale-head"), + "actor": "cwl-noema-review[bot]", + "verdict": {"decision": "approve", "summary": "ok", "findings": []}, + } + with pytest.raises(RuntimeError, match="PR head changed"): + noema.submit_pending_verdict("owner/repo", 7, state) + assert calls == [] + + def test_call_llm_rejects_empty_review_content(monkeypatch): monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") @@ -638,7 +845,10 @@ def __enter__(self): def __exit__(self, *args): return None - def read(self): + def read(self, amt=None): + if getattr(self, "_read_done", False): + return b"" + self._read_done = True return json.dumps({"choices": [{"message": {"content": '{"decision":"approve"}'}}]}).encode() monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", lambda *args, **kwargs: Response()) @@ -663,7 +873,10 @@ def __enter__(self): def __exit__(self, *args): return None - def read(self): + def read(self, amt=None): + if getattr(self, "_read_done", False): + return b"" + self._read_done = True return json.dumps({"choices": [{"message": {"content": json.dumps(verdict)}}]}).encode() monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", lambda *args, **kwargs: Response()) @@ -696,7 +909,10 @@ def __enter__(self): def __exit__(self, *args): return None - def read(self): + def read(self, amt=None): + if getattr(self, "_read_done", False): + return b"" + self._read_done = True return json.dumps({"choices": [{"message": {"content": json.dumps(verdict)}}]}).encode() monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", lambda *args, **kwargs: Response()) @@ -716,7 +932,10 @@ def __enter__(self): def __exit__(self, *args): return None - def read(self): + def read(self, amt=None): + if getattr(self, "_read_done", False): + return b"" + self._read_done = True return json.dumps({"choices": [{"message": {"content": json.dumps(verdict)}}]}).encode() monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", lambda *args, **kwargs: Response()) @@ -781,6 +1000,7 @@ def test_call_llm_repairs_one_rejected_changed_line_verdict(monkeypatch): class Response: def __init__(self, verdict): self.verdict = verdict + self._read_done = False def __enter__(self): return self @@ -788,7 +1008,10 @@ def __enter__(self): def __exit__(self, *_args): return False - def read(self): + def read(self, amt=None): + if self._read_done: + return b"" + self._read_done = True return json.dumps( {"choices": [{"message": {"content": json.dumps(self.verdict)}}]} ).encode() From 327211050acb22893788be9ad65faa1040f969a9 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 12:12:12 +0000 Subject: [PATCH 4/5] fix(noema): give each call_llm attempt its own read-watchdog deadline Devin Review (ContextualWisdomLab/.github#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 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- docs/product-technical-gap-baseline.md | 67 +++++++++++++++++++++ scripts/ci/noema_review_gate.py | 47 ++++++++++----- tests/test_noema_review_gate.py | 81 ++++++++++++++++++++++++++ 3 files changed, 182 insertions(+), 13 deletions(-) diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 42cf3487e5..88179e3fff 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1978,6 +1978,73 @@ coverage on `scripts/ci/` (`coverage run -m pytest tests && coverage report --sh `bash -n`; the workflow still parses under PyYAML with the same 12 steps and `timeout-minutes: 300` (only two `run:` step bodies gained lines; no step was added, removed, or renamed). +## 2026-08-31 PR #1509 Devin Review follow-up: shared total-budget deadline was reused as each attempt's own read watchdog, starving the repair retry + +Devin Review posted a further round on `ContextualWisdomLab/.github#1509` (the same PR as the three +entries above) with one "bug"-severity finding on `call_llm` around line 790, plus one "analysis"-kind +note on `write_review_state` atomicity. Investigated the bug finding directly against the current +`call_llm` and `_read_response_body_within_deadline` implementation rather than trusting the bot's +framing. **Confirmed real.** The atomicity note is informational/forward-looking (current step +ordering is safe for today's single-writer usage; only a future concurrent-reuse change would need +atomic replacement) and was left as-is per the PR's own scope discipline. + +**Finding — the shared total-budget deadline was passed directly to the per-attempt read watchdog.** +Read `call_llm` in full. `deadline` is computed once, on the first (non-retry) call, as +`time.monotonic() + LLM_REQUEST_TOTAL_BUDGET_SECONDS` (4h) and threaded unchanged through the +recursive repair-retry call, which is correct as an *outer* backstop on the pair together. The bug: +that same shared `deadline` value was passed straight into `_read_response_body_within_deadline` as +*each* attempt's own read-watchdog deadline, instead of each attempt getting its own fresh +`attempt_start_time + LLM_REQUEST_TIMEOUT_SECONDS` (2h) bound. Concretely, a slow-but-healthy original +attempt whose response trickled for, say, 3 hours -- well past its own fair 2-hour allowance in +practice, since nothing was actually enforcing that allowance on the read -- would not be cut off +until the shared 4-hour deadline, leaving as little as ~1 hour for a subsequent repair retry even +though this org's own policy (`docs/product-goal-directive.md`) grants each model call, including a +repair retry, its own full two-hour allowance. The `attempt_timeout` passed to `opener.open(...)` was +already correctly computed as `min(LLM_REQUEST_TIMEOUT_SECONDS, remaining_budget)` (algebraically the +same per-attempt-vs-outer-backstop minimum the fix below makes explicit), so only the read-watchdog +deadline itself was wrong -- but since `opener.open`'s `timeout=` bounds only the connection phase and +each individual blocking socket read (not the cumulative time in `response.read()`, per the Finding-3 +entry above), a trickling response could still ride the read watchdog's shared deadline well past its +own attempt's fair share while never tripping the correctly-bounded connection timeout. + +**Fix.** `call_llm` now captures `attempt_start_time = time.monotonic()` fresh on every call +(including the repair-retry recursion), computes `attempt_deadline = attempt_start_time + +LLM_REQUEST_TIMEOUT_SECONDS`, and derives `effective_deadline = min(attempt_deadline, deadline)` -- +the earlier of this attempt's own two-hour bound and the outer four-hour backstop threaded in via the +unchanged `deadline` parameter. Both the connection-level `attempt_timeout` (`effective_deadline - +attempt_start_time`, replacing the old `remaining_budget`-based expression with an equivalent but more +legible one) and the read watchdog (`_read_response_body_within_deadline(response, effective_deadline)`, +replacing the old direct `deadline` pass-through) now derive from the same per-attempt math, so neither +attempt can individually exceed its two-hour allowance while the original-plus-retry pair still cannot +exceed the four-hour backstop. With each attempt capped at 7200s and at most two attempts, the natural +worst-case total is already 14400s, so the outer backstop is now primarily defense-in-depth rather than +the load-bearing bound -- matching Devin's own suggested fix, which this PR verified before applying. +The top-of-function pre-attempt budget check (`deadline - attempt_start_time <= 0` -> fail closed +"before this attempt could start") is preserved unchanged. `_read_response_body_within_deadline`'s +docstring was updated to describe the deadline it receives as the caller's per-attempt-or-backstop +`effective_deadline` rather than assuming it is always the total budget; its `RuntimeError` message text +was intentionally left unchanged (still names `LLM_REQUEST_TOTAL_BUDGET_SECONDS`) since the helper +cannot know which of the two bounds actually fired and re-deriving that label was out of scope for this +fix. + +Regression coverage (`tests/test_noema_review_gate.py`): +`test_call_llm_caps_a_slow_trickling_original_attempt_at_its_own_per_attempt_bound` is the mirror image +of the existing `test_call_llm_enforces_monotonic_deadline_on_a_slow_trickling_response` (same real +local `http.server.HTTPServer` trickling-chunks fixture, routed through the loopback sidecar +allowlist), but with the two monkeypatched constants swapped: a tiny `LLM_REQUEST_TIMEOUT_SECONDS` +(1.0s) and a generous `LLM_REQUEST_TOTAL_BUDGET_SECONDS` (10.0s), with the response trickling for 3.6s +-- longer than the per-attempt bound but comfortably under the total budget. It asserts the original +attempt is stopped well before the full trickle (proving it is not riding out the generous total +budget) and that a full `LLM_REQUEST_TIMEOUT_SECONDS`-sized share of the total budget remains +unconsumed afterward (proving a hypothetical repair retry would not have been starved). All prior +`call_llm`/`_read_response_body_within_deadline` tests were re-verified unchanged and still pass, +including the existing total-budget-binding trickle test (where the per-attempt bound is deliberately +set larger than the total budget, so `effective_deadline` still resolves to the total-budget deadline +and that test's behavior is unaffected). 2142 tests pass (2141 plus the one pre-existing skip; up from +the prior entry's 2141 total -- the net of this one new regression test); 100% coverage and 100% +docstring coverage on `scripts/ci/` (`coverage run -m pytest tests && coverage report --show-missing`, +`interrogate`). + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 1baeb5f9f3..a2dc3ac3c8 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -645,9 +645,12 @@ def _read_response_body_within_deadline(response: Any, deadline: float) -> bytes ``urllib.request``/``socket`` timeout semantics) -- it does not bound the total time spent in ``response.read()``. A server that keeps trickling small amounts of data at intervals shorter than that per-read timeout - could otherwise keep ``response.read()`` blocked well past - ``LLM_REQUEST_TOTAL_BUDGET_SECONDS``: ``io.BufferedReader.read()`` issues - as many underlying socket reads as it takes to reach EOF, and each one + could otherwise keep ``response.read()`` blocked well past the caller's + ``deadline`` -- ``call_llm`` passes the earlier of this attempt's own + ``LLM_REQUEST_TIMEOUT_SECONDS`` bound and the outer + ``LLM_REQUEST_TOTAL_BUDGET_SECONDS`` backstop, so this helper enforces + whichever of the two is closer: ``io.BufferedReader.read()`` issues as + many underlying socket reads as it takes to reach EOF, and each one individually satisfies the per-read timeout even though their sum does not, so re-checking the deadline only *between* whole ``read()`` calls would never see the pathological case in time. @@ -716,20 +719,38 @@ def call_llm( ) -> dict[str, Any]: """Call the configured OpenAI-compatible LLM endpoint for a review verdict. - ``deadline`` is a ``time.monotonic()`` timestamp shared across the - original attempt and its at-most-one repair retry, so the two together - are held to ``LLM_REQUEST_TOTAL_BUDGET_SECONDS`` end-to-end rather than - each independently getting a fresh full timeout. Callers should leave it + ``deadline`` is the outer ``call_start_time + LLM_REQUEST_TOTAL_BUDGET_SECONDS`` + ``time.monotonic()`` bound shared across the original attempt and its + at-most-one repair retry -- a defense-in-depth backstop on the *pair* + together, not a budget either attempt draws down from individually. Each + attempt (the original call, and the repair retry if the model's response + fails validation) instead gets its own fresh ``attempt_start_time`` here + and its own full ``LLM_REQUEST_TIMEOUT_SECONDS`` from that point, per this + org's per-model-call policy (see the comment above + ``LLM_REQUEST_TIMEOUT_SECONDS``). An earlier version of this function + reused the shared ``deadline`` directly as the response-read watchdog's + deadline for both attempts, so a slow-but-healthy original attempt could + run for up to the *entire* ``LLM_REQUEST_TOTAL_BUDGET_SECONDS`` before + being cut off, silently starving a subsequent repair retry of its fair + two-hour share (ContextualWisdomLab/.github#1509, Devin Review). + ``effective_deadline`` below -- the earlier of this attempt's own bound + and the outer backstop -- is what is actually enforced against this + attempt's connection and response read; ``deadline`` itself is only + re-checked, unchanged, as the outer backstop each time this function (or + its repair-retry recursion) starts. Callers should leave ``deadline`` unset; it is set once here on the first (non-retry) call and threaded - through the recursive repair-retry call below. + through the recursive repair-retry call below unchanged, so both attempts + share the same outer backstop. """ + attempt_start_time = time.monotonic() if deadline is None: - deadline = time.monotonic() + LLM_REQUEST_TOTAL_BUDGET_SECONDS - remaining_budget = deadline - time.monotonic() - if remaining_budget <= 0: + deadline = attempt_start_time + LLM_REQUEST_TOTAL_BUDGET_SECONDS + if deadline - attempt_start_time <= 0: raise RuntimeError( "Noema LLM request exceeded LLM_REQUEST_TOTAL_BUDGET_SECONDS before this attempt could start" ) + attempt_deadline = attempt_start_time + LLM_REQUEST_TIMEOUT_SECONDS + effective_deadline = min(attempt_deadline, deadline) api_url = os.environ.get("NOEMA_LLM_API_URL", "").strip() api_key = os.environ.get("NOEMA_LLM_API_KEY", "").strip() model = os.environ.get("NOEMA_LLM_MODEL", "").strip() or "noema-default" @@ -785,9 +806,9 @@ def call_llm( method="POST", ) opener = urllib.request.build_opener(NoRedirectHandler()) - attempt_timeout = min(LLM_REQUEST_TIMEOUT_SECONDS, remaining_budget) + attempt_timeout = effective_deadline - attempt_start_time with opener.open(request, timeout=attempt_timeout) as response: # nosec B310 - raw = _read_response_body_within_deadline(response, deadline).decode("utf-8") + raw = _read_response_body_within_deadline(response, effective_deadline).decode("utf-8") data = json.loads(raw) content = (((data.get("choices") or [{}])[0].get("message") or {}).get("content") or "").strip() verdict = extract_json_object(content) diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index ff4f80db50..8939a2903d 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -491,6 +491,87 @@ def log_message(self, *_args): assert elapsed < chunk_count * chunk_delay_seconds +def test_call_llm_caps_a_slow_trickling_original_attempt_at_its_own_per_attempt_bound(monkeypatch): + """The original attempt must be cut off at its own bound, not the shared total budget. + + Regression coverage for Devin Review's finding on ContextualWisdomLab/.github#1509, + round two: ``call_llm`` used the shared ``LLM_REQUEST_TOTAL_BUDGET_SECONDS`` deadline + directly as the response-read watchdog's deadline for *both* the original attempt and + the repair retry, instead of giving each attempt its own fresh + ``attempt_start_time + LLM_REQUEST_TIMEOUT_SECONDS`` bound. A slow-but-healthy original + attempt could therefore run for up to the entire total budget before being cut off, + silently starving a subsequent repair retry of the fair per-model share the org's policy + promises it. + + This is the mirror image of + ``test_call_llm_enforces_monotonic_deadline_on_a_slow_trickling_response`` above (same + real local ``http.server`` trickling-chunks fixture), but with the two monkeypatched + constants swapped: here ``LLM_REQUEST_TIMEOUT_SECONDS`` (the per-attempt bound) is the + tiny one and ``LLM_REQUEST_TOTAL_BUDGET_SECONDS`` (the shared backstop) is generously + large, so the response trickles for longer than the per-attempt bound but comfortably + under the total budget. Before the fix this would run for the full ~3.6s trickle (or + until the generous total budget); after the fix it must abort at close to the tiny + per-attempt bound, proving the attempt is capped by its own fair share rather than by + the shared backstop, and leaving most of the total budget unconsumed for a hypothetical + repair retry. + """ + chunk_delay_seconds = 0.3 + chunk_count = 12 # 3.6s of total trickle time, well over the per-attempt bound below + + class SlowTrickleHandler(http.server.BaseHTTPRequestHandler): + """A local HTTP handler that dribbles its body out in small pieces.""" + + def do_POST(self): + """Consume the request body, then trickle a slow, chunked reply.""" + content_length = int(self.headers.get("Content-Length", "0")) + self.rfile.read(content_length) + self.send_response(200) + self.send_header("Content-Type", "application/json") + self.send_header("Connection", "close") + self.end_headers() + try: + for _ in range(chunk_count): + self.wfile.write(b" ") + self.wfile.flush() + time.sleep(chunk_delay_seconds) + except OSError: + pass # The client is expected to disconnect once its deadline fires. + + def log_message(self, *_args): + """Silence the default per-request stderr logging.""" + + server = http.server.HTTPServer(("127.0.0.1", 0), SlowTrickleHandler) + server_thread = threading.Thread(target=server.serve_forever, daemon=True) + server_thread.start() + try: + origin = f"http://127.0.0.1:{server.server_address[1]}" + monkeypatch.setenv("NOEMA_LLM_API_URL", f"{origin}/v1/chat/completions") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") + # Route through the loopback sidecar allowlist (is_allowed_orchestrator_sidecar_url) + # so reject_private_llm_url permits this 127.0.0.1 target. + monkeypatch.setenv("CONTEXTUAL_ORCHESTRATOR_BASE_URL", origin) + # Per-attempt bound is the tiny one here; the shared total budget is generous -- + # the opposite of the sibling test above -- so it is the per-attempt bound, not + # the total budget, that must be what actually cuts this attempt off. + monkeypatch.setattr(noema, "LLM_REQUEST_TIMEOUT_SECONDS", 1.0) + monkeypatch.setattr(noema, "LLM_REQUEST_TOTAL_BUDGET_SECONDS", 10.0) + + start = time.monotonic() + with pytest.raises(RuntimeError, match="while reading the response body"): + noema.call_llm("owner/repo", 7, make_pr(), "diff", False) + elapsed = time.monotonic() - start + finally: + server.shutdown() + server_thread.join(timeout=5) + + # Must abort close to the 1.0s per-attempt bound, not after the full ~3.6s trickle. + assert elapsed < chunk_count * chunk_delay_seconds + # And it must not have silently burned through time that a hypothetical repair retry + # is entitled to: after this attempt fails, a full LLM_REQUEST_TIMEOUT_SECONDS-sized + # share of the total budget must still remain unconsumed. + assert elapsed < noema.LLM_REQUEST_TOTAL_BUDGET_SECONDS - noema.LLM_REQUEST_TIMEOUT_SECONDS + + def test_read_response_body_within_deadline_rejects_already_expired_deadline(): """An already-passed deadline must fail before any read is attempted.""" From 3d2b6a8262e323c794885387d1e70297b39366b5 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 12:39:14 +0000 Subject: [PATCH 5/5] fix(noema): watchdog-guard opener.open() so trickling response headers 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 (ContextualWisdomLab/.github#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 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- docs/product-technical-gap-baseline.md | 108 ++++++++++ scripts/ci/noema_review_gate.py | 262 ++++++++++++++++++++--- tests/test_noema_review_gate.py | 276 +++++++++++++++++++++++++ 3 files changed, 616 insertions(+), 30 deletions(-) diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 88179e3fff..90f2ef8182 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -2045,6 +2045,114 @@ the prior entry's 2141 total -- the net of this one new regression test); 100% c docstring coverage on `scripts/ci/` (`coverage run -m pytest tests && coverage report --show-missing`, `interrogate`). +## 2026-08-31 PR #1509 Devin Review follow-up, round two: response HEADERS bypassed the deadline too + +Devin Review posted a further "bug"-severity finding on `ContextualWisdomLab/.github#1509` (same PR +as the four entries above), on `call_llm` around line 810. Read the current `call_llm` implementation +in full, plus `opener.open`/`http.client` receive-side semantics, before touching anything. + +**Finding -- `opener.open()` itself was unguarded, only `response.read()` was.** The prior round of +fixes (entry directly above) added `_read_response_body_within_deadline`, a watchdog that force-closes +the socket if `response.read()` runs past `effective_deadline`. That watchdog starts only once +`opener.open(request, timeout=attempt_timeout)` has already returned -- i.e. only once the status line +and headers are already fully received. `timeout=` on a socket bounds each individual blocking +operation (connect, or one `recv()`) against inactivity, not the call's total wall time, exactly as the +Finding-3/Finding-4 entries above already established for the body. A provider that instead trickles +the response HEADER bytes slowly -- one byte every `timeout - epsilon` seconds, each individual `recv()` +comfortably satisfying the per-op timeout -- can keep `opener.open()` blocked (covering TCP connect, TLS +handshake, request transmission, and status-line/header receipt) well past `effective_deadline`, with +no watchdog anywhere in that path. Confirmed real by direct empirical reproduction: a local +`http.server.HTTPServer` writing its raw status line and headers one byte at a time, with the fix +absent, blocks `opener.open()` for the full trickle duration regardless of a tiny monkeypatched +`LLM_REQUEST_TOTAL_BUDGET_SECONDS`. + +**Fix.** Applied Devin's own suggested approach (option (b), a custom `http.client.HTTPConnection`/ +`HTTPSConnection`), reusing the existing body-read watchdog's mechanism rather than inventing a +parallel one. The watchdog-arming logic itself was extracted unchanged from +`_read_response_body_within_deadline` into a new shared `_arm_deadline_watchdog(raw_socket, remaining)` +helper (same `threading.Timer` + `shutdown(SHUT_RDWR)` + `threading.Event` pattern as before; +`_read_response_body_within_deadline`'s own behavior, including its already-tested branches, is +unchanged). A new `_deadline_guarded_connection(base, deadline, state)` builds an `HTTPConnection`/ +`HTTPSConnection` subclass whose `connect()` calls `super().connect()` and then immediately arms the +shared watchdog on `self.sock` -- the earliest point a real socket exists. Because request transmission +and status-line/header receipt (`h.request()`/`h.getresponse()` inside +`urllib.request.AbstractHTTPHandler.do_open`) run on that same socket immediately afterward, still +inside the same `connect()` caller's `opener.open()` call frame, one watchdog armed at `connect()`-return +covers both phases. Two new thin handler classes, `_DeadlineHTTPHandler`/`_DeadlineHTTPSHandler`, swap +this guarded connection class into `urllib.request.build_opener(...)` in place of the stock +`HTTPHandler`/`HTTPSHandler` `call_llm` implicitly got before (`build_opener` skips its own defaults +for any handler class passed that subclasses them); `NoRedirectHandler` and all other request +construction are untouched. A new `_open_response_within_deadline(opener, request, attempt_timeout, +deadline, watchdog_state)` wraps the actual `opener.open(...)` call, mirroring +`_read_response_body_within_deadline`'s three outcomes for the header phase: an `OSError` raised after +the watchdog fired becomes the module's bounded `RuntimeError` ("...before the response headers could +be read"); an `OSError` raised for an unrelated reason (including before `connect()` ever ran, e.g. a +DNS failure) propagates unchanged; and a response handed back despite the watchdog having already fired +(its own `shutdown()` call failing, mirroring the already-tested body-read case) is rejected rather than +trusted. `call_llm` cancels the header-phase watchdog once `opener.open()` returns or raises, before the +unchanged body-read watchdog takes over for `response.read()`. + +**Vulnerability-class closure assessment (explicitly requested, not assumed).** Traced every remaining +blocking phase of one `call_llm` HTTP attempt against this module's own socket: +- TCP connect: a single blocking syscall bounded absolutely by `timeout=` (not decomposable into + repeated small reads each individually resetting an inactivity clock), so it was never trickle-able + the way headers/body are, with or without this fix. +- Request transmission and status-line/header receipt: now covered by the new watchdog above. +- Response body, including any chunked-transfer-encoding trailers: `_read_response_body_within_deadline` + wraps the entire `response.read()` call as one unit regardless of whether the internal path is + content-length- or chunk-bounded, so trailer processing was already inside the guarded region -- + not a separate gap. +- Connection pooling: not applicable -- `call_llm` builds one connection per attempt + (`Connection: close`, no keep-alive/pool reuse), so no pool-wait phase exists here to bypass anything. + +One phase remains genuinely outside both watchdogs: **the TLS handshake performed *inside* `connect()` +for an `https://` target.** A watchdog armed on the pre-TLS-wrap plain socket cannot protect the +handshake either, because the `ssl` module's `SSLContext.wrap_socket()` calls the plain socket's +`detach()` early in constructing the new `SSLSocket` (moving the live file descriptor onto the new +object and invalidating the old one) before performing the (synchronous, on-connect) handshake I/O -- +so a watchdog holding the pre-wrap socket object would be shutting down an already-invalid descriptor +by the time a slow handshake is actually blocked, and no watchdog can be armed on the post-wrap +`SSLSocket` earlier than that, since `wrap_socket()` does not return until the handshake it performs +has already finished. A malicious or compromised HTTPS `NOEMA_LLM_API_URL` endpoint could in principle +trickle TLS handshake message bytes the same way headers were trickled here, stalling `connect()` -- +and therefore `opener.open()` -- past `effective_deadline` with neither watchdog able to see it. This is +reported per this round's explicit instruction to identify rather than chase a further gap: closing it +would need either driving the TLS handshake manually with `do_handshake_on_connect=False` on a socket +whose descriptor is controlled from before the wrap, or a deadline mechanism above the socket layer +entirely (e.g. running `opener.open()` in a bounded worker and abandoning/interrupting it structurally +rather than via `shutdown()`), and is left for a follow-up PR to scope and decide rather than folded into +this one. `docs/CWL-MASTER-CONTEXT.md`'s Gap register and Project #1 should track it if the org wants it +hardened further. `NOEMA_LLM_API_URL`'s DNS resolution (inside `socket.create_connection`, itself inside +`connect()`) is a related but distinct, pre-existing stdlib limitation noted for completeness rather than +as a new finding: `socket.getaddrinfo()` takes no timeout argument at all and is bounded only by the +system resolver, unprotected by this fix or its predecessor equally. + +Regression coverage (`tests/test_noema_review_gate.py`): +`test_call_llm_enforces_monotonic_deadline_on_a_slow_trickling_response_headers` mirrors the existing +`test_call_llm_enforces_monotonic_deadline_on_a_slow_trickling_response` (same real local +`http.server.HTTPServer` fixture and loopback sidecar allowlist routing, same monkeypatched tiny +`LLM_REQUEST_TOTAL_BUDGET_SECONDS`/generous `LLM_REQUEST_TIMEOUT_SECONDS` pattern), but writes the raw +HTTP status line and headers directly onto `self.wfile` one byte at a time with a small delay between +each (`http.server.BaseHTTPRequestHandler`'s own `send_response`/`send_header`/`end_headers` helpers +buffer and flush the whole header block in one write, so they cannot produce this trickle on their own). +It asserts `call_llm` fails closed with `RuntimeError` naming "before the response headers could be +read" well before the full ~4.55s header trickle would have completed. Eight further isolated unit +tests cover the new helpers directly with fake opener/connection/socket doubles, following the existing +`_read_response_body_within_deadline` unit-test pattern (`_UnreadableResponse`, `_SlowResponse`, etc.): +both `_open_response_within_deadline` outcomes on top of the already-covered success path (OSError after +the watchdog fired -> bounded `RuntimeError`; unrelated/pre-connect OSError -> unchanged re-raise; a +response returned despite the watchdog having already fired -> rejected), watchdog cancellation on a +successful open, `_deadline_guarded_connection`'s `connect()` override arming `state["watchdog"]`/ +`state["timed_out"]` on a fake base connection (including the already-expired-deadline case, which +still arms an immediately-firing timer rather than skipping protection), and `_DeadlineHTTPSHandler +.https_open` routing through `do_open` with the guarded connection class and the stock TLS +`context`/`check_hostname`. All prior `call_llm`/`_read_response_body_within_deadline` tests were +re-verified unchanged and still pass, including both existing trickling-body tests (now additionally +exercised through the new connection classes on their real local-server HTTP path, with no behavior +change). 2151 tests total (2150 passing plus the one pre-existing skip; up from the prior entry's 2142 +total -- the net of these nine new regression tests); 100% coverage and 100% docstring coverage on +`scripts/ci/` (`coverage run -m pytest tests && coverage report --show-missing`, `interrogate`). + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index a2dc3ac3c8..8f8c7eade0 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -6,6 +6,7 @@ import argparse import ast import base64 +import http.client import ipaddress import json import os @@ -637,6 +638,50 @@ def _response_raw_socket(response: Any) -> Any | None: return None +def _arm_deadline_watchdog(raw_socket: Any, remaining: float) -> tuple[threading.Timer | None, threading.Event]: + """Arm a watchdog that force-closes ``raw_socket``'s read side once ``remaining`` elapses. + + Shared by every phase of a ``call_llm`` HTTP attempt that can block past + its ``effective_deadline`` on the same underlying socket: ``connect()`` + (request transmission and status-line/header receipt happen on the same + socket afterward, still inside the same ``opener.open()`` call -- see + ``_deadline_guarded_connection`` below) and ``response.read()`` (see + ``_read_response_body_within_deadline``). A socket ``timeout=`` only + bounds each individual blocking operation (``connect()``, or one + ``recv()``) against inactivity; a peer that keeps sending small amounts + of data at intervals shorter than that per-op timeout can keep the whole + call blocked far past an absolute deadline even though no single + operation ever times out on its own. + + Uses ``shutdown(SHUT_RDWR)`` rather than ``close()`` because only the + former reliably unblocks a concurrent blocking read on the same socket + from another thread. ``remaining`` is clamped to zero so an + already-elapsed deadline still arms an (immediately-firing) timer instead + of silently skipping protection. Returns the started ``threading.Timer`` + (``None`` if ``raw_socket`` is ``None`` -- a socket-like object that + doesn't exist yet or doesn't expose one, e.g. test doubles) and the + ``threading.Event`` the timer sets when it fires, so the caller can + distinguish "the deadline is why this call raised" from any other + failure once the guarded operation returns or raises. + """ + timed_out = threading.Event() + watchdog: threading.Timer | None = None + if raw_socket is not None: + + def _expire() -> None: + """Mark the deadline as passed and force-close the read side.""" + timed_out.set() + try: + raw_socket.shutdown(socket.SHUT_RDWR) + except OSError: + pass + + watchdog = threading.Timer(max(remaining, 0.0), _expire) + watchdog.daemon = True + watchdog.start() + return watchdog, timed_out + + def _read_response_body_within_deadline(response: Any, deadline: float) -> bytes: """Read an HTTP response body without exceeding a monotonic deadline. @@ -655,13 +700,12 @@ def _read_response_body_within_deadline(response: Any, deadline: float) -> bytes not, so re-checking the deadline only *between* whole ``read()`` calls would never see the pathological case in time. - Instead, arm a watchdog timer for the remaining budget that force-closes - the read side of the socket (``shutdown(SHUT_RDWR)``, which reliably - unblocks a concurrent blocking read on the same socket, unlike - ``close()``) if it fires before ``response.read()`` returns on its own, - then read the body in one call exactly as before. If the watchdog fired, - raise the same bounded, fail-closed ``RuntimeError`` this module already - uses elsewhere, whether the interrupted read raised an ``OSError`` or + Instead, arm a watchdog timer (``_arm_deadline_watchdog``) for the + remaining budget that force-closes the read side of the socket if it + fires before ``response.read()`` returns on its own, then read the body + in one call exactly as before. If the watchdog fired, raise the same + bounded, fail-closed ``RuntimeError`` this module already uses + elsewhere, whether the interrupted read raised an ``OSError`` or returned a truncated body without one. """ remaining = deadline - time.monotonic() @@ -671,21 +715,7 @@ def _read_response_body_within_deadline(response: Any, deadline: float) -> bytes "before the response body could be read" ) raw_socket = _response_raw_socket(response) - timed_out = threading.Event() - watchdog: threading.Timer | None = None - if raw_socket is not None: - - def _expire() -> None: - """Mark the deadline as passed and force-close the read side.""" - timed_out.set() - try: - raw_socket.shutdown(socket.SHUT_RDWR) - except OSError: - pass - - watchdog = threading.Timer(remaining, _expire) - watchdog.daemon = True - watchdog.start() + watchdog, timed_out = _arm_deadline_watchdog(raw_socket, remaining) try: body = response.read() except OSError as exc: @@ -706,6 +736,153 @@ def _expire() -> None: return body +def _deadline_guarded_connection(base: type, deadline: float, state: dict[str, Any]) -> type: + """Build an ``http.client`` connection subclass that watchdog-guards ``connect()``. + + ``opener.open(request, timeout=...)`` -- covering TCP connect, TLS + handshake, request transmission, and status-line/header receipt -- can + itself be kept blocked well past ``effective_deadline`` by a peer that + trickles response HEADER bytes slowly (one byte every ``timeout - + epsilon`` seconds), for exactly the reason ``_arm_deadline_watchdog``'s + docstring gives: ``timeout=`` bounds only each individual blocking + socket operation, not the call's total wall time. This is the same + vulnerability class ``_read_response_body_within_deadline`` already + fixes for the body, one phase earlier (ContextualWisdomLab/.github#1509, + Devin Review): that watchdog only starts once ``opener.open()`` has + already returned, i.e. once headers are fully received, so it cannot + protect anything that happens before then. + + ``urllib.request`` gives no way to guard ``opener.open()`` from the + outside -- the socket it blocks on does not exist until a connection + object is constructed deep inside it, and by the time ``opener.open()`` + returns or raises it is too late to have protected the call. So this + subclasses the connection itself instead: the instant ``connect()`` + hands back a live socket, arm the same shared watchdog + ``_read_response_body_within_deadline`` uses via + ``_arm_deadline_watchdog``, on that exact socket. Request transmission + and status-line/header receipt (``h.request()``/``h.getresponse()`` in + ``AbstractHTTPHandler.do_open``) happen on that same socket immediately + afterward, still inside this same ``connect()`` caller's + ``opener.open()`` frame, so one watchdog covers both. + + ``state`` is a plain ``dict`` the caller owns and can always inspect + afterward, whether ``opener.open()`` returns or raises, since the + connection object itself is never handed back on either path -- see + ``_open_response_within_deadline``, which interprets it. + + This does not, and cannot cheaply, extend the same guarantee to the TLS + handshake that happens *inside* ``connect()`` for an HTTPS base class: + the ``ssl`` module detaches the plain socket's file descriptor into a + new ``SSLSocket`` partway through wrapping it, so a watchdog armed on + the pre-wrap socket object would be shutting down an already-invalidated + descriptor by the time a slow handshake is actually blocked, and a + watchdog armed on the post-wrap ``SSLSocket`` cannot exist until the + (synchronous, on-connect) handshake has already finished. See + ``call_llm``'s docstring for why this is accepted as a known, reported + residual gap rather than chased here. + """ + + class _DeadlineGuardedConnection(base): # type: ignore[misc] + """``base`` whose socket is watchdog-armed the instant ``connect()`` returns.""" + + def connect(self) -> None: + """Connect via the base class, then arm the shared deadline watchdog.""" + super().connect() + state["watchdog"], state["timed_out"] = _arm_deadline_watchdog( + self.sock, deadline - time.monotonic() + ) + + return _DeadlineGuardedConnection + + +class _DeadlineHTTPHandler(urllib.request.HTTPHandler): + """HTTPHandler that opens connections via a caller-supplied connection class. + + Used by ``call_llm`` to swap in a ``_deadline_guarded_connection`` result + without otherwise changing how ``urllib.request`` builds and issues the + request (see ``_deadline_guarded_connection``'s docstring for why). + """ + + def __init__(self, connection_class: type) -> None: + """Store the watchdog-guarded connection class to use for every request.""" + super().__init__() + self._connection_class = connection_class + + def http_open(self, req: urllib.request.Request) -> Any: + """Open the request using the watchdog-guarded HTTP connection class.""" + return self.do_open(self._connection_class, req) + + +class _DeadlineHTTPSHandler(urllib.request.HTTPSHandler): + """HTTPSHandler that opens connections via a caller-supplied connection class. + + Mirrors ``_DeadlineHTTPHandler`` for ``https://`` targets, forwarding the + same ``context``/``check_hostname`` a stock ``HTTPSHandler`` would so TLS + verification behavior is unchanged. + """ + + def __init__(self, connection_class: type) -> None: + """Store the watchdog-guarded connection class to use for every request.""" + super().__init__() + self._connection_class = connection_class + + def https_open(self, req: urllib.request.Request) -> Any: + """Open the request using the watchdog-guarded HTTPS connection class.""" + return self.do_open( + self._connection_class, + req, + context=self._context, + check_hostname=self._check_hostname, + ) + + +def _open_response_within_deadline( + opener: urllib.request.OpenerDirector, + request: urllib.request.Request, + attempt_timeout: float, + deadline: float, + watchdog_state: dict[str, Any], +) -> Any: + """Open ``request`` through ``opener``, failing closed if the header-phase deadline fires. + + ``opener`` must already be built with connection classes + (``_deadline_guarded_connection`` via ``_DeadlineHTTPHandler`` / + ``_DeadlineHTTPSHandler``) that populate ``watchdog_state["watchdog"]`` + and ``watchdog_state["timed_out"]`` the instant their socket connects -- + this function only interprets that state around the call, exactly as + ``_read_response_body_within_deadline`` interprets its own watchdog's + state around ``response.read()``. Mirrors that function's outcomes: an + ``OSError`` raised after the watchdog fired becomes the module's + bounded, fail-closed ``RuntimeError``; an ``OSError`` raised for an + unrelated reason (including one raised before ``connect()`` ever ran, + e.g. a DNS failure, when ``watchdog_state`` is still empty) propagates + unchanged; and a response handed back despite the watchdog having + already fired -- for example because its own ``shutdown()`` call failed + -- is rejected rather than trusted. + """ + try: + response = opener.open(request, timeout=attempt_timeout) # nosec B310 + except OSError as exc: + timed_out = watchdog_state.get("timed_out") + if timed_out is not None and timed_out.is_set(): + raise RuntimeError( + "Noema LLM request exceeded LLM_REQUEST_TOTAL_BUDGET_SECONDS " + "before the response headers could be read" + ) from exc + raise + finally: + watchdog = watchdog_state.get("watchdog") + if watchdog is not None: + watchdog.cancel() + timed_out = watchdog_state.get("timed_out") + if timed_out is not None and timed_out.is_set(): + raise RuntimeError( + "Noema LLM request exceeded LLM_REQUEST_TOTAL_BUDGET_SECONDS " + "before the response headers could be read" + ) + return response + + def call_llm( repo: str, number: int, @@ -735,12 +912,30 @@ def call_llm( two-hour share (ContextualWisdomLab/.github#1509, Devin Review). ``effective_deadline`` below -- the earlier of this attempt's own bound and the outer backstop -- is what is actually enforced against this - attempt's connection and response read; ``deadline`` itself is only - re-checked, unchanged, as the outer backstop each time this function (or - its repair-retry recursion) starts. Callers should leave ``deadline`` - unset; it is set once here on the first (non-retry) call and threaded - through the recursive repair-retry call below unchanged, so both attempts - share the same outer backstop. + attempt's connection, response headers, and response body; ``deadline`` + itself is only re-checked, unchanged, as the outer backstop each time + this function (or its repair-retry recursion) starts. Callers should + leave ``deadline`` unset; it is set once here on the first (non-retry) + call and threaded through the recursive repair-retry call below + unchanged, so both attempts share the same outer backstop. + + ``opener.open(request, timeout=attempt_timeout)`` itself -- not just the + subsequent ``response.read()`` -- is also watchdog-guarded against + ``effective_deadline`` (``_open_response_within_deadline``, backed by + ``_deadline_guarded_connection``): a peer that trickles response HEADER + bytes slowly can otherwise keep ``opener.open()`` blocked well past + ``effective_deadline`` for the same reason a trickled body could keep + ``response.read()`` blocked, since ``timeout=`` bounds only individual + blocking socket operations, not either call's total wall time + (ContextualWisdomLab/.github#1509, Devin Review). Together, the two + watchdogs bound every phase of one HTTP attempt that can block on this + module's own socket: connect, request transmission, status-line/header + receipt, and body receipt (including any chunked-transfer trailers, + which are consumed inside the same guarded ``response.read()`` call). + The one phase neither watchdog reaches is the TLS handshake performed + *inside* ``connect()`` for an ``https://`` target -- seeing why is + ``_deadline_guarded_connection``'s docstring; this is a known, reported + residual gap, not silently accepted. """ attempt_start_time = time.monotonic() if deadline is None: @@ -805,9 +1000,16 @@ def call_llm( }, method="POST", ) - opener = urllib.request.build_opener(NoRedirectHandler()) + watchdog_state: dict[str, Any] = {} + opener = urllib.request.build_opener( + NoRedirectHandler(), + _DeadlineHTTPHandler(_deadline_guarded_connection(http.client.HTTPConnection, effective_deadline, watchdog_state)), + _DeadlineHTTPSHandler(_deadline_guarded_connection(http.client.HTTPSConnection, effective_deadline, watchdog_state)), + ) attempt_timeout = effective_deadline - attempt_start_time - with opener.open(request, timeout=attempt_timeout) as response: # nosec B310 + with _open_response_within_deadline( + opener, request, attempt_timeout, effective_deadline, watchdog_state + ) as response: raw = _read_response_body_within_deadline(response, effective_deadline).decode("utf-8") data = json.loads(raw) content = (((data.get("choices") or [{}])[0].get("message") or {}).get("content") or "").strip() diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 8939a2903d..fcad2e37b8 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -655,6 +655,282 @@ def read(self): noema._read_response_body_within_deadline(_ImmediatelyBrokenResponse(), time.monotonic() + 5) +def test_call_llm_enforces_monotonic_deadline_on_a_slow_trickling_response_headers(monkeypatch): + """call_llm must not let a trickling response HEADER phase outlive the total budget. + + Regression coverage for Devin Review's follow-up finding on + ContextualWisdomLab/.github#1509 (around line 810): the read-watchdog in + ``_read_response_body_within_deadline`` only starts protecting the + response once ``opener.open()`` has already returned -- i.e. once the + status line and headers are fully received -- so it cannot see a + provider that instead trickles the response HEADER bytes slowly (one + byte every ``header_byte_delay_seconds``, comfortably under any per-recv + inactivity timeout). Before the fix, that could keep ``opener.open()`` + itself -- connect, TLS handshake, request transmission, and + status-line/header receipt -- blocked well past ``effective_deadline``, + escaping every deadline check in this module. + + Mirrors ``test_call_llm_enforces_monotonic_deadline_on_a_slow_trickling_response`` + above (same real local ``http.server.HTTPServer`` fixture, routed through + the loopback sidecar allowlist), but trickles the raw status + line/header bytes one at a time instead of the body, which + ``http.server.BaseHTTPRequestHandler``'s own ``send_response``/ + ``send_header``/``end_headers`` helpers do not do on their own (they + buffer and flush the whole header block in one write), so the response + is written directly onto ``self.wfile`` as raw bytes instead. + """ + header_byte_delay_seconds = 0.05 + raw_header = ( + b"HTTP/1.1 200 OK\r\n" + b"Content-Type: application/json\r\n" + b"Content-Length: 2\r\n" + b"Connection: close\r\n" + b"\r\n" + ) + # len(raw_header) (91) * 0.05s =~ 4.55s of total header-trickle time, well + # over the tiny total budget below, while each individual byte-to-byte gap + # stays far under the monkeypatched per-attempt LLM_REQUEST_TIMEOUT_SECONDS, + # so no single socket recv() ever times out on its own. + + class SlowHeaderTrickleHandler(http.server.BaseHTTPRequestHandler): + """A local HTTP handler that dribbles its status line/headers out a byte at a time.""" + + def do_POST(self): + """Consume the request body, then trickle a slow, byte-at-a-time header block.""" + content_length = int(self.headers.get("Content-Length", "0")) + self.rfile.read(content_length) + try: + for i in range(len(raw_header)): + self.wfile.write(raw_header[i : i + 1]) + self.wfile.flush() + time.sleep(header_byte_delay_seconds) + self.wfile.write(b"{}") + except OSError: + pass # The client is expected to disconnect once its deadline fires. + + def log_message(self, *_args): + """Silence the default per-request stderr logging.""" + + server = http.server.HTTPServer(("127.0.0.1", 0), SlowHeaderTrickleHandler) + server_thread = threading.Thread(target=server.serve_forever, daemon=True) + server_thread.start() + try: + origin = f"http://127.0.0.1:{server.server_address[1]}" + monkeypatch.setenv("NOEMA_LLM_API_URL", f"{origin}/v1/chat/completions") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") + # Route through the loopback sidecar allowlist (is_allowed_orchestrator_sidecar_url) + # so reject_private_llm_url permits this 127.0.0.1 target. + monkeypatch.setenv("CONTEXTUAL_ORCHESTRATOR_BASE_URL", origin) + monkeypatch.setattr(noema, "LLM_REQUEST_TOTAL_BUDGET_SECONDS", 1.0) + monkeypatch.setattr(noema, "LLM_REQUEST_TIMEOUT_SECONDS", 10) + + start = time.monotonic() + with pytest.raises(RuntimeError, match="before the response headers could be read"): + noema.call_llm("owner/repo", 7, make_pr(), "diff", False) + elapsed = time.monotonic() - start + finally: + server.shutdown() + server_thread.join(timeout=5) + + # Must abort close to the 1.0s budget, not after the full ~4.55s header trickle + # and not after the (monkeypatched to 10s) per-attempt LLM_REQUEST_TIMEOUT_SECONDS. + assert elapsed < len(raw_header) * header_byte_delay_seconds + + +def test_open_response_within_deadline_returns_response_when_not_timed_out(): + """A response opened before the deadline fires must be returned unchanged.""" + + class _FakeOpener: + def open(self, request, timeout=None): + return "the response" + + watchdog_state: dict = {"timed_out": threading.Event()} + result = noema._open_response_within_deadline( + _FakeOpener(), "request", 5.0, time.monotonic() + 5, watchdog_state + ) + assert result == "the response" + + +def test_open_response_within_deadline_cancels_the_watchdog_once_open_returns(): + """A still-armed watchdog must be canceled once opener.open() returns.""" + + class _FakeWatchdog: + def __init__(self): + self.canceled = False + + def cancel(self): + self.canceled = True + + class _FakeOpener: + def open(self, request, timeout=None): + return "the response" + + watchdog = _FakeWatchdog() + watchdog_state: dict = {"watchdog": watchdog, "timed_out": threading.Event()} + noema._open_response_within_deadline( + _FakeOpener(), "request", 5.0, time.monotonic() + 5, watchdog_state + ) + assert watchdog.canceled is True + + +def test_open_response_within_deadline_converts_os_error_after_watchdog_fires(): + """An OSError raised once the header-phase watchdog has fired must fail closed.""" + + class _FakeOpener: + def open(self, request, timeout=None): + raise OSError("connection reset by the watchdog's shutdown()") + + timed_out = threading.Event() + timed_out.set() + watchdog_state: dict = {"timed_out": timed_out} + with pytest.raises(RuntimeError, match="before the response headers could be read"): + noema._open_response_within_deadline( + _FakeOpener(), "request", 5.0, time.monotonic() + 5, watchdog_state + ) + + +def test_open_response_within_deadline_reraises_unrelated_os_error(): + """An OSError raised before the watchdog ever fires (or before connect() even ran, + e.g. a DNS failure -- watchdog_state stays empty) must propagate unchanged.""" + + class _FakeOpener: + def open(self, request, timeout=None): + raise OSError("connection refused, unrelated to the deadline") + + with pytest.raises(OSError, match="unrelated to the deadline"): + noema._open_response_within_deadline( + _FakeOpener(), "request", 5.0, time.monotonic() + 5, {} + ) + + +def test_open_response_within_deadline_rejects_response_when_watchdog_fired_despite_success(): + """A response handed back despite the watchdog having already fired must be rejected. + + Mirrors ``test_read_response_body_within_deadline_tolerates_a_failing_shutdown``: + if the watchdog's own ``shutdown()`` call failed to actually interrupt the + socket, ``opener.open()`` can still return what looks like a normal + response even though the deadline was already exceeded. That must not be + silently trusted. + """ + + class _FakeOpener: + def open(self, request, timeout=None): + return "a response received despite the expired deadline" + + timed_out = threading.Event() + timed_out.set() + watchdog_state: dict = {"timed_out": timed_out} + with pytest.raises(RuntimeError, match="before the response headers could be read"): + noema._open_response_within_deadline( + _FakeOpener(), "request", 5.0, time.monotonic() + 5, watchdog_state + ) + + +def test_deadline_guarded_connection_arms_watchdog_state_on_connect(): + """The guarded connection's connect() must arm the shared watchdog on its own socket. + + Isolated unit coverage for ``_deadline_guarded_connection`` alongside the + real end-to-end coverage from the header-trickle test above: a minimal + fake base class stands in for ``http.client.HTTPConnection`` so this can + assert the watchdog-arming side effect deterministically, without a real + socket or network call. + """ + + class _FakeSocket: + def __init__(self): + self.shutdown_calls = [] + + def shutdown(self, how): + self.shutdown_calls.append(how) + + class _FakeBaseConnection: + def __init__(self): + self.sock = None + + def connect(self): + self.sock = _FakeSocket() + + state: dict = {} + guarded_class = noema._deadline_guarded_connection( + _FakeBaseConnection, time.monotonic() + 5, state + ) + connection = guarded_class() + connection.connect() + + assert isinstance(connection, _FakeBaseConnection) + assert connection.sock is not None + assert "watchdog" in state and "timed_out" in state + assert isinstance(state["timed_out"], threading.Event) + assert state["timed_out"].is_set() is False + state["watchdog"].cancel() + + +def test_deadline_guarded_connection_still_arms_a_watchdog_for_an_already_expired_deadline(): + """A deadline that has already passed by connect() time must still be enforced. + + ``_arm_deadline_watchdog`` clamps a negative remaining time to zero + rather than skipping the watchdog, so a connection that connects after + its deadline has technically already elapsed still gets its read side + force-closed almost immediately, instead of being left unprotected. + """ + + class _FakeSocket: + def __init__(self): + self.shutdown_called = threading.Event() + + def shutdown(self, how): + self.shutdown_called.set() + + class _FakeBaseConnection: + def __init__(self): + self.sock = None + + def connect(self): + self.sock = _FakeSocket() + + state: dict = {} + guarded_class = noema._deadline_guarded_connection( + _FakeBaseConnection, time.monotonic() - 1, state + ) + connection = guarded_class() + connection.connect() + + assert connection.sock.shutdown_called.wait(timeout=5) + assert state["timed_out"].is_set() is True + + +def test_deadline_https_handler_opens_via_the_guarded_connection_class_and_tls_context(): + """https_open must route through do_open with the guarded class and TLS context. + + ``_DeadlineHTTPHandler.http_open`` already gets real end-to-end coverage + from the trickling-headers test above (that test targets a plain + ``http://`` origin). This directly exercises the ``https://`` sibling's + ``https_open`` the same way ``test_noema_redirect_handler_rejects_redirects`` + exercises ``NoRedirectHandler`` directly, without needing a real TLS + server: ``https_open`` is a plain method, callable with any object able + to stand in for ``self``. + """ + + calls = [] + + class _FakeSelf: + _connection_class = object() + _context = "the-tls-context" + _check_hostname = None + + def do_open(self, connection_class, req, **kwargs): + calls.append((connection_class, req, kwargs)) + return "the response" + + fake_self = _FakeSelf() + result = noema._DeadlineHTTPSHandler.https_open(fake_self, "the request") + + assert result == "the response" + assert calls == [ + (_FakeSelf._connection_class, "the request", {"context": "the-tls-context", "check_hostname": None}) + ] + + def test_call_llm_rejects_an_already_expired_deadline_before_any_request(monkeypatch): """call_llm's own top-of-function budget check must fail closed pre-request.