diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 643a57c5c5..5ab7e830f3 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -1319,32 +1319,75 @@ def _safe_model_identifier(value: Any) -> str | None: return candidate -def _extract_http_error_served_model(exc: urllib.error.HTTPError) -> str | None: - """Read a bounded gateway error envelope and return only its safe model id. +def _extract_http_error_telemetry(exc: urllib.error.HTTPError) -> dict[str, str | int]: + """Read bounded, allowlisted gateway failure telemetry without raw diagnostics. The response body is never returned or logged. Only the canonical - ``error.detail.model`` field is allowed; malformed, oversized, or unexpected - envelopes fail closed to an unknown model. + ``error.detail`` receipt fields are allowed; malformed, oversized, or + unexpected envelopes fail closed to no telemetry. """ try: raw_bytes = exc.read(MAX_HTTP_ERROR_BODY_BYTES + 1) - except (AttributeError, OSError, ValueError): - return None + except (AttributeError, OSError, ValueError, http.client.HTTPException): + return {} if len(raw_bytes) > MAX_HTTP_ERROR_BODY_BYTES: - return None + return {} try: payload = json.loads(raw_bytes.decode("utf-8")) except (UnicodeDecodeError, json.JSONDecodeError, TypeError, ValueError): - return None + return {} if not isinstance(payload, dict): - return None + return {} error = payload.get("error") if not isinstance(error, dict): - return None + return {} detail = error.get("detail") if not isinstance(detail, dict): - return None - return _safe_model_identifier(detail.get("model")) + return {} + telemetry: dict[str, str | int] = {} + model = _safe_model_identifier(detail.get("model")) + terminal_reason = _safe_model_identifier(detail.get("terminal_reason")) + attempts = detail.get("attempts") + if model is not None: + telemetry["served_model"] = model + if terminal_reason is not None: + telemetry["terminal_reason"] = terminal_reason + if isinstance(attempts, list) and attempts and len(attempts) <= 64: + last_attempt = attempts[-1] + if isinstance(last_attempt, dict): + provider_name = _safe_model_identifier(last_attempt.get("provider_name")) + phase = _safe_model_identifier(last_attempt.get("phase")) + attempt_number = last_attempt.get("attempt_number") + provider_status = last_attempt.get("provider_status") + if provider_name is not None: + telemetry["provider_name"] = provider_name + if phase is not None: + telemetry["upstream_phase"] = phase + if type(attempt_number) is int and 1 <= attempt_number <= 64: + telemetry["attempt_number"] = attempt_number + if type(provider_status) is int and 100 <= provider_status <= 599: + telemetry["upstream_status"] = provider_status + return telemetry + + +def _extract_http_error_served_model(exc: urllib.error.HTTPError) -> str | None: + """Return the safe served model from one bounded gateway error envelope.""" + model = _extract_http_error_telemetry(exc).get("served_model") + return model if isinstance(model, str) else None + + +def _format_gateway_error_telemetry(telemetry: dict[str, str | int]) -> str: + """Format only allowlisted scalar receipt fields for a public Actions log.""" + ordered_keys = ( + "provider_name", + "upstream_phase", + "attempt_number", + "upstream_status", + "terminal_reason", + ) + return " ".join( + f"{key}={telemetry[key]}" for key in ordered_keys if key in telemetry + ) def _bounded_allowed_locations_json(allowed_locations: Sequence[dict[str, Any]]) -> str: @@ -1590,20 +1633,26 @@ def call_llm( ) validate_substantive_verdict(verdict, diff, changed_paths) except (RuntimeError, urllib.error.URLError, http.client.HTTPException, OSError) as exc: + gateway_telemetry: dict[str, str | int] = {} if isinstance(exc, urllib.error.HTTPError): active_phase = "response_error" - served_model = _extract_http_error_served_model(exc) + gateway_telemetry = _extract_http_error_telemetry(exc) + model_value = gateway_telemetry.get("served_model") + served_model = model_value if isinstance(model_value, str) else None elapsed = time.monotonic() - attempt_started current_failure = _stable_failure_diagnostic(exc) model_note = served_model or "unknown" + gateway_note = _format_gateway_error_telemetry(gateway_telemetry) 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)." + + (f" gateway {gateway_note}" if gateway_note else "") ) suffix = ( f"; caller attempts=1, duration={elapsed:.1f}s, " f"phase={active_phase}, served_model={model_note}" + + (f", gateway {gateway_note}" if gateway_note else "") ) if isinstance(exc, NoemaModelOutputError): raise NoemaModelOutputError( diff --git a/scripts/ci/opencode_adversarial_receipts.py b/scripts/ci/opencode_adversarial_receipts.py index 9d97cccf7e..f0880d9d68 100644 --- a/scripts/ci/opencode_adversarial_receipts.py +++ b/scripts/ci/opencode_adversarial_receipts.py @@ -189,8 +189,6 @@ def collect_receipts( valid_lines = [ line for line in changed_lines if 1 <= line <= len(source_lines) ] - if not valid_lines: - valid_lines = [1] for line in select_bounded_lines(valid_lines, lines_per_file): digest = hashlib.sha256(source_lines[line - 1]).hexdigest() receipts.append(SourceLineReceipt(path=path, line=line, digest=digest)) diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index a0c27a51bb..ec45c228ac 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -1535,7 +1535,18 @@ def test_call_llm_reports_only_safe_model_from_bounded_http_error(monkeypatch, c body = json.dumps( { "error": { - "detail": {"model": "github_models/deepseek-v3", "secret": secret}, + "detail": { + "model": "github_models/deepseek-v3", + "terminal_reason": "eligible_candidates_exhausted", + "attempts": [{ + "provider_name": "nvidia_nim", + "phase": "connecting", + "attempt_number": 2, + "provider_status": 503, + "secret": secret, + }], + "secret": secret, + }, "message": secret, }, "arbitrary": secret, @@ -1559,6 +1570,11 @@ def open(self, request): assert "served_model=github_models/deepseek-v3" in output assert "phase=response_error" in diagnostic assert "served_model=github_models/deepseek-v3" in diagnostic + assert "provider_name=nvidia_nim" in output + assert "upstream_phase=connecting" in output + assert "attempt_number=2" in output + assert "upstream_status=503" in output + assert "terminal_reason=eligible_candidates_exhausted" in output assert secret not in output assert secret not in diagnostic @@ -1595,6 +1611,37 @@ def open(self, request): assert body.decode("utf-8", errors="ignore") not in output +def test_call_llm_http_error_incomplete_body_stays_a_transport_failure( + monkeypatch, capsys +): + """A truncated gateway error body cannot bypass the stable transport boundary.""" + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example.test/chat") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "secret") + + class BrokenBody: + def read(self, _limit): + raise noema.http.client.IncompleteRead(b'{"error":') + + def close(self): + return None + + class Opener: + def open(self, request): + raise noema.urllib.error.HTTPError( + request.full_url, 502, "Bad Gateway", {}, BrokenBody() + ) + + monkeypatch.setattr(noema.urllib.request, "build_opener", lambda *_args: Opener()) + + with pytest.raises(noema.NoemaTransportError, match="served_model=unknown"): + noema.call_llm("owner/repo", 1, make_pr(), "diff", False, "head") + + output = capsys.readouterr().out + assert "phase=response_error" in output + assert "served_model=unknown" in output + assert '{"error":' not in output + + def test_noema_redirect_handler_rejects_redirects(): """Noema must not follow redirects after validating the initial URL.""" handler = noema.NoRedirectHandler() diff --git a/tests/test_opencode_adversarial_receipts.py b/tests/test_opencode_adversarial_receipts.py index 9a0da62b2b..c9a7865288 100644 --- a/tests/test_opencode_adversarial_receipts.py +++ b/tests/test_opencode_adversarial_receipts.py @@ -156,6 +156,18 @@ def test_skips_deleted_unsafe_external_and_oversized_paths(tmp_path: Path): assert [(item.path, item.line) for item in found] == [("kept.py", 1)] +def test_skips_files_with_only_deleted_lines(tmp_path: Path): + """Receipts never fabricate line one when the diff has no changed-side line.""" + repo = initialized_repo(tmp_path) + source = repo / "deletion.py" + source.write_text("kept\nremoved\n", encoding="utf-8") + base_sha = commit_all(repo, "base") + source.write_text("kept\n", encoding="utf-8") + head_sha = commit_all(repo, "head") + + assert receipts.collect_receipts(repo, base_sha, head_sha, ["deletion.py"]) == [] + + def test_render_markdown_exposes_only_json_metadata_not_source_text(): """Model evidence receives exact receipt metadata without untrusted line text.""" receipt = receipts.SourceLineReceipt( @@ -258,35 +270,31 @@ def test_changed_line_and_selection_edges_are_deterministic( assert receipts.select_bounded_lines([1, 2, 3, 4], 3) == [1, 3, 4] -def test_receipt_collection_falls_back_to_first_line_and_honors_limits(tmp_path: Path): - """Metadata-only head deltas still bind a safe line and respect hard caps.""" +def test_receipt_collection_skips_unchanged_files_and_honors_limits(tmp_path: Path): + """Unchanged files yield no receipt and the global limit bounds changed lines.""" repo = initialized_repo(tmp_path) stable = repo / "stable.py" - marker = repo / "marker.txt" + changed = repo / "changed.py" stable.write_text("first\nsecond\n", encoding="utf-8") + changed.write_text("before one\nbefore two\n", encoding="utf-8") base_sha = commit_all(repo, "base") - marker.write_text("head changed elsewhere\n", encoding="utf-8") + changed.write_text("after one\nafter two\n", encoding="utf-8") head_sha = commit_all(repo, "head") assert receipts.collect_receipts( - repo, - base_sha, - head_sha, - ["stable.py"], - max_receipts=1, - ) == [ - receipts.SourceLineReceipt( - path="stable.py", - line=1, - digest=hashlib.sha256(b"first").hexdigest(), - ) - ] + repo, base_sha, head_sha, ["stable.py"], max_receipts=1 + ) == [] + bounded = receipts.collect_receipts( + repo, base_sha, head_sha, ["stable.py", "changed.py"], max_receipts=1 + ) + assert len(bounded) == 1 + assert bounded[0].path == "changed.py" assert ( receipts.collect_receipts( repo, base_sha, head_sha, - ["stable.py"], + ["stable.py", "changed.py"], lines_per_file=0, ) == []