diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index ce90b8bc84..af52927948 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -58,7 +58,10 @@ MAX_FILE_CONTEXT_CHARS = 4000 MAX_REVIEW_CONTEXT_CHARS = 24000 MAX_THREAD_BODY_CHARS = 1200 +MAX_ALLOWED_LOCATIONS_JSON_BYTES = 32 * 1024 +MAX_HTTP_ERROR_BODY_BYTES = 16 * 1024 DIFF_HUNK_RE = re.compile(r"^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@") +SAFE_MODEL_IDENTIFIER_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._:/@+-]{0,199}$") ORCHESTRATOR_LOOPBACK_HOSTS = frozenset({"127.0.0.1", "::1"}) ORCHESTRATOR_BASE_ENV = "CONTEXTUAL_ORCHESTRATOR_BASE_URL" @@ -1303,14 +1306,71 @@ def _extract_served_model(raw: str) -> str | None: return None if not isinstance(data, dict): return None - served = data.get("model") - if not isinstance(served, str) or not served.strip(): + return _safe_model_identifier(data.get("model")) + + +def _safe_model_identifier(value: Any) -> str | None: + """Accept only a conservative, bounded model identifier safe for public logs.""" + if not isinstance(value, str): + return None + candidate = value.strip() + if not SAFE_MODEL_IDENTIFIER_RE.fullmatch(candidate): + return 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. + + 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. + """ + try: + raw_bytes = exc.read(MAX_HTTP_ERROR_BODY_BYTES + 1) + except (AttributeError, OSError, ValueError): + return None + if len(raw_bytes) > MAX_HTTP_ERROR_BODY_BYTES: + return None + try: + payload = json.loads(raw_bytes.decode("utf-8")) + except (UnicodeDecodeError, json.JSONDecodeError, TypeError, ValueError): + return None + if not isinstance(payload, dict): + return None + error = payload.get("error") + if not isinstance(error, dict): 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 + detail = error.get("detail") + if not isinstance(detail, dict): + return None + return _safe_model_identifier(detail.get("model")) + + +def _bounded_allowed_locations_json(allowed_locations: Sequence[dict[str, Any]]) -> str: + """Serialize the largest location prefix that fits the prompt byte budget.""" + total_count = len(allowed_locations) + + def render(count: int) -> str: + return json.dumps( + { + "total_count": total_count, + "truncated": count < total_count, + "locations": list(allowed_locations[:count]), + }, + ensure_ascii=False, + separators=(",", ":"), + ) + + low = 0 + high = total_count + while low < high: + midpoint = (low + high + 1) // 2 + if len(render(midpoint).encode("utf-8")) <= MAX_ALLOWED_LOCATIONS_JSON_BYTES: + low = midpoint + else: + high = midpoint - 1 + return render(low) def _truthy_env(name: str) -> bool: @@ -1434,6 +1494,7 @@ def call_llm( location_example = allowed_locations[0] if allowed_locations else { "path": "path", "line": 0, "side": "RIGHT" } + allowed_locations_json = _bounded_allowed_locations_json(allowed_locations) prompt = { "role": "user", "content": "\n".join( @@ -1442,6 +1503,9 @@ def call_llm( "Review the PR diff plus the additional changed-file and review-thread context for correctness, security, maintainability, and behavioral regressions.", "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 only path, line, and side tuples listed in the bounded allowed-locations JSON below. If it is truncated, omit a formal verdict for any location not listed instead of guessing.", + f"Allowed changed-side locations: {allowed_locations_json}", + f"Location shape example: {json.dumps(location_example, separators=(',', ':'))}", "Use request_changes only for blocking, concrete issues. A generic no-issues statement is not review evidence.", f"Repository: {repo}", f"PR: #{number}", @@ -1525,6 +1589,9 @@ def call_llm( ) validate_substantive_verdict(verdict, diff, changed_paths) except (RuntimeError, urllib.error.URLError, http.client.HTTPException, OSError) as exc: + if isinstance(exc, urllib.error.HTTPError): + active_phase = "response_error" + served_model = _extract_http_error_served_model(exc) elapsed = time.monotonic() - attempt_started current_failure = _stable_failure_diagnostic(exc) model_note = served_model or "unknown" diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index ba65ba6b1f..008388b707 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -1,6 +1,7 @@ import base64 import hashlib import http.client +import io import json import os import shlex @@ -1457,6 +1458,142 @@ def fake_getaddrinfo_invalid_ip(host, port, *args, **kwargs): assert noema.call_llm("owner/repo", 1, pr, "diff", True, "head")["decision"] == "approve" +def test_call_llm_prompts_with_bounded_exact_changed_locations(monkeypatch): + """The model receives the same exact-line contract enforced after inference.""" + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example.test/chat") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "secret") + monkeypatch.setattr(noema, "validate_substantive_verdict", lambda *_args: None) + captured = {} + verdict = {"decision": "approve", "summary": "checked", "findings": []} + + class Response: + def __enter__(self): + return self + + def __exit__(self, *args): + return False + + def read(self): + return json.dumps( + {"choices": [{"message": {"content": json.dumps(verdict)}}]} + ).encode() + + class Opener: + def open(self, request): + captured.update(json.loads(request.data.decode())) + return Response() + + diff = """diff --git a/tool.py b/tool.py +--- a/tool.py ++++ b/tool.py +@@ -292,2 +295,2 @@ +-old = True ++new = True +""" + monkeypatch.setattr(noema.urllib.request, "build_opener", lambda *_args: Opener()) + + noema.call_llm("owner/repo", 1, make_pr(), diff, False, "head") + + prompt = captured["messages"][1]["content"] + marker = "Allowed changed-side locations: " + locations_line = next(line for line in prompt.splitlines() if line.startswith(marker)) + envelope = json.loads(locations_line.removeprefix(marker)) + assert len(locations_line.removeprefix(marker).encode()) <= noema.MAX_ALLOWED_LOCATIONS_JSON_BYTES + assert envelope == { + "total_count": 2, + "truncated": False, + "locations": [ + {"path": "tool.py", "line": 292, "side": "LEFT"}, + {"path": "tool.py", "line": 295, "side": "RIGHT"}, + ], + } + assert '"line":293' not in locations_line + + +def test_allowed_locations_json_truncates_at_the_byte_budget(): + """Large changed-line sets remain valid JSON within the prompt budget.""" + locations = [ + {"path": f"src/{index:05d}-{'가' * 80}.py", "line": index + 1, "side": "RIGHT"} + for index in range(1000) + ] + + rendered = noema._bounded_allowed_locations_json(locations) + envelope = json.loads(rendered) + + assert len(rendered.encode("utf-8")) <= noema.MAX_ALLOWED_LOCATIONS_JSON_BYTES + assert envelope["total_count"] == len(locations) + assert envelope["truncated"] is True + assert 0 < len(envelope["locations"]) < len(locations) + + +def test_call_llm_reports_only_safe_model_from_bounded_http_error(monkeypatch, capsys): + """A gateway HTTP error exposes only its canonical safe model identifier.""" + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example.test/chat") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "secret") + secret = "never-print-this-error-detail" + body = json.dumps( + { + "error": { + "detail": {"model": "github_models/deepseek-v3", "secret": secret}, + "message": secret, + }, + "arbitrary": secret, + } + ).encode() + + class Opener: + def open(self, request): + raise noema.urllib.error.HTTPError( + request.full_url, 502, "Bad Gateway", {}, io.BytesIO(body) + ) + + monkeypatch.setattr(noema.urllib.request, "build_opener", lambda *_args: Opener()) + + with pytest.raises(noema.NoemaTransportError) as exc_info: + noema.call_llm("owner/repo", 1, make_pr(), "diff", False, "head") + + output = capsys.readouterr().out + diagnostic = str(exc_info.value) + assert "phase=response_error" in output + 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 secret not in output + assert secret not in diagnostic + + +@pytest.mark.parametrize( + "body", + [ + b"not-json", + b'{"error":{"detail":{"model":"unsafe model value"}}}', + b"x" * (noema.MAX_HTTP_ERROR_BODY_BYTES + 1), + ], +) +def test_call_llm_http_error_malformed_or_oversized_model_is_unknown( + monkeypatch, capsys, body +): + """Malformed, unsafe, and oversized HTTP error bodies fail closed.""" + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example.test/chat") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "secret") + + class Opener: + def open(self, request): + raise noema.urllib.error.HTTPError( + request.full_url, 502, "Bad Gateway", {}, io.BytesIO(body) + ) + + 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 body.decode("utf-8", errors="ignore") not in output + + def test_noema_redirect_handler_rejects_redirects(): """Noema must not follow redirects after validating the initial URL.""" handler = noema.NoRedirectHandler()