diff --git a/.github/workflows/codeql-scan-dispatch.yml b/.github/workflows/codeql-scan-dispatch.yml index 04656e5b34..2034456107 100644 --- a/.github/workflows/codeql-scan-dispatch.yml +++ b/.github/workflows/codeql-scan-dispatch.yml @@ -65,6 +65,7 @@ jobs: rerun_schema: ${{ steps.validate.outputs.rerun_schema }} producer_source_sha: ${{ steps.validate.outputs.producer_source_sha }} dispatch_protocol: ${{ steps.validate.outputs.dispatch_protocol }} + stale: ${{ steps.validate.outputs.stale }} steps: - name: Exchange OpenCode app token for target repository metadata reads id: metadata_read_app_token @@ -367,7 +368,10 @@ jobs: live_merge_commit_sha="$(jq -r '.merge_commit_sha // empty' <<<"$pull_request_json")" live_state="$(jq -r '.state // empty' <<<"$pull_request_json")" - if [ "$live_state" != "open" ] || + # Validate the live identity that must remain stable before deciding + # whether a superseded head can be retired. Head SHA is intentionally + # checked later because a proven descendant is the stale condition. + if { [ "$live_state" != "open" ] && [ "$live_state" != "closed" ]; } || [ "$live_base_repository" != "$TARGET_REPOSITORY" ] || [ "$live_head_repository" != "$TARGET_REPOSITORY" ] || ! [[ "$live_base_sha" =~ ^[0-9a-fA-F]{40}$ ]] || @@ -378,6 +382,78 @@ jobs: exit 1 fi + stable_identity_mismatches=() + [ "$SUPPLIED_BASE_REF" = "$live_base_ref" ] || stable_identity_mismatches+=("base_ref") + [ "$SUPPLIED_BASE_SHA" = "$live_base_sha" ] || stable_identity_mismatches+=("base_sha") + [ "$SUPPLIED_HEAD_REF" = "$live_head_ref" ] || stable_identity_mismatches+=("head_ref") + if [ "${#stable_identity_mismatches[@]}" -gt 0 ]; then + printf '::error::repository_dispatch metadata does not match the live pull request: %s. supplied_base=%s/%s live_base=%s/%s supplied_head=%s/%s live_head=%s/%s\n' "$(IFS=,; printf '%s' "${stable_identity_mismatches[*]}")" "${SUPPLIED_BASE_REF:-}" "${SUPPLIED_BASE_SHA:-}" "$live_base_ref" "$live_base_sha" "${SUPPLIED_HEAD_REF:-}" "${SUPPLIED_HEAD_SHA:-}" "$live_head_ref" "$live_head_sha" + exit 1 + fi + + # Stale-dispatch retirement. Under runner-queue saturation this run can + # start hours after the dispatched head was superseded or the pull + # request closed; the newer dispatch is itself still queued, so the + # workflow-level cancel-in-progress group cannot retire this run first. + # Only a well-formed live answer proving the dispatched head is no + # longer current ends the run as a notice. A failed `gh api` lookup has + # already exited non-zero above, and every other disagreement below + # (base, head ref, fork, malformed metadata) stays fail-closed. + # No EVENT_NAME guard is needed here, unlike opencode-review-dispatch.yml: + # this workflow is triggered only by repository_dispatch and the + # actor/sender authorization above is unconditional, so every run that + # reaches this point is an authorized dispatch. The OpenCode guard exists + # because its authorization block is itself nested under that event check. + stale_reason="" + if [ "$live_state" = "closed" ]; then + stale_reason="pull request is closed" + elif [ "$live_state" = "open" ] && + [[ "$live_head_sha" =~ ^[0-9a-fA-F]{40}$ ]] && + [[ "$SUPPLIED_HEAD_SHA" =~ ^[0-9a-fA-F]{40}$ ]] && + [ "${SUPPLIED_HEAD_SHA,,}" != "${live_head_sha,,}" ]; then + # A mismatch alone is not proof: right after a push the API can + # briefly serve the previous head, which would retire a brand-new + # dispatch. Retire only when the compare API proves the dispatched + # head is a strict ancestor of the live head (status "ahead", + # behind_by 0), i.e. the live head is newer and gets its own + # dispatch. Behind, diverged (force-push), or a failed/unexpected + # compare falls through to the fail-closed head_sha mismatch below. + head_compare_status="" + if head_compare_json="$(gh api "repos/${TARGET_REPOSITORY}/compare/${SUPPLIED_HEAD_SHA}...${live_head_sha}" 2>/dev/null)"; then + head_compare_status="$(jq -r ' + if .status == "ahead" and .behind_by == 0 and ((.ahead_by | type) == "number") and .ahead_by >= 1 + then "proven-ancestor" else ((.status // "unknown") | tostring) end + ' <<<"$head_compare_json" 2>/dev/null || true)" + fi + if [ "$head_compare_status" = "proven-ancestor" ]; then + stale_reason="pull request head moved ahead of the dispatched head" + else + printf 'Not retiring CodeQL dispatch for %s#%s as stale: dispatched head is not a proven ancestor of the live head (compare=%s).\n' "$TARGET_REPOSITORY" "$PR_NUMBER" "${head_compare_status:-unavailable}" + fi + fi + if [ -n "$stale_reason" ]; then + printf '::notice::Skipping stale CodeQL dispatch for %s#%s: %s (dispatched head=%s, live head=%s, state=%s).\n' "$TARGET_REPOSITORY" "$PR_NUMBER" "$stale_reason" "$SUPPLIED_HEAD_SHA" "${live_head_sha:-}" "$live_state" + # Backticks are literal Markdown code spans in the step summary. + # shellcheck disable=SC2016 + printf -- '- Skipped stale CodeQL dispatch for %s#%s: %s (dispatched head `%s`, live head `%s`, state `%s`).\n' "$TARGET_REPOSITORY" "$PR_NUMBER" "$stale_reason" "$SUPPLIED_HEAD_SHA" "${live_head_sha:-}" "$live_state" >>"${GITHUB_STEP_SUMMARY:-/dev/null}" + { + printf 'stale=true\n' + printf 'target_repository=%s\n' "$TARGET_REPOSITORY" + printf 'pr_number=%s\n' "$PR_NUMBER" + # Keep the matrix well-formed so the skipped scan job's + # strategy never evaluates fromJSON on an empty string. + echo "matrix<>"$GITHUB_OUTPUT" + exit 0 + fi + + if [ "$live_state" != "open" ]; then + printf '::error::PR metadata validation rejected closed, missing, cross-fork, or malformed live metadata. target=%s#%s state=%s base_repo=%s head_repo=%s base=%s head=%s\n' "$TARGET_REPOSITORY" "$PR_NUMBER" "${live_state:-}" "${live_base_repository:-}" "${live_head_repository:-}" "${live_base_sha:-}" "${live_head_sha:-}" + exit 1 + fi + mismatches=() [ "$SUPPLIED_BASE_REF" = "$live_base_ref" ] || mismatches+=("base_ref") [ "$SUPPLIED_BASE_SHA" = "$live_base_sha" ] || mismatches+=("base_sha") @@ -409,6 +485,7 @@ jobs: fi { + printf 'stale=false\n' printf 'target_repository=%s\n' "$TARGET_REPOSITORY" printf 'pr_number=%s\n' "$PR_NUMBER" printf 'base_ref=%s\n' "$live_base_ref" @@ -432,6 +509,9 @@ jobs: scan: name: CodeQL dispatch scan (${{ matrix.language }}) needs: validate-dispatch + # A stale dispatch (closed PR or moved head) ends validate-dispatch with + # a notice; skipping here also skips settle-required-run via needs. + if: needs.validate-dispatch.outputs.stale != 'true' runs-on: group: CWL central CodeQL labels: [self-hosted, linux, x64] diff --git a/.github/workflows/opencode-review-dispatch.yml b/.github/workflows/opencode-review-dispatch.yml index 09bbf8181a..5e609e0699 100644 --- a/.github/workflows/opencode-review-dispatch.yml +++ b/.github/workflows/opencode-review-dispatch.yml @@ -71,6 +71,7 @@ jobs: head_ref: ${{ steps.validate.outputs.head_ref }} head_sha: ${{ steps.validate.outputs.head_sha }} is_private: ${{ steps.validate.outputs.is_private }} + stale: ${{ steps.validate.outputs.stale }} steps: - name: Exchange OpenCode app token for target repository metadata reads id: metadata_read_app_token @@ -223,7 +224,10 @@ jobs: *) live_is_private="" ;; esac - if [ "$live_state" != "open" ] || + # Validate the live identity that must remain stable before deciding + # whether a superseded head can be retired. Head SHA is intentionally + # checked later because a proven descendant is the stale condition. + if { [ "$live_state" != "open" ] && [ "$live_state" != "closed" ]; } || [ "$live_base_repository" != "$TARGET_REPOSITORY" ] || ! [[ "$live_head_repository" =~ ^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ ]] || ! [[ "$live_base_sha" =~ ^[0-9a-fA-F]{40}$ ]] || @@ -235,6 +239,76 @@ jobs: exit 1 fi + if [ "$EVENT_NAME" = "repository_dispatch" ]; then + stable_identity_mismatches=() + [ "$SUPPLIED_BASE_REF" = "$live_base_ref" ] || stable_identity_mismatches+=("base_ref") + [ "$SUPPLIED_BASE_SHA" = "$live_base_sha" ] || stable_identity_mismatches+=("base_sha") + [ "$SUPPLIED_HEAD_REF" = "$live_head_ref" ] || stable_identity_mismatches+=("head_ref") + if [ "${#stable_identity_mismatches[@]}" -gt 0 ]; then + printf '::error::repository_dispatch metadata does not match the live pull request: %s. supplied_base=%s/%s live_base=%s/%s supplied_head=%s/%s live_head=%s/%s\n' "$(IFS=,; printf '%s' "${stable_identity_mismatches[*]}")" "${SUPPLIED_BASE_REF:-}" "${SUPPLIED_BASE_SHA:-}" "$live_base_ref" "$live_base_sha" "${SUPPLIED_HEAD_REF:-}" "${SUPPLIED_HEAD_SHA:-}" "$live_head_ref" "$live_head_sha" + exit 1 + fi + fi + + # Stale-dispatch retirement. Under runner-queue saturation this run can + # start hours after the dispatched head was superseded or the pull + # request closed; the newer dispatch is itself still queued, so the + # workflow-level cancel-in-progress group cannot retire this run first. + # Only a well-formed live answer proving the dispatched head is no + # longer current ends the run as a notice. A failed `gh api` lookup has + # already exited non-zero above, and every other disagreement below + # (base, head ref, fork, malformed metadata) stays fail-closed. + # The EVENT_NAME guard mirrors this script's shape: the actor/sender/ + # target authorization above and the supplied-head binding below run + # only for repository_dispatch (other events bind to the live head), so + # retirement is reachable only on the authorized dispatch path. + stale_reason="" + if [ "$EVENT_NAME" != "repository_dispatch" ]; then + : + elif [ "$live_state" = "closed" ]; then + stale_reason="pull request is closed" + elif [ "$live_state" = "open" ] && + [[ "$live_head_sha" =~ ^[0-9a-fA-F]{40}$ ]] && + [[ "$SUPPLIED_HEAD_SHA" =~ ^[0-9a-fA-F]{40}$ ]] && + [ "${SUPPLIED_HEAD_SHA,,}" != "${live_head_sha,,}" ]; then + # A mismatch alone is not proof: right after a push the API can + # briefly serve the previous head, which would retire a brand-new + # dispatch. Retire only when the compare API proves the dispatched + # head is a strict ancestor of the live head (status "ahead", + # behind_by 0), i.e. the live head is newer and gets its own + # dispatch. Behind, diverged (force-push), or a failed/unexpected + # compare falls through to the fail-closed head_sha mismatch below. + head_compare_status="" + if head_compare_json="$(gh api "repos/${TARGET_REPOSITORY}/compare/${SUPPLIED_HEAD_SHA}...${live_head_sha}" 2>/dev/null)"; then + head_compare_status="$(jq -r ' + if .status == "ahead" and .behind_by == 0 and ((.ahead_by | type) == "number") and .ahead_by >= 1 + then "proven-ancestor" else ((.status // "unknown") | tostring) end + ' <<<"$head_compare_json" 2>/dev/null || true)" + fi + if [ "$head_compare_status" = "proven-ancestor" ]; then + stale_reason="pull request head moved ahead of the dispatched head" + else + printf 'Not retiring OpenCode review dispatch for %s#%s as stale: dispatched head is not a proven ancestor of the live head (compare=%s).\n' "$TARGET_REPOSITORY" "$PR_NUMBER" "${head_compare_status:-unavailable}" + fi + fi + if [ -n "$stale_reason" ]; then + printf '::notice::Skipping stale OpenCode review dispatch for %s#%s: %s (dispatched head=%s, live head=%s, state=%s).\n' "$TARGET_REPOSITORY" "$PR_NUMBER" "$stale_reason" "$SUPPLIED_HEAD_SHA" "${live_head_sha:-}" "$live_state" + # Backticks are literal Markdown code spans in the step summary. + # shellcheck disable=SC2016 + printf -- '- Skipped stale OpenCode review dispatch for %s#%s: %s (dispatched head `%s`, live head `%s`, state `%s`).\n' "$TARGET_REPOSITORY" "$PR_NUMBER" "$stale_reason" "$SUPPLIED_HEAD_SHA" "${live_head_sha:-}" "$live_state" >>"${GITHUB_STEP_SUMMARY:-/dev/null}" + { + printf 'stale=true\n' + printf 'target_repository=%s\n' "$TARGET_REPOSITORY" + printf 'pr_number=%s\n' "$PR_NUMBER" + } >>"$GITHUB_OUTPUT" + exit 0 + fi + + if [ "$live_state" != "open" ]; then + printf '::error::PR metadata validation rejected closed, missing, or malformed live metadata. target=%s#%s state=%s base_repo=%s head_repo=%s base=%s head=%s\n' "$TARGET_REPOSITORY" "$PR_NUMBER" "${live_state:-}" "${live_base_repository:-}" "${live_head_repository:-}" "${live_base_sha:-}" "${live_head_sha:-}" + exit 1 + fi + if [ "$EVENT_NAME" = "repository_dispatch" ]; then mismatches=() [ "$SUPPLIED_BASE_REF" = "$live_base_ref" ] || mismatches+=("base_ref") @@ -248,6 +322,7 @@ jobs: fi { + printf 'stale=false\n' printf 'target_repository=%s\n' "$TARGET_REPOSITORY" printf 'pr_number=%s\n' "$PR_NUMBER" printf 'base_ref=%s\n' "$live_base_ref" @@ -262,6 +337,7 @@ jobs: id: coverage_read_app_token if: >- github.event_name == 'repository_dispatch' + && steps.validate.outputs.stale != 'true' && steps.validate.outputs.target_repository != '' && steps.validate.outputs.target_repository != github.repository env: @@ -329,6 +405,7 @@ jobs: } >>"$GITHUB_OUTPUT" - name: Materialize pull request merge tree for coverage measurement + if: steps.validate.outputs.stale != 'true' env: GH_TOKEN: ${{ steps.coverage_read_app_token.outputs.token || secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN || github.token }} TARGET_REPOSITORY: ${{ steps.validate.outputs.target_repository }} @@ -383,6 +460,7 @@ jobs: tar -cf "$COVERAGE_SOURCE_ARCHIVE" -C "$COVERAGE_SOURCE_WORKDIR" . - name: Upload materialized pull request merge tree + if: steps.validate.outputs.stale != 'true' uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: opencode-coverage-source @@ -396,6 +474,7 @@ jobs: if: >- needs.validate-pr-metadata.result == 'success' && github.event_name == 'repository_dispatch' + && needs.validate-pr-metadata.outputs.stale != 'true' runs-on: group: CWL central OpenCode labels: [self-hosted, linux, x64] @@ -2523,6 +2602,7 @@ jobs: && needs.validate-pr-metadata.result == 'success' && needs.coverage-evidence.result != 'cancelled' && github.event_name == 'repository_dispatch' + && needs.validate-pr-metadata.outputs.stale != 'true' concurrency: group: >- opencode-review-${{ diff --git a/CHANGELOG.d/20260926-dispatch-stale-head-skip.md b/CHANGELOG.d/20260926-dispatch-stale-head-skip.md new file mode 100644 index 0000000000..8755306324 --- /dev/null +++ b/CHANGELOG.d/20260926-dispatch-stale-head-skip.md @@ -0,0 +1,23 @@ +### Stale CodeQL/OpenCode dispatches end as a notice instead of failing + +- `codeql-scan-dispatch.yml` (`validate-dispatch`) and `opencode-review-dispatch.yml` + (`validate-pr-metadata`) now retire a dispatch whose target pull request is closed, or whose + live head has moved strictly past the dispatched head, with a `::notice::`, a step-summary + line and a `stale=true` output. A head mismatch alone is not enough: the compare API + (`repos/{target}/compare/{dispatched}...{live}`) must answer `status: ahead` with + `behind_by: 0`, proving the live head descends from the dispatched one and therefore gets + its own dispatch. A lagging API that still serves the previous head right after a push + (`behind`), a force-push (`diverged`), or a failed or malformed compare keeps the original + fail-closed `head_sha` mismatch, so a fresh head is never skipped. The CodeQL `scan` matrix (and therefore `settle-required-run`) + and the OpenCode coverage-materialization steps, `coverage-evidence` and `opencode-review` + skip on that output, so the run concludes success instead of failure. Under runner-queue + saturation the newer dispatch for the same pull request is itself queued, so workflow-level + `cancel-in-progress` could not retire the stale run before it started. +- Still fail-closed: dispatch authorization and payload validation (checked first), a failed or + malformed live pull-request lookup, base ref/SHA or head ref disagreement at the same head, + cross-fork metadata, and the later privileged re-validation steps. Draft state is not treated + as stale because ruleset-launched required workflows do not re-dispatch on `ready_for_review`. +- The OpenCode side keeps retirement behind `EVENT_NAME == repository_dispatch` because its + dispatch authorization and supplied-head binding are nested under that check; the CodeQL + side needs no such guard because it triggers only on `repository_dispatch` and authorizes + unconditionally before the live lookup. diff --git a/CHANGELOG.md b/CHANGELOG.md index d90fa0c899..e61e1d53db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,13 @@ +### Stale dispatch retirement validates stable PR identity first + +- CodeQL and OpenCode dispatch validators now reject malformed live metadata, + cross-repository identity where forbidden, and base/ref disagreements before + treating a proven superseded head as stale. A newer descendant head remains + the only head-SHA mismatch that can retire an open-PR dispatch successfully; + unrelated identity drift fails closed. Regression evidence is 132 passed and + 1 skipped in both local and Actions mode, and the full warnings-as-errors + suite is 5,177 passed, 11 skipped, and 40 subtests passed. + ### Intel macOS native archives are bound to x86_64 bytes - The release prescreener now requires every native member in an Intel macOS diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 6e5f1c549a..2e0b307e13 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -7,6 +7,12 @@ 이 문서는 제품·기술·운영 Gap을 현재 문서와 현재 GitHub 상태에 묶어 두는 기준선이다. 새 작업은 먼저 이 문서의 Gap ID를 PR 설명과 테스트 증거에 연결하고, PR의 정확한 exact HEAD·Checks·리뷰를 다시 수집한 뒤 구현한다. 표의 상태는 작성 시점의 관측값이므로, 병합 판단에는 재사용하지 않는다. 이 인벤토리는 스냅샷이며 merge authorization이 아니다. +### 2026-10-07 stale-dispatch stack reconciliation delta + +| Gap ID | 상태 | exact-head evidence | causal owner / next gate | +|---|---|---|---| +| CONTROL-STALE-DISPATCH-STACK-01 | **Proposed — `.github#2382` source repair complete; hosted acceptance pending** | Canonical prerequisite `.github#2385` merged and ordinary two-parent merge `ca3fe966c0508a0274be7435ad4642756db2ac49` preserved both parents. Fresh review found that a proven descendant head could retire the run before base SHA/ref identity was checked, masking unrelated metadata drift. RED `d7d82dbf` reproduces the mixed moved-head/base-SHA mismatch in both CodeQL and OpenCode validators; GREEN `f7de3e2c` validates live shape and stable base/head-ref identity before stale retirement and advances the reviewed OpenCode workflow blob pin. Exact tree evidence is 132 passed and 1 skipped in both local and Actions mode, plus 5,177 passed, 11 skipped, and 40 subtests in the warnings-as-errors repository suite. | Central `.github` owns dispatch retirement and runner admission. Publish only by expected-head, non-force fast-forward; then re-fetch the new exact head, reviews, threads, and hosted Checks. Ready status does not authorize merge: terminal source-executing hosted Checks, no unresolved actionable review, qualifying independent approval, and ordinary protected integration remain mandatory. | + ### 2026-09-19 exact-head incident delta | Gap ID | 상태 | exact-head evidence | causal owner / next gate | diff --git a/tests/test_codeql_scan_dispatch_workflow_contract.py b/tests/test_codeql_scan_dispatch_workflow_contract.py index 74c01d7989..cc8a25f7f5 100644 --- a/tests/test_codeql_scan_dispatch_workflow_contract.py +++ b/tests/test_codeql_scan_dispatch_workflow_contract.py @@ -147,8 +147,12 @@ def _run_validate_step(tmp_path: Path, env_overrides: dict[str, str], pull_reque "set -euo pipefail\n" 'test "$1" = api\n' 'endpoint="${!#}"\n' + 'printf \'%s\\n\' "$endpoint" >>"$FAKE_GH_LOG"\n' 'case "$endpoint" in\n' ' repos/ContextualWisdomLab/.github/compare/*) printf \'%s\\n\' "$FAKE_SOURCE_COMPARE_JSON" ;;\n' + ' repos/ContextualWisdomLab/*/compare/*)\n' + ' if [ -z "$FAKE_HEAD_COMPARE_JSON" ]; then echo \'HTTP 404\' >&2; exit 1; fi\n' + ' printf \'%s\\n\' "$FAKE_HEAD_COMPARE_JSON" ;;\n' ' repos/ContextualWisdomLab/*/git/commits/*) printf \'%s\\n\' "$FAKE_PRODUCER_COMMIT_JSON" ;;\n' ' *) printf \'%s\\n\' "$FAKE_PULL_JSON" ;;\n' 'esac\n', @@ -162,6 +166,10 @@ def _run_validate_step(tmp_path: Path, env_overrides: dict[str, str], pull_reque "PATH": f"{fake_bin}:{os.environ['PATH']}", "FAKE_PULL_JSON": json.dumps(pull_request), "FAKE_SOURCE_COMPARE_JSON": "{}", + # Head-ancestry compare used by stale-dispatch retirement; empty means + # the compare lookup fails (HTTP 404). + "FAKE_HEAD_COMPARE_JSON": "", + "FAKE_GH_LOG": str(tmp_path / "gh-calls.log"), "FAKE_PRODUCER_COMMIT_JSON": json.dumps( { "sha": "c" * 40, @@ -940,26 +948,240 @@ def test_codeql_scan_dispatch_validate_step_rejects_unusable_legacy_payload(tmp_ assert "does not match the dispatched languages one-to-one" in invalid_job_id.stdout -def test_codeql_scan_dispatch_validate_step_rejects_stale_head_sha(tmp_path): - """A dispatch whose supplied head SHA no longer matches the live PR head is rejected.""" +def _assert_stale_skip(tmp_path: Path, result: subprocess.CompletedProcess[str], reason: str) -> None: + """A stale dispatch ends validate-dispatch successfully with a notice and skip output.""" + assert result.returncode == 0, result.stdout + result.stderr + assert "::notice::Skipping stale CodeQL dispatch for ContextualWisdomLab/naruon#42" in result.stdout + assert reason in result.stdout + assert "::error::" not in result.stdout + outputs = result.output_path.read_text(encoding="utf-8") # type: ignore[attr-defined] + assert "stale=true\n" in outputs + assert "stale=false" not in outputs + # The skipped scan job still receives a well-formed matrix, but no head + # identity is published for privileged steps to consume. + assert "matrix<busy", + "lookup_failed": "", + } + for name, compare_json in cases.items(): + moved_head = _matching_pull_request() + moved_head["head"]["sha"] = "d" * 40 + result = _run_validate_step(tmp_path / name, {"FAKE_HEAD_COMPARE_JSON": compare_json}, moved_head) + assert result.returncode == 1, (name, result.stdout, result.stderr) + assert "does not match the live pull request: head_sha" in result.stdout, name + assert "Not retiring CodeQL dispatch" in result.stdout, name + assert "::notice::Skipping stale" not in result.stdout, name + outputs_path = result.output_path # type: ignore[attr-defined] + assert not outputs_path.exists() or "stale=true" not in outputs_path.read_text(encoding="utf-8"), name + + +def test_codeql_scan_dispatch_current_head_never_calls_head_compare(tmp_path): + """The ancestry lookup only runs on a head mismatch; a current dispatch needs no extra API call.""" + result = _run_validate_step(tmp_path, {}, _matching_pull_request()) + + assert result.returncode == 0, result.stdout + result.stderr + calls = (tmp_path / "gh-calls.log").read_text(encoding="utf-8").splitlines() + assert not [call for call in calls if "/naruon/compare/" in call] + + +def test_codeql_scan_dispatch_unauthorized_dispatch_is_rejected_before_stale_detection(tmp_path): + """Retirement is reachable only after the unconditional actor/sender authorization.""" + moved_head = _matching_pull_request() + moved_head["head"]["sha"] = "d" * 40 + + result = _run_validate_step( + tmp_path, + {"DISPATCH_SENDER": "someone-else", "FAKE_HEAD_COMPARE_JSON": AHEAD_COMPARE_JSON}, + moved_head, + ) assert result.returncode == 1 - assert "does not match the live pull request: head_sha" in result.stdout + assert "repository_dispatch authorization rejected" in result.stdout + assert "::notice::Skipping stale" not in result.stdout + outputs_path = result.output_path # type: ignore[attr-defined] + assert not outputs_path.exists() or "stale=true" not in outputs_path.read_text(encoding="utf-8") + + +def test_codeql_stale_retirement_needs_no_event_guard_unlike_opencode(): + """The EVENT_NAME asymmetry with opencode-review-dispatch.yml is structural, not an omission. + + OpenCode nests its dispatch authorization under `EVENT_NAME == repository_dispatch`, + so its stale retirement carries the same guard. CodeQL is triggered only by + repository_dispatch and authorizes unconditionally before the live lookup, so + every run reaching retirement is already an authorized dispatch. If either + premise changes, this test forces the guard question to be revisited. + """ + workflow = WORKFLOW_PATH.read_text(encoding="utf-8") + triggers = workflow.split("\non:\n", 1)[1].split("\nconcurrency:\n", 1)[0] + assert [line.strip() for line in triggers.splitlines() if line.strip()] == [ + "repository_dispatch:", + "types: [codeql-scan, codeql-scan-v2]", + ] + script = _extract_run_block(workflow, VALIDATE_STEP_NAME) + assert "$EVENT_NAME" not in script and "github.event_name" not in script + auth_index = script.index('if [ "$actor_allowed" -ne 1 ]; then') + lookup_index = script.index('pull_request_json="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}")"') + stale_index = script.index('stale_reason=""') + assert auth_index < lookup_index < stale_index + # The authorization check sits at the top level of the script, not inside a branch. + assert script.splitlines()[script[:auth_index].count("\n")].startswith("if ") -def test_codeql_scan_dispatch_validate_step_rejects_closed_pull_request(tmp_path): - """A dispatch targeting a pull request that closed before this run started is rejected.""" + opencode = (REPO_ROOT / ".github/workflows/opencode-review-dispatch.yml").read_text(encoding="utf-8") + opencode_script = _extract_run_block(opencode, VALIDATE_STEP_NAME) + assert opencode_script.startswith('set -euo pipefail\nif [ "$EVENT_NAME" = "repository_dispatch" ]; then\n') + assert 'stale_reason=""\nif [ "$EVENT_NAME" != "repository_dispatch" ]; then\n :\n' in opencode_script + + +def test_codeql_scan_dispatch_validate_step_skips_closed_pull_request(tmp_path): + """A dispatch targeting a pull request that closed before this run started is retired as a notice.""" closed_pull_request = _matching_pull_request() closed_pull_request["state"] = "closed" - result = _run_validate_step(tmp_path, {}, closed_pull_request) + result = _run_validate_step( + tmp_path, {"GITHUB_STEP_SUMMARY": str(tmp_path / "step-summary")}, closed_pull_request + ) + + _assert_stale_skip(tmp_path, result, "pull request is closed") - assert result.returncode == 1 - assert "rejected closed, missing, cross-fork, or malformed live metadata" in result.stdout + +def test_codeql_scan_dispatch_validate_step_marks_current_head_not_stale(tmp_path): + """A dispatch for the live head publishes stale=false and keeps the full identity.""" + result = _run_validate_step(tmp_path, {}, _matching_pull_request()) + + assert result.returncode == 0, result.stdout + result.stderr + outputs = result.output_path.read_text(encoding="utf-8") # type: ignore[attr-defined] + assert "stale=false\n" in outputs + assert "stale=true" not in outputs + assert f"head_sha={'b' * 40}\n" in outputs + assert "::notice::Skipping stale" not in result.stdout + + +def test_codeql_scan_dispatch_stale_skip_keeps_other_mismatches_fail_closed(tmp_path): + """Only a moved head or a closed PR is retired; every other disagreement still fails.""" + moved_base = _matching_pull_request() + moved_base["base"]["sha"] = "e" * 40 + renamed_head_ref = _matching_pull_request() + renamed_head_ref["head"]["ref"] = "other" + cross_fork = _matching_pull_request() + cross_fork["head"]["repo"]["full_name"] = "someone/naruon" + malformed_head = _matching_pull_request() + malformed_head["head"]["sha"] = "not-a-sha" + missing_state = _matching_pull_request() + del missing_state["state"] + + cases = { + "moved_base": (moved_base, "does not match the live pull request: base_sha"), + "renamed_head_ref": (renamed_head_ref, "does not match the live pull request: head_ref"), + "cross_fork": (cross_fork, "rejected closed, missing, cross-fork, or malformed live metadata"), + "malformed_head": (malformed_head, "rejected closed, missing, cross-fork, or malformed live metadata"), + "missing_state": (missing_state, "rejected closed, missing, cross-fork, or malformed live metadata"), + } + for name, (pull_request, message) in cases.items(): + result = _run_validate_step(tmp_path / name, {}, pull_request) + assert result.returncode == 1, name + assert message in result.stdout, name + assert "::notice::Skipping stale" not in result.stdout, name + outputs_path = result.output_path # type: ignore[attr-defined] + assert not outputs_path.exists() or "stale=true" not in outputs_path.read_text(encoding="utf-8"), name + + +def test_codeql_scan_dispatch_stale_skip_does_not_mask_lookup_failure(tmp_path): + """If the live pull request lookup itself fails, the run fails instead of skipping.""" + failing_bin = tmp_path / "failing-bin" + failing_bin.mkdir() + failing_gh = failing_bin / "gh" + failing_gh.write_text("#!/usr/bin/env bash\necho 'HTTP 502' >&2\nexit 1\n", encoding="utf-8") + failing_gh.chmod(0o755) + + result = _run_validate_step( + tmp_path / "run", + {"PATH": f"{failing_bin}:{os.environ['PATH']}"}, + _matching_pull_request(), + ) + + assert result.returncode != 0 + assert "::notice::Skipping stale" not in result.stdout + outputs_path = result.output_path # type: ignore[attr-defined] + assert not outputs_path.exists() or "stale=true" not in outputs_path.read_text(encoding="utf-8") + + +def test_codeql_scan_dispatch_stale_output_gates_scan_and_settlement(): + """stale=true skips the matrix scan job, which in turn skips settlement.""" + workflow = WORKFLOW_PATH.read_text(encoding="utf-8") + validate_job = workflow.split(" validate-dispatch:\n", 1)[1].split(" steps:\n", 1)[0] + scan_header = workflow.split(" scan:\n", 1)[1].split(" strategy:\n", 1)[0] + settle_header = workflow.split(" settle-required-run:\n", 1)[1].split(" steps:\n", 1)[0] + + assert "stale: ${{ steps.validate.outputs.stale }}" in validate_job + assert "if: needs.validate-dispatch.outputs.stale != 'true'" in scan_header + assert "needs.scan.result != 'skipped'" in settle_header + assert "needs.validate-dispatch.result == 'success'" in settle_header def test_codeql_scan_dispatch_is_not_in_the_required_workflow_ruleset_scope(): diff --git a/tests/test_opencode_review_dispatch_stale_skip.py b/tests/test_opencode_review_dispatch_stale_skip.py new file mode 100644 index 0000000000..361d709618 --- /dev/null +++ b/tests/test_opencode_review_dispatch_stale_skip.py @@ -0,0 +1,296 @@ +"""Stale OpenCode review dispatches end as a notice instead of a failure. + +Under organization runner-queue saturation a repository_dispatch for an older +pull request head can start hours after the head moved or the pull request +closed. The newer dispatch is itself still queued, so the workflow-level +``cancel-in-progress`` group cannot retire the stale run first. The validate +step must then end the run with a ``::notice::`` and ``stale=true`` so the +coverage and review jobs skip, while every other metadata disagreement and any +failed lookup keeps failing closed. +""" + +from __future__ import annotations + +import json +import os +import shutil +import subprocess +from pathlib import Path + +from tests.test_opencode_workflow_shell_syntax import _extract_run_block + +REPO_ROOT = Path(__file__).resolve().parents[1] +WORKFLOW_PATH = REPO_ROOT / ".github/workflows/opencode-review-dispatch.yml" +VALIDATE_STEP_NAME = "Bind workflow inputs to live organization pull request metadata" +TARGET = "ContextualWisdomLab/naruon" + + +def _live_pull_request() -> dict: + """A live PR payload matching the default supplied dispatch metadata.""" + return { + "state": "open", + "base": { + "repo": {"full_name": TARGET, "visibility": "private"}, + "ref": "main", + "sha": "a" * 40, + }, + "head": {"repo": {"full_name": TARGET}, "ref": "feature", "sha": "b" * 40}, + } + + +def _run_validate_step( + tmp_path: Path, + pull_request: dict, + env_overrides: dict[str, str] | None = None, +) -> subprocess.CompletedProcess[str]: + """Execute the real validate-pr-metadata shell block against a fake `gh api`.""" + bash = shutil.which("bash") + jq = shutil.which("jq") + assert bash is not None and jq is not None, "bash and jq are required to run this test" + + script = _extract_run_block(WORKFLOW_PATH.read_text(encoding="utf-8"), VALIDATE_STEP_NAME) + fake_bin = tmp_path / "bin" + fake_bin.mkdir(parents=True) + fake_gh = fake_bin / "gh" + fake_gh.write_text( + "#!/usr/bin/env bash\n" + "set -euo pipefail\n" + 'test "$1" = api\n' + 'endpoint="${!#}"\n' + 'printf \'%s\\n\' "$endpoint" >>"$FAKE_GH_LOG"\n' + 'case "$endpoint" in\n' + ' repos/*/compare/*)\n' + ' if [ -z "$FAKE_HEAD_COMPARE_JSON" ]; then echo \'HTTP 404\' >&2; exit 1; fi\n' + ' printf \'%s\\n\' "$FAKE_HEAD_COMPARE_JSON" ;;\n' + ' *) printf \'%s\\n\' "$FAKE_PULL_JSON" ;;\n' + 'esac\n', + encoding="utf-8", + ) + fake_gh.chmod(0o755) + + output = tmp_path / "github-output" + summary = tmp_path / "step-summary" + env = { + **os.environ, + "PATH": f"{fake_bin}:{os.environ['PATH']}", + "FAKE_PULL_JSON": json.dumps(pull_request), + # Head-ancestry compare used by stale retirement; empty = lookup fails. + "FAKE_HEAD_COMPARE_JSON": "", + "FAKE_GH_LOG": str(tmp_path / "gh-calls.log"), + "GITHUB_OUTPUT": str(output), + "GITHUB_STEP_SUMMARY": str(summary), + "EVENT_NAME": "repository_dispatch", + "DISPATCH_ACTOR": "opencode-agent[bot]", + "DISPATCH_SENDER": "opencode-agent[bot]", + "ALLOWED_DISPATCH_ACTOR": "opencode-agent[bot]", + "ALLOWED_DISPATCH_TARGETS": TARGET, + "TARGET_REPOSITORY": TARGET, + "PR_NUMBER": "42", + "SUPPLIED_BASE_REF": "main", + "SUPPLIED_BASE_SHA": "a" * 40, + "SUPPLIED_HEAD_REF": "feature", + "SUPPLIED_HEAD_SHA": "b" * 40, + **(env_overrides or {}), + } + result = subprocess.run([bash], input=script, text=True, capture_output=True, check=False, env=env) + result.output_path = output # type: ignore[attr-defined] + result.summary_path = summary # type: ignore[attr-defined] + return result + + +def _outputs(result: subprocess.CompletedProcess[str]) -> str: + path = result.output_path # type: ignore[attr-defined] + return path.read_text(encoding="utf-8") if path.exists() else "" + + +def _assert_stale_skip(result: subprocess.CompletedProcess[str], reason: str) -> None: + assert result.returncode == 0, result.stdout + result.stderr + assert f"::notice::Skipping stale OpenCode review dispatch for {TARGET}#42" in result.stdout + assert reason in result.stdout + assert "::error::" not in result.stdout + outputs = _outputs(result) + assert "stale=true\n" in outputs + assert "stale=false" not in outputs + assert "head_sha=" not in outputs + assert "base_sha=" not in outputs + summary = result.summary_path.read_text(encoding="utf-8") # type: ignore[attr-defined] + assert f"Skipped stale OpenCode review dispatch for {TARGET}#42" in summary + + +def test_current_head_dispatch_is_not_stale(tmp_path): + result = _run_validate_step(tmp_path, _live_pull_request()) + + assert result.returncode == 0, result.stdout + result.stderr + outputs = _outputs(result) + assert "stale=false\n" in outputs + assert f"head_sha={'b' * 40}\n" in outputs + assert "::notice::Skipping stale" not in result.stdout + + +AHEAD_COMPARE_JSON = json.dumps({"status": "ahead", "ahead_by": 2, "behind_by": 0}) + + +def test_moved_head_dispatch_is_skipped_with_notice(tmp_path): + pull_request = _live_pull_request() + pull_request["head"]["sha"] = "c" * 40 + + result = _run_validate_step(tmp_path, pull_request, {"FAKE_HEAD_COMPARE_JSON": AHEAD_COMPARE_JSON}) + + _assert_stale_skip(result, "pull request head moved ahead of the dispatched head") + assert f"dispatched head={'b' * 40}, live head={'c' * 40}" in result.stdout + # dispatched...live order: "ahead" means the live head descends from the dispatched head. + calls = (tmp_path / "gh-calls.log").read_text(encoding="utf-8").splitlines() + assert f"repos/{TARGET}/compare/{'b' * 40}...{'c' * 40}" in calls + + +def test_moved_head_dispatch_does_not_mask_base_mismatch(tmp_path): + """Proven head ancestry cannot retire a dispatch bound to a different base.""" + pull_request = _live_pull_request() + pull_request["head"]["sha"] = "c" * 40 + pull_request["base"]["sha"] = "e" * 40 + + result = _run_validate_step( + tmp_path, + pull_request, + {"FAKE_HEAD_COMPARE_JSON": AHEAD_COMPARE_JSON}, + ) + + assert result.returncode == 1, result.stdout + result.stderr + assert "does not match the live pull request: base_sha" in result.stdout + assert "::notice::Skipping stale" not in result.stdout + assert "stale=true" not in _outputs(result) + + +def test_head_mismatch_without_proven_ancestry_stays_fail_closed(tmp_path): + """Only a live head that provably descends from the dispatched head retires the run. + + API lag right after a push (live=previous head, compare "behind"), force-push + ("diverged"), and any failed or contradictory compare answer keep the original + fail-closed head_sha mismatch, so a fresh head is never silently skipped. + """ + cases = { + "api_lag_behind": json.dumps({"status": "behind", "ahead_by": 0, "behind_by": 1}), + "force_push_diverged": json.dumps({"status": "diverged", "ahead_by": 1, "behind_by": 3}), + "ahead_but_behind_by": json.dumps({"status": "ahead", "ahead_by": 1, "behind_by": 1}), + "ahead_without_counts": json.dumps({"status": "ahead"}), + "not_json": "busy", + "lookup_failed": "", + } + for name, compare_json in cases.items(): + pull_request = _live_pull_request() + pull_request["head"]["sha"] = "c" * 40 + result = _run_validate_step(tmp_path / name, pull_request, {"FAKE_HEAD_COMPARE_JSON": compare_json}) + assert result.returncode == 1, (name, result.stdout, result.stderr) + assert "does not match the live pull request: head_sha" in result.stdout, name + assert "Not retiring OpenCode review dispatch" in result.stdout, name + assert "::notice::Skipping stale" not in result.stdout, name + assert "stale=true" not in _outputs(result), name + + +def test_non_dispatch_event_is_never_retired(tmp_path): + """The EVENT_NAME guard mirrors the script's dispatch-only authorization and binding. + + For any other event the step never authorizes a dispatcher and ignores the + supplied head, binding to the live head instead, so there is no dispatched head + that could be stale. A moved supplied head must not retire the run, and a + closed pull request keeps failing closed. + """ + moved_head = _live_pull_request() + moved_head["head"]["sha"] = "c" * 40 + override = {"EVENT_NAME": "workflow_dispatch", "FAKE_HEAD_COMPARE_JSON": AHEAD_COMPARE_JSON} + + result = _run_validate_step(tmp_path / "moved_head", moved_head, override) + assert result.returncode == 0, result.stdout + result.stderr + assert "::notice::Skipping stale" not in result.stdout + outputs = _outputs(result) + assert "stale=false\n" in outputs and "stale=true" not in outputs + assert f"head_sha={'c' * 40}\n" in outputs + + closed = _live_pull_request() + closed["state"] = "closed" + result = _run_validate_step(tmp_path / "closed", closed, override) + assert result.returncode == 1 + assert "rejected closed, missing, or malformed live metadata" in result.stdout + assert "::notice::Skipping stale" not in result.stdout + assert "stale=true" not in _outputs(result) + + +def test_closed_pull_request_dispatch_is_skipped_with_notice(tmp_path): + pull_request = _live_pull_request() + pull_request["state"] = "closed" + + result = _run_validate_step(tmp_path, pull_request) + + _assert_stale_skip(result, "pull request is closed") + + +def test_other_metadata_disagreements_stay_fail_closed(tmp_path): + moved_base = _live_pull_request() + moved_base["base"]["sha"] = "e" * 40 + renamed_head_ref = _live_pull_request() + renamed_head_ref["head"]["ref"] = "other" + malformed_head = _live_pull_request() + malformed_head["head"]["sha"] = "not-a-sha" + missing_state = _live_pull_request() + del missing_state["state"] + + cases = { + "moved_base": (moved_base, "does not match the live pull request: base_sha"), + "renamed_head_ref": (renamed_head_ref, "does not match the live pull request: head_ref"), + "malformed_head": (malformed_head, "rejected closed, missing, or malformed live metadata"), + "missing_state": (missing_state, "rejected closed, missing, or malformed live metadata"), + } + for name, (pull_request, message) in cases.items(): + result = _run_validate_step(tmp_path / name, pull_request) + assert result.returncode == 1, name + assert message in result.stdout, name + assert "::notice::Skipping stale" not in result.stdout, name + assert "stale=true" not in _outputs(result), name + + +def test_unauthorized_dispatch_is_rejected_before_stale_detection(tmp_path): + pull_request = _live_pull_request() + pull_request["head"]["sha"] = "c" * 40 + + result = _run_validate_step( + tmp_path, + pull_request, + {"DISPATCH_SENDER": "someone-else", "FAKE_HEAD_COMPARE_JSON": AHEAD_COMPARE_JSON}, + ) + + assert result.returncode == 1 + assert "repository_dispatch authorization rejected" in result.stdout + assert "stale=true" not in _outputs(result) + + +def test_failed_lookup_is_not_treated_as_stale(tmp_path): + failing_bin = tmp_path / "failing-bin" + failing_bin.mkdir() + failing_gh = failing_bin / "gh" + failing_gh.write_text("#!/usr/bin/env bash\necho 'HTTP 502' >&2\nexit 1\n", encoding="utf-8") + failing_gh.chmod(0o755) + + result = _run_validate_step( + tmp_path / "run", + _live_pull_request(), + {"PATH": f"{failing_bin}:{os.environ['PATH']}"}, + ) + + assert result.returncode != 0 + assert "::notice::Skipping stale" not in result.stdout + assert "stale=true" not in _outputs(result) + + +def test_stale_output_gates_every_later_step_and_job(): + workflow = WORKFLOW_PATH.read_text(encoding="utf-8") + metadata_job = workflow.split(" validate-pr-metadata:\n", 1)[1].split("\n coverage-evidence:\n", 1)[0] + coverage_header = workflow.split("\n coverage-evidence:\n", 1)[1].split(" runs-on:", 1)[0] + review_header = workflow.split("\n opencode-review-target:\n", 1)[1].split(" concurrency:", 1)[0] + + assert "stale: ${{ steps.validate.outputs.stale }}" in metadata_job + later_steps = metadata_job.split(f" - name: {VALIDATE_STEP_NAME}\n", 1)[1].split("\n - name: ")[1:] + assert later_steps, "validate must be followed by the coverage materialization steps" + for step in later_steps: + assert "steps.validate.outputs.stale != 'true'" in step, step.splitlines()[0] + assert "needs.validate-pr-metadata.outputs.stale != 'true'" in coverage_header + assert "needs.validate-pr-metadata.outputs.stale != 'true'" in review_header diff --git a/tests/test_pr_review_autofix_nvidia_nim_contract.py b/tests/test_pr_review_autofix_nvidia_nim_contract.py index 1bfc13292e..3635f489d0 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 = "09bbf8181a443f7a5630ec91ca958440c5dcfc68" +REVIEW_DISPATCH_BLOB_SHA = "5e609e0699cedb9aeeb0158d28e66dd4e4da8fca" def _workflow_text(path: Path) -> str: