From 8f556b5c3fff9c1fef52819d5f8b615de27ad7d9 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 08:17:29 +0000 Subject: [PATCH 1/9] fix(noema): add standalone-CLI import fallback to noema_review_gate.py PR #1497 (4a5dfd82) added an unconditional `from scripts.ci.opencode_review_normalize_output import changed_file_is_material` at module scope in noema_review_gate.py. Under bare-script invocation (`python3 scripts/ci/noema_review_gate.py ...`, with no PYTHONPATH set), sys.path[0] is the script's own directory (scripts/ci/), not the repository root, so the absolute import always raised `ModuleNotFoundError: No module named 'scripts'`. This was confirmed live in contextual-orchestrator PR #946, run 33370760438, job `noema-review`. Note: by the time this branch was pushed, PR #1501 (c8cc68a3) had already landed on main and fixed the immediate breakage by changing the noema-review.yml call site to invoke the script as a module (`python3 -m scripts.ci.noema_review_gate`), which also makes the absolute import resolve correctly. This commit is a complementary defense-in-depth fix: it applies the same `if __package__: ... else: ...` conditional-import fallback already used by noema_review_handoff.py for the identical dual-invocation requirement, so the script itself is robust to being invoked either as a package module or as a bare script by any current or future caller (this workflow, another repo's tooling, or manual debugging), rather than depending solely on every call site remembering to use `-m`. 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 the exact production failure mode pre-fix and proving it no longer occurs. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX --- CHANGELOG.md | 18 +++++++++++++++++ scripts/ci/noema_review_gate.py | 5 ++++- tests/test_noema_review_gate.py | 35 +++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 39c61c142b..536286f049 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,24 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Fix a critical org-wide Noema review outage: PR #1497 added an + unconditional `from scripts.ci.opencode_review_normalize_output import + changed_file_is_material` at module scope in `noema_review_gate.py`, but + the central `noema-review.yml` workflow invokes this script as a bare + script (`python3 scripts/ci/noema_review_gate.py ...`) with no + `PYTHONPATH` set, so `sys.path[0]` is the script's own directory + (`scripts/ci/`), not the repository root, and the absolute import always + raised `ModuleNotFoundError: No module named 'scripts'`. This crashed + every Noema review, in every repository the org runs the central + `pull_request_target` review workflow against, since #1497 merged + (confirmed live in `contextual-orchestrator` PR #946, run + `33370760438`, job `noema-review`). Applied the same + `if __package__: ... else: ...` conditional-import fallback already used + by `noema_review_handoff.py` for the identical bare-script-vs-package + problem, and added a regression test that runs + `python3 scripts/ci/noema_review_gate.py --help` as a subprocess from the + repository root with `PYTHONPATH` cleared, reproducing the exact + production invocation shape and proving the fix. - Harden the review sidecar's per-account catalog cap against silent drift: `contextual_orchestrator_review_launcher.py`'s two `build_zdr_prioritized_catalog` call sites now source their diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 8d8d964691..538a9474c1 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -19,7 +19,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 = { diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 2cda59d128..fae78b6280 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -1,12 +1,47 @@ import base64 import json +import os +import subprocess import sys +from pathlib import Path import pytest from scripts.ci import noema_review_gate as noema +def test_standalone_cli_runs_from_repo_root_without_pythonpath(): + """Reproduce the production workflow invocation shape and prove it works. + + The central ``noema-review.yml`` workflow runs this script as a bare + script (``python3 scripts/ci/noema_review_gate.py ...``) from the + repository root with no ``PYTHONPATH`` set. 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. This test drives the real production shape end to end. + """ + 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 fake_secret(*parts: str) -> str: return "".join(parts) From fc9ff367a17b978ca8692680ca603322a8915a17 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 08:23:18 +0000 Subject: [PATCH 2/9] docs: correct CHANGELOG narrative on #1503 to reflect #1501 already fixing the outage Devin's review correctly flagged that the changelog entry read as if the bare-script ModuleNotFoundError were still a live outage. PR #1501 already fixed the active incident (workflow now invokes the script via `python3 -m`); this PR's import-level fallback is complementary hardening, not an outage fix. Reword to say so plainly. --- CHANGELOG.md | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 536286f049..fb0e61a43e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,24 +5,25 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] -- Fix a critical org-wide Noema review outage: PR #1497 added an - unconditional `from scripts.ci.opencode_review_normalize_output import - changed_file_is_material` at module scope in `noema_review_gate.py`, but - the central `noema-review.yml` workflow invokes this script as a bare - script (`python3 scripts/ci/noema_review_gate.py ...`) with no - `PYTHONPATH` set, so `sys.path[0]` is the script's own directory - (`scripts/ci/`), not the repository root, and the absolute import always - raised `ModuleNotFoundError: No module named 'scripts'`. This crashed - every Noema review, in every repository the org runs the central - `pull_request_target` review workflow against, since #1497 merged - (confirmed live in `contextual-orchestrator` PR #946, run - `33370760438`, job `noema-review`). Applied the same - `if __package__: ... else: ...` conditional-import fallback already used - by `noema_review_handoff.py` for the identical bare-script-vs-package - problem, and added a regression test that runs - `python3 scripts/ci/noema_review_gate.py --help` as a subprocess from the - repository root with `PYTHONPATH` cleared, reproducing the exact - production invocation shape and proving the fix. +- 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 `contextual-orchestrator` + PR #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. - Harden the review sidecar's per-account catalog cap against silent drift: `contextual_orchestrator_review_launcher.py`'s two `build_zdr_prioritized_catalog` call sites now source their From 6ccf913eac1a20fbb403f1588dc4dabab733072a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 08:26:35 +0000 Subject: [PATCH 3/9] docs: use full owner/repo#num cross-repo reference in CHANGELOG Devin flagged that "contextual-orchestrator PR #946" doesn't follow this org's binding cross-repo reference convention (owner/repo#num or a full URL), so GitHub can't create the cross-repository link. --- CHANGELOG.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fb0e61a43e..7e73d1b1f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,11 +12,11 @@ Semantic Versioning where the repository publishes a release. `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 `contextual-orchestrator` - PR #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 + 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 From 76750f1f413b2bc5dbcc484a27a2190e309f4280 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 08:52:08 +0000 Subject: [PATCH 4/9] fix(ci): correct required-workflow-bootstrap job scope in strix quick gate test assert_opencode_review_uses_codegraph_and_contextual_orchestrator's awk range `/^ required-workflow-bootstrap:$/,/^[^ ]/` never terminates in this file, since job keys are always 2-space indented and no truly-unindented line exists anywhere in the jobs: section. This silently pulled every job after required-workflow-bootstrap into the "must have no if:" check, tripping on an unrelated, legitimate if: condition on a later job's step and failing this required check on every open .github-repo PR. required-workflow-bootstrap itself has always had zero if: conditions -- only the test's own job-scoping was broken. Replace the range with an explicit state machine that starts at the bootstrap job header and stops at the next 2-space-indented job key. Verified: `bash scripts/ci/test_strix_quick_gate.sh` now passes (previously failed with exactly the false-positive record_failure this fix removes); full suite (2125 passed, 1 skipped, 21 subtests) and `git diff --check` clean. --- CHANGELOG.md | 17 +++++++++++++++++ scripts/ci/test_strix_quick_gate.sh | 6 +++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e73d1b1f3..3eb4e4803c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,23 @@ Semantic Versioning where the repository publishes a release. 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. +- Fix a broken CI contract test that was blocking every open `.github`-repo + PR: `test_strix_quick_gate.sh`'s + `assert_opencode_review_uses_codegraph_and_contextual_orchestrator` used an + `awk '/^ required-workflow-bootstrap:$/,/^[^ ]/'` range to isolate that + one job's YAML block in `opencode-review.yml`, intending to assert it has + no `if:` condition on any step (a real trust-boundary invariant: this + bootstrap job must never depend on event-payload fields). Because job keys + in that file are always 2-space indented, `/^[^ ]/` (a truly unindented + line) never matches anywhere in the `jobs:` section, so the range never + closed and silently swallowed every job defined after + `required-workflow-bootstrap` too — including the unrelated, + legitimate `if: github.event.action != 'closed'` on a completely different + job's step. `required-workflow-bootstrap` itself has always had zero `if:` + conditions; only the test's own job-scoping was wrong. Replaced the range + with an explicit awk state machine that starts at the bootstrap job header + and stops at the next 2-space-indented job key, so it correctly isolates + only that job's steps. - Harden the review sidecar's per-account catalog cap against silent drift: `contextual_orchestrator_review_launcher.py`'s two `build_zdr_prioritized_catalog` call sites now source their diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index 4053f4fd53..1008a03be1 100644 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -522,7 +522,11 @@ assert_opencode_review_uses_codegraph_and_contextual_orchestrator() { assert_file_not_contains "$workflow_file" "Wait for trusted OpenCode approval review" "opencode pull_request bridge was removed to avoid duplicate required-check resource use" assert_file_not_contains "$workflow_file" "Trusted OpenCode requested changes for head" "opencode pull_request bridge no longer reconsumes stale trusted review state" assert_file_not_contains "$workflow_file" "github.event.pull_request.number == 240" "opencode review workflow must not hard-code repository-specific PR bypasses" - if awk '/^ required-workflow-bootstrap:$/,/^[^ ]/' "$bootstrap_file" | grep -q '^[[:space:]]*if:'; then + if awk ' + /^ required-workflow-bootstrap:$/ { in_job = 1; print; next } + in_job && /^ [A-Za-z0-9_-]+:$/ { exit } + in_job { print } + ' "$bootstrap_file" | grep -q '^[[:space:]]*if:'; then record_failure "opencode required workflow bootstrap must not depend on required-workflow event payload fields" fi assert_file_contains "$workflow_file" 'github.event.client_payload.target_repository || github.repository' "opencode review scopes concurrency by target repository" From 342bd079d34b881f942c3800eca317d690484661 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 08:40:06 +0000 Subject: [PATCH 5/9] fix(tests): update stale draft/head-moved assertion left by #1697 While validating the merge with main on this PR, the full suite surfaced tests/test_opencode_live_draft_state_regression.py::test_draft_exemption_fails_closed_when_live_head_moved failing. Reproduced identically on a clean, unmodified checkout of origin/main (5c561a65) alone via a disposable worktree, confirming this is pre-existing on protected main and unrelated to this PR's own diff (scripts/ci/noema_review_gate.py's standalone-import fallback). Root cause: #1697 ("retire stale draft/head dispatches without false failure") deliberately inverted opencode-review.yml's check order -- the draft/closed exemption now runs before the head-moved check, and a still-draft PR whose live head has also moved now exits 0 quietly instead of failing closed with exit 1 -- fixing a real production false-failure (contextual-orchestrator run 33548447878/job 100066104033). #1697 updated its own new tests in test_opencode_required_verdict_regression.py to match, but this file's independent _run_step harness covering the identical production script text was never updated, so it kept asserting the superseded pre-#1697 contract. Same pattern as this repo's own previously-documented "stale test assertions left by a merged PR" class of fix (see CHANGELOG's #1654/#1656/ #1658 entry): no production behavior changed here, only the test's assertion and docstring, which now match the intentional, already-reviewed #1697 contract and cross-reference its sibling coverage. Verified: tests/test_opencode_live_draft_state_regression.py 19/19 passed; interrogate 100% docstring coverage. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX --- ...st_opencode_live_draft_state_regression.py | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/tests/test_opencode_live_draft_state_regression.py b/tests/test_opencode_live_draft_state_regression.py index a9d9c518bc..1473882aa6 100644 --- a/tests/test_opencode_live_draft_state_regression.py +++ b/tests/test_opencode_live_draft_state_regression.py @@ -186,15 +186,31 @@ def test_stale_draft_request_reuses_live_ready_approval(tmp_path: Path) -> None: @pytest.mark.parametrize("script", (request_review_script(), fail_closed_script())) -def test_draft_exemption_fails_closed_when_live_head_moved( +def test_draft_exemption_also_applies_when_live_head_has_moved( tmp_path: Path, script: str, ) -> None: - """The event cannot exempt a different live head even when it is still draft.""" + """A still-draft PR is exempted even if its live head has also moved. + + Stale-test regression: this asserted the pre-#1697 contract (head-moved + checked, and failed closed, before the draft exemption), which #1697 + deliberately inverted to stop a still-iterating draft PR from failing + with a spurious ``head moved`` error on every race between the event + snapshot and this step's own live re-fetch + (https://github.com/ContextualWisdomLab/contextual-orchestrator/actions/runs/33548447878/job/100066104033, + fixed in `.github/workflows/opencode-review.yml` and covered from the + production-incident angle by + `test_request_review_step_exempts_a_draft_pr_whose_live_head_has_moved` + in `test_opencode_required_verdict_regression.py`). This file's own + `_run_step` harness never had its matching case updated to the new + contract, so it kept failing after #1697 merged. + """ result = _run_step(tmp_path, script, live_draft=True, live_head="b" * 40) - assert result.returncode == 1 - assert "head moved while validating live" in result.stdout + assert result.returncode == 0, result.stderr + assert "still a draft on the live exact head" in result.stdout + assert "head moved while validating live" not in result.stdout + assert "::error::" not in result.stdout @pytest.mark.parametrize("script", (request_review_script(), fail_closed_script())) From b1647f00c4216e20ee2e787a4e9f169cb58805f0 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 13:45:46 +0000 Subject: [PATCH 6/9] docs(tests): correct stale production-path claim in noema_review_gate test Devin Review on #1503: the test docstring described bare-script invocation as "the production workflow invocation shape," but the central noema-review.yml workflow no longer calls noema_review_gate.py directly at all. The current production entry point is the two-phase handoff (.github/actions/noema-review/two_phase.py), which imports noema_review_gate as a package after explicitly inserting the repository root onto sys.path -- it never exercises the bare-script failure mode this test reproduces. Rewrote the docstring to describe this test's actual purpose: coverage for the standalone-CLI shape (any other repo's tooling, or a human debugging the script directly), not the live workflow's own invocation shape. No behavior or assertion changed. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX --- tests/test_noema_review_gate.py | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 40942fd12b..e8b9c0e2a9 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -16,16 +16,23 @@ def test_standalone_cli_runs_from_repo_root_without_pythonpath(): - """Reproduce the production workflow invocation shape and prove it works. - - The central ``noema-review.yml`` workflow runs this script as a bare - script (``python3 scripts/ci/noema_review_gate.py ...``) from the - repository root with no ``PYTHONPATH`` set. 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. This test drives the real production shape end to end. + """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) From 9c58d550d78b86bc9bd5025a7353c47e848b0075 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 3 Sep 2026 00:58:16 +0000 Subject: [PATCH 7/9] fix(ci): update stale quick-gate assertion for hourly scheduler cadence test_strix_quick_gate.sh's assert_pr_review_merge_scheduler_uses_github_actions_bot_token still asserted the pre-lengthening cron literal 'cron: "*/30 * * * *"' for pr-review-merge-scheduler.yml's scan-pr-queue heartbeat. That cadence was deliberately lengthened to hourly ('cron: "30 * * * *"') for Actions-capacity reasons (docs/doctoring/actions-queue-saturation-hourly-sweep.md, #1630), and tests/test_actions_queue_saturation_scheduler_cadence.py's test_scan_pr_queue_heartbeat_is_hourly_and_offset_not_removed already enforces exactly that hourly value and explicitly forbids the old */30 literal -- but this quick-gate assertion was never updated in the same change, so it started failing this PR's exact-head-path-policy check against the current (correct) workflow content. Reproduces identically on unmodified main; not specific to this PR's own diff. Verified: the exact grep -F literal this assertion checks was RED against the old '*/30 * * * *' string and is GREEN against the current '30 * * * *' string; full scripts/ci/test_strix_quick_gate.sh run passes; tests/test_actions_queue_saturation_scheduler_cadence.py still passes. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX --- scripts/ci/test_strix_quick_gate.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index d5db849145..06821735f2 100755 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -1559,7 +1559,7 @@ assert_pr_review_merge_scheduler_uses_github_actions_bot_token() { assert_file_contains "$workflow_file" 'pull_request_target:' "scheduler can run as an organization required workflow without repository-local copies" assert_file_contains "$workflow_file" 'auto_merge_enabled' "scheduler rechecks already stale PRs as soon as native auto-merge is enabled" assert_file_contains "$workflow_file" 'workflows: ["Required OpenCode Review", "Strix Security Scan"]' "scheduler reruns after review or security evidence completion so approvals can trigger merge/update actions" - assert_file_contains "$workflow_file" 'cron: "*/30 * * * *"' "scheduler wakes frequently enough to clear auto-merge PRs that become stale after their initial PR events" + assert_file_contains "$workflow_file" 'cron: "30 * * * *"' "scheduler wakes on an hourly heartbeat to clear auto-merge PRs that become stale after their initial PR events" 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" assert_file_contains "$workflow_file" "github.event_name == 'workflow_run' && github.event.workflow_run.pull_requests[0].number && format('pr-{0}', github.event.workflow_run.pull_requests[0].number)" "scheduler scopes workflow_run concurrency to the completed review PR" From acdd47e7d68c941e1d1a66087cc9197be17eb192 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 20:41:22 +0000 Subject: [PATCH 8/9] docs(changelog): document the stale org-sweep cron assertion fix Found while merging current main into PR #1503's branch: the quick-gate's assert_pr_review_merge_scheduler_uses_github_actions_bot_token required a cron the org-sweep dispatch-only redesign already retired, contradicting two other regression contracts already on main. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX --- CHANGELOG.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d264e1d9a..2fcd1b9728 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,20 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- **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 From d5963045d6061136960c377979e23883481f4caa Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 21:22:38 +0000 Subject: [PATCH 9/9] fix(noema): close CWE-532 telemetry leak and schema-repair gap in review gate Address two CodeRabbit "Major" findings on PR #1503 against the current state of scripts/ci/noema_review_gate.py: - SAFE_MODEL_IDENTIFIER_RE's character-class allowlist for the served_model/terminal_reason/provider_name/upstream_phase gateway-error telemetry fields was broad enough to also accept a GitHub PAT (gh[pousr]_<36+ alnum chars>) or a JWT (three dot-separated base64url segments), which would then be printed into this pull_request_target workflow's public Actions logs (CWE-532). _safe_model_identifier now also rejects both explicit secret shapes via a new _looks_like_secret_shape check, dropping the field entirely rather than reflecting it, matching this module's existing redaction idiom. - _noema_verdict_json_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 skipped the gateway's schema-repair path entirely and failed the review outright after one request. The schema now carries two allOf/if/then branches (_noema_decision_requirement) encoding exactly what the Python validators already enforce per decision, verified against the real jsonschema library (contextual-orchestrator's own validator). Also closes a pre-existing 100% branch-coverage gap in _extract_http_error_telemetry (malformed last-attempt entry / out-of- range attempt scalars) surfaced while adding these regression tests. Full tests/test_noema_review_gate.py suite: 123 passed. Coverage and docstrings on scripts/ci/noema_review_gate.py: 100%. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX --- CHANGELOG.md | 2 + scripts/ci/noema_review_gate.py | 113 ++++++++- tests/test_noema_review_gate.py | 410 ++++++++++++++++++++++++++++++++ 3 files changed, 524 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2fcd1b9728..b077e30475 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,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. diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 2044519a85..dc477a347c 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -65,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" @@ -142,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. @@ -150,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", @@ -187,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), + ], } @@ -1313,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/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 5623e7c52e..88b1bf867b 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -700,6 +700,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") @@ -1566,6 +1683,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") @@ -1681,6 +2006,91 @@ def open(self, request): assert '{"error":' not in output +def test_call_llm_http_error_ignores_non_dict_last_attempt(monkeypatch, capsys): + """A non-dict ``attempts[-1]`` entry drops all per-attempt telemetry. + + ``_extract_http_error_telemetry`` only reads ``provider_name``/``phase``/ + ``attempt_number``/``provider_status`` when ``attempts[-1]`` is itself a + dict; a malformed last entry (here, a bare string) must not crash the + review job, and none of those four fields may appear in the log. + """ + 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": ["not-a-dict"], + }, + }, + } + ).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=" not in output + assert "upstream_phase=" not in output + assert "attempt_number=" not in output + 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()