diff --git a/CHANGELOG.md b/CHANGELOG.md index 43020db98..3460386f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,32 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Fix `existing_noema_review()` treating a "legacy" Noema review (one posted before + `NOEMA_REVIEW_FOOTER_MARKER` existed) as proof the current head was already reviewed. + `noema_review_handoff.py`'s `noema_review_state()` can never recognize such a review as a + valid current-head verdict (its trusted-span helpers return empty without the footer marker), + so an unchanged PR carrying only a legacy review would stall forever: the gate skips + republishing believing it is done, and the handoff never accepts what was already posted. + `existing_noema_review()` now also requires `NOEMA_REVIEW_FOOTER_MARKER` before treating a + review as already covering the head, so a legacy review no longer suppresses a rerun that + would publish a current-format replacement. +- 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. - Fail closed when the first top-level Noema JSON candidate is malformed, preventing a later approval object from overriding malformed preface data; multiple-object output remains supported when its first object is valid. diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index d7ef15a2e..1d3476265 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -28,6 +28,14 @@ "opencode-agent", } GITHUB_APP_BOT_RE = re.compile(r"^[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\[bot\]$") +# Wraps the start of the fixed-format footer submit_review() writes below the +# LLM-generated summary/findings text. This lets noema_review_handoff.py +# locate the footer by *position* (the trusted, machine-emitted span between +# this marker and the closing "" +# comment) instead of by scanning for a content pattern that the LLM's own +# unsanitized output could coincidentally reproduce. Keep this literal in +# exact sync with NOEMA_REVIEW_FOOTER_MARKER in noema_review_handoff.py. +NOEMA_REVIEW_FOOTER_MARKER = "" MAX_DIFF_CHARS = 60000 MAX_CONTEXT_FILES = 12 MAX_FILE_CONTEXT_CHARS = 4000 @@ -178,7 +186,17 @@ def review_commit(review: dict[str, Any]) -> str: def existing_noema_review(pr: dict[str, Any], actor: str) -> bool: - """Return whether Noema already reviewed the current head.""" + """Return whether Noema already posted a trusted verdict for the current head. + + Requires ``NOEMA_REVIEW_FOOTER_MARKER`` alongside the base marker, matching + the trust boundary ``noema_review_handoff.py``'s ``noema_review_state()`` + enforces. A review predating that footer marker (a "legacy" review, from + before this exact position-anchored binding existed) is a review + ``noema_review_state()`` can never recognize as a valid current-head + verdict; treating it as "already reviewed" here would let it silently + suppress every future publish attempt for an otherwise-unchanged head, + stalling the PR until its head changes for an unrelated reason. + """ head_sha = str(pr.get("headRefOid") or "") marker = "") -NOEMA_BODY_HEAD_RE = re.compile(r"Head SHA:\s*`([0-9a-fA-F]{40})`") +# Must stay byte-for-byte identical to NOEMA_REVIEW_FOOTER_MARKER in +# noema_review_gate.py's submit_review(). See _isolate_trusted_footer() for +# why this positional bound exists. +NOEMA_REVIEW_FOOTER_MARKER = "" +# Matches only the literal footer bullet submit_review() writes +# ("- Head SHA: ``", one full line via re.MULTILINE, nothing else). This +# is deliberately *not* the sole defense — see _isolate_trusted_footer(). +NOEMA_BODY_HEAD_RE = re.compile(r"^- Head SHA:\s*`([0-9a-fA-F]{40})`$", re.MULTILINE) TERMINAL_NOEMA_STATES = {"APPROVED", "CHANGES_REQUESTED", "COMMENTED"} @@ -95,6 +102,67 @@ def fetch_reviews( return flatten_reviews(document) +def _isolate_trusted_footer(body: str) -> str: + """Return the machine-emitted footer span of a Noema review body. + + submit_review() writes its fixed-format footer (the ``Result`` / + ``Head SHA`` / ``Reviewer credential`` / ``Actor`` bullets) in one + specific position: after ``NOEMA_REVIEW_FOOTER_MARKER`` and before the + closing ```` comment. Everything + else in the body — the summary and findings the LLM itself generates — + is unsanitized and can in principle contain a line that merely + *resembles* a footer bullet (a standalone ``- Head SHA: ```` line + included in prose, for instance, which an earlier version of this + extraction only excluded when it did not fall on its own line, and did + not exclude at all before that). Locating the footer by *position* + between the two trusted, machine-emitted delimiters — rather than by + scanning the whole body for a content pattern the LLM's own output could + reproduce, deliberately or by coincidence — removes that class of + collision entirely: LLM text can never land inside a span bounded on + both sides by markers only ``submit_review()`` emits. + + Returns an empty string when the footer marker cannot be found (for + example, a review body posted before this marker existed), which causes + the caller's exact-one-match check to fail closed rather than fall back + to scanning untrusted text. + """ + before_end_marker = body.rsplit(NOEMA_REVIEW_MARKER, 1)[0] + parts = before_end_marker.rsplit(NOEMA_REVIEW_FOOTER_MARKER, 1) + return parts[1] if len(parts) == 2 else "" + + +def _isolate_trusted_marker_tail(body: str) -> str: + """Return the machine-emitted tail of a Noema review body, footer onward. + + ``submit_review()``'s ``"\\n".join([...])`` writes ``NOEMA_REVIEW_FOOTER_MARKER`` + immediately before its fixed-format footer bullets, and the closing + ```` comment is + unconditionally the *last* element of that join — nothing follows it. + So, just like the span ``_isolate_trusted_footer()`` extracts, everything + from the footer marker to the end of the body is exclusively + machine-emitted text the LLM's own summary/findings prose can never + reach. + + ``noema_review_state()`` used to run ``NOEMA_MARKER_HEAD_RE`` over the + raw, unsanitized ``body`` to find the closing marker — the marker-side + counterpart of the body-side gap ``_isolate_trusted_footer()`` was added + to close. An LLM can, in principle, generate a complete, + correctly-formatted ````-shaped string of its own (for instance while discussing this exact + review format) anywhere in its free-form prose *before* the real footer. + Searching this trusted tail instead removes that string from + consideration entirely, the same way position-anchoring already does for + the body-side bullet. + + Returns an empty string when the footer marker cannot be found (for + example, a review body posted before this marker existed), which causes + the caller's exact-one-match check to fail closed, matching + ``_isolate_trusted_footer()``'s own behavior. + """ + parts = body.rsplit(NOEMA_REVIEW_FOOTER_MARKER, 1) + return parts[1] if len(parts) == 2 else "" + + def noema_review_state(reviews: list[dict[str, Any]], head_sha: str) -> str | None: """Return Noema's latest terminal verdict for the exact current head.""" for review in reversed(reviews): @@ -106,8 +174,10 @@ def noema_review_state(reviews: list[dict[str, Any]], head_sha: str) -> str | No if NOEMA_REVIEW_MARKER not in str(review.get("body") or ""): continue body = str(review.get("body") or "") - marker_heads = NOEMA_MARKER_HEAD_RE.findall(body) - body_heads = NOEMA_BODY_HEAD_RE.findall(body) + marker_tail = _isolate_trusted_marker_tail(body) + marker_heads = NOEMA_MARKER_HEAD_RE.findall(marker_tail) + footer_text = _isolate_trusted_footer(body) + body_heads = NOEMA_BODY_HEAD_RE.findall(footer_text) if len(marker_heads) != 1 or len(body_heads) != 1: continue if marker_heads[0].lower() != head_sha.lower() or body_heads[0].lower() != head_sha.lower(): diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 522960562..58ca88138 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -687,7 +687,7 @@ def fake_run(args, stdin=None): def test_existing_noema_review_matches_actor_and_head(): - noema_marker = "" + noema_marker = noema.NOEMA_REVIEW_FOOTER_MARKER + "" assert noema.existing_noema_review( make_pr(reviews={"nodes": [review(login="noema", body=noema_marker)]}), "noema", @@ -708,6 +708,22 @@ def test_existing_noema_review_matches_actor_and_head(): assert not noema.existing_noema_review(make_pr(reviews={"nodes": [review(commit="old", login="noema")]}), "noema") +def test_existing_noema_review_rejects_legacy_body_without_footer_marker(): + """A review predating NOEMA_REVIEW_FOOTER_MARKER must not suppress a rerun. + + noema_review_handoff.py's noema_review_state() can never recognize such a + review as a valid current-head verdict (its trusted-span helpers return + empty without the footer marker), so treating it as "already reviewed" + here would stall an unchanged PR forever: the gate skips republishing, + and the handoff never accepts what was already posted. + """ + legacy_marker = "" + assert not noema.existing_noema_review( + make_pr(reviews={"nodes": [review(login="noema", body=legacy_marker)]}), + "noema", + ) + + def test_current_actor_fetch_diff_and_json_extraction(monkeypatch): monkeypatch.setenv("NOEMA_REVIEW_ACTOR", "cwl-noema-review[bot]") monkeypatch.setenv("NOEMA_REVIEW_INSTALLATION_ID", "123") @@ -1674,7 +1690,19 @@ def test_inspect_and_review_skip_paths(monkeypatch): cases = [ (make_pr(isDraft=True), "noema"), - (make_pr(reviews={"nodes": [review(login="noema", body="")]}), "noema"), + ( + make_pr( + reviews={ + "nodes": [ + review( + login="noema", + body=noema.NOEMA_REVIEW_FOOTER_MARKER + "", + ) + ] + } + ), + "noema", + ), ] for pr, actor in cases: calls.clear() @@ -1683,6 +1711,19 @@ def test_inspect_and_review_skip_paths(monkeypatch): assert noema.inspect_and_review("owner/repo", 7, "head") == 0 assert calls == [] + # A review predating NOEMA_REVIEW_FOOTER_MARKER must not suppress a + # rerun: noema_review_handoff.py's noema_review_state() can never accept + # it as a valid current-head verdict, so the gate must republish rather + # than silently stall the PR on an unchanged head. + legacy_pr = make_pr( + reviews={"nodes": [review(login="noema", body="")]} + ) + calls.clear() + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number, pr=legacy_pr: pr) + monkeypatch.setattr(noema, "current_actor", lambda: "noema") + assert noema.inspect_and_review("owner/repo", 7, "head") == 0 + assert calls + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: clean_pr) monkeypatch.setattr(noema, "current_actor", lambda: "") with pytest.raises(RuntimeError, match="identity could not be verified"): diff --git a/tests/test_noema_review_handoff.py b/tests/test_noema_review_handoff.py index a7c3582fe..3a13a713c 100644 --- a/tests/test_noema_review_handoff.py +++ b/tests/test_noema_review_handoff.py @@ -6,6 +6,7 @@ import pytest +from scripts.ci import noema_review_gate as gate from scripts.ci import noema_review_handoff as handoff @@ -13,6 +14,23 @@ OTHER_HEAD = "b" * 40 +def test_footer_marker_stays_synchronized_between_publisher_and_consumer(): + """The publisher's and consumer's footer marker literals must be identical. + + ``noema_review_gate.submit_review`` (the publisher) and + ``noema_review_handoff.noema_review_state`` (the consumer) each hardcode + their own copy of ``NOEMA_REVIEW_FOOTER_MARKER`` rather than sharing one + definition (Devin review finding on #1500). A one-sided future edit to + either copy would silently desynchronize the trust boundary: the + publisher would keep emitting its old marker, the consumer would keep + searching for its new one, and every future Noema verdict would fail the + handoff's exact-one-match check and time out closed with no direct + signal pointing at the actual cause. This contract test is the direct + signal instead. + """ + assert gate.NOEMA_REVIEW_FOOTER_MARKER == handoff.NOEMA_REVIEW_FOOTER_MARKER + + def test_standalone_cli_starts_outside_repository_root(tmp_path): """The workflow's direct script invocation must not depend on its cwd.""" completed = subprocess.run( @@ -42,12 +60,14 @@ def opencode_review(head: str = HEAD) -> dict: def noema_review(state: str = "APPROVED", head: str = HEAD) -> dict: + """Build a minimal, correctly-formed Noema review for the given head.""" return { "id": 8, "state": state, "commit_id": head, "user": {"login": "cwl-noema-review[bot]"}, "body": ( + f"{handoff.NOEMA_REVIEW_FOOTER_MARKER}\n" f"- Head SHA: `{head}`\n" f"" ), @@ -129,19 +149,203 @@ def test_noema_state_ignores_forged_marker_from_other_actor(): @pytest.mark.parametrize( "body", [ + # No footer marker and no body-side bullet at all: nothing to bind. f"", - f"- Head SHA: `{OTHER_HEAD}`\n", - f"- Head SHA: `{HEAD}`\n", - f"- Head SHA: `{HEAD}`\n- Head SHA: `{HEAD}`\n", - f"- Head SHA: `{HEAD}`\n\n", + # The trusted footer marker is present but empty: still nothing to bind. + f"{handoff.NOEMA_REVIEW_FOOTER_MARKER}\n", + # Body-side bullet inside the trusted footer, but the wrong value. + f"{handoff.NOEMA_REVIEW_FOOTER_MARKER}\n- Head SHA: `{OTHER_HEAD}`\n", + # Marker-side value wrong instead. + f"{handoff.NOEMA_REVIEW_FOOTER_MARKER}\n- Head SHA: `{HEAD}`\n", + # Genuinely duplicated body-side binding, both inside the trusted footer. + f"{handoff.NOEMA_REVIEW_FOOTER_MARKER}\n- Head SHA: `{HEAD}`\n- Head SHA: `{HEAD}`\n", + # Genuinely duplicated marker-side binding. + f"{handoff.NOEMA_REVIEW_FOOTER_MARKER}\n- Head SHA: `{HEAD}`\n\n", ], ) def test_noema_state_rejects_missing_stale_or_duplicate_head_bindings(body): + """The dual head-SHA binding #1480/#1483 added must still reject a real defect. + + Every case here is a genuine problem with the binding itself (missing, + wrong value, or truly duplicated) rather than incidental LLM text — the + two acceptance tests below prove the fix does not conflate the two. + """ value = noema_review() value["body"] = body assert handoff.noema_review_state([value], HEAD) is None +@pytest.mark.parametrize( + "prose", + [ + # A prose sentence (LLM summary) echoing the exact footer phrasing — + # plausible when Noema reviews a PR touching this very mechanism + # (noema_review_gate.py / noema_review_handoff.py) or a commit + # message discussing a git SHA in this shape. + f"This PR's handoff logic previously mismatched when a stale Head SHA: `{OTHER_HEAD}` lingered in prose.", + # The identical SHA repeated in prose, not just a different one — + # the bug is about counting matches, not about which value they hold. + f"Note: the canonical footer below repeats Head SHA: `{HEAD}` for readability.", + ], +) +def test_noema_state_accepts_valid_review_despite_incidental_body_text(prose): + """A genuine verdict must survive LLM prose that merely resembles the footer. + + Regression test for the false-positive rejection Devin's automated review + flagged on PR #1415 (root cause pre-existing on `main` since #1480/#1483): + the original unanchored ``NOEMA_BODY_HEAD_RE`` searched the *entire* + review body, so an LLM-generated summary or finding that happened to + contain the literal shape ``Head SHA: `<40 hex chars>``` — anywhere, not + just in the fixed-format footer ``submit_review()`` writes — produced a + second match, tripped the ``len(body_heads) != 1`` duplicate guard, and + made ``noema_review_state()`` wrongly return ``None`` for an otherwise + valid, correctly-authored Noema verdict. The negative-control tests + immediately above this one prove the fix did not weaken the dual-binding + property #1480/#1483 added (missing / stale / genuinely duplicated + bindings must still reject); this test proves incidental mid-sentence + prose no longer does. See + ``test_noema_state_ignores_standalone_body_head_bullet_before_footer`` + below for the follow-up case (a complete standalone bullet line, not + just a mid-sentence phrase) Devin's review of the first fix caught. + """ + body = "\n".join( + [ + "## Noema LLM review", + "", + prose, + "", + "### Findings", + "- No blocking findings.", + "", + handoff.NOEMA_REVIEW_FOOTER_MARKER, + "- Result: APPROVE", + f"- Head SHA: `{HEAD}`", + "- Reviewer credential: `NOEMA_REVIEW_TOKEN`", + "- Actor: `noema-bot`", + "", + f"", + ] + ) + value = noema_review() + value["body"] = body + assert handoff.noema_review_state([value], HEAD) == "APPROVED" + + +@pytest.mark.parametrize( + "rogue_head", + [OTHER_HEAD, HEAD], + ids=["different-sha", "same-sha"], +) +def test_noema_state_ignores_standalone_body_head_bullet_before_footer(rogue_head): + """A complete standalone footer-shaped bullet in LLM text must not count. + + Regression test for the follow-up gap Devin's automated review found in + the first fix on PR #1500: anchoring ``NOEMA_BODY_HEAD_RE`` to a whole + line (``re.MULTILINE``) narrowed the collision surface from "anywhere in + the body" down to "any full line before the trusted end marker" — but an + LLM's own summary/findings text is free-form and unsanitized, so it can + still emit a complete, correctly-formatted ``- Head SHA: ```` line + of its own (e.g. while quoting or discussing this exact review format, + the same self-referential scenario that makes the underlying bug + likely). That line still satisfied the whole-line regex, so counting + matches anywhere before the end marker still produced 2 and still + wrongly rejected a valid verdict. + + The actual fix isolates the footer by *position* instead of by content + pattern: only the span between ``NOEMA_REVIEW_FOOTER_MARKER`` and the + closing HTML comment — both machine-emitted by ``submit_review()`` and + never reachable by the LLM's own text — is searched. A standalone bullet + placed anywhere before that span is now excluded regardless of how + precisely it mimics the real footer line, and regardless of whether it + holds a different SHA or the very same one as the real binding. + """ + body = "\n".join( + [ + "## Noema LLM review", + "", + "Earlier attempts at this mechanism produced review bodies like:", + f"- Head SHA: `{rogue_head}`", + "which is exactly the bullet shape this fix now ignores outside the footer.", + "", + "### Findings", + "- No blocking findings.", + "", + handoff.NOEMA_REVIEW_FOOTER_MARKER, + "- Result: APPROVE", + f"- Head SHA: `{HEAD}`", + "- Reviewer credential: `NOEMA_REVIEW_TOKEN`", + "- Actor: `noema-bot`", + "", + f"", + ] + ) + value = noema_review() + value["body"] = body + assert handoff.noema_review_state([value], HEAD) == "APPROVED" + + +@pytest.mark.parametrize( + "rogue_head", + [OTHER_HEAD, HEAD], + ids=["different-sha", "same-sha"], +) +def test_noema_state_ignores_standalone_closing_marker_before_footer(rogue_head): + """A complete standalone closing-marker string in LLM text must not count. + + Regression test for the marker-side asymmetry Devin's automated review + found in the second fix on PR #1500 (comment on + ``noema_review_handoff.py:146``, "Marker-shaped model text still rejects + reviews"): position-anchoring fixed the *body-side* ``- Head SHA:`` + bullet check (see + ``test_noema_state_ignores_standalone_body_head_bullet_before_footer`` + above) but left the *marker-side* check unanchored — + ``NOEMA_MARKER_HEAD_RE.findall(body)`` still scanned the entire + unsanitized body for anything shaped like the closing + ```` comment. An + LLM's own summary/findings text is free-form, so it can emit a complete, + correctly-formatted closing-marker-shaped string of its own — the same + self-referential scenario that makes the body-side bug likely (Noema + reviewing a PR that touches this very mechanism, or discussing a git SHA + in this shape) — anywhere before the real footer. That produced 2 + matches for ``len(marker_heads) != 1`` and wrongly rejected an otherwise + valid, correctly-authored verdict, regardless of whether the fake + marker's SHA matched the real head or a different one. + + The fix applies the identical position-anchoring already used for the + body-side bullet: ``_isolate_trusted_marker_tail()`` returns only the + span from ``NOEMA_REVIEW_FOOTER_MARKER`` to the end of the body — which + ``submit_review()`` guarantees is exclusively machine-emitted, since the + real closing marker is unconditionally the last element of its + ``"\\n".join([...])`` — and the marker search now runs against that tail + instead of the raw body. A standalone closing-marker-shaped string placed + anywhere before the real footer marker is now excluded regardless of + which SHA it carries. + """ + body = "\n".join( + [ + "## Noema LLM review", + "", + "Earlier attempts at this mechanism produced review bodies like:", + f"", + "which is exactly the closing-marker shape this fix now ignores outside the footer.", + "", + "### Findings", + "- No blocking findings.", + "", + handoff.NOEMA_REVIEW_FOOTER_MARKER, + "- Result: APPROVE", + f"- Head SHA: `{HEAD}`", + "- Reviewer credential: `NOEMA_REVIEW_TOKEN`", + "- Actor: `noema-bot`", + "", + f"", + ] + ) + value = noema_review() + value["body"] = body + assert handoff.noema_review_state([value], HEAD) == "APPROVED" + + def test_stale_initial_head_never_reads_reviews_or_dispatches(capsys): fake = FakeGitHub([[opencode_review()]], heads=[OTHER_HEAD]) diff --git a/tests/test_repository_branch_coverage_review_schedulers.py b/tests/test_repository_branch_coverage_review_schedulers.py index 4928e1804..ff2037398 100644 --- a/tests/test_repository_branch_coverage_review_schedulers.py +++ b/tests/test_repository_branch_coverage_review_schedulers.py @@ -80,6 +80,7 @@ def test_noema_handoff_returns_current_terminal_state() -> None: "commit_id": head, "user": {"login": handoff.NOEMA_REVIEW_AUTHOR}, "body": ( + f"{handoff.NOEMA_REVIEW_FOOTER_MARKER}\n" f"- Head SHA: `{head}`\n" f"" ),