From cd416962a1be3dba3859eba04557dd7e0669057f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 05:44:01 +0000 Subject: [PATCH 1/6] fix(noema): re-check for a concurrent submission immediately before posting Follow-up to #1477 (Noema independent review). That PR's noema-review.yml concurrency-group fix (dropping github.event_name so pull_request_target, workflow_run, and repository_dispatch triggers for the same PR share one group) is the primary defense against two runs racing to review the same head, but GitHub's cancel-in-progress is best-effort and does not preempt a run mid-step -- a run that is already past the "no existing review yet" check when a newer trigger supersedes it can still complete its own diff/context/LLM call and post afterward. inspect_and_review now re-fetches the PR and re-checks existing_noema_review immediately before the POST, narrowing that window from the full diff/context/LLM-call duration down to one GraphQL round trip. Also removes the GraphQL query's statusCheckRollup block, left over from before #1477 removed its only consumer (blocking_checks) -- confirmed unused anywhere in the module. Evidence: full suite 2106 passed, 1 skipped, 21 subtests; 100% line/branch coverage; 100% docstrings. --- scripts/ci/noema_review_gate.py | 32 +++++++++++--------------------- tests/test_noema_review_gate.py | 33 +++++++++++++++++++++++++++++++-- 2 files changed, 42 insertions(+), 23 deletions(-) diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 1f7fa40335..d0998ac667 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -123,27 +123,6 @@ def graphql(query: str, **fields: str | int) -> dict[str, Any]: commit { oid } } } - statusCheckRollup { - contexts(first: 100) { - nodes { - __typename - ... on CheckRun { - name - status - conclusion - checkSuite { - workflowRun { - workflow { name } - } - } - } - ... on StatusContext { - context - state - } - } - } - } } } } @@ -595,6 +574,17 @@ def inspect_and_review(repo: str, number: int) -> int: diff, truncated = fetch_diff(repo, number) review_context = build_review_context(repo, number, pr) verdict = call_llm(repo, number, pr, diff, truncated, review_context) + # Re-check for a concurrent Noema submission immediately before posting. + # noema-review.yml's concurrency group (shared across pull_request_target, + # workflow_run, and repository_dispatch triggers) is the primary defense + # against two runs racing to review the same head, but GitHub's + # cancel-in-progress is best-effort and does not preempt a run mid-step; + # this narrows the remaining race window from the full diff/context/LLM + # call duration down to one GraphQL round trip immediately before the + # POST. + if existing_noema_review(fetch_pr(repo, number), actor): + print("Current head already has a Noema review as of just before submission; not posting a duplicate.") + return 0 submit_review(repo, number, pr, actor, verdict) return 0 diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 8855dffd39..f5b09279e6 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -21,7 +21,6 @@ def make_pr(**overrides): "headRefOid": "head", "reviews": {"nodes": []}, "reviewThreads": {"nodes": []}, - "statusCheckRollup": {"contexts": {"nodes": []}}, } value.update(overrides) return value @@ -451,7 +450,6 @@ def test_inspect_and_review_does_not_wait_for_other_reviews_or_checks(monkeypatc pr = make_pr( reviews={"nodes": [review("CHANGES_REQUESTED")]}, reviewThreads={"nodes": [{"isResolved": False, "isOutdated": False}]}, - statusCheckRollup={"contexts": {"nodes": [{"__typename": "StatusContext", "context": "ci", "state": "FAILURE"}]}}, ) calls = [] monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: pr) @@ -465,6 +463,37 @@ def test_inspect_and_review_does_not_wait_for_other_reviews_or_checks(monkeypatc assert calls +def test_inspect_and_review_rechecks_for_a_concurrent_submission_before_posting(monkeypatch): + """A second trigger (e.g. workflow_run) that starts while this run is still + building context/calling the LLM must not publish a duplicate verdict once + the first run has already submitted one for the same head. noema-review.yml's + concurrency-group serialization is the primary defense; this re-check + narrows the remaining race window (cancel-in-progress is best-effort and + does not preempt a run mid-step) down to one GraphQL round trip + immediately before the POST.""" + first_call_pr = make_pr() + already_reviewed_pr = make_pr( + reviews={"nodes": [review(login="noema", body="")]} + ) + fetch_pr_calls = [] + + def fake_fetch_pr(repo, number): + fetch_pr_calls.append(1) + return first_call_pr if len(fetch_pr_calls) == 1 else already_reviewed_pr + + calls = [] + monkeypatch.setattr(noema, "fetch_pr", fake_fetch_pr) + monkeypatch.setattr(noema, "current_actor", lambda: "noema") + monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) + monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr: "context") + monkeypatch.setattr(noema, "call_llm", lambda *args, **kwargs: {"decision": "approve", "summary": "ok", "findings": []}) + monkeypatch.setattr(noema, "submit_review", lambda *args, **kwargs: calls.append(args)) + + assert noema.inspect_and_review("owner/repo", 7) == 0 + assert calls == [] + assert len(fetch_pr_calls) == 2 + + def test_call_llm_rejects_empty_review_content(monkeypatch): monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") From f89833c41d13d4d7964cda510be3d910296ce34b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 01:29:54 +0000 Subject: [PATCH 2/6] fix(test): mock fetch_changed_file_paths after merging main's changed_paths threading Follow-up to the main merge: test_inspect_and_review_rechecks_for_a_ concurrent_submission_before_posting didn't mock fetch_changed_file_paths, which main's #1508 newly threads into inspect_and_review's call_llm invocation. Left unmocked it fell through to a real gh subprocess call and failed closed with FileNotFoundError in this sandbox (no gh CLI). Mocked it to return [] matching this file's established pattern elsewhere. Also documents the merge's one real conflict in CHANGELOG. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4 --- CHANGELOG.md | 8 ++++++++ tests/test_noema_review_gate.py | 1 + 2 files changed, 9 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 39c61c142b..559b213f34 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,14 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Merged `main` into `fix/noema-review-race-and-dead-field` to resolve a real conflict in + `scripts/ci/noema_review_gate.py`'s `inspect_and_review`: this branch's pre-POST re-check for a + concurrent Noema submission (narrowing the duplicate-verdict race window to one GraphQL round trip) + and `main`'s newly-landed `changed_paths` threading into `call_llm` both touched the same call site. + Kept both — the re-check now runs after the `changed_paths`-aware `call_llm` call. Updated + `test_inspect_and_review_rechecks_for_a_concurrent_submission_before_posting` to mock the + now-present `fetch_changed_file_paths` call (previously unmocked in this branch, so it fell through + to a real `gh` subprocess call and failed with `FileNotFoundError` once merged). - 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/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index df30cd46e2..d925f7d1d7 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -540,6 +540,7 @@ def fake_fetch_pr(repo, number): monkeypatch.setattr(noema, "fetch_pr", fake_fetch_pr) monkeypatch.setattr(noema, "current_actor", lambda: "noema") monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) + monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: []) monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr: "context") monkeypatch.setattr(noema, "call_llm", lambda *args, **kwargs: {"decision": "approve", "summary": "ok", "findings": []}) monkeypatch.setattr(noema, "submit_review", lambda *args, **kwargs: calls.append(args)) From 964e47dd07a69f9379de982352a78804e35cf831 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 01:38:11 +0000 Subject: [PATCH 3/6] fix(ci): port main's stale review-dispatch blob pin fix from #1536 CI on this PR's own head surfaced 3 failures caused by main itself being red, not by this PR: #1533 ("proceed on head-only advance in review dispatch validation") changed opencode-review-dispatch.yml's head_sha handling (warn-and-proceed instead of hard-fail on a dispatch/live head mismatch, since downstream jobs already re-validate the live head independently) without updating the two tests pinning that workflow's exact blob hash or the test asserting the old hard-fail behavior. #1536 already fixes this upstream (root-caused, reproduced against unmodified main first, Devin-reviewed, full suite green) but hasn't merged yet. Porting the same two-file diff here per the drive-to-green protocol -- it no-ops once main carries #1536. Full suite: 2128 passed, 1 skipped, 0 failures. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4 --- tests/test_opencode_agent_contract.py | 9 ++++++++- tests/test_pr_review_autofix_nvidia_nim_contract.py | 2 +- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/tests/test_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index 79fdba39aa..6f1478fe96 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -2660,7 +2660,14 @@ def test_opencode_privileged_review_security_boundaries_are_fail_closed(): '^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ ]]' ) in metadata_step assert '[ "$live_head_repository" != "$TARGET_REPOSITORY" ]' not in metadata_step - assert '[ "$SUPPLIED_HEAD_SHA" = "$live_head_sha" ]' in metadata_step + assert '[ "$SUPPLIED_BASE_REF" = "$live_base_ref" ] || mismatches+=("base_ref")' in metadata_step + assert '[ "$SUPPLIED_BASE_SHA" = "$live_base_sha" ] || mismatches+=("base_sha")' in metadata_step + assert '[ "$SUPPLIED_HEAD_REF" = "$live_head_ref" ] || mismatches+=("head_ref")' in metadata_step + assert 'mismatches+=("head_sha")' not in metadata_step + assert ( + 'if [ -n "$SUPPLIED_HEAD_SHA" ] && [ "$SUPPLIED_HEAD_SHA" != "$live_head_sha" ]; then' + ) in metadata_step + assert "::warning::repository_dispatch head advanced since dispatch" in metadata_step assert ( 'live_visibility="$(jq -r \'.base.repo.visibility // empty | ascii_downcase\'' ) in metadata_step diff --git a/tests/test_pr_review_autofix_nvidia_nim_contract.py b/tests/test_pr_review_autofix_nvidia_nim_contract.py index 3dcfe2cdd8..68a0614c01 100644 --- a/tests/test_pr_review_autofix_nvidia_nim_contract.py +++ b/tests/test_pr_review_autofix_nvidia_nim_contract.py @@ -19,7 +19,7 @@ DOCTORING_RECORD = Path("docs/doctoring/hourly-nvidia-nim-autofix.md") CHANGELOG = Path("CHANGELOG.md") REVIEW_DISPATCH_WORKFLOW = Path(".github/workflows/opencode-review-dispatch.yml") -REVIEW_DISPATCH_BLOB_SHA = "2aa245e7f2a053a4c0b7a9cc8bac0d5d44d38092" +REVIEW_DISPATCH_BLOB_SHA = "3762183eb31c2805317362d2b2c2546e4fccdf09" def _workflow_text(path: Path) -> str: From fecf370cd2ffec120824c9c93b9f2de5b8cf3187 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 09:11:28 +0000 Subject: [PATCH 4/6] fix(test): port SIGPIPE flake fix inherited from main via merge The scheduler-wake dispatches fake gh's dispatches branch doesn't consume stdin, causing SIGPIPE on the upstream jq | gh pipe intermittently. Already diagnosed and fixed elsewhere this session; ported the identical one-line `cat >/dev/null` fix here since this branch inherited the flake via merging origin/main, which doesn't have the fix yet. Confirmed clean over 75 repeated runs of the affected test. --- tests/test_opencode_required_verdict_regression.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index 8f8047ff10..0e5d30805b 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -173,6 +173,7 @@ def test_scheduler_wake_reuses_trusted_receipt_predicate( elif [[ "$*" == *"/pulls/7/reviews"* ]]; then printf '[%s]' "$FAKE_REVIEWS" elif [[ "$*" == *"repos/ContextualWisdomLab/.github/dispatches"* ]]; then + cat >/dev/null printf 'dispatch\n' >>"$DISPATCH_CALLS" fi """, From e07f9daf6d4785c81bb3eb14b94f96dd98d3c1e8 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 7 Sep 2026 13:03:44 +0900 Subject: [PATCH 5/6] test(noema): preserve final same-head review revalidation --- tests/test_noema_review_gate.py | 55 +++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index e8a0dd6f59..20312a5a8a 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -1933,6 +1933,61 @@ def test_inspect_and_review_does_not_wait_for_other_reviews_or_checks(monkeypatc assert calls +def test_inspect_and_review_rechecks_for_a_concurrent_submission_before_posting( + monkeypatch, +): + """Do not publish when another run reviewed the same head during model work.""" + head = "a" * 40 + first_pr = make_pr(headRefOid=head) + marker = "\n".join( + [ + noema.NOEMA_REVIEW_FOOTER_MARKER, + "- Result: APPROVE", + f"- Head SHA: `{head}`", + "- Reviewer credential: `test`", + "- Actor: `noema`", + "", + f"", + ] + ) + reviewed_pr = make_pr( + headRefOid=head, + reviews={"nodes": [review(commit=head, login="noema", body=marker)]}, + ) + pull_requests = iter((first_pr, reviewed_pr)) + submissions = [] + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: next(pull_requests)) + monkeypatch.setattr(noema, "current_actor", lambda: "noema") + monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) + monkeypatch.setattr( + noema, + "fetch_changed_files", + lambda repo, number: [("tool.py", "modified")], + ) + monkeypatch.setattr( + noema, + "build_review_context", + lambda repo, number, pr, changed_files=None: "context", + ) + monkeypatch.setattr( + noema, + "call_llm", + lambda *args, **kwargs: { + "decision": "approve", + "summary": "ok", + "findings": [], + }, + ) + monkeypatch.setattr( + noema, + "submit_review", + lambda *args, **kwargs: submissions.append(args), + ) + + assert noema.inspect_and_review("owner/repo", 7, head) == 0 + assert submissions == [] + + def test_stale_trigger_stops_before_identity_or_model_work(monkeypatch): monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr(headRefOid="b" * 40)) monkeypatch.setattr( From 2e92e82fe907cdf708a48351173f68a02d68ab54 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 7 Sep 2026 13:03:49 +0900 Subject: [PATCH 6/6] fix(noema): revalidate exact-head review before submission --- CHANGELOG.md | 4 ++ .../noema-final-submission-revalidation.md | 39 +++++++++++++++++++ docs/product-technical-gap-baseline.md | 6 +++ scripts/ci/noema_review_gate.py | 27 +++---------- 4 files changed, 55 insertions(+), 21 deletions(-) create mode 100644 docs/doctoring/noema-final-submission-revalidation.md diff --git a/CHANGELOG.md b/CHANGELOG.md index bf192f6a9e..e7c3af134f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +### Noema review publication revalidates exact-head uniqueness + +- `inspect_and_review` now re-fetches the live pull request and repeats the trusted Noema receipt check immediately before review submission. A concurrent exact-head review published during model work can no longer be duplicated. The same repair removes the unconsumed `statusCheckRollup` query field. Proposed in ContextualWisdomLab/.github#1482. + ### Failed-check finding names the Strix sandbox instead of the gateway - `opencode-review-dispatch.yml`'s `emit_strix_provider_failure_finding` rendered one fixed finding for every `STRIX_PROVIDER_UNAVAILABLE` line, whose Root cause read "The contextual-orchestrator gateway or its discovered provider pool was unavailable for this run". `#1953` had just given the Strix sandbox bootstrap failure its own second verdict token (`STRIX_SANDBOX_UNAVAILABLE`) precisely because that attribution is wrong for it -- the sandbox container never reaches its Caido proxy, so the run dies before the gateway serves anything -- and this consumer re-applied the wrong attribution one step downstream, into the review findings and the failure census. The emitter now branches on the second token: a sandbox verdict gets a finding that names Strix's sandbox, says the verdict does not name the gateway, and tells the reader not to change gateway or provider configuration on its strength. A `STRIX_PROVIDER_UNAVAILABLE` line without the token keeps its existing text verbatim, so the gateway class has no regression surface. No test covered this finding text at all before (`gateway or its discovered provider pool` matched nothing under `tests/`); `tests/test_opencode_dispatch_strix_sandbox_finding.py` now runs the production emitter from the published run block and pins both directions plus the no-signal case. Refs #1953, #1935. diff --git a/docs/doctoring/noema-final-submission-revalidation.md b/docs/doctoring/noema-final-submission-revalidation.md new file mode 100644 index 0000000000..73fc0165aa --- /dev/null +++ b/docs/doctoring/noema-final-submission-revalidation.md @@ -0,0 +1,39 @@ +# Noema exact-head review submission is revalidated at the write boundary + +검토 기준일: **2026-09-07** + +## Problem + +GitHub Actions concurrency cancellation is best-effort. A run already executing +model work can reach the review POST after another run has published a valid +Noema review for the same head. Head equality alone does not prove that the +write is still unique. + +## Decision + +The Noema gate keeps the pre-model duplicate check, then re-fetches the live +pull request after model work. Immediately before `submit_review`, it repeats +the trusted exact-head Noema receipt check. A concurrent receipt returns +successfully without publishing a duplicate. Closed or moved heads continue to +fail closed before this check. + +The unused `statusCheckRollup` selection is removed because review publication +does not consume check contexts. + +## Verification contract + +`test_inspect_and_review_rechecks_for_a_concurrent_submission_before_posting` +models two live reads: the first has no receipt and the second contains a +trusted receipt for the same head. The POST recorder must remain empty. Hosted +exact-head checks remain mandatory. + +## Status + +**Proposed** in ContextualWisdomLab/.github#1482. Protected `main` remains the +release authority. + +## Reference + +GitHub. (n.d.). *Control the concurrency of workflows and jobs*. GitHub Docs. +Retrieved September 7, 2026, from +https://docs.github.com/en/actions/how-tos/write-workflows/choose-when-workflows-run/control-workflow-concurrency diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 1cc9e20313..59078d2d05 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -7,6 +7,12 @@ 이 문서는 제품·기술·운영 Gap을 현재 문서와 현재 GitHub 상태에 묶어 두는 기준선이다. 새 작업은 먼저 이 문서의 Gap ID를 PR 설명과 테스트 증거에 연결하고, PR의 정확한 exact HEAD·Checks·리뷰를 다시 수집한 뒤 구현한다. 표의 상태는 작성 시점의 관측값이므로, 병합 판단에는 재사용하지 않는다. 이 인벤토리는 스냅샷이며 merge authorization이 아니다. +### 2026-09-07 Noema final-submission concurrency amendment + +- **Gap:** best-effort Actions cancellation cannot stop a run already inside model work, so another run can publish an exact-head Noema review before the first run reaches its POST. +- **Action:** ContextualWisdomLab/.github#1482 re-fetches the live PR after model work and re-checks the independent Noema receipt immediately before submission; the unused status-check query is removed. +- **Status:** Proposed; exact-head hosted Checks, independent review, ordinary protected integration, and post-merge current-main verification remain required. + ## 1. 근거와 범위 ### 1.1 우선순위가 높은 근거 diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 5ab7e830f3..0530299db3 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -329,27 +329,6 @@ def graphql(query: str, **fields: str | int) -> dict[str, Any]: commit { oid } } } - statusCheckRollup { - contexts(first: 100) { - nodes { - __typename - ... on CheckRun { - name - status - conclusion - checkSuite { - workflowRun { - workflow { name } - } - } - } - ... on StatusContext { - context - state - } - } - } - } } } } @@ -1795,6 +1774,12 @@ def inspect_and_review(repo: str, number: int, expected_head: str) -> int: except RuntimeError: print("Pull request closed or its head changed during review; stale verdict was not published.") return 0 + if existing_noema_review(current_pr, actor): + print( + "Current head already has a Noema review immediately before submission; " + "duplicate verdict was not published." + ) + return 0 submit_review(repo, number, current_pr, actor, verdict) return 0