diff --git a/.github/workflows/strix.yml b/.github/workflows/strix.yml index f15b29f564..be12f93f1d 100644 --- a/.github/workflows/strix.yml +++ b/.github/workflows/strix.yml @@ -109,8 +109,19 @@ concurrency: github.event.pull_request.number || github.event.client_payload.pr_number || (github.event_name == 'push' && format('push-{0}', github.ref_name)) || - github.run_id }} - cancel-in-progress: true + github.run_id }}-${{ + github.event.action == 'closed' && github.run_id || + github.event_name == 'push' && 'protected-ref' || + github.event.pull_request.head.sha || + github.event.client_payload.pr_head_sha || github.run_id }} + # Draft/Ready and duplicate admission events can share one exact head. Do + # not let those lifecycle events destroy an executing scanner verdict. The + # metadata-only cleanup job below remains the cancellation owner for a + # verified superseded head or closed pull request. Closed events use their + # unique run id above so cleanup cannot queue behind the scan it must stop. + # Only a newer protected-branch push cancels in-progress work; PR lifecycle + # events use exact-head identities and never stop same-head provider work. + cancel-in-progress: ${{ github.event_name == 'push' }} # Scorecard Token-Permissions (alert #43): keep the workflow-level token # read-only and scope same-repo status publication to the Strix scan job. @@ -242,8 +253,10 @@ jobs: cancel-superseded-pr-runs: if: >- - github.event_name == 'pull_request_target' && - (github.event.action == 'synchronize' || github.event.action == 'converted_to_draft' || github.event.action == 'closed') + (github.event_name == 'pull_request_target' && + (github.event.action == 'synchronize' || github.event.action == 'closed')) || + (github.event_name == 'repository_dispatch' && + github.event.client_payload.pr_number != '') # Idempotent per PR: a fresh sweep re-verifies live state (live_target_matches # below) before selecting or cancelling anything, so it fully subsumes # whatever an older, not-yet-run instance would have done. cancel-in-progress @@ -257,7 +270,9 @@ jobs: concurrency: group: >- cancel-superseded-pr-runs-${{ + github.event.client_payload.target_repository || github.event.pull_request.base.repo.full_name || github.repository }}-${{ + github.event.client_payload.pr_number || github.event.pull_request.number || github.run_id }} cancel-in-progress: true runs-on: ubuntu-24.04 @@ -277,10 +292,11 @@ jobs: pull-requests: read env: GH_TOKEN: ${{ secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN || github.token }} - TARGET_REPOSITORY: ${{ github.event.pull_request.base.repo.full_name || github.repository }} - TARGET_PR_NUMBER: ${{ github.event.pull_request.number }} - TARGET_PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} - PR_ACTION: ${{ github.event.action }} + RUN_REPOSITORY: ${{ github.repository }} + TARGET_REPOSITORY: ${{ github.event.client_payload.target_repository || github.event.pull_request.base.repo.full_name || github.repository }} + TARGET_PR_NUMBER: ${{ github.event.client_payload.pr_number || github.event.pull_request.number }} + TARGET_PR_HEAD_SHA: ${{ github.event.client_payload.pr_head_sha || github.event.pull_request.head.sha }} + PR_ACTION: ${{ github.event_name == 'repository_dispatch' && 'synchronize' || github.event.action }} CURRENT_RUN_ID: ${{ github.run_id }} steps: - name: Cancel queued and running scans for superseded or inactive pull requests @@ -289,18 +305,16 @@ jobs: set -euo pipefail live_target_matches() { - local live_pr_json live_state live_draft live_head + local live_pr_json live_state live_head if ! live_pr_json="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${TARGET_PR_NUMBER}" 2>/tmp/strix-cleanup-gh-error)"; then echo "::warning::Strix cleanup could not verify the live pull request; leaving runs unchanged." sed 's/^/ /' /tmp/strix-cleanup-gh-error >&2 || true return 1 fi 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_head" = "$TARGET_PR_HEAD_SHA" ] && { { [ "$PR_ACTION" = "closed" ] && [ "$live_state" = "closed" ]; } || - { [ "$PR_ACTION" = "converted_to_draft" ] && [ "$live_state" = "open" ] && [ "$live_draft" = "true" ]; } || { [ "$PR_ACTION" = "synchronize" ] && [ "$live_state" = "open" ]; } } } @@ -311,7 +325,7 @@ jobs: echo "::notice::Strix cleanup target changed before run selection; leaving runs unchanged." return 0 fi - local runs_url="repos/${TARGET_REPOSITORY}/actions/runs?status=${status}&per_page=100" + local runs_url="repos/${RUN_REPOSITORY}/actions/runs?status=${status}&per_page=100" local runs_json if ! runs_json="$(gh api --paginate "$runs_url" 2>/tmp/strix-cleanup-gh-error)"; then echo "::warning::Strix cleanup could not inspect ${TARGET_REPOSITORY}; leaving runs unchanged." @@ -320,25 +334,26 @@ jobs: 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" ' + --arg action "$PR_ACTION" --arg repo "$TARGET_REPOSITORY" --arg run_repo "$RUN_REPOSITORY" \ + --arg current "$CURRENT_RUN_ID" ' .workflow_runs[] | select((.id | tostring) != $current) | select(.name == "Strix Security Scan") - | select(.event == "pull_request_target") + | select(.event == "pull_request_target" or .event == "repository_dispatch") + | (($run_repo | ascii_downcase) == ($repo | ascii_downcase)) as $metadata_is_target_repository | ((.display_title // "") | startswith("Strix Security Scan " + $repo + "#" + $pr + "@")) as $title_matches - | ((.pull_requests // []) | any((.number | tostring) == $pr)) as $metadata_matches + | ($metadata_is_target_repository and ((.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( + | ($metadata_is_target_repository and ((.pull_requests // []) | any( ((.number | tostring) == $pr) and ((.head.sha // "") | ascii_downcase) == ($head_sha | ascii_downcase) - )) as $metadata_is_current - | ((.pull_requests // []) | any( + ))) as $metadata_is_current + | ($metadata_is_target_repository and ((.pull_requests // []) | any( ((.number | tostring) == $pr) and ((.head.sha // "") != "") - )) as $metadata_has_head + ))) 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 @@ -352,11 +367,11 @@ jobs: echo "::notice::Strix cleanup target changed before cancellation; leaving runs unchanged." return 0 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}." + if gh api --method POST "repos/${RUN_REPOSITORY}/actions/runs/${run_id}/cancel" >/dev/null 2>/tmp/strix-cleanup-cancel-error || + gh api --method POST "repos/${RUN_REPOSITORY}/actions/runs/${run_id}/force-cancel" >/dev/null 2>>/tmp/strix-cleanup-cancel-error; then + echo "Cancelled obsolete Strix run ${run_id} in ${RUN_REPOSITORY} for ${TARGET_REPOSITORY} 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." + echo "::warning::Strix cleanup could not cancel run ${run_id} in ${RUN_REPOSITORY}; it may have finished or the credential lacks Actions write access." sed 's/^/ /' /tmp/strix-cleanup-cancel-error >&2 || true fi done <<<"$run_ids" @@ -367,8 +382,13 @@ jobs: done strix: - needs: [changed-scope, admit-current-head] - if: needs.changed-scope.outputs.code == 'true' && needs.admit-current-head.outputs.admitted == 'true' + needs: [changed-scope, admit-current-head, cancel-superseded-pr-runs] + if: >- + always() && !cancelled() && + needs.changed-scope.outputs.code == 'true' && + needs.admit-current-head.outputs.admitted == 'true' && + (needs.cancel-superseded-pr-runs.result == 'success' || + needs.cancel-superseded-pr-runs.result == 'skipped') # Large, actively-growing repositories (e.g. contextual-orchestrator) can # legitimately require well over two hours to scan -- this org's own # standing operating directive accepts that central OpenCode/Strix/Noema diff --git a/CHANGELOG.md b/CHANGELOG.md index bf192f6a9e..81a324c4af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,26 @@ +### Strix reruns bind the exact PR base as well as the head + +- The scheduler now rejects a failed Strix job whose native pull-request + association belongs to an older base SHA, and revalidates both live base and + head immediately before the rerun mutation. Retargeting an unchanged head can + no longer replay an old-base scanner job as current evidence. + +### Strix preserves PR evidence and retires superseded push scans + +- Workflow-level `cancel-in-progress` is true only for `push`; Draft/Ready and + duplicate same-head PR admission events cannot destroy an executing scanner + verdict. The PR group is exact-head scoped, so a synchronized new head can start + its metadata-only superseded-run cleanup without waiting behind the old scan; + its provider job now waits for that cleanup to finish. A closed event uses its + unique run id for the same reason. Draft transitions preserve the current scan, + while the live-revalidated cleanup job covers both native and dispatched PR + runs and cancels only verified superseded heads or a closed pull request. + Native PR metadata is accepted only when the run and target repositories match, + preventing same-number cross-repository cancellation. No provider deadline or + merge-gate relaxation was added. This repairs the cancellation pattern + seen in runs `34068478185`, `34067942252`, and PR #1999 run `34067362987`, + while preserving #1938's protected-ref push coalescing and cancellation. + ### Failed-check finding names the Strix sandbox instead of the gateway - `opencode-review-dispatch.yml`'s `emit_strix_provider_failure_finding` rendered one fixed finding for every `STRIX_PROVIDER_UNAVAILABLE` line, whose Root cause read "The contextual-orchestrator gateway or its discovered provider pool was unavailable for this run". `#1953` had just given the Strix sandbox bootstrap failure its own second verdict token (`STRIX_SANDBOX_UNAVAILABLE`) precisely because that attribution is wrong for it -- the sandbox container never reaches its Caido proxy, so the run dies before the gateway serves anything -- and this consumer re-applied the wrong attribution one step downstream, into the review findings and the failure census. The emitter now branches on the second token: a sandbox verdict gets a finding that names Strix's sandbox, says the verdict does not name the gateway, and tells the reader not to change gateway or provider configuration on its strength. A `STRIX_PROVIDER_UNAVAILABLE` line without the token keeps its existing text verbatim, so the gateway class has no regression surface. No test covered this finding text at all before (`gateway or its discovered provider pool` matched nothing under `tests/`); `tests/test_opencode_dispatch_strix_sandbox_finding.py` now runs the production emitter from the published run block and pins both directions plus the no-signal case. Refs #1953, #1935. diff --git a/docs/doctoring/scheduler-explicit-open-live-dispatch-guard.md b/docs/doctoring/scheduler-explicit-open-live-dispatch-guard.md new file mode 100644 index 0000000000..333adfccea --- /dev/null +++ b/docs/doctoring/scheduler-explicit-open-live-dispatch-guard.md @@ -0,0 +1,48 @@ +# Scheduler의 명시적 OPEN·현재 head 확인 + +## 원인과 범위 + +#1902의 후속 조사에서 CodeQL 복구 primitive보다 먼저 고칠 공통 결함을 확인했다. +`4bf80b99b6908c0323ac406d7e30e8346e09a50d`의 +`scripts/ci/pr_review_merge_scheduler_core.py`는 GraphQL 공통 PR fragment에서 +`state`를 요청하지 않았고, REST PR 정규화에서도 그 필드를 보존하지 않았다. +그런데 `live_dispatch_head_matches`는 누락되거나 빈 state를 OPEN으로 취급했다. +단일 PR 조회는 닫힌 PR도 반환하므로 head가 그대로면 닫힌 PR을 허용할 수 있었다. +또한 양쪽 head를 빈 문자열로 대체해 비교했으므로 빈 값끼리도 일치했다. + +영향 범위는 OpenCode repository dispatch, Strix의 기존 job rerun, +Strix repository dispatch 직전의 공통 guard다. 이번 수정은 이 세 경로의 +새 실행 요청을 막는 조건만 다룬다. 앞서 수행되는 stale-run cleanup의 순서나 +cancellation 정책은 바꾸지 않는다. + +## 수정 + +- GraphQL 공통 fragment가 PR state를 실제로 요청한다. +- REST fallback은 원본 state를 대문자로 보존하고, 누락은 빈 값으로 남긴다. +- guard는 정확히 한 PR, 명시적 `OPEN`, 양쪽의 문자열 타입 40자리 hex SHA, + 대소문자를 제외한 동일 head를 모두 요구한다. +- 기존 정상 fixture는 `OPEN`을 명시한다. 누락 사례를 정상 fixture로 대체하지 않는다. + +토큰, 권한, trigger, queue, concurrency, dispatch payload는 변경하지 않았다. +CodeQL primitive도 추가하지 않았다. 조회 직후 PR 상태가 바뀔 수 있는 경쟁 조건과 +중복 전송의 원자성은 여전히 미해결이며, 이 guard는 exact-once 보장이 아니다. +Cross-repo target callback의 Actions-write 권한도 별도 미해결 조건이다. + +## 회귀 검증 + +`tests/test_scheduler_live_dispatch_guard.py`는 실제 guard와 세 caller를 실행하고 +외부 API 및 실행 요청만 대체한다. 누락·빈 값·CLOSED·MERGED·UNKNOWN은 dispatch와 +rerun에 도달하지 않아야 하며, OPEN의 정상 경로는 계속 도달해야 한다. +별도 사례가 빈 값, 잘못된 길이, 비-hex, 비문자열, 서로 다른 SHA를 거부하고 +GraphQL 실제 query와 REST fallback의 state 전달을 확인한다. + +Production 수정 전 새 회귀는 17 failed / 19 passed였다. 이후 실제 live head만 +잘못된 사례 두 건도 추가했다. 최종 관련 5파일은 `-W error`를 적용해 정상 환경에서 +380 passed, `GITHUB_ACTIONS=true` 환경에서도 380 passed를 확인했다. +검증 명령은 다음과 같다. + +```sh +python -m pytest -q -W error tests/test_scheduler_live_dispatch_guard.py tests/test_pr_review_merge_scheduler.py tests/test_strix_rerun_job_selection.py tests/test_repository_branch_coverage_review_schedulers.py tests/test_pr_review_fix_scheduler_rest_workflow_identity.py +``` + +로컬 회귀 통과는 실제 GitHub dispatch, protected merge, 대상 job 복구의 증거가 아니다. diff --git a/docs/doctoring/startup-failure-and-strix-concurrency-20260904.md b/docs/doctoring/startup-failure-and-strix-concurrency-20260904.md index 5ba354947a..e233105725 100644 --- a/docs/doctoring/startup-failure-and-strix-concurrency-20260904.md +++ b/docs/doctoring/startup-failure-and-strix-concurrency-20260904.md @@ -72,6 +72,16 @@ burst each new head cancels the previous scan; the burst's final head is scanned, and the weekly full-tree `schedule` scan (unique run id, never cancelled) is the floor under a sustained burst. +**Amendment (2026-09-07).** The workflow group now carries two independent +identities. Pull-request work includes the exact head, while `closed` uses a +run-unique suffix; with cancellation disabled for PR events, Ready, Draft, and +same-head dispatch admission preserve an executing verdict, while a new head +or closed cleanup does not wait behind it. Push work remains grouped by +protected ref and is the only event class with `cancel-in-progress` authority. +The replacement provider waits for live-revalidated cleanup to finish, and +that cleanup enumerates both native and dispatched PR runs. No elapsed-time +condition can cancel provider work. + ## Verification - `python -m pytest -q tests/test_pr_review_merge_scheduler.py -k 'startup_failures or startup_failure'` diff --git a/docs/doctoring/strix-rerun-job-identity-binding.md b/docs/doctoring/strix-rerun-job-identity-binding.md new file mode 100644 index 0000000000..c5760004b9 --- /dev/null +++ b/docs/doctoring/strix-rerun-job-identity-binding.md @@ -0,0 +1,84 @@ +# Strix 재실행 대상 job의 신원 결합 + +## 확인한 결함 + +현재 main `ee5567f7b15f0441a61ec2435415603b9518f1c6`과 #1902의 +`951d0ecd1b5398a9eac293a13bba220a6528df24`에서 Strix 재실행 선택 경로를 비교했다. +관련 core 차이는 이전 OPEN-state 수리뿐이었다. 이번 작업은 951 위에서 진행하며 +main을 merge하거나 다른 세션의 workflow 변경을 덮어쓰지 않았다. + +기존 선택기는 check의 details URL에서 job ID만 추출했다. 직전 live guard는 +PR snapshot이 최신인지 확인했지만, 선택한 job이 그 PR head를 스캔했는지는 +확인하지 않았다. 로컬 mock-only 회귀에서 실제 caller와 rerun wrapper를 실행한 +결과 8 failed / 2 passed였다. 실패 사례는 job/run 조회 없이 mock POST에 도달했다. +실제 GitHub 위조 요청이나 job 재실행을 실행한 결과가 아니다. + +## 최소 수리와 보류 조건 + +기존 selector와 API 조회 helper를 유지하고 Strix rerun 분기에 검증 하나를 추가했다. +GraphQL과 REST 정규화는 selected check의 database ID를 보존한다. + +- selected check URL은 같은 repo의 정확한 run/job을 지정해야 한다. +- 실제 job의 ID, run ID, 이름, 완료 상태와 재실행 가능한 실패 결론을 확인한다. + 현재 허용 결론은 failure, cancelled, timed_out이다. 다른 결론은 자동 재실행을 보류한다. +- job이 가리키는 실제 check ID가 selected check와 같아야 한다. Check publisher는 + github-actions여야 하며 check suite와 run의 연결도 일치해야 한다. +- 실제 run과 workflow 조회는 같은 repo의 `.github/workflows/strix.yml`, + `Strix Security Scan` 이름을 확인한다. +- pull_request_target은 정확히 하나의 PR association, base/head repository, + association의 PR base/head SHA, event에서 생성한 정확한 run-name이 모두 일치해야 한다. + job/run의 top-level head_sha가 base SHA인 정상 사례를 허용한다. 이 필드를 + PR head로 간주하지 않는다. 누락되거나 상충하는 repository 식별자는 거부한다. +- repository_dispatch는 제어 코드의 실행 SHA만으로 target head를 증명할 수 없다. + 이 경로에는 인증된 target receipt를 소비하는 계약이 없으므로, 제목이 맞더라도 + 자동 재실행을 보류한다. push 등 다른 event도 새로 허용하지 않는다. +- 검증 전후 live PR의 base/head SHA를 다시 확인한다. 같은 head가 다른 base로 + retarget된 경우에도 과거 run을 재사용하지 않는다. API 실패나 불완전한 metadata는 + `identity_unverified`로 보류하고 새 dispatch로 우회하지 않는다. 세 상위 caller도 + 이를 실행 완료가 아닌 wait로 보고한다. + +## 검증과 한계 + +`tests/test_strix_job_binding.py`는 실제 REST 정규화, selector, live guard, +dispatch caller, actor 검사, rerun wrapper를 실행한다. 외부 명령은 모두 mock 경계에서 +차단한다. 정상 대조군은 top-level base SHA와 PR head SHA, REST repository URL 형식을 +포함한다. 음성 사례는 stale·상충·누락·다른 repo/workflow/publisher/event·API 실패 및 +검증 중 base/head 이동을 포함한다. 정상 사례는 네 metadata GET과 단일 mock POST를 요구한다. + +기존 state-only, 명령형식, sibling 선택 테스트 세 곳은 각자의 검증 대상을 유지하도록 +새 guard만 국소적으로 대체했다. 신원 결합 자체는 별도 회귀에서 실제 구현을 사용한다. + +권한, 토큰 선택, actor allowlist, queue, concurrency, CodeQL primitive는 변경하지 않았다. +조회와 POST 사이의 원자성, cross-repo callback 권한, hosted 복구는 해결했다고 주장하지 +않는다. 신뢰할 provenance가 없는 역사적 run은 자동 복구가 보류될 수 있다. + +## Draft/Ready 수명주기 증거 보존 + +2026-09-07의 current-head 재검토에서 별도의 실행 전 취소 결함을 확인했다. PR #1706의 +Strix run `34068478185`와 PR #1150의 run `34067942252`는 같은 head에서 실행 중이었지만, +Draft/Ready 상태 전환이 PR 단위 workflow concurrency group에 다시 들어오자 provider +실행 중 취소되었다. replacement run은 queue에만 남았고 terminal Strix verdict와 +publisher evidence는 생성되지 않았다. PR #1999 자체의 run `34067362987`도 `Run Strix +(quick)` 단계에서 취소되고 publisher job이 취소되어 같은 실패 형태를 재현했다. + +Workflow-level `cancel-in-progress`는 `push`에서만 참이고 PR concurrency group은 exact +head까지 포함한다. Ready와 Draft는 같은 head group을 공유하므로 실행 중인 증거를 무효화하지 +않고, 새 head의 `synchronize`는 이전 head group 뒤에 대기하지 않는다. `closed` event는 +고유 run id group을 사용해 종료 대상 scan 뒤에 막히지 않고 metadata-only +`cancel-superseded-pr-runs` job을 실행한다. 이 job은 live PR을 재조회하고 각 mutation 직전 +head와 상태를 다시 검증하므로, `synchronize`의 이전 head와 실제 closed PR만 취소한다. +새 head의 provider job은 이 cleanup 결과가 success 또는 비대상 event의 skipped일 때만 +시작한다. 따라서 old/new provider가 cleanup 전에 겹치지 않는다. Cleanup selector는 native +`pull_request_target`뿐 아니라 같은 repository/PR/head를 run-name으로 증명하는 +`repository_dispatch` 실행도 포함한다. 중앙 dispatch에서는 live PR의 +`TARGET_REPOSITORY`와 Actions run을 소유한 `RUN_REPOSITORY`를 분리해, leaf PR을 +재검증하면서 중앙 `.github` run을 조회·취소한다. Native PR metadata는 두 저장소가 +동일할 때만 신뢰하므로, 같은 번호의 중앙 PR run을 leaf cleanup으로 취소하지 않는다. +같은 protected ref의 새 push만 superseded push scan을 취소한다. Provider 실행에는 +elapsed-time cancellation을 추가하지 않았다. + +회귀 계약은 workflow-level non-cancellation을 직접 파싱하고, 기존 subprocess fixture로 +head가 전진한 뒤에는 취소하지 않음, selection 뒤 재검증 실패 시 mutation하지 않음, +cleanup-before-provider dependency와 stale dispatched run 선택, +검증된 Draft 전환에서는 current scan을 보존하고 closed event는 독립 group에서 cleanup을 +실행함을 함께 증명한다. diff --git a/scripts/ci/pr_review_merge_scheduler_core.py b/scripts/ci/pr_review_merge_scheduler_core.py index 4df4dac3de..970d43923c 100644 --- a/scripts/ci/pr_review_merge_scheduler_core.py +++ b/scripts/ci/pr_review_merge_scheduler_core.py @@ -167,17 +167,30 @@ def review_dispatch_admitted(component: str, repo: str, pr: dict[str, Any]) -> b def live_dispatch_head_matches(repo: str, pr: dict[str, Any]) -> bool: """Re-read the authoritative PR immediately before an Actions side effect.""" live = fetch_pr(validate_github_repository(repo), int(pr["number"])) + expected_head = pr.get("headRefOid") + expected_base = pr.get("baseRefOid") + live_head = live[0].get("headRefOid") if len(live) == 1 else None + live_base = live[0].get("baseRefOid") if len(live) == 1 else None return ( len(live) == 1 - and str(live[0].get("state") or "OPEN").upper() == "OPEN" - and str(live[0].get("headRefOid") or "").lower() - == str(pr.get("headRefOid") or "").lower() + and live[0].get("state") == "OPEN" + and isinstance(expected_head, str) + and isinstance(expected_base, str) + and isinstance(live_head, str) + and isinstance(live_base, str) + and GIT_SHA_RE.fullmatch(expected_head) is not None + and GIT_SHA_RE.fullmatch(expected_base) is not None + and GIT_SHA_RE.fullmatch(live_head) is not None + and GIT_SHA_RE.fullmatch(live_base) is not None + and live_head.lower() == expected_head.lower() + and live_base.lower() == expected_base.lower() ) PULL_REQUEST_FIELDS_FRAGMENT = """\ fragment SchedulerPullRequestFields on PullRequest { number + state title author { login } isDraft @@ -226,6 +239,7 @@ def live_dispatch_head_matches(repo: str, pr: dict[str, Any]) -> bool: nodes { __typename ... on CheckRun { + databaseId name status conclusion @@ -306,7 +320,7 @@ def live_dispatch_head_matches(repo: str, pr: dict[str, Any]) -> bool: nodes { __typename ... on CheckRun { - name status conclusion startedAt detailsUrl + databaseId name status conclusion startedAt detailsUrl checkSuite { createdAt workflowRun { workflow { name } } } } ... on StatusContext { context state } @@ -1276,6 +1290,7 @@ def rest_check_node( workflow = {"name": workflow_name} if workflow_name else {} return { "__typename": "CheckRun", + "databaseId": check.get("id"), "name": check.get("name"), "status": (check.get("status") or "").upper(), "conclusion": (check.get("conclusion") or "").upper() if check.get("conclusion") else None, @@ -1341,6 +1356,7 @@ def rest_pr_node(repo: str, pr: dict[str, Any]) -> dict[str, Any]: ) return { "number": number, + "state": str(pr.get("state") or "").upper(), "title": pr.get("title"), "author": {"login": ((pr.get("user") or {}).get("login"))}, "isDraft": bool(pr.get("draft")), @@ -2906,6 +2922,8 @@ def post_update_branch_followup( if wait_reason: return f"{head_note}; {wait_reason}" dispatch_result = dispatch_strix_evidence(repo, security_workflow, updated_pr, dry_run=dry_run) + if dispatch_result in {"identity_unverified", "stale_head"}: + return f"{head_note}; Strix rerun waits for verified current-target job identity" if dispatch_result == "admission_deferred": return f"{head_note}; bounded admission budget is exhausted" if dispatch_result == "already_running": @@ -3757,6 +3775,86 @@ def is_strix_scan_check_run(node: dict[str, Any]) -> bool: ) +def strix_rerun_identity_verified(repo: str, pr: dict[str, Any], job_id: str) -> bool: + """Bind a selected Strix job to authenticated native PR-target run evidence. + + Dispatch runs require target provenance beyond their control-plane SHA; + without an authenticated target receipt this path deliberately defers them. + A PR-target execution SHA may be the base SHA, so it is never used as the + target PR head. Association and trusted workflow run-name must agree instead. + """ + try: + repo = validate_github_repository(repo) + head = validate_git_sha(pr["headRefOid"]).lower() + base = validate_git_sha(pr["baseRefOid"]).lower() + head_repo = validate_github_repository(pr["headRepository"]["nameWithOwner"]) + if not re.fullmatch(r"[1-9][0-9]*", job_id): + return False + candidates = [node for node in context_nodes(pr) + if is_strix_scan_check_run(node) + and actions_job_id_from_details_url(node.get("detailsUrl")) == job_id] + if len(candidates) != 1: + return False + selected = candidates[0] + url_match = re.fullmatch( + rf"https://github\.com/{re.escape(repo)}/actions/runs/([1-9][0-9]*)/job/{job_id}", + selected.get("detailsUrl") or "", + ) + if not url_match: + return False + run_id = url_match.group(1) + job = gh_api_json(f"repos/{repo}/actions/jobs/{job_id}") + if (job.get("id") != int(job_id) or job.get("run_id") != int(run_id) + or job.get("name") != "strix" or job.get("status") != "completed" + or job.get("conclusion") not in {"failure", "cancelled", "timed_out"}): + return False + check_match = re.fullmatch( + rf"https://api\.github\.com/repos/{re.escape(repo)}/check-runs/([1-9][0-9]*)", + job.get("check_run_url") or "", + ) + if not check_match or selected.get("databaseId") != int(check_match.group(1)): + return False + check = gh_api_json(f"repos/{repo}/check-runs/{check_match.group(1)}") + run_data = gh_api_json(f"repos/{repo}/actions/runs/{run_id}") + if (check.get("id") != selected["databaseId"] or check.get("name") != "strix" + or check.get("app", {}).get("slug") != "github-actions" + or run_data.get("id") != int(run_id) + or run_data.get("repository", {}).get("full_name") != repo + or run_data.get("event") != "pull_request_target" + or run_data.get("status") != "completed" + or run_data.get("name") != "Strix Security Scan" + or run_data.get("path") != ".github/workflows/strix.yml" + or not check.get("check_suite", {}).get("id") + or check["check_suite"]["id"] != run_data.get("check_suite_id")): + return False + workflow_id = run_data.get("workflow_id") + if type(workflow_id) is not int or workflow_id <= 0: + return False + workflow = gh_api_json(f"repos/{repo}/actions/workflows/{workflow_id}") + if (workflow.get("id") != workflow_id + or workflow.get("name") != "Strix Security Scan" + or workflow.get("path") != ".github/workflows/strix.yml"): + return False + associations = run_data.get("pull_requests") or [] + if len(associations) != 1 or associations[0].get("number") != int(pr["number"]): + return False + association = associations[0] + for side, expected_repo in (("base", repo), ("head", head_repo)): + repository = association[side]["repo"] + if (not (repository.get("full_name") or repository.get("url")) + or (repository.get("full_name") is not None and repository["full_name"] != expected_repo) + or (repository.get("url") is not None + and repository["url"] != f"https://api.github.com/repos/{expected_repo}")): + return False + return ( + validate_git_sha(association["head"]["sha"]).lower() == head + and validate_git_sha(association["base"]["sha"]).lower() == base + and run_data.get("display_title") == f"Strix Security Scan {repo}#{pr['number']}@{head}" + ) + except (RuntimeError, ValueError, TypeError, KeyError, AttributeError): + return False + + def dispatch_strix_evidence(repo: str, workflow: str, pr: dict[str, Any], *, dry_run: bool) -> str: """Dispatch same-head Strix workflow evidence before OpenCode reviews.""" job_id = matching_actions_job_id(pr, is_strix_scan_check_run) @@ -3765,6 +3863,10 @@ def dispatch_strix_evidence(repo: str, workflow: str, pr: dict[str, Any], *, dry return "admission_deferred" if not dry_run and not live_dispatch_head_matches(repo, pr): return "stale_head" + if not dry_run and not strix_rerun_identity_verified(repo, pr, job_id): + return "identity_unverified" + if not dry_run and not live_dispatch_head_matches(repo, pr): + return "stale_head" rerun_actions_job(repo, job_id, dry_run=dry_run, action="rerun-strix-evidence") return "rerun" if not dry_run else "dry_run" if dry_run: @@ -4116,6 +4218,8 @@ def dispatch_draft_review_only( f"draft PR review-only dispatch; current head has no completed Strix evidence; {wait_reason}", ) dispatch_result = dispatch_strix_evidence(repo, security_workflow, pr, dry_run=dry_run) + if dispatch_result in {"identity_unverified", "stale_head"}: + return Decision(number, "wait", "Strix rerun waits for verified current-target job identity") if dispatch_result == "admission_deferred": return Decision(number, "wait", "draft PR review-only dispatch; bounded admission budget is exhausted") if dispatch_result == "already_running": @@ -4920,6 +5024,8 @@ def request_branch_update(freshness_reason: str, *, suffix: str = "") -> Decisio if wait_reason: return decide("wait", f"current head has no completed Strix evidence; {wait_reason}") dispatch_result = dispatch_strix_evidence(repo, security_workflow, pr, dry_run=dry_run) + if dispatch_result in {"identity_unverified", "stale_head"}: + return decide("wait", "Strix rerun waits for verified current-target job identity") if dispatch_result == "admission_deferred": return decide("wait", "bounded admission budget is exhausted") if dispatch_result == "already_running": diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index 6ea00c099f..c0aadba3ef 100755 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -200,11 +200,16 @@ assert_strix_workflow_pr_trigger_hardened() { assert_file_contains "$workflow_file" "branches: [main, develop, master]" "strix workflow scans GitHub Flow and Git Flow protected branches" assert_file_contains "$workflow_file" "pull_request_target:" "strix workflow uses trusted PR trigger" assert_file_contains "$workflow_file" "admit-current-head:" "strix workflow admits the live pull request head before provider execution" - assert_file_contains "$workflow_file" "needs: [changed-scope, admit-current-head]" "strix provider queue waits for live-head admission" + assert_file_contains "$workflow_file" "needs: [changed-scope, admit-current-head, cancel-superseded-pr-runs]" "strix provider queue waits for live-head admission and superseded-run cleanup" + assert_file_contains "$workflow_file" "needs.cancel-superseded-pr-runs.result == 'success'" "strix provider accepts successful cleanup before execution" + assert_file_contains "$workflow_file" "needs.cancel-superseded-pr-runs.result == 'skipped'" "strix provider preserves non-cleanup event admission" assert_file_contains "$workflow_file" 'strix-security-scan-${{' "strix workflow coalesces by repository and PR before job admission" assert_file_not_contains "$workflow_file" 'strix-security-scan-${{ needs.admit-current-head.outputs.target_repository }}-${{' "strix concurrency is not delayed until job admission" assert_file_contains "$workflow_file" "format('push-{0}', github.ref_name)" "strix push scans coalesce per protected branch instead of one group per run id" assert_file_contains "$workflow_file" "cancel-superseded-pr-runs:" "strix workflow runs superseded-head cleanup outside the provider scan queue" + assert_file_contains "$workflow_file" '.event == "pull_request_target" or .event == "repository_dispatch"' "strix cleanup includes native and dispatched PR scans" + assert_file_contains "$workflow_file" 'RUN_REPOSITORY: ${{ github.repository }}' "strix cleanup separates the workflow run repository from the target PR repository" + assert_file_contains "$workflow_file" 'repos/${RUN_REPOSITORY}/actions/runs' "strix cleanup queries the repository that owns each workflow run" assert_file_not_contains "$workflow_file" "format('closed-pr-{0}-{1}'" "strix cleanup does not need a second concurrency queue" assert_file_contains "$workflow_file" 'echo "pr_number=${GITHUB_RUN_ID}"' "strix workflow preserves independent push and schedule evidence" assert_file_contains "$workflow_file" "github.event.client_payload.target_repository ||" "strix manual dispatch concurrency scopes to the target repository when provided" @@ -212,7 +217,7 @@ assert_strix_workflow_pr_trigger_hardened() { assert_file_contains "$workflow_file" "github.event.pull_request.number ||" "strix workflow scopes native evidence to the pull request" assert_file_contains "$workflow_file" "github.event.client_payload.pr_number ||" "strix workflow scopes dispatched evidence to the same pull request" assert_file_not_contains "$workflow_file" "format('pr-{0}-{1}'" "strix workflow does not keep stale head-specific concurrency groups" - assert_file_contains "$workflow_file" "cancel-in-progress: true" "strix workflow cancels superseded same-PR scans" + assert_file_contains "$workflow_file" "cancel-in-progress: \${{ github.event_name == 'push' }}" "strix workflow cancels superseded push scans but preserves PR lifecycle evidence" assert_file_not_contains "$workflow_file" "queue: max" "strix workflow uses only supported GitHub concurrency keys" assert_file_not_contains "$workflow_file" "format('{0}-{1}-{2}', github.event_name," "strix workflow unifies pull-request and repository-dispatch evidence for one PR" assert_file_contains "$workflow_file" "Strix event does not match the live pull request head; skipping stale evidence." "strix workflow rejects stale events before provider concurrency" diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index ba47b89c8d..791a6e3138 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -60,6 +60,7 @@ def fake_fine_grained_github_token(body): def make_pr(**overrides): value = { "number": 1, + "state": "OPEN", "title": "Central review", "author": {"login": "pull-request-author"}, "isDraft": False, @@ -4611,6 +4612,8 @@ def fake_run(args, stdin=None): } ) sched.dispatch_opencode_review("owner/repo", "OpenCode Review", required_workflow_pr, dry_run=False) + # Command-shape contract; selected-job binding is exercised separately. + monkeypatch.setattr(sched, "strix_rerun_identity_verified", lambda *_: True) sched.dispatch_strix_evidence("owner/repo", "Strix Security Scan", required_workflow_pr, dry_run=False) assert calls[:2] == [ [ @@ -7884,6 +7887,16 @@ def test_draft_pr_review_only_dispatch_waits_when_strix_already_running(monkeypa assert decision.reason == "draft PR review-only dispatch; same-head Strix evidence is still running" +@pytest.mark.parametrize("result", ["identity_unverified", "stale_head"]) +@pytest.mark.parametrize("draft", [False, True]) +def test_unverified_strix_rerun_is_reported_as_wait(monkeypatch, result, draft): + """A withheld rerun must never be reported as a successful security dispatch.""" + monkeypatch.setattr(sched, "dispatch_strix_evidence", lambda *_, **__: result) + decision = inspect(make_pr(isDraft=draft), allow_draft_review_dispatch=draft) + assert decision.action == "wait" + assert "verified current-target job identity" in decision.reason + + def test_draft_pr_review_only_dispatch_waits_when_repository_is_busy(monkeypatch): monkeypatch.setattr( sched, @@ -8293,6 +8306,11 @@ def followup(updated_pr, **overrides): statusCheckRollup={"contexts": {"nodes": [strix_check(status="IN_PROGRESS", conclusion="")]}}, ) ) + for withheld in ("identity_unverified", "stale_head"): + monkeypatch.setattr(sched, "dispatch_strix_evidence", lambda *_, **__: withheld) + assert "waits for verified current-target job identity" in followup( + make_pr(headRefOid="new-head") + ) assert "same-head OpenCode review is already running" in followup( make_pr( headRefOid="new-head", diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index d3001766d2..7d10496d04 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -130,6 +130,49 @@ def workflow_level_cancels_in_progress(workflow: str) -> bool: ) +def workflow_level_cancel_expression(workflow: str) -> str: + """Return the workflow-level cancellation scalar, excluding comments.""" + block_match = WORKFLOW_LEVEL_CONCURRENCY_BLOCK.search(workflow) + if block_match is None: + raise AssertionError("workflow declares no workflow-level concurrency block") + match = re.search( + r"(?m)^\s+cancel-in-progress:\s*(\S.*?)\s*$", block_match.group("body") + ) + if match is None: + raise AssertionError("workflow declares no workflow-level cancellation value") + return _strip_yaml_inline_comment(match.group(1)).strip() + + +def strix_concurrency_key( + *, + event_name: str, + run_id: str, + repository: str = "owner/repo", + pr_number: str = "", + head_sha: str = "", + action: str = "", + ref_name: str = "", +) -> str: + """Model the source-pinned Strix workflow concurrency identity.""" + group = workflow_level_concurrency_group(workflow_text("strix.yml")) + assert "github.event.pull_request.number" in group + assert "github.event.client_payload.pr_number" in group + assert "format('push-{0}', github.ref_name)" in group + assert "github.event.action == 'closed' && github.run_id" in group + assert "github.event_name == 'push' && 'protected-ref'" in group + assert "github.event.pull_request.head.sha" in group + assert "github.event.client_payload.pr_head_sha" in group + subject = pr_number or (f"push-{ref_name}" if event_name == "push" else run_id) + revision = ( + run_id + if action == "closed" + else "protected-ref" + if event_name == "push" + else head_sha or run_id + ) + return f"strix-security-scan-{repository}-{subject}-{revision}" + + def workflow_step(workflow: str, name: str) -> str: """Extract one named workflow step without parsing YAML dynamically.""" step = f" - name: {name}\n" @@ -787,10 +830,11 @@ def test_strix_serializes_provider_evidence_per_repository_and_pr() -> None: Restored to PR-scoped on explicit owner authorization (2026-09-03) after confirming NVIDIA_NIM_API_KEY and NVIDIA_NIM_API_KEY_SUB have independent - rate limits rather than a shared pool. The workflow-level group now retires - superseded runs before runner admission, including runs still blocked by - the organization-wide job ceiling. Native and dispatched evidence share - one group; non-PR events use a unique run id. + rate limits rather than a shared pool. Native and dispatched evidence share + one exact-head group; non-PR events and closed cleanup use a unique run id. + The group serializes work but does not cancel an executing same-head scan + when Draft/Ready admission is repeated. A separate live-revalidated cleanup + job retires verified superseded heads and closed pull requests. 2026-09-05: push events are scoped per protected branch (``push-``) instead of a unique run id. Measured that morning in this repository: @@ -813,7 +857,13 @@ def test_strix_serializes_provider_evidence_per_repository_and_pr() -> None: group_value = workflow_level_concurrency_group(workflow) assert re.search(r"(?m)^concurrency:", workflow) - assert "needs: [changed-scope, admit-current-head]" in strix_job + assert ( + "needs: [changed-scope, admit-current-head, cancel-superseded-pr-runs]" + in strix_job + ) + assert "always() && !cancelled()" in strix_job + assert "needs.cancel-superseded-pr-runs.result == 'success'" in strix_job + assert "needs.cancel-superseded-pr-runs.result == 'skipped'" in strix_job assert "needs.admit-current-head.outputs.admitted == 'true'" in strix_job assert "strix-security-scan-${{" in group_value assert "github.event.pull_request.base.repo.full_name" in group_value @@ -821,6 +871,9 @@ def test_strix_serializes_provider_evidence_per_repository_and_pr() -> None: assert "github.event.pull_request.number" in group_value assert "github.event.client_payload.pr_number" in group_value assert "github.run_id" in group_value + assert "github.event.pull_request.head.sha" in concurrency_contract + assert "github.event.client_payload.pr_head_sha" in concurrency_contract + assert "github.event.action == 'closed'" in concurrency_contract # Asserted against group_value, not the whole concurrency block: #1970 made # these keys immune to comment leakage, and this file's own prose now # discusses the push clause at length, so the comment text would otherwise @@ -829,9 +882,9 @@ def test_strix_serializes_provider_evidence_per_repository_and_pr() -> None: "(github.event_name == 'push' && format('push-{0}', github.ref_name)) ||" in group_value ) - assert "github.event.pull_request.head.sha" not in concurrency_contract - assert "github.event.client_payload.pr_head_sha" not in concurrency_contract - assert workflow_level_cancels_in_progress(workflow) + assert "github.event_name == 'push' && 'protected-ref'" in group_value + assert not workflow_level_cancels_in_progress(workflow) + assert workflow_level_cancel_expression(workflow) == "${{ github.event_name == 'push' }}" assert " concurrency:" not in strix_job.split(" permissions:", 1)[0] assert "queue: max" not in workflow assert workflow.index("admit-current-head:") < workflow.index("\n strix:\n") @@ -839,6 +892,12 @@ def test_strix_serializes_provider_evidence_per_repository_and_pr() -> None: " strix:", 1 )[0] assert "github.event.action == 'synchronize'" in cleanup_job + assert "github.event_name == 'repository_dispatch'" in cleanup_job + assert "github.event.client_payload.target_repository" in cleanup_job + assert "github.event.client_payload.pr_number" in cleanup_job + assert "RUN_REPOSITORY: ${{ github.repository }}" in cleanup_job + assert 'repos/${RUN_REPOSITORY}/actions/runs' in cleanup_job + assert 'repos/${RUN_REPOSITORY}/actions/runs/${run_id}/cancel' in cleanup_job assert 'endswith("@" + $head_sha)' in cleanup_job assert "/force-cancel" in cleanup_job assert 'gh api "repos/${TARGET_REPOSITORY}/pulls/${TARGET_PR_NUMBER}"' in cleanup_job @@ -846,10 +905,10 @@ def test_strix_serializes_provider_evidence_per_repository_and_pr() -> None: assert "target changed before run selection" in cleanup_job assert "target changed before cancellation" in cleanup_job assert cleanup_job.index("if ! live_target_matches") < cleanup_job.index( - 'runs_url="repos/${TARGET_REPOSITORY}/actions/runs?status=${status}&per_page=100"' + 'runs_url="repos/${RUN_REPOSITORY}/actions/runs?status=${status}&per_page=100"' ) assert cleanup_job.rindex("if ! live_target_matches") < cleanup_job.index( - 'gh api --method POST "repos/${TARGET_REPOSITORY}/actions/runs/${run_id}/cancel"' + 'gh api --method POST "repos/${RUN_REPOSITORY}/actions/runs/${run_id}/cancel"' ) assert "actions: write" in cleanup_job assert "pull-requests: read" in cleanup_job @@ -860,6 +919,59 @@ def test_strix_serializes_provider_evidence_per_repository_and_pr() -> None: ) +def test_strix_concurrency_identity_is_event_lifecycle_sensitive() -> None: + """Preserve same-head evidence while admitting replacement and cleanup runs.""" + opened = strix_concurrency_key( + event_name="pull_request_target", run_id="1", pr_number="7", head_sha="abc" + ) + ready = strix_concurrency_key( + event_name="pull_request_target", + action="ready_for_review", + run_id="2", + pr_number="7", + head_sha="abc", + ) + draft = strix_concurrency_key( + event_name="pull_request_target", + action="converted_to_draft", + run_id="3", + pr_number="7", + head_sha="abc", + ) + dispatched = strix_concurrency_key( + event_name="repository_dispatch", run_id="4", pr_number="7", head_sha="abc" + ) + synchronized = strix_concurrency_key( + event_name="pull_request_target", + action="synchronize", + run_id="5", + pr_number="7", + head_sha="def", + ) + closed = strix_concurrency_key( + event_name="pull_request_target", + action="closed", + run_id="6", + pr_number="7", + head_sha="abc", + ) + + assert opened == ready == draft == dispatched + assert synchronized != opened + assert closed != opened + assert strix_concurrency_key(event_name="push", run_id="7", ref_name="main") == strix_concurrency_key( + event_name="push", run_id="8", ref_name="main" + ) + assert strix_concurrency_key(event_name="push", run_id="9", ref_name="develop") != strix_concurrency_key( + event_name="push", run_id="8", ref_name="main" + ) + assert strix_concurrency_key(event_name="schedule", run_id="10") != strix_concurrency_key( + event_name="schedule", run_id="11" + ) + cancel_expression = workflow_level_cancel_expression(workflow_text("strix.yml")) + assert cancel_expression == "${{ github.event_name == 'push' }}" + + def test_strix_install_normalizes_executable_permissions_before_hashing() -> None: """Normalize the Strix executable before its trusted hash is computed.""" workflow = workflow_text("strix.yml") @@ -877,13 +989,17 @@ def test_strix_install_normalizes_executable_permissions_before_hashing() -> Non ) -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.""" +def test_strix_cleanup_selects_native_and_dispatched_stale_pr_runs() -> None: + """Cleanup selects stale native metadata and dispatched run-name evidence.""" 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' + marker = ( + '--arg action "$PR_ACTION" --arg repo "$TARGET_REPOSITORY" ' + '--arg run_repo "$RUN_REPOSITORY" \\\n' + ' --arg current "$CURRENT_RUN_ID" \'\n' + ) start = workflow.index(marker) + len(marker) end = workflow.index('\n \' <<<"$runs_json"', start) runs = { @@ -893,20 +1009,28 @@ def test_strix_cleanup_uses_pr_metadata_when_custom_title_is_absent() -> None: {"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": 6, "name": "Strix Security Scan", "event": "repository_dispatch", "display_title": "Strix Security Scan owner/repo#7@old", "pull_requests": []}, + {"id": 7, "name": "Strix Security Scan", "event": "repository_dispatch", "display_title": "Strix Security Scan owner/repo#7@current", "pull_requests": []}, + {"id": 8, "name": "Strix Security Scan", "event": "repository_dispatch", "display_title": "Strix Security Scan owner/repo#8@old", "pull_requests": []}, ] } 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]], + [jq, "-r", "--arg", "pr", "7", "--arg", "head_sha", "current", "--arg", "action", "synchronize", "--arg", "repo", "owner/repo", "--arg", "run_repo", "owner/repo", "--arg", "current", "99", workflow[start:end]], input=json.dumps(runs), text=True, capture_output=True, check=True, ) - assert result.stdout.splitlines() == ["1"] + assert result.stdout.splitlines() == ["1", "6"] 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", + run_repository: str = "owner/repo", + run: dict[str, object] | None = None, ) -> str: """Execute the production cleanup step against a stateful fake ``gh``.""" jq = shutil.which("jq") @@ -922,10 +1046,27 @@ def _run_strix_cleanup( fake_bin.mkdir() calls = tmp_path / "calls" pulls = tmp_path / "pulls" + runs = tmp_path / "runs" pulls.write_text( "\n".join(json.dumps(state) for state in pull_states) + "\n", encoding="utf-8", ) + runs.write_text( + json.dumps( + { + "workflow_runs": [ + run + or { + "id": 100, + "name": "Strix Security Scan", + "event": "pull_request_target", + "pull_requests": [{"number": 7, "head": {"sha": "old"}}], + } + ] + } + ), + encoding="utf-8", + ) fake_gh = fake_bin / "gh" fake_gh.write_text( """#!/usr/bin/env bash @@ -941,7 +1082,7 @@ 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"}}]}]}' + cat "$FAKE_RUNS" exit 0 fi if [[ "$*" == *"actions/runs?status="* ]]; then @@ -958,6 +1099,8 @@ def _run_strix_cleanup( "PATH": f"{fake_bin}{os.pathsep}{os.environ['PATH']}", "FAKE_CALLS": str(calls), "FAKE_PULLS": str(pulls), + "FAKE_RUNS": str(runs), + "RUN_REPOSITORY": run_repository, "TARGET_REPOSITORY": "owner/repo", "TARGET_PR_NUMBER": "7", "TARGET_PR_HEAD_SHA": "current", @@ -968,6 +1111,52 @@ def _run_strix_cleanup( return calls.read_text(encoding="utf-8") +def test_strix_dispatch_cleanup_targets_central_execution_repository( + tmp_path: Path, +) -> None: + """A dispatched stale scan is selected and cancelled in its run repository.""" + calls = _run_strix_cleanup( + tmp_path, + [{"state": "open", "draft": False, "head": {"sha": "current"}}] * 6, + run_repository="ContextualWisdomLab/.github", + run={ + "id": 100, + "name": "Strix Security Scan", + "event": "repository_dispatch", + "display_title": "Strix Security Scan owner/repo#7@old", + "pull_requests": [], + }, + ) + + assert "repos/owner/repo/pulls/7" in calls + assert "repos/ContextualWisdomLab/.github/actions/runs?status=queued" in calls + assert "repos/ContextualWisdomLab/.github/actions/runs/100/cancel" in calls + + +def test_strix_dispatch_cleanup_ignores_same_number_native_pr_run( + tmp_path: Path, +) -> None: + """Leaf cleanup cannot cancel a central native PR with the same number.""" + calls = _run_strix_cleanup( + tmp_path, + [{"state": "open", "draft": False, "head": {"sha": "current"}}] * 6, + run_repository="ContextualWisdomLab/.github", + run={ + "id": 100, + "name": "Strix Security Scan", + "event": "pull_request_target", + "display_title": ( + "Strix Security Scan ContextualWisdomLab/.github#7@central-head" + ), + "pull_requests": [ + {"number": 7, "head": {"sha": "central-head"}} + ], + }, + ) + + assert "repos/ContextualWisdomLab/.github/actions/runs/100/cancel" not in calls + + def test_old_strix_cleanup_never_lists_or_cancels_after_live_head_advanced( tmp_path: Path, ) -> None: @@ -999,15 +1188,17 @@ 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_draft_transition_preserves_current_scan(tmp_path: Path) -> None: + """A same-head Draft transition preserves the executing Strix evidence.""" calls = _run_strix_cleanup( tmp_path, [{"state": "open", "draft": True, "head": {"sha": "current"}}] * 6, action="converted_to_draft", ) - assert "/actions/runs/100/cancel" in calls + assert "actions/runs?status=" not in calls + assert "/actions/runs/100/cancel" not in calls + assert "/actions/runs/100/force-cancel" not in calls def test_pr_keyed_scan_workflows_pin_cancellation_as_a_value() -> None: @@ -1061,8 +1252,10 @@ 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 'select(.event == "repository_dispatch")' not in workflow + assert ( + 'select(.event == "pull_request_target" or ' + '.event == "repository_dispatch")' in workflow + ) assert "(.pull_requests // [])" in workflow assert ".head.sha // \"\"" in workflow assert "leaving runs unchanged" in workflow @@ -1073,6 +1266,7 @@ def test_pull_request_close_events_cancel_superseded_runs_without_heavy_jobs() - cleanup_job = workflow.split(" cancel-superseded-pr-runs:", 1)[1].split( " strix:", 1 )[0] + assert "converted_to_draft" not in cleanup_job elif filename == "noema-review.yml": assert "cancel-closed-pr-runs:" in workflow assert "Cancel queued and running Noema reviews for the inactive pull request" in workflow @@ -1113,11 +1307,18 @@ def test_pull_request_close_events_cancel_superseded_runs_without_heavy_jobs() - assert "${{ secrets." not in opencode_bootstrap strix_workflow = workflow_text("strix.yml") - # Strix admits the live head before same-PR cancellation while cleanup stays - # outside that queue so synchronize and close events can retire old work. + # Draft/Ready events for one unchanged head are review admission, not + # evidence invalidation. Only protected-branch pushes cancel in progress. assert "admit-current-head:" in strix_workflow assert "skipping stale evidence" in strix_workflow - assert workflow_level_cancels_in_progress(strix_workflow) + assert workflow_level_cancel_expression(strix_workflow) == ( + "${{ github.event_name == 'push' }}" + ) + group_value = workflow_level_concurrency_group(strix_workflow) + assert "github.event.pull_request.head.sha" in group_value + assert "github.event.client_payload.pr_head_sha" in group_value + assert "github.event.action == 'closed'" in group_value + assert "github.run_id" in group_value def test_merge_scheduler_owns_empty_pr_cleanup_without_checkout() -> None: diff --git a/tests/test_scheduler_live_dispatch_guard.py b/tests/test_scheduler_live_dispatch_guard.py new file mode 100644 index 0000000000..e3db9ddf13 --- /dev/null +++ b/tests/test_scheduler_live_dispatch_guard.py @@ -0,0 +1,113 @@ +"""Fail-closed live PR evidence before review dispatch or exact Strix rerun.""" + +import pytest + +from scripts.ci import pr_review_merge_scheduler as sched + + +HEAD = "a" * 40 + + +def candidate(): + """Return explicit open PR metadata with canonical refs and SHAs.""" + return { + "number": 7, "state": "OPEN", "headRefOid": HEAD, + "baseRefOid": "b" * 40, "baseRefName": "main", "headRefName": "feature", + } + + +@pytest.mark.parametrize("caller", ["opencode", "strix-dispatch", "strix-rerun"]) +@pytest.mark.parametrize("state", [None, "", "CLOSED", "MERGED", "UNKNOWN", "OPEN"]) +def test_live_state_gates_all_three_side_effects(monkeypatch, caller, state): + """Only an explicitly open live PR may reach any guarded side effect.""" + pr = candidate() + live = candidate() + if state is None: + live.pop("state") + else: + live["state"] = state + effects = [] + monkeypatch.setattr(sched, "fetch_pr", lambda *_: [live]) + monkeypatch.setattr(sched, "require_github_actions_control_actor", lambda *_: None) + monkeypatch.setattr(sched, "review_dispatch_admitted", lambda *_: True) + # This suite isolates live PR state; job provenance has its own real-caller suite. + monkeypatch.setattr(sched, "strix_rerun_identity_verified", lambda *_: True) + monkeypatch.setattr(sched, "active_opencode_run_refs", lambda *_: ([], [])) + monkeypatch.setattr(sched, "active_review_run_refs", lambda *_, **__: ([], [])) + monkeypatch.setattr(sched, "_cancel_revalidated_review_run_refs", lambda *_: ([], [])) + monkeypatch.setattr(sched, "active_workflow_runs", lambda *_: []) + monkeypatch.setattr(sched, "complete_paginated_pr_contexts", lambda *_: None) + monkeypatch.setattr(sched, "matching_actions_run_id", lambda *_: None) + monkeypatch.setattr(sched, "discover_opencode_required_run_id", lambda *_: None) + monkeypatch.setattr(sched, "matching_actions_job_id", lambda *_: "202" if caller == "strix-rerun" else None) + monkeypatch.setattr(sched, "repository_dispatch_target", lambda _: "ContextualWisdomLab/.github") + monkeypatch.setattr(sched, "run_github_dispatch", lambda *_, **__: effects.append("dispatch")) + monkeypatch.setattr(sched, "rerun_actions_job", lambda *_, **__: effects.append("rerun")) + dispatch = sched.dispatch_opencode_review if caller == "opencode" else sched.dispatch_strix_evidence + result = dispatch("owner/repo", "review", pr, dry_run=False) + if state == "OPEN": + assert result == ("rerun" if caller == "strix-rerun" else "dispatched") + assert effects == ["rerun" if caller == "strix-rerun" else "dispatch"] + else: + assert result == "stale_head" + assert effects == [] + + +@pytest.mark.parametrize("expected,observed,accepted", [ + (HEAD, HEAD, True), (HEAD.upper(), HEAD, True), + (HEAD, "b" * 40, False), ("", "", False), (None, None, False), + ("bad", "bad", False), ("g" * 40, "g" * 40, False), + ("a" * 39, "a" * 39, False), ("a" * 41, "a" * 41, False), + (123, 123, False), (HEAD, None, False), (None, HEAD, False), + (HEAD, "g" * 40, False), (HEAD, "", False), +]) +def test_live_guard_requires_two_canonical_matching_heads(monkeypatch, expected, observed, accepted): + """Equal empty, malformed, or non-string heads are not identity evidence.""" + pr = candidate() + pr["headRefOid"] = expected + monkeypatch.setattr( + sched, + "fetch_pr", + lambda *_: [{**candidate(), "headRefOid": observed}], + ) + assert sched.live_dispatch_head_matches("owner/repo", pr) is accepted + + +@pytest.mark.parametrize("rows", [[], [candidate(), candidate()]]) +def test_live_guard_rejects_missing_or_ambiguous_pr(monkeypatch, rows): + """The live lookup must identify exactly one PR.""" + monkeypatch.setattr(sched, "fetch_pr", lambda *_: rows) + assert not sched.live_dispatch_head_matches("owner/repo", candidate()) + + +def test_exact_graphql_fetch_requests_pr_state(monkeypatch): + """The actual single-PR query must request state in its shared fragment.""" + def graphql(query, **fields): + fragment = query.split("fragment SchedulerPullRequestFields on PullRequest {", 1)[1] + assert " state" in fragment.split(" author", 1)[0].splitlines() + assert fields == {"owner": "owner", "name": "repo", "number": 7} + return {"data": {"repository": {"pullRequest": {**candidate(), "state": "CLOSED"}}}} + + monkeypatch.setattr(sched, "gh_graphql", graphql) + monkeypatch.setattr(sched, "complete_all_pr_reviews", lambda *_: None) + monkeypatch.setattr(sched, "enrich_rest_mergeable_states", lambda *_: None) + assert sched.fetch_pr("owner/repo", 7)[0]["state"] == "CLOSED" + assert not sched.live_dispatch_head_matches("owner/repo", candidate()) + + +@pytest.mark.parametrize("state,normalized", [("open", "OPEN"), ("closed", "CLOSED"), (None, "")]) +def test_rest_normalization_preserves_state_without_open_default(monkeypatch, state, normalized): + """REST fallback must retain closure and must not manufacture open state.""" + pr = {"number": 7, "head": {"sha": HEAD}} + if state is not None: + pr["state"] = state + responses = { + "repos/owner/repo/pulls/7": pr, + f"repos/owner/repo/commits/{HEAD}/check-runs?per_page=100": {}, + f"repos/owner/repo/commits/{HEAD}/check-suites?per_page=100": {}, + f"repos/owner/repo/commits/{HEAD}/status": {}, + "repos/owner/repo/pulls/7/files?per_page=20": [], + } + monkeypatch.setattr(sched, "gh_api_json", lambda endpoint: responses[endpoint]) + monkeypatch.setattr(sched, "fetch_all_pr_reviews_rest", lambda *_: []) + assert sched.fetch_pr_rest("owner/repo", 7)[0]["state"] == normalized diff --git a/tests/test_strix_job_binding.py b/tests/test_strix_job_binding.py new file mode 100644 index 0000000000..2c96508c7c --- /dev/null +++ b/tests/test_strix_job_binding.py @@ -0,0 +1,171 @@ +"""Mock-only Strix rerun identity contracts through the real scheduler caller.""" + +import json + +import pytest + +from scripts.ci import pr_review_merge_scheduler as sched + + +@pytest.mark.parametrize("case,allowed", [ + ("current-associated-head-top-level-base", True), + ("current-associated-head-top-level-head", True), + ("retargeted-base-association", False), + ("stale-associated-head", False), + ("stale-association-top-level-current", False), + ("contradictory-title", False), + ("missing-association", False), + ("foreign-details-repository", False), + ("different-workflow-path", False), + ("untrusted-check-publisher", False), + ("unrelated-push-event", False), + ("dispatch-without-target-receipt", False), + ("api-unavailable", False), + ("wrong-job-run", False), + ("wrong-check-suite", False), + ("contradictory-repository", False), + ("missing-check-id", False), + ("current-head-moves-during-binding", False), + ("current-base-moves-during-binding", False), + ("repository-api-url-only", True), + ("zero-job-id", False), + ("ambiguous-job-candidates", False), + ("invalid-workflow-id", False), + ("mismatched-workflow-metadata", False), +]) +def test_actual_strix_rerun_caller_binds_selected_job(monkeypatch, case, allowed): + """Selected job/run provenance, not a current PR snapshot alone, authorizes rerun.""" + repo = "owner/repo" + current_head, stale_head, base_sha = "b" * 40, "a" * 40, "c" * 40 + job_id, run_id, suite_id, workflow_id = 202, 101, 303, 404 + if case == "zero-job-id": + job_id = 0 + associated_head = stale_head if case.startswith("stale-") else current_head + title_head = stale_head if case in {"stale-associated-head", "contradictory-title"} else current_head + execution_sha = current_head if case in { + "current-associated-head-top-level-head", "stale-association-top-level-current" + } else base_sha + details_repo = "other/repo" if case == "foreign-details-repository" else repo + publisher = "third-party-app" if case == "untrusted-check-publisher" else "github-actions" + actual_check_id = 606 if publisher != "github-actions" else 505 + check = { + "id": 505, "name": "strix", "status": "completed", "conclusion": "failure", + "head_sha": current_head, + "details_url": f"https://github.com/{details_repo}/actions/runs/{run_id}/job/{job_id}", + "app": {"slug": publisher}, "check_suite": {"id": suite_id}, + } + node = sched.rest_check_node( + check, {}, {} if publisher != "github-actions" else {suite_id: "Strix Security Scan"} + ) + if case == "missing-check-id": + node.pop("databaseId") + pr = { + "number": 7, "state": "OPEN", "headRefOid": current_head, + "baseRefOid": base_sha, "headRefName": "feature", "baseRefName": "main", + "headRepository": {"nameWithOwner": repo}, + "statusCheckRollup": {"contexts": {"nodes": [node]}}, + } + if case == "ambiguous-job-candidates": + pr["statusCheckRollup"]["contexts"]["nodes"].append({**node, "databaseId": 506}) + job = { + "id": job_id, "run_id": run_id, "head_sha": execution_sha, + "name": "strix", "status": "completed", "conclusion": "failure", + "check_run_url": f"https://api.github.com/repos/{repo}/check-runs/{actual_check_id}", + "html_url": f"https://github.com/{repo}/actions/runs/{run_id}/job/{job_id}", + } + run = { + "id": run_id, "head_sha": execution_sha, "workflow_id": workflow_id, + "check_suite_id": suite_id, "repository": {"full_name": repo}, + "event": "push" if case == "unrelated-push-event" else "pull_request_target", + "path": ".github/workflows/other.yml" if case == "different-workflow-path" else ".github/workflows/strix.yml", + "name": "Strix Security Scan", "status": "completed", "conclusion": "failure", + "display_title": f"Strix Security Scan {repo}#7@{title_head}", + "pull_requests": [] if case == "missing-association" else [{ + "number": 7, "base": {"sha": base_sha, "repo": {"name": "repo", "full_name": repo}}, + "head": {"sha": associated_head, "repo": {"name": "repo", "full_name": repo}}, + }], + } + if case == "dispatch-without-target-receipt": + run["event"] = "repository_dispatch" + run["pull_requests"] = [] + if case == "wrong-job-run": + job["run_id"] = 999 + if case == "wrong-check-suite": + run["check_suite_id"] = 999 + if case == "invalid-workflow-id": + run["workflow_id"] = 0 + if case == "contradictory-repository": + run["pull_requests"][0]["head"]["repo"]["url"] = "https://api.github.com/repos/other/repo" + if case == "retargeted-base-association": + run["pull_requests"][0]["base"]["sha"] = "d" * 40 + if case == "repository-api-url-only": + for side in ("base", "head"): + run["pull_requests"][0][side]["repo"] = {"name": "repo", "url": f"https://api.github.com/repos/{repo}"} + reads, posts = [], [] + responses = { + f"repos/{repo}/actions/jobs/{job_id}": job, + f"repos/{repo}/actions/runs/{run_id}": run, + f"repos/{repo}/check-runs/505": check, + f"repos/{repo}/actions/workflows/{workflow_id}": { + "id": workflow_id, "path": run["path"], "name": "Strix Security Scan", + }, + } + if case == "mismatched-workflow-metadata": + responses[f"repos/{repo}/actions/workflows/{workflow_id}"]["path"] = ".github/workflows/other.yml" + if actual_check_id != 505: + responses[f"repos/{repo}/check-runs/{actual_check_id}"] = { + **check, "id": actual_check_id, "app": {"slug": "github-actions"}, + } + + def read(endpoint): + reads.append(endpoint) + if case == "api-unavailable": + raise RuntimeError("metadata unavailable") + assert endpoint in responses, f"Unexpected metadata lookup: {endpoint}" + return responses[endpoint] + + def actions(args, *, stdin=None): + assert stdin is None + if args == ["gh", "api", "-X", "POST", f"repos/{repo}/actions/jobs/{job_id}/rerun"]: + posts.append(args) + return "" + assert args[:2] == ["gh", "api"] and len(args) == 3 + return json.dumps(read(args[2])) + + def no_external_call(*args, **kwargs): + pytest.fail("Unexpected unmocked command boundary") + + monkeypatch.setenv("GITHUB_ACTIONS", "true") + monkeypatch.setenv("SCHEDULER_ACTIONS_TOKEN", "fixture-token") + monkeypatch.setattr(sched, "run", no_external_call) + monkeypatch.setattr(sched, "run_with_env", no_external_call) + monkeypatch.setattr(sched, "gh_api_json", read) + monkeypatch.setattr(sched, "run_github_read", actions) + monkeypatch.setattr(sched, "run_github_actions", actions) + monkeypatch.setattr(sched, "review_dispatch_admitted", lambda *_: True) + def fetch_pr(*_): + if case == "current-head-moves-during-binding" and reads: + return [{**pr, "headRefOid": "d" * 40}] + if case == "current-base-moves-during-binding" and reads: + return [{**pr, "baseRefOid": "d" * 40}] + return [pr] + + monkeypatch.setattr(sched, "fetch_pr", fetch_pr) + # Keep real selection, live guard, caller, control-actor check and rerun wrapper. + result = sched.dispatch_strix_evidence(repo, "Strix Security Scan", pr, dry_run=False) + if allowed: + assert result == "rerun" + assert len(posts) == 1 + assert len(reads) == 4 + else: + assert posts == [], f"Unsafe rerun reached POST without binding metadata; reads={reads}; run={run}" + assert result in {"identity_unverified", "stale_head"} + expected_reads = { + "zero-job-id": 0, + "ambiguous-job-candidates": 0, + "invalid-workflow-id": 3, + "mismatched-workflow-metadata": 4, + } + if case in expected_reads: + assert result == "identity_unverified" + assert len(reads) == expected_reads[case] diff --git a/tests/test_strix_rerun_job_selection.py b/tests/test_strix_rerun_job_selection.py index c1926b2ce3..84ca5f9852 100644 --- a/tests/test_strix_rerun_job_selection.py +++ b/tests/test_strix_rerun_job_selection.py @@ -23,6 +23,9 @@ def test_dispatch_strix_reruns_scan_job_not_sibling_publisher(monkeypatch) -> No """A skipped status-publisher sibling must never be selected as the Strix rerun target.""" pr = { "number": 1055, + "state": "OPEN", + "headRefOid": "a" * 40, + "baseRefOid": "b" * 40, "statusCheckRollup": { "contexts": { "nodes": [ @@ -39,6 +42,8 @@ def record_rerun(repo: str, job_id: str, *, dry_run: bool, action: str) -> None: monkeypatch.setattr(sched, "rerun_actions_job", record_rerun) monkeypatch.setattr(sched, "fetch_pr", lambda *_args: [pr]) + # Keep this test focused on sibling selection, not the independent API binding. + monkeypatch.setattr(sched, "strix_rerun_identity_verified", lambda *_args: True) assert ( sched.dispatch_strix_evidence(