From d781aca0b7170917103ab0e7138734579bdfd172 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sat, 5 Sep 2026 15:46:25 +0900 Subject: [PATCH 1/7] fix(actions): isolate review reruns from current PR evidence --- .github/workflows/noema-review.yml | 4 +- .../workflows/opencode-review-dispatch.yml | 1 + .github/workflows/opencode-review.yml | 16 +-- .github/workflows/strix.yml | 19 ++- .../review-rerun-concurrency-isolation.md | 73 ++++++++++ docs/product-technical-gap-baseline.md | 6 + tests/test_noema_review_gate.py | 11 +- tests/test_pr_review_merge_scheduler.py | 23 +++- tests/test_review_rerun_concurrency.py | 128 ++++++++++++++++++ 9 files changed, 247 insertions(+), 34 deletions(-) create mode 100644 docs/doctoring/review-rerun-concurrency-isolation.md create mode 100644 tests/test_review_rerun_concurrency.py diff --git a/.github/workflows/noema-review.yml b/.github/workflows/noema-review.yml index 21ea967201..3422b8e10a 100644 --- a/.github/workflows/noema-review.yml +++ b/.github/workflows/noema-review.yml @@ -15,12 +15,12 @@ on: types: [noema-review] concurrency: - # Workflow-level admission is required: a queued run cannot reach a job-level - # cancellation guard while the organization is at its Actions job ceiling. + # Coalesce entire first-attempt runs; isolate old retries before admission. group: >- required-noema-review-${{ github.event.pull_request.base.repo.full_name || github.event.client_payload.target_repository || github.repository }}-${{ + github.run_attempt > 1 && format('rerun-{0}', github.run_id) || github.event.pull_request.number || github.event.client_payload.pr_number || github.run_id }} cancel-in-progress: true diff --git a/.github/workflows/opencode-review-dispatch.yml b/.github/workflows/opencode-review-dispatch.yml index ade10b37c4..d426bb0c2e 100644 --- a/.github/workflows/opencode-review-dispatch.yml +++ b/.github/workflows/opencode-review-dispatch.yml @@ -2294,6 +2294,7 @@ jobs: group: >- opencode-review-${{ needs.validate-pr-metadata.outputs.target_repository }}-${{ + github.run_attempt > 1 && format('rerun-{0}', github.run_id) || needs.validate-pr-metadata.outputs.pr_number || github.run_id }} cancel-in-progress: true runs-on: ubuntu-24.04 diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 4494e74090..9d5cd94305 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -17,11 +17,12 @@ on: types: [opened, synchronize, reopened, ready_for_review, converted_to_draft, closed] concurrency: - # Coalesce before runner admission. The live-head job and scheduler still - # reject or replace a delayed stale event after native queue cancellation. + # Coalesce first attempts. Exact-run receipt wakeups and manual reruns use a + # separate group, then revalidate the live head before dispatch/publication. group: >- required-opencode-review-${{ github.event.pull_request.base.repo.full_name || github.repository }}-${{ + github.run_attempt > 1 && format('rerun-{0}', github.run_id) || github.event.pull_request.number || github.run_id }} cancel-in-progress: true @@ -491,14 +492,9 @@ jobs: echo "Current-head OpenCode verdict: ${verdict}." cancel-superseded-opencode-review-runs: - # This job -- not the bootstrap concurrency group above -- is the primary - # mechanism that actively cancels a same-PR run for an outdated head. The - # bootstrap group is now `cancel-in-progress: false` (see its own comment): - # nothing is ever preempted there, by design, to structurally close the - # #1568 stale-cancels-fresh race regardless of arrival order. This job - # achieves precise, safe "cancel only outdated runs of the same PR" - # instead: it re-verifies the live PR head immediately before selecting - # candidates AND immediately before every individual cancellation call, so + # Native concurrency coalesces first attempts; this cleanup also reaches + # outdated runs isolated in rerun groups. It re-verifies the live PR before + # selecting candidates AND before every individual cancellation call, so # a cleanup run that is itself delayed/stale cannot cancel a # still-authoritative run, and it only ever targets runs whose recorded # head no longer matches the live one. The target job also revalidates the diff --git a/.github/workflows/strix.yml b/.github/workflows/strix.yml index 58ed3dab8d..80f4cdc634 100644 --- a/.github/workflows/strix.yml +++ b/.github/workflows/strix.yml @@ -42,15 +42,10 @@ on: # repositories ruleset 18156473 excludes (.github, noema, # IRT-bibliography-set). In every other repository the ruleset ignores # them, so the same doc/image-only decision is enforced by the - # changed-scope job below. The run-name - # includes the PR number and head SHA for status grouping, while the - # concurrency group is scoped per repository and event class to prevent - # shared-provider key rate-limit storms. Strix runs intentionally do not - # cancel in progress because a pre-job cancellation leaves no scanner log to - # review. GitHub keeps one active and one pending run per group; the merge - # scheduler re-dispatches exact-head evidence when a pending run is - # superseded. For PRs the merge scheduler manages, same-head Strix evidence - # is still forced at merge time via repository_dispatch (which paths-ignore + # changed-scope job below. First attempts coalesce by repository and PR; + # reruns use a separate run-ID group so an old retry cannot cancel a newer + # run before live-head admission. For PRs the merge scheduler manages, Strix + # evidence is still forced at merge time via repository_dispatch (which paths-ignore # does not affect), so merged code never loses evidence. paths-ignore: - '**/*.md' @@ -75,12 +70,13 @@ on: types: [strix-scan] concurrency: - # Workflow-level admission is required: job-level groups are never evaluated - # while the whole run is queued behind the organization job ceiling. + # Coalesce entire first-attempt runs, including their bootstrap jobs. + # Old reruns must not displace current evidence before live-head admission. group: >- strix-security-scan-${{ github.event.pull_request.base.repo.full_name || github.event.client_payload.target_repository || github.repository }}-${{ + github.run_attempt > 1 && format('rerun-{0}', github.run_id) || github.event.pull_request.number || github.event.client_payload.pr_number || github.run_id }} cancel-in-progress: true @@ -231,6 +227,7 @@ jobs: group: >- cancel-superseded-pr-runs-${{ github.event.pull_request.base.repo.full_name || github.repository }}-${{ + github.run_attempt > 1 && format('rerun-{0}', github.run_id) || github.event.pull_request.number || github.run_id }} cancel-in-progress: true runs-on: ubuntu-24.04 diff --git a/docs/doctoring/review-rerun-concurrency-isolation.md b/docs/doctoring/review-rerun-concurrency-isolation.md new file mode 100644 index 0000000000..a8364cbd20 --- /dev/null +++ b/docs/doctoring/review-rerun-concurrency-isolation.md @@ -0,0 +1,73 @@ +# Review rerun concurrency isolation + +## Cause and scope + +G-02/G-03 follow-up, inspected on 2026-09-05 against central main +`6d7fbebec8aec31d88a30a36e71ca5b3925d241d`. This is a proposed repair, not +protected-main or hosted-runtime evidence. Procedure documentation is tracked +separately in #1885. + +GitHub reruns retain their run ID and increment `github.run_attempt`. A rerun +of an older PR event therefore re-enters the same cancellable PR group unless +the expression distinguishes attempts. It can cancel current evidence before +the stale-head guard executes. Required OpenCode's receipt wakeup intentionally +uses `rerun-failed-jobs`, so disabling reruns would break its existing flow. + +The five cancellable groups in Strix, Noema, Required OpenCode, Strix cleanup, +and OpenCode dispatch now use `rerun-` for attempts greater than one. +First attempts retain the workflow/repository/PR key and cancellation policy. +No jobs, dependencies, permissions, provider routes, or gate exceptions are +added. Live-head admission and publication checks remain mandatory. + +## Checks + +`tests/test_review_rerun_concurrency.py` evaluates the actual group expressions +with a restricted stdlib AST interpreter, without `eval`, workflow execution, +or a new dependency. Before the workflow edits, 17 assertions failed and five +passed. The tests cover numeric/string attempts, distinct retries, first-push +coalescing, repository/PR isolation, non-PR fallback, and native/dispatch parity. +Existing Noema and central dispatch cleanup tests also exercise retry metadata; +they prove selection and API requests, not GitHub terminal cancellation. + +Run the focused suite: + +```sh +uv run pytest -q tests/test_review_rerun_concurrency.py tests/test_required_workflow_queue_contract.py tests/test_noema_orchestrator_workflow_contract.py tests/test_opencode_required_rerun_capacity.py tests/test_opencode_required_verdict_regression.py tests/test_strix_rerun_job_selection.py tests/test_current_head_run_coalescer.py tests/test_opencode_live_draft_state_regression.py tests/test_noema_review_gate.py tests/test_pr_review_merge_scheduler.py tests/test_pr1669_cancel_stale_opencode_runs.py +``` + +Local actionlint 1.7.12 hung writing large shell input before starting its child +process. A diagnostic stack matched upstream `process.go`; this failed run is +not passing evidence. The additional `-shellcheck= -pyflakes=` invocation +passed workflow syntax/expression validation only, not external lint. No hosted +gate was changed or disabled. + +## Remaining limits and acceptance + +- A new first attempt cannot natively cancel an isolated historical retry. + Existing Strix/Required OpenCode cleanup jobs and Noema's in-job cleanup + require a runner. Adding another cleanup job would not guarantee service + under runner saturation. +- Central OpenCode dispatch retries are considered by the existing scheduler's + `dispatch_opencode_review` cleanup, not Required OpenCode's local cleanup. + This requires the scheduler to reach that branch; Strix waits, credential + waits, and earlier returns can leave a retry active. Immediate recovery of + every old retry is not claimed. +- Delayed *first* attempts still share a cancellable key; this repair does not + establish chronological scheduling or solve that separate arrival-order race. +- Older runs reuse their original workflow revision. After protected merge, + create fresh run evidence under the new revision and verify that rerunning + its older PR event preserves the newer run. Observe any cleanup candidate + reach `completed/cancelled`; an accepted cancel request is insufficient. +- Hosted lint/security checks, independent exact-head approval, protected merge, + and the live probe remain required. The 41-item objective completion count + does not increase from these local checks alone. + +## Sources + +- GitHub. (n.d.). [Contexts reference](https://docs.github.com/en/actions/reference/workflows-and-actions/contexts). +- GitHub. (n.d.). [Control workflow concurrency](https://docs.github.com/en/actions/how-tos/write-workflows/choose-when-workflows-run/control-workflow-concurrency). +- GitHub. (n.d.). [Re-running workflows and jobs](https://docs.github.com/en/actions/how-tos/manage-workflow-runs/re-run-workflows-and-jobs). +- rhysd. (n.d.). [actionlint v1.7.12 process runner](https://github.com/rhysd/actionlint/blob/v1.7.12/process.go#L23-L41). + +Retrieved 2026-09-05. These platform/tool sources ground a configuration bug; +no research-paper PDF is needed for this bounded repair. diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index b6ccc8f8b3..5b705bfb4d 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -76,6 +76,12 @@ flowchart LR 우선순위는 구매자 체감, 보안/증거 위험, 선행 의존성 순서다. +2026-09-05 G-02/G-03 follow-up: [review rerun isolation](doctoring/review-rerun-concurrency-isolation.md) +reproduces an older retry sharing the current PR's cancellation key. The proposed +five-group repair preserves first-attempt coalescing and existing cleanup guards. +Immediate stale-retry recovery under runner saturation and protected hosted +delivery remain unverified; this does not close either gap or the full objective. + | Gap ID | 현재 관측 | 구매자 영향 | 우선 구현/검증 | |---|---|---|---| | G-01 | 열린 PR은 107개다. metadata 상태는 BLOCKED=17, BEHIND=16, DIRTY=74, draft 13개다. 상태는 independent exact-head approval과 terminal required Checks를 자동으로 의미하지 않는다 | 안전하게 출시할 변경과 대기 중인 변경을 구별할 수 없다 | PR마다 current head, reviews, threads, required Checks, merge-result tree를 재수집하고 보호 조건 미충족이면 merge하지 않는다 | diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 6422ae5012..b5a059ecdb 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -323,14 +323,17 @@ def _superseded_cleanup_script() -> str: ) -def test_superseded_cleanup_preserves_current_and_newer_run_ids(tmp_path: Path) -> None: +@pytest.mark.parametrize("run_attempt", [1, 2]) +def test_superseded_cleanup_preserves_current_and_newer_run_ids( + tmp_path: Path, run_attempt: int, +) -> None: """Execute cleanup and cancel only the same PR's older, different-head run.""" current_head = "b" * 40 workflow_path = ".github/workflows/noema-review.yml" runs = {"workflow_runs": [ - {"id": 100, "path": workflow_path, "name": "Required Noema Review", "display_title": "Required Noema Review ContextualWisdomLab/example#7@" + "a" * 40}, - {"id": 199, "path": workflow_path, "name": "Required Noema Review", "display_title": "Required Noema Review ContextualWisdomLab/example#7@" + current_head}, - {"id": 201, "path": workflow_path, "name": "Required Noema Review", "display_title": "Required Noema Review ContextualWisdomLab/example#7@" + "c" * 40}, + {"id": 100, "run_attempt": run_attempt, "path": workflow_path, "name": "Required Noema Review", "display_title": "Required Noema Review ContextualWisdomLab/example#7@" + "a" * 40}, + {"id": 199, "run_attempt": run_attempt, "path": workflow_path, "name": "Required Noema Review", "display_title": "Required Noema Review ContextualWisdomLab/example#7@" + current_head}, + {"id": 201, "run_attempt": run_attempt, "path": workflow_path, "name": "Required Noema Review", "display_title": "Required Noema Review ContextualWisdomLab/example#7@" + "c" * 40}, {"id": 99, "path": workflow_path, "name": "Required Noema Review", "display_title": "Required Noema Review ContextualWisdomLab/example#8@" + "a" * 40}, ]} fixture = tmp_path / "runs.json" diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 4a13bd3bf1..0f8b7132f5 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -5831,7 +5831,14 @@ def fake_run(args, stdin=None): assert responses == [] # every canned response was consumed: no call was skipped or reused -def test_dispatch_strix_cancels_stale_central_run_and_keeps_current(monkeypatch, capsys): +@pytest.mark.parametrize("run_attempt", [1, 2]) +@pytest.mark.parametrize("workflow_name,dispatch", [ + ("Strix Security Scan", sched.dispatch_strix_evidence), + ("OpenCode Review Dispatch", sched.dispatch_opencode_review), +]) +def test_dispatch_review_cancels_stale_central_run_and_keeps_current( + monkeypatch, capsys, workflow_name, dispatch, run_attempt, +): monkeypatch.setattr(sched, "_review_run_still_superseded", lambda *_args: True) calls = [] head_sha = "a" * 40 @@ -5840,18 +5847,20 @@ def test_dispatch_strix_cancels_stale_central_run_and_keeps_current(monkeypatch, central_runs = [ { "id": 9300, - "name": "Strix Security Scan", + "run_attempt": run_attempt, + "name": workflow_name, "event": "repository_dispatch", "head_sha": "default-branch-sha", - "display_title": f"Strix Security Scan owner/repo#1@{stale_sha}", + "display_title": f"{workflow_name} owner/repo#1@{stale_sha}", "pull_requests": [], }, { "id": 9301, - "name": "Strix Security Scan", + "run_attempt": run_attempt, + "name": workflow_name, "event": "repository_dispatch", "head_sha": "default-branch-sha", - "display_title": f"Strix Security Scan owner/repo#1@{head_sha}", + "display_title": f"{workflow_name} owner/repo#1@{head_sha}", "pull_requests": [], }, ] @@ -5880,9 +5889,9 @@ def fake_run(args, stdin=None): "ContextualWisdomLab/.github", ) - result = sched.dispatch_strix_evidence( + result = dispatch( "owner/repo", - "Strix Security Scan", + workflow_name, make_pr(baseRefOid=base_sha, headRefOid=head_sha), dry_run=False, ) diff --git a/tests/test_review_rerun_concurrency.py b/tests/test_review_rerun_concurrency.py new file mode 100644 index 0000000000..50d8065644 --- /dev/null +++ b/tests/test_review_rerun_concurrency.py @@ -0,0 +1,128 @@ +"""Evaluate the real review group expressions without executing workflow code.""" + +import ast +import re +from pathlib import Path + +import pytest + + +WORKFLOWS = Path(__file__).resolve().parents[1] / ".github" / "workflows" +REVIEW_GROUPS = ( + ("strix.yml", None, "strix-security-scan"), + ("noema-review.yml", None, "required-noema-review"), + ("opencode-review.yml", None, "required-opencode-review"), + ("strix.yml", "cancel-superseded-pr-runs", "cancel-superseded-pr-runs"), + ("opencode-review-dispatch.yml", "opencode-review-target", "opencode-review"), +) + + +def expression_value(node: ast.AST, context: dict): + """Interpret only this contract's literals, lookups, booleans, > and format.""" + if isinstance(node, ast.Constant): + return node.value + if isinstance(node, ast.Name): + return context[node.id] + if isinstance(node, ast.Attribute): + parent = expression_value(node.value, context) + return parent.get(node.attr, "") if isinstance(parent, dict) else "" + if isinstance(node, ast.BoolOp): + for operand in node.values: + value = expression_value(operand, context) + if isinstance(node.op, ast.Or) and value: + return value + if isinstance(node.op, ast.And) and not value: + return value + return value + if isinstance(node, ast.Compare) and len(node.ops) == 1: + assert isinstance(node.ops[0], ast.Gt), "unsupported comparison" + # GitHub coerces numeric strings for relational comparisons. + return float(expression_value(node.left, context)) > float( + expression_value(node.comparators[0], context) + ) + if isinstance(node, ast.Call): + assert isinstance(node.func, ast.Name) and node.func.id == "format" + assert not node.keywords and isinstance(node.args[0], ast.Constant) + return node.args[0].value.format( + *(expression_value(argument, context) for argument in node.args[1:]) + ) + raise AssertionError(f"unsupported group expression: {ast.dump(node)}") + + +def review_group(filename, job, *, run_id, attempt=1, pr=7, + repository="ContextualWisdomLab/example", dispatched=False): + """Render the declared YAML group using an explicit event/needs snapshot.""" + source = (WORKFLOWS / filename).read_text(encoding="utf-8") + if job: + source = source.split(f"\n {job}:\n", 1)[1] + marker = "\n concurrency:\n" + else: + marker = "\nconcurrency:\n" + policy = source.split(marker, 1)[1] + group, cancellation = policy.split("group: >-\n", 1)[1].split( + "cancel-in-progress:", 1 + ) + assert cancellation.splitlines()[0].strip() == "true" + event = ( + {"client_payload": {"target_repository": repository, "pr_number": str(pr)}} + if dispatched else + {"pull_request": {"base": {"repo": {"full_name": repository}}, "number": pr}} + ) if pr else {} + context = { + "github": {"repository": repository, "run_id": str(run_id), + "run_attempt": attempt, "event": event}, + "needs": {"validate-pr-metadata": {"outputs": { + "target_repository": repository, "pr_number": str(pr) if pr else "", + }}}, + } + + def render(match): + expression = match[1].strip().replace("&&", " and ").replace("||", " or ") + # Hyphens in GitHub property names are dictionary keys, not subtraction. + expression = expression.replace("needs.validate-pr-metadata", "metadata") + tree = ast.parse(expression, mode="eval") + return str(expression_value(tree.body, { + **context, "metadata": context["needs"]["validate-pr-metadata"], + })) + + return re.sub(r"\$\{\{(.*?)\}\}", render, " ".join(group.split())) + + +@pytest.mark.parametrize("filename,job,prefix", REVIEW_GROUPS) +@pytest.mark.parametrize("attempt", [2, "2", 3]) +def test_old_rerun_cannot_cancel_current_pr_run(filename, job, prefix, attempt): + """An older run retry must not share the current run's cancellation group.""" + current = review_group(filename, job, run_id=202) + old_retry = review_group(filename, job, run_id=101, attempt=attempt) + assert current == f"{prefix}-ContextualWisdomLab/example-7" + assert old_retry == f"{prefix}-ContextualWisdomLab/example-rerun-101" + assert old_retry != current + assert old_retry != review_group(filename, job, run_id=202, attempt=2) + + +@pytest.mark.parametrize("filename,job,prefix", REVIEW_GROUPS) +def test_new_push_still_coalesces_only_its_pr(filename, job, prefix): + """Run-ID isolation must not disable normal same-PR first-attempt cleanup.""" + current = review_group(filename, job, run_id=202) + assert current == review_group(filename, job, run_id=101, attempt="1") + assert current != review_group(filename, job, run_id=203, pr=8) + assert current != review_group( + filename, job, run_id=204, repository="ContextualWisdomLab/other" + ) + assert review_group(filename, job, run_id=205, pr=None) != review_group( + filename, job, run_id=206, pr=None + ) + + +@pytest.mark.parametrize("filename,prefix", [ + ("strix.yml", "strix-security-scan"), + ("noema-review.yml", "required-noema-review"), +]) +def test_dispatched_reviews_keep_pr_identity_and_rerun_isolation(filename, prefix): + """Native and dispatched first attempts coalesce, but old dispatch retries do not.""" + assert review_group(filename, None, run_id=202, dispatched=True) == ( + f"{prefix}-ContextualWisdomLab/example-7" + ) + assert review_group(filename, None, run_id=101, attempt=2, dispatched=True) == ( + f"{prefix}-ContextualWisdomLab/example-rerun-101" + ) From 2000b4f0c892b95f2609ea956b5266218a511fe3 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sat, 5 Sep 2026 15:47:48 +0900 Subject: [PATCH 2/7] test(actions): exercise the central OpenCode workflow alias --- tests/test_pr_review_merge_scheduler.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 0f8b7132f5..985b43c570 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -5832,12 +5832,12 @@ def fake_run(args, stdin=None): @pytest.mark.parametrize("run_attempt", [1, 2]) -@pytest.mark.parametrize("workflow_name,dispatch", [ - ("Strix Security Scan", sched.dispatch_strix_evidence), - ("OpenCode Review Dispatch", sched.dispatch_opencode_review), +@pytest.mark.parametrize("workflow_name,run_name,dispatch", [ + ("Strix Security Scan", "Strix Security Scan", sched.dispatch_strix_evidence), + ("Required OpenCode Review", "OpenCode Review Dispatch", sched.dispatch_opencode_review), ]) def test_dispatch_review_cancels_stale_central_run_and_keeps_current( - monkeypatch, capsys, workflow_name, dispatch, run_attempt, + monkeypatch, capsys, workflow_name, run_name, dispatch, run_attempt, ): monkeypatch.setattr(sched, "_review_run_still_superseded", lambda *_args: True) calls = [] @@ -5848,19 +5848,19 @@ def test_dispatch_review_cancels_stale_central_run_and_keeps_current( { "id": 9300, "run_attempt": run_attempt, - "name": workflow_name, + "name": run_name, "event": "repository_dispatch", "head_sha": "default-branch-sha", - "display_title": f"{workflow_name} owner/repo#1@{stale_sha}", + "display_title": f"{run_name} owner/repo#1@{stale_sha}", "pull_requests": [], }, { "id": 9301, "run_attempt": run_attempt, - "name": workflow_name, + "name": run_name, "event": "repository_dispatch", "head_sha": "default-branch-sha", - "display_title": f"{workflow_name} owner/repo#1@{head_sha}", + "display_title": f"{run_name} owner/repo#1@{head_sha}", "pull_requests": [], }, ] From 51345839f78d269ecf192aa386f1679508581a32 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sat, 5 Sep 2026 16:17:23 +0900 Subject: [PATCH 3/7] =?UTF-8?q?fix(ci):=20=EC=98=A4=EB=9E=98=EB=90=9C=20?= =?UTF-8?q?=EC=8B=A4=ED=96=89=EC=9D=98=20=EC=B5=9C=EC=8B=A0=20=EA=B2=80?= =?UTF-8?q?=EC=A6=9D=20=EC=B7=A8=EC=86=8C=EB=A5=BC=20=EC=B0=A8=EB=8B=A8?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../workflows/opencode-review-dispatch.yml | 67 +++++++++++++++++-- docs/doctoring/current-head-run-coalescing.md | 32 ++++++++- .../review-rerun-concurrency-isolation.md | 32 ++++++++- docs/product-technical-gap-baseline.md | 4 ++ scripts/ci/current_head_run_coalescer.py | 7 +- tests/test_current_head_run_coalescer.py | 47 +++++++++++-- ...t_head_run_coalescer_review_regressions.py | 6 +- tests/test_opencode_workflow_shell_syntax.py | 43 ++++++++++++ 8 files changed, 214 insertions(+), 24 deletions(-) diff --git a/.github/workflows/opencode-review-dispatch.yml b/.github/workflows/opencode-review-dispatch.yml index d426bb0c2e..fec6de078c 100644 --- a/.github/workflows/opencode-review-dispatch.yml +++ b/.github/workflows/opencode-review-dispatch.yml @@ -630,7 +630,7 @@ jobs: changed_basename="${changed_path##*/}" case "$changed_basename" in *.py|pyproject.toml|uv.lock|poetry.lock|Pipfile|Pipfile.lock|\ - setup.py|setup.cfg|tox.ini|pytest.ini|.pytest.ini|\ + setup.cfg|tox.ini|pytest.ini|.pytest.ini|\ requirements.lock|requirements*.txt|requirements*.in|\ environment.yml|environment.yaml) python_coverage_required=1 @@ -946,9 +946,11 @@ jobs: } append_command() { - printf '$ ' >>"$summary_file" - printf '%q ' "$@" >>"$summary_file" - printf '\n' >>"$summary_file" + { + printf '$ ' + printf '%q ' "$@" + printf '\n' + } >>"$summary_file" } emit_captured_log() { @@ -1221,6 +1223,8 @@ jobs: --command-json "$configured_command_json" done <<<"$configured_commands_json" else + # Positional parameters expand in the child bash. + # shellcheck disable=SC2016 run_and_capture "Python coverage with missing-line report (${project_dir})" \ bash -c 'cd "$1" && PYTHONPATH="$([ -d src ] && printf src:. || printf .)" python3 -m coverage run -m pytest tests && python3 -m coverage report --show-missing' bash "$project_dir" fi @@ -1228,6 +1232,8 @@ jobs: if [ "$measured_projects" -eq 0 ]; then if has_tracked_files '*.py'; then + # Command substitution expands in the child bash. + # shellcheck disable=SC2016 run_and_capture "Python coverage with missing-line report" \ bash -c 'PYTHONPATH="$([ -d src ] && printf src:. || printf .)" python3 -m coverage run -m pytest && python3 -m coverage report --show-missing' elif python3 -I -c 'import pytest_cov' >/dev/null 2>&1; then @@ -1379,6 +1385,8 @@ jobs: while IFS= read -r project_dir; do if [ -f "${project_dir}/tests/test_docstrings.py" ]; then measured_projects=1 + # Positional parameters expand in the child bash. + # shellcheck disable=SC2016 run_and_capture "Python docstring coverage (${project_dir})" \ bash -c 'cd "$1" && PYTHONPATH="$([ -d src ] && printf src:. || printf .)" python3 -m pytest tests/test_docstrings.py' bash "$project_dir" fi @@ -1748,6 +1756,8 @@ jobs: if [ -n "$package_name" ] && jq -e '.workspaces // empty' package.json >/dev/null 2>&1; then run_and_capture "Tauri frontendDist build (${package_dir})" corepack npm run build --workspace "$package_name" else + # Positional parameters expand in the child bash. + # shellcheck disable=SC2016 run_and_capture "Tauri frontendDist build (${package_dir})" bash -c 'cd "$1" && corepack npm run build' bash "$package_dir" fi ;; @@ -1755,6 +1765,8 @@ jobs: if [ -n "$package_name" ] && [ -f pnpm-workspace.yaml ]; then run_and_capture "Tauri frontendDist build (${package_dir})" corepack pnpm --filter "$package_name" run build else + # Positional parameters expand in the child bash. + # shellcheck disable=SC2016 run_and_capture "Tauri frontendDist build (${package_dir})" bash -c 'cd "$1" && corepack pnpm run build' bash "$package_dir" fi ;; @@ -1762,6 +1774,8 @@ jobs: if [ -n "$package_name" ] && jq -e '.workspaces // empty' package.json >/dev/null 2>&1; then run_and_capture "Tauri frontendDist build (${package_dir})" yarn workspace "$package_name" build else + # Positional parameters expand in the child bash. + # shellcheck disable=SC2016 run_and_capture "Tauri frontendDist build (${package_dir})" bash -c 'cd "$1" && yarn build' bash "$package_dir" fi ;; @@ -1878,8 +1892,11 @@ jobs: # coverage command still runs and reports any uncovered GPU lines # exactly as before, so Rust repositories without GPU code are # unaffected and no gate is weakened. - if ls /usr/share/vulkan/icd.d/lvp_icd*.json >/dev/null 2>&1; then - lvp_icd="$(ls /usr/share/vulkan/icd.d/lvp_icd*.json | head -n1)" + local lvp_icd="" + while IFS= read -r lvp_icd; do + break + done < <(compgen -G '/usr/share/vulkan/icd.d/lvp_icd*.json') + if [ -n "$lvp_icd" ]; then export VK_ICD_FILENAMES="$lvp_icd" export VK_DRIVER_FILES="$lvp_icd" export WGPU_BACKEND=vulkan @@ -3050,12 +3067,20 @@ jobs: language_signal="Match changed prose" fi + # Markdown backticks in the single-quoted format are intentional literals. + # shellcheck disable=SC2016 printf -- '- Preferred review language: `%s`\n' "$language_signal" printf -- '- Rule: write human-readable review prose in the preferred language; keep file paths, identifiers, logs, quoted source, error text, and protocol literals unchanged.\n' + # Markdown backticks in the single-quoted format are intentional literals. + # shellcheck disable=SC2016 printf -- '- PR title: `%s`\n' "$(printf '%s' "$title" | tr '\r\n`' ' ' | cut -c 1-240)" if [ -n "$body" ]; then + # Markdown backticks in the single-quoted format are intentional literals. + # shellcheck disable=SC2016 printf -- '- PR body excerpt: `%s`\n' "$(printf '%s' "$body" | tr '\r\n`' ' ' | cut -c 1-360)" else + # Markdown backticks in the single-quoted format are intentional literals. + # shellcheck disable=SC2016 printf -- '- PR body excerpt: `[empty]`\n' fi } @@ -3335,6 +3360,8 @@ jobs: shift if ! git -C "$OPENCODE_SOURCE_WORKDIR" diff "$@"; then + # Markdown backticks in the single-quoted format are intentional literals. + # shellcheck disable=SC2016 printf 'Unable to collect %s from `%s` to `%s`; continue review from available changed-file evidence and direct file inspection.\n' "$description" "$PR_MERGE_BASE" "$PR_HEAD_SHA" fi } @@ -3345,12 +3372,16 @@ jobs: printf -- "- Base SHA: \`%s\`\n" "$PR_BASE_SHA" printf -- "- Head SHA: \`%s\`\n\n" "$PR_HEAD_SHA" if ! PR_MERGE_BASE="$(git -C "$OPENCODE_SOURCE_WORKDIR" merge-base "$PR_BASE_SHA" "$PR_HEAD_SHA")"; then + # Markdown backticks in the single-quoted format are intentional literals. + # shellcheck disable=SC2016 printf 'Merge-base discovery failed for `%s` and `%s`; falling back to base SHA for bounded diff evidence.\n\n' "$PR_BASE_SHA" "$PR_HEAD_SHA" PR_MERGE_BASE="$PR_BASE_SHA" fi printf -- "- Merge base SHA: \`%s\`\n\n" "$PR_MERGE_BASE" printf '## Current-head authority order\n\n' printf 'Treat current-head sections in this file as authoritative for this run: Other unresolved review thread evidence, Failed GitHub Check evidence, Coverage execution evidence, Changed files, and Focused changed hunks.\n' + # Markdown backticks in the single-quoted format are intentional literals. + # shellcheck disable=SC2016 printf 'All PR reviews and comments evidence is historical context only and may contain stale bot conclusions. Do not infer active failed checks, unresolved threads, or missing changed files from those comments unless current-head evidence corroborates the same claim for Head SHA `%s`.\n\n' "$PR_HEAD_SHA" if ! git -C "$OPENCODE_SOURCE_WORKDIR" diff --name-only --find-renames "$PR_MERGE_BASE" "$PR_HEAD_SHA" | awk 'NF > 0 && $0 !~ /^\// && $0 !~ /(^|\/)\.\.($|\/)/ { print }' >"$OPENCODE_CHANGED_FILES_FILE"; then @@ -4362,6 +4393,8 @@ jobs: "$@" } + # This is a jq program, not shell expansion. + # shellcheck disable=SC2016 self_check_filter=' def self_check: (.name // "") as $n @@ -5029,6 +5062,8 @@ jobs: if [ -n "${GITHUB_STEP_SUMMARY:-}" ]; then { printf '## OpenCode %s review body\n\n' "$event" + # Markdown backticks in the single-quoted format are intentional literals. + # shellcheck disable=SC2016 printf -- '- Head SHA: `%s`\n' "$HEAD_SHA" printf -- '- Workflow run: %s\n' "$RUN_ID" printf -- '- Workflow attempt: %s\n\n' "$RUN_ATTEMPT" @@ -5383,6 +5418,8 @@ jobs: printf '## Summary\n\n' printf '%s\n\n' "$summary" printf '## Adversarial validation\n\n' + # Markdown backticks in the single-quoted format are intentional literals. + # shellcheck disable=SC2016 printf '```json\n%s\n```\n\n' "$adversarial_evidence" printf -- '- Result: REQUEST_CHANGES\n' printf -- '- Reason: %s\n\n' "$reason" @@ -6180,6 +6217,8 @@ jobs: case "$mode" in failed) + # This is a jq program, not shell expansion. + # shellcheck disable=SC2016 jq_filter=' [.[].check_runs[]?] | sort_by((.started_at // .completed_at // .created_at // ""), (.id // 0)) @@ -6196,6 +6235,8 @@ jobs: ' ;; pending) + # This is a jq program, not shell expansion. + # shellcheck disable=SC2016 jq_filter=' [.[].check_runs[]?] | sort_by((.started_at // .completed_at // .created_at // ""), (.id // 0)) @@ -6260,6 +6301,8 @@ jobs: local owner="${GH_REPOSITORY%%/*}" local name="${GH_REPOSITORY#*/}" + # GraphQL variables expand on the server, not in this shell. + # shellcheck disable=SC2016 timeout "$(check_lookup_api_timeout_seconds)s" gh api graphql \ -f owner="$owner" \ -f name="$name" \ @@ -6413,6 +6456,8 @@ jobs: commit_check_runs_file="$(mktemp)" filtered_rollup_file="$(mktemp)" successful_check_names_file="$(mktemp)" + # GraphQL variables expand on the server, not in this shell. + # shellcheck disable=SC2016 if ! pr_node_id="$(timeout "$(check_lookup_api_timeout_seconds)s" gh api graphql \ -f owner="$owner" \ -f name="$name" \ @@ -6856,6 +6901,8 @@ jobs: head_ref="$(printf '%s\n' "$pr_json" | jq -r '.headRefName // empty')" [ -n "$head_ref" ] || return 1 lookup_error_file="$(mktemp)" + # This is a jq program, not shell expansion. + # shellcheck disable=SC2016 if ! GH_TOKEN="$scan_token" timeout "$(check_lookup_api_timeout_seconds)s" \ gh api -X GET "repos/${GH_REPOSITORY}/code-scanning/alerts" \ -f "ref=refs/heads/${head_ref}" \ @@ -6923,6 +6970,8 @@ jobs: printf 'OpenCode could not approve from deterministic current-head evidence because GitHub Checks have failed.\n\n' printf '## Findings\n\n' printf '### 1. HIGH Current-head GitHub Checks - Fix failed required checks before approval\n' + # Markdown backticks in the single-quoted format are intentional literals. + # shellcheck disable=SC2016 printf -- '- Problem: Failed same-head checks remain for `%s`.\n' "$HEAD_SHA" printf -- '- Root cause: The model-unavailable evidence fallback is allowed only when peer GitHub Checks are complete and clean.\n' printf -- '- Fix: Read and fix the failed check logs below, then rerun the current-head checks.\n' @@ -6979,10 +7028,16 @@ jobs: if [ -n "${GITHUB_STEP_SUMMARY:-}" ]; then { printf '## OpenCode required check satisfied by existing same-head approval\n\n' + # Markdown backticks in the single-quoted format are intentional literals. + # shellcheck disable=SC2016 printf -- '- Result: `EXISTING_CURRENT_HEAD_APPROVAL`\n' + # Markdown backticks in the single-quoted format are intentional literals. + # shellcheck disable=SC2016 printf -- '- Head SHA: `%s`\n' "$HEAD_SHA" printf -- '- Workflow run: %s\n' "$RUN_ID" printf -- '- Workflow attempt: %s\n' "$RUN_ATTEMPT" + # Markdown backticks in the single-quoted format are intentional literals. + # shellcheck disable=SC2016 printf -- '- Model-pool outcome: `%s`\n' "${OPENCODE_MODEL_POOL_OUTCOME:-unknown}" printf -- '- Reason: a prior real-model OpenCode APPROVED review with passed structured adversarial probes already targets this exact head, and the fallback rechecked coverage, peer checks, code-scanning alerts, and unresolved review threads before accepting it.\n' printf -- '- Review state: unchanged; no duplicate APPROVE review was posted from model-output-unavailable evidence.\n\n' diff --git a/docs/doctoring/current-head-run-coalescing.md b/docs/doctoring/current-head-run-coalescing.md index 0a7a5ffc26..a1eb98485f 100644 --- a/docs/doctoring/current-head-run-coalescing.md +++ b/docs/doctoring/current-head-run-coalescing.md @@ -12,7 +12,9 @@ The coalescing step runs inside `.github/workflows/pr-review-merge-scheduler.yml The live-head admission and coalescing work share one job. Workflow-level concurrency includes the repository and PR number, so a new PR event retires an older queued execution before either consumes another job slot. The first step re-fetches the PR and gates every mutation on the exact current HEAD. This avoids both the former two-job admission dependency and the former HEAD-scoped group that allowed one stale queued coalescer per pushed commit to survive under the organization ceiling. -The script re-fetches the live PR before classification. It lists all queued and in-progress repository runs rather than filtering only by workflow-run `head_sha`, because `pull_request_target` runs execute on the trusted base and their workflow head is not the PR head. Those runs are instead bound to the associated pull request's head identity. GitHub exposes repository identity in two different trusted REST shapes: the pull-request endpoint supplies a full repository object with `full_name`, while workflow-run `pull_requests[*].head.repo` and `base.repo` associations can contain only `id`, `name`, and canonical `https://api.github.com/repos/{owner}/{repo}` URL. `_repository_full_name()` therefore normalizes a valid full name directly or derives `owner/name` only from an exact HTTPS `api.github.com/repos/...` URL; malformed, query-bearing, foreign-host, non-HTTPS, or path-sentinel identities fail closed. This prevents a missing `full_name` field from turning every real workflow-run association into an empty repository identity while retaining a narrow authenticated GitHub boundary. +The script re-fetches the live PR before classification and lists queued and in-progress repository runs. For both PR event families, REST run `head_sha` must match the live PR head before repository/ref and PR-association checks can authorize coalescing. Runtime `github.sha`/`GITHUB_SHA` and REST run `head_sha` are different fields: the trusted-base execution context of `pull_request_target` does not establish that its REST run revision is the base SHA. PR associations can refresh after another push, so their current head alone cannot prove which revision an older run checks. + +GitHub exposes repository identity in two different trusted REST shapes: the pull-request endpoint supplies a full repository object with `full_name`, while workflow-run `pull_requests[*].head.repo` and `base.repo` associations can contain only `id`, `name`, and canonical `https://api.github.com/repos/{owner}/{repo}` URL. `_repository_full_name()` therefore normalizes a valid full name directly or derives `owner/name` only from an exact HTTPS `api.github.com/repos/...` URL; malformed, query-bearing, foreign-host, non-HTTPS, or path-sentinel identities fail closed. This prevents a missing `full_name` field from turning every real workflow-run association into an empty repository identity while retaining a narrow authenticated GitHub boundary. Before every cancellation the script re-fetches active same-head state, exact non-current PR associations, each possible same-workflow authoritative sibling, the current PR, and finally the candidate itself. Missing, malformed, moved, closed, completed, timed-out, or ambiguous evidence preserves the candidate or fails closed. @@ -24,7 +26,7 @@ A workflow run may authorize cancellation only inside the current PR's evidence Runs are eligible only when all of the following are true: -1. the run was triggered by `pull_request` or `pull_request_target` and is bound to the current live PR head through the correct event-specific identity; +1. the run was triggered by `pull_request` or `pull_request_target`, its recorded REST `head_sha` equals the live PR head, and its repository/ref identity matches; 2. its PR association belongs either to the current PR or to a proven closed predecessor with the same exact head and exact base repository/ref/SHA identity; 3. its stable numeric `workflow_id` matches another run inside the same PR evidence boundary; 4. each candidate authoritative sibling identified from the bulk Actions snapshot is re-fetched by exact run ID and must still be queued or in progress with the same workflow/head/PR scope; @@ -39,12 +41,36 @@ This invariant is deliberately separate from old-head cancellation. #1348 remain ## Executable evidence -`tests/test_current_head_run_coalescer.py`, `tests/test_current_head_run_coalescer_review_regressions.py`, and `tests/test_current_head_coalescer_self_cancellation.py` pin the source and integrated workflow contract. Coverage includes one-run retention, in-progress preservation, `pull_request_target` base/head separation, real minimal Actions repository-association normalization for both PR event families, fail-closed repository URL normalization, isolation between concurrently open PRs, exact-base isolation across closed predecessor succession, same-workflow sibling re-fetch, completed-sibling preservation, workflow/head/branch/repository/event isolation, moved-head/status fail-closed behavior, per-call timeouts, explicit cancellation authentication, complete pagination, final candidate re-fetch, ready/draft transition triggers, trusted-source materialization, shell-injection resistance, PR-stable concurrency, and minimum workflow permissions. +`tests/test_current_head_run_coalescer.py`, `tests/test_current_head_run_coalescer_review_regressions.py`, and `tests/test_current_head_coalescer_self_cancellation.py` pin the source and integrated workflow contract. Coverage includes one-run retention, in-progress preservation, original REST run revision despite refreshed PR associations, real minimal Actions repository-association normalization for both PR event families, fail-closed repository URL normalization, isolation between concurrently open PRs, exact-base isolation across closed predecessor succession, same-workflow sibling re-fetch, completed-sibling preservation, workflow/head/branch/repository/event isolation, moved-head/status fail-closed behavior, per-call timeouts, explicit cancellation authentication, complete pagination, final candidate re-fetch, ready/draft transition triggers, trusted-source materialization, shell-injection resistance, PR-stable concurrency, and minimum workflow permissions. The minimal-repository-shape regression was committed before the production normalization repair. On the pre-fix source `_head_tuple()` read only `repo.full_name`, so the real Actions fixture deterministically normalized to an empty repository string. Production now accepts the fuller pull-request representation and the minimal workflow-run representation through the same bounded owner/name normalization contract. A one-use read-only branch workflow was attempted solely to capture hosted RED/GREEN evidence; GitHub did not schedule newly introduced branch-only push workflows in this repository, so no hosted result is claimed from that mechanism and it was deleted from the publishable tree. Ordinary protected PR checks and independent review on the exact production head remain authoritative. +### Refreshed-association correction (2026-09-05) + +Read-only REST inspection found that organization-required OpenCode run +[`33949656057`](https://github.com/ContextualWisdomLab/contextual-orchestrator/actions/runs/33949656057) +retained `head_sha=1481c595dc1d16e7bf4b65addaf0bd30322cf2b8`, while its PR #1067 +association had moved to `6d1b30803888e893d7bdbdf4d12605a16c36162d`. +The newer run +[`33950557383`](https://github.com/ContextualWisdomLab/contextual-orchestrator/actions/runs/33950557383) +recorded that newer SHA in both fields. Native central run +[`33950857678`](https://github.com/ContextualWisdomLab/.github/actions/runs/33950857678) +likewise recorded PR #1899 head `2000b4f0c892b95f2609ea956b5266218a511fe3` +in REST `head_sha`, not the trusted-base SHA. + +The old matcher accepted a refreshed association even when the recorded run +revision disagreed. An old in-progress run could therefore appear to authorize +cancelling the sole queued current-head run. Six regressions failed before the +shared revision guard: queued/in-progress old-run selection for both PR events, +and final authority revalidation for both events. The repair rejects missing or +different REST run revisions before considering associations; valid same-head +coalescing and all existing PR/base/repository boundaries remain in place. +The old live run was already cancelled when inspected. No cancellation POST +was made for this investigation, and these samples do not prove a historical +false cancellation or hosted execution of the proposed repair. + ## Recovery and rollback If the coalescer reports unexpected preservation, inspect the live PR/run/sibling identities before changing policy. Do not weaken repository normalization, exact-head, exact-base, PR-association, final-status, refreshed-sibling, or authoritative-sibling checks to improve cancellation volume. If a false cancellation is ever observed, disable the `Retire redundant queued exact-head runs` scheduler step first while retaining #1348 stale-head queue hygiene, then reproduce the identity race with a deterministic regression before repair. diff --git a/docs/doctoring/review-rerun-concurrency-isolation.md b/docs/doctoring/review-rerun-concurrency-isolation.md index a8364cbd20..7d9ede08d6 100644 --- a/docs/doctoring/review-rerun-concurrency-isolation.md +++ b/docs/doctoring/review-rerun-concurrency-isolation.md @@ -19,6 +19,12 @@ First attempts retain the workflow/repository/PR key and cancellation policy. No jobs, dependencies, permissions, provider routes, or gate exceptions are added. Live-head admission and publication checks remain mandatory. +Read-only follow-up also exposed a second cancellation risk in the existing +same-head coalescer: REST PR associations can move to a newer head while the +run retains its original `head_sha`. The shared identity matcher now requires +that recorded revision to match the live PR before considering associations. +See the [live samples and regression evidence](current-head-run-coalescing.md#refreshed-association-correction-2026-09-05). + ## Checks `tests/test_review_rerun_concurrency.py` evaluates the actual group expressions @@ -28,18 +34,37 @@ passed. The tests cover numeric/string attempts, distinct retries, first-push coalescing, repository/PR isolation, non-PR fallback, and native/dispatch parity. Existing Noema and central dispatch cleanup tests also exercise retry metadata; they prove selection and API requests, not GitHub terminal cancellation. +The coalescer's separate three-file suite passes 57 tests with 100% statement +and branch coverage (252 statements, 118 branches). Six new tests failed on the +old source before the shared guard; an older positive fixture was corrected +because it conflated runtime `GITHUB_SHA` with REST run `head_sha`. Run the focused suite: ```sh -uv run pytest -q tests/test_review_rerun_concurrency.py tests/test_required_workflow_queue_contract.py tests/test_noema_orchestrator_workflow_contract.py tests/test_opencode_required_rerun_capacity.py tests/test_opencode_required_verdict_regression.py tests/test_strix_rerun_job_selection.py tests/test_current_head_run_coalescer.py tests/test_opencode_live_draft_state_regression.py tests/test_noema_review_gate.py tests/test_pr_review_merge_scheduler.py tests/test_pr1669_cancel_stale_opencode_runs.py +uv run pytest -q tests/test_review_rerun_concurrency.py tests/test_required_workflow_queue_contract.py tests/test_noema_orchestrator_workflow_contract.py tests/test_opencode_required_rerun_capacity.py tests/test_opencode_required_verdict_regression.py tests/test_strix_rerun_job_selection.py tests/test_current_head_run_coalescer.py tests/test_current_head_run_coalescer_review_regressions.py tests/test_current_head_coalescer_self_cancellation.py tests/test_opencode_live_draft_state_regression.py tests/test_noema_review_gate.py tests/test_pr_review_merge_scheduler.py tests/test_pr1669_cancel_stale_opencode_runs.py tests/test_opencode_workflow_shell_syntax.py ``` Local actionlint 1.7.12 hung writing large shell input before starting its child process. A diagnostic stack matched upstream `process.go`; this failed run is not passing evidence. The additional `-shellcheck= -pyflakes=` invocation -passed workflow syntax/expression validation only, not external lint. No hosted -gate was changed or disabled. +passed workflow syntax/expression validation only, not external lint. + +An isolated build of official actionlint commit +`011a6d15e749bb3f2d771eed9c7aa0e7e3e10ee7` avoids that tool deadlock without a +system installation or project dependency change. Full lint then reported the +same 29 ShellCheck diagnostics on head and base; neither was a passing run. +The dispatch workflow now removes a redundant case pattern, combines repeated +append redirects, and uses Bash filename discovery without changing shell +options. Literal child-shell/jq/GraphQL programs and Markdown backticks carry +command-scoped SC2016 annotations, not a blanket suppression. Full lint with +ShellCheck enabled now exits zero without output on all four changed workflows. + +Review caught an intermediate annotation inserted inside a continued `printf`, +which lint alone missed. A new test executes that existing body-building +function: it first failed with `## Findings: command not found`, then passed +after removal of the misplaced annotation. The defective intermediate edit +was not committed. No hosted gate was changed or disabled. ## Remaining limits and acceptance @@ -68,6 +93,7 @@ gate was changed or disabled. - GitHub. (n.d.). [Control workflow concurrency](https://docs.github.com/en/actions/how-tos/write-workflows/choose-when-workflows-run/control-workflow-concurrency). - GitHub. (n.d.). [Re-running workflows and jobs](https://docs.github.com/en/actions/how-tos/manage-workflow-runs/re-run-workflows-and-jobs). - rhysd. (n.d.). [actionlint v1.7.12 process runner](https://github.com/rhysd/actionlint/blob/v1.7.12/process.go#L23-L41). +- rhysd. (n.d.). [Pinned upstream process runner](https://github.com/rhysd/actionlint/blob/011a6d15e749bb3f2d771eed9c7aa0e7e3e10ee7/process.go). Retrieved 2026-09-05. These platform/tool sources ground a configuration bug; no research-paper PDF is needed for this bounded repair. diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 5b705bfb4d..44398fae5f 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -79,6 +79,10 @@ flowchart LR 2026-09-05 G-02/G-03 follow-up: [review rerun isolation](doctoring/review-rerun-concurrency-isolation.md) reproduces an older retry sharing the current PR's cancellation key. The proposed five-group repair preserves first-attempt coalescing and existing cleanup guards. +The same cancellation-boundary follow-up also corrects +[mutable PR-association authority](doctoring/current-head-run-coalescing.md#refreshed-association-correction-2026-09-05): +an old run must retain its recorded REST revision even if its PR association +now names the latest head. Selection and final revalidation reject that mismatch. Immediate stale-retry recovery under runner saturation and protected hosted delivery remain unverified; this does not close either gap or the full objective. diff --git a/scripts/ci/current_head_run_coalescer.py b/scripts/ci/current_head_run_coalescer.py index ae40b85ac4..14ed84ad18 100644 --- a/scripts/ci/current_head_run_coalescer.py +++ b/scripts/ci/current_head_run_coalescer.py @@ -105,10 +105,13 @@ def _run_matches_head_identity( event = run_data.get("event") if event not in PR_EVENTS: return False + # REST run head_sha is not GITHUB_SHA. PR associations can follow a newer + # push and must never promote an older run into current-head authority. + if str(run_data.get("head_sha") or "").lower() != head_sha: + return False if event == "pull_request": if ( - str(run_data.get("head_sha") or "").lower() == head_sha - and run_data.get("head_branch") == branch + run_data.get("head_branch") == branch and _repository_full_name(run_data.get("head_repository")) == repository ): return True diff --git a/tests/test_current_head_run_coalescer.py b/tests/test_current_head_run_coalescer.py index 571368677f..a953d6619b 100644 --- a/tests/test_current_head_run_coalescer.py +++ b/tests/test_current_head_run_coalescer.py @@ -53,15 +53,14 @@ def run_record( repository: str = "ContextualWisdomLab/.github", event: str = "pull_request", pr_number: int = 1, - execution_head_sha: str | None = None, associations: list[dict[str, object]] | None = None, ) -> dict[str, object]: - """Return one bounded Actions run fixture with authoritative PR association.""" + """Return a REST run revision plus its independently mutable PR association.""" return { "id": run_id, "workflow_id": workflow_id, "status": status, - "head_sha": execution_head_sha or head_sha, + "head_sha": head_sha, "head_branch": branch, "event": event, "head_repository": {"full_name": repository}, @@ -144,11 +143,44 @@ def test_group_with_no_queued_runs_has_nothing_to_coalesce() -> None: ) == [] -def test_pull_request_target_uses_associated_pr_head_not_execution_head() -> None: - """Trusted-base pull_request_target runs coalesce by their associated PR head.""" +@pytest.mark.parametrize("event", ["pull_request", "pull_request_target"]) +@pytest.mark.parametrize("old_status", ["queued", "in_progress"]) +def test_refreshed_pr_association_cannot_promote_an_old_run(event, old_status) -> None: + """A live PR association does not change the revision an old run checks.""" module = load_module() - target = run_record(100, 10, event="pull_request_target", execution_head_sha="b" * 40) - newer = run_record(101, 10, event="pull_request_target", execution_head_sha="b" * 40) + older = run_record( + 100, 10, event=event, status=old_status, head_sha="b" * 40, + associations=[pr_association()], + ) + current = run_record(101, 10, event=event) + assert module.select_duplicate_queued_run_ids( + [older, current], + repository="ContextualWisdomLab/.github", + branch="feature/current", + head_sha="a" * 40, + ) == [] + + +@pytest.mark.parametrize("event", ["pull_request", "pull_request_target"]) +def test_revalidation_cannot_use_an_old_run_as_current_authority(event) -> None: + """Re-fetching mutable associations must not authorize a current-run cancel.""" + module = load_module() + older = run_record( + 100, 10, event=event, status="in_progress", head_sha="b" * 40, + associations=[pr_association()], + ) + current = run_record(101, 10, event=event) + with pytest.raises(module.CoalescingRefused, match="no distinct authoritative sibling"): + module.validate_candidate_against_live_state( + current, live_pr=live_pr(), active_same_head_runs=[older, current], + ) + + +def test_pull_request_target_coalesces_matching_rest_run_revisions() -> None: + """REST head_sha records the PR head even for a trusted-base workflow.""" + module = load_module() + target = run_record(100, 10, event="pull_request_target") + newer = run_record(101, 10, event="pull_request_target") assert module.select_duplicate_queued_run_ids( [target, newer], repository="ContextualWisdomLab/.github", @@ -170,6 +202,7 @@ def test_other_identities_and_malformed_runs_are_not_coalesced() -> None: run_record(0, 10), run_record(106, 0), {**run_record(107, 10), "status": "completed"}, + {**run_record(108, 10), "head_sha": None}, ] assert module.select_duplicate_queued_run_ids( runs, diff --git a/tests/test_current_head_run_coalescer_review_regressions.py b/tests/test_current_head_run_coalescer_review_regressions.py index 3f10ab3165..6ae6f2c7c7 100644 --- a/tests/test_current_head_run_coalescer_review_regressions.py +++ b/tests/test_current_head_run_coalescer_review_regressions.py @@ -161,8 +161,8 @@ def test_minimal_actions_associations_pass_exact_scope_for_both_pr_events() -> N ) -def test_pull_request_target_matches_associated_pr_head_not_trusted_base_head() -> None: - """Target-event runs bind to associated PR head rather than workflow base head.""" +def test_pull_request_target_rejects_refreshed_association_on_an_old_run() -> None: + """A minimal PR association cannot replace the original REST run revision.""" module = load_module() target_run = run_record( 100, @@ -171,7 +171,7 @@ def test_pull_request_target_matches_associated_pr_head_not_trusted_base_head() top_head_branch="main", minimal_association=True, ) - assert module._run_identity_matches( + assert not module._run_identity_matches( target_run, repository="ContextualWisdomLab/.github", branch="feature/current", diff --git a/tests/test_opencode_workflow_shell_syntax.py b/tests/test_opencode_workflow_shell_syntax.py index b0a672b1a2..61515c0a78 100644 --- a/tests/test_opencode_workflow_shell_syntax.py +++ b/tests/test_opencode_workflow_shell_syntax.py @@ -5,6 +5,8 @@ import sys from pathlib import Path +import pytest + REPO_ROOT = Path(__file__).resolve().parents[1] @@ -28,6 +30,13 @@ def _extract_run_block(workflow_text: str, step_name: str) -> str: return "\n".join(block_lines) + "\n" +def _extract_shell_function(script: str, function_name: str) -> str: + """Extract one top-level Bash function from an already isolated run block.""" + start = script.index(f"{function_name}() {{") + end = script.index("\n}\n", start) + len("\n}\n") + return script[start:end] + + def test_opencode_review_run_blocks_are_valid_bash(): workflow_text = (REPO_ROOT / ".github/workflows/opencode-review-dispatch.yml").read_text( encoding="utf-8" @@ -61,6 +70,40 @@ def test_opencode_review_run_blocks_are_valid_bash(): assert result.returncode == 0, f"{step_name}: {result.stderr}" +def test_unresolved_reviewer_threads_body_function_executes_as_one_printf(tmp_path): + """Comments must not split the continued printf argument list into commands.""" + if sys.platform == "win32": + pytest.skip("workflow body execution requires Bash") + bash = shutil.which("bash") + if bash is None: + pytest.skip("Bash is not installed") + + workflow_text = (REPO_ROOT / ".github/workflows/opencode-review-dispatch.yml").read_text( + encoding="utf-8" + ) + run_block = _extract_run_block(workflow_text, "Publish OpenCode review outcome") + function = _extract_shell_function(run_block, "build_unresolved_reviewer_threads_body") + evidence_file = tmp_path / "threads.md" + evidence_file.write_text("thread evidence\n", encoding="utf-8") + body_file = tmp_path / "body.md" + result = subprocess.run( + [bash, "-c", f"set -euo pipefail\n{function}\nbuild_unresolved_reviewer_threads_body \"$1\" \"$2\"", "bash", str(evidence_file), str(body_file)], + text=True, + capture_output=True, + check=False, + env={ + **os.environ, + "HEAD_SHA": "a" * 40, + "RUN_ID": "123", + "RUN_ATTEMPT": "2", + }, + ) + + assert result.returncode == 0, result.stderr + assert "## Findings" in body_file.read_text(encoding="utf-8") + assert "thread evidence" in body_file.read_text(encoding="utf-8") + + def test_opencode_review_comment_helpers_are_shared_and_valid_bash(): workflow_text = (REPO_ROOT / ".github/workflows/opencode-review-dispatch.yml").read_text( encoding="utf-8" From f08f035c3763a7f419c01ad39f208fbbe7342333 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sat, 5 Sep 2026 17:13:19 +0900 Subject: [PATCH 4/7] =?UTF-8?q?fix(ci):=20=EC=B7=A8=EC=86=8C=20=EC=A7=81?= =?UTF-8?q?=EC=A0=84=20=EC=8B=A4=ED=96=89=20=EA=B8=B0=EB=A1=9D=EA=B3=BC=20?= =?UTF-8?q?=ED=99=9C=EC=84=B1=20=EC=83=81=ED=83=9C=20=EC=9E=AC=EA=B2=80?= =?UTF-8?q?=EC=A6=9D?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .github/workflows/opencode-review.yml | 74 +++++-- .github/workflows/strix.yml | 69 +++--- .../review-rerun-concurrency-isolation.md | 22 ++ docs/product-technical-gap-baseline.md | 6 + ...st_opencode_required_verdict_regression.py | 201 +++++++++++++++++- .../test_required_workflow_queue_contract.py | 191 ++++++++++++++--- 6 files changed, 481 insertions(+), 82 deletions(-) diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 9d5cd94305..ab67e69d3f 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -518,13 +518,54 @@ jobs: set -euo pipefail live_head_matches() { - local live_head - if ! live_head="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${TARGET_PR_NUMBER}" --jq '.head.sha' 2>/tmp/opencode-cleanup-gh-error)"; then - echo "::warning::OpenCode review cleanup could not verify the live pull request head; leaving runs unchanged." + local live_pr_json live_state live_draft live_head + if ! live_pr_json="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${TARGET_PR_NUMBER}" 2>/tmp/opencode-cleanup-gh-error)"; then + echo "::warning::OpenCode review cleanup could not verify the live pull request; leaving runs unchanged." sed 's/^/ /' /tmp/opencode-cleanup-gh-error >&2 || true return 1 fi - [ "${live_head,,}" = "${TARGET_PR_HEAD_SHA,,}" ] + live_state="$(jq -r '.state // ""' <<<"$live_pr_json")" + live_draft="$(jq -r '.draft // false' <<<"$live_pr_json")" + live_head="$(jq -r '.head.sha // ""' <<<"$live_pr_json")" + [ "$live_state" = "open" ] && [ "$live_draft" = "false" ] && + [ "${live_head,,}" = "${TARGET_PR_HEAD_SHA,,}" ] + } + + # jq variables are bound by --arg when this literal program executes. + # shellcheck disable=SC2016 + candidate_jq=' + (if type == "array" then .[] else . end) + | select(.status == "queued" or .status == "in_progress" or .status == "requested" or .status == "waiting" or .status == "pending") + | select(.name == "Required OpenCode Review" and .event == "pull_request_target") + | ((.display_title // "") | startswith("Required OpenCode Review " + $repo + "#" + $pr + "@")) as $title_matches + | ((.display_title // "") | test("^Required OpenCode Review .+#[0-9]+@[0-9a-fA-F]+$")) as $title_is_scoped + | select((.pull_requests // []) | any((.number | tostring) == $pr)) + | select(($title_is_scoped | not) or $title_matches) + | ((.head_sha // "") | ascii_downcase) as $recorded_head + | ((.display_title // "") | if $title_matches then split("@")[-1] | ascii_downcase else "" end) as $title_head + | select($recorded_head | test("^[0-9a-f]{40}$")) + | select(($title_matches | not) or (($title_head | test("^[0-9a-f]{40}$")) and $title_head == $recorded_head)) + | select($recorded_head != ($head_sha | ascii_downcase)) + ' + + filter_candidate_runs() { + jq -c --arg pr "$TARGET_PR_NUMBER" --arg head_sha "$TARGET_PR_HEAD_SHA" \ + --arg repo "$TARGET_REPOSITORY" "$candidate_jq" + } + + run_still_superseded() { + local run_id="$1" run_json filtered_run + if ! run_json="$(gh api "repos/${TARGET_REPOSITORY}/actions/runs/${run_id}" 2>/tmp/opencode-cleanup-gh-error)"; then + echo "::warning::OpenCode review cleanup could not revalidate run ${run_id}; leaving it unchanged." + return 1 + fi + if ! filtered_run="$( + jq -ce --arg run_id "$run_id" 'select(type == "object" and (.id | tostring) == $run_id)' <<<"$run_json" | + filter_candidate_runs + )"; then + return 1 + fi + [ -n "$filtered_run" ] } cancel_runs() { @@ -541,23 +582,8 @@ jobs: return 0 fi local run_ids - if ! run_ids="$(jq -r --arg pr "$TARGET_PR_NUMBER" --arg head_sha "$TARGET_PR_HEAD_SHA" \ - --arg repo "$TARGET_REPOSITORY" --arg current "$CURRENT_RUN_ID" ' - .workflow_runs[] - | select((.id | tostring) != $current) - | select(.name == "Required OpenCode Review") - | select(.event == "pull_request_target") - | ((.display_title // "") | startswith("Required OpenCode Review " + $repo + "#" + $pr + "@")) as $title_matches - | ((.pull_requests // []) | any((.number | tostring) == $pr)) as $metadata_matches - | select($title_matches or $metadata_matches) - | ((.display_title // "") | endswith("@" + $head_sha)) as $title_is_current - | ((.pull_requests // []) | any( - ((.number | tostring) == $pr) - and ((.head.sha // "") | ascii_downcase) == ($head_sha | ascii_downcase) - )) as $metadata_is_current - | select(($title_is_current or $metadata_is_current) | not) - | .id - ' <<<"$runs_json")"; then + if ! run_ids="$(jq -c --arg current "$CURRENT_RUN_ID" '[.workflow_runs[] | select((.id | tostring) != $current)]' <<<"$runs_json" | + filter_candidate_runs | jq -r '.id')"; then echo "::warning::OpenCode review cleanup received invalid run data for ${TARGET_REPOSITORY}; leaving runs unchanged." return 0 fi @@ -567,9 +593,13 @@ jobs: echo "::notice::OpenCode review cleanup target changed before cancellation; leaving runs unchanged." return 0 fi + if ! run_still_superseded "$run_id"; then + echo "::notice::OpenCode review run ${run_id} is no longer a proven superseded candidate; leaving it unchanged." + continue + fi if gh api --method POST "repos/${TARGET_REPOSITORY}/actions/runs/${run_id}/cancel" >/dev/null 2>/tmp/opencode-cleanup-cancel-error || gh api --method POST "repos/${TARGET_REPOSITORY}/actions/runs/${run_id}/force-cancel" >/dev/null 2>>/tmp/opencode-cleanup-cancel-error; then - echo "Cancelled superseded Required OpenCode Review run ${run_id} in ${TARGET_REPOSITORY} for PR #${TARGET_PR_NUMBER}." + echo "Cancellation requested for superseded Required OpenCode Review run ${run_id} in ${TARGET_REPOSITORY} for PR #${TARGET_PR_NUMBER}." else echo "::warning::OpenCode review cleanup could not cancel run ${run_id} in ${TARGET_REPOSITORY}; it may have finished or the credential lacks Actions write access." sed 's/^/ /' /tmp/opencode-cleanup-cancel-error >&2 || true diff --git a/.github/workflows/strix.yml b/.github/workflows/strix.yml index 80f4cdc634..750abde661 100644 --- a/.github/workflows/strix.yml +++ b/.github/workflows/strix.yml @@ -275,6 +275,43 @@ jobs: } } + # jq variables are bound by --arg when this literal program executes. + # shellcheck disable=SC2016 + candidate_jq=' + (if type == "array" then .[] else . end) + | select(.status == "queued" or .status == "in_progress" or .status == "requested" or .status == "waiting" or .status == "pending") + | select(.name == "Strix Security Scan" and .event == "pull_request_target") + | ((.display_title // "") | startswith("Strix Security Scan " + $repo + "#" + $pr + "@")) as $title_matches + | ((.display_title // "") | test("^Strix Security Scan .+#[0-9]+@[0-9a-fA-F]+$")) as $title_is_scoped + | select((.pull_requests // []) | any((.number | tostring) == $pr)) + | select(($title_is_scoped | not) or $title_matches) + | ((.head_sha // "") | ascii_downcase) as $recorded_head + | ((.display_title // "") | if $title_matches then split("@")[-1] | ascii_downcase else "" end) as $title_head + | select($recorded_head | test("^[0-9a-f]{40}$")) + | select(($title_matches | not) or (($title_head | test("^[0-9a-f]{40}$")) and $title_head == $recorded_head)) + | select($action == "closed" or $action == "converted_to_draft" or $recorded_head != ($head_sha | ascii_downcase)) + ' + + filter_candidate_runs() { + jq -c --arg pr "$TARGET_PR_NUMBER" --arg head_sha "$TARGET_PR_HEAD_SHA" \ + --arg action "$PR_ACTION" --arg repo "$TARGET_REPOSITORY" "$candidate_jq" + } + + run_still_eligible() { + local run_id="$1" run_json filtered_run + if ! run_json="$(gh api "repos/${TARGET_REPOSITORY}/actions/runs/${run_id}" 2>/tmp/strix-cleanup-gh-error)"; then + echo "::warning::Strix cleanup could not revalidate run ${run_id}; leaving it unchanged." + return 1 + fi + if ! filtered_run="$( + jq -ce --arg run_id "$run_id" 'select(type == "object" and (.id | tostring) == $run_id)' <<<"$run_json" | + filter_candidate_runs + )"; then + return 1 + fi + [ -n "$filtered_run" ] + } + cancel_runs() { local status="$1" if ! live_target_matches; then @@ -289,30 +326,8 @@ jobs: return 0 fi local run_ids - if ! run_ids="$(jq -r --arg pr "$TARGET_PR_NUMBER" --arg head_sha "$TARGET_PR_HEAD_SHA" \ - --arg action "$PR_ACTION" --arg repo "$TARGET_REPOSITORY" --arg current "$CURRENT_RUN_ID" ' - .workflow_runs[] - | select((.id | tostring) != $current) - | select(.name == "Strix Security Scan") - | select(.event == "pull_request_target") - | ((.display_title // "") | startswith("Strix Security Scan " + $repo + "#" + $pr + "@")) as $title_matches - | ((.pull_requests // []) | any((.number | tostring) == $pr)) as $metadata_matches - | select($title_matches or $metadata_matches) - | ((.display_title // "") | endswith("@" + $head_sha)) as $title_is_current - | ((.pull_requests // []) | any( - ((.number | tostring) == $pr) - and ((.head.sha // "") | ascii_downcase) == ($head_sha | ascii_downcase) - )) as $metadata_is_current - | ((.pull_requests // []) | any( - ((.number | tostring) == $pr) and ((.head.sha // "") != "") - )) as $metadata_has_head - | select( - $action == "closed" - or $action == "converted_to_draft" - or (($title_matches or $metadata_has_head) and (($title_is_current or $metadata_is_current) | not)) - ) - | .id - ' <<<"$runs_json")"; then + if ! run_ids="$(jq -c --arg current "$CURRENT_RUN_ID" '[.workflow_runs[] | select((.id | tostring) != $current)]' <<<"$runs_json" | + filter_candidate_runs | jq -r '.id')"; then echo "::warning::Strix cleanup received invalid run data for ${TARGET_REPOSITORY}; leaving runs unchanged." return 0 fi @@ -322,9 +337,13 @@ jobs: echo "::notice::Strix cleanup target changed before cancellation; leaving runs unchanged." return 0 fi + if ! run_still_eligible "$run_id"; then + echo "::notice::Strix run ${run_id} is no longer a proven cancellation candidate; leaving it unchanged." + continue + fi if gh api --method POST "repos/${TARGET_REPOSITORY}/actions/runs/${run_id}/cancel" >/dev/null 2>/tmp/strix-cleanup-cancel-error || gh api --method POST "repos/${TARGET_REPOSITORY}/actions/runs/${run_id}/force-cancel" >/dev/null 2>>/tmp/strix-cleanup-cancel-error; then - echo "Cancelled obsolete Strix run ${run_id} in ${TARGET_REPOSITORY} for PR #${TARGET_PR_NUMBER}." + echo "Cancellation requested for obsolete Strix run ${run_id} in ${TARGET_REPOSITORY} for PR #${TARGET_PR_NUMBER}." else echo "::warning::Strix cleanup could not cancel run ${run_id} in ${TARGET_REPOSITORY}; it may have finished or the credential lacks Actions write access." sed 's/^/ /' /tmp/strix-cleanup-cancel-error >&2 || true diff --git a/docs/doctoring/review-rerun-concurrency-isolation.md b/docs/doctoring/review-rerun-concurrency-isolation.md index 7d9ede08d6..0fa3e44511 100644 --- a/docs/doctoring/review-rerun-concurrency-isolation.md +++ b/docs/doctoring/review-rerun-concurrency-isolation.md @@ -25,6 +25,18 @@ run retains its original `head_sha`. The shared identity matcher now requires that recorded revision to match the live PR before considering associations. See the [live samples and regression evidence](current-head-run-coalescing.md#refreshed-association-correction-2026-09-05). +The existing Required OpenCode and Strix stale-run cleanup had the inverse +failure: the refreshed association made an old run look current, so cleanup +preserved it. Each workflow now reuses one jq predicate for list selection and +the exact run fetched immediately before cancellation. It requires an active +status, the expected workflow/event and PR association, and a valid recorded +`head_sha`; a conflicting scoped title is not accepted. Missing or malformed +data, an unexpected run ID, a non-object response, or a failed parse preserves +the run. A partial result from a failed pipeline cannot authorize cancellation. +OpenCode also requires the live PR to remain open, non-Draft, and at the target +head. Strix retains its explicit closed/Draft cleanup, including current-head +runs. Successful API calls are logged as cancellation requests, not completion. + ## Checks `tests/test_review_rerun_concurrency.py` evaluates the actual group expressions @@ -39,6 +51,16 @@ and branch coverage (252 statements, 118 branches). Six new tests failed on the old source before the shared guard; an older positive fixture was corrected because it conflated runtime `GITHUB_SHA` with REST run `head_sha`. +The stale-cleanup follow-up first failed two tests that execute the production +jq selectors against refreshed associations. Review then reproduced four unsafe +Strix cancellations on an intermediate implementation before adding strict +final-response validation. The production-shell tests now prove that completed +or current runs, another PR, API failure, missing/unknown status, a different run +ID, arrays, and partial/invalid responses issue neither cancel nor force-cancel. +Closed/Draft Strix cleanup and the live-PR guard have separate regression cases. +The final 14-file focused suite passes 681 tests; these are local request-boundary +checks, not hosted terminal-cancellation evidence. + Run the focused suite: ```sh diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 44398fae5f..9a0a5d168a 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -83,6 +83,12 @@ The same cancellation-boundary follow-up also corrects [mutable PR-association authority](doctoring/current-head-run-coalescing.md#refreshed-association-correction-2026-09-05): an old run must retain its recorded REST revision even if its PR association now names the latest head. Selection and final revalidation reject that mismatch. +The existing Required OpenCode/Strix stale-run cleanup uses that recorded +revision too: refreshed associations must not preserve an old run as current. +Both cleanup paths reuse their selector when fetching the exact run before +cancellation and preserve runs whose identity or active state cannot be verified. +Local shell regressions prove request selection and suppression, not terminal +GitHub cancellation; log messages therefore report requests rather than completion. Immediate stale-retry recovery under runner saturation and protected hosted delivery remain unverified; this does not close either gap or the full objective. diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index f29b97a663..37778bb63c 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -174,12 +174,9 @@ def cleanup_candidate_run_ids( if jq is None: pytest.skip("jq is required to execute the production cleanup filter") workflow = WORKFLOW.read_text(encoding="utf-8") - marker = ( - 'jq -r --arg pr "$TARGET_PR_NUMBER" --arg head_sha "$TARGET_PR_HEAD_SHA" \\\n' - ' --arg repo "$TARGET_REPOSITORY" --arg current "$CURRENT_RUN_ID" \'' - ) + marker = " candidate_jq='\n" start = workflow.index(marker) + len(marker) - end = workflow.index("\n ' <<<\"$runs_json\")", start) + end = workflow.index("\n '\n", start) result = subprocess.run( [ jq, @@ -193,12 +190,9 @@ def cleanup_candidate_run_ids( "--arg", "repo", repository, - "--arg", - "current", - current_run_id, - workflow[start:end], + f"{workflow[start:end]} | .id", ], - input=json.dumps({"workflow_runs": runs}), + input=json.dumps([run for run in runs if str(run.get("id")) != current_run_id]), text=True, capture_output=True, check=False, @@ -224,8 +218,10 @@ def _cleanup_run( ) return { "id": run_id, + "status": "queued", "name": name, "event": event, + "head_sha": head_sha, "display_title": title, "pull_requests": [{"number": pr_number, "head": {"sha": head_sha}}], } @@ -273,6 +269,39 @@ def test_cleanup_matches_by_pull_requests_metadata_when_title_omits_the_suffix() assert cleanup_candidate_run_ids([metadata_only], current_run_id="999") == ["1"] +def test_cleanup_uses_recorded_head_not_refreshed_pr_association() -> None: + """A refreshed association cannot promote an old REST run to current.""" + stale = _cleanup_run( + run_id=1, + head_sha="b" * 40, + display_title="Required OpenCode Review", + ) + stale["pull_requests"] = [{"number": 1437, "head": {"sha": HEAD}}] + assert cleanup_candidate_run_ids([stale], current_run_id="999") == ["1"] + + +@pytest.mark.parametrize("recorded_head", ("", "not-a-revision")) +def test_cleanup_preserves_unknown_or_malformed_recorded_revision(recorded_head: str) -> None: + """Unknown run revisions are preserved instead of guessed stale.""" + run = _cleanup_run(run_id=1, head_sha=HEAD, display_title="Required OpenCode Review") + run["head_sha"] = recorded_head + assert cleanup_candidate_run_ids([run], current_run_id="999") == [] + + +def test_cleanup_preserves_conflicting_title_and_recorded_revision() -> None: + """Conflicting immutable-looking signals fail closed without cancellation.""" + run = _cleanup_run(run_id=1, head_sha="b" * 40) + run["display_title"] = f"Required OpenCode Review ContextualWisdomLab/example#1437@{HEAD}" + assert cleanup_candidate_run_ids([run], current_run_id="999") == [] + + +def test_cleanup_preserves_a_scoped_title_for_another_repository() -> None: + """PR association cannot override a conflicting repository-scoped title.""" + run = _cleanup_run(run_id=1, head_sha="b" * 40) + run["display_title"] = f"Required OpenCode Review other/repo#1437@{'b' * 40}" + assert cleanup_candidate_run_ids([run], current_run_id="999") == [] + + def test_cleanup_job_is_scoped_to_synchronize_events_with_actions_write() -> None: """The cleanup job only fires on synchronize and can cancel runs.""" workflow = WORKFLOW.read_text(encoding="utf-8") @@ -282,6 +311,158 @@ def test_cleanup_job_is_scoped_to_synchronize_events_with_actions_write() -> Non "github.event.action == 'synchronize'" ) in job assert "actions: write" in job.split("steps:", 1)[0] + assert 'gh api "repos/${TARGET_REPOSITORY}/actions/runs/${run_id}"' in job + assert job.index('actions/runs/${run_id}"') < job.index('actions/runs/${run_id}/cancel"') + assert "Cancellation requested for superseded Required OpenCode Review" in job + + +@pytest.mark.parametrize( + ("final_variant", "expects_cancel"), + ( + ("stale", True), + ("completed", False), + ("current", False), + ("other_pr", False), + ("api_failure", False), + ("missing_status", False), + ("unknown_status", False), + ("wrong_id", False), + ("array", False), + ("partial_failure", False), + ), +) +def test_cleanup_revalidates_exact_run_before_requesting_cancellation( + tmp_path: Path, final_variant: str, expects_cancel: bool +) -> None: + """Execute the production cleanup shell through its destructive boundary.""" + workflow = WORKFLOW.read_text(encoding="utf-8") + step = workflow.split( + " - name: Cancel queued and running OpenCode review runs for a superseded pull request head\n", + 1, + )[1] + script = textwrap.dedent(step.split(" run: |\n", 1)[1]) + fake_bin = tmp_path / "bin" + fake_bin.mkdir() + calls = tmp_path / "calls" + fake_gh = fake_bin / "gh" + fake_gh.write_text( + """#!/usr/bin/env bash +set -euo pipefail +printf '%s\n' "$*" >>"$FAKE_CALLS" +if [[ "$*" == *"repos/ContextualWisdomLab/example/pulls/1437"* ]]; then + if [[ "$*" == *"--jq .head.sha"* ]]; then printf '%s\n' "$LIVE_HEAD"; else printf '%s\n' "$LIVE_PR_JSON"; fi +elif [[ "$*" == *"actions/runs?status=queued"* ]]; then + printf '{"workflow_runs":[%s]}\n' "$FAKE_RUN_JSON" +elif [[ "$*" == *"actions/runs?status="* ]]; then + printf '%s\n' '{"workflow_runs":[]}' +elif [[ "$*" == "api repos/ContextualWisdomLab/example/actions/runs/100" ]]; then + [[ "$FINAL_VARIANT" != "api_failure" ]] || exit 17 + printf '%s\n' "$FINAL_RUN_JSON" +elif [[ "$*" == "api --method POST repos/ContextualWisdomLab/example/actions/runs/100/cancel" ]]; then + exit 0 +else + exit 1 +fi +""", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + stale_head = "b" * 40 + final_run = { + "id": 101 if final_variant == "wrong_id" else 100, + "status": "completed" if final_variant == "completed" else "queued", + "name": "Required OpenCode Review", + "event": "pull_request_target", + "head_sha": HEAD if final_variant == "current" else stale_head, + "display_title": "Required OpenCode Review", + "pull_requests": [{"number": 9999 if final_variant == "other_pr" else 1437, "head": {"sha": HEAD}}], + } + if final_variant == "missing_status": + final_run.pop("status") + elif final_variant == "unknown_status": + final_run["status"] = "mystery" + final_payload: object = final_run + if final_variant == "array": + final_payload = [final_run] + elif final_variant == "partial_failure": + final_payload = [final_run, 7] + env = { + **os.environ, + "PATH": f"{fake_bin}{os.pathsep}{os.environ['PATH']}", + "FAKE_CALLS": str(calls), + "LIVE_HEAD": HEAD, + "LIVE_PR_JSON": json.dumps({ + "state": "closed" if final_variant == "closed_pr" else "open", + "draft": False, + "head": {"sha": HEAD}, + }), + "FAKE_RUN_JSON": json.dumps({ + "id": 100, + "status": "queued", + "name": "Required OpenCode Review", + "event": "pull_request_target", + "head_sha": stale_head, + "display_title": "Required OpenCode Review", + "pull_requests": [{"number": 1437, "head": {"sha": HEAD}}], + }), + "FINAL_RUN_JSON": json.dumps(final_payload), + "FINAL_VARIANT": final_variant, + "TARGET_REPOSITORY": "ContextualWisdomLab/example", + "TARGET_PR_NUMBER": "1437", + "TARGET_PR_HEAD_SHA": HEAD, + "CURRENT_RUN_ID": "999", + } + result = subprocess.run(["bash", "-c", script], env=env, text=True, capture_output=True) + assert result.returncode == 0, result.stderr + log = calls.read_text(encoding="utf-8") + exact_get = "api repos/ContextualWisdomLab/example/actions/runs/100" + cancel = "api --method POST repos/ContextualWisdomLab/example/actions/runs/100/cancel" + assert exact_get in log + if expects_cancel: + assert cancel in log + assert log.index(exact_get) < log.index(cancel) + assert "Cancellation requested for superseded Required OpenCode Review run 100" in result.stdout + else: + assert "/actions/runs/100/cancel" not in log + assert "/actions/runs/100/force-cancel" not in log + + +def test_cleanup_requires_live_open_non_draft_pr(tmp_path: Path) -> None: + """A closed PR stops OpenCode synchronization cleanup before run listing.""" + workflow = WORKFLOW.read_text(encoding="utf-8") + step = workflow.split( + " - name: Cancel queued and running OpenCode review runs for a superseded pull request head\n", + 1, + )[1] + script = textwrap.dedent(step.split(" run: |\n", 1)[1]) + fake_gh = tmp_path / "gh" + calls = tmp_path / "calls" + fake_gh.write_text( + "#!/usr/bin/env bash\n" + "printf '%s\\n' \"$*\" >>\"$FAKE_CALLS\"\n" + "printf '%s\\n' \"$LIVE_PR_JSON\"\n", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + result = subprocess.run( + ["bash", "-c", script], + env={ + **os.environ, + "PATH": f"{tmp_path}{os.pathsep}{os.environ['PATH']}", + "FAKE_CALLS": str(calls), + "LIVE_PR_JSON": json.dumps({"state": "closed", "draft": False, "head": {"sha": HEAD}}), + "TARGET_REPOSITORY": "ContextualWisdomLab/example", + "TARGET_PR_NUMBER": "1437", + "TARGET_PR_HEAD_SHA": HEAD, + "CURRENT_RUN_ID": "999", + }, + text=True, + capture_output=True, + ) + assert result.returncode == 0, result.stderr + log = calls.read_text(encoding="utf-8") + assert "actions/runs?status=" not in log + assert "/cancel" not in log and "/force-cancel" not in log def test_required_verdict_has_one_executable_owner() -> None: diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index 803d43ab59..8ae9fc1718 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -408,7 +408,8 @@ def test_strix_serializes_provider_evidence_per_repository_and_pr() -> None: " strix:", 1 )[0] assert "github.event.action == 'synchronize'" in cleanup_job - assert 'endswith("@" + $head_sha)' in cleanup_job + assert '((.head_sha // "") | ascii_downcase) as $recorded_head' in cleanup_job + assert '$recorded_head != ($head_sha | ascii_downcase)' in cleanup_job assert "/force-cancel" in cleanup_job assert 'gh api "repos/${TARGET_REPOSITORY}/pulls/${TARGET_PR_NUMBER}"' in cleanup_job assert "could not verify the live pull request" in cleanup_job @@ -446,36 +447,90 @@ def test_strix_install_normalizes_executable_permissions_before_hashing() -> Non ) +def _strix_cleanup_candidate_jq() -> str: + """Extract the single jq predicate shared by list and final-run checks.""" + workflow = workflow_text("strix.yml") + marker = " candidate_jq='\n" + start = workflow.index(marker) + len(marker) + return workflow[start : workflow.index("\n '\n", start)] + + def test_strix_cleanup_uses_pr_metadata_when_custom_title_is_absent() -> None: """Required-workflow runs retain exact PR/head cleanup without run-name rendering.""" jq = shutil.which("jq") if jq is None: pytest.skip("jq is required to execute the production cleanup selector") - workflow = workflow_text("strix.yml") - marker = '--arg action "$PR_ACTION" --arg repo "$TARGET_REPOSITORY" --arg current "$CURRENT_RUN_ID" \'\n' - start = workflow.index(marker) + len(marker) - end = workflow.index('\n \' <<<"$runs_json"', start) + candidate_jq = _strix_cleanup_candidate_jq() runs = { "workflow_runs": [ - {"id": 1, "name": "Strix Security Scan", "event": "pull_request_target", "pull_requests": [{"number": 7, "head": {"sha": "old"}}]}, - {"id": 2, "name": "Strix Security Scan", "event": "pull_request_target", "pull_requests": [{"number": 7, "head": {"sha": "current"}}]}, - {"id": 3, "name": "Strix Security Scan", "event": "pull_request_target", "pull_requests": [{"number": 7}]}, - {"id": 4, "name": "Strix Security Scan", "event": "pull_request_target", "display_title": "Strix Security Scan owner/repo#7@old", "pull_requests": [{"number": 7, "head": {"sha": "current"}}]}, - {"id": 5, "name": "Strix Security Scan", "event": "pull_request_target", "pull_requests": [{"number": 8, "head": {"sha": "old"}}]}, + {"id": 1, "status": "queued", "name": "Strix Security Scan", "event": "pull_request_target", "head_sha": "b" * 40, "pull_requests": [{"number": 7, "head": {"sha": "b" * 40}}]}, + {"id": 2, "status": "queued", "name": "Strix Security Scan", "event": "pull_request_target", "head_sha": "a" * 40, "pull_requests": [{"number": 7, "head": {"sha": "a" * 40}}]}, + {"id": 3, "status": "queued", "name": "Strix Security Scan", "event": "pull_request_target", "pull_requests": [{"number": 7}]}, + {"id": 4, "status": "queued", "name": "Strix Security Scan", "event": "pull_request_target", "head_sha": "b" * 40, "display_title": "Strix Security Scan owner/repo#7@" + "b" * 40, "pull_requests": [{"number": 7, "head": {"sha": "a" * 40}}]}, + {"id": 5, "status": "queued", "name": "Strix Security Scan", "event": "pull_request_target", "head_sha": "b" * 40, "pull_requests": [{"number": 8, "head": {"sha": "b" * 40}}]}, ] } result = subprocess.run( - [jq, "-r", "--arg", "pr", "7", "--arg", "head_sha", "current", "--arg", "action", "synchronize", "--arg", "repo", "owner/repo", "--arg", "current", "99", workflow[start:end]], - input=json.dumps(runs), + [jq, "-r", "--arg", "pr", "7", "--arg", "head_sha", "a" * 40, "--arg", "action", "synchronize", "--arg", "repo", "owner/repo", f"{candidate_jq} | .id"], + input=json.dumps(runs["workflow_runs"]), text=True, capture_output=True, check=True, ) - assert result.stdout.splitlines() == ["1"] + assert result.stdout.splitlines() == ["1", "4"] + + +def test_strix_cleanup_uses_recorded_head_not_refreshed_pr_association() -> None: + """Real jq execution: mutable association heads cannot preserve stale runs.""" + jq = shutil.which("jq") + if jq is None: + pytest.skip("jq is required to execute the production cleanup selector") + candidate_jq = _strix_cleanup_candidate_jq() + runs = {"workflow_runs": [{ + "id": 41, + "status": "queued", + "name": "Strix Security Scan", + "event": "pull_request_target", + "head_sha": "b" * 40, + "display_title": "Strix Security Scan", + "pull_requests": [{"number": 7, "head": {"sha": "a" * 40}}], + }]} + result = subprocess.run( + [jq, "-r", "--arg", "pr", "7", "--arg", "head_sha", "a" * 40, + "--arg", "action", "synchronize", "--arg", "repo", "owner/repo", + f"{candidate_jq} | .id"], + input=json.dumps(runs["workflow_runs"]), text=True, capture_output=True, check=True, + ) + assert result.stdout.splitlines() == ["41"] + + +def test_strix_cleanup_preserves_conflicting_or_malformed_recorded_revision() -> None: + """Real jq execution: ambiguous run revisions fail closed.""" + jq = shutil.which("jq") + if jq is None: + pytest.skip("jq is required to execute the production cleanup selector") + candidate_jq = _strix_cleanup_candidate_jq() + runs = {"workflow_runs": [ + {"id": 51, "status": "queued", "name": "Strix Security Scan", "event": "pull_request_target", "head_sha": "bad", "pull_requests": [{"number": 7}]}, + {"id": 52, "status": "queued", "name": "Strix Security Scan", "event": "pull_request_target", "head_sha": "b" * 40, "display_title": "Strix Security Scan owner/repo#7@" + "a" * 40, "pull_requests": [{"number": 7}]}, + {"id": 53, "status": "queued", "name": "Strix Security Scan", "event": "pull_request_target", "head_sha": "b" * 40, "display_title": "Strix Security Scan other/repo#7@" + "b" * 40, "pull_requests": [{"number": 7}]}, + ]} + result = subprocess.run( + [jq, "-r", "--arg", "pr", "7", "--arg", "head_sha", "a" * 40, + "--arg", "action", "synchronize", "--arg", "repo", "owner/repo", + candidate_jq], + input=json.dumps(runs["workflow_runs"]), text=True, capture_output=True, check=True, + ) + assert result.stdout == "" def _run_strix_cleanup( - tmp_path: Path, pull_states: list[dict[str, object]], *, action: str = "synchronize" + tmp_path: Path, + pull_states: list[dict[str, object]], + *, + action: str = "synchronize", + final_variant: str = "stale", + listed_head: str | None = None, ) -> str: """Execute the production cleanup step against a stateful fake ``gh``.""" jq = shutil.which("jq") @@ -510,26 +565,61 @@ def _run_strix_cleanup( exit 0 fi if [[ "$*" == *"actions/runs?status=queued"* ]]; then - printf '%s\n' '{"workflow_runs":[{"id":100,"name":"Strix Security Scan","event":"pull_request_target","pull_requests":[{"number":7,"head":{"sha":"old"}}]}]}' + printf '{"workflow_runs":[%s]}\n' "$FAKE_RUN_JSON" exit 0 fi if [[ "$*" == *"actions/runs?status="* ]]; then printf '%s\n' '{"workflow_runs":[]}' exit 0 fi +if [[ "$*" == "api repos/owner/repo/actions/runs/100" ]]; then + [[ "$FINAL_VARIANT" != "api_failure" ]] || exit 17 + printf '%s\n' "$FINAL_RUN_JSON" + exit 0 +fi exit 0 """, encoding="utf-8", ) fake_gh.chmod(0o755) + candidate_head = listed_head or "b" * 40 + final_run = { + "id": 101 if final_variant == "wrong_id" else 100, + "status": "completed" if final_variant == "completed" else "queued", + "name": "Strix Security Scan", + "event": "pull_request_target", + "head_sha": "a" * 40 if final_variant == "current" else candidate_head, + "display_title": "Strix Security Scan", + "pull_requests": [{"number": 8 if final_variant == "other_pr" else 7, "head": {"sha": "a" * 40}}], + } + if final_variant == "missing_status": + final_run.pop("status") + elif final_variant == "unknown_status": + final_run["status"] = "mystery" + final_payload: object = final_run + if final_variant == "array": + final_payload = [final_run] + elif final_variant == "partial_failure": + final_payload = [final_run, 7] env = { **os.environ, "PATH": f"{fake_bin}{os.pathsep}{os.environ['PATH']}", "FAKE_CALLS": str(calls), "FAKE_PULLS": str(pulls), + "FAKE_RUN_JSON": json.dumps({ + "id": 100, + "status": "queued", + "name": "Strix Security Scan", + "event": "pull_request_target", + "head_sha": candidate_head, + "display_title": "Strix Security Scan", + "pull_requests": [{"number": 7, "head": {"sha": "a" * 40}}], + }), + "FINAL_RUN_JSON": json.dumps(final_payload), + "FINAL_VARIANT": final_variant, "TARGET_REPOSITORY": "owner/repo", "TARGET_PR_NUMBER": "7", - "TARGET_PR_HEAD_SHA": "current", + "TARGET_PR_HEAD_SHA": "a" * 40, "PR_ACTION": action, "CURRENT_RUN_ID": "999", } @@ -542,7 +632,7 @@ def test_old_strix_cleanup_never_lists_or_cancels_after_live_head_advanced( ) -> None: """A late old synchronize job must stop before selecting current runs.""" calls = _run_strix_cleanup( - tmp_path, [{"state": "open", "head": {"sha": "newer"}}] * 5 + tmp_path, [{"state": "open", "head": {"sha": "c" * 40}}] * 5 ) assert "actions/runs?status=" not in calls @@ -557,10 +647,10 @@ def test_strix_cleanup_revalidates_after_selection_before_cancellation( calls = _run_strix_cleanup( tmp_path, [ - {"state": "open", "draft": False, "head": {"sha": "current"}}, - {"state": "open", "draft": False, "head": {"sha": "newer"}}, + {"state": "open", "draft": False, "head": {"sha": "a" * 40}}, + {"state": "open", "draft": False, "head": {"sha": "c" * 40}}, ] - + [{"state": "open", "draft": False, "head": {"sha": "newer"}}] * 4, + + [{"state": "open", "draft": False, "head": {"sha": "c" * 40}}] * 4, ) assert "actions/runs?status=queued" in calls @@ -568,12 +658,63 @@ def test_strix_cleanup_revalidates_after_selection_before_cancellation( assert "/actions/runs/100/force-cancel" not in calls -def test_strix_draft_transition_cancels_current_scan(tmp_path: Path) -> None: - """A verified Draft transition retires the current expensive Strix run.""" +def test_strix_cleanup_revalidates_exact_run_before_requesting_cancellation( + tmp_path: Path, +) -> None: + """The destructive boundary refreshes both live PR and candidate run.""" + calls = _run_strix_cleanup( + tmp_path, + [{"state": "open", "draft": False, "head": {"sha": "a" * 40}}] * 7, + ) + exact_get = "api repos/owner/repo/actions/runs/100" + cancel = "api --method POST repos/owner/repo/actions/runs/100/cancel" + assert exact_get in calls + assert cancel in calls + assert calls.index(exact_get) < calls.index(cancel) + assert "Cancellation requested for obsolete Strix run" in workflow_text("strix.yml") + + +@pytest.mark.parametrize( + "final_variant", + ( + "completed", + "current", + "other_pr", + "api_failure", + "missing_status", + "unknown_status", + "wrong_id", + "array", + "partial_failure", + ), +) +def test_strix_final_run_revalidation_blocks_cancellation( + tmp_path: Path, final_variant: str +) -> None: + """A changed or unavailable exact run is preserved after list selection.""" + calls = _run_strix_cleanup( + tmp_path, + [{"state": "open", "draft": False, "head": {"sha": "a" * 40}}] * 7, + final_variant=final_variant, + ) + assert "api repos/owner/repo/actions/runs/100" in calls + assert "/actions/runs/100/cancel" not in calls + assert "/actions/runs/100/force-cancel" not in calls + + +@pytest.mark.parametrize( + ("action", "live_state", "live_draft"), + (("converted_to_draft", "open", True), ("closed", "closed", False)), +) +def test_strix_inactive_transition_cancels_current_scan( + tmp_path: Path, action: str, live_state: str, live_draft: bool +) -> None: + """A verified inactive transition retires even the current-head Strix run.""" calls = _run_strix_cleanup( tmp_path, - [{"state": "open", "draft": True, "head": {"sha": "current"}}] * 6, - action="converted_to_draft", + [{"state": live_state, "draft": live_draft, "head": {"sha": "a" * 40}}] * 6, + action=action, + listed_head="a" * 40, ) assert "/actions/runs/100/cancel" in calls @@ -604,7 +745,7 @@ def test_pull_request_close_events_cancel_superseded_runs_without_heavy_jobs() - ) in workflow assert "DISPATCH_REPOSITORY" not in workflow assert "TARGET_PR_HEAD_SHA" in workflow - assert 'select(.event == "pull_request_target")' in workflow + assert '.event == "pull_request_target"' in workflow assert 'select(.event == "repository_dispatch")' not in workflow assert "(.pull_requests // [])" in workflow assert ".head.sha // \"\"" in workflow From 4d291513d6d8c5cb047115506c2e14f6d6a42e75 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sat, 5 Sep 2026 17:17:28 +0900 Subject: [PATCH 5/7] =?UTF-8?q?docs(ci):=20=EA=B2=80=EC=A6=9D=20=EC=88=98?= =?UTF-8?q?=EC=B9=98=EB=8A=94=20exact-head=20PR=20=EA=B8=B0=EB=A1=9D?= =?UTF-8?q?=EC=9C=BC=EB=A1=9C=20=EC=97=B0=EA=B2=B0?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/doctoring/review-rerun-concurrency-isolation.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/doctoring/review-rerun-concurrency-isolation.md b/docs/doctoring/review-rerun-concurrency-isolation.md index 0fa3e44511..407bb899a1 100644 --- a/docs/doctoring/review-rerun-concurrency-isolation.md +++ b/docs/doctoring/review-rerun-concurrency-isolation.md @@ -58,8 +58,8 @@ final-response validation. The production-shell tests now prove that completed or current runs, another PR, API failure, missing/unknown status, a different run ID, arrays, and partial/invalid responses issue neither cancel nor force-cancel. Closed/Draft Strix cleanup and the live-PR guard have separate regression cases. -The final 14-file focused suite passes 681 tests; these are local request-boundary -checks, not hosted terminal-cancellation evidence. +The 14-file focused suite includes these local request-boundary checks, not +hosted terminal-cancellation evidence. Exact-head results are recorded in the PR. Run the focused suite: From db105e9d4688fa72dd1d21bfb5549b6ee44ff2bd Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sat, 5 Sep 2026 18:08:49 +0900 Subject: [PATCH 6/7] fix(ci): coalesce first Strix branch push runs --- .github/workflows/strix.yml | 4 +- .../review-rerun-concurrency-isolation.md | 24 +++++- docs/product-technical-gap-baseline.md | 8 ++ tests/test_review_rerun_concurrency.py | 79 +++++++++++++++++-- 4 files changed, 106 insertions(+), 9 deletions(-) diff --git a/.github/workflows/strix.yml b/.github/workflows/strix.yml index 750abde661..72229cec73 100644 --- a/.github/workflows/strix.yml +++ b/.github/workflows/strix.yml @@ -78,7 +78,9 @@ concurrency: github.event.client_payload.target_repository || github.repository }}-${{ github.run_attempt > 1 && format('rerun-{0}', github.run_id) || github.event.pull_request.number || - github.event.client_payload.pr_number || github.run_id }} + github.event.client_payload.pr_number || + github.event_name == 'push' && github.ref_type == 'branch' && github.ref || + github.run_id }} cancel-in-progress: true # Scorecard Token-Permissions (alert #43): keep the workflow-level token diff --git a/docs/doctoring/review-rerun-concurrency-isolation.md b/docs/doctoring/review-rerun-concurrency-isolation.md index 407bb899a1..ea27e669c1 100644 --- a/docs/doctoring/review-rerun-concurrency-isolation.md +++ b/docs/doctoring/review-rerun-concurrency-isolation.md @@ -19,6 +19,24 @@ First attempts retain the workflow/repository/PR key and cancellation policy. No jobs, dependencies, permissions, provider routes, or gate exceptions are added. Live-head admission and publication checks remain mandatory. +The Strix workflow's first branch-push attempt now uses its repository and full +Git ref as the cancellation key. This lets a newer commit on the same branch +retire an older first attempt without combining different repositories or +branches. The branch key applies only when `event_name == 'push'`, +`ref_type == 'branch'`, and `github.ref` is present. Reruns still take the +run-ID key first; tags, schedules, manual or non-PR dispatches, unknown events, +and missing refs retain the existing per-run key. PR-native and dispatched PR +keys are unchanged. Release, deployment, and migration workflows are outside +this Strix-only change. + +Read-only REST evidence before this change found three simultaneous central +Strix push runs on `main`: run `33933530334` at +`b5efbc2762e472e4a380b0503b1f050f76fbb008`, run `33932271770` at +`1b65dbc35e7183722ad77894e2d80b39993be90d`, and run `33928897846` at +`a9aeee8fc94ad6002a059b380b268590ce496ef0`. This observation motivates future +branch-push prevention only. It neither cancels those existing runs nor proves +that the organization reached a 60-job ceiling. + Read-only follow-up also exposed a second cancellation risk in the existing same-head coalescer: REST PR associations can move to a newer head while the run retains its original `head_sha`. The shared identity matcher now requires @@ -42,10 +60,14 @@ runs. Successful API calls are logged as cancellation requests, not completion. `tests/test_review_rerun_concurrency.py` evaluates the actual group expressions with a restricted stdlib AST interpreter, without `eval`, workflow execution, or a new dependency. Before the workflow edits, 17 assertions failed and five -passed. The tests cover numeric/string attempts, distinct retries, first-push +passed. The tests cover numeric/string attempts, distinct retries, PR first-attempt coalescing, repository/PR isolation, non-PR fallback, and native/dispatch parity. Existing Noema and central dispatch cleanup tests also exercise retry metadata; they prove selection and API requests, not GitHub terminal cancellation. +The branch-push regression separately produced `1 failed, 27 passed` before +the Strix expression change and `28 passed` afterward. Its literal expectations +cover same-branch coalescing, branch/repository isolation, rerun priority, and +run-ID preservation for tags, schedules, unknown events, and missing refs. The coalescer's separate three-file suite passes 57 tests with 100% statement and branch coverage (252 statements, 118 branches). Six new tests failed on the old source before the shared guard; an older positive fixture was corrected diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 9a0a5d168a..6e0b84c3fc 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -91,6 +91,14 @@ Local shell regressions prove request selection and suppression, not terminal GitHub cancellation; log messages therefore report requests rather than completion. Immediate stale-retry recovery under runner saturation and protected hosted delivery remain unverified; this does not close either gap or the full objective. +The same bounded follow-up observed central Strix `main` push runs +`33933530334` (`b5efbc2762e472e4a380b0503b1f050f76fbb008`), `33932271770` +(`1b65dbc35e7183722ad77894e2d80b39993be90d`), and `33928897846` +(`a9aeee8fc94ad6002a059b380b268590ce496ef0`) running concurrently. Strix now +coalesces only first branch-push attempts by repository/full ref while retaining +run-ID isolation for reruns and all non-branch-push events. This prevents future +same-branch accumulation; it does not cancel those runs, prove a 60-job ceiling, +or establish that runner saturation is resolved. | Gap ID | 현재 관측 | 구매자 영향 | 우선 구현/검증 | |---|---|---|---| diff --git a/tests/test_review_rerun_concurrency.py b/tests/test_review_rerun_concurrency.py index 50d8065644..ded0897929 100644 --- a/tests/test_review_rerun_concurrency.py +++ b/tests/test_review_rerun_concurrency.py @@ -35,11 +35,15 @@ def expression_value(node: ast.AST, context: dict): return value return value if isinstance(node, ast.Compare) and len(node.ops) == 1: - assert isinstance(node.ops[0], ast.Gt), "unsupported comparison" - # GitHub coerces numeric strings for relational comparisons. - return float(expression_value(node.left, context)) > float( - expression_value(node.comparators[0], context) - ) + left = expression_value(node.left, context) + right = expression_value(node.comparators[0], context) + if isinstance(node.ops[0], ast.Gt): + # GitHub coerces numeric strings for relational comparisons. + return float(left) > float(right) + if isinstance(node.ops[0], ast.Eq): + assert isinstance(left, str) and isinstance(right, str) + return left.casefold() == right.casefold() + raise AssertionError("unsupported comparison") if isinstance(node, ast.Call): assert isinstance(node.func, ast.Name) and node.func.id == "format" assert not node.keywords and isinstance(node.args[0], ast.Constant) @@ -50,7 +54,8 @@ def expression_value(node: ast.AST, context: dict): def review_group(filename, job, *, run_id, attempt=1, pr=7, - repository="ContextualWisdomLab/example", dispatched=False): + repository="ContextualWisdomLab/example", dispatched=False, + event_name=None, ref="", ref_type=""): """Render the declared YAML group using an explicit event/needs snapshot.""" source = (WORKFLOWS / filename).read_text(encoding="utf-8") if job: @@ -70,7 +75,10 @@ def review_group(filename, job, *, run_id, attempt=1, pr=7, ) if pr else {} context = { "github": {"repository": repository, "run_id": str(run_id), - "run_attempt": attempt, "event": event}, + "run_attempt": attempt, "event": event, + "event_name": event_name or ("repository_dispatch" if dispatched else ( + "pull_request_target" if pr else "unknown" + )), "ref": ref, "ref_type": ref_type}, "needs": {"validate-pr-metadata": {"outputs": { "target_repository": repository, "pr_number": str(pr) if pr else "", }}}, @@ -126,3 +134,60 @@ def test_dispatched_reviews_keep_pr_identity_and_rerun_isolation(filename, prefi assert review_group(filename, None, run_id=101, attempt=2, dispatched=True) == ( f"{prefix}-ContextualWisdomLab/example-rerun-101" ) + + +def test_strix_first_branch_push_coalesces_only_the_same_repository_and_ref(): + """A newer first branch push must replace only its same-branch predecessor.""" + current = review_group( + "strix.yml", None, run_id=202, pr=None, + event_name="push", ref_type="branch", ref="refs/heads/main", + ) + assert current == "strix-security-scan-ContextualWisdomLab/example-refs/heads/main" + assert current == review_group( + "strix.yml", None, run_id=101, pr=None, + event_name="push", ref_type="branch", ref="refs/heads/main", + ) + assert current != review_group( + "strix.yml", None, run_id=203, pr=None, + event_name="push", ref_type="branch", ref="refs/heads/release", + ) + assert current != review_group( + "strix.yml", None, run_id=204, pr=None, + repository="ContextualWisdomLab/other", event_name="push", + ref_type="branch", ref="refs/heads/main", + ) + + +@pytest.mark.parametrize( + "event_name,ref_type,ref", + [ + ("push", "tag", "refs/tags/v1.0.0"), + ("schedule", "", "refs/heads/main"), + ("workflow_dispatch", "branch", "refs/heads/main"), + ("repository_dispatch", "branch", "refs/heads/main"), + ("unknown", "", "refs/heads/main"), + ("push", "branch", ""), + ], +) +def test_strix_non_branch_push_first_attempts_remain_run_isolated( + event_name, ref_type, ref, +): + """Non-branch-push events and missing refs must not cancel sibling runs.""" + first = review_group( + "strix.yml", None, run_id=101, pr=None, + event_name=event_name, ref_type=ref_type, ref=ref, + ) + second = review_group( + "strix.yml", None, run_id=202, pr=None, + event_name=event_name, ref_type=ref_type, ref=ref, + ) + assert first == "strix-security-scan-ContextualWisdomLab/example-101" + assert second == "strix-security-scan-ContextualWisdomLab/example-202" + + +def test_strix_old_branch_push_rerun_remains_run_isolated(): + """A branch-push rerun must not cancel a newer first attempt.""" + assert review_group( + "strix.yml", None, run_id=101, attempt=2, pr=None, + event_name="push", ref_type="branch", ref="refs/heads/main", + ) == "strix-security-scan-ContextualWisdomLab/example-rerun-101" From b981306a54ae0116934f88f66095e5a737d0c10a Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sun, 6 Sep 2026 22:46:07 +0900 Subject: [PATCH 7/7] fix(actions): isolate dispatch workflow reruns Signed-off-by: Seongho Bae --- .../workflows/opencode-review-dispatch.yml | 1 + ...t_pr_review_autofix_nvidia_nim_contract.py | 2 +- tests/test_review_rerun_concurrency.py | 23 +++++++++++++++++++ 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/.github/workflows/opencode-review-dispatch.yml b/.github/workflows/opencode-review-dispatch.yml index 4e49904a91..f99cd7552c 100644 --- a/.github/workflows/opencode-review-dispatch.yml +++ b/.github/workflows/opencode-review-dispatch.yml @@ -30,6 +30,7 @@ concurrency: group: >- opencode-review-dispatch-${{ github.event.client_payload.target_repository || github.repository }}-${{ + github.run_attempt > 1 && format('rerun-{0}', github.run_id) || github.event.client_payload.pr_number || github.run_id }} cancel-in-progress: true diff --git a/tests/test_pr_review_autofix_nvidia_nim_contract.py b/tests/test_pr_review_autofix_nvidia_nim_contract.py index b3eb16a252..96751bdd6c 100644 --- a/tests/test_pr_review_autofix_nvidia_nim_contract.py +++ b/tests/test_pr_review_autofix_nvidia_nim_contract.py @@ -17,7 +17,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 = "4e49904a91e2876400b556f449fd04ceff630e4f" +REVIEW_DISPATCH_BLOB_SHA = "f99cd7552c02bd8507e81b3c2f68216d19f5318b" def _workflow_text(path: Path) -> str: diff --git a/tests/test_review_rerun_concurrency.py b/tests/test_review_rerun_concurrency.py index ded0897929..b065b975df 100644 --- a/tests/test_review_rerun_concurrency.py +++ b/tests/test_review_rerun_concurrency.py @@ -136,6 +136,29 @@ def test_dispatched_reviews_keep_pr_identity_and_rerun_isolation(filename, prefi ) +def test_opencode_dispatch_workflow_reruns_cannot_cancel_current_dispatch(): + """Workflow admission must isolate retries before the guarded job can start.""" + current = review_group( + "opencode-review-dispatch.yml", None, run_id=202, dispatched=True + ) + same_pr_first_attempt = review_group( + "opencode-review-dispatch.yml", None, run_id=101, dispatched=True + ) + old_retry = review_group( + "opencode-review-dispatch.yml", None, run_id=101, attempt=2, dispatched=True + ) + other_retry = review_group( + "opencode-review-dispatch.yml", None, run_id=303, attempt=2, dispatched=True + ) + + assert current == "opencode-review-dispatch-ContextualWisdomLab/example-7" + assert same_pr_first_attempt == current + assert old_retry == "opencode-review-dispatch-ContextualWisdomLab/example-rerun-101" + assert other_retry == "opencode-review-dispatch-ContextualWisdomLab/example-rerun-303" + assert old_retry != current + assert other_retry != old_retry + + def test_strix_first_branch_push_coalesces_only_the_same_repository_and_ref(): """A newer first branch push must replace only its same-branch predecessor.""" current = review_group(