From afd5c682c6e6532165676d4d408b2d0d03fce468 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 7 Sep 2026 03:41:28 +0900 Subject: [PATCH 1/7] fix(scheduler): read workflow identity that run-name cannot rewrite The REST fallback built workflow identity from a run's own `name`, which GitHub renders through `run-name:`. For the three review workflows that declare one, that field carries the pull request and head SHA instead of the workflow's identity, so it matches none of the declared names the policy predicates compare against. Joined on `check_suite_id` for one head, all three workflows declaring `run-name:` diverge from GraphQL's `workflow.name` and all six without it are identical. On that path `is_strix_context` returned False for a real Strix check run and the coverage-evidence predicate returned False for a real coverage-evidence check. `REST_UNKNOWN_GITHUB_ACTIONS_WORKFLOW` did not engage because it is keyed on the name being absent, and here the name is present and wrong. Read the declared name from the workflow resource instead, cached per invocation through the existing reset entry point. A run whose workflow cannot be identified now contributes no entry, so the absent-identity sentinel engages as designed. Prefix-stripping the rendered title was rejected: `run-name:` need not begin with the workflow's name, and assuming it does is the same unenforced parse contract. Also announce both GraphQL-to-REST fallbacks. Neither could previously be observed at any sample size: the permission branch is not retried and raises ahead of any print, and the transient branch raises on the final attempt before `gh_graphql` prints its retry line, so `attempt 4/4` is a string this program cannot emit. Refs #1941 Co-Authored-By: Claude Opus 5 --- scripts/ci/pr_review_merge_scheduler_core.py | 81 +++++- ...ew_fix_scheduler_rest_workflow_identity.py | 251 +++++++++++++++++- 2 files changed, 320 insertions(+), 12 deletions(-) diff --git a/scripts/ci/pr_review_merge_scheduler_core.py b/scripts/ci/pr_review_merge_scheduler_core.py index cffb52cb53..97f4f06959 100644 --- a/scripts/ci/pr_review_merge_scheduler_core.py +++ b/scripts/ci/pr_review_merge_scheduler_core.py @@ -1136,6 +1136,31 @@ def github_resource_inaccessible(exc: RuntimeError) -> bool: return "Resource not accessible by integration" in str(exc) +def warn_graphql_rest_fallback(repo: str, scope: str, exc: RuntimeError) -> None: + """Record that a GraphQL read fell back to REST, naming which cause opened it. + + Neither cause is otherwise visible in a run log. :func:`gh_graphql` raises + before printing its retry line, so the final attempt never announces + itself, and a permission failure is not retried at all -- it raises on the + first attempt, ahead of any print. The message is then consumed by the + caller's ``except``. Without this line the fallback's frequency cannot be + recovered from logs at any sample size. + + The two causes are reported separately because they call for different + responses: a permission failure is a standing token-scope condition, while + a transient API error tracks a GitHub incident. + """ + cause = ( + "integration permission" + if github_resource_inaccessible(exc) + else "transient API error" + ) + print( + f"::warning::GraphQL {scope} read for {repo} fell back to REST " + f"({cause}): {exc}" + ) + + def gh_api_json(path: str) -> Any: """Run a GitHub REST API request through gh and decode the JSON response. @@ -1221,6 +1246,39 @@ def fetch_all_pr_reviews_rest(repo: str, number: int) -> list[dict[str, Any]]: return reviews +_workflow_static_names_cache: dict[tuple[str, int], str] = {} + + +def workflow_static_name(repo: str, workflow_id: int) -> str: + """Return a workflow's declared ``name:``, which ``run-name:`` never rewrites. + + A run payload's ``name`` is the *rendered* run title: when a workflow + declares ``run-name:``, GitHub substitutes it, so ``name`` carries the + pull request and head SHA rather than the workflow's identity. GraphQL's + ``workflowRun.workflow.name`` is the declared name in both cases, so + reading the workflow resource is what keeps the two paths equivalent. + + Workflow identity is immutable for the lifetime of a scheduler run, so + the lookup is cached per invocation (cleared by + :func:`reset_active_workflow_runs_cache`). A workflow the integration + cannot read yields an empty name, leaving the caller with no identity at + all so the fail-closed sentinel engages instead of a contaminated one. + """ + cache_key = (repo, workflow_id) + cached = _workflow_static_names_cache.get(cache_key) + if cached is not None: + return cached + try: + payload = gh_api_json(f"repos/{repo}/actions/workflows/{workflow_id}") + except RuntimeError as exc: + if not github_resource_inaccessible(exc): + raise + payload = {} + name = str((payload or {}).get("name") or "").strip() + _workflow_static_names_cache[cache_key] = name + return name + + def fetch_workflow_names_by_check_suite_rest( repo: str, head_sha: str ) -> dict[int, str]: @@ -1231,6 +1289,13 @@ def fetch_workflow_names_by_check_suite_rest( retain the same workflow-level policy boundary as the GraphQL path. When the integration cannot read Actions, callers receive an empty map and GitHub Actions checks are marked with a fail-closed sentinel. + + Identity comes from :func:`workflow_static_name` rather than the run's + own ``name``: a workflow declaring ``run-name:`` reports a rendered title + there, which matches none of the workflow names this module's policy + predicates compare against. Stripping a prefix off the rendered title + would only re-encode that unenforced parse contract, so a run whose + workflow cannot be identified contributes no entry. """ workflow_names: dict[int, str] = {} page = 1 @@ -1247,7 +1312,12 @@ def fetch_workflow_names_by_check_suite_rest( workflow_runs = payload.get("workflow_runs") or [] for workflow_run in workflow_runs: suite_id = workflow_run.get("check_suite_id") - workflow_name = str(workflow_run.get("name") or "").strip() + workflow_id = workflow_run.get("workflow_id") + workflow_name = ( + workflow_static_name(repo, int(workflow_id)) + if workflow_id is not None + else "" + ) if suite_id is not None and workflow_name: workflow_names[int(suite_id)] = workflow_name if len(workflow_runs) < 100: @@ -1473,6 +1543,7 @@ def fetch_open_prs( payload = gh_graphql(OPEN_PRS_QUERY, **fields) except RuntimeError as exc: if github_resource_inaccessible(exc) or is_transient_github_api_error(exc): + warn_graphql_rest_fallback(repo, "open pull request", exc) return fetch_open_prs_rest( repo, max_prs, offset=offset, window_size=window_size ) @@ -1501,6 +1572,7 @@ def fetch_pr(repo: str, number: int) -> list[dict[str, Any]]: payload = gh_graphql(PR_BY_NUMBER_QUERY, owner=owner, name=name, number=number) except RuntimeError as exc: if github_resource_inaccessible(exc) or is_transient_github_api_error(exc): + warn_graphql_rest_fallback(repo, f"pull request #{number}", exc) return fetch_pr_rest(repo, number) raise pr = payload["data"]["repository"].get("pullRequest") @@ -3105,8 +3177,15 @@ def reset_active_workflow_runs_cache() -> None: snapshot; :func:`force_cancel_workflow_runs`, :func:`rerun_actions_job`, :func:`dispatch_opencode_review`, and :func:`dispatch_strix_evidence` all do this immediately after their mutating call. + + The workflow-identity cache behind :func:`workflow_static_name` is + cleared here too. Identity never changes mid-run, so it needs no + mutation-driven invalidation; sharing this entry point simply guarantees + it is reset once per invocation without adding call sites that a later + change could forget. """ _active_workflow_runs_cache.clear() + _workflow_static_names_cache.clear() def active_workflow_runs( diff --git a/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py index 4e36544061..1186eee0b9 100644 --- a/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py +++ b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py @@ -49,10 +49,16 @@ def fake_api(path: str) -> Any: "workflow_runs": [ { "check_suite_id": 777, - "name": "Required OpenCode Review", + "workflow_id": 11, + # GitHub renders run-name: into the run's own name. + "name": ( + "Required OpenCode Review owner/repo#42@" + head_sha + ), } ] } + if path == "repos/owner/repo/actions/workflows/11": + return {"name": "Required OpenCode Review"} return payloads[path] monkeypatch.setattr(merge, "gh_api_json", fake_api) @@ -162,9 +168,10 @@ def test_fetch_workflow_names_by_check_suite_rest_paginates_past_100( """A first page of exactly 100 runs must fetch a second page and merge both.""" head_sha = "e" * 40 page1 = [ - {"check_suite_id": i, "name": f"workflow-{i}"} for i in range(100) + {"check_suite_id": i, "workflow_id": i, "name": f"rendered-{i}"} + for i in range(100) ] - page2 = [{"check_suite_id": 100, "name": "workflow-100"}] + page2 = [{"check_suite_id": 100, "workflow_id": 100, "name": "rendered-100"}] calls: list[str] = [] def fake_api(path: str) -> Any: @@ -174,14 +181,19 @@ def fake_api(path: str) -> Any: return {"workflow_runs": page1} if path.endswith("page=2"): return {"workflow_runs": page2} + prefix = "repos/owner/repo/actions/workflows/" + if path.startswith(prefix): + return {"name": f"workflow-{path[len(prefix):]}"} raise AssertionError(f"unexpected path {path}") monkeypatch.setattr(merge, "gh_api_json", fake_api) + merge.reset_active_workflow_runs_cache() names = merge.fetch_workflow_names_by_check_suite_rest("owner/repo", head_sha) assert names == {i: f"workflow-{i}" for i in range(101)} - assert calls == [ + run_list_calls = [path for path in calls if "/actions/runs?" in path] + assert run_list_calls == [ f"repos/owner/repo/actions/runs?head_sha={head_sha}&per_page=100&page=1", f"repos/owner/repo/actions/runs?head_sha={head_sha}&per_page=100&page=2", ] @@ -190,20 +202,28 @@ def fake_api(path: str) -> Any: def test_fetch_workflow_names_by_check_suite_rest_skips_entries_missing_suite_id_or_name( monkeypatch: Any, ) -> None: - """A run with no check-suite id or a blank name must not populate the map.""" + """A run missing a check-suite id, a workflow id, or a name must not populate the map.""" head_sha = "f" * 40 def fake_api(path: str) -> Any: """Return workflow runs that exercise incomplete-identity filtering.""" + if path.startswith("repos/owner/repo/actions/runs?"): + return { + "workflow_runs": [ + {"check_suite_id": None, "workflow_id": 1, "name": "orphaned"}, + {"check_suite_id": 900, "workflow_id": 2, "name": "blank"}, + {"check_suite_id": 902, "name": "no workflow id"}, + {"check_suite_id": 901, "workflow_id": 3, "name": "rendered"}, + ] + } return { - "workflow_runs": [ - {"check_suite_id": None, "name": "orphaned run"}, - {"check_suite_id": 900, "name": ""}, - {"check_suite_id": 901, "name": "kept run"}, - ] - } + "repos/owner/repo/actions/workflows/1": {"name": "orphaned run"}, + "repos/owner/repo/actions/workflows/2": {"name": ""}, + "repos/owner/repo/actions/workflows/3": {"name": "kept run"}, + }[path] monkeypatch.setattr(merge, "gh_api_json", fake_api) + merge.reset_active_workflow_runs_cache() names = merge.fetch_workflow_names_by_check_suite_rest("owner/repo", head_sha) @@ -224,3 +244,212 @@ def fake_api(path: str) -> Any: with pytest.raises(RuntimeError, match="HTTP 502"): merge.fetch_workflow_names_by_check_suite_rest("owner/repo", head_sha) + + +def test_rest_fallback_identifies_strix_behind_a_rendered_run_name( + monkeypatch: Any, +) -> None: + """A workflow declaring run-name: stays identifiable through the REST fallback. + + GitHub renders ``run-name:`` into a run's own ``name``, so the Actions run + list reports a title carrying the pull request and head SHA. Reading that + as workflow identity leaves it matching none of the declared names the + policy predicates compare against, and the fail-closed sentinel does not + engage because a name is present -- it is simply the wrong one. + """ + head_sha = "1" * 40 + payloads: dict[str, Any] = { + "repos/owner/repo/pulls/44/reviews?per_page=100&page=1": [], + f"repos/owner/repo/commits/{head_sha}/check-runs?per_page=100": { + "check_runs": [ + { + "name": "strix", + "status": "completed", + "conclusion": "success", + "started_at": "2026-09-01T05:00:00Z", + "details_url": ( + "https://github.com/owner/repo/actions/runs/900/job/901" + ), + "check_suite": {"id": 779}, + "app": {"slug": "github-actions"}, + } + ] + }, + f"repos/owner/repo/commits/{head_sha}/check-suites?per_page=100": { + "check_suites": [{"id": 779, "created_at": "2026-09-01T05:00:00Z"}] + }, + f"repos/owner/repo/commits/{head_sha}/status": {"statuses": []}, + "repos/owner/repo/pulls/44/files?per_page=20": [], + } + + def fake_api(path: str) -> Any: + """Serve a Strix run whose name has been rewritten by run-name:.""" + if path.startswith("repos/owner/repo/actions/runs?"): + return { + "workflow_runs": [ + { + "check_suite_id": 779, + "workflow_id": 55, + "name": f"Strix Security Scan owner/repo#44@{head_sha}", + } + ] + } + if path == "repos/owner/repo/actions/workflows/55": + return {"name": "Strix Security Scan"} + return payloads[path] + + monkeypatch.setattr(merge, "gh_api_json", fake_api) + merge.reset_active_workflow_runs_cache() + + pr = merge.rest_pr_node( + "owner/repo", + { + "number": 44, + "title": "REST fallback rendered run name", + "draft": False, + "mergeable": True, + "mergeable_state": "clean", + "maintainer_can_modify": True, + "auto_merge": None, + "user": {"login": "author"}, + "head": { + "ref": "feature", + "sha": head_sha, + "repo": {"full_name": "owner/repo"}, + }, + "base": {"ref": "main", "sha": "2" * 40}, + }, + ) + + context = merge.context_nodes(pr)[0] + workflow = context["checkSuite"]["workflowRun"]["workflow"] + assert workflow["name"] == "Strix Security Scan" + assert merge.is_strix_context(context) + + +def test_workflow_static_name_caches_each_workflow_once(monkeypatch: Any) -> None: + """Workflow identity is immutable per run, so it is read at most once.""" + calls: list[str] = [] + + def fake_api(path: str) -> Any: + """Count workflow-resource reads.""" + calls.append(path) + return {"name": "Strix Security Scan"} + + monkeypatch.setattr(merge, "gh_api_json", fake_api) + merge.reset_active_workflow_runs_cache() + + assert merge.workflow_static_name("owner/repo", 55) == "Strix Security Scan" + assert merge.workflow_static_name("owner/repo", 55) == "Strix Security Scan" + + assert calls == ["repos/owner/repo/actions/workflows/55"] + + +def test_workflow_static_name_caches_an_unreadable_workflow_as_no_identity( + monkeypatch: Any, +) -> None: + """A workflow the integration cannot read yields no identity, and is not re-read.""" + calls: list[str] = [] + + def fake_api(path: str) -> Any: + """Deny the workflow resource the way a scoped token does.""" + calls.append(path) + raise RuntimeError("Resource not accessible by integration") + + monkeypatch.setattr(merge, "gh_api_json", fake_api) + merge.reset_active_workflow_runs_cache() + + assert merge.workflow_static_name("owner/repo", 56) == "" + assert merge.workflow_static_name("owner/repo", 56) == "" + + assert calls == ["repos/owner/repo/actions/workflows/56"] + + +def test_workflow_static_name_propagates_non_access_errors(monkeypatch: Any) -> None: + """An unrelated REST failure must not be recorded as absent identity.""" + + def fake_api(path: str) -> Any: + """Simulate a non-access REST failure that must propagate.""" + raise RuntimeError("gh: HTTP 502 (exhausted retries)") + + monkeypatch.setattr(merge, "gh_api_json", fake_api) + merge.reset_active_workflow_runs_cache() + + with pytest.raises(RuntimeError, match="HTTP 502"): + merge.workflow_static_name("owner/repo", 57) + + +def test_reset_active_workflow_runs_cache_clears_workflow_identity( + monkeypatch: Any, +) -> None: + """The reset entry point must not leave stale identity behind for a later run.""" + names = iter(["First Name", "Second Name"]) + + def fake_api(path: str) -> Any: + """Return a different declared name on each read.""" + return {"name": next(names)} + + monkeypatch.setattr(merge, "gh_api_json", fake_api) + merge.reset_active_workflow_runs_cache() + + assert merge.workflow_static_name("owner/repo", 58) == "First Name" + merge.reset_active_workflow_runs_cache() + assert merge.workflow_static_name("owner/repo", 58) == "Second Name" + + +@pytest.mark.parametrize( + ("error", "cause"), + [ + ("Resource not accessible by integration", "integration permission"), + ("gh: HTTP 502 (exhausted retries)", "transient API error"), + ], +) +def test_warn_graphql_rest_fallback_names_the_cause( + capsys: Any, error: str, cause: str +) -> None: + """Each fallback cause is reported separately because each needs a different fix.""" + merge.warn_graphql_rest_fallback("owner/repo", "pull request #7", RuntimeError(error)) + + captured = capsys.readouterr().out + assert "::warning::GraphQL pull request #7 read for owner/repo" in captured + assert f"fell back to REST ({cause})" in captured + + +def test_fetch_pr_announces_its_rest_fallback(monkeypatch: Any, capsys: Any) -> None: + """A single-PR fallback leaves a trace; gh_graphql raises before printing one.""" + + def fake_graphql(query: str, **fields: Any) -> Any: + """Deny the GraphQL read the way a scoped token does.""" + raise RuntimeError("Resource not accessible by integration") + + monkeypatch.setattr(merge, "gh_graphql", fake_graphql) + monkeypatch.setattr(merge, "fetch_pr_rest", lambda repo, number: ["rest"]) + + assert merge.fetch_pr("owner/repo", 7) == ["rest"] + + captured = capsys.readouterr().out + assert "::warning::GraphQL pull request #7 read for owner/repo" in captured + assert "(integration permission)" in captured + + +def test_fetch_open_prs_announces_its_rest_fallback( + monkeypatch: Any, capsys: Any +) -> None: + """The queue scan's fallback is silent otherwise, and carries no pragma-covered trace.""" + + def fake_graphql(query: str, **fields: Any) -> Any: + """Fail the GraphQL read with a transient error.""" + raise RuntimeError("gh: HTTP 502 (exhausted retries)") + + monkeypatch.setattr(merge, "gh_graphql", fake_graphql) + monkeypatch.setattr( + merge, + "fetch_open_prs_rest", + lambda repo, max_prs, offset=0, window_size=None: ["rest"], + ) + + assert merge.fetch_open_prs("owner/repo", 5) == ["rest"] + + captured = capsys.readouterr().out + assert "::warning::GraphQL open pull request read for owner/repo" in captured + assert "(transient API error)" in captured From ac372a3704d53b31734cc67f6f0f0d251d14f9be Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 7 Sep 2026 03:48:59 +0900 Subject: [PATCH 2/7] test(scheduler): pin coverage evidence in the fail-open direction The contaminated workflow name does not withhold coverage evidence: the predicate is negative and the filter keeps a check when it returns False, so the REST path admits evidence GraphQL rejects. Pin that direction explicitly, since the failure reads as evidence loss otherwise. Co-Authored-By: Claude Opus 5 --- ...ew_fix_scheduler_rest_workflow_identity.py | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py index 1186eee0b9..d5f059c974 100644 --- a/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py +++ b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py @@ -453,3 +453,86 @@ def fake_graphql(query: str, **fields: Any) -> Any: captured = capsys.readouterr().out assert "::warning::GraphQL open pull request read for owner/repo" in captured assert "(transient API error)" in captured + + +def test_rest_fallback_still_excludes_non_authoritative_coverage_evidence( + monkeypatch: Any, +) -> None: + """Central metadata-only coverage evidence stays excluded behind a rendered run name. + + ``is_non_authoritative_coverage_check_run`` is a negative predicate and + ``coverage_evidence_indices`` keeps a check only when it returns False, so + a contaminated workflow name does not withhold evidence here -- it admits + evidence the GraphQL path rejects. This consumer therefore fails *open*, + which is the opposite direction from ``is_strix_context``. + """ + monkeypatch.setenv("SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", "owner/central") + head_sha = "3" * 40 + payloads: dict[str, Any] = { + "repos/owner/repo/pulls/45/reviews?per_page=100&page=1": [], + f"repos/owner/repo/commits/{head_sha}/check-runs?per_page=100": { + "check_runs": [ + { + "name": "coverage-evidence", + "status": "completed", + "conclusion": "success", + "started_at": "2026-09-01T06:00:00Z", + "details_url": ( + "https://github.com/owner/repo/actions/runs/910/job/911" + ), + "check_suite": {"id": 780}, + "app": {"slug": "github-actions"}, + } + ] + }, + f"repos/owner/repo/commits/{head_sha}/check-suites?per_page=100": { + "check_suites": [{"id": 780, "created_at": "2026-09-01T06:00:00Z"}] + }, + f"repos/owner/repo/commits/{head_sha}/status": {"statuses": []}, + "repos/owner/repo/pulls/45/files?per_page=20": [], + } + + def fake_api(path: str) -> Any: + """Serve a coverage-evidence run whose name has been rewritten by run-name:.""" + if path.startswith("repos/owner/repo/actions/runs?"): + return { + "workflow_runs": [ + { + "check_suite_id": 780, + "workflow_id": 66, + "name": ( + f"Required OpenCode Review owner/repo#45@{head_sha}" + ), + } + ] + } + if path == "repos/owner/repo/actions/workflows/66": + return {"name": "Required OpenCode Review"} + return payloads[path] + + monkeypatch.setattr(merge, "gh_api_json", fake_api) + merge.reset_active_workflow_runs_cache() + + pr = merge.rest_pr_node( + "owner/repo", + { + "number": 45, + "title": "REST fallback coverage evidence", + "draft": False, + "mergeable": True, + "mergeable_state": "clean", + "maintainer_can_modify": True, + "auto_merge": None, + "user": {"login": "author"}, + "head": { + "ref": "feature", + "sha": head_sha, + "repo": {"full_name": "owner/repo"}, + }, + "base": {"ref": "main", "sha": "4" * 40}, + }, + ) + + checks = merge.context_nodes(pr) + assert merge.is_non_authoritative_coverage_check_run(checks[0]) + assert merge.coverage_evidence_indices(checks) == [] From a665def0f8647e0aefb311f1f86a4fca1515d620 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 7 Sep 2026 03:53:51 +0900 Subject: [PATCH 3/7] fix(scheduler): report every fallback cause that applies, not the first One GraphQL failure message can satisfy both predicates: a partial failure arrives as 200 with an `errors` array, so a forbidden field and a `server error` marker can share a message. An if/else reported only the permission cause and silently under-counted transient failures in the one log this line exists to make countable. Test each predicate independently and join the labels. Only a transient failure is retried -- a permission failure matches neither retry predicate and raises on the first attempt -- so the transient label now carries that fact rather than leaving "was it retried?" open. Co-Authored-By: Claude Opus 5 --- scripts/ci/pr_review_merge_scheduler_core.py | 25 +++++++++++++------ ...ew_fix_scheduler_rest_workflow_identity.py | 21 +++++++++++++--- 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/scripts/ci/pr_review_merge_scheduler_core.py b/scripts/ci/pr_review_merge_scheduler_core.py index 0e86607bd3..14a0e255d3 100644 --- a/scripts/ci/pr_review_merge_scheduler_core.py +++ b/scripts/ci/pr_review_merge_scheduler_core.py @@ -1148,16 +1148,27 @@ def warn_graphql_rest_fallback(repo: str, scope: str, exc: RuntimeError) -> None The two causes are reported separately because they call for different responses: a permission failure is a standing token-scope condition, while - a transient API error tracks a GitHub incident. + a transient API error tracks a GitHub incident. They are tested + independently rather than as an if/else, because one message can carry + both -- GraphQL answers a partial failure with 200 and an ``errors`` + array, so a forbidden field and a ``server error`` marker can arrive + together. Reporting only the first would under-count the other exactly + where this line exists to count them. + + Only a transient failure is retried: :func:`gh_graphql` retries while + ``is_transient_github_api_error`` holds and exhausts its attempts before + raising, whereas a permission failure matches neither retry predicate and + raises on the first attempt. The transient label carries that fact so the + log does not leave "was it retried?" open. """ - cause = ( - "integration permission" - if github_resource_inaccessible(exc) - else "transient API error" - ) + causes = [] + if github_resource_inaccessible(exc): + causes.append("integration permission") + if is_transient_github_api_error(exc): + causes.append("transient API error after exhausted retries") print( f"::warning::GraphQL {scope} read for {repo} fell back to REST " - f"({cause}): {exc}" + f"({' + '.join(causes)}): {exc}" ) diff --git a/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py index d5f059c974..2c331826b9 100644 --- a/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py +++ b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py @@ -401,13 +401,26 @@ def fake_api(path: str) -> Any: ("error", "cause"), [ ("Resource not accessible by integration", "integration permission"), - ("gh: HTTP 502 (exhausted retries)", "transient API error"), + ( + "gh: HTTP 502 (bad gateway)", + "transient API error after exhausted retries", + ), + # GraphQL answers a partial failure with 200 and an `errors` array, so a + # forbidden field and a server-error marker can arrive in one message. + ( + "Resource not accessible by integration (server error while resolving)", + "integration permission + transient API error after exhausted retries", + ), ], ) -def test_warn_graphql_rest_fallback_names_the_cause( +def test_warn_graphql_rest_fallback_names_every_cause_that_applies( capsys: Any, error: str, cause: str ) -> None: - """Each fallback cause is reported separately because each needs a different fix.""" + """Both causes are reported when both hold; neither is silently dropped. + + Reporting only the first would under-count the other in exactly the log + this line exists to make countable. + """ merge.warn_graphql_rest_fallback("owner/repo", "pull request #7", RuntimeError(error)) captured = capsys.readouterr().out @@ -452,7 +465,7 @@ def fake_graphql(query: str, **fields: Any) -> Any: captured = capsys.readouterr().out assert "::warning::GraphQL open pull request read for owner/repo" in captured - assert "(transient API error)" in captured + assert "(transient API error after exhausted retries)" in captured def test_rest_fallback_still_excludes_non_authoritative_coverage_evidence( From 2c7109282ce09a89885884a0c0a13f87a9d0a59d Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 7 Sep 2026 04:05:10 +0900 Subject: [PATCH 4/7] docs(scheduler): stop calling the identity sentinel uniformly fail-closed The docstring justified the unreadable-workflow path by saying the fail-closed sentinel engages. That holds for is_strix_context, which names the sentinel and keeps the evidence, but not for is_non_authoritative_coverage_check_run: it is a negative predicate that answers True for one exact declared name, so unknown identity leaves coverage evidence admitted. The behaviour is unchanged and unregressed -- unknown identity keeps whatever polarity each consumer already had, and this function only removes the contaminated-identity case. It is the justification that was wrong. Co-Authored-By: Claude Opus 5 --- scripts/ci/pr_review_merge_scheduler_core.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/scripts/ci/pr_review_merge_scheduler_core.py b/scripts/ci/pr_review_merge_scheduler_core.py index 14a0e255d3..2b00c77929 100644 --- a/scripts/ci/pr_review_merge_scheduler_core.py +++ b/scripts/ci/pr_review_merge_scheduler_core.py @@ -1272,8 +1272,17 @@ def workflow_static_name(repo: str, workflow_id: int) -> str: Workflow identity is immutable for the lifetime of a scheduler run, so the lookup is cached per invocation (cleared by :func:`reset_active_workflow_runs_cache`). A workflow the integration - cannot read yields an empty name, leaving the caller with no identity at - all so the fail-closed sentinel engages instead of a contaminated one. + cannot read yields an empty name, so the caller records no identity and + :data:`REST_UNKNOWN_GITHUB_ACTIONS_WORKFLOW` stands in for it. + + That sentinel is not uniformly safe, and reading it as "fail-closed" + would be wrong. :func:`is_strix_context` names it explicitly and keeps + the evidence, but :func:`is_non_authoritative_coverage_check_run` is a + negative predicate that answers True only for one exact declared name -- + so unknown identity leaves coverage evidence *admitted*, exactly as an + unreadable workflow already did before identity was resolved here. This + function removes the contaminated-identity case; the unknown-identity + case keeps whatever polarity each consumer already had. """ cache_key = (repo, workflow_id) cached = _workflow_static_names_cache.get(cache_key) From 009834b9d877149115ca79ec1bac2a01c190a24f Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 7 Sep 2026 07:32:38 +0900 Subject: [PATCH 5/7] test(scheduler): reproduce workflow identity lookup races --- ...ew_fix_scheduler_rest_workflow_identity.py | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py index 2c331826b9..853ea1ba18 100644 --- a/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py +++ b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py @@ -2,6 +2,8 @@ from __future__ import annotations +import concurrent.futures +import threading from typing import Any import pytest @@ -365,6 +367,87 @@ def fake_api(path: str) -> Any: assert calls == ["repos/owner/repo/actions/workflows/56"] +def test_workflow_static_name_caches_a_deleted_workflow_as_no_identity( + monkeypatch: Any, +) -> None: + """A deleted workflow is absent identity, while unrelated failures propagate.""" + calls: list[str] = [] + + def fake_api(path: str) -> Any: + """Return the exact 404 shape emitted by ``gh api`` for a deleted workflow.""" + calls.append(path) + raise RuntimeError("gh: Not Found (HTTP 404)") + + monkeypatch.setattr(merge, "gh_api_json", fake_api) + merge.reset_active_workflow_runs_cache() + + assert merge.workflow_static_name("owner/repo", 59) == "" + assert merge.workflow_static_name("owner/repo", 59) == "" + + assert calls == ["repos/owner/repo/actions/workflows/59"] + + +def test_workflow_static_name_coalesces_concurrent_reads_per_workflow( + monkeypatch: Any, +) -> None: + """Concurrent REST hydration performs one immutable workflow-resource read.""" + worker_count = 12 + start = threading.Barrier(worker_count) + first_read = threading.Event() + release_read = threading.Event() + call_count = 0 + count_lock = threading.Lock() + + def fake_api(path: str) -> Any: + """Hold the first resource read while competing callers reach the cache.""" + nonlocal call_count + with count_lock: + call_count += 1 + first_read.set() + assert release_read.wait(timeout=5) + return {"name": "Strix Security Scan"} + + def read_name() -> str: + """Release all callers into the same empty-cache window.""" + start.wait(timeout=5) + return merge.workflow_static_name("owner/repo", 60) + + monkeypatch.setattr(merge, "gh_api_json", fake_api) + merge.reset_active_workflow_runs_cache() + + with concurrent.futures.ThreadPoolExecutor(max_workers=worker_count) as executor: + futures = [executor.submit(read_name) for _ in range(worker_count)] + assert first_read.wait(timeout=5) + release_read.set() + assert [future.result(timeout=5) for future in futures] == [ + "Strix Security Scan" + ] * worker_count + + assert call_count == 1 + + +def test_workflow_static_name_keeps_different_workflow_reads_parallel( + monkeypatch: Any, +) -> None: + """Single-flight locking for one workflow does not serialize other identities.""" + concurrent_reads = threading.Barrier(2) + + def fake_api(path: str) -> Any: + """Both different workflow resources must enter before either can return.""" + concurrent_reads.wait(timeout=5) + return {"name": path.rsplit("/", 1)[-1]} + + monkeypatch.setattr(merge, "gh_api_json", fake_api) + merge.reset_active_workflow_runs_cache() + + with concurrent.futures.ThreadPoolExecutor(max_workers=2) as executor: + futures = [ + executor.submit(merge.workflow_static_name, "owner/repo", workflow_id) + for workflow_id in (61, 62) + ] + assert [future.result(timeout=5) for future in futures] == ["61", "62"] + + def test_workflow_static_name_propagates_non_access_errors(monkeypatch: Any) -> None: """An unrelated REST failure must not be recorded as absent identity.""" From 86156b00d3318e597e114135596d0beec88ed068 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 7 Sep 2026 07:32:39 +0900 Subject: [PATCH 6/7] fix(scheduler): coalesce workflow identity reads --- scripts/ci/pr_review_merge_scheduler_core.py | 33 ++++++++++++++------ 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/scripts/ci/pr_review_merge_scheduler_core.py b/scripts/ci/pr_review_merge_scheduler_core.py index 2b00c77929..ac3651df9d 100644 --- a/scripts/ci/pr_review_merge_scheduler_core.py +++ b/scripts/ci/pr_review_merge_scheduler_core.py @@ -13,6 +13,7 @@ import subprocess import sys import tempfile +import threading import time from collections.abc import Iterator, Sequence from dataclasses import dataclass @@ -1258,6 +1259,8 @@ def fetch_all_pr_reviews_rest(repo: str, number: int) -> list[dict[str, Any]]: _workflow_static_names_cache: dict[tuple[str, int], str] = {} +_workflow_static_name_locks: dict[tuple[str, int], threading.Lock] = {} +_workflow_static_name_locks_guard = threading.Lock() def workflow_static_name(repo: str, workflow_id: int) -> str: @@ -1288,15 +1291,23 @@ def workflow_static_name(repo: str, workflow_id: int) -> str: cached = _workflow_static_names_cache.get(cache_key) if cached is not None: return cached - try: - payload = gh_api_json(f"repos/{repo}/actions/workflows/{workflow_id}") - except RuntimeError as exc: - if not github_resource_inaccessible(exc): - raise - payload = {} - name = str((payload or {}).get("name") or "").strip() - _workflow_static_names_cache[cache_key] = name - return name + with _workflow_static_name_locks_guard: + cache_lock = _workflow_static_name_locks.setdefault(cache_key, threading.Lock()) + with cache_lock: + cached = _workflow_static_names_cache.get(cache_key) + if cached is not None: + return cached + try: + payload = gh_api_json(f"repos/{repo}/actions/workflows/{workflow_id}") + except RuntimeError as exc: + if not ( + github_resource_inaccessible(exc) or "HTTP 404" in str(exc) + ): + raise + payload = {} + name = str((payload or {}).get("name") or "").strip() + _workflow_static_names_cache[cache_key] = name + return name def fetch_workflow_names_by_check_suite_rest( @@ -3203,7 +3214,9 @@ def reset_active_workflow_runs_cache() -> None: change could forget. """ _active_workflow_runs_cache.clear() - _workflow_static_names_cache.clear() + with _workflow_static_name_locks_guard: + _workflow_static_names_cache.clear() + _workflow_static_name_locks.clear() def active_workflow_runs( From 4604909a9b68cb29cda431d71bc0ed3d37f11af3 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 7 Sep 2026 07:34:12 +0900 Subject: [PATCH 7/7] test(scheduler): pin deleted workflow map exclusion --- ...ew_fix_scheduler_rest_workflow_identity.py | 36 +++++++++++++++++-- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py index 853ea1ba18..9ee2391485 100644 --- a/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py +++ b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py @@ -232,6 +232,28 @@ def fake_api(path: str) -> Any: assert names == {901: "kept run"} +def test_fetch_workflow_names_by_check_suite_rest_skips_a_deleted_workflow( + monkeypatch: Any, +) -> None: + """A deleted workflow contributes no check-suite identity to the result map.""" + head_sha = "9" * 40 + + def fake_api(path: str) -> Any: + """Return one run whose immutable workflow resource was deleted.""" + if "/actions/runs?" in path: + return { + "workflow_runs": [ + {"check_suite_id": 903, "workflow_id": 4, "name": "rendered"} + ] + } + raise RuntimeError("gh: Not Found (HTTP 404)") + + monkeypatch.setattr(merge, "gh_api_json", fake_api) + merge.reset_active_workflow_runs_cache() + + assert merge.fetch_workflow_names_by_check_suite_rest("owner/repo", head_sha) == {} + + def test_fetch_workflow_names_by_check_suite_rest_propagates_non_access_errors( monkeypatch: Any, ) -> None: @@ -401,16 +423,24 @@ def test_workflow_static_name_coalesces_concurrent_reads_per_workflow( def fake_api(path: str) -> Any: """Hold the first resource read while competing callers reach the cache.""" nonlocal call_count + if "/actions/runs?" in path: + return { + "workflow_runs": [ + {"check_suite_id": 904, "workflow_id": 60, "name": "rendered"} + ] + } with count_lock: call_count += 1 first_read.set() assert release_read.wait(timeout=5) return {"name": "Strix Security Scan"} - def read_name() -> str: + def read_name() -> dict[int, str]: """Release all callers into the same empty-cache window.""" start.wait(timeout=5) - return merge.workflow_static_name("owner/repo", 60) + return merge.fetch_workflow_names_by_check_suite_rest( + "owner/repo", "8" * 40 + ) monkeypatch.setattr(merge, "gh_api_json", fake_api) merge.reset_active_workflow_runs_cache() @@ -420,7 +450,7 @@ def read_name() -> str: assert first_read.wait(timeout=5) release_read.set() assert [future.result(timeout=5) for future in futures] == [ - "Strix Security Scan" + {904: "Strix Security Scan"} ] * worker_count assert call_count == 1