diff --git a/CHANGELOG.md b/CHANGELOG.md index 46d599a320..135c797444 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -106,6 +106,8 @@ - Add `.github/actions/orchestrator-free-sidecar`, an immutable composite-action boundary that checks out the exact central control-plane revision selected by `github.action_ref` and provisions the contextual-orchestrator `orchestrator/free` gateway. Provider bootstrap remains inside the central sidecar; callers receive only the gateway URL/token-file contract for the subsequent Agent step. - Repointed 10 `scripts/ci/test_strix_quick_gate.sh` self-test assertions that had gone stale after the `pr_review_merge_scheduler.py`/`pr_review_merge_scheduler_core.py` facade/core split (#1803): they checked the now-98-line facade file for content (the exact-head branch-update guard, the squash-fallback retry, the subprocess-safety flags, the same-head Strix/OpenCode dispatch markers, and the `pr_head_ref` repository-dispatch payload) that lives in the core module instead, so they had been silently failing on every run since the split. The same repair aligns the wake-workflow list and daily recovery assertions with the current event-driven scheduler contract. A coverage/docstring version of the same gap was already fixed via #1810; this bash contract script was missed. - **Fix the `coalesce` required check crashing instead of exiting cleanly for a superseded queued run.** `current-head-run-coalescer.yml`'s own design comment documents that `current_head_run_coalescer.py` raising `CoalescingRefused` (its remembered head no longer matching the PR's live head) is "a safe no-op" — but `main()` only ever called `coalesce()` directly, so the exception raised by `coalesce()`'s own top-level live-PR-state check propagated uncaught and crashed the job with exit code 1, instead of the intended graceful no-op. Reproduced live on `ContextualWisdomLab/.github#1503` (run `33766056421`, job `100684095620`): a stale queued run drained from the org-wide Actions capacity backlog against an already-superseded head failed the required `coalesce` check with `CoalescingRefused: pull request head moved before duplicate classification`. `main()` now catches `CoalescingRefused` specifically and exits 0 with an informational message; any other exception (malformed identity, an unavailable GitHub API) still fails closed. +- **Fix `noema_review_gate.py` accepting secret-shaped values into public gateway-error telemetry (CWE-532).** `SAFE_MODEL_IDENTIFIER_RE`'s character-class allowlist for `served_model`/`terminal_reason`/`provider_name`/`upstream_phase` was broad enough to also accept a GitHub PAT (`gh[pousr]_<36+ alnum chars>`) and a JWT (three dot-separated base64url segments) placed by an untrusted gateway HTTP error envelope, which would then be printed into this `pull_request_target` workflow's public Actions logs. CodeRabbit finding (Major, CWE-532) on PR #1503. `_safe_model_identifier` now also rejects both explicit secret shapes via a new `_looks_like_secret_shape` check, dropping the field entirely (matching this module's existing "unsafe input is omitted, never reflected" idiom) rather than emitting a redacted placeholder in its place. Added regression coverage proving a secret-shaped value in each of the four fields never reaches the printed log or the raised diagnostic. +- **Make `_noema_verdict_json_schema()` decision-conditional so a semantically-invalid verdict is schema-invalid too.** The schema allowed `reviewed_lines`/`adversarial_validation` to be `null` and `findings` to be empty even for `approve`/`request_changes` decisions, while `validate_substantive_verdict()`/`call_llm()` separately reject exactly those shapes in Python — so a schema-valid-but-semantically-invalid response (e.g. `decision: "approve"` with `findings: []` and null `reviewed_lines`) skipped the gateway's own schema-repair/correction path entirely and failed the whole review outright after a single request. CodeRabbit finding (Major, Stability) on PR #1503. The schema now carries two `allOf`/`if`/`then` branches (via a new `_noema_decision_requirement` helper) requiring, per decision, exactly the non-null `reviewed_lines`, non-null `adversarial_validation` with the decision's exact required `status`, and (`request_changes` only, matching `call_llm`'s own check) non-empty `findings` that the Python validators already enforce — no more, no less, so `comment` verdicts stay exactly as permissive as before. Verified directly against the real `jsonschema` library (`contextual-orchestrator`'s own validator) and added regression tests proving each violation is now rejected at the schema level. ## 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. @@ -139,6 +141,47 @@ Semantic Versioning where the repository publishes a release. extended `tests/test_required_review_runner_image_contract.py` (already refactored to a shared `assert_explicit_supported_image` helper by concurrent work) with a fourth case for this file. +- **Fix a `test_strix_quick_gate.sh` assertion left stale by the org-sweep + dispatch-only redesign.** `assert_pr_review_merge_scheduler_uses_github_actions_bot_token` + still required `pr-review-merge-scheduler.yml` to contain a second daily cron + (`cron: "17 3 * * *"`) for the organization missed-event sweep. That cron was + already retired in favor of an explicit `github.event.client_payload.org_sweep + == true` dispatch-only trigger, and its absence is already an enforced + regression contract in both `tests/test_actions_queue_saturation_scheduler_cadence.py` + (`test_org_queue_sweep_is_explicit_recovery_not_scheduled_polling`) and + `tests/test_required_workflow_queue_contract.py` -- so the quick-gate assertion + directly contradicted two other tests already on `main` and failed on every PR + merging current `main`, independent of that PR's own diff. Updated the + assertion to require the cron's *absence*, matching the other two contracts. + Found while resolving a merge conflict on PR #1503 (this PR's own branch + predates the org-sweep dispatch-only redesign). +- Harden `noema_review_gate.py` against the bare-script import failure PR + #1497 introduced: it added an unconditional `from + scripts.ci.opencode_review_normalize_output import + changed_file_is_material` at module scope, which raises + `ModuleNotFoundError: No module named 'scripts'` when the script is run + directly (`python3 scripts/ci/noema_review_gate.py ...`, `sys.path[0]` + being the script's own directory rather than the repository root). This + was a live, org-wide Noema review outage (confirmed in + `ContextualWisdomLab/contextual-orchestrator#946`, run `33370760438`, job + `noema-review`) until PR #1501 fixed the active incident by changing + `noema-review.yml`'s call site to `python3 -m scripts.ci.noema_review_gate + ...`, which also resolves the absolute import. This change is complementary defense-in-depth, not an active-outage + fix: it applies the same `if __package__: ... else: ...` conditional-import + fallback already used by `noema_review_handoff.py` directly to + `noema_review_gate.py`, so the script also runs correctly as a bare script + (not just as a module), and adds a regression test that runs `python3 + scripts/ci/noema_review_gate.py --help` as a subprocess from the + repository root with `PYTHONPATH` cleared, reproducing that invocation + shape and proving the fix independently of #1501's workflow-level fix. +- Update `test_strix_quick_gate.sh`'s `assert_pr_review_merge_scheduler_uses_github_actions_bot_token` + assertion to expect `pr-review-merge-scheduler.yml`'s current `scan-pr-queue` heartbeat + cron (`"30 * * * *"`, hourly) instead of the pre-lengthening `"*/30 * * * *"` literal. + The cadence was deliberately lengthened for Actions-capacity reasons + (`docs/doctoring/actions-queue-saturation-hourly-sweep.md`, #1630) and + `tests/test_actions_queue_saturation_scheduler_cadence.py` already enforces the new + value and forbids the old one, but this quick-gate assertion was never updated in the + same change. No workflow behavior changes; test-only fix. - **Catch scheduler target-list drift before it silently fails an hourly heartbeat.** `hourly-review-repair.yml`'s per-cron `target_repository` matrix and the `OPENCODE_REPOSITORY_DISPATCH_TARGETS` repository variable (which gates `ALLOWED_TARGET_REPOSITORIES` in `pr-review-merge-scheduler.yml`/`pr-review-fix-scheduler.yml`) are two independently hand-maintained lists with no structural link -- three repositories (`governance-risk-compliance`, `nonnest2`, `quarantine-sandbox-runtime`) were added to the hourly matrix without a corresponding variable update, so their hourly heartbeat failed closed with "target repository is not allowlisted" until each was found and fixed the same day. Added `scripts/ci/opencode_repository_dispatch_targets.json`, a hand-maintained mirror of the variable's live value, and a new contract test (`test_every_hourly_caller_target_is_in_the_dispatch_targets_mirror`) asserting every hourly-caller target is present in it, so a future PR that repeats the omission fails at review time instead of at the next silent hourly failure. See `docs/doctoring/scheduler-target-list-drift-20260902.md`. - **Fix a stale `test_strix_quick_gate.sh` assertion left broken by the `#1630` scheduler-cadence lengthening.** `pr-review-merge-scheduler.yml`'s repository-local diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 5ab7e830f3..dc477a347c 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -22,7 +22,10 @@ from collections.abc import Sequence from typing import Any -from scripts.ci.opencode_review_normalize_output import changed_file_is_material +if __package__: + from scripts.ci.opencode_review_normalize_output import changed_file_is_material +else: # pragma: no cover - exercised by the standalone CLI regression test + from opencode_review_normalize_output import changed_file_is_material PRIMARY_REVIEW_AUTHORS = { @@ -62,6 +65,38 @@ 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}$") +# The character class above is intentionally broad enough to cover every +# legitimate provider/model/phase/reason identifier this module has ever +# observed (e.g. "github_models/deepseek-v3", "nvidia_nim", +# "eligible_candidates_exhausted", "response_error") -- but that same +# breadth also accepts two well-known *secret* shapes built entirely from +# characters the class already allows: a GitHub PAT +# ("gh[pousr]_<36+ alnum chars>") and a JWT (three dot-separated +# base64url segments). Both shapes are checked explicitly and rejected in +# `_safe_model_identifier`, in addition to (never instead of) the +# character-class check above, because these telemetry fields +# (served_model/terminal_reason/provider_name/upstream_phase) are read from +# an untrusted gateway HTTP error envelope and then printed into this +# `pull_request_target` workflow's public Actions logs -- CWE-532 (CodeRabbit +# finding on PR #1503): a compromised or misbehaving upstream could place a +# real leaked credential in any of these fields to exfiltrate it through the +# log, and the plain character-class check alone could not have caught that. +_GITHUB_PAT_SHAPE_RE = re.compile(r"gh[pousr]_[A-Za-z0-9]{36,}") +_JWT_SHAPE_RE = re.compile(r"^[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+$") + + +def _looks_like_secret_shape(candidate: str) -> bool: + """Return whether candidate structurally resembles a GitHub PAT or a JWT. + + A GitHub PAT is matched anywhere inside ``candidate`` (``search``, not + ``fullmatch``) because a token could be embedded as a substring of an + otherwise plausible-looking identifier; a JWT's three-dot-segment shape + is matched against the whole value (``fullmatch``) since that shape is + only meaningful end to end. Neither pattern needs a minimum overall + length beyond what each shape itself already implies -- a short + JWT-shaped value is exactly as unsafe to log as a long one. + """ + return bool(_GITHUB_PAT_SHAPE_RE.search(candidate) or _JWT_SHAPE_RE.fullmatch(candidate)) ORCHESTRATOR_LOOPBACK_HOSTS = frozenset({"127.0.0.1", "::1"}) ORCHESTRATOR_BASE_ENV = "CONTEXTUAL_ORCHESTRATOR_BASE_URL" @@ -139,6 +174,55 @@ }, "required": ["severity", "file", "line", "side", "message"], } +def _noema_decision_requirement(decision: str, adversarial_status: str, *, require_findings: bool) -> dict[str, Any]: + """Return one ``allOf`` branch requiring exact-line evidence for one decision. + + Fires only when the verdict's ``decision`` equals this branch's exact + literal value (``if.properties.decision.const``); a non-matching + decision leaves the branch's ``then`` unevaluated, per JSON Schema's own + ``if``/``then`` semantics. When it does fire, ``then`` demands a non-null, + non-empty ``reviewed_lines`` array and a non-null ``adversarial_validation`` + object whose ``status`` is this decision's exact required value -- and, + only when ``require_findings`` is set, a non-empty ``findings`` array. + + This mirrors ``validate_substantive_verdict`` and ``call_llm`` field for + field rather than in the aggregate: both already reject a decision != + "comment" verdict with a null/empty ``reviewed_lines``, a missing + ``adversarial_validation``, or the wrong ``adversarial_validation.status`` + for the decision (``"passed"`` for approve, ``"failed"`` for + request_changes); ``call_llm`` additionally rejects an empty ``findings`` + array specifically for ``request_changes`` (not ``approve``, which may + legitimately have none). Encoding exactly this -- no more, no less -- at + the schema level means a response that would fail either Python check is + now already schema-invalid, so it is caught by the gateway's own + schema-repair/correction path instead of reaching ``call_llm`` and + failing the whole review outright after a single request (CodeRabbit + finding on PR #1503). + """ + then_properties: dict[str, Any] = { + "reviewed_lines": {"type": "array", "minItems": 1}, + "adversarial_validation": { + "type": "object", + "properties": {"status": {"const": adversarial_status}}, + "required": ["status"], + }, + } + then_required = ["reviewed_lines", "adversarial_validation"] + if require_findings: + then_properties["findings"] = {"type": "array", "minItems": 1} + then_required.append("findings") + return { + "if": { + "properties": {"decision": {"const": decision}}, + "required": ["decision"], + }, + "then": { + "properties": then_properties, + "required": then_required, + }, + } + + def _noema_verdict_json_schema(required_probes: int) -> dict[str, Any]: """Build the verdict JSON Schema with this request's exact probe floor. @@ -147,6 +231,19 @@ def _noema_verdict_json_schema(required_probes: int) -> dict[str, Any]: -- 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. + + The base per-property schemas below stay permissive on their own + (``reviewed_lines``/``adversarial_validation`` remain nullable, and + ``findings`` has no ``minItems``) because that is the correct, and only, + shape for a ``comment`` decision -- ``validate_substantive_verdict`` + returns immediately for ``comment`` without checking any of these + fields, and ``call_llm`` never requires findings for it either. The two + ``allOf`` branches from ``_noema_decision_requirement`` layer the + additional, decision-specific requirements on top for ``approve`` and + ``request_changes`` only, so a schema-valid ``comment`` response is + unaffected while an ``approve``/``request_changes`` response now carries + exactly the same requirements ``validate_substantive_verdict``/ + ``call_llm`` already enforce. """ return { "type": "object", @@ -184,6 +281,10 @@ def _noema_verdict_json_schema(required_probes: int) -> dict[str, Any]: "adversarial_validation", "findings", ], + "allOf": [ + _noema_decision_requirement("approve", "passed", require_findings=False), + _noema_decision_requirement("request_changes", "failed", require_findings=True), + ], } @@ -1310,12 +1411,25 @@ def _extract_served_model(raw: str) -> str | None: def _safe_model_identifier(value: Any) -> str | None: - """Accept only a conservative, bounded model identifier safe for public logs.""" + """Accept only a conservative, bounded model identifier safe for public logs. + + Rejects both a value outside the conservative character-class allowlist + and a value that, despite passing that allowlist, structurally matches a + GitHub PAT or a JWT (see ``_looks_like_secret_shape``). Either rejection + drops the field entirely rather than emitting a redacted placeholder in + its place -- matching this module's existing "unsafe input is omitted, + never partially reflected" idiom for these bounded gateway-telemetry + fields (the caller falls back to a fixed "unknown" placeholder for + ``served_model``, and simply omits the field from + ``_format_gateway_error_telemetry`` otherwise). + """ if not isinstance(value, str): return None candidate = value.strip() if not SAFE_MODEL_IDENTIFIER_RE.fullmatch(candidate): return None + if _looks_like_secret_shape(candidate): + return None return candidate diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index b9b1c43de3..b1c5aaf810 100755 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -1562,6 +1562,7 @@ assert_pr_review_merge_scheduler_uses_github_actions_bot_token() { assert_file_contains "$workflow_file" 'auto_merge_enabled' "scheduler rechecks already stale PRs as soon as native auto-merge is enabled" assert_file_not_contains "$workflow_file" 'workflow_run:' "required-check completion relies on GitHub auto-merge without spawning scheduler runs" assert_file_contains "$workflow_file" 'cron: "47 3 * * *"' "scheduler keeps one daily central missed-event recovery" + assert_file_not_contains "$workflow_file" 'cron: "17 3 * * *"' "org sweep is an explicit dispatch-only recovery, not a second scheduled cron (matches test_actions_queue_saturation_scheduler_cadence.py and test_required_workflow_queue_contract.py)" assert_file_not_contains "$workflow_file" "org-queue-sweep" "scheduler does not consume a runner on organization-wide polling" assert_file_not_contains "$workflow_file" "github.event.pull_request.number == 240" "scheduler must not hard-code repository-specific PR bypasses" assert_file_contains "$workflow_file" "github.event_name == 'pull_request_target' && format('pr-{0}', github.event.pull_request.number)" "scheduler scopes pull_request_target concurrency to the active PR" diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 5fa23dec53..552fae8302 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -16,6 +16,45 @@ from scripts.ci import noema_review_gate as noema +def test_standalone_cli_runs_from_repo_root_without_pythonpath(): + """Prove the standalone-CLI invocation shape works without PYTHONPATH. + + ``noema_review_gate.py`` is no longer the central ``noema-review.yml`` + workflow's own invocation shape: the two-phase Noema review handoff + (``.github/actions/noema-review/two_phase.py``) is the current production + entry point, and it imports this module as a package + (``from scripts.ci import noema_review_gate as gate``) after explicitly + inserting the repository root onto ``sys.path`` -- so it never hits the + bare-script failure mode this test reproduces. This test instead covers + the standalone-CLI shape directly (``python3 + scripts/ci/noema_review_gate.py ...`` from the repository root with no + ``PYTHONPATH`` set): any other repo's tooling, or a human debugging this + script directly, invokes it that way. PR #1497 added an unconditional + ``from scripts.ci.opencode_review_normalize_output import ...`` at module + scope, which crashes with ``ModuleNotFoundError: No module named + 'scripts'`` under that exact invocation because ``sys.path[0]`` is the + script's own directory (``scripts/ci``), not the repository root. + """ + repo_root = Path(noema.__file__).resolve().parent.parent.parent + env = dict(os.environ) + env.pop("PYTHONPATH", None) + + completed = subprocess.run( + [sys.executable, "scripts/ci/noema_review_gate.py", "--help"], + cwd=repo_root, + env=env, + check=False, + text=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + timeout=10, + ) + + assert completed.returncode == 0 + assert "noema_review_gate.py" in completed.stdout + assert "ModuleNotFoundError" not in completed.stderr + + def test_gitleaks_ignore_is_exactly_scoped_to_superseded_uuid_fixture(): entries = { line @@ -721,6 +760,123 @@ def test_scrub_sensitive_data_authorization_headers(): assert noema.scrub_sensitive_data("authorization: bearer xyz") == "authorization: bearer ***" +def test_safe_model_identifier_rejects_github_pat_and_jwt_shapes(): + """CWE-532 regression: a secret-shaped value must never pass as "safe". + + ``SAFE_MODEL_IDENTIFIER_RE``'s character class alone (alnum plus + ``._:/@+-``) is broad enough to also accept a GitHub PAT + (``gh[pousr]_<36+ alnum chars>``) and a JWT (three dot-separated + base64url segments) -- CodeRabbit finding on PR #1503. Both shapes must + now be rejected by ``_safe_model_identifier`` even though every + character in them individually passes the allowlist, while every + previously-accepted real identifier this module has ever observed still + passes unchanged. + """ + github_pat_shaped = fake_secret( + "ghp", "_", "1234567890abcdef1234567890abcdef1234" + ) + github_pat_shaped_other_prefix = fake_secret( + "gho", "_", "abcdef1234567890abcdef1234567890abcdef" + ) + jwt_shaped = fake_secret( + "eyJhbGciOiJIUzI1NiJ9", + ".", + "eyJzdWIiOiIxMjM0NTY3ODkwIn0", + ".", + "SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV-adQssw5c", + ) + short_jwt_shaped = fake_secret("a", ".", "b", ".", "c") + embedded_pat = fake_secret( + "model-", "ghp", "_", "1234567890abcdef1234567890abcdef1234", "-canary" + ) + + for secret_value in ( + github_pat_shaped, + github_pat_shaped_other_prefix, + jwt_shaped, + short_jwt_shaped, + embedded_pat, + ): + assert noema._looks_like_secret_shape(secret_value) is True + assert noema._safe_model_identifier(secret_value) is None + + for legitimate_value in ( + "github_models/deepseek-v3", + "nvidia_nim", + "eligible_candidates_exhausted", + "connecting", + "response_error", + "openai/gpt-4o", + "meta-llama/Llama-3.1-8b-instruct", + ): + assert noema._looks_like_secret_shape(legitimate_value) is False + assert noema._safe_model_identifier(legitimate_value) == legitimate_value + + +def test_call_llm_http_error_never_emits_secret_shaped_telemetry(monkeypatch, capsys): + """CWE-532 regression: no telemetry field ever leaks a secret-shaped value. + + Every one of the four gateway-supplied telemetry fields + (``served_model``, ``terminal_reason``, ``provider_name``, + ``upstream_phase``) is exercised with a secret-shaped value here; none of + them may reach the printed Actions-log lines or the raised diagnostic. + Rejected fields are dropped entirely (this module's existing idiom for + "unsafe" telemetry -- see ``_safe_model_identifier``), so ``served_model`` + falls back to the fixed ``"unknown"`` placeholder and the other three are + simply absent from ``_format_gateway_error_telemetry``'s output. + """ + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example.test/chat") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "secret") + github_pat_shaped = fake_secret( + "ghp", "_", "1234567890abcdef1234567890abcdef1234" + ) + jwt_shaped = fake_secret( + "eyJhbGciOiJIUzI1NiJ9", + ".", + "eyJzdWIiOiIxMjM0NTY3ODkwIn0", + ".", + "SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV-adQssw5c", + ) + body = json.dumps( + { + "error": { + "detail": { + "model": github_pat_shaped, + "terminal_reason": jwt_shaped, + "attempts": [{ + "provider_name": github_pat_shaped, + "phase": jwt_shaped, + "attempt_number": 1, + "provider_status": 502, + }], + }, + }, + } + ).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) + for leaked_surface in (output, diagnostic): + assert github_pat_shaped not in leaked_surface + assert jwt_shaped not in leaked_surface + assert "provider_name=" not in leaked_surface + assert "upstream_phase=" not in leaked_surface + assert "terminal_reason=" not in leaked_surface + assert "served_model=unknown" in output + assert "served_model=unknown" in diagnostic + + def test_split_repo_and_graphql(monkeypatch): with pytest.raises(ValueError): noema.split_repo("owner") @@ -1587,6 +1743,214 @@ def test_allowed_locations_json_truncates_at_the_byte_budget(): assert 0 < len(envelope["locations"]) < len(locations) +def _mini_schema_valid(schema, instance): + """Validate instance against the narrow JSON Schema subset this suite needs. + + ``jsonschema`` is not in this repo's hash-pinned CI dependency set + (``requirements-opencode-review-ci-hashes.txt`` -- see CLAUDE.md's Hash- + pinned requirements discipline), so this test file cannot import it and + still run under the exact toolchain CI installs. This helper implements + exactly the keywords ``_noema_verdict_json_schema`` emits -- ``type`` + (string or list), ``enum``, ``const``, ``properties``, ``required``, + ``additionalProperties: False``, ``items``, ``minItems``, ``allOf``, and + ``if``/``then`` -- against one instance value. It is deliberately not a + general JSON Schema engine; it only needs to prove the exact + decision-conditional constraint the schema now encodes. Its correctness + is itself cross-checked against the real ``jsonschema`` library + (``validator_for(schema).validate(instance)``, the same call + ``contextual-orchestrator``'s gateway makes) during this fix's + development; see PR #1503. + """ + type_map = { + "object": dict, + "array": list, + "string": str, + "integer": int, + "boolean": bool, + "null": type(None), + } + if "type" in schema: + types = schema["type"] if isinstance(schema["type"], list) else [schema["type"]] + if not any( + t in type_map + and isinstance(instance, type_map[t]) + and not (t == "integer" and isinstance(instance, bool)) + for t in types + ): + return False + if "enum" in schema and instance not in schema["enum"]: + return False + if "const" in schema and instance != schema["const"]: + return False + if isinstance(instance, dict): + properties = schema.get("properties", {}) + for key, subschema in properties.items(): + if key in instance and not _mini_schema_valid(subschema, instance[key]): + return False + for key in schema.get("required", []): + if key not in instance: + return False + if schema.get("additionalProperties") is False and not set(instance) <= set(properties): + return False + if isinstance(instance, list): + if "items" in schema and any(not _mini_schema_valid(schema["items"], item) for item in instance): + return False + if "minItems" in schema and len(instance) < schema["minItems"]: + return False + if any(not _mini_schema_valid(branch, instance) for branch in schema.get("allOf", [])): + return False + if "if" in schema: + branch_key = "then" if _mini_schema_valid(schema["if"], instance) else "else" + if branch_key in schema and not _mini_schema_valid(schema[branch_key], instance): + return False + return True + + +def _reviewed_line(path="a.py", line=1, side="RIGHT", analysis="ok"): + """Build one minimal schema-valid ``reviewed_lines`` entry.""" + return {"path": path, "line": line, "side": side, "analysis": analysis} + + +def _probe(path="a.py", line=1, side="RIGHT", outcome="falsified"): + """Build one minimal schema-valid adversarial probe entry.""" + return { + "path": path, + "line": line, + "side": side, + "hypothesis": "h", + "attack_or_counterexample": "a", + "evidence": "e", + "outcome": outcome, + } + + +def _adversarial_validation(status, probe_count=1, outcome="falsified"): + """Build one minimal schema-valid ``adversarial_validation`` object.""" + return { + "status": status, + "residual_risk": "none", + "probes": [_probe(outcome=outcome) for _ in range(probe_count)], + } + + +@pytest.mark.parametrize( + "verdict", + [ + pytest.param( + { + "decision": "approve", + "summary": "s", + "reviewed_lines": None, + "adversarial_validation": _adversarial_validation("passed"), + "findings": [], + }, + id="approve-null-reviewed_lines", + ), + pytest.param( + { + "decision": "approve", + "summary": "s", + "reviewed_lines": [_reviewed_line()], + "adversarial_validation": None, + "findings": [], + }, + id="approve-null-adversarial_validation", + ), + pytest.param( + { + "decision": "approve", + "summary": "s", + "reviewed_lines": [_reviewed_line()], + "adversarial_validation": _adversarial_validation("failed", outcome="confirmed"), + "findings": [], + }, + id="approve-wrong-adversarial_status", + ), + pytest.param( + { + "decision": "request_changes", + "summary": "s", + "reviewed_lines": [_reviewed_line()], + "adversarial_validation": _adversarial_validation("failed", outcome="confirmed"), + "findings": [], + }, + id="request_changes-empty-findings", + ), + pytest.param( + { + "decision": "request_changes", + "summary": "s", + "reviewed_lines": None, + "adversarial_validation": _adversarial_validation("failed", outcome="confirmed"), + "findings": [{"severity": "high", "file": "a.py", "line": 1, "side": "RIGHT", "message": "m"}], + }, + id="request_changes-null-reviewed_lines", + ), + ], +) +def test_verdict_schema_rejects_decision_conditional_violations(verdict): + """Stability regression: schema-invalid, not just Python-invalid. + + Before this fix, each of these verdicts was schema-*valid* (it only + failed later, inside ``validate_substantive_verdict``/``call_llm``), + so a single-shot LLM response shaped exactly like this skipped the + gateway's own schema-repair/correction path entirely and failed the + whole review outright -- CodeRabbit finding on PR #1503. Each case here + reproduces one specific requirement ``validate_substantive_verdict`` or + ``call_llm`` already enforces in Python for ``approve``/ + ``request_changes`` decisions, now also encoded at the schema level via + ``_noema_decision_requirement``'s ``allOf``/``if``/``then`` branches. + """ + schema = noema._noema_verdict_json_schema(1) + assert _mini_schema_valid(schema, verdict) is False + + +def test_verdict_schema_accepts_substantive_approve_and_request_changes(): + """Positive control: a fully substantive verdict is still schema-valid. + + Guards against the decision-conditional branches added for the Finding 2 + fix over-constraining a genuinely complete verdict for either decision. + """ + schema = noema._noema_verdict_json_schema(1) + approve = { + "decision": "approve", + "summary": "s", + "reviewed_lines": [_reviewed_line()], + "adversarial_validation": _adversarial_validation("passed"), + "findings": [], + } + assert _mini_schema_valid(schema, approve) is True + + request_changes = { + "decision": "request_changes", + "summary": "s", + "reviewed_lines": [_reviewed_line()], + "adversarial_validation": _adversarial_validation("failed", outcome="confirmed"), + "findings": [{"severity": "high", "file": "a.py", "line": 1, "side": "RIGHT", "message": "m"}], + } + assert _mini_schema_valid(schema, request_changes) is True + + +def test_verdict_schema_stays_permissive_for_comment(): + """A ``comment`` verdict is unaffected by the new decision-conditional branches. + + ``validate_substantive_verdict`` returns immediately for ``comment`` + without checking ``reviewed_lines``/``adversarial_validation``/ + ``findings`` at all, and ``call_llm`` never requires findings for it + either -- the schema must stay exactly as permissive as before for this + decision. + """ + schema = noema._noema_verdict_json_schema(1) + comment = { + "decision": "comment", + "summary": "just a note", + "reviewed_lines": None, + "adversarial_validation": None, + "findings": [], + } + assert _mini_schema_valid(schema, comment) is True + + 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") @@ -1738,6 +2102,51 @@ def open(self, request): assert "upstream_status=" not in output +def test_call_llm_http_error_ignores_out_of_bound_attempt_scalars(monkeypatch, capsys): + """Out-of-range/wrong-type ``attempt_number``/``provider_status`` are dropped. + + ``attempt_number`` is only trusted as exactly ``int`` in ``1..64``, and + ``provider_status`` only as exactly ``int`` in ``100..599``. The + surrounding safe-identifier fields (``provider_name``/``phase``) on the + very same attempt entry are unaffected and still reported. + """ + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example.test/chat") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "secret") + body = json.dumps( + { + "error": { + "detail": { + "model": "github_models/deepseek-v3", + "attempts": [{ + "provider_name": "nvidia_nim", + "phase": "connecting", + "attempt_number": "two", + "provider_status": 999, + }], + }, + }, + } + ).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): + noema.call_llm("owner/repo", 1, make_pr(), "diff", False, "head") + + output = capsys.readouterr().out + assert "served_model=github_models/deepseek-v3" in output + assert "provider_name=nvidia_nim" in output + assert "upstream_phase=connecting" in output + assert "attempt_number=" not in output + assert "upstream_status=" not in output + + def test_noema_redirect_handler_rejects_redirects(): """Noema must not follow redirects after validating the initial URL.""" handler = noema.NoRedirectHandler()