diff --git a/.github/actions/noema-review/two_phase.py b/.github/actions/noema-review/two_phase.py index 4137556a96..2815d7a050 100755 --- a/.github/actions/noema-review/two_phase.py +++ b/.github/actions/noema-review/two_phase.py @@ -167,20 +167,16 @@ def prepare_verdict(repo: str, number: int, expected_head: str, path: Path) -> i changed_files = gate.fetch_changed_files(repo, number) changed_paths = tuple(file_path for file_path, _status in changed_files) review_context = gate.build_review_context(repo, number, pull_request, changed_files) - try: - verdict = gate.call_llm( - repo, - number, - pull_request, - diff, - truncated, - expected, - review_context, - changed_paths, - ) - except gate.StaleHeadDuringRepairRetryError: - print("Pull request head changed during model repair retry; verdict was not sealed.") - return 0 + verdict = gate.call_llm( + repo, + number, + pull_request, + diff, + truncated, + expected, + review_context, + changed_paths, + ) _write_envelope( path, diff --git a/CHANGELOG.md b/CHANGELOG.md index ac1985d86f..9c5b26a684 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +## 2026-09-02 — Noema single-request gateway ownership + +- Removed the repository-owned 900-second repair deadline and duplicate model repair call from Noema. The GitHub Actions caller now issues one structured-output request while `contextual-orchestrator` owns repair/failover/timeouts. +- Hardened serving-model telemetry against control-character/workflow-command injection and lone-surrogate encoding failures, restored actionable exact changed-line diagnostics, and constrained local trailing-comma repair to complete JSON values. +- Added permanent single-request/no-fixed-timeout regressions and retired obsolete deadline/retry fixtures. + # Changelog All notable changes to the organization automation repository are documented in diff --git a/docs/doctoring/noema-repair-attempt-telemetry.md b/docs/doctoring/noema-repair-attempt-telemetry.md new file mode 100644 index 0000000000..ee4d681a59 --- /dev/null +++ b/docs/doctoring/noema-repair-attempt-telemetry.md @@ -0,0 +1,34 @@ +# Noema single-request review incident and telemetry contract + +## Incident + +On 2026-09-02, a required Noema review reported only a caller-owned 900-second repair deadline after a malformed structured response. The bound had no owner-specified or measured basis and conflicted with ADR-0003: model inference and repair verdict calls do not carry repository-authored fixed wall-clock deadlines. + +```text +initial malformed structured response -> repository repair request -> fixed 900-second abort +``` + +The later review established a second ownership error: `contextual-orchestrator` already owns structured-output validation and its governed repair/failover. Issuing another repository-side model request duplicated that policy and could turn one gateway failure into two expensive calls. + +## Final executable contract + +Noema now sends exactly one structured-output request to the configured gateway. GitHub Actions fixes the model alias to `orchestrator/free`; the caller declares no provider, paid fallback, sampling temperature, or fixed inference timeout. `contextual-orchestrator` owns provider discovery, capability routing, structured-output repair, failover, and upstream completion. The repository remains responsible for deterministic local validation and exact-head publication. + +Every gateway call emits exactly one passive Actions annotation. Success and failure annotations include caller attempt count, elapsed duration, active phase (`connecting`, `reading`, `decoding`, or `validating`), and a best-effort serving-model identifier. Serving-model text is secret-scrubbed, control-character-normalized, UTF-8 printable, and bounded before it can reach an annotation. Raw model output is never logged. + +The local trailing-comma parser remains a deterministic syntax transform only. It may remove a genuine trailing comma after a complete JSON value, but missing-value forms such as `[,]`, `{,}`, `[1,,]`, and `{"a":,}` remain invalid. The transform emits no second attempt-level annotation and never bypasses semantic verdict validation. + +Exact changed-line diagnostics include the rejected path/line/side, an unambiguous array position, and a bounded nearest-line hint. This keeps a failed verdict repairable at the gateway without expanding the output contract to one record per changed line. + +## Ownership and failure scenes + +```text +Noema workflow -> local contextual-orchestrator sidecar -> orchestrator/free -> routed free candidate + -> one returned envelope -> local deterministic validation -> exact-head publication +``` + +If the gateway cannot produce a valid structured verdict, Noema fails closed after that one caller request. If the PR head moves during model work, the post-call exact-head check discards the stale verdict. If telemetry carries hostile model identifiers, annotation sanitization prevents CR/LF or surrogate data from becoming workflow commands or crashing the runner. + +## Verification + +The permanent contract test forbids `NOEMA_REPAIR_DEADLINE_SECONDS`, `_repair_wall_clock_deadline`, `NoemaRepairDeadlineExceeded`, `signal.setitimer`, retry-only parameters/recursion, and caller-specified `temperature`. Focused regressions prove one request on success and failure, one annotation per attempt, safe serving-model telemetry, strict missing-value rejection, accepted genuine trailing commas, and preserved exact changed-line diagnostics. diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 29acdfeecc..36c86ae045 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -2613,3 +2613,15 @@ Higgins, S. S., Crepalde, N., & Fernandes, L. (2021). Segmented multiplexity: A **Expected effect.** No observable change to any current GitHub Actions review run (every current invocation already resolves to `free`). The effect is structural: it is no longer possible for a future workflow edit or manual dispatch override to admit priced-model spend into a required review check without an explicit, reviewed code change to this one `case` statement (and its now-locked-in regression test) first. **Follow-up.** If the organization later solves free+ZDR routing robustly enough to deliberately widen required-review CI to `orchestrator/auto` (e.g. once a spend ceiling and reviewer-visible cost evidence exist for that path), the change is exactly one `case` arm plus the corresponding assertions in `test_sidecar_pins_the_pool_to_free_for_github_actions` — this entry is the record of *why* it was narrowed, not a permanent prohibition. + +## Noema single-request model-control ownership — PR #1672 (2026-09-02) + +**Status:** Proposed / exact-head verification required before merge. + +**Root cause.** Noema duplicated `contextual-orchestrator` structured-output repair by making a second model request and wrapped that request in an unmeasured 900-second repository wall-clock deadline. This created a self-hosting admission failure: the required review could terminate valid long inference using policy that the gateway already owns. + +**Context Map / responsibility boundary.** `.github` owns CI review orchestration, exact-revision evidence, deterministic verdict validation and publication. `contextual-orchestrator` owns provider discovery, capability routing, `orchestrator/free`, structured-output repair/failover and provider completion. No provider/model-specific fallback or caller wall-clock timeout crosses that boundary. + +**Action.** Replace recursive caller repair with one structured-output gateway request; remove fixed deadline/signal machinery and sampling temperature; retain exact-head checks before and after model work; sanitize serving-model telemetry; restore exact changed-line diagnostics; retain bounded non-heuristic evidence cardinality and strict local JSON parsing. + +**Evidence / acceptance.** Permanent tests forbid retry/deadline/sampling symbols and prove one gateway request, one attempt annotation, control-character-safe telemetry, missing-value rejection, valid trailing-comma normalization, and exact changed-line guidance. Fresh exact-head repository checks/reviews remain the admission authority; predecessor-head evidence is not transferable. diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index f1c39a51bd..ce90b8bc84 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -6,17 +6,16 @@ import argparse import ast import base64 -import contextlib import hashlib import http.client import ipaddress import json import os import re -import signal import socket import subprocess import sys +import time import urllib.error import urllib.parse import urllib.request @@ -63,12 +62,138 @@ ORCHESTRATOR_LOOPBACK_HOSTS = frozenset({"127.0.0.1", "::1"}) ORCHESTRATOR_BASE_ENV = "CONTEXTUAL_ORCHESTRATOR_BASE_URL" -# A repair request corrects an already-completed model verdict; it is not a -# second unbounded full review. Fifteen minutes is an absolute wall-clock -# deadline for the complete corrective attempt (open/read/decode/validate), -# not a socket inactivity timeout. The primary review remains governed by -# contextual-orchestrator rather than a fixed inference timeout. -NOEMA_REPAIR_DEADLINE_SECONDS = 15 * 60 +# OpenAI Chat Completions structured-output envelope for the verdict shape +# ``validate_substantive_verdict`` enforces. contextual-orchestrator's +# ``orchestrator/free`` sidecar is proven (ADR-0003) to be an OpenAI- +# COMPATIBLE endpoint, so the outer envelope (``type`` / +# ``json_schema.name`` / ``json_schema.strict`` / ``json_schema.schema``) +# must be OpenAI's specific wrapping convention -- not bare JSON Schema and +# not Claude's tool-forcing convention. Only the inner ``schema`` value is +# the general JSON Schema document. Whether the gateway correctly translates +# this OpenAI-shaped request for a non-OpenAI-compatible backend it may +# route to is contextual-orchestrator's own translation responsibility, not +# this caller's: adding per-provider format detection here would recreate +# the layering violation the repo owner already rejected in PR #1602 one +# level down. ``strict: true`` requires every property to be listed in +# ``required`` (a conditionally-absent field is expressed as a nullable +# type, e.g. ``["array", "null"]``, never an omitted key) and every object +# to set ``additionalProperties: false``. +# +# ``adversarial_validation.probes`` carries a ``minItems`` floor built fresh +# per request from ``_required_probe_count`` rather than a fixed number: per +# ADR-0035 (`contextual-orchestrator`), the gateway parses the returned +# content and validates it against this exact declared schema -- provider +# acceptance of ``response_format`` is not proof of conformance -- and makes +# one governed same-provider repair call on a violation before this ever +# reaches Noema's own ``validate_substantive_verdict`` second pass. Without +# this floor, an insufficient-probe verdict (schema-valid JSON, just too few +# probes) reaches that second pass and fails the whole review outright with +# no earlier, cheaper structural catch -- exactly what happened in +# `ContextualWisdomLab/ConceptWeave` run `33527145686`, job `99920767480` +# ("Noema adversarial validation requires at least 2 concrete probe(s)"). +_NOEMA_REVIEWED_LINE_SCHEMA: dict[str, Any] = { + "type": "object", + "additionalProperties": False, + "properties": { + "path": {"type": "string"}, + "line": {"type": "integer"}, + "side": {"type": "string", "enum": ["LEFT", "RIGHT"]}, + "analysis": {"type": "string"}, + }, + "required": ["path", "line", "side", "analysis"], +} +_NOEMA_PROBE_SCHEMA: dict[str, Any] = { + "type": "object", + "additionalProperties": False, + "properties": { + "path": {"type": "string"}, + "line": {"type": "integer"}, + "side": {"type": "string", "enum": ["LEFT", "RIGHT"]}, + "hypothesis": {"type": "string"}, + "attack_or_counterexample": {"type": "string"}, + "evidence": {"type": "string"}, + "outcome": {"type": "string", "enum": ["falsified", "confirmed"]}, + }, + "required": [ + "path", + "line", + "side", + "hypothesis", + "attack_or_counterexample", + "evidence", + "outcome", + ], +} +_NOEMA_FINDING_SCHEMA: dict[str, Any] = { + "type": "object", + "additionalProperties": False, + "properties": { + "severity": {"type": "string", "enum": ["high", "medium", "low"]}, + "file": {"type": "string"}, + "line": {"type": "integer"}, + "side": {"type": "string", "enum": ["LEFT", "RIGHT"]}, + "message": {"type": "string"}, + }, + "required": ["severity", "file", "line", "side", "message"], +} +def _noema_verdict_json_schema(required_probes: int) -> dict[str, Any]: + """Build the verdict JSON Schema with this request's exact probe floor. + + ``required_probes`` must come from ``_required_probe_count(diff, + changed_paths)`` -- the same call ``validate_substantive_verdict`` uses + -- so the gateway-enforced structural floor and the Python-side backstop + can never silently diverge. The static per-field schemas above are safe + to share by reference here since nothing in this module mutates them. + """ + return { + "type": "object", + "additionalProperties": False, + "properties": { + "decision": { + "type": "string", + "enum": ["approve", "request_changes", "comment"], + }, + "summary": {"type": "string"}, + "reviewed_lines": { + "type": ["array", "null"], + "items": _NOEMA_REVIEWED_LINE_SCHEMA, + }, + "adversarial_validation": { + "type": ["object", "null"], + "additionalProperties": False, + "properties": { + "status": {"type": "string", "enum": ["passed", "failed"]}, + "residual_risk": {"type": "string"}, + "probes": { + "type": "array", + "minItems": required_probes, + "items": _NOEMA_PROBE_SCHEMA, + }, + }, + "required": ["status", "residual_risk", "probes"], + }, + "findings": {"type": "array", "items": _NOEMA_FINDING_SCHEMA}, + }, + "required": [ + "decision", + "summary", + "reviewed_lines", + "adversarial_validation", + "findings", + ], + } + + +def _noema_verdict_response_format(required_probes: int) -> dict[str, Any]: + """Build the OpenAI ``response_format`` envelope for this request's probe floor.""" + return { + "type": "json_schema", + "json_schema": { + "name": "noema_review_verdict", + "strict": True, + "schema": _noema_verdict_json_schema(required_probes), + }, + } class NoemaModelOutputError(RuntimeError): @@ -79,9 +204,6 @@ class NoemaTransportError(RuntimeError): """Raised when the bounded review transport cannot produce usable evidence.""" -class NoemaRepairDeadlineExceeded(TimeoutError): - """Raised when the corrective attempt exceeds its total wall-clock budget.""" - def _stable_failure_diagnostic(exc: BaseException) -> str: """Return actionable trusted diagnostics without reflecting model values.""" @@ -423,48 +545,34 @@ def parse_diff_path(raw: str, prefix: str) -> str: return value.removeprefix(prefix) -def _entry_ordinal(position: int, total: int) -> str: - """Return an unambiguous array-position label for a validated JSON entry. - - ``position`` is the entry's 1-based place in the array being validated — - an array position, not a source-code line number. The historical message - text ("Noema reviewed line N is not an exact changed-side line") read as - if N named literal file line N; it only ever named "the Nth entry" of - ``reviewed_lines``/``probes``, so two failures on entries 1 and 3 of a - 3-entry array could be misread as complaints about file lines 1 and 3 - (see the naruon#1503 investigation this fixes). Every caller splices this - immediately after the fixed ``"Noema reviewed line "``/``"Noema - adversarial probe "`` prefix so ``_stable_failure_diagnostic``'s - trusted-prefix allowlist still recognizes the message as trusted - structural validator output. +def _required_probe_count(diff: str, changed_paths: Sequence[str] = ()) -> int: + """Return the minimum adversarial-probe count a formal verdict must carry. + + This is the single source of truth shared by the structured-output schema + and deterministic local validator. Executable/test/workflow changes require + two distinct probes; other diffs require one. The bound is cardinality- + based and independent of repository path count, so a near-MAX_DIFF_CHARS + review remains representable within the gateway output budget. """ + locations = changed_diff_locations(diff) + all_changed_paths = set(changed_paths) or {path for path, _line, _side in locations} + return 2 if any(changed_file_is_material(path) for path in all_changed_paths) else 1 + + +def _entry_ordinal(position: int, total: int) -> str: + """Return an unambiguous 1-based array-position label for diagnostics.""" return f"entry {position}/{total} (array index {position - 1}, not a source line)" def _format_location(path: Any, line: Any, side: Any) -> str: - """Format one rejected path/line/side citation for a diagnostic message. - - ``repr()`` on each raw value (rather than plain interpolation) keeps a - non-string ``path``, a non-int ``line``, or a ``None`` deliberately - distinguishable in the rendered text instead of silently coercing to a - misleading string. - """ + """Format one rejected path/line/side citation without coercing its types.""" return f"path={path!r} line={line!r} side={side!r}" def _nearby_changed_locations( locations: set[tuple[str, int, str]], path: Any, line: Any, *, limit: int = 5 ) -> str: - """Return a short hint of the closest real changed locations sharing ``path``. - - Scoped to ``locations`` entries whose path matches ``path`` exactly, then - sorted nearest-line-first (so a citation just one line off a real changed - line is obviously close, rather than buried in an unsorted dump) and - capped at ``limit`` entries to keep the GitHub Actions ``::error::`` - annotation this feeds into readable. Returns ``""`` — no hint — when - ``path`` is not a string or no changed location shares it; there is - nothing useful to compare against. - """ + """Return a bounded nearest-line hint for the rejected path.""" if not isinstance(path, str): return "" same_path = [location for location in locations if location[0] == path] @@ -483,7 +591,7 @@ def _nearby_changed_locations( def validate_substantive_verdict( verdict: dict[str, Any], diff: str, changed_paths: Sequence[str] = () ) -> None: - """Reject formal verdicts without changed-line and adversarial evidence.""" + """Reject formal verdicts without exact changed-line/adversarial evidence.""" decision = str(verdict.get("decision") or "").lower() if decision == "comment": return @@ -522,10 +630,11 @@ def validate_substantive_verdict( if not isinstance(residual_risk, str) or not residual_risk.strip(): raise NoemaModelOutputError("Noema adversarial validation requires residual_risk") probes = validation.get("probes") - all_changed_paths = set(changed_paths) or {path for path, _line, _side in locations} - required_probes = 2 if any(changed_file_is_material(path) for path in all_changed_paths) else 1 + required_probes = _required_probe_count(diff, changed_paths) if not isinstance(probes, list) or len(probes) < required_probes: - raise NoemaModelOutputError(f"Noema adversarial validation requires at least {required_probes} concrete probe(s)") + raise NoemaModelOutputError( + f"Noema adversarial validation requires at least {required_probes} concrete probe(s)" + ) confirmed: set[tuple[str, int, str]] = set() identities: set[tuple[Any, ...]] = set() @@ -548,8 +657,14 @@ def validate_substantive_verdict( raise NoemaModelOutputError(f"Noema adversarial probe {entry} requires {field}") outcome = probe.get("outcome") if outcome not in {"falsified", "confirmed"}: - raise NoemaModelOutputError(f"Noema adversarial probe {entry} outcome must be falsified or confirmed") - identity = (*location, probe["hypothesis"].strip().casefold(), probe["attack_or_counterexample"].strip().casefold()) + raise NoemaModelOutputError( + f"Noema adversarial probe {entry} outcome must be falsified or confirmed" + ) + identity = ( + *location, + probe["hypothesis"].strip().casefold(), + probe["attack_or_counterexample"].strip().casefold(), + ) if identity in identities: raise NoemaModelOutputError(f"Noema adversarial probe {entry} duplicates an earlier probe") identities.add(identity) @@ -565,7 +680,9 @@ def validate_substantive_verdict( if isinstance(finding, dict) } if not confirmed or not confirmed.intersection(finding_locations): - raise NoemaModelOutputError("Noema request_changes requires a confirmed probe on a published finding") + raise NoemaModelOutputError( + "Noema request_changes requires a confirmed probe on a published finding" + ) def truncate_text(text: str, limit: int) -> str: @@ -866,7 +983,77 @@ def _json_nesting_within_bound(text: str, start: int, max_depth: int) -> bool: MAX_JSON_NESTING_DEPTH = 100 +def _strip_trailing_commas_outside_strings(text: str) -> str: + """Remove only a genuine trailing comma after a complete JSON value. + + Missing-value forms such as ``[,]``, ``{,}``, ``[1,,]`` and ``{"a":,}`` + remain malformed and therefore fail closed. String contents are untouched. + """ + result: list[str] = [] + in_string = False + escaped = False + index = 0 + length = len(text) + while index < length: + char = text[index] + if in_string: + result.append(char) + if escaped: + escaped = False + elif char == "\\": + escaped = True + elif char == '"': + in_string = False + index += 1 + continue + if char == '"': + in_string = True + result.append(char) + index += 1 + continue + if char == ",": + lookahead = index + 1 + while lookahead < length and text[lookahead] in " \t\r\n": + lookahead += 1 + previous = len(result) - 1 + while previous >= 0 and result[previous] in " \t\r\n": + previous -= 1 + prior = result[previous] if previous >= 0 else "" + value_ending = prior in {'"', '}', ']'} or prior.isdigit() or prior in {'e', 'l'} + if lookahead < length and text[lookahead] in "}]" and value_ending: + index += 1 + continue + result.append(char) + index += 1 + return "".join(result) + + def extract_json_object(text: str) -> dict[str, Any]: + """Extract a JSON object, retrying once through a lossless local repair. + + Delegates to ``_extract_json_object_once``. If that fails, this makes + exactly one additional attempt against + ``_strip_trailing_commas_outside_strings(text)`` -- a deterministic, + semantically lossless fixup for the single well-known trailing-comma + malformation class -- before giving up. This is a local, non-network + second chance: it can resolve some malformed-JSON cases without ever + spending the bounded repair path's network round trip and wall-clock + budget, and it emits a ``::notice::`` (no raw content) when it is what + actually rescued the response, since that is itself useful repair-path + telemetry. It does not attempt to guess-repair any other malformation + shape; those still fail closed exactly as before. + """ + try: + return _extract_json_object_once(text) + except NoemaModelOutputError: + repaired = _strip_trailing_commas_outside_strings(text.strip()) + if repaired == text.strip(): + raise + verdict = _extract_json_object_once(repaired) + return verdict + + +def _extract_json_object_once(text: str) -> dict[str, Any]: """Extract a JSON object from a strict or lightly wrapped LLM response. Fails closed with ``NoemaModelOutputError`` — the same "no usable verdict" failure @@ -1108,6 +1295,24 @@ def decode_llm_response_body(raw_bytes: bytes) -> str: ) from exc +def _extract_served_model(raw: str) -> str | None: + """Return a bounded, scrubbed, single-line UTF-8-printable serving model id.""" + try: + data = json.loads(raw) + except (json.JSONDecodeError, TypeError, ValueError): + return None + if not isinstance(data, dict): + return None + served = data.get("model") + if not isinstance(served, str) or not served.strip(): + return None + scrubbed = scrub_sensitive_data(served.strip()) or "" + printable = scrubbed.encode("utf-8", errors="backslashreplace").decode("utf-8") + printable = "".join(" " if ord(char) < 32 or ord(char) == 127 else char for char in printable) + printable = " ".join(printable.split()) + return printable[:200] or None + + def _truthy_env(name: str) -> bool: """Return whether a process environment flag is an explicit truthy value.""" return os.environ.get(name, "").strip().lower() in {"1", "true", "yes", "on"} @@ -1195,47 +1400,6 @@ def reject_private_llm_url(api_url: str) -> None: raise ValueError("URL cannot target internal IP addresses") -@contextlib.contextmanager -def _repair_wall_clock_deadline(seconds: float): - """Interrupt the entire corrective attempt after ``seconds`` of wall time. - - ``urllib``'s timeout is a socket-operation timeout and can be extended by - trickling bytes. Required Noema Review runs on Linux, so ITIMER_REAL gives - the repair attempt one process-level wall-clock budget across open, read, - decode, and deterministic validation. An existing process alarm is not - overwritten; that condition fails closed instead. - """ - if seconds <= 0: - raise ValueError("repair wall-clock deadline must be positive") - if not hasattr(signal, "setitimer") or not hasattr(signal, "ITIMER_REAL"): - raise RuntimeError("repair wall-clock deadline requires POSIX setitimer support") - previous_remaining, previous_interval = signal.getitimer(signal.ITIMER_REAL) - if previous_remaining > 0 or previous_interval > 0: - raise RuntimeError("repair wall-clock deadline refused to overwrite an active process alarm") - previous_handler = signal.getsignal(signal.SIGALRM) - - def expire(_signum, _frame): - """Raise the typed deadline signal without reflecting response content.""" - raise NoemaRepairDeadlineExceeded( - f"Noema repair exceeded {seconds:g}-second absolute wall-clock deadline" - ) - - try: - signal.signal(signal.SIGALRM, expire) - except ValueError as exc: - raise RuntimeError("repair wall-clock deadline must run on the process main thread") from exc - signal.setitimer(signal.ITIMER_REAL, seconds) - try: - yield - finally: - signal.setitimer(signal.ITIMER_REAL, 0) - signal.signal(signal.SIGALRM, previous_handler) - - -class StaleHeadDuringRepairRetryError(RuntimeError): - """Raised when the PR head moves before ``call_llm``'s repair-retry request fires.""" - - def call_llm( repo: str, number: int, @@ -1245,94 +1409,40 @@ def call_llm( expected_head: str, review_context: str = "", changed_paths: Sequence[str] = (), - repair_error: str = "", - is_retry: bool = False, ) -> dict[str, Any]: - """Call the configured OpenAI-compatible LLM endpoint for a review verdict. - - ``expected_head`` is the same normalized (lowercase) SHA - ``inspect_and_review`` already checks before model work and before - publication. It is threaded through here so the one-time repair-retry - request below — fired only after the first attempt's verdict was - malformed — can also confirm the PR head has not moved before spending a - second, potentially multi-hour model call on a - review that ``inspect_and_review``'s own post-call stale-head check would - discard anyway once this function returns. See ``fetch_pr`` for the live - lookup and ``StaleHeadDuringRepairRetryError`` for how that stale - condition is reported distinctly to the caller. - - ``is_retry`` tracks retry state independently of ``repair_error``'s text: - several transport exceptions (a bare ``OSError``/``TimeoutError`` or - ``http.client.HTTPException`` raised with no message) stringify to an - empty string, so gating on ``repair_error``'s truthiness alone would let - an empty-message failure retry unboundedly instead of failing closed - after one attempt. + """Issue exactly one structured-output request through contextual-orchestrator. + + The gateway owns provider discovery, schema repair, candidate exclusion, + failover, and model timeouts. This caller therefore performs one request, + carries no fixed model wall-clock deadline or sampling temperature, and + fails closed if the gateway does not return a locally valid verdict. + Publication still performs a fresh exact-head check after model work. """ 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" + model = os.environ.get("NOEMA_LLM_MODEL", "").strip() or "orchestrator/free" if not api_url or not api_key: - raise RuntimeError("Noema LLM review unavailable: NOEMA_LLM_API_URL or NOEMA_LLM_API_KEY is not configured.") + raise RuntimeError( + "Noema LLM review unavailable: NOEMA_LLM_API_URL or NOEMA_LLM_API_KEY is not configured." + ) reject_private_llm_url(api_url) allowed_locations = [ {"path": path, "line": line, "side": side} for path, line, side in sorted(changed_diff_locations(diff)) ] - location_example = ( - allowed_locations[0] - if allowed_locations - else {"path": "path", "line": 0, "side": "RIGHT"} - ) - + location_example = allowed_locations[0] if allowed_locations else { + "path": "path", "line": 0, "side": "RIGHT" + } prompt = { "role": "user", "content": "\n".join( [ "You are Noema, an independent pull request reviewer for ContextualWisdomLab.", "Review the PR diff plus the additional changed-file and review-thread context for correctness, security, maintainability, and behavioral regressions.", - "Return only JSON with this shape:", - json.dumps( - { - "decision": "approve|request_changes|comment", - "summary": "...", - "reviewed_lines": [{**location_example, "analysis": "..."}], - "adversarial_validation": { - "status": "passed|failed", - "residual_risk": "...", - "probes": [ - { - **location_example, - "hypothesis": "...", - "attack_or_counterexample": "...", - "evidence": "observed or source-traced result", - "outcome": "falsified|confirmed", - } - ], - }, - "findings": [ - { - "severity": "high|medium|low", - "file": location_example["path"], - "line": location_example["line"], - "side": location_example["side"], - "message": "...", - } - ], - }, - separators=(",", ":"), - ), + "Return only JSON with the declared response_format schema.", "Every formal verdict must cite exact changed-side lines. APPROVE requires falsifying concrete regression hypotheses; source or test changes require at least two distinct probes and other changes require at least one. REQUEST_CHANGES requires a confirmed probe at a finding location.", "Use request_changes only for blocking, concrete issues. A generic no-issues statement is not review evidence.", - *( - [ - "Your prior verdict was rejected by the trusted validator: " - f"{repair_error or 'no diagnostic message was available'}", - "Return one corrected JSON verdict using only exact changed-side locations from the supplied diff.", - ] - if is_retry - else [] - ), f"Repository: {repo}", f"PR: #{number}", f"Title: {pr.get('title') or ''}", @@ -1347,7 +1457,9 @@ def call_llm( } payload = { "model": model, - "temperature": 0, + "response_format": _noema_verdict_response_format( + _required_probe_count(diff, changed_paths) + ), "messages": [ {"role": "system", "content": "Return strict JSON only. Do not include markdown."}, prompt, @@ -1363,82 +1475,85 @@ def call_llm( method="POST", ) opener = urllib.request.build_opener(NoRedirectHandler()) + attempt_started = time.monotonic() + active_phase = "connecting" + served_model: str | None = None try: - deadline_context = ( - _repair_wall_clock_deadline(NOEMA_REPAIR_DEADLINE_SECONDS) - if is_retry - else contextlib.nullcontext() - ) - with deadline_context: - with opener.open(request) as response: # nosec B310 - raw_bytes = response.read() - raw = decode_llm_response_body(raw_bytes) - content = extract_llm_message_content(raw) - verdict = extract_json_object(content) - decision = str(verdict.get("decision") or "").strip().lower() - if decision not in {"approve", "request_changes", "comment"}: - raise NoemaModelOutputError(f"Noema LLM returned unsupported decision: {decision!r}") - summary = verdict.get("summary") - if not isinstance(summary, str) or not summary.strip(): - raise NoemaModelOutputError("Noema LLM response did not contain a substantive summary") - findings = verdict.get("findings") - if not isinstance(findings, list) or any(not isinstance(finding, dict) for finding in findings): - raise NoemaModelOutputError("Noema LLM response findings must be a list of objects") - for finding in findings: - if ( - finding.get("severity") not in {"high", "medium", "low"} - or not isinstance(finding.get("file"), str) - or not finding["file"].strip() - or type(finding.get("line")) is not int - or finding["line"] <= 0 - or finding.get("side") not in {"RIGHT", "LEFT"} - or not isinstance(finding.get("message"), str) - or not finding["message"].strip() - ): - raise NoemaModelOutputError("Noema LLM response contained a malformed finding") - if decision == "request_changes" and not findings: - raise NoemaModelOutputError("Noema LLM request_changes response did not contain a substantive finding") - validate_substantive_verdict(verdict, diff, changed_paths) - except (RuntimeError, urllib.error.URLError, http.client.HTTPException, OSError) as exc: - current_failure = _stable_failure_diagnostic(exc) - if is_retry: - initial_failure = ( - scrub_sensitive_data(repair_error) - or "no diagnostic message was available" + with opener.open(request) as response: # nosec B310 + active_phase = "reading" + raw_bytes = response.read() + active_phase = "decoding" + raw = decode_llm_response_body(raw_bytes) + served_model = _extract_served_model(raw) + content = extract_llm_message_content(raw) + verdict = extract_json_object(content) + active_phase = "validating" + decision = str(verdict.get("decision") or "").strip().lower() + if decision not in {"approve", "request_changes", "comment"}: + raise NoemaModelOutputError( + f"Noema LLM returned unsupported decision: {decision!r}" ) - if isinstance(exc, NoemaModelOutputError): - raise NoemaModelOutputError( - "Noema model-output repair remained invalid; " - f"initial failure: {initial_failure}; repair failure: {current_failure}" - ) from None - if isinstance( - exc, (urllib.error.URLError, http.client.HTTPException, OSError) + summary = verdict.get("summary") + if not isinstance(summary, str) or not summary.strip(): + raise NoemaModelOutputError( + "Noema LLM response did not contain a substantive summary" + ) + findings = verdict.get("findings") + if not isinstance(findings, list) or any( + not isinstance(finding, dict) for finding in findings + ): + raise NoemaModelOutputError( + "Noema LLM response findings must be a list of objects" + ) + for finding in findings: + if ( + finding.get("severity") not in {"high", "medium", "low"} + or not isinstance(finding.get("file"), str) + or not finding["file"].strip() + or type(finding.get("line")) is not int + or finding["line"] <= 0 + or finding.get("side") not in {"RIGHT", "LEFT"} + or not isinstance(finding.get("message"), str) + or not finding["message"].strip() ): - raise NoemaTransportError( - "Noema bounded repair transport was exhausted; " - f"initial failure: {initial_failure}; repair failure: " - f"{type(exc).__name__}: {current_failure}" - ) from exc - raise RuntimeError( - "Noema repair failed closed; " - f"initial failure: {initial_failure}; repair failure: {current_failure}" - ) from exc - if str(fetch_pr(repo, number).get("headRefOid") or "").lower() != expected_head: - raise StaleHeadDuringRepairRetryError( - "Pull request head changed during review; stale before repair retry." - ) from exc - return call_llm( - repo, - number, - pr, - diff, - truncated, - expected_head, - review_context, - changed_paths, - current_failure, - is_retry=True, + raise NoemaModelOutputError( + "Noema LLM response contained a malformed finding" + ) + if decision == "request_changes" and not findings: + raise NoemaModelOutputError( + "Noema LLM request_changes response did not contain a substantive finding" + ) + validate_substantive_verdict(verdict, diff, changed_paths) + except (RuntimeError, urllib.error.URLError, http.client.HTTPException, OSError) as exc: + elapsed = time.monotonic() - attempt_started + current_failure = _stable_failure_diagnostic(exc) + model_note = served_model or "unknown" + print( + f"::warning::Noema gateway attempt outcome=failed phase={active_phase} " + f"duration={elapsed:.1f}s served_model={model_note}; " + "caller attempts=1 (gateway owns repair/failover)." + ) + suffix = ( + f"; caller attempts=1, duration={elapsed:.1f}s, " + f"phase={active_phase}, served_model={model_note}" ) + if isinstance(exc, NoemaModelOutputError): + raise NoemaModelOutputError( + f"Noema model output failed local validation: {current_failure}{suffix}" + ) from None + if isinstance(exc, (urllib.error.URLError, http.client.HTTPException, OSError)): + raise NoemaTransportError( + f"Noema gateway transport failed: {type(exc).__name__}: {current_failure}{suffix}" + ) from exc + raise RuntimeError( + f"Noema review failed closed: {current_failure}{suffix}" + ) from exc + elapsed = time.monotonic() - attempt_started + print( + f"::notice::Noema gateway attempt outcome=success phase={active_phase} " + f"duration={elapsed:.1f}s served_model={served_model or 'unknown'}; " + "caller attempts=1." + ) return verdict @@ -1527,8 +1642,7 @@ def inspect_and_review(repo: str, number: int, expected_head: str) -> int: """Inspect PR state and submit Noema's independent LLM review. ``expected_head`` is normalized defensively before the stale-head - comparisons below, and before the one ``call_llm`` performs on its own - repair-retry path (see ``StaleHeadDuringRepairRetryError``). The CLI and + comparisons below and the post-model publication check. The CLI and workflow require canonical lowercase SHA input so equivalent casing cannot split the workflow concurrency group. """ @@ -1557,11 +1671,7 @@ def inspect_and_review(repo: str, number: int, expected_head: str) -> int: changed_files = fetch_changed_files(repo, number) changed_paths = tuple(path for path, _status in changed_files) review_context = build_review_context(repo, number, pr, changed_files) - try: - verdict = call_llm(repo, number, pr, diff, truncated, expected_head, review_context, changed_paths) - except StaleHeadDuringRepairRetryError: - print("Pull request head changed during review; Noema review skipped before repair retry.") - return 0 + verdict = call_llm(repo, number, pr, diff, truncated, expected_head, review_context, changed_paths) current_pr = fetch_pr(repo, number) try: require_expected_head(current_pr, expected_head) diff --git a/tests/test_noema_model_output_edge_coverage.py b/tests/test_noema_model_output_edge_coverage.py new file mode 100644 index 0000000000..1965e6723b --- /dev/null +++ b/tests/test_noema_model_output_edge_coverage.py @@ -0,0 +1,31 @@ +"""Edge regressions for Noema model-output parsing and telemetry helpers.""" + +from __future__ import annotations + +from scripts.ci.noema_review_gate import ( + _extract_served_model, + _strip_trailing_commas_outside_strings, + extract_json_object, +) + + +def test_trailing_comma_stripper_preserves_escaped_string_content() -> None: + """Quote/escape state must preserve backslashes and commas inside strings.""" + source = '{"value":"x\\\\y,",}' + assert _strip_trailing_commas_outside_strings(source) == '{"value":"x\\\\y,"}' + + +def test_trailing_comma_stripper_handles_whitespace_before_comma() -> None: + """Whitespace before a structural trailing comma must not hide the prior value.""" + source = '{"value": 1 , }' + assert _strip_trailing_commas_outside_strings(source) == '{"value": 1 }' + + +def test_extract_json_object_recovers_only_lossless_trailing_comma() -> None: + """The local second chance must recover a syntactically trailing comma.""" + assert extract_json_object('{"ok": true,}') == {"ok": True} + + +def test_extract_served_model_rejects_malformed_json() -> None: + """Malformed response metadata must never fabricate a serving-model identity.""" + assert _extract_served_model("not-json") is None diff --git a/tests/test_noema_model_output_failure_classification.py b/tests/test_noema_model_output_failure_classification.py index 82305a6533..4038fc5dc3 100644 --- a/tests/test_noema_model_output_failure_classification.py +++ b/tests/test_noema_model_output_failure_classification.py @@ -65,281 +65,20 @@ def test_invalid_probe_outcome_is_typed_model_output_failure() -> None: -def test_bounded_repair_preserves_initial_schema_and_transport_evidence(monkeypatch) -> None: - """A malformed verdict followed by 502 keeps both typed evidence classes.""" - import json - import urllib.error - - monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") - monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") - head_sha = "a" * 40 - requests: list[tuple[object, dict]] = [] - - class Response: - def __enter__(self): - return self - - def __exit__(self, *_args): - return None - - def read(self): - return json.dumps( - {"choices": [{"message": {"content": json.dumps(_verdict())}}]} - ).encode() - - def open_response(_opener, request, **kwargs): - requests.append((request, kwargs)) - if len(requests) == 1: - return Response() - raise urllib.error.HTTPError(request.full_url, 502, "Bad Gateway", {}, None) - - monkeypatch.setattr(gate.urllib.request.OpenerDirector, "open", open_response) - monkeypatch.setattr( - gate, - "fetch_pr", - lambda _repo, _number: {"headRefOid": head_sha}, - ) - with pytest.raises(gate.NoemaTransportError) as exc_info: - gate.call_llm( - "owner/repo", - 7, - {"title": "test", "headRefOid": head_sha}, - DIFF, - False, - head_sha, - changed_paths=("README.md",), - ) - - message = str(exc_info.value) - assert "outcome must be falsified or confirmed" in message - assert "HTTPError" in message - assert "502" in message - assert len(requests) == 2 - assert requests[0][1] == {} - assert requests[1][1] == {} - - -def test_repeated_model_output_failure_remains_typed(monkeypatch) -> None: - """A second malformed verdict fails closed as model-output evidence.""" - import json - - monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") - monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") - head_sha = "b" * 40 - - class Response: - def __enter__(self): - return self - - def __exit__(self, *_args): - return None - - def read(self): - return json.dumps( - {"choices": [{"message": {"content": json.dumps(_verdict())}}]} - ).encode() - - monkeypatch.setattr( - gate.urllib.request.OpenerDirector, - "open", - lambda *_args, **_kwargs: Response(), - ) - monkeypatch.setattr( - gate, - "fetch_pr", - lambda _repo, _number: {"headRefOid": head_sha}, - ) - with pytest.raises(gate.NoemaModelOutputError) as exc_info: - gate.call_llm( - "owner/repo", - 7, - {"title": "test", "headRefOid": head_sha}, - DIFF, - False, - head_sha, - changed_paths=("README.md",), - ) - - assert "initial failure" in str(exc_info.value) - assert "repair failure" in str(exc_info.value) - - - -def test_total_repair_wall_clock_deadline_interrupts_slow_read(monkeypatch) -> None: - """Trickling/slow response activity cannot extend the one repair budget.""" - import json - import signal - import time - - if not hasattr(signal, "setitimer"): - pytest.skip("POSIX process timer is required by the Linux review runner") - - monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") - monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") - monkeypatch.setattr(gate, "NOEMA_REPAIR_DEADLINE_SECONDS", 0.05) - head_sha = "d" * 40 - calls = 0 - - class FirstResponse: - def __enter__(self): - return self - - def __exit__(self, *_args): - return None - - def read(self): - return json.dumps( - {"choices": [{"message": {"content": json.dumps(_verdict())}}]} - ).encode() - - class SlowRepairResponse: - def __enter__(self): - return self - - def __exit__(self, *_args): - return None - def read(self): - time.sleep(2) - return b"{}" - - def open_response(_opener, _request, **kwargs): - nonlocal calls - calls += 1 - assert kwargs == {} - return FirstResponse() if calls == 1 else SlowRepairResponse() - - monkeypatch.setattr(gate.urllib.request.OpenerDirector, "open", open_response) - monkeypatch.setattr(gate, "fetch_pr", lambda _repo, _number: {"headRefOid": head_sha}) - - started = time.monotonic() - with pytest.raises(gate.NoemaTransportError) as exc_info: - gate.call_llm( - "owner/repo", - 7, - {"title": "test", "headRefOid": head_sha}, - DIFF, - False, - head_sha, - changed_paths=("README.md",), - ) - elapsed = time.monotonic() - started - - message = str(exc_info.value) - assert "outcome must be falsified or confirmed" in message - assert "NoemaRepairDeadlineExceeded" in message - assert "wall-clock deadline" in message - assert elapsed < 1.0 - assert calls == 2 - assert signal.getitimer(signal.ITIMER_REAL)[0] == 0 - - - -def test_repair_wall_clock_deadline_defensive_fail_closed_paths(monkeypatch) -> None: - """Invalid budgets/platform state fail closed instead of weakening the bound.""" - import signal - - with pytest.raises(ValueError, match="must be positive"): - with gate._repair_wall_clock_deadline(0): - pass - - if not hasattr(signal, "setitimer"): - pytest.skip("remaining cases require POSIX setitimer") - - monkeypatch.delattr(gate.signal, "setitimer") - with pytest.raises(RuntimeError, match="requires POSIX setitimer support"): - with gate._repair_wall_clock_deadline(1): - pass - - -def test_repair_wall_clock_deadline_refuses_existing_process_alarm() -> None: - """Noema never overwrites another caller's active process alarm.""" - import signal - - if not hasattr(signal, "setitimer"): - pytest.skip("POSIX process timer is required by the Linux review runner") - signal.setitimer(signal.ITIMER_REAL, 30) - try: - with pytest.raises(RuntimeError, match="refused to overwrite"): - with gate._repair_wall_clock_deadline(1): - pass - finally: - signal.setitimer(signal.ITIMER_REAL, 0) - - -def test_repair_wall_clock_deadline_rejects_non_main_thread_signal_context(monkeypatch) -> None: - """A signal handler that cannot be installed fails closed before any timer starts.""" - import signal - - if not hasattr(signal, "setitimer"): - pytest.skip("POSIX process timer is required by the Linux review runner") - - def reject_signal(*_args, **_kwargs): - raise ValueError("signal only works in main thread") - - monkeypatch.setattr(gate.signal, "signal", reject_signal) - with pytest.raises(RuntimeError, match="process main thread"): - with gate._repair_wall_clock_deadline(1): - pass - assert signal.getitimer(signal.ITIMER_REAL)[0] == 0 - - -def test_repair_unexpected_runtime_failure_preserves_initial_model_evidence(monkeypatch) -> None: - """Unexpected corrective parser/runtime failures keep the first trusted diagnostic.""" - import json - - monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") - monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") - head_sha = "e" * 40 - - class Response: - def __enter__(self): - return self - - def __exit__(self, *_args): - return None - - def read(self): - return json.dumps( - {"choices": [{"message": {"content": json.dumps(_verdict())}}]} - ).encode() - - monkeypatch.setattr( - gate.urllib.request.OpenerDirector, - "open", - lambda *_args, **_kwargs: Response(), - ) - monkeypatch.setattr(gate, "fetch_pr", lambda _repo, _number: {"headRefOid": head_sha}) - original_decode = gate.decode_llm_response_body - decode_calls = 0 - def decode_once_then_fail(raw_bytes): - nonlocal decode_calls - decode_calls += 1 - if decode_calls == 2: - raise RuntimeError("repair parser invariant failed") - return original_decode(raw_bytes) - monkeypatch.setattr(gate, "decode_llm_response_body", decode_once_then_fail) - with pytest.raises(RuntimeError) as exc_info: - gate.call_llm( - "owner/repo", - 7, - {"title": "test", "headRefOid": head_sha}, - DIFF, - False, - head_sha, - changed_paths=("README.md",), - ) - message = str(exc_info.value) - assert "Noema repair failed closed" in message - assert "outcome must be falsified or confirmed" in message - assert "repair parser invariant failed" in message - assert decode_calls == 2 + + + + + + + @@ -351,54 +90,6 @@ def test_unparseable_diff_remains_source_evidence() -> None: assert "parseable changed-line evidence" in str(exc_info.value) -def test_model_sentinel_never_reaches_repair_prompt_or_final_diagnostic(monkeypatch) -> None: - """Model-controlled invalid values are redacted while the defect class stays actionable.""" - import json - - sentinel = "MODEL_SENTINEL_DO_NOT_REFLECT" - monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") - monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") - head_sha = "e" * 40 - requests = [] - - class Response: - def __enter__(self): - return self - - def __exit__(self, *_args): - return None - - def read(self): - return json.dumps( - {"choices": [{"message": {"content": json.dumps({"decision": sentinel})}}]} - ).encode() - - def open_response(_opener, request, **kwargs): - assert kwargs == {} - requests.append(request) - return Response() - - monkeypatch.setattr(gate.urllib.request.OpenerDirector, "open", open_response) - monkeypatch.setattr(gate, "fetch_pr", lambda _repo, _number: {"headRefOid": head_sha}) - - with pytest.raises(gate.NoemaModelOutputError) as exc_info: - gate.call_llm( - "owner/repo", - 7, - {"title": "test", "headRefOid": head_sha}, - DIFF, - False, - head_sha, - changed_paths=("README.md",), - ) - - assert len(requests) == 2 - repair_payload = requests[1].data.decode("utf-8") - assert sentinel not in repair_payload - assert "Noema LLM returned unsupported decision" in repair_payload - assert sentinel not in str(exc_info.value) - assert "Noema LLM returned unsupported decision" in str(exc_info.value) - assert exc_info.value.__cause__ is None def test_stable_failure_diagnostic_preserves_trusted_structure_and_redacts_values() -> None: @@ -418,43 +109,3 @@ def test_stable_failure_diagnostic_preserves_trusted_structure_and_redacts_value gate.NoemaModelOutputError("secret-ish model text") ) == "model-output-contract-invalid" assert gate._stable_failure_diagnostic(TimeoutError()) == "TimeoutError" - - -def test_repair_deadline_rejects_nonpositive_budget() -> None: - with pytest.raises(ValueError, match="must be positive"): - with gate._repair_wall_clock_deadline(0): - pass - - -def test_repair_deadline_requires_setitimer(monkeypatch) -> None: - monkeypatch.delattr(gate.signal, "setitimer") - with pytest.raises(RuntimeError, match="requires POSIX"): - with gate._repair_wall_clock_deadline(1): - pass - - -def test_repair_deadline_requires_itimer_real(monkeypatch) -> None: - monkeypatch.delattr(gate.signal, "ITIMER_REAL") - with pytest.raises(RuntimeError, match="requires POSIX"): - with gate._repair_wall_clock_deadline(1): - pass - - -@pytest.mark.parametrize("timer_state", [(1.0, 0.0), (0.0, 1.0)]) -def test_repair_deadline_refuses_existing_process_alarm(monkeypatch, timer_state) -> None: - monkeypatch.setattr(gate.signal, "getitimer", lambda _which: timer_state) - with pytest.raises(RuntimeError, match="active process alarm"): - with gate._repair_wall_clock_deadline(1): - pass - - -def test_repair_deadline_requires_main_thread_signal_registration(monkeypatch) -> None: - monkeypatch.setattr(gate.signal, "getitimer", lambda _which: (0.0, 0.0)) - - def reject_signal(*_args): - raise ValueError("signal only works in main thread") - - monkeypatch.setattr(gate.signal, "signal", reject_signal) - with pytest.raises(RuntimeError, match="process main thread"): - with gate._repair_wall_clock_deadline(1): - pass diff --git a/tests/test_noema_repair_attempt_telemetry.py b/tests/test_noema_repair_attempt_telemetry.py new file mode 100644 index 0000000000..8485305698 --- /dev/null +++ b/tests/test_noema_repair_attempt_telemetry.py @@ -0,0 +1,214 @@ +"""Exact contracts for Noema's single gateway request and passive telemetry.""" + +import json + +import pytest + +from scripts.ci import noema_review_gate as gate + + +DIFF = """diff --git a/README.md b/README.md +index 1111111..2222222 100644 +--- a/README.md ++++ b/README.md +@@ -1 +1 @@ +-old ++new +""" + + +def _verdict() -> dict: + return { + "decision": "approve", + "summary": "Reviewed the exact changed line.", + "reviewed_lines": [{"path": "README.md", "line": 1, "side": "RIGHT", "analysis": "Bounded replacement."}], + "adversarial_validation": { + "status": "passed", + "residual_risk": "No additional risk identified.", + "probes": [{ + "path": "README.md", "line": 1, "side": "RIGHT", + "hypothesis": "The replacement could be wrong.", + "attack_or_counterexample": "Inspect the exact changed line.", + "evidence": "The new value is present at the cited line.", + "outcome": "falsified", + }], + }, + "findings": [], + } + + +def _configure(monkeypatch, raw: bytes): + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") + requests = [] + + class Response: + def __enter__(self): return self + def __exit__(self, *_args): return None + def read(self): return raw + + def open_response(_opener, request, **kwargs): + requests.append(request) + assert kwargs == {} + return Response() + + monkeypatch.setattr(gate.urllib.request.OpenerDirector, "open", open_response) + return requests + + +def test_success_uses_one_request_and_one_phase_annotation(monkeypatch, capsys) -> None: + raw = json.dumps({"model": "provider/model", "choices": [{"message": {"content": json.dumps(_verdict())}}]}).encode() + requests = _configure(monkeypatch, raw) + verdict = gate.call_llm("owner/repo", 7, {"title": "t", "headRefOid": "a" * 40}, DIFF, False, "a" * 40, changed_paths=("README.md",)) + assert verdict["decision"] == "approve" + assert len(requests) == 1 + output = capsys.readouterr().out + assert output.count("::notice::Noema gateway attempt") == 1 + assert "phase=validating" in output + assert "caller attempts=1" in output + + +def test_malformed_output_fails_closed_without_caller_retry(monkeypatch, capsys) -> None: + raw = json.dumps({"model": "provider/model", "choices": [{"message": {"content": "not-json"}}]}).encode() + requests = _configure(monkeypatch, raw) + with pytest.raises(gate.NoemaModelOutputError, match="caller attempts=1"): + gate.call_llm("owner/repo", 7, {"title": "t", "headRefOid": "b" * 40}, DIFF, False, "b" * 40, changed_paths=("README.md",)) + assert len(requests) == 1 + output = capsys.readouterr().out + assert output.count("::warning::Noema gateway attempt") == 1 + + +def test_served_model_is_annotation_safe() -> None: + raw = json.dumps({"model": "bad\r\n::error::boom\u0000\ud800"}) + value = gate._extract_served_model(raw) + assert value is not None + assert "\r" not in value and "\n" not in value and "\x00" not in value + assert "\\ud800" in value + assert len(value) <= 200 + + +@pytest.mark.parametrize("text", ["[,]", "{,}", "[1,,]", '{"a":,}']) +def test_local_json_repair_never_fabricates_missing_values(text: str) -> None: + assert gate._strip_trailing_commas_outside_strings(text) == text + + +@pytest.mark.parametrize( + ("text", "expected"), + [ + ('{"a":"x",}', '{"a":"x"}'), + ('{"a":1,}', '{"a":1}'), + ('{"a":true,}', '{"a":true}'), + ('{"a":null,}', '{"a":null}'), + ('{"a":{},}', '{"a":{}}'), + ('{"a":[],}', '{"a":[]}'), + ('["x",]', '["x"]'), + ('[1,]', '[1]'), + ], +) +def test_local_json_repair_accepts_only_complete_value_trailing_commas(text: str, expected: str) -> None: + assert gate._strip_trailing_commas_outside_strings(text) == expected + + +def _single_request_transport(monkeypatch, *, raw=None, open_error=None, read_error=None): + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") + calls = [] + + class Response: + def __enter__(self): + return self + + def __exit__(self, *_args): + return None + + def read(self): + if read_error is not None: + raise read_error + assert raw is not None + return raw + + def open_response(_opener, request, **kwargs): + calls.append(request) + assert kwargs == {} + if open_error is not None: + raise open_error(request) if callable(open_error) else open_error + return Response() + + monkeypatch.setattr(gate.urllib.request.OpenerDirector, "open", open_response) + return calls + + +def _invoke_once(monkeypatch, **transport): + calls = _single_request_transport(monkeypatch, **transport) + kwargs = dict( + repo="owner/repo", + number=7, + pr={"title": "t", "headRefOid": "c" * 40}, + diff=DIFF, + truncated=False, + expected_head="c" * 40, + changed_paths=("README.md",), + ) + return calls, kwargs + + +def test_malformed_gateway_envelope_is_one_request_fail_closed(monkeypatch) -> None: + calls, kwargs = _invoke_once(monkeypatch, raw=b"[]") + with pytest.raises(gate.NoemaModelOutputError, match="caller attempts=1"): + gate.call_llm(**kwargs) + assert len(calls) == 1 + + +def test_invalid_utf8_is_one_request_fail_closed(monkeypatch) -> None: + calls, kwargs = _invoke_once(monkeypatch, raw=b"invalid: \x80\x81\xfe") + with pytest.raises(gate.NoemaModelOutputError, match="caller attempts=1"): + gate.call_llm(**kwargs) + assert len(calls) == 1 + + +@pytest.mark.parametrize( + "failure", + [ + lambda request: gate.urllib.error.HTTPError(request.full_url, 502, "Bad Gateway", {}, None), + OSError("socket timeout"), + ], +) +def test_connect_failures_are_one_request_and_typed(monkeypatch, failure) -> None: + calls, kwargs = _invoke_once(monkeypatch, open_error=failure) + with pytest.raises(gate.NoemaTransportError, match="caller attempts=1"): + gate.call_llm(**kwargs) + assert len(calls) == 1 + + +def test_truncated_read_is_one_request_and_typed(monkeypatch) -> None: + calls, kwargs = _invoke_once( + monkeypatch, read_error=gate.http.client.IncompleteRead(b"partial", 10) + ) + with pytest.raises(gate.NoemaTransportError, match="caller attempts=1"): + gate.call_llm(**kwargs) + assert len(calls) == 1 + + +def test_malformed_verdict_json_is_not_retried(monkeypatch) -> None: + raw = json.dumps({"model": "provider/model", "choices": [{"message": {"content": "{bad"}}]}).encode() + calls, kwargs = _invoke_once(monkeypatch, raw=raw) + with pytest.raises(gate.NoemaModelOutputError, match="caller attempts=1"): + gate.call_llm(**kwargs) + assert len(calls) == 1 + + +def test_rejected_changed_line_verdict_is_not_retried(monkeypatch) -> None: + verdict = _verdict() + verdict["decision"] = "request_changes" + verdict["findings"] = [{ + "severity": "high", + "file": "README.md", + "line": 99, + "side": "RIGHT", + "message": "Outside the changed hunk.", + }] + raw = json.dumps({"model": "provider/model", "choices": [{"message": {"content": json.dumps(verdict)}}]}).encode() + calls, kwargs = _invoke_once(monkeypatch, raw=raw) + with pytest.raises(gate.NoemaModelOutputError, match="caller attempts=1"): + gate.call_llm(**kwargs) + assert len(calls) == 1 diff --git a/tests/test_noema_repair_deadline_alarm_safety.py b/tests/test_noema_repair_deadline_alarm_safety.py deleted file mode 100644 index 11f5f9569f..0000000000 --- a/tests/test_noema_repair_deadline_alarm_safety.py +++ /dev/null @@ -1,25 +0,0 @@ -"""Regression coverage for Noema repair wall-clock alarm ownership.""" - -import pytest - -from scripts.ci import noema_review_gate as gate - - -def test_repair_deadline_refuses_to_clobber_an_existing_process_alarm(monkeypatch) -> None: - """A repair deadline must fail closed before replacing another alarm owner.""" - monkeypatch.setattr(gate.signal, "getitimer", lambda _kind: (5.0, 0.0)) - set_calls: list[tuple[object, ...]] = [] - monkeypatch.setattr( - gate.signal, - "setitimer", - lambda *args: set_calls.append(args), - ) - - with pytest.raises( - RuntimeError, - match="refused to overwrite an active process alarm", - ): - with gate._repair_wall_clock_deadline(0.05): - pytest.fail("deadline context must not run while another alarm is active") - - assert set_calls == [] diff --git a/tests/test_noema_repair_has_no_fixed_wall_clock_deadline.py b/tests/test_noema_repair_has_no_fixed_wall_clock_deadline.py new file mode 100644 index 0000000000..9122bdddfd --- /dev/null +++ b/tests/test_noema_repair_has_no_fixed_wall_clock_deadline.py @@ -0,0 +1,30 @@ +"""Fail-closed contracts for Noema model-call policy ownership.""" + +from pathlib import Path + + +_SOURCE = Path("scripts/ci/noema_review_gate.py") + + +def test_noema_has_no_repository_fixed_wall_clock_deadline() -> None: + """Keep model inference free of caller-authored elapsed-time termination.""" + source = _SOURCE.read_text(encoding="utf-8") + assert "NOEMA_REPAIR_DEADLINE_SECONDS" not in source + assert "_repair_wall_clock_deadline(" not in source + assert "NoemaRepairDeadlineExceeded" not in source + assert "signal.setitimer" not in source + + +def test_noema_has_no_caller_authored_model_retry() -> None: + """Malformed/transport evidence fails closed instead of authorizing another inference.""" + source = _SOURCE.read_text(encoding="utf-8") + assert "is_retry" not in source + assert "repair_error" not in source + assert "StaleHeadDuringRepairRetryError" not in source + assert "return call_llm(" not in source + + +def test_noema_does_not_assign_a_sampling_temperature() -> None: + """Noema declares output structure but delegates sampling policy to contextual-orchestrator.""" + source = _SOURCE.read_text(encoding="utf-8") + assert '"temperature"' not in source diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index c2bf379d40..ba65ba6b1f 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -1167,549 +1167,33 @@ def test_decode_llm_response_body_fails_closed_on_invalid_utf8(): assert f"sha256={fingerprint}" in message -def test_call_llm_repairs_one_malformed_envelope_before_failing_closed(monkeypatch): - """The envelope-level fail-closed path integrates with the existing - verdict-repair boundary: a malformed gateway reply gets one repair-retry - request before failing closed, exactly like a malformed verdict JSON - already does.""" - monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") - monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") - bodies = iter( - ( - "not-json-at-all", - json.dumps( - { - "choices": [ - { - "message": { - "content": json.dumps( - {"decision": "comment", "summary": "Recovered", "findings": []} - ) - } - } - ] - } - ), - ) - ) - requests = [] - - class Response: - def __enter__(self): - return self - - def __exit__(self, *args): - return None - - def read(self): - return next(bodies).encode() - - def open_response(_opener, request, **_kwargs): - requests.append(json.loads(request.data)) - return Response() - - monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) - monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) - - verdict = noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") - - assert verdict["summary"] == "Recovered" - assert len(requests) == 2 - assert "prior verdict was rejected" in requests[1]["messages"][1]["content"] - - -def test_call_llm_skips_repair_retry_when_head_moves_before_it_fires(monkeypatch): - """CodeRabbit finding on PR #1507: ``expected_head`` is checked before - model work and before publication, but the one-time repair-retry request - inside ``call_llm`` used to fire unconditionally on a malformed first - verdict, even if the PR head had already moved. That burns a second, - potentially multi-hour model call on a review - ``inspect_and_review``'s own post-call stale-head check would discard - anyway. ``call_llm`` must instead re-check the live head via ``fetch_pr`` - before the retry request and fail closed with - ``StaleHeadDuringRepairRetryError`` — cleanly, not a crash — issuing only - the one doomed first request.""" - monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") - monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") - open_calls = [] - - class Response: - def __enter__(self): - return self - - def __exit__(self, *args): - return None - - def read(self): - # Malformed: missing "choices" triggers call_llm's fail-closed - # RuntimeError path on the very first attempt. - return b"[]" - - def open_response(_opener, request, **_kwargs): - open_calls.append(request) - return Response() - - monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) - # The live PR head has moved on since the trigger fetched "head". - monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr(headRefOid="new")) - - with pytest.raises(noema.StaleHeadDuringRepairRetryError, match="stale before repair retry"): - noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") - # Only the first, already-doomed request was made — the repair-retry - # request never fired once the live head no longer matched. - assert len(open_calls) == 1 - - -def test_call_llm_still_repairs_once_when_head_has_not_moved(monkeypatch): - """A matching live head must not block the existing one-time repair - retry — this is a narrow addition to the existing repair boundary, not a - behavior change for the unstale case.""" - monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") - monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") - contents = iter( - ( - "not-json-at-all", - json.dumps({"decision": "comment", "summary": "Recovered", "findings": []}), - ) - ) - open_calls = [] - - class Response: - def __enter__(self): - return self - - def __exit__(self, *args): - return None - - def read(self): - content = next(contents) - return json.dumps({"choices": [{"message": {"content": content}}]}).encode() - - def open_response(_opener, request, **_kwargs): - open_calls.append(request) - return Response() - - monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) - monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr(headRefOid="head")) - - verdict = noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") - - assert verdict["summary"] == "Recovered" - assert len(open_calls) == 2 - - -def test_inspect_and_review_reports_stale_before_repair_retry_cleanly(monkeypatch): - """``inspect_and_review`` must treat a stale-during-repair-retry signal - exactly like its own pre-model and pre-publication stale checks: a clean - skip (return 0), never an unhandled exception or a published review.""" - head = "a" * 40 - pr = make_pr(headRefOid=head) - monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: pr) - monkeypatch.setattr(noema, "current_actor", lambda: "noema") - monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) - monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [("tool.py", "modified")]) - monkeypatch.setattr(noema, "build_review_context", lambda repo, number, value, changed_files=None: "context") - - def fake_call_llm(*args, **kwargs): - raise noema.StaleHeadDuringRepairRetryError( - "Pull request head changed during review; stale before repair retry." - ) - - monkeypatch.setattr(noema, "call_llm", fake_call_llm) - monkeypatch.setattr( - noema, - "submit_review", - lambda *args, **kwargs: pytest.fail("stale-during-repair verdict must not publish"), - ) - assert noema.inspect_and_review("owner/repo", 7, head) == 0 - - -def test_call_llm_fails_closed_after_repeated_malformed_envelope(monkeypatch): - """Two consecutive malformed envelopes must produce a single clean - top-level RuntimeError diagnostic, never an unhandled traceback — but - the first still gets a repair-retry request like a malformed verdict - would.""" - monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") - monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") - open_calls = [] - - class Response: - def __enter__(self): - return self - - def __exit__(self, *args): - return None - - def read(self): - # Top-level JSON is a bare list — no "choices" object to speak of. - return b"[]" - - def open_response(_opener, request, **_kwargs): - open_calls.append(request) - return Response() - - monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) - monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) - - with pytest.raises(RuntimeError, match="response body was not a JSON object"): - noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") - assert len(open_calls) == 2 - - -def test_call_llm_fails_closed_after_repeated_invalid_utf8_response(monkeypatch): - """Devin Review bug finding on PR #1507 round 3: a gateway reply - containing invalid UTF-8 bytes used to raise UnicodeDecodeError before - extract_llm_message_content or the verdict-JSON repair boundary ever - ran, crashing the required review check with an unhandled traceback. - It must instead integrate with the existing repair-retry boundary - exactly like a malformed JSON envelope already does: one repair-retry - request, then a single clean top-level RuntimeError when the retry - response is *also* invalid UTF-8 — never an unhandled traceback.""" - monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") - monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") - open_calls = [] - - class Response: - def __enter__(self): - return self - - def __exit__(self, *args): - return None - - def read(self): - # Invalid UTF-8: a lone continuation byte with no lead byte. - return b"not utf-8 at all: \x80\x81\xfe" - - def open_response(_opener, request, **_kwargs): - open_calls.append(request) - return Response() - - monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) - monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) - - with pytest.raises(RuntimeError, match="response body was not valid UTF-8"): - noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") - # One initial request plus exactly one repair-retry request — not an - # unbounded retry loop, and not a crash on the first attempt. - assert len(open_calls) == 2 - assert "prior verdict was rejected" in json.loads(open_calls[1].data)["messages"][1]["content"] - - -def test_call_llm_repairs_once_after_a_transport_error_then_succeeds(monkeypatch): - """A transport-level failure (e.g. a genuine HTTP 502 from the gateway) - must not crash the job with an unhandled traceback. - - Live incident (ContextualWisdomLab/naruon#1486): ``opener.open(request)`` - sat outside the surrounding try/except, which only guarded the - JSON-decode/validation step after a successful response. Any transport - exception (HTTPError, URLError) from the request itself propagated as an - unhandled traceback instead of getting the same one-time repair-retry the - malformed-verdict path already has. Widening the try to also cover the - request itself, and catching ``urllib.error.URLError`` alongside - ``RuntimeError``, integrates it with that existing boundary.""" - monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") - monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") - monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) - attempts = [] - class Response: - def __enter__(self): - return self - def __exit__(self, *args): - return None - def read(self): - return json.dumps( - { - "choices": [ - { - "message": { - "content": json.dumps( - {"decision": "comment", "summary": "Recovered", "findings": []} - ) - } - } - ] - } - ).encode() - def open_response(_opener, request, **_kwargs): - attempts.append(request) - if len(attempts) == 1: - raise noema.urllib.error.HTTPError(request.full_url, 502, "Bad Gateway", {}, None) - return Response() - monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) - verdict = noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") - assert verdict["summary"] == "Recovered" - assert len(attempts) == 2 -def test_call_llm_fails_closed_after_a_repeated_transport_error(monkeypatch): - """Two consecutive transport errors must produce a single clean - RuntimeError diagnostic, never an unhandled traceback -- the first still - gets a repair-retry request like any other recoverable failure would.""" - monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") - monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") - monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) - open_calls = [] - - def open_response(_opener, request, **_kwargs): - open_calls.append(request) - raise noema.urllib.error.HTTPError(request.full_url, 502, "Bad Gateway", {}, None) - monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) - with pytest.raises(RuntimeError, match="Bad Gateway"): - noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") - assert len(open_calls) == 2 -def test_call_llm_repairs_once_after_a_truncated_response_then_succeeds(monkeypatch): - """A truncated response body must not crash the job either. - ``response.read()`` can raise ``http.client.IncompleteRead`` when the - server closes the connection before delivering the full - ``Content-Length`` body. That exception is neither a ``RuntimeError`` - nor a ``urllib.error.URLError`` -- it is a plain ``http.client - .HTTPException`` -- so it slipped through the transport-error boundary - added for the HTTPError/URLError case and still crashed the required - check with an unhandled traceback.""" - monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") - monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") - monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) - attempts = [] - class TruncatedResponse: - def __enter__(self): - return self - def __exit__(self, *args): - return None - def read(self): - raise http.client.IncompleteRead(b"", 10) - class Response: - def __enter__(self): - return self - def __exit__(self, *args): - return None - def read(self): - return json.dumps( - { - "choices": [ - { - "message": { - "content": json.dumps( - {"decision": "comment", "summary": "Recovered", "findings": []} - ) - } - } - ] - } - ).encode() - def open_response(_opener, request, **_kwargs): - attempts.append(request) - if len(attempts) == 1: - return TruncatedResponse() - return Response() - monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) - verdict = noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") - assert verdict["summary"] == "Recovered" - assert len(attempts) == 2 -def test_call_llm_fails_closed_after_a_repeated_truncated_response(monkeypatch): - """Two consecutive truncated reads must produce a single clean - RuntimeError diagnostic, never an unhandled traceback -- the first still - gets a repair-retry request like any other recoverable failure would.""" - monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") - monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") - monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) - open_calls = [] - - class TruncatedResponse: - def __enter__(self): - return self - - def __exit__(self, *args): - return None - - def read(self): - raise http.client.IncompleteRead(b"", 10) - - def open_response(_opener, request, **_kwargs): - open_calls.append(request) - return TruncatedResponse() - - monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) - - with pytest.raises(RuntimeError, match="IncompleteRead"): - noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") - assert len(open_calls) == 2 - - -def test_call_llm_repairs_once_after_a_socket_timeout_then_succeeds(monkeypatch): - """A raw socket-level failure during the request/connect phase -- not - wrapped as a ``urllib.error.URLError`` -- must not crash the job either. - - ``opener.open(request)`` can raise a bare ``OSError`` subtype (e.g. a - ``TimeoutError``/``socket.timeout``, or a connection reset) directly from - the underlying ``http.client`` connection when the failure happens before - urllib gets a chance to wrap it as ``URLError``. This exercises the - ``OSError`` branch of the transport-failure boundary on a distinct - exception path from the ``http.client.HTTPException`` branch - (``IncompleteRead``, above) and the ``urllib.error.URLError`` branch - (``HTTPError``, further above).""" - monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") - monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") - monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) - attempts = [] - - class Response: - def __enter__(self): - return self - - def __exit__(self, *args): - return None - - def read(self): - return json.dumps( - { - "choices": [ - { - "message": { - "content": json.dumps( - {"decision": "comment", "summary": "Recovered", "findings": []} - ) - } - } - ] - } - ).encode() - - def open_response(_opener, request, **_kwargs): - attempts.append(request) - if len(attempts) == 1: - raise TimeoutError("timed out waiting for the gateway") - return Response() - - monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) - - verdict = noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") - - assert verdict["summary"] == "Recovered" - assert len(attempts) == 2 - - -def test_call_llm_fails_closed_after_a_repeated_socket_timeout(monkeypatch): - """Two consecutive raw socket timeouts must produce a single clean - RuntimeError diagnostic, never an unhandled traceback -- the first still - gets a repair-retry request like any other recoverable failure would.""" - monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") - monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") - monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) - open_calls = [] - - def open_response(_opener, request, **_kwargs): - open_calls.append(request) - raise TimeoutError("timed out waiting for the gateway") - - monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) - - with pytest.raises(RuntimeError, match="timed out waiting for the gateway"): - noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") - assert len(open_calls) == 2 - - -def test_call_llm_repairs_once_after_an_empty_message_transport_error_then_succeeds(monkeypatch): - """A transport exception whose ``str()`` is empty (a bare ``OSError()``/ - ``TimeoutError()``, or an ``http.client.HTTPException`` raised with no - message -- all of these stringify to ``''`` in practice) must still get - exactly one repair retry, the same as any other transport failure. - - Devin Review on #1566: gating the retry-vs-fail-closed decision on - ``repair_error``'s truthiness conflated "is this the second attempt" - with "does the caught exception have display text" -- an empty-message - failure on the first attempt would keep ``repair_error`` falsy on the - recursive call too, so the retry state was lost. ``is_retry`` now tracks - that state explicitly and independently of the exception's text.""" - monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") - monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") - monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) - attempts = [] - - class Response: - def __enter__(self): - return self - - def __exit__(self, *args): - return None - - def read(self): - return json.dumps( - { - "choices": [ - { - "message": { - "content": json.dumps( - {"decision": "comment", "summary": "Recovered", "findings": []} - ) - } - } - ] - } - ).encode() - - def open_response(_opener, request, **_kwargs): - attempts.append(request) - if len(attempts) == 1: - raise OSError() - return Response() - - monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) - - verdict = noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") - - assert verdict["summary"] == "Recovered" - assert len(attempts) == 2 - - -def test_call_llm_fails_closed_after_a_repeated_empty_message_transport_error(monkeypatch): - """Two consecutive empty-message transport failures must still fail - closed after exactly one repair retry, never retry unboundedly. - - Bounds the fixture at 6 open() calls so a regression that reintroduces - unbounded recursion fails this test fast with a clear AssertionError - instead of recursing until CPython's own recursion limit.""" - monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") - monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") - monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) - open_calls = [] - - def open_response(_opener, request, **_kwargs): - open_calls.append(request) - if len(open_calls) > 5: - raise AssertionError("call_llm retried more than once on an empty-message transport error") - raise OSError() - - monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) - - with pytest.raises(RuntimeError): - noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") - assert len(open_calls) == 2 - @pytest.mark.parametrize("choices", [{"a": 1}, 5]) def test_call_llm_fails_closed_on_wrong_shaped_gateway_choices(monkeypatch, choices): @@ -2330,41 +1814,6 @@ def read(self): noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") -def test_call_llm_repairs_one_malformed_json_response(monkeypatch): - """Ask once for corrected JSON before failing the required review closed.""" - monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") - monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") - contents = iter( - ( - '{"decision":"approve", trailing garbage not: "quoted}', - json.dumps({"decision": "comment", "summary": "Repaired JSON", "findings": []}), - ) - ) - requests = [] - - class Response: - def __enter__(self): - return self - - def __exit__(self, *args): - return None - - def read(self): - content = next(contents) - return json.dumps({"choices": [{"message": {"content": content}}]}).encode() - - def open_response(_opener, request, **_kwargs): - requests.append(json.loads(request.data)) - return Response() - - monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) - monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) - - verdict = noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") - - assert verdict["summary"] == "Repaired JSON" - assert len(requests) == 2 - assert "prior verdict was rejected" in requests[1]["messages"][1]["content"] @pytest.mark.parametrize("message", [[], {}, 0, " "]) @@ -2448,91 +1897,6 @@ def read(self): noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") -def test_call_llm_repairs_one_rejected_changed_line_verdict(monkeypatch): - monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") - monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") - diff = """--- a/tool.py -+++ b/tool.py -@@ -1 +1 @@ --old = True -+new = True -""" - invalid = { - "decision": "approve", - "summary": "Checked the replacement.", - "findings": [], - "reviewed_lines": [ - {"path": "tool.py", "line": 2, "side": "RIGHT", "analysis": "Checked."} - ], - "adversarial_validation": { - "status": "passed", - "residual_risk": "Callers were not executed.", - "probes": [], - }, - } - valid = { - **invalid, - "reviewed_lines": [ - {"path": "tool.py", "line": 1, "side": "RIGHT", "analysis": "Checked."} - ], - "adversarial_validation": { - "status": "passed", - "residual_risk": "Callers were not executed.", - "probes": [ - { - "path": "tool.py", - "line": 1, - "side": "RIGHT", - "hypothesis": "The assignment was removed.", - "attack_or_counterexample": "Inspect the added hunk line.", - "evidence": "The RIGHT-side assignment remains present.", - "outcome": "falsified", - }, - { - "path": "tool.py", - "line": 1, - "side": "RIGHT", - "hypothesis": "The value became false.", - "attack_or_counterexample": "Read the replacement literal.", - "evidence": "The literal is True.", - "outcome": "falsified", - }, - ], - }, - } - payloads = [] - - class Response: - def __init__(self, verdict): - self.verdict = verdict - - def __enter__(self): - return self - - def __exit__(self, *_args): - return False - - def read(self): - return json.dumps( - {"choices": [{"message": {"content": json.dumps(self.verdict)}}]} - ).encode() - - class Opener: - def open(self, request, timeout=None): - assert timeout is None - payloads.append(json.loads(request.data)) - return Response(invalid if len(payloads) == 1 else valid) - - monkeypatch.setattr(noema.urllib.request, "build_opener", lambda *_args: Opener()) - monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) - - assert noema.call_llm("owner/repo", 7, make_pr(), diff, False, "head")["decision"] == "approve" - assert len(payloads) == 2 - assert "trusted validator" in payloads[1]["messages"][1]["content"] - assert ( - '"reviewed_lines":[{"path":"tool.py","line":1,"side":"LEFT"' - in payloads[1]["messages"][1]["content"] - ) def test_noema_adr_forbids_fixed_model_inference_timeouts() -> None: