diff --git a/.github/workflows/noema-review.yml b/.github/workflows/noema-review.yml index 064c4e5ae..3940356ef 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' || ( @@ -205,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 @@ -299,8 +318,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 +330,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 +346,125 @@ 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" \ + --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 + } + + 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 + + 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" + --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 758ef2961..90f2ef818 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1715,6 +1715,444 @@ 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/`. + +## 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/`. + +## 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). + +## 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`). + +## 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 90a69bed3..8f8c7eade 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 @@ -13,6 +14,8 @@ import socket import subprocess import sys +import threading +import time import urllib.error import urllib.parse import urllib.request @@ -34,6 +37,38 @@ 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 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" @@ -587,6 +622,267 @@ 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 _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. + + ``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 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. + + 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() + 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) + watchdog, timed_out = _arm_deadline_watchdog(raw_socket, remaining) + 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 _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, @@ -596,8 +892,60 @@ 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 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, 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: + 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" @@ -652,9 +1000,17 @@ def call_llm( }, method="POST", ) - opener = urllib.request.build_opener(NoRedirectHandler()) - with opener.open(request, timeout=120) as response: # nosec B310 - raw = response.read().decode("utf-8") + 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 _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() verdict = extract_json_object(content) @@ -695,6 +1051,7 @@ def call_llm( review_context, changed_paths, str(exc), + deadline=deadline, ) return verdict @@ -779,8 +1136,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: @@ -792,15 +1158,92 @@ 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. + + 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: + 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." + ) + 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"]) + + +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 @@ -809,6 +1252,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) @@ -817,7 +1277,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 75ad5242c..af8bdc97e 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 338d46ba8..fcad2e37b 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") @@ -386,6 +392,566 @@ 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 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 == 7200 + max_call_llm_attempts_per_review = 2 # original call + at most one repair retry + 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_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_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.""" + + 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_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. + + 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() @@ -520,6 +1086,111 @@ 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]") + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) + 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_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") @@ -531,7 +1202,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()) @@ -556,7 +1230,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()) @@ -589,7 +1266,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()) @@ -609,7 +1289,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()) @@ -674,6 +1357,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 @@ -681,14 +1365,17 @@ 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() 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) @@ -697,6 +1384,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(): @@ -998,6 +1695,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) @@ -1006,3 +1705,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_repository_branch_coverage_review_schedulers.py b/tests/test_repository_branch_coverage_review_schedulers.py index 85cdc0b96..0ff28870a 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()) diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index b3eac37fa..cf03bcedd 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 = {