From 67c68928eab022b146cdd9d9405628aad0d83335 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 1 Oct 2026 09:17:58 +0900 Subject: [PATCH 01/10] fix(opencode): serialize exact-head dispatch admission --- .../workflows/opencode-review-dispatch.yml | 552 +++++++++++++-- .github/workflows/opencode-review.yml | 110 +++ ...opencode-same-head-dispatch-idempotency.md | 7 + CHANGELOG.md | 24 + ...opencode-same-head-dispatch-idempotency.md | 125 ++++ docs/product-technical-gap-baseline.md | 6 + tests/test_opencode_agent_contract.py | 32 +- .../test_opencode_required_rerun_capacity.py | 378 +++++++++- ...st_opencode_required_verdict_regression.py | 647 ++++++++++++++++-- ...t_pr_review_autofix_nvidia_nim_contract.py | 2 +- ...t_required_review_runner_image_contract.py | 4 +- .../test_required_workflow_queue_contract.py | 29 +- 12 files changed, 1771 insertions(+), 145 deletions(-) create mode 100644 CHANGELOG.d/20261001-opencode-same-head-dispatch-idempotency.md create mode 100644 docs/doctoring/opencode-same-head-dispatch-idempotency.md diff --git a/.github/workflows/opencode-review-dispatch.yml b/.github/workflows/opencode-review-dispatch.yml index 09bbf8181a..10707b0704 100644 --- a/.github/workflows/opencode-review-dispatch.yml +++ b/.github/workflows/opencode-review-dispatch.yml @@ -11,34 +11,360 @@ on: repository_dispatch: types: [opencode-review] -concurrency: - # Workflow-level admission, for the same reason strix.yml, noema-review.yml and - # opencode-review.yml carry theirs at this level: a job-level group is never - # evaluated while the whole run waits behind the organization job ceiling, so - # superseded dispatches for one pull request coalesce only after each of them - # has already been allocated a runner. Measured on 2026-09-06: of the five - # dispatch runs that passed `validate-pr-metadata`, four were rejected hours - # later by `opencode-review`'s privileged metadata check because the head had - # moved while they queued (runs 34002473295, 34010256951, 34015973300, - # 34016922761) -- each after `coverage-source-tree` and `coverage-evidence` - # had run. Cancelling the superseded run at creation returns that slot instead - # of spending it to discover the review's subject no longer exists. - # - # The key is the target pull request, matching the job-level group below and - # codeql-scan-dispatch.yml's workflow-level group; `github.run_id` keeps runs - # without a payload in their own groups rather than colliding. - group: >- - opencode-review-dispatch-${{ - github.event.client_payload.target_repository || github.repository }}-${{ - github.event.client_payload.pr_number || github.run_id }} - cancel-in-progress: true - permissions: contents: read jobs: + admit-exact-head-dispatch: + name: admit-exact-head-dispatch + if: github.event_name == 'repository_dispatch' + runs-on: + group: CWL central OpenCode + labels: [self-hosted, linux, x64] + timeout-minutes: 10 + permissions: + actions: read + contents: write + pull-requests: read + id-token: write + outputs: + admitted: ${{ steps.single_flight.outputs.admitted }} + steps: + - name: Authorize repository dispatch envelope + env: + EVENT_NAME: ${{ github.event_name }} + DISPATCH_ACTOR: ${{ github.triggering_actor }} + DISPATCH_SENDER: ${{ github.event.sender.login || '' }} + ALLOWED_DISPATCH_ACTOR: ${{ vars.OPENCODE_REPOSITORY_DISPATCH_ACTOR }} + ALLOWED_DISPATCH_TARGETS: ${{ vars.OPENCODE_REPOSITORY_DISPATCH_TARGETS }} + TARGET_REPOSITORY: ${{ github.event.client_payload.target_repository }} + PR_NUMBER: ${{ github.event.client_payload.pr_number }} + SUPPLIED_BASE_REF: ${{ github.event.client_payload.pr_base_ref || '' }} + SUPPLIED_BASE_SHA: ${{ github.event.client_payload.pr_base_sha || '' }} + SUPPLIED_HEAD_REF: ${{ github.event.client_payload.pr_head_ref || '' }} + SUPPLIED_HEAD_SHA: ${{ github.event.client_payload.pr_head_sha || '' }} + run: | + set -euo pipefail + [ "$EVENT_NAME" = "repository_dispatch" ] || { + echo "::error::Central OpenCode admission accepts repository_dispatch only." + exit 1 + } + + actor_allowed=0 + IFS=',' read -r -a allowed_dispatch_actors <<<"$ALLOWED_DISPATCH_ACTOR" + for allowed_actor in "${allowed_dispatch_actors[@]}"; do + allowed_actor="${allowed_actor//[[:space:]]/}" + if [ -n "$allowed_actor" ] && + [ "$DISPATCH_ACTOR" = "$allowed_actor" ] && + [ "$DISPATCH_SENDER" = "$allowed_actor" ]; then + actor_allowed=1 + break + fi + done + if [ "$actor_allowed" -ne 1 ]; then + printf '::error::repository_dispatch authorization rejected actor=%s sender=%s because both must match one configured scheduler identity.\n' "${DISPATCH_ACTOR:-}" "${DISPATCH_SENDER:-}" + exit 1 + fi + + target_allowed=0 + IFS=',' read -r -a allowed_dispatch_targets <<<"$ALLOWED_DISPATCH_TARGETS" + for allowed_target in "${allowed_dispatch_targets[@]}"; do + allowed_target="${allowed_target//[[:space:]]/}" + if [ -n "$allowed_target" ] && [ "$TARGET_REPOSITORY" = "$allowed_target" ]; then + target_allowed=1 + break + fi + done + if [ "$target_allowed" -ne 1 ]; then + printf '::error::repository_dispatch authorization rejected target=%s because it is absent from the configured exact repository allowlist.\n' "${TARGET_REPOSITORY:-}" + exit 1 + fi + + if ! [[ "$TARGET_REPOSITORY" =~ ^ContextualWisdomLab/[A-Za-z0-9_.-]+$ ]] || + ! [[ "$PR_NUMBER" =~ ^[1-9][0-9]*$ ]] || + [ -z "$SUPPLIED_BASE_REF" ] || + [ -z "$SUPPLIED_HEAD_REF" ] || + ! [[ "$SUPPLIED_BASE_SHA" =~ ^[0-9a-fA-F]{40}$ ]] || + ! [[ "$SUPPLIED_HEAD_SHA" =~ ^[0-9a-fA-F]{40}$ ]]; then + printf '::error::repository_dispatch admission rejected malformed PR identity metadata. target=%s pr=%s\n' "${TARGET_REPOSITORY:-}" "${PR_NUMBER:-}" + exit 1 + fi + printf 'Authorized exact repository_dispatch envelope for %s#%s.\n' "$TARGET_REPOSITORY" "$PR_NUMBER" + + - name: Exchange OpenCode app token for target repository metadata reads + id: metadata_read_app_token + if: >- + github.event_name == 'repository_dispatch' + && github.event.client_payload.target_repository != '' + && github.event.client_payload.target_repository != github.repository + env: + OIDC_AUDIENCE: opencode-github-action + OPENCODE_API_BASE_URL: https://api.opencode.ai + run: | + set -euo pipefail + + mark_unavailable() { + echo "available=false" >>"$GITHUB_OUTPUT" + } + + if [ -z "${ACTIONS_ID_TOKEN_REQUEST_TOKEN:-}" ] || + [ -z "${ACTIONS_ID_TOKEN_REQUEST_URL:-}" ]; then + echo "OpenCode app token exchange unavailable: OIDC request environment is missing." + mark_unavailable + exit 0 + fi + + request_url="${ACTIONS_ID_TOKEN_REQUEST_URL}" + separator="&" + case "$request_url" in + *\?*) ;; + *) separator="?" ;; + esac + + if ! oidc_response="$( + curl -fsS \ + -H "Authorization: Bearer ${ACTIONS_ID_TOKEN_REQUEST_TOKEN}" \ + "${request_url}${separator}audience=${OIDC_AUDIENCE}" + )"; then + echo "OpenCode app token exchange unavailable: OIDC token request did not complete." + mark_unavailable + exit 0 + fi + + oidc_token="$(jq -r '.value // empty' <<<"$oidc_response")" + if [ -z "$oidc_token" ]; then + echo "OpenCode app token exchange unavailable: OIDC token response was empty." + mark_unavailable + exit 0 + fi + + if ! token_response="$( + curl -fsS \ + -X POST \ + -H "Authorization: Bearer ${oidc_token}" \ + "${OPENCODE_API_BASE_URL}/exchange_github_app_token" + )"; then + echo "OpenCode app token exchange unavailable: app token request did not complete." + mark_unavailable + exit 0 + fi + + app_token="$(jq -r '.token // empty' <<<"$token_response")" + if [ -z "$app_token" ]; then + echo "OpenCode app token exchange unavailable: app token response was empty." + mark_unavailable + exit 0 + fi + + echo "::add-mask::$app_token" + { + echo "available=true" + echo "token=$app_token" + } >>"$GITHUB_OUTPUT" + + + + - name: Admit one exact-head central dispatch + id: single_flight + env: + GH_TOKEN: ${{ github.token }} + TARGET_READ_TOKEN: ${{ steps.metadata_read_app_token.outputs.token || secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN || github.token }} + TARGET_REPOSITORY: ${{ github.event.client_payload.target_repository }} + PR_NUMBER: ${{ github.event.client_payload.pr_number }} + HEAD_SHA: ${{ github.event.client_payload.pr_head_sha }} + SUPPLIED_BASE_REF: ${{ github.event.client_payload.pr_base_ref }} + SUPPLIED_BASE_SHA: ${{ github.event.client_payload.pr_base_sha }} + SUPPLIED_HEAD_REF: ${{ github.event.client_payload.pr_head_ref }} + SUPPLIED_HEAD_SHA: ${{ github.event.client_payload.pr_head_sha }} + run: | + set -euo pipefail + exact_title="OpenCode Review Dispatch ${TARGET_REPOSITORY}#${PR_NUMBER}@${HEAD_SHA}" + lease_branch="opencode-dispatch-leases" + lease_key="$(printf '%s#%s' "$TARGET_REPOSITORY" "$PR_NUMBER" | sha256sum | cut -d' ' -f1)" + lease_path="opencode-dispatch-leases/${lease_key}.json" + lease_ref_api="repos/ContextualWisdomLab/.github/git/ref/heads/${lease_branch}" + + live_authority_matches() { + local live_pr live_base_repo live_base_ref live_base_sha + local live_head_repo live_head_ref live_head_sha live_draft live_state + live_pr="$(GH_TOKEN="$TARGET_READ_TOKEN" gh api \ + "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}")" || return 1 + live_base_repo="$(jq -r '.base.repo.full_name // empty' <<<"$live_pr")" + live_base_ref="$(jq -r '.base.ref // empty' <<<"$live_pr")" + live_base_sha="$(jq -r '.base.sha // empty' <<<"$live_pr")" + live_head_repo="$(jq -r '.head.repo.full_name // empty' <<<"$live_pr")" + live_head_ref="$(jq -r '.head.ref // empty' <<<"$live_pr")" + live_head_sha="$(jq -r '.head.sha // empty' <<<"$live_pr")" + live_draft="$(jq -r \ + 'if (.draft | type) == "boolean" then (.draft | tostring) else empty end' \ + <<<"$live_pr")" + live_state="$(jq -r \ + 'if (.state | type) == "string" then .state else empty end' \ + <<<"$live_pr")" + [ "$live_state" = "open" ] && [ "$live_draft" = "false" ] && + [ "$live_base_repo" = "$TARGET_REPOSITORY" ] && + [[ "$live_head_repo" =~ ^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ ]] && + [ "$live_base_ref" = "$SUPPLIED_BASE_REF" ] && + [ "$live_base_sha" = "$SUPPLIED_BASE_SHA" ] && + [ "$live_head_ref" = "$SUPPLIED_HEAD_REF" ] && + [ "${live_head_sha,,}" = "${SUPPLIED_HEAD_SHA,,}" ] && + [ "${live_head_sha,,}" = "${HEAD_SHA,,}" ] + } + if ! live_authority_matches; then + echo "::error::Pull request authority changed before atomic OpenCode admission." + exit 1 + fi + + validate_lease_ref() { + jq -e ' + .object.type == "commit" + and (.object.sha | type) == "string" + and (.object.sha | test("^[0-9a-f]{40}$")) + ' >/dev/null + } + if ! lease_ref="$(gh api "$lease_ref_api" 2>/dev/null)"; then + if ! jq -cn --arg ref "refs/heads/${lease_branch}" --arg sha "$GITHUB_SHA" \ + '{ref:$ref,sha:$sha}' | + gh api --method POST \ + repos/ContextualWisdomLab/.github/git/refs --input - >/dev/null; then + lease_ref="$(gh api "$lease_ref_api")" || { + echo "::error::Could not initialize or read the central OpenCode lease branch." + exit 1 + } + printf '%s' "$lease_ref" | validate_lease_ref || { + echo "::error::Central OpenCode lease branch identity was malformed." + exit 1 + } + fi + else + printf '%s' "$lease_ref" | validate_lease_ref || { + echo "::error::Central OpenCode lease branch identity was malformed." + exit 1 + } + fi + + lease_content="$(jq -cn \ + --argjson owner_run_id "$GITHUB_RUN_ID" \ + --arg exact_title "$exact_title" \ + --arg head_sha "$HEAD_SHA" \ + '{owner_run_id:$owner_run_id,exact_title:$exact_title,head_sha:$head_sha}')" + encoded_lease="$(printf '%s' "$lease_content" | base64 | tr -d '\n')" + for lease_attempt in 1 2 3 4 5; do + current_blob_sha="" + if current_file="$(gh api \ + "repos/ContextualWisdomLab/.github/contents/${lease_path}?ref=${lease_branch}" \ + 2>/dev/null)"; then + if ! current_blob_sha="$(jq -er ' + select(.encoding == "base64") + | .sha + | select(type == "string" and test("^[0-9a-f]{40}$")) + ' <<<"$current_file")"; then + echo "::error::Central OpenCode lease metadata was malformed." + exit 1 + fi + if ! current_content="$(jq -er '.content' <<<"$current_file" | base64 --decode)"; then + echo "::error::Central OpenCode lease content was not valid base64." + exit 1 + fi + if ! owner_run_id="$(jq -er ' + .owner_run_id | select(type == "number" and . > 0) + ' <<<"$current_content")" || + ! owner_title="$(jq -er \ + --arg prefix "OpenCode Review Dispatch ${TARGET_REPOSITORY}#${PR_NUMBER}@" ' + .exact_title + | select( + type == "string" + and ((ascii_downcase) | startswith($prefix | ascii_downcase)) + ) + ' <<<"$current_content")" || + ! owner_head="$(jq -er \ + '.head_sha | select(type == "string" and test("^[0-9a-fA-F]{40}$"))' \ + <<<"$current_content")"; then + echo "::error::Central OpenCode lease identity was malformed." + exit 1 + fi + expected_owner_title="OpenCode Review Dispatch ${TARGET_REPOSITORY}#${PR_NUMBER}@${owner_head}" + if [ "${owner_title,,}" != "${expected_owner_title,,}" ]; then + echo "::error::Central OpenCode lease title and head identity disagreed." + exit 1 + fi + if [ "$owner_run_id" = "$GITHUB_RUN_ID" ] && + [ "${owner_head,,}" = "${HEAD_SHA,,}" ]; then + echo "admitted=true" >>"$GITHUB_OUTPUT" + echo "Central exact-head rerun ${GITHUB_RUN_ID} retained its canonical lease." + exit 0 + fi + if ! owner_run="$(gh api \ + "repos/ContextualWisdomLab/.github/actions/runs/${owner_run_id}")"; then + echo "::error::Could not validate the central OpenCode lease owner." + exit 1 + fi + if ! owner_status="$(jq -er \ + --argjson owner "$owner_run_id" --arg title "$owner_title" ' + select( + .id == $owner + and .path == ".github/workflows/opencode-review-dispatch.yml" + and .event == "repository_dispatch" + and (.display_title | ascii_downcase) == ($title | ascii_downcase) + and (.status | type) == "string" + ) + | .status + ' <<<"$owner_run")"; then + echo "::error::Central OpenCode lease owner identity was malformed." + exit 1 + fi + case "$owner_status" in + requested|waiting|pending|queued|in_progress|completed) ;; + *) + echo "::error::Central OpenCode lease owner returned invalid status ${owner_status:-}." + exit 1 + ;; + esac + if [ "${owner_head,,}" = "${HEAD_SHA,,}" ] && + [ "$owner_status" != "completed" ]; then + echo "admitted=false" >>"$GITHUB_OUTPUT" + echo "Central exact-head dispatch ${GITHUB_RUN_ID} deferred to active lease owner ${owner_run_id}." + exit 0 + fi + fi + + lease_request="$(jq -cn \ + --arg message "Acquire OpenCode exact-head lease for run ${GITHUB_RUN_ID}" \ + --arg content "$encoded_lease" \ + --arg branch "$lease_branch" \ + --arg sha "$current_blob_sha" ' + {message:$message,content:$content,branch:$branch} + + if $sha == "" then {} else {sha:$sha} end + ')" + if ! live_authority_matches; then + echo "::error::Pull request authority changed before the atomic OpenCode lease mutation." + exit 1 + fi + if lease_result="$(printf '%s' "$lease_request" | gh api --method PUT \ + "repos/ContextualWisdomLab/.github/contents/${lease_path}" --input - \ + 2>/dev/null)"; then + if ! lease_blob_sha="$(jq -er ' + .content.sha + | select(type == "string" and test("^[0-9a-f]{40}$")) + ' <<<"$lease_result")"; then + echo "::error::Central OpenCode lease acquisition returned malformed metadata." + exit 1 + fi + { + echo "admitted=true" + } >>"$GITHUB_OUTPUT" + echo "Central exact-head dispatch ${GITHUB_RUN_ID} acquired the atomic lease." + exit 0 + fi + echo "Central exact-head lease changed during attempt ${lease_attempt}; retrying from the authoritative branch." + done + echo "::error::Could not acquire the atomic central OpenCode lease after bounded retries." + exit 1 + + validate-pr-metadata: name: validate-pr-metadata + needs: [admit-exact-head-dispatch] # Folded together with the former coverage-source-tree job (2026-09-17): # both jobs only ever exchanged the OpenCode app token for READ-scoped # data (target-repository metadata, then the PR merge tree) and neither @@ -52,7 +378,10 @@ jobs: # for the measurement (run 34931908846: 21 minutes of job execution # inside a 13h57m run, ~97.5% of which was queue wait between exactly # these job boundaries). - if: github.event_name == 'repository_dispatch' + if: >- + needs.admit-exact-head-dispatch.result == 'success' + && needs.admit-exact-head-dispatch.outputs.admitted == 'true' + && github.event_name == 'repository_dispatch' runs-on: group: CWL central OpenCode labels: [self-hosted, linux, x64] @@ -392,8 +721,11 @@ jobs: coverage-evidence: name: coverage-evidence - needs: [validate-pr-metadata] + needs: [admit-exact-head-dispatch, validate-pr-metadata] if: >- + needs.admit-exact-head-dispatch.result == 'success' + && needs.admit-exact-head-dispatch.outputs.admitted == 'true' + && needs.validate-pr-metadata.result == 'success' && github.event_name == 'repository_dispatch' runs-on: @@ -2517,18 +2849,14 @@ jobs: opencode-review-target: name: opencode-review - needs: [validate-pr-metadata, coverage-evidence] + needs: [admit-exact-head-dispatch, validate-pr-metadata, coverage-evidence] if: >- always() + && needs.admit-exact-head-dispatch.result == 'success' + && needs.admit-exact-head-dispatch.outputs.admitted == 'true' && needs.validate-pr-metadata.result == 'success' && needs.coverage-evidence.result != 'cancelled' && github.event_name == 'repository_dispatch' - concurrency: - group: >- - opencode-review-${{ - needs.validate-pr-metadata.outputs.target_repository }}-${{ - needs.validate-pr-metadata.outputs.pr_number || github.run_id }} - cancel-in-progress: true runs-on: group: CWL central OpenCode labels: [self-hosted, linux, x64] @@ -7793,19 +8121,18 @@ jobs: --head-sha "$PR_HEAD_SHA" \ "${draft_args[@]}" - - name: Wake exact-head required OpenCode workflow + - name: Wake every failed exact-head Required OpenCode workflow if: >- always() && github.event_name == 'repository_dispatch' && steps.formal_review_receipt.outcome == 'success' && needs.validate-pr-metadata.outputs.target_repository != '' && needs.validate-pr-metadata.outputs.head_sha != '' - && github.event.client_payload.required_run_id != '' env: GH_TOKEN: ${{ needs.validate-pr-metadata.outputs.target_repository == github.repository && github.token || secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN }} GH_REPOSITORY: ${{ needs.validate-pr-metadata.outputs.target_repository }} + PR_NUMBER: ${{ needs.validate-pr-metadata.outputs.pr_number }} PR_HEAD_SHA: ${{ needs.validate-pr-metadata.outputs.head_sha }} - REQUIRED_RUN_ID: ${{ github.event.client_payload.required_run_id }} WAKE_TOKEN_SOURCE: ${{ needs.validate-pr-metadata.outputs.target_repository == github.repository && 'github-token' || secrets.PR_REVIEW_MERGE_TOKEN != '' && 'PR_REVIEW_MERGE_TOKEN' || secrets.OPENCODE_APPROVE_TOKEN != '' && 'OPENCODE_APPROVE_TOKEN' || 'unavailable' }} run: | set -euo pipefail @@ -7813,39 +8140,152 @@ jobs: echo "::error::Actions-capable wake credential is unavailable. Native runs use github.token; sibling runs require PR_REVIEW_MERGE_TOKEN or OPENCODE_APPROVE_TOKEN." exit 1 fi - [[ "$REQUIRED_RUN_ID" =~ ^[1-9][0-9]*$ ]] || { - echo "::error::Required OpenCode run id is missing or non-canonical." + live_authority_matches() { + local live_pr live_state live_draft live_head live_created_at + live_pr="$(gh api "repos/${GH_REPOSITORY}/pulls/${PR_NUMBER}")" || return 1 + live_state="$(jq -r '.state // empty' <<<"$live_pr")" + live_draft="$(jq -r 'if (.draft | type) == "boolean" then (.draft | tostring) else empty end' <<<"$live_pr")" + live_head="$(jq -r '.head.sha // empty' <<<"$live_pr")" + live_created_at="$(jq -r '.created_at // empty' <<<"$live_pr")" + [ "$live_state" = "open" ] && [ "$live_draft" = "false" ] && + [ "${live_head,,}" = "${PR_HEAD_SHA,,}" ] && + [[ "$live_created_at" =~ ^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}Z$ ]] || + return 1 + LIVE_PR_CREATED_AT="$live_created_at" + } + if ! live_authority_matches; then + echo "::error::Pull request authority changed before exact-head Required OpenCode wake inventory." exit 1 + fi + inventory_start="$LIVE_PR_CREATED_AT" + collect_required_run_pages() { + local range_start range_end range_total range_probe + local start_epoch end_epoch midpoint_epoch midpoint_time right_start + local probe_url page_url page_output raw_count expected_total=0 + local range_index=0 + local -a range_starts=("$inventory_start") + local -a range_ends=("$(date -u +%Y-%m-%dT%H:%M:%SZ)") + local inventory_file + inventory_file="$(mktemp)" + while [ "$range_index" -lt "${#range_starts[@]}" ]; do + range_start="${range_starts[$range_index]}" + range_end="${range_ends[$range_index]}" + range_index=$((range_index + 1)) + probe_url="repos/${GH_REPOSITORY}/actions/runs?event=pull_request_target&created=${range_start}..${range_end}&per_page=1" + if ! range_probe="$(gh api "$probe_url")" || + ! range_total="$(jq -er ' + .total_count + | if type == "number" and floor == . and . >= 0 then tostring + elif . == "2,500+" then "overflow" + else error("invalid total_count") + end + ' <<<"$range_probe")"; then + echo "::error::Could not validate a bounded Required OpenCode run interval." >&2 + rm -f "$inventory_file" + return 1 + fi + if [ "$range_total" = "overflow" ] || [ "$range_total" -gt 1000 ]; then + start_epoch="$(date -u -d "$range_start" +%s)" || return 1 + end_epoch="$(date -u -d "$range_end" +%s)" || return 1 + if [ "$start_epoch" -ge "$end_epoch" ]; then + echo "::error::More than 1,000 pull_request_target runs share one second; exact inventory cannot be proven." >&2 + rm -f "$inventory_file" + return 1 + fi + midpoint_epoch=$(((start_epoch + end_epoch) / 2)) + midpoint_time="$(date -u -d "@${midpoint_epoch}" +%Y-%m-%dT%H:%M:%SZ)" + right_start="$(date -u -d "@$((midpoint_epoch + 1))" +%Y-%m-%dT%H:%M:%SZ)" + range_starts+=("$range_start" "$right_start") + range_ends+=("$midpoint_time" "$range_end") + continue + fi + page_url="repos/${GH_REPOSITORY}/actions/runs?event=pull_request_target&created=${range_start}..${range_end}&per_page=100" + if ! page_output="$(gh api --paginate "$page_url")"; then + echo "::error::Could not list a bounded Required OpenCode run interval." >&2 + rm -f "$inventory_file" + return 1 + fi + printf '%s\n' "$page_output" >>"$inventory_file" + expected_total=$((expected_total + range_total)) + done + if ! raw_count="$(jq -e -s '[.[] | .workflow_runs[]] | length' "$inventory_file")" || + [ "$raw_count" -ne "$expected_total" ]; then + printf '::error::Required OpenCode inventory was truncated: expected=%s collected=%s.\n' "$expected_total" "${raw_count:-invalid}" >&2 + rm -f "$inventory_file" + return 1 + fi + cat "$inventory_file" + rm -f "$inventory_file" } - # The immutable run id is scoped to GH_REPOSITORY. Revalidate its - # event, central workflow path, and live PR head before rerunning it; - # rendered titles and workflow_url differ between native and - # organization-required workflow contexts. for attempt in $(seq 1 12); do - run="$(gh api "repos/${GH_REPOSITORY}/actions/runs/${REQUIRED_RUN_ID}")" - required_run="$(printf '%s\n' "$run" | jq -r --arg head "$PR_HEAD_SHA" --argjson run_id "$REQUIRED_RUN_ID" ' - select(.id == $run_id) - | select(.event == "pull_request_target") - | select(.path == ".github/workflows/opencode-review.yml") - | select(.head_sha == $head) - | [(.id // ""), (.status // ""), (.conclusion // "")] - | @tsv - ')" - IFS=$'\t' read -r required_run_id required_status required_conclusion <<<"$required_run" - if [ "$required_status" = "completed" ] && [ "$required_conclusion" = "failure" ]; then - gh api -X POST "repos/${GH_REPOSITORY}/actions/runs/${required_run_id}/rerun-failed-jobs" >/dev/null - echo "Re-ran failed jobs for exact-head Required OpenCode Review run ${required_run_id}." + if ! run_pages="$(collect_required_run_pages)"; then + echo "::error::Could not list exact-head Required OpenCode workflow runs." + exit 1 + fi + if ! run_inventory="$(jq -c -s --arg head "$PR_HEAD_SHA" --argjson pr "$PR_NUMBER" ' + if all(.[]; (.workflow_runs | type) == "array") + and all( + .[] | .workflow_runs[]; + (.id | type) == "number" + and (.event | type) == "string" + and (.path | type) == "string" + and (.head_sha | type) == "string" + and (.status | type) == "string" + and ((.conclusion == null) or (.conclusion | type) == "string") + and (.pull_requests | type) == "array" + and all( + .pull_requests[]; + (.number | type) == "number" + and (.head.sha | type) == "string" + ) + ) + then + [ + .[] | .workflow_runs[] + | select( + .event == "pull_request_target" + and .path == ".github/workflows/opencode-review.yml" + and any( + .pull_requests[]; + .number == $pr + and (.head.sha | ascii_downcase) == ($head | ascii_downcase) + ) + ) + ] | unique_by(.id) + else + error("invalid required-run inventory") + end + ' <<<"$run_pages")"; then + echo "::error::Exact-head Required OpenCode workflow inventory was malformed." + exit 1 + fi + failed_ids="$(jq -r ' + .[] + | select(.status == "completed" and .conclusion == "failure") + | .id + ' <<<"$run_inventory")" + if [ -n "$failed_ids" ]; then + while IFS= read -r failed_id; do + [ -n "$failed_id" ] || continue + if ! live_authority_matches; then + echo "::error::Pull request authority changed before Required OpenCode rerun ${failed_id}." + exit 1 + fi + gh api -X POST \ + "repos/${GH_REPOSITORY}/actions/runs/${failed_id}/rerun-failed-jobs" >/dev/null + echo "Re-ran failed jobs for exact-head Required OpenCode Review run ${failed_id}." + done <<<"$failed_ids" exit 0 fi - if [ "$required_status" = "completed" ] && [ "$required_conclusion" = "success" ]; then - echo "Exact-head Required OpenCode Review run ${required_run_id} already succeeded." + if jq -e 'any(.[]; .status == "completed" and .conclusion == "success")' <<<"$run_inventory" >/dev/null; then + echo "An exact-head Required OpenCode Review run already succeeded." exit 0 fi if [ "$attempt" -lt 12 ]; then sleep 5 fi done - echo "::error::Formal OpenCode receipt exists, but the exact-head required workflow did not reach a rerunnable failed state." + echo "::error::Formal OpenCode receipt exists, but no exact-head required workflow reached a rerunnable failed or successful state." exit 1 - name: Publish repository_dispatch OpenCode status diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index bbe692b417..d4db5efa9c 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -501,6 +501,116 @@ jobs: exit 1 fi echo "::add-mask::$app_token" + + # GitHub native concurrency replaces an older pending group member + # even when cancel-in-progress is false, so the receiver deliberately + # has no lossy concurrency group. Avoid duplicate work here instead: + # inventory every active admission state twice so a transition cannot + # disappear between status queries, and collect older-head central + # runs for exact-identity retirement before posting the current head. + active_dispatch_title="OpenCode Review Dispatch ${TARGET_REPOSITORY}#${PR_NUMBER}@${HEAD_SHA}" + active_dispatch_prefix="OpenCode Review Dispatch ${TARGET_REPOSITORY}#${PR_NUMBER}@" + stale_dispatches_file="$(mktemp)" + same_head_found=false + trap 'rm -f "$helper" "$stale_dispatches_file"' EXIT + for inventory_pass in 1 2; do + for active_status in requested waiting pending queued in_progress; do + runs_url="repos/ContextualWisdomLab/.github/actions/workflows/opencode-review-dispatch.yml/runs?event=repository_dispatch&status=${active_status}&per_page=100" + if ! active_runs="$(GH_TOKEN="$app_token" gh api --paginate "$runs_url")"; then + echo "::error::Could not inspect active OpenCode dispatches; refusing a duplicate scheduler wake." + exit 1 + fi + if ! same_head_active="$(jq -r -s --arg title "$active_dispatch_title" --arg status "$active_status" ' + if all(.[]; (.workflow_runs | type) == "array") + and all( + .[] | .workflow_runs[]; + (.id | type) == "number" + and .path == ".github/workflows/opencode-review-dispatch.yml" + and .event == "repository_dispatch" + and .status == $status + and (.display_title | type) == "string" + ) + then + any( + .[] | .workflow_runs[]; + (.display_title | ascii_downcase) == ($title | ascii_downcase) + ) + else + error("invalid workflow-runs response") + end + ' <<<"$active_runs")"; then + echo "::error::Active OpenCode dispatch data was invalid; refusing a duplicate scheduler wake." + exit 1 + fi + case "$same_head_active" in + true) same_head_found=true ;; + false) ;; + *) + echo "::error::Active OpenCode dispatch guard returned an invalid state." + exit 1 + ;; + esac + jq -r -s --arg prefix "$active_dispatch_prefix" --arg title "$active_dispatch_title" ' + .[] | .workflow_runs[] + | select((.display_title | ascii_downcase) | startswith($prefix | ascii_downcase)) + | select((.display_title | ascii_downcase) != ($title | ascii_downcase)) + | .id + ' <<<"$active_runs" >>"$stale_dispatches_file" + done + done + + live_authority_matches() { + local live_pr live_head live_draft live_state + live_pr="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}")" || return 1 + live_head="$(printf '%s' "$live_pr" | jq -r '.head.sha // empty')" + live_draft="$(printf '%s' "$live_pr" | jq -r 'if (.draft | type) == "boolean" then (.draft | tostring) else empty end')" + live_state="$(printf '%s' "$live_pr" | jq -r 'if (.state | type) == "string" then .state else empty end')" + [ "$live_state" = "open" ] && [ "$live_draft" = "false" ] && + [ "${live_head,,}" = "${HEAD_SHA,,}" ] + } + if ! live_authority_matches; then + echo "Pull request authority changed before OpenCode dispatch; scheduler wake skipped." + exit 0 + fi + + while IFS= read -r stale_run_id; do + [ -n "$stale_run_id" ] || continue + if ! live_authority_matches; then + echo "Pull request authority changed before stale OpenCode retirement; scheduler wake skipped." + exit 0 + fi + if ! GH_TOKEN="$app_token" gh api --method POST \ + "repos/ContextualWisdomLab/.github/actions/runs/${stale_run_id}/cancel" >/dev/null; then + echo "::error::Could not cancel superseded central OpenCode dispatch ${stale_run_id}; refusing to add current-head work behind it." + exit 1 + fi + if ! stale_status="$(GH_TOKEN="$app_token" gh api \ + "repos/ContextualWisdomLab/.github/actions/runs/${stale_run_id}" --jq '.status')"; then + echo "::error::Could not verify superseded central OpenCode dispatch ${stale_run_id} cancellation." + exit 1 + fi + case "$stale_status" in + requested|waiting|pending|queued|in_progress) + echo "Superseded central OpenCode dispatch ${stale_run_id} cancellation is still ${stale_status}; current-head admission continues without waiting." + ;; + completed) ;; + *) + echo "::error::Superseded central OpenCode dispatch ${stale_run_id} returned invalid status ${stale_status:-}." + exit 1 + ;; + esac + echo "Cancellation accepted for superseded central OpenCode dispatch ${stale_run_id} after live exact-head revalidation." + done < <(sort -nu "$stale_dispatches_file") + + if [ "$same_head_found" = "true" ]; then + echo "An exact-head OpenCode dispatch is already active; scheduler wake skipped after older-head retirement." + exit 0 + fi + + if ! live_authority_matches; then + echo "Pull request authority changed before OpenCode dispatch; scheduler wake skipped." + exit 0 + fi jq -cn \ --arg target_repository "$TARGET_REPOSITORY" \ --arg pr_number "$PR_NUMBER" \ diff --git a/CHANGELOG.d/20261001-opencode-same-head-dispatch-idempotency.md b/CHANGELOG.d/20261001-opencode-same-head-dispatch-idempotency.md new file mode 100644 index 0000000000..5b3f230417 --- /dev/null +++ b/CHANGELOG.d/20261001-opencode-same-head-dispatch-idempotency.md @@ -0,0 +1,7 @@ +### Fixed + +- Preserve any active exact-head OpenCode dispatch across all GitHub admission + states when the required-check wake path is retried, fail closed on ambiguous + inventory, avoid GitHub's lossy pending concurrency replacement, retire only + live-head-verified older central runs with completion evidence, and revalidate + pull-request authority immediately before a new dispatch. diff --git a/CHANGELOG.md b/CHANGELOG.md index 2dad14d146..615f0a5cf3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,27 @@ +### OpenCode preserves exact-head queue position + +- Remove the central receiver's lossy native concurrency group after live + evidence showed a later same-head wake cancelling the queued authoritative + run. The required wake now inventories all five active states twice, retires + only identity-validated older-head runs after live authority checks, and + continues current-head admission when GitHub has accepted but not completed + an asynchronous cancellation. The receiver atomically compare-and-swaps one + repository/PR lease file on a dedicated central branch before source + materialization, coverage, or model execution, closing the cross-producer + check-then-POST race without lossy native concurrency. A dedicated minimal + admission job owns the central `contents: write` grant, rejects unauthorized + or malformed envelopes before OIDC exchange, and revalidates the complete + live state/draft/base/head identity immediately before each compare-and-swap; + metadata and source jobs remain read-only. Self-reruns retain their lease and + different-head takeovers validate the recorded owner. After formal receipt, + the publisher inventories repository-wide runs, binds the intended PR head + through `pull_requests[]` rather than the trusted-base run-level SHA, + recursively partitions the PR-lifetime `created` range below GitHub's + 1,000-result filtered-search ceiling, rejects `total_count`/collection + mismatches, revalidates live authority before each POST, and reruns every + matching failed Required OpenCode job. A losing duplicate therefore needs + neither a callback payload nor a polling runner. + ### Shared Strix lock advances beyond the PyJWT recursion DoS - Advance the explicit Strix source pin and generated hash lock from PyJWT diff --git a/docs/doctoring/opencode-same-head-dispatch-idempotency.md b/docs/doctoring/opencode-same-head-dispatch-idempotency.md new file mode 100644 index 0000000000..709dfe27f0 --- /dev/null +++ b/docs/doctoring/opencode-same-head-dispatch-idempotency.md @@ -0,0 +1,125 @@ +# OpenCode exact-head dispatch idempotency + +## Incident + +On 2026-09-30, `ContextualWisdomLab/.github#2545` remained on exact head +`9a4af5e438283a31dc05814d6bc2818caee782a3` while the required OpenCode wake +path requested review execution more than once. Central dispatch run +[`36776536447`](https://github.com/ContextualWisdomLab/.github/actions/runs/36776536447) +was already queued for that exact repository, PR, and head. A later same-head +request created run +[`36778773766`](https://github.com/ContextualWisdomLab/.github/actions/runs/36778773766), +and the receiver's PR-keyed `cancel-in-progress: true` concurrency retired the +older run. No PR head change or substantive review result justified losing its +queue position. + +The required workflow correctly checked for an existing current-head formal +review, but, when that receipt was absent, posted `repository_dispatch` +unconditionally. The merge scheduler already deduplicated active same-head +OpenCode runs; this direct required-check path bypassed that guard. + +## Repair contract + +Before posting a new dispatch, the trusted required-check path now: + +1. validates that the target PR is live, open, ready, and still on the event + head; +2. evaluates the existing formal-review receipt predicate; +3. exchanges the existing repository-scoped OpenCode App token; +4. inventories `requested`, `waiting`, `pending`, `queued`, and `in_progress` + runs of the canonical `opencode-review-dispatch.yml` receiver twice, so a + state transition during the inventory remains observable; +5. validates each returned run's identity fields and matches its protected + exact `repository#PR@head` title plus workflow path and trigger; +6. records an exact-head execution without returning until canonical + older-head executions have been retired; and +7. re-fetches the PR immediately before dispatch, retiring the request if + state, draft status, or head authority changed. + +Run-list failures and malformed run records fail closed. The receiver has no +native concurrency group: GitHub replaces an older pending group member even +when `cancel-in-progress` is false, so native concurrency cannot preserve every +distinct callback payload. Producers deduplicate exact-head work from trusted +inventory. Before a current-head POST, the required workflow revalidates live +repository/PR/head authority and cancels only canonical older-head central +runs. A refused cancellation, failed status lookup, or invalid status fails +closed. GitHub may still report an accepted cancellation as active; that +asynchronous state no longer abandons the current-head dispatch or holds a +runner. A final live-authority read guards the POST. + +Producer observation and POST are not atomic, so the receiver first authorizes +the exact actor/sender pair, repository allowlist membership, and complete +payload shape before any OIDC exchange or central mutation. It then acquires a +central repository-owned lease after full live state/draft/base/head metadata +validation and before source +materialization, coverage, or model execution. The lease uses one file per +repository/PR on the dedicated `opencode-dispatch-leases` branch. GitHub's +Contents API compares the observed blob SHA during update: simultaneous cache +misses or terminal-owner takeovers can commit only one owner, while a loser +reloads the authoritative file and defers to the active exact-head run. A +different-head run may replace an owner only after a fresh live PR/head check +immediately before the compare-and-swap. Every recorded owner is validated +against its canonical workflow, event, title, and head; a rerun with the same +GitHub run ID retains its own lease. The lease file remains as auditable +bounded state and is updated, not multiplied, on later heads. + +The dedicated admission job alone holds `contents: write` for that lease +branch; later metadata validation and source materialization return to +`contents: read`. +The required target-repository job retains read-only Actions and contents +authority; central cancellation uses the existing repository-scoped App token. +After a formal receipt, the privileged publisher uses the repository-wide run +inventory and matches the intended PR number plus +`pull_requests[].head.sha`. It deliberately does not treat the run-level +`head_sha` as the PR head because `pull_request_target` executes trusted +default-branch workflow code. GitHub caps every filtered workflow-run search at +1,000 results, so the publisher recursively bisects the PR-lifetime `created` +range until each interval is within the API bound, then requires the sum of +every interval's `total_count` to equal the collected rows. An interval with +more than 1,000 runs in a single second fails closed. The publisher revalidates +live open/non-draft head authority immediately before every mutation and reruns +each matching failed Required OpenCode job. Therefore a duplicate +producer that lost the lease does not need its own callback payload or a +polling runner. Human review events do not enter or cancel the required +workflow. The repair does not weaken the required verdict, accept predecessor +evidence, or broaden model/provider permissions. + +## Executable evidence + +`tests/test_opencode_required_verdict_regression.py` executes the production +shell step. Its RED fixture proved that a queued exact-head run still produced +a second dispatch. The GREEN cases cover all five active admission states, +transition-safe two-pass inventory, malformed and unavailable inventory +fail-closure, current-head preservation, older-head non-suppression, +no-active-run dispatch, existing formal receipts, and a head movement between +initial validation and the mutation boundary. Queue-contract tests prohibit +lossy native receiver concurrency; execution fixtures prove exact older-head +cancellation, deduplication, asynchronous cancellation continuation, and +fail-closure when a cancellation is refused or returns an invalid status. +Receiver fixtures execute absent, active-owner, terminal-owner, self-rerun, +different-head takeover, and branch-initialization-race lease paths. +A two-process fixture starts simultaneous cache misses against an atomic fake +Contents API and proves exactly one `admitted=true` result. The independent +negative fixtures prove that unauthorized envelopes perform no OIDC or GitHub +API call and that closed, draft, or mismatched base/head metadata performs no +Contents mutation. Wake fixtures use a trusted-base run-level SHA distinct from +the PR head and cover multiple failures, another PR sharing the same commit, +authority movement between mutations, partial rerun failure, recursive +partitioning above 1,000 results, and detected pagination truncation. The +independent byte-for-byte reviewer pin was regenerated from the repaired +receiver as exact Git blob +`10707b070475c5e0889501ae4178b7868c6a7cc9`. + +Local focused verification on the stacked successor base +`5a91ce9f9c3e773aa1172f1055fd791ccde8fdaa`: + +- required-workflow, nonblocking capacity, queue, receiver, and integrity-pin + contracts after independent-review repair: `248 passed, 1 skipped`; +- complete Python 3.14 warnings-fatal suite: `5339 passed, 5 skipped, 40 + subtests passed`, with + owned production `18729/18729` statements and `7642/7642` branches, + Docstring `100%`, and zero warnings. + +Protected merge still requires hosted exact-head security and quality Checks, +zero unresolved review threads, qualifying independent approval, and ordinary +branch protection. diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index f32681a60e..bb7a2c7563 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1,5 +1,11 @@ # Product and Technical Gap Baseline +## 2026-10-01 OpenCode same-head dispatch idempotency + +| Gap | Exact evidence | Action | Status | +|---|---|---|---| +| A repeated required-check wake for unchanged `.github#2545@9a4af5e438283a31dc05814d6bc2818caee782a3` cancelled queued central dispatch `36776536447` when same-head run `36778773766` was created, resetting queue position without new source authority | [Run 36776536447](https://github.com/ContextualWisdomLab/.github/actions/runs/36776536447); [run 36778773766](https://github.com/ContextualWisdomLab/.github/actions/runs/36778773766); protected `main@37b10243cec3d160ecc9c1be75c71428b160a703` | At the central `.github` owner, inventory all five active admission states twice, validate run identity and fail closed on ambiguity, remove lossy native receiver concurrency, retire only canonical older-head central runs after live repository/PR/head validation, and continue current-head admission after an accepted asynchronous cancellation. Because producer inventory and POST are not atomic, authorize actor/sender/target and payload shape before OIDC, then acquire one repository/PR lease through the Contents API's blob-SHA compare-and-swap only after full live state/draft/base/head validation; active same-head losers terminate cheaply, self-reruns and terminal owners recover, and a freshly revalidated live head may replace a fully validated older-head owner. Isolate central `contents: write` in a dedicated lease job and keep later jobs read-only. After formal receipt, use repository-wide run inventory, bind PR number and `pull_requests[].head.sha` rather than the trusted-base run-level SHA, recursively partition the PR-lifetime `created` range below GitHub's 1,000-result ceiling, verify `total_count` completeness, and revalidate before each failed-job rerun so duplicate callbacks are unnecessary. See [RCA and executable acceptance](doctoring/opencode-same-head-dispatch-idempotency.md) | **Proposed / local RED→GREEN complete; exact-head hosted Checks and independent approval required** | + ## 2026-10-01 gap baseline source integrity | Gap | Exact evidence | Action | Status | diff --git a/tests/test_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index 8a80d65461..2e170db815 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -1225,7 +1225,17 @@ def test_opencode_model_exhaustion_retry_stays_owned_by_central_scheduler(): workflow = Path(".github/workflows/opencode-review-dispatch.yml").read_text(encoding="utf-8") assert "opencode-exhausted-retry:" not in workflow assert "RETRY_DISPATCH_TOKEN" not in workflow - assert "contents: write" not in workflow + admission_job = workflow.split(" admit-exact-head-dispatch:\n", 1)[1].split( + "\n validate-pr-metadata:", 1 + )[0] + validation_job = workflow.split(" validate-pr-metadata:\n", 1)[1].split( + "\n coverage-evidence:", 1 + )[0] + review_job = workflow.split(" opencode-review-target:\n", 1)[1] + assert admission_job.count("contents: write") == 1 + assert "opencode-dispatch-leases" in admission_job + assert "contents: write" not in validation_job + assert "contents: write" not in review_job def test_sandbox_git_config_env_trusts_only_the_validated_worktree(tmp_path): @@ -1854,19 +1864,13 @@ def test_workflow_provisions_sandbox_tool_and_reviewer_agent(): assert "run_opencode_review_model_pool.sh" in workflow assert "rekick_model_pool_on_exhaustion" not in workflow assert "publish stage performs no duplicate model-catalog pass" in workflow - # The review job's own group, addressed by its indentation: the workflow - # also carries a workflow-level admission group (pinned in - # tests/test_required_workflow_queue_contract.py), so splitting on the - # first "concurrency:" would read that one instead of this one. - concurrency_contract = workflow.split("\n concurrency:", 1)[1].split( - "\n runs-on:", 1 - )[0] - assert "needs.validate-pr-metadata.outputs.target_repository" in concurrency_contract - assert "needs.validate-pr-metadata.outputs.pr_number || github.run_id" in concurrency_contract - assert "format('pr-{0}-{1}'" not in concurrency_contract - assert "github.event.client_payload.pr_head_sha" not in concurrency_contract - assert "github.event.client_payload.pr_number" not in concurrency_contract - assert "github.event.pull_request" not in concurrency_contract + # GitHub native concurrency replaces an older pending member even with + # cancel-in-progress disabled. Producer inventory and live-head retirement + # own admission, so the receiver intentionally has no concurrency group. + workflow_header = workflow.split("permissions:", 1)[0] + review_job = workflow.split("\n opencode-review-target:\n", 1)[1] + assert not re.search(r"(?m)^concurrency:", workflow_header) + assert not re.search(r"(?m)^ concurrency:", review_job) assert "OPENCODE_MODEL_CANDIDATES" in workflow model_pool_runner = Path("scripts/ci/run_opencode_review_model_pool.sh").read_text( encoding="utf-8" diff --git a/tests/test_opencode_required_rerun_capacity.py b/tests/test_opencode_required_rerun_capacity.py index c85bc24e3c..f2438c46d4 100644 --- a/tests/test_opencode_required_rerun_capacity.py +++ b/tests/test_opencode_required_rerun_capacity.py @@ -7,6 +7,7 @@ import os from pathlib import Path import subprocess +import textwrap from tests.test_opencode_required_verdict_regression import HEAD, fail_closed_script @@ -15,6 +16,28 @@ DISPATCH = Path(".github/workflows/opencode-review-dispatch.yml") +def wake_failed_required_runs_script() -> str: + """Extract the exact production wake step shell.""" + dispatch = DISPATCH.read_text(encoding="utf-8") + step = dispatch.split( + " - name: Wake every failed exact-head Required OpenCode workflow\n", 1 + )[1].split("\n\n - name:", 1)[0] + return textwrap.dedent(step.split(" run: |\n", 1)[1]) + + +def required_run(run_id: int, *, pr_number: int = 7) -> dict[str, object]: + """Build one exact-head required-workflow list record.""" + return { + "id": run_id, + "event": "pull_request_target", + "path": ".github/workflows/opencode-review.yml", + "head_sha": "c" * 40, + "status": "completed", + "conclusion": "failure", + "pull_requests": [{"number": pr_number, "head": {"sha": HEAD}}], + } + + def test_required_job_releases_runner_until_exact_run_wakeup() -> None: required = REQUIRED.read_text(encoding="utf-8") target = required.split(" opencode-review-target:\n", 1)[1].split( @@ -29,18 +52,355 @@ def test_required_job_releases_runner_until_exact_run_wakeup() -> None: assert "will rerun this failed job" in target -def test_dispatch_wakes_only_the_exact_failed_current_head_run() -> None: +def test_dispatch_wakes_every_exact_failed_current_head_run() -> None: dispatch = DISPATCH.read_text(encoding="utf-8") - wake = dispatch.split(" - name: Wake exact-head required OpenCode workflow\n", 1)[1].split( - "\n\n - name:", 1 - )[0] + wake = dispatch.split( + " - name: Wake every failed exact-head Required OpenCode workflow\n", 1 + )[1].split("\n\n - name:", 1)[0] - assert "github.event.client_payload.required_run_id != ''" in wake - assert "select(.id == $run_id)" in wake - assert 'select(.event == "pull_request_target")' in wake - assert 'select(.path == ".github/workflows/opencode-review.yml")' in wake - assert "select(.head_sha == $head)" in wake + assert "github.event.client_payload.required_run_id != ''" not in wake + assert '.event == "pull_request_target"' in wake + assert '.path == ".github/workflows/opencode-review.yml"' in wake + assert "(.head.sha | ascii_downcase) == ($head | ascii_downcase)" in wake + assert ".number == $pr" in wake + assert "unique_by(.id)" in wake assert "rerun-failed-jobs" in wake + assert "actions/workflows/opencode-review.yml/runs" not in wake + assert "actions/runs?event=pull_request_target&created=" in wake + assert "range_total" in wake + assert "expected_total" in wake + assert "live_authority_matches" in wake + + +def test_dispatch_wake_binds_shared_head_runs_to_exact_pull_request( + tmp_path: Path, +) -> None: + """Wake all failures for this PR and none for another PR sharing its head.""" + calls = tmp_path / "calls" + fake_gh = tmp_path / "gh" + inventory = { + "total_count": 3, + "workflow_runs": [ + required_run(41), + required_run(42, pr_number=8), + required_run(43), + ] + } + fake_gh.write_text( + """#!/usr/bin/env bash +set -euo pipefail +printf '%s\n' "$*" >>"$CALLS" +if [[ "$*" == "api repos/owner/repo/pulls/7" ]]; then + printf '%s' "$LIVE_PR" +elif [[ "$*" == *"repos/owner/repo/actions/runs?"* ]]; then + printf '%s' "$RUN_INVENTORY" +elif [[ "$*" == *"rerun-failed-jobs"* ]]; then + exit 0 +else + exit 97 +fi +""", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + result = subprocess.run( + ["bash", "-c", wake_failed_required_runs_script()], + env={ + **os.environ, + "PATH": f"{tmp_path}{os.pathsep}{os.environ['PATH']}", + "CALLS": str(calls), + "GH_TOKEN": "token", + "WAKE_TOKEN_SOURCE": "PR_REVIEW_MERGE_TOKEN", + "GH_REPOSITORY": "owner/repo", + "PR_NUMBER": "7", + "PR_HEAD_SHA": HEAD, + "LIVE_PR": json.dumps( + { + "state": "open", + "draft": False, + "created_at": "2026-09-30T00:00:00Z", + "head": {"sha": HEAD}, + } + ), + "RUN_INVENTORY": json.dumps(inventory), + }, + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 0, result.stderr + reruns = [line for line in calls.read_text().splitlines() if "rerun-failed-jobs" in line] + assert reruns == [ + "api -X POST repos/owner/repo/actions/runs/41/rerun-failed-jobs", + "api -X POST repos/owner/repo/actions/runs/43/rerun-failed-jobs", + ] + assert calls.read_text().count("api repos/owner/repo/pulls/7") == 3 + + +def test_dispatch_wake_stops_when_live_authority_moves_between_mutations( + tmp_path: Path, +) -> None: + """Revalidate the live open head immediately before every rerun POST.""" + calls = tmp_path / "calls" + authority_reads = tmp_path / "authority-reads" + fake_gh = tmp_path / "gh" + fake_gh.write_text( + """#!/usr/bin/env bash +set -euo pipefail +printf '%s\n' "$*" >>"$CALLS" +if [[ "$*" == "api repos/owner/repo/pulls/7" ]]; then + count=0 + [[ ! -f "$AUTHORITY_READS" ]] || count="$(cat "$AUTHORITY_READS")" + count=$((count + 1)) + printf '%s' "$count" >"$AUTHORITY_READS" + if [[ "$count" -lt 3 ]]; then head="$PR_HEAD_SHA"; else head="$(printf 'd%.0s' {1..40})"; fi + jq -cn --arg head "$head" '{state:"open",draft:false,created_at:"2026-09-30T00:00:00Z",head:{sha:$head}}' +elif [[ "$*" == *"repos/owner/repo/actions/runs?"* ]]; then + printf '%s' "$RUN_INVENTORY" +elif [[ "$*" == *"rerun-failed-jobs"* ]]; then + exit 0 +else + exit 97 +fi +""", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + result = subprocess.run( + ["bash", "-c", wake_failed_required_runs_script()], + env={ + **os.environ, + "PATH": f"{tmp_path}{os.pathsep}{os.environ['PATH']}", + "CALLS": str(calls), + "AUTHORITY_READS": str(authority_reads), + "GH_TOKEN": "token", + "WAKE_TOKEN_SOURCE": "PR_REVIEW_MERGE_TOKEN", + "GH_REPOSITORY": "owner/repo", + "PR_NUMBER": "7", + "PR_HEAD_SHA": HEAD, + "RUN_INVENTORY": json.dumps( + { + "total_count": 2, + "workflow_runs": [required_run(41), required_run(43)], + } + ), + }, + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 1 + assert "authority changed before Required OpenCode rerun 43" in result.stdout + reruns = [line for line in calls.read_text().splitlines() if "rerun-failed-jobs" in line] + assert reruns == ["api -X POST repos/owner/repo/actions/runs/41/rerun-failed-jobs"] + + +def test_dispatch_wake_reports_partial_rerun_api_failure(tmp_path: Path) -> None: + """A later rerun failure must fail the publisher after recording prior work.""" + calls = tmp_path / "calls" + fake_gh = tmp_path / "gh" + fake_gh.write_text( + """#!/usr/bin/env bash +set -euo pipefail +printf '%s\n' "$*" >>"$CALLS" +if [[ "$*" == "api repos/owner/repo/pulls/7" ]]; then + jq -cn --arg head "$PR_HEAD_SHA" '{state:"open",draft:false,created_at:"2026-09-30T00:00:00Z",head:{sha:$head}}' +elif [[ "$*" == *"repos/owner/repo/actions/runs?"* ]]; then + printf '%s' "$RUN_INVENTORY" +elif [[ "$*" == *"actions/runs/41/rerun-failed-jobs"* ]]; then + exit 0 +elif [[ "$*" == *"actions/runs/43/rerun-failed-jobs"* ]]; then + exit 22 +else + exit 97 +fi +""", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + result = subprocess.run( + ["bash", "-c", wake_failed_required_runs_script()], + env={ + **os.environ, + "PATH": f"{tmp_path}{os.pathsep}{os.environ['PATH']}", + "CALLS": str(calls), + "GH_TOKEN": "token", + "WAKE_TOKEN_SOURCE": "PR_REVIEW_MERGE_TOKEN", + "GH_REPOSITORY": "owner/repo", + "PR_NUMBER": "7", + "PR_HEAD_SHA": HEAD, + "RUN_INVENTORY": json.dumps( + { + "total_count": 2, + "workflow_runs": [required_run(41), required_run(43)], + } + ), + }, + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 22 + reruns = [line for line in calls.read_text().splitlines() if "rerun-failed-jobs" in line] + assert reruns == [ + "api -X POST repos/owner/repo/actions/runs/41/rerun-failed-jobs", + "api -X POST repos/owner/repo/actions/runs/43/rerun-failed-jobs", + ] + + +def test_dispatch_wake_partitions_inventory_above_github_search_cap( + tmp_path: Path, +) -> None: + """Bisect GitHub's overflow sentinel before collecting bounded pages.""" + calls = tmp_path / "calls" + probe_count = tmp_path / "probe-count" + page_count = tmp_path / "page-count" + fake_gh = tmp_path / "gh" + fake_gh.write_text( + """#!/usr/bin/env bash +set -euo pipefail +printf '%s\n' "$*" >>"$CALLS" +if [[ "$*" == "api repos/owner/repo/pulls/7" ]]; then + jq -cn --arg head "$PR_HEAD_SHA" '{state:"open",draft:false,created_at:"2026-09-30T00:00:00Z",head:{sha:$head}}' +elif [[ "$*" == *"actions/runs?"* && "$*" == *"per_page=1" ]]; then + count=0 + [[ ! -f "$PROBE_COUNT" ]] || count="$(cat "$PROBE_COUNT")" + count=$((count + 1)) + printf '%s' "$count" >"$PROBE_COUNT" + case "$count" in + 1) printf '%s' '{"total_count":"2,500+","workflow_runs":[]}' ;; + 2) printf '%s' '{"total_count":1,"workflow_runs":[]}' ;; + 3) printf '%s' '{"total_count":0,"workflow_runs":[]}' ;; + *) exit 96 ;; + esac +elif [[ "$*" == *"api --paginate"* && "$*" == *"per_page=100"* ]]; then + count=0 + [[ ! -f "$PAGE_COUNT" ]] || count="$(cat "$PAGE_COUNT")" + count=$((count + 1)) + printf '%s' "$count" >"$PAGE_COUNT" + if [[ "$count" -eq 1 ]]; then printf '%s' "$ONE_RUN"; else printf '%s' '{"workflow_runs":[]}'; fi +elif [[ "$*" == *"actions/runs/41/rerun-failed-jobs"* ]]; then + exit 0 +else + exit 97 +fi +""", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + result = subprocess.run( + ["bash", "-c", wake_failed_required_runs_script()], + env={ + **os.environ, + "PATH": f"{tmp_path}{os.pathsep}{os.environ['PATH']}", + "CALLS": str(calls), + "PROBE_COUNT": str(probe_count), + "PAGE_COUNT": str(page_count), + "GH_TOKEN": "token", + "WAKE_TOKEN_SOURCE": "PR_REVIEW_MERGE_TOKEN", + "GH_REPOSITORY": "owner/repo", + "PR_NUMBER": "7", + "PR_HEAD_SHA": HEAD, + "ONE_RUN": json.dumps({"workflow_runs": [required_run(41)]}), + }, + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 0, result.stderr + assert probe_count.read_text() == "3" + assert page_count.read_text() == "2" + assert "actions/runs/41/rerun-failed-jobs" in calls.read_text() + + +def test_dispatch_wake_fails_when_overflow_shares_one_second(tmp_path: Path) -> None: + """A non-partitionable overflow sentinel fails before run selection.""" + fake_gh = tmp_path / "gh" + fake_date = tmp_path / "date" + fake_gh.write_text( + """#!/usr/bin/env bash +set -euo pipefail +if [[ "$*" == "api repos/owner/repo/pulls/7" ]]; then + jq -cn --arg head "$PR_HEAD_SHA" '{state:"open",draft:false,created_at:"2026-09-30T00:00:00Z",head:{sha:$head}}' +elif [[ "$*" == *"actions/runs?"* && "$*" == *"per_page=1" ]]; then + printf '%s' '{"total_count":"2,500+","workflow_runs":[]}' +else + exit 97 +fi +""", + encoding="utf-8", + ) + fake_date.write_text( + """#!/usr/bin/env bash +set -euo pipefail +if [[ "$*" == "-u +%Y-%m-%dT%H:%M:%SZ" ]]; then + printf '%s\n' '2026-09-30T00:00:00Z' +elif [[ "$*" == *"+%s"* ]]; then + printf '%s\n' '1' +else + exit 98 +fi +""", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + fake_date.chmod(0o755) + result = subprocess.run( + ["bash", "-c", wake_failed_required_runs_script()], + env={ + **os.environ, + "PATH": f"{tmp_path}{os.pathsep}{os.environ['PATH']}", + "GH_TOKEN": "token", + "WAKE_TOKEN_SOURCE": "PR_REVIEW_MERGE_TOKEN", + "GH_REPOSITORY": "owner/repo", + "PR_NUMBER": "7", + "PR_HEAD_SHA": HEAD, + }, + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 1 + assert "More than 1,000 pull_request_target runs share one second" in result.stderr + + +def test_dispatch_wake_rejects_paginated_inventory_truncation(tmp_path: Path) -> None: + """Compare API total_count with collected rows before selecting mutations.""" + fake_gh = tmp_path / "gh" + fake_gh.write_text( + """#!/usr/bin/env bash +set -euo pipefail +if [[ "$*" == "api repos/owner/repo/pulls/7" ]]; then + jq -cn --arg head "$PR_HEAD_SHA" '{state:"open",draft:false,created_at:"2026-09-30T00:00:00Z",head:{sha:$head}}' +elif [[ "$*" == *"actions/runs?"* && "$*" == *"per_page=1" ]]; then + printf '%s' '{"total_count":2,"workflow_runs":[]}' +elif [[ "$*" == *"api --paginate"* && "$*" == *"per_page=100"* ]]; then + printf '%s' "$ONE_RUN" +else + exit 97 +fi +""", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + result = subprocess.run( + ["bash", "-c", wake_failed_required_runs_script()], + env={ + **os.environ, + "PATH": f"{tmp_path}{os.pathsep}{os.environ['PATH']}", + "GH_TOKEN": "token", + "WAKE_TOKEN_SOURCE": "PR_REVIEW_MERGE_TOKEN", + "GH_REPOSITORY": "owner/repo", + "PR_NUMBER": "7", + "PR_HEAD_SHA": HEAD, + "ONE_RUN": json.dumps({"workflow_runs": [required_run(41)]}), + }, + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 1 + assert "inventory was truncated: expected=2 collected=1" in result.stderr def test_native_cancellation_runs_before_runner_admission() -> None: diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index c764ad0ad2..0394640908 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -52,6 +52,26 @@ def admission_script() -> str: return textwrap.dedent(step.split(" run: |\n", 1)[1].split("\n\n changed-scope:", 1)[0]) +def central_single_flight_script() -> str: + """Extract the receiver's deterministic exact-head winner selection.""" + workflow = DISPATCH_WORKFLOW.read_text(encoding="utf-8") + step = workflow.split( + " - name: Admit one exact-head central dispatch\n", 1 + )[1] + block = step.split(" run: |\n", 1)[1].split("\n\n - name:", 1)[0] + return textwrap.dedent(block) + + +def central_authorization_script() -> str: + """Extract the unprivileged authorization boundary before token exchange.""" + workflow = DISPATCH_WORKFLOW.read_text(encoding="utf-8") + step = workflow.split( + " - name: Authorize repository dispatch envelope\n", 1 + )[1] + block = step.split(" run: |\n", 1)[1].split("\n\n - name:", 1)[0] + return textwrap.dedent(block) + + def test_stale_opencode_event_never_reaches_review_concurrency(tmp_path: Path) -> None: """A delayed old synchronize event is retired by live-head admission.""" fake_gh = tmp_path / "gh" @@ -82,16 +102,401 @@ def test_stale_opencode_event_never_reaches_review_concurrency(tmp_path: Path) - assert "retired a stale event" in result.stdout -def test_opencode_dispatch_uses_the_same_target_repo_pr_group() -> None: - """PR and repository_dispatch review jobs compute the same group text.""" +def test_opencode_dispatch_never_uses_lossy_native_concurrency() -> None: + """The receiver must not let GitHub replace a pending exact-head run.""" required = WORKFLOW.read_text(encoding="utf-8") dispatched = DISPATCH_WORKFLOW.read_text(encoding="utf-8") assert "opencode-review-${{" in required - assert "opencode-review-${{" in dispatched - assert "needs.validate-pr-metadata.outputs.target_repository" in dispatched - assert "needs.validate-pr-metadata.outputs.pr_number || github.run_id" in dispatched - assert workflow_level_cancels_in_progress(dispatched) - assert dispatched.index("validate-pr-metadata:") < dispatched.index(" concurrency:") + header = dispatched.split("permissions:", 1)[0] + review_job = dispatched.split("\n opencode-review-target:\n", 1)[1] + required_job = required.split("\n opencode-review-target:\n", 1)[1] + assert not re.search(r"(?m)^concurrency:", header) + assert not re.search(r"(?m)^ concurrency:", review_job) + required_permissions = required_job.split(" permissions:\n", 1)[1].split( + " steps:", 1 + )[0] + assert "actions: write" not in required_permissions + admission_job = dispatched.split("\n admit-exact-head-dispatch:\n", 1)[1].split( + "\n validate-pr-metadata:\n", 1 + )[0] + validation_job = dispatched.split("\n validate-pr-metadata:\n", 1)[1].split( + "\n coverage-evidence:\n", 1 + )[0] + assert "contents: write" in admission_job + assert "contents: write" not in validation_job + assert "contents: read" in validation_job + assert "admitted: ${{ steps.single_flight.outputs.admitted }}" in admission_job + assert "needs.admit-exact-head-dispatch.outputs.admitted == 'true'" in dispatched + assert "pull_request_review:" not in required + assert "Wake every failed exact-head Required OpenCode workflow" in dispatched + + +@pytest.mark.parametrize( + ("override", "error_text"), + ( + ({"DISPATCH_SENDER": "untrusted"}, "rejected actor="), + ( + {"TARGET_REPOSITORY": "ContextualWisdomLab/unlisted"}, + "rejected target=", + ), + ({"SUPPLIED_HEAD_SHA": "mutable"}, "malformed PR identity metadata"), + ), +) +def test_central_authorization_rejects_before_any_token_or_lease_access( + tmp_path: Path, override: dict[str, str], error_text: str +) -> None: + """Untrusted envelopes stop before OIDC exchange or Contents API mutation.""" + workflow = DISPATCH_WORKFLOW.read_text(encoding="utf-8") + authorize = workflow.index( + " - name: Authorize repository dispatch envelope\n" + ) + exchange = workflow.index( + " - name: Exchange OpenCode app token for target repository metadata reads\n" + ) + lease = workflow.index(" - name: Admit one exact-head central dispatch\n") + assert authorize < exchange < lease + + calls = tmp_path / "external-calls" + for command in ("curl", "gh"): + fake_command = tmp_path / command + fake_command.write_text( + "#!/usr/bin/env bash\nprintf '%s\\n' \"$0 $*\" >>\"$EXTERNAL_CALLS\"\nexit 99\n", + encoding="utf-8", + ) + fake_command.chmod(0o755) + env = { + **os.environ, + "PATH": f"{tmp_path}{os.pathsep}{os.environ.get('PATH', '')}", + "EXTERNAL_CALLS": str(calls), + "EVENT_NAME": "repository_dispatch", + "DISPATCH_ACTOR": "opencode-agent[bot]", + "DISPATCH_SENDER": "opencode-agent[bot]", + "ALLOWED_DISPATCH_ACTOR": "opencode-agent[bot]", + "ALLOWED_DISPATCH_TARGETS": "ContextualWisdomLab/example", + "TARGET_REPOSITORY": "ContextualWisdomLab/example", + "PR_NUMBER": "7", + "SUPPLIED_BASE_REF": "main", + "SUPPLIED_BASE_SHA": "b" * 40, + "SUPPLIED_HEAD_REF": "feature", + "SUPPLIED_HEAD_SHA": HEAD, + **override, + } + result = subprocess.run( + [shutil.which("bash") or "/bin/bash", "-c", central_authorization_script()], + env=env, + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 1 + assert error_text in result.stdout + assert not calls.exists() + + +@pytest.mark.parametrize( + "live_override", + ( + {"state": "closed"}, + {"draft": True}, + {"base": {"ref": "release", "sha": "b" * 40, "repo": {"full_name": "owner/repo"}}}, + {"base": {"ref": "main", "sha": "d" * 40, "repo": {"full_name": "owner/repo"}}}, + {"head": {"ref": "other", "sha": HEAD, "repo": {"full_name": "owner/repo"}}}, + {"head": {"ref": "feature", "sha": "d" * 40, "repo": {"full_name": "owner/repo"}}}, + ), +) +def test_central_admission_rejects_changed_live_identity_before_contents_mutation( + tmp_path: Path, live_override: dict[str, object] +) -> None: + """Every live state/base/head mismatch fails before the central lease write.""" + live_pr: dict[str, object] = { + "state": "open", + "draft": False, + "base": { + "ref": "main", + "sha": "b" * 40, + "repo": {"full_name": "owner/repo"}, + }, + "head": { + "ref": "feature", + "sha": HEAD, + "repo": {"full_name": "owner/repo"}, + }, + } + live_pr.update(live_override) + calls = tmp_path / "calls" + fake_gh = tmp_path / "gh" + fake_gh.write_text( + """#!/usr/bin/env bash +set -euo pipefail +printf '%s\n' "$*" >>"$CALLS" +if [[ "$*" == "api repos/owner/repo/pulls/7" ]]; then + printf '%s' "$LIVE_PR" + exit 0 +fi +exit 97 +""", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + result = subprocess.run( + [shutil.which("bash") or "/bin/bash", "-c", central_single_flight_script()], + env={ + **os.environ, + "PATH": f"{tmp_path}{os.pathsep}{os.environ.get('PATH', '')}", + "CALLS": str(calls), + "LIVE_PR": json.dumps(live_pr), + "GITHUB_OUTPUT": str(tmp_path / "github-output"), + "GITHUB_RUN_ID": "42", + "GITHUB_SHA": "a" * 40, + "TARGET_REPOSITORY": "owner/repo", + "PR_NUMBER": "7", + "HEAD_SHA": HEAD, + "SUPPLIED_BASE_REF": "main", + "SUPPLIED_BASE_SHA": "b" * 40, + "SUPPLIED_HEAD_REF": "feature", + "SUPPLIED_HEAD_SHA": HEAD, + "GH_TOKEN": "token", + "TARGET_READ_TOKEN": "target-token", + }, + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 1 + assert "authority changed before atomic OpenCode admission" in result.stdout + assert calls.read_text().splitlines() == ["api repos/owner/repo/pulls/7"] + + +@pytest.mark.parametrize( + ("lease_state", "owner_run_id", "owner_head", "owner_status", "expected_admitted"), + ( + ("absent", 41, HEAD, "", True), + ("present", 41, HEAD, "in_progress", False), + ("present", 41, HEAD, "completed", True), + ("present", 42, HEAD, "in_progress", True), + ("present", 41, "d" * 40, "in_progress", True), + ("present", 43, "d" * 40, "in_progress", True), + ), +) +def test_central_dispatch_single_flight_uses_atomic_contents_lease( + tmp_path: Path, + lease_state: str, + owner_run_id: int, + owner_head: str, + owner_status: str, + expected_admitted: bool, +) -> None: + """One atomic lease owner reaches expensive work; terminal leases recover.""" + fake_gh = tmp_path / "gh" + fake_gh.write_text( + """#!/usr/bin/env bash +set -euo pipefail +printf '%s\n' "$*" >>"$FAKE_CALLS" +if [[ "$*" == *"git/ref/heads/opencode-dispatch-leases"* ]]; then + printf '{"object":{"type":"commit","sha":"%s"}}' "$GITHUB_SHA" +elif [[ "$*" == *"repos/owner/repo/pulls/7"* ]]; then + jq -cn --arg base "$SUPPLIED_BASE_SHA" --arg head "$HEAD_SHA" \ + '{state:"open",draft:false,base:{ref:"main",sha:$base,repo:{full_name:"owner/repo"}},head:{ref:"feature",sha:$head,repo:{full_name:"owner/repo"}}}' +elif [[ "$*" == *"contents/opencode-dispatch-leases/"* && "$*" != *"--method PUT"* ]]; then + if [[ "$FAKE_LEASE_STATE" == "absent" ]]; then exit 1; fi + owner_title="OpenCode Review Dispatch owner/repo#7@${FAKE_OWNER_HEAD}" + content="$(jq -cn --argjson owner "$FAKE_OWNER_ID" --arg title "$owner_title" --arg head "$FAKE_OWNER_HEAD" \ + '{owner_run_id:$owner,exact_title:$title,head_sha:$head}')" + encoded="$(printf '%s' "$content" | base64 | tr -d '\n')" + jq -cn --arg encoded "$encoded" --arg sha "$(printf 'b%.0s' {1..40})" \ + '{sha:$sha,encoding:"base64",content:$encoded}' +elif [[ "$*" == *"actions/runs/"* ]]; then + owner_title="OpenCode Review Dispatch owner/repo#7@${FAKE_OWNER_HEAD}" + jq -cn --argjson owner "$FAKE_OWNER_ID" --arg status "$FAKE_OWNER_STATUS" --arg title "$owner_title" \ + '{id:$owner,path:".github/workflows/opencode-review-dispatch.yml",event:"repository_dispatch",display_title:$title,status:$status}' +elif [[ "$*" == *"contents/opencode-dispatch-leases/"* && "$*" == *"--method PUT"* ]]; then + jq -cn --arg sha "$(printf 'c%.0s' {1..40})" '{content:{sha:$sha}}' +else + printf 'unexpected gh call: %s\n' "$*" >&2 + exit 97 +fi +""", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + output = tmp_path / "github-output" + calls = tmp_path / "calls" + result = subprocess.run( + [shutil.which("bash") or "/bin/bash", "-c", central_single_flight_script()], + env={ + **os.environ, + "PATH": f"{tmp_path}{os.pathsep}{os.environ.get('PATH', '')}", + "FAKE_CALLS": str(calls), + "FAKE_LEASE_STATE": lease_state, + "FAKE_OWNER_ID": str(owner_run_id), + "FAKE_OWNER_HEAD": owner_head, + "FAKE_OWNER_STATUS": owner_status, + "GITHUB_OUTPUT": str(output), + "GITHUB_RUN_ID": "42", + "GITHUB_SHA": "a" * 40, + "TARGET_REPOSITORY": "owner/repo", + "PR_NUMBER": "7", + "HEAD_SHA": HEAD, + "SUPPLIED_BASE_REF": "main", + "SUPPLIED_BASE_SHA": "b" * 40, + "SUPPLIED_HEAD_REF": "feature", + "SUPPLIED_HEAD_SHA": HEAD, + "EXACT_TITLE": f"OpenCode Review Dispatch owner/repo#7@{HEAD}", + "GH_TOKEN": "token", + "TARGET_READ_TOKEN": "target-token", + }, + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 0, result.stderr + output_lines = output.read_text(encoding="utf-8").splitlines() + assert output_lines[0] == f"admitted={str(expected_admitted).lower()}" + expected_mutation = expected_admitted and owner_run_id != 42 + assert any("--method PUT" in call for call in calls.read_text().splitlines()) is expected_mutation + + +def test_atomic_contents_lease_admits_one_concurrent_receiver(tmp_path: Path) -> None: + """Two simultaneous cache misses converge on one compare-and-swap owner.""" + fake_gh = tmp_path / "gh" + fake_gh.write_text( + """#!/usr/bin/env bash +set -euo pipefail +if [[ "$*" == *"git/ref/heads/opencode-dispatch-leases"* ]]; then + printf '{"object":{"type":"commit","sha":"%s"}}' "$GITHUB_SHA" +elif [[ "$*" == *"repos/owner/repo/pulls/7"* ]]; then + jq -cn --arg base "$SUPPLIED_BASE_SHA" --arg head "$HEAD_SHA" \ + '{state:"open",draft:false,base:{ref:"main",sha:$base,repo:{full_name:"owner/repo"}},head:{ref:"feature",sha:$head,repo:{full_name:"owner/repo"}}}' +elif [[ "$*" == *"contents/opencode-dispatch-leases/"* && "$*" != *"--method PUT"* ]]; then + [[ -f "$LEASE_OWNER" ]] || exit 1 + owner="$(cat "$LEASE_OWNER")" + content="$(jq -cn --argjson owner "$owner" --arg title "$EXACT_TITLE" --arg head "$HEAD_SHA" \ + '{owner_run_id:$owner,exact_title:$title,head_sha:$head}')" + encoded="$(printf '%s' "$content" | base64 | tr -d '\n')" + jq -cn --arg encoded "$encoded" --arg sha "$(printf 'd%.0s' {1..40})" \ + '{sha:$sha,encoding:"base64",content:$encoded}' +elif [[ "$*" == *"actions/runs/"* ]]; then + [[ "$*" =~ actions/runs/([0-9]+) ]] || exit 96 + owner="${BASH_REMATCH[1]}" + jq -cn --argjson owner "$owner" --arg title "$EXACT_TITLE" \ + '{id:$owner,path:".github/workflows/opencode-review-dispatch.yml",event:"repository_dispatch",display_title:$title,status:"in_progress"}' +elif [[ "$*" == *"contents/opencode-dispatch-leases/"* && "$*" == *"--method PUT"* ]]; then + payload="$(cat)" + if mkdir "$LEASE_MUTEX" 2>/dev/null; then + printf '%s' "$payload" | jq -r '.content' | base64 --decode | jq -r '.owner_run_id' >"$LEASE_OWNER" + jq -cn --arg sha "$(printf 'e%.0s' {1..40})" '{content:{sha:$sha}}' + else + sleep 0.05 + exit 1 + fi +else + exit 97 +fi +""", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + base_env = { + **os.environ, + "PATH": f"{tmp_path}{os.pathsep}{os.environ.get('PATH', '')}", + "GITHUB_SHA": "a" * 40, + "TARGET_REPOSITORY": "owner/repo", + "PR_NUMBER": "7", + "HEAD_SHA": HEAD, + "SUPPLIED_BASE_REF": "main", + "SUPPLIED_BASE_SHA": "b" * 40, + "SUPPLIED_HEAD_REF": "feature", + "SUPPLIED_HEAD_SHA": HEAD, + "EXACT_TITLE": f"OpenCode Review Dispatch owner/repo#7@{HEAD}", + "GH_TOKEN": "token", + "TARGET_READ_TOKEN": "target-token", + "LEASE_MUTEX": str(tmp_path / "lease-mutex"), + "LEASE_OWNER": str(tmp_path / "lease-owner"), + } + processes: list[tuple[subprocess.Popen[str], Path]] = [] + for run_id in (41, 42): + output = tmp_path / f"output-{run_id}" + process = subprocess.Popen( # noqa: S603 + [shutil.which("bash") or "/bin/bash", "-c", central_single_flight_script()], + env={ + **base_env, + "GITHUB_RUN_ID": str(run_id), + "GITHUB_OUTPUT": str(output), + }, + text=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + ) + processes.append((process, output)) + admissions: list[str] = [] + for process, output in processes: + stdout, stderr = process.communicate(timeout=10) + assert process.returncode == 0, f"{stdout}\n{stderr}" + admissions.append(output.read_text(encoding="utf-8").splitlines()[0]) + assert sorted(admissions) == ["admitted=false", "admitted=true"] + + +def test_atomic_lease_branch_initialization_accepts_a_concurrent_creator( + tmp_path: Path, +) -> None: + """A ref-create conflict must re-read the exact central lease branch.""" + fake_gh = tmp_path / "gh" + ref_reads = tmp_path / "ref-reads" + fake_gh.write_text( + """#!/usr/bin/env bash +set -euo pipefail +if [[ "$*" == *"git/ref/heads/opencode-dispatch-leases"* ]]; then + reads=0 + [[ ! -f "$REF_READS" ]] || reads="$(cat "$REF_READS")" + reads=$((reads + 1)) + printf '%s' "$reads" >"$REF_READS" + [[ "$reads" -gt 1 ]] || exit 1 + printf '{"object":{"type":"commit","sha":"%s"}}' "$GITHUB_SHA" +elif [[ "$*" == *"repos/owner/repo/pulls/7"* ]]; then + jq -cn --arg base "$SUPPLIED_BASE_SHA" --arg head "$HEAD_SHA" \ + '{state:"open",draft:false,base:{ref:"main",sha:$base,repo:{full_name:"owner/repo"}},head:{ref:"feature",sha:$head,repo:{full_name:"owner/repo"}}}' +elif [[ "$*" == *"git/refs"* && "$*" == *"--method POST"* ]]; then + cat >/dev/null + exit 1 +elif [[ "$*" == *"contents/opencode-dispatch-leases/"* && "$*" != *"--method PUT"* ]]; then + exit 1 +elif [[ "$*" == *"contents/opencode-dispatch-leases/"* && "$*" == *"--method PUT"* ]]; then + cat >/dev/null + jq -cn --arg sha "$(printf 'f%.0s' {1..40})" '{content:{sha:$sha}}' +else + exit 97 +fi +""", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + output = tmp_path / "github-output" + result = subprocess.run( + [shutil.which("bash") or "/bin/bash", "-c", central_single_flight_script()], + env={ + **os.environ, + "PATH": f"{tmp_path}{os.pathsep}{os.environ.get('PATH', '')}", + "REF_READS": str(ref_reads), + "GITHUB_OUTPUT": str(output), + "GITHUB_RUN_ID": "42", + "GITHUB_SHA": "a" * 40, + "TARGET_REPOSITORY": "owner/repo", + "PR_NUMBER": "7", + "HEAD_SHA": HEAD, + "SUPPLIED_BASE_REF": "main", + "SUPPLIED_BASE_SHA": "b" * 40, + "SUPPLIED_HEAD_REF": "feature", + "SUPPLIED_HEAD_SHA": HEAD, + "GH_TOKEN": "token", + "TARGET_READ_TOKEN": "target-token", + }, + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 0, result.stderr + assert output.read_text(encoding="utf-8").splitlines() == ["admitted=true"] + assert ref_reads.read_text(encoding="utf-8") == "2" def review(*, state: str, commit_id: str = HEAD, body: str = "") -> dict[str, object]: @@ -104,6 +509,22 @@ def review(*, state: str, commit_id: str = HEAD, body: str = "") -> dict[str, ob } +def active_dispatch( + *, status: str, head_sha: str = HEAD, run_id: int = 42 +) -> dict[str, object]: + """Build one complete central OpenCode workflow-run identity.""" + return { + "id": run_id, + "name": "OpenCode Review Dispatch", + "display_title": f"OpenCode Review Dispatch owner/repo#7@{head_sha}", + "path": ".github/workflows/opencode-review-dispatch.yml", + "event": "repository_dispatch", + "status": status, + "head_sha": "c" * 40, + "pull_requests": [], + } + + def runtime_verdict(reviews: list[dict[str, object]], head_sha: str = HEAD) -> str: """Execute the jq program embedded in the required workflow.""" jq = shutil.which("jq") @@ -669,19 +1090,67 @@ def test_fail_closed_step_checks_once_for_a_non_draft_pr(tmp_path: Path) -> None @pytest.mark.parametrize( - ("reviews", "dispatches"), ( - ([{"id": 7, **review(state="APPROVED", body="## Verdict\nApprove")}], 0), - ([{"id": 8, **review(state="CHANGES_REQUESTED", body="## Verdict\nRequest changes")}], 0), - ([], 1), - ([{"id": 9, **review(state="APPROVED", commit_id="b" * 40, body="## Verdict\nApprove")}], 1), - ([{"id": 10, **review(state="APPROVED", body="## Pull request overview\n\ndeterministic fallback approval")}], 1), + "reviews", + "active_runs", + "later_active_runs", + "revalidated_head", + "lookup_failure", + "expected_returncode", + "dispatches", + ), + ( + ([{"id": 7, **review(state="APPROVED", body="## Verdict\nApprove")}], [], None, HEAD, False, 0, 0), + ([{"id": 8, **review(state="CHANGES_REQUESTED", body="## Verdict\nRequest changes")}], [], None, HEAD, False, 0, 0), + ([], [], None, HEAD, False, 0, 1), + ([], [], None, "e" * 40, False, 0, 0), + ([], [active_dispatch(status="queued")], None, HEAD, False, 0, 0), + ([], [active_dispatch(status="in_progress")], None, HEAD, False, 0, 0), + ([], [active_dispatch(status="requested")], None, HEAD, False, 0, 0), + ([], [active_dispatch(status="waiting")], None, HEAD, False, 0, 0), + ([], [active_dispatch(status="pending")], None, HEAD, False, 0, 0), + ( + [], + [ + active_dispatch(status="queued"), + active_dispatch(status="queued", head_sha="d" * 40, run_id=48), + ], + None, + HEAD, + False, + 0, + 0, + ), + ([], [active_dispatch(status="queued", head_sha="d" * 40)], None, HEAD, False, 0, 1), + ([], [active_dispatch(status="queued", head_sha="d" * 40, run_id=45)], None, HEAD, False, 1, 0), + ([], [active_dispatch(status="queued", head_sha="d" * 40, run_id=46)], None, HEAD, False, 1, 0), + ([], [active_dispatch(status="queued", head_sha="d" * 40, run_id=47)], None, HEAD, False, 0, 1), + ([], [], [active_dispatch(status="pending")], HEAD, False, 0, 0), + ( + [], + [{"id": 44, "path": ".github/workflows/opencode-review-dispatch.yml", "event": "repository_dispatch", "status": "queued"}], + None, + HEAD, + False, + 1, + 0, + ), + ([], [], None, HEAD, True, 1, 0), + ([{"id": 9, **review(state="APPROVED", commit_id="b" * 40, body="## Verdict\nApprove")}], [], None, HEAD, False, 0, 1), + ([{"id": 10, **review(state="APPROVED", body="## Pull request overview\n\ndeterministic fallback approval")}], [], None, HEAD, False, 0, 1), ), ) def test_scheduler_wake_reuses_trusted_receipt_predicate( - tmp_path: Path, reviews: list[dict[str, object]], dispatches: int + tmp_path: Path, + reviews: list[dict[str, object]], + active_runs: list[dict[str, object]], + later_active_runs: list[dict[str, object]] | None, + revalidated_head: str, + lookup_failure: bool, + expected_returncode: int, + dispatches: int, ) -> None: - """Only missing, stale, or fallback-only evidence wakes the scheduler.""" + """Only missing, stale, inactive evidence wakes one exact-head execution.""" fake_bin = tmp_path / "bin" fake_bin.mkdir() calls = tmp_path / "dispatches" @@ -690,11 +1159,52 @@ def test_scheduler_wake_reuses_trusted_receipt_predicate( """#!/usr/bin/env bash set -euo pipefail if [[ "$*" == "api repos/owner/repo/pulls/7" ]]; then - printf '%s' "$LIVE_PR_JSON" + count=0 + [[ ! -f "$LIVE_PR_CALLS" ]] || count="$(cat "$LIVE_PR_CALLS")" + count=$((count + 1)) + printf '%s' "$count" >"$LIVE_PR_CALLS" + if [[ "$count" -eq 1 ]]; then + printf '%s' "$LIVE_PR_JSON" + else + printf '%s' "$REVALIDATED_LIVE_PR_JSON" + fi elif [[ "$*" == *"contents/scripts/ci/opencode_review_receipt_gate.py"* ]]; then python3 -c 'import base64, pathlib, sys; sys.stdout.write(base64.b64encode(pathlib.Path(sys.argv[1]).read_bytes()).decode())' "$REAL_RECEIPT_HELPER" elif [[ "$*" == *"/pulls/7/reviews"* ]]; then printf '[%s]' "$FAKE_REVIEWS" +elif [[ "$*" == *"actions/workflows/opencode-review-dispatch.yml/runs"* ]]; then + if [[ "$FAKE_ACTIVE_LOOKUP_FAILURE" == "true" ]]; then + exit 19 + fi + count=0 + [[ ! -f "$ACTIVE_RUN_CALLS" ]] || count="$(cat "$ACTIVE_RUN_CALLS")" + count=$((count + 1)) + printf '%s' "$count" >"$ACTIVE_RUN_CALLS" + runs_request="${!#}" + active_status="${runs_request#*status=}" + active_status="${active_status%%&*}" + if [[ "$count" -le 5 ]]; then + active_source="$FAKE_ACTIVE_RUNS" + else + active_source="$FAKE_LATER_ACTIVE_RUNS" + fi + active_selection="$(jq -c --arg status "$active_status" '[.[] | select(.status == $status)]' <<<"$active_source")" + printf '{"workflow_runs":%s}' "$active_selection" +elif [[ "$*" == *"repos/ContextualWisdomLab/.github/actions/runs/"*"/cancel"* ]]; then + [[ "$*" =~ actions/runs/([0-9]+) ]] || exit 90 + run_id="${BASH_REMATCH[1]}" + printf '%s\n' "$run_id" >>"$CANCEL_CALLS" + [[ "$run_id" != "45" ]] || exit 19 +elif [[ "$*" == *"repos/ContextualWisdomLab/.github/actions/runs/"* ]]; then + [[ "$*" =~ actions/runs/([0-9]+) ]] || exit 91 + run_id="${BASH_REMATCH[1]}" + if [[ "$run_id" == "46" ]]; then + printf 'mystery' + elif [[ "$run_id" == "47" ]]; then + printf 'queued' + else + printf 'completed' + fi elif [[ "$*" == *"repos/ContextualWisdomLab/.github/dispatches"* ]]; then cat >/dev/null printf 'dispatch\n' >>"$DISPATCH_CALLS" @@ -716,6 +1226,13 @@ def test_scheduler_wake_reuses_trusted_receipt_predicate( "PATH": f"{fake_bin}{os.pathsep}{os.environ['PATH']}", "REAL_RECEIPT_HELPER": str(RECEIPT_HELPER.resolve()), "FAKE_REVIEWS": json.dumps(reviews), + "FAKE_ACTIVE_RUNS": json.dumps(active_runs), + "FAKE_LATER_ACTIVE_RUNS": json.dumps( + active_runs if later_active_runs is None else later_active_runs + ), + "FAKE_ACTIVE_LOOKUP_FAILURE": str(lookup_failure).lower(), + "ACTIVE_RUN_CALLS": str(tmp_path / "active-run-calls"), + "CANCEL_CALLS": str(tmp_path / "cancel-calls"), "DISPATCH_CALLS": str(calls), "ACTIONS_ID_TOKEN_REQUEST_TOKEN": "request", "ACTIONS_ID_TOKEN_REQUEST_URL": "https://token.example", @@ -734,13 +1251,36 @@ def test_scheduler_wake_reuses_trusted_receipt_predicate( "LIVE_PR_JSON": json.dumps( {"draft": False, "head": {"sha": HEAD}, "state": "open"} ), + "REVALIDATED_LIVE_PR_JSON": json.dumps( + {"draft": False, "head": {"sha": revalidated_head}, "state": "open"} + ), + "LIVE_PR_CALLS": str(tmp_path / "live-pr-calls"), } result = subprocess.run( ["bash", "-c", request_review_script()], env=env, text=True, capture_output=True ) - assert result.returncode == 0, result.stderr + assert result.returncode == expected_returncode, result.stderr actual = calls.read_text(encoding="utf-8").count("dispatch") if calls.exists() else 0 assert actual == dispatches + cancel_calls = tmp_path / "cancel-calls" + actual_cancel_ids = ( + sorted(set(cancel_calls.read_text(encoding="utf-8").splitlines())) + if cancel_calls.exists() + else [] + ) + expected_cancel_ids = sorted( + { + str(run["id"]) + for run in [*active_runs, *(later_active_runs or [])] + if isinstance(run.get("display_title"), str) + and str(run["display_title"]).startswith( + "OpenCode Review Dispatch owner/repo#7@" + ) + and str(run["display_title"]).lower() + != f"OpenCode Review Dispatch owner/repo#7@{HEAD}".lower() + } + ) + assert actual_cancel_ids == expected_cancel_ids def test_formal_receipt_wake_reruns_the_immediately_failed_required_job() -> None: @@ -754,13 +1294,13 @@ def test_formal_receipt_wake_reruns_the_immediately_failed_required_job() -> Non assert "rerun-failed-jobs" in dispatched assert "id: formal_review_receipt" in dispatched assert "steps.formal_review_receipt.outcome == 'success'" in dispatched - assert "github.event.client_payload.required_run_id != ''" in dispatched - assert 'gh api "repos/${GH_REPOSITORY}/actions/runs/${REQUIRED_RUN_ID}"' in dispatched - assert "select(.id == $run_id)" in dispatched - assert 'select(.event == "pull_request_target")' in dispatched - assert 'select(.path == ".github/workflows/opencode-review.yml")' in dispatched - assert "select(.head_sha == $head)" in dispatched - wake_step = dispatched.split("Wake exact-head required OpenCode workflow", 1)[1].split("\n\n - name:", 1)[0] + assert "actions/runs?event=pull_request_target" in dispatched + assert '.event == "pull_request_target"' in dispatched + assert '.path == ".github/workflows/opencode-review.yml"' in dispatched + assert "(.head.sha | ascii_downcase) == ($head | ascii_downcase)" in dispatched + wake_step = dispatched.split( + "Wake every failed exact-head Required OpenCode workflow", 1 + )[1].split("\n\n - name:", 1)[0] target_job = dispatched.split(" opencode-review-target:\n", 1)[1] target_permissions = target_job.split(" env:\n", 1)[0] assert "actions: write" in target_permissions @@ -771,32 +1311,38 @@ def test_formal_receipt_wake_reruns_the_immediately_failed_required_job() -> Non assert "steps.opencode_app_token.outputs.token" not in wake_step assert "WAKE_TOKEN_SOURCE" in wake_step assert '"$WAKE_TOKEN_SOURCE" = "unavailable"' in wake_step - assert "--paginate" not in wake_step - # Identity is the immutable target-repository run id plus event/path/head; - # do not depend on context-specific title or workflow_url rendering. + assert "--paginate" in wake_step + # Inventory identity is event/path/head/PR; do not depend on context-specific + # title or workflow_url rendering. + assert ".number == $pr" in wake_step assert "display_title ==" not in wake_step assert ".name | startswith(" not in wake_step assert 'workflow_url | contains("/actions/required_workflows/")' not in wake_step -def wake_selector(run: dict[str, object], *, head: str = HEAD, run_id: int = 42) -> str: - """Execute the wake step's run-validation jq program in isolation.""" +def wake_selector(run: dict[str, object], *, head: str = HEAD) -> str: + """Execute the wake inventory's fail-closed jq program in isolation.""" jq = shutil.which("jq") if jq is None: pytest.skip("jq is required to execute the production wake selector") dispatched = DISPATCH_WORKFLOW.read_text(encoding="utf-8") - marker = """jq -r --arg head "$PR_HEAD_SHA" --argjson run_id "$REQUIRED_RUN_ID" '""" + marker = """jq -c -s --arg head "$PR_HEAD_SHA" --argjson pr "$PR_NUMBER" '""" start = dispatched.index(marker) + len(marker) - end = dispatched.index("\n ')", start) + end = dispatched.index("\n ' <<<\"$run_pages\")", start) result = subprocess.run( - [jq, "-r", "--arg", "head", head, "--argjson", "run_id", str(run_id), dispatched[start:end]], - input=json.dumps(run), + [jq, "-c", "-s", "--arg", "head", head, "--argjson", "pr", "7", dispatched[start:end]], + input=json.dumps({"workflow_runs": [run]}), text=True, capture_output=True, check=False, ) - assert result.returncode == 0, result.stderr - return result.stdout.strip() + if result.returncode != 0: + return "" + inventory = json.loads(result.stdout) + if not inventory: + return "" + selected = inventory[0] + return f"{selected['id']}\t{selected['status']}\t{selected['conclusion']}" def required_run(*, run_id: int = 42, head_sha: str = HEAD, path: str = ".github/workflows/opencode-review.yml") -> dict[str, object]: @@ -810,7 +1356,9 @@ def required_run(*, run_id: int = 42, head_sha: str = HEAD, path: str = ".github """ return { "id": run_id, - "head_sha": head_sha, + # pull_request_target executes the trusted default-branch workflow, so + # run-level head_sha is not the pull request head. + "head_sha": "c" * 40, "event": "pull_request_target", "name": "Required OpenCode Review", "display_title": "Fix an unrelated example bug", @@ -821,11 +1369,12 @@ def required_run(*, run_id: int = 42, head_sha: str = HEAD, path: str = ".github ), "status": "completed", "conclusion": "failure", + "pull_requests": [{"number": 7, "head": {"sha": head_sha}}], } def test_wake_selector_matches_the_referenced_run_without_name_or_display_title() -> None: - """The exact-id, exact-head run is matched using only id/event/path/head_sha.""" + """An exact-head run is matched using only id/event/path/head_sha.""" assert wake_selector(required_run()) == "42\tcompleted\tfailure" @@ -847,7 +1396,9 @@ def test_wake_selector_rejects_a_referenced_run_for_a_different_workflow() -> No def test_formal_receipt_wakes_the_exact_head_failed_required_run(tmp_path: Path) -> None: """Execute the production wake script end-to-end against a fake GitHub API.""" dispatched = DISPATCH_WORKFLOW.read_text(encoding="utf-8") - step = dispatched.split(" - name: Wake exact-head required OpenCode workflow\n", 1)[1] + step = dispatched.split( + " - name: Wake every failed exact-head Required OpenCode workflow\n", 1 + )[1] run_block = step.split(" run: |\n", 1)[1].split("\n\n - name:", 1)[0] script = textwrap.dedent(run_block) calls = tmp_path / "calls" @@ -856,8 +1407,15 @@ def test_formal_receipt_wakes_the_exact_head_failed_required_run(tmp_path: Path) f"""#!/usr/bin/env bash set -euo pipefail printf '%s\\n' "$*" >>"$FAKE_CALLS" +if [[ "$*" == "api repos/ContextualWisdomLab/example/pulls/7" ]]; then + printf '%s' '{{"state":"open","draft":false,"created_at":"2026-09-30T00:00:00Z","head":{{"sha":"{HEAD}"}}}}' + exit 0 +fi if [[ "$*" == *"actions/runs/42/rerun-failed-jobs"* ]]; then exit 0; fi -if [[ "$*" == *"actions/runs/42"* ]]; then printf '%s\\n' '{json.dumps(required_run())}'; exit 0; fi +if [[ "$*" == *"actions/runs?event=pull_request_target"* ]]; then + printf '%s\\n' '{{"total_count":1,"workflow_runs":[{json.dumps(required_run())}]}}' + exit 0 +fi exit 1 """, encoding="utf-8", @@ -870,9 +1428,9 @@ def test_formal_receipt_wakes_the_exact_head_failed_required_run(tmp_path: Path) "PATH": f"{tmp_path}{os.pathsep}{os.environ.get('PATH', '')}", "FAKE_CALLS": str(calls), "GH_REPOSITORY": "ContextualWisdomLab/example", + "PR_NUMBER": "7", "GH_TOKEN": "actions-write-token", "PR_HEAD_SHA": HEAD, - "REQUIRED_RUN_ID": "42", "WAKE_TOKEN_SOURCE": "PR_REVIEW_MERGE_TOKEN", }, capture_output=True, @@ -882,14 +1440,16 @@ def test_formal_receipt_wakes_the_exact_head_failed_required_run(tmp_path: Path) assert result.returncode == 0, result.stderr recorded = calls.read_text(encoding="utf-8") assert "actions/runs/42/rerun-failed-jobs" in recorded - assert "repos/ContextualWisdomLab/example/actions/runs/42" in recorded - assert "--paginate" not in recorded + assert "actions/runs?event=pull_request_target" in recorded + assert "--paginate" in recorded def test_sibling_formal_receipt_fails_closed_without_actions_token() -> None: """A sibling wake without either Actions-capable PAT fails before GitHub I/O.""" dispatched = DISPATCH_WORKFLOW.read_text(encoding="utf-8") - step = dispatched.split(" - name: Wake exact-head required OpenCode workflow\n", 1)[1] + step = dispatched.split( + " - name: Wake every failed exact-head Required OpenCode workflow\n", 1 + )[1] script = textwrap.dedent( step.split(" run: |\n", 1)[1].split("\n\n - name:", 1)[0] ) @@ -900,7 +1460,6 @@ def test_sibling_formal_receipt_fails_closed_without_actions_token() -> None: "GH_TOKEN": "", "GH_REPOSITORY": "ContextualWisdomLab/example", "PR_HEAD_SHA": HEAD, - "REQUIRED_RUN_ID": "42", "WAKE_TOKEN_SOURCE": "unavailable", }, capture_output=True, diff --git a/tests/test_pr_review_autofix_nvidia_nim_contract.py b/tests/test_pr_review_autofix_nvidia_nim_contract.py index 1bfc13292e..44560b1e67 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 = "10707b070475c5e0889501ae4178b7868c6a7cc9" def _workflow_text(path: Path) -> str: diff --git a/tests/test_required_review_runner_image_contract.py b/tests/test_required_review_runner_image_contract.py index ab7ede7aa4..ad80e2ff40 100644 --- a/tests/test_required_review_runner_image_contract.py +++ b/tests/test_required_review_runner_image_contract.py @@ -85,8 +85,8 @@ def test_opencode_review_dispatch_uses_explicit_supported_image(self) -> None: follow-up sweep as still open). """ workflow = OPENCODE_REVIEW_DISPATCH.read_text(encoding="utf-8") - self.assertEqual(workflow.count("group: CWL central OpenCode"), 3) - self.assertEqual(workflow.count("labels: [self-hosted, linux, x64]"), 3) + self.assertEqual(workflow.count("group: CWL central OpenCode"), 4) + self.assertEqual(workflow.count("labels: [self-hosted, linux, x64]"), 4) self.assertNotIn("runs-on: ubuntu-latest", workflow) self.assertNotIn("runs-on: ubuntu-24.04", workflow) diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index a24b3c7d0c..5bfd102f48 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -25,7 +25,7 @@ def test_central_dispatch_and_control_jobs_use_dedicated_groups() -> None: """Central-only workflows cannot fall back into the general Ubuntu pool.""" for name, group, jobs in ( ("codeql-scan-dispatch.yml", "CWL central CodeQL", 3), - ("opencode-review-dispatch.yml", "CWL central OpenCode", 3), + ("opencode-review-dispatch.yml", "CWL central OpenCode", 4), ("agent-mention-router.yml", "CWL central control", 2), ("hourly-review-repair.yml", "CWL central control", 1), ): @@ -356,8 +356,8 @@ def test_privileged_review_retries_use_default_branch_repository_dispatch() -> N assert '"gh",\n "workflow",\n "run"' not in autofix_scheduler -def test_privileged_review_dispatch_coalesces_superseded_runs_before_admission() -> None: - """A superseded dispatch must be cancelled while queued, not after it takes a runner. +def test_privileged_review_dispatch_avoids_lossy_native_concurrency() -> None: + """The receiver must never let GitHub replace pending exact-head work. ``opencode-review-dispatch.yml`` carried its concurrency group only on the long ``opencode-review-target`` job. A job-level group is not evaluated @@ -369,25 +369,16 @@ def test_privileged_review_dispatch_coalesces_superseded_runs_before_admission() while they queued, every one of them after ``coverage-source-tree`` and ``coverage-evidence`` had already run. - The workflow-level group is keyed by the dispatched pull request, matching - ``codeql-scan-dispatch.yml``'s workflow-level group and the job-level group - this workflow keeps for the review job itself. + GitHub native concurrency retains at most one pending run per group and + replaces an older pending member even when ``cancel-in-progress`` is false. + The receiver therefore uses no native concurrency group. Trusted producer + deduplication and live-head cleanup remain the admission authorities. """ workflow = workflow_text("opencode-review-dispatch.yml") header = workflow.split("permissions:", 1)[0] - concurrency_contract = header.split("concurrency:", 1)[1] - group_value = workflow_level_concurrency_group(workflow) - - assert re.search(r"(?m)^concurrency:", header) - assert "opencode-review-dispatch-" in group_value - assert ( - "github.event.client_payload.target_repository || github.repository" - in group_value - ) - assert "github.event.client_payload.pr_number || github.run_id" in group_value - assert workflow_level_cancels_in_progress(workflow) - assert "github.event.client_payload.pr_head_sha" not in concurrency_contract - assert re.search(r"(?m)^ concurrency:", workflow) + assert not re.search(r"(?m)^concurrency:", header) + review_job = workflow.split("\n opencode-review-target:\n", 1)[1] + assert not re.search(r"(?m)^ concurrency:", review_job) @pytest.mark.parametrize( From 57044408a7b76563b47aadc481c4419cac828489 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 1 Oct 2026 13:09:53 +0900 Subject: [PATCH 02/10] fix(opencode): recheck receipt after receiver lease --- .../workflows/opencode-review-dispatch.yml | 45 ++++++++++++++-- ...opencode-same-head-dispatch-idempotency.md | 5 +- ...opencode-same-head-dispatch-idempotency.md | 9 +++- docs/product-technical-gap-baseline.md | 2 +- tests/test_opencode_agent_contract.py | 24 +++++++++ ...st_opencode_required_verdict_regression.py | 54 +++++++++++++++---- ...t_pr_review_autofix_nvidia_nim_contract.py | 2 +- 7 files changed, 122 insertions(+), 19 deletions(-) diff --git a/.github/workflows/opencode-review-dispatch.yml b/.github/workflows/opencode-review-dispatch.yml index 10707b0704..5c87c74863 100644 --- a/.github/workflows/opencode-review-dispatch.yml +++ b/.github/workflows/opencode-review-dispatch.yml @@ -183,6 +183,8 @@ jobs: lease_key="$(printf '%s#%s' "$TARGET_REPOSITORY" "$PR_NUMBER" | sha256sum | cut -d' ' -f1)" lease_path="opencode-dispatch-leases/${lease_key}.json" lease_ref_api="repos/ContextualWisdomLab/.github/git/ref/heads/${lease_branch}" + receipt_helper="$(mktemp)" + trap 'rm -f "$receipt_helper"' EXIT live_authority_matches() { local live_pr live_base_repo live_base_ref live_base_sha @@ -215,6 +217,41 @@ jobs: exit 1 fi + definite_receipt_state() { + if [ ! -s "$receipt_helper" ] && ! gh api \ + "repos/ContextualWisdomLab/.github/contents/scripts/ci/opencode_review_receipt_gate.py?ref=${GITHUB_SHA}" \ + --jq .content | base64 --decode >"$receipt_helper"; then + echo "::error::Could not load the trusted OpenCode receipt helper." >&2 + return 1 + fi + GH_TOKEN="$TARGET_READ_TOKEN" python3 -c 'import runpy,sys; gate=runpy.run_path(sys.argv[1]); reviews=gate["fetch_reviews"](sys.argv[2],int(sys.argv[3])); receipt,_reason=gate["evaluate_receipts"](reviews,sys.argv[4],is_draft=False); print("present" if receipt is not None else "missing")' \ + "$receipt_helper" "$TARGET_REPOSITORY" "$PR_NUMBER" "$HEAD_SHA" + } + + complete_receipt_admission() { + if ! live_authority_matches; then + echo "::error::Pull request authority changed before receiver receipt admission." + exit 1 + fi + if ! receiver_receipt_state="$(definite_receipt_state)"; then + echo "::error::Could not revalidate the formal exact-head receipt after lease acquisition." + exit 1 + fi + case "$receiver_receipt_state" in + present) + echo "admitted=false" >>"$GITHUB_OUTPUT" + echo "A formal exact-head OpenCode receipt already exists; duplicate receiver work skipped." + ;; + missing) + echo "admitted=true" >>"$GITHUB_OUTPUT" + ;; + *) + echo "::error::Trusted OpenCode receipt helper returned an invalid state." + exit 1 + ;; + esac + } + validate_lease_ref() { jq -e ' .object.type == "commit" @@ -290,7 +327,8 @@ jobs: fi if [ "$owner_run_id" = "$GITHUB_RUN_ID" ] && [ "${owner_head,,}" = "${HEAD_SHA,,}" ]; then - echo "admitted=true" >>"$GITHUB_OUTPUT" + complete_receipt_admission + [ "$receiver_receipt_state" = "missing" ] || exit 0 echo "Central exact-head rerun ${GITHUB_RUN_ID} retained its canonical lease." exit 0 fi @@ -350,10 +388,9 @@ jobs: echo "::error::Central OpenCode lease acquisition returned malformed metadata." exit 1 fi - { - echo "admitted=true" - } >>"$GITHUB_OUTPUT" echo "Central exact-head dispatch ${GITHUB_RUN_ID} acquired the atomic lease." + complete_receipt_admission + [ "$receiver_receipt_state" = "missing" ] || exit 0 exit 0 fi echo "Central exact-head lease changed during attempt ${lease_attempt}; retrying from the authoritative branch." diff --git a/CHANGELOG.d/20261001-opencode-same-head-dispatch-idempotency.md b/CHANGELOG.d/20261001-opencode-same-head-dispatch-idempotency.md index 5b3f230417..5e2198e7c6 100644 --- a/CHANGELOG.d/20261001-opencode-same-head-dispatch-idempotency.md +++ b/CHANGELOG.d/20261001-opencode-same-head-dispatch-idempotency.md @@ -4,4 +4,7 @@ states when the required-check wake path is retried, fail closed on ambiguous inventory, avoid GitHub's lossy pending concurrency replacement, retire only live-head-verified older central runs with completion evidence, and revalidate - pull-request authority immediately before a new dispatch. + pull-request authority immediately before a new dispatch. After a receiver + acquires the exact-PR lease, revalidate live authority and the formal + exact-head review receipt again so a completed prior receiver cannot trigger + duplicate coverage or model execution. diff --git a/docs/doctoring/opencode-same-head-dispatch-idempotency.md b/docs/doctoring/opencode-same-head-dispatch-idempotency.md index 709dfe27f0..6c256e8daa 100644 --- a/docs/doctoring/opencode-same-head-dispatch-idempotency.md +++ b/docs/doctoring/opencode-same-head-dispatch-idempotency.md @@ -63,6 +63,13 @@ against its canonical workflow, event, title, and head; a rerun with the same GitHub run ID retains its own lease. The lease file remains as auditable bounded state and is updated, not multiplied, on later heads. +Lease ownership alone is not a durable completion receipt. Immediately after +acquiring or retaining the exact-PR lease, the receiver revalidates live +repository/PR/head authority and evaluates the formal exact-head review receipt +from the trusted default-branch helper. An existing receipt returns +`admitted=false` before source materialization, coverage, or model execution; +an unavailable or malformed receipt lookup fails closed. + The dedicated admission job alone holds `contents: write` for that lease branch; later metadata validation and source materialization return to `contents: read`. @@ -108,7 +115,7 @@ authority movement between mutations, partial rerun failure, recursive partitioning above 1,000 results, and detected pagination truncation. The independent byte-for-byte reviewer pin was regenerated from the repaired receiver as exact Git blob -`10707b070475c5e0889501ae4178b7868c6a7cc9`. +`5c87c74863ef6872c1ef7136d5b330071920c09e`. Local focused verification on the stacked successor base `5a91ce9f9c3e773aa1172f1055fd791ccde8fdaa`: diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index bafc954bd6..3154adb1aa 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -4,7 +4,7 @@ | Gap | Exact evidence | Action | Status | |---|---|---|---| -| A repeated required-check wake for unchanged `.github#2545@9a4af5e438283a31dc05814d6bc2818caee782a3` cancelled queued central dispatch `36776536447` when same-head run `36778773766` was created, resetting queue position without new source authority | [Run 36776536447](https://github.com/ContextualWisdomLab/.github/actions/runs/36776536447); [run 36778773766](https://github.com/ContextualWisdomLab/.github/actions/runs/36778773766); protected `main@37b10243cec3d160ecc9c1be75c71428b160a703` | At the central `.github` owner, inventory all five active admission states twice, validate run identity and fail closed on ambiguity, remove lossy native receiver concurrency, retire only canonical older-head central runs after live repository/PR/head validation, and continue current-head admission after an accepted asynchronous cancellation. Because producer inventory and POST are not atomic, authorize actor/sender/target and payload shape before OIDC, then acquire one repository/PR lease through the Contents API's blob-SHA compare-and-swap only after full live state/draft/base/head validation; active same-head losers terminate cheaply, self-reruns and terminal owners recover, and a freshly revalidated live head may replace a fully validated older-head owner. Isolate central `contents: write` in a dedicated lease job and keep later jobs read-only. After formal receipt, use repository-wide run inventory, bind PR number and `pull_requests[].head.sha` rather than the trusted-base run-level SHA, recursively partition the PR-lifetime `created` range below GitHub's 1,000-result ceiling, verify `total_count` completeness, and revalidate before each failed-job rerun so duplicate callbacks are unnecessary. See [RCA and executable acceptance](doctoring/opencode-same-head-dispatch-idempotency.md) | **Proposed / local RED→GREEN complete; exact-head hosted Checks and independent approval required** | +| A repeated required-check wake for unchanged `.github#2545@9a4af5e438283a31dc05814d6bc2818caee782a3` cancelled queued central dispatch `36776536447` when same-head run `36778773766` was created, resetting queue position without new source authority | [Run 36776536447](https://github.com/ContextualWisdomLab/.github/actions/runs/36776536447); [run 36778773766](https://github.com/ContextualWisdomLab/.github/actions/runs/36778773766); protected `main@37b10243cec3d160ecc9c1be75c71428b160a703` | At the central `.github` owner, inventory all five active admission states twice, validate run identity and fail closed on ambiguity, remove lossy native receiver concurrency, retire only canonical older-head central runs after live repository/PR/head validation, and continue current-head admission after an accepted asynchronous cancellation. Because producer inventory and POST are not atomic, authorize actor/sender/target and payload shape before OIDC, then acquire one repository/PR lease through the Contents API's blob-SHA compare-and-swap only after full live state/draft/base/head validation; active same-head losers terminate cheaply, self-reruns and terminal owners recover, and a freshly revalidated live head may replace a fully validated older-head owner. After lease acquisition or retention, revalidate live repository/PR/head authority and the trusted formal exact-head receipt; an existing receipt must return `admitted=false` before source materialization, coverage, or model execution, and an unavailable or malformed lookup fails closed. Isolate central `contents: write` in a dedicated lease job and keep later jobs read-only. After formal receipt, use repository-wide run inventory, bind PR number and `pull_requests[].head.sha` rather than the trusted-base run-level SHA, recursively partition the PR-lifetime `created` range below GitHub's 1,000-result ceiling, verify `total_count` completeness, and revalidate before each failed-job rerun so duplicate callbacks are unnecessary. See [RCA and executable acceptance](doctoring/opencode-same-head-dispatch-idempotency.md) | **Proposed / local RED→GREEN complete; exact-head hosted Checks and independent approval required** | ## 2026-10-01 bounded Maturin release downloader SAST closure diff --git a/tests/test_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index 2e170db815..148013e502 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -1238,6 +1238,30 @@ def test_opencode_model_exhaustion_retry_stays_owned_by_central_scheduler(): assert "contents: write" not in review_job +def test_receiver_rechecks_formal_receipt_after_exact_head_lease(): + """A completed owner must not let a duplicate receiver repeat model work.""" + workflow = Path(".github/workflows/opencode-review-dispatch.yml").read_text( + encoding="utf-8" + ) + admission_job = workflow.split(" admit-exact-head-dispatch:\n", 1)[1].split( + "\n validate-pr-metadata:", 1 + )[0] + + assert "opencode_review_receipt_gate.py?ref=${GITHUB_SHA}" in admission_job + assert "definite_receipt_state()" in admission_job + assert 'gate["evaluate_receipts"](' in admission_job + assert 'echo "admitted=false" >>"$GITHUB_OUTPUT"' in admission_job + assert admission_job.count('echo "admitted=true" >>"$GITHUB_OUTPUT"') == 1 + + lease_acquired = admission_job.index( + 'echo "Central exact-head dispatch ${GITHUB_RUN_ID} acquired the atomic lease."' + ) + receipt_recheck = admission_job.index( + "complete_receipt_admission", lease_acquired + ) + assert lease_acquired < receipt_recheck + + def test_sandbox_git_config_env_trusts_only_the_validated_worktree(tmp_path): """The propagated Git config names one exact worktree and no wildcard.""" worktree = tmp_path / "work" diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index 0394640908..e78a3fa2e4 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -268,14 +268,23 @@ def test_central_admission_rejects_changed_live_identity_before_contents_mutatio @pytest.mark.parametrize( - ("lease_state", "owner_run_id", "owner_head", "owner_status", "expected_admitted"), ( - ("absent", 41, HEAD, "", True), - ("present", 41, HEAD, "in_progress", False), - ("present", 41, HEAD, "completed", True), - ("present", 42, HEAD, "in_progress", True), - ("present", 41, "d" * 40, "in_progress", True), - ("present", 43, "d" * 40, "in_progress", True), + "lease_state", + "owner_run_id", + "owner_head", + "owner_status", + "receipt_present", + "expected_admitted", + ), + ( + ("absent", 41, HEAD, "", False, True), + ("present", 41, HEAD, "in_progress", False, False), + ("present", 41, HEAD, "completed", False, True), + ("present", 41, HEAD, "completed", True, False), + ("present", 42, HEAD, "in_progress", False, True), + ("present", 42, HEAD, "in_progress", True, False), + ("present", 41, "d" * 40, "in_progress", False, True), + ("present", 43, "d" * 40, "in_progress", False, True), ), ) def test_central_dispatch_single_flight_uses_atomic_contents_lease( @@ -284,6 +293,7 @@ def test_central_dispatch_single_flight_uses_atomic_contents_lease( owner_run_id: int, owner_head: str, owner_status: str, + receipt_present: bool, expected_admitted: bool, ) -> None: """One atomic lease owner reaches expensive work; terminal leases recover.""" @@ -294,9 +304,17 @@ def test_central_dispatch_single_flight_uses_atomic_contents_lease( printf '%s\n' "$*" >>"$FAKE_CALLS" if [[ "$*" == *"git/ref/heads/opencode-dispatch-leases"* ]]; then printf '{"object":{"type":"commit","sha":"%s"}}' "$GITHUB_SHA" -elif [[ "$*" == *"repos/owner/repo/pulls/7"* ]]; then +elif [[ "$*" == *"repos/owner/repo/pulls/7"* && "$*" != *"/reviews"* ]]; then jq -cn --arg base "$SUPPLIED_BASE_SHA" --arg head "$HEAD_SHA" \ '{state:"open",draft:false,base:{ref:"main",sha:$base,repo:{full_name:"owner/repo"}},head:{ref:"feature",sha:$head,repo:{full_name:"owner/repo"}}}' +elif [[ "$*" == *"opencode_review_receipt_gate.py?ref="* ]]; then + base64 <"$RECEIPT_HELPER_SOURCE" | tr -d '\n' +elif [[ "$*" == *"repos/owner/repo/pulls/7/reviews"* ]]; then + if [[ "$FAKE_RECEIPT_PRESENT" == "true" ]]; then + jq -cn --arg head "$HEAD_SHA" '[[{id:91,user:{login:"opencode-agent[bot]"},state:"CHANGES_REQUESTED",commit_id:$head,body:"## Pull request overview"}]]' + else + printf '[[]]' + fi elif [[ "$*" == *"contents/opencode-dispatch-leases/"* && "$*" != *"--method PUT"* ]]; then if [[ "$FAKE_LEASE_STATE" == "absent" ]]; then exit 1; fi owner_title="OpenCode Review Dispatch owner/repo#7@${FAKE_OWNER_HEAD}" @@ -331,6 +349,8 @@ def test_central_dispatch_single_flight_uses_atomic_contents_lease( "FAKE_OWNER_ID": str(owner_run_id), "FAKE_OWNER_HEAD": owner_head, "FAKE_OWNER_STATUS": owner_status, + "FAKE_RECEIPT_PRESENT": str(receipt_present).lower(), + "RECEIPT_HELPER_SOURCE": str(RECEIPT_HELPER.resolve()), "GITHUB_OUTPUT": str(output), "GITHUB_RUN_ID": "42", "GITHUB_SHA": "a" * 40, @@ -352,7 +372,9 @@ def test_central_dispatch_single_flight_uses_atomic_contents_lease( assert result.returncode == 0, result.stderr output_lines = output.read_text(encoding="utf-8").splitlines() assert output_lines[0] == f"admitted={str(expected_admitted).lower()}" - expected_mutation = expected_admitted and owner_run_id != 42 + expected_mutation = owner_run_id != 42 and not ( + owner_head == HEAD and owner_status == "in_progress" + ) assert any("--method PUT" in call for call in calls.read_text().splitlines()) is expected_mutation @@ -364,9 +386,13 @@ def test_atomic_contents_lease_admits_one_concurrent_receiver(tmp_path: Path) -> set -euo pipefail if [[ "$*" == *"git/ref/heads/opencode-dispatch-leases"* ]]; then printf '{"object":{"type":"commit","sha":"%s"}}' "$GITHUB_SHA" -elif [[ "$*" == *"repos/owner/repo/pulls/7"* ]]; then +elif [[ "$*" == *"repos/owner/repo/pulls/7"* && "$*" != *"/reviews"* ]]; then jq -cn --arg base "$SUPPLIED_BASE_SHA" --arg head "$HEAD_SHA" \ '{state:"open",draft:false,base:{ref:"main",sha:$base,repo:{full_name:"owner/repo"}},head:{ref:"feature",sha:$head,repo:{full_name:"owner/repo"}}}' +elif [[ "$*" == *"opencode_review_receipt_gate.py?ref="* ]]; then + base64 <"$RECEIPT_HELPER_SOURCE" | tr -d '\n' +elif [[ "$*" == *"repos/owner/repo/pulls/7/reviews"* ]]; then + printf '[[]]' elif [[ "$*" == *"contents/opencode-dispatch-leases/"* && "$*" != *"--method PUT"* ]]; then [[ -f "$LEASE_OWNER" ]] || exit 1 owner="$(cat "$LEASE_OWNER")" @@ -410,6 +436,7 @@ def test_atomic_contents_lease_admits_one_concurrent_receiver(tmp_path: Path) -> "EXACT_TITLE": f"OpenCode Review Dispatch owner/repo#7@{HEAD}", "GH_TOKEN": "token", "TARGET_READ_TOKEN": "target-token", + "RECEIPT_HELPER_SOURCE": str(RECEIPT_HELPER.resolve()), "LEASE_MUTEX": str(tmp_path / "lease-mutex"), "LEASE_OWNER": str(tmp_path / "lease-owner"), } @@ -452,9 +479,13 @@ def test_atomic_lease_branch_initialization_accepts_a_concurrent_creator( printf '%s' "$reads" >"$REF_READS" [[ "$reads" -gt 1 ]] || exit 1 printf '{"object":{"type":"commit","sha":"%s"}}' "$GITHUB_SHA" -elif [[ "$*" == *"repos/owner/repo/pulls/7"* ]]; then +elif [[ "$*" == *"repos/owner/repo/pulls/7"* && "$*" != *"/reviews"* ]]; then jq -cn --arg base "$SUPPLIED_BASE_SHA" --arg head "$HEAD_SHA" \ '{state:"open",draft:false,base:{ref:"main",sha:$base,repo:{full_name:"owner/repo"}},head:{ref:"feature",sha:$head,repo:{full_name:"owner/repo"}}}' +elif [[ "$*" == *"opencode_review_receipt_gate.py?ref="* ]]; then + base64 <"$RECEIPT_HELPER_SOURCE" | tr -d '\n' +elif [[ "$*" == *"repos/owner/repo/pulls/7/reviews"* ]]; then + printf '[[]]' elif [[ "$*" == *"git/refs"* && "$*" == *"--method POST"* ]]; then cat >/dev/null exit 1 @@ -489,6 +520,7 @@ def test_atomic_lease_branch_initialization_accepts_a_concurrent_creator( "SUPPLIED_HEAD_SHA": HEAD, "GH_TOKEN": "token", "TARGET_READ_TOKEN": "target-token", + "RECEIPT_HELPER_SOURCE": str(RECEIPT_HELPER.resolve()), }, capture_output=True, text=True, diff --git a/tests/test_pr_review_autofix_nvidia_nim_contract.py b/tests/test_pr_review_autofix_nvidia_nim_contract.py index 44560b1e67..2d0078242b 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 = "10707b070475c5e0889501ae4178b7868c6a7cc9" +REVIEW_DISPATCH_BLOB_SHA = "5c87c74863ef6872c1ef7136d5b330071920c09e" def _workflow_text(path: Path) -> str: From 1a62357d0fae38c633293fce9e46d115ddd745fd Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 1 Oct 2026 14:00:49 +0900 Subject: [PATCH 03/10] fix(scheduler): prove stale-run retirement --- .github/workflows/opencode-review.yml | 44 +++-- .../workflows/pr-review-merge-scheduler.yml | 161 ++++++++++++++++- ...opencode-same-head-dispatch-idempotency.md | 4 + CHANGELOG.md | 6 +- ...opencode-same-head-dispatch-idempotency.md | 31 ++-- docs/product-technical-gap-baseline.md | 2 +- tests/test_close_empty_pr_queue_pressure.py | 17 +- ...urrent_head_coalescer_self_cancellation.py | 25 ++- tests/test_opencode_agent_contract.py | 11 +- ...st_opencode_required_verdict_regression.py | 52 ++++-- tests/test_opencode_workflow_shell_syntax.py | 17 ++ .../test_required_workflow_queue_contract.py | 170 +++++++++++++++++- 12 files changed, 472 insertions(+), 68 deletions(-) diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index d4db5efa9c..ff791c5922 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -584,22 +584,38 @@ jobs: echo "::error::Could not cancel superseded central OpenCode dispatch ${stale_run_id}; refusing to add current-head work behind it." exit 1 fi - if ! stale_status="$(GH_TOKEN="$app_token" gh api \ - "repos/ContextualWisdomLab/.github/actions/runs/${stale_run_id}" --jq '.status')"; then - echo "::error::Could not verify superseded central OpenCode dispatch ${stale_run_id} cancellation." + cancellation_verified=false + for cancellation_attempt in 1 2 3 4 5 6; do + if ! stale_state="$(GH_TOKEN="$app_token" gh api \ + "repos/ContextualWisdomLab/.github/actions/runs/${stale_run_id}" \ + --jq '[.status // "", .conclusion // ""] | @tsv')"; then + echo "::error::Could not verify superseded central OpenCode dispatch ${stale_run_id} cancellation." + exit 1 + fi + IFS=$'\t' read -r stale_status stale_conclusion <<<"$stale_state" + if [ "$stale_status" = "completed" ] && + [ "$stale_conclusion" = "cancelled" ]; then + cancellation_verified=true + break + fi + case "$stale_status" in + requested|waiting|pending|queued|in_progress) + ;; + completed) + echo "::error::Superseded central OpenCode dispatch ${stale_run_id} completed with non-cancelled conclusion ${stale_conclusion:-}." + exit 1 + ;; + *) + echo "::error::Superseded central OpenCode dispatch ${stale_run_id} returned invalid status ${stale_status:-}." + exit 1 + ;; + esac + done + if [ "$cancellation_verified" != "true" ]; then + echo "::error::Superseded central OpenCode dispatch ${stale_run_id} did not reach completed/cancelled after accepted cancellation." exit 1 fi - case "$stale_status" in - requested|waiting|pending|queued|in_progress) - echo "Superseded central OpenCode dispatch ${stale_run_id} cancellation is still ${stale_status}; current-head admission continues without waiting." - ;; - completed) ;; - *) - echo "::error::Superseded central OpenCode dispatch ${stale_run_id} returned invalid status ${stale_status:-}." - exit 1 - ;; - esac - echo "Cancellation accepted for superseded central OpenCode dispatch ${stale_run_id} after live exact-head revalidation." + echo "Verified cancelled superseded central OpenCode dispatch ${stale_run_id} after live exact-head revalidation." done < <(sort -nu "$stale_dispatches_file") if [ "$same_head_found" = "true" ]; then diff --git a/.github/workflows/pr-review-merge-scheduler.yml b/.github/workflows/pr-review-merge-scheduler.yml index 7777f02bed..f42f062799 100644 --- a/.github/workflows/pr-review-merge-scheduler.yml +++ b/.github/workflows/pr-review-merge-scheduler.yml @@ -87,8 +87,16 @@ concurrency: github.event_name == 'repository_dispatch' && github.event.client_payload.target_repository != '' && github.event.client_payload.pr_number != '' && format('target-{0}-pr-{1}', github.event.client_payload.target_repository, github.event.client_payload.pr_number) || github.event_name == 'repository_dispatch' && github.event.client_payload.pr_number != '' && format('pr-{0}', github.event.client_payload.pr_number) || github.event_name == 'repository_dispatch' && format('repo-dispatch-{0}', github.repository) || - github.ref }} - cancel-in-progress: ${{ github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review' || github.event_name == 'repository_dispatch' }} + github.ref }}-${{ + github.event_name == 'pull_request_target' && github.event.action == 'closed' && format('head-{0}-closed', github.event.pull_request.head.sha) || + github.event_name == 'pull_request_target' && format('head-{0}', github.event.pull_request.head.sha) || + github.event_name == 'pull_request_review' && format('head-{0}', github.event.pull_request.head.sha) || + github.event_name == 'repository_dispatch' && github.event.client_payload.pr_head_sha != '' && format('head-{0}', github.event.client_payload.pr_head_sha) || + 'no-head' }} + # Preserve every same-head admission in GitHub's bounded FIFO queue. A new + # head receives a distinct group; the metadata-only cleanup job below retires + # only revalidated predecessor work and proves terminal cancellation. + queue: max # Scorecard Token-Permissions (alert #9): declare a least-privilege default at # the workflow level. The scan-pr-queue job that actually needs write access @@ -98,6 +106,145 @@ permissions: contents: read jobs: + cancel-superseded-pr-runs: + if: >- + github.event_name == 'pull_request_target' && + ( + github.event.action == 'synchronize' || + github.event.action == 'closed' + ) + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: + actions: write + contents: read + pull-requests: read + env: + GH_TOKEN: ${{ github.token }} + TARGET_REPOSITORY: ${{ github.repository }} + TARGET_PR_NUMBER: ${{ github.event.pull_request.number }} + TARGET_PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} + TARGET_ACTION: ${{ github.event.action }} + steps: + - name: Cancel revalidated predecessor scheduler runs + shell: bash + run: | + set -euo pipefail + + if ! [[ "$TARGET_REPOSITORY" =~ ^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ ]] || + ! [[ "$TARGET_PR_NUMBER" =~ ^[1-9][0-9]*$ ]] || + ! [[ "$TARGET_PR_HEAD_SHA" =~ ^[0-9a-fA-F]{40}$ ]]; then + echo "::error::Superseded-run cleanup rejected malformed target identity." + exit 1 + fi + + live_target_matches() { + local live_pull live_repository live_number live_state live_head_sha + live_pull="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${TARGET_PR_NUMBER}")" + live_repository="$(jq -r '.base.repo.full_name // empty' <<<"$live_pull")" + live_number="$(jq -r '.number // 0' <<<"$live_pull")" + live_state="$(jq -r '.state // empty' <<<"$live_pull")" + live_head_sha="$(jq -r '.head.sha // empty' <<<"$live_pull")" + [ "$live_repository" = "$TARGET_REPOSITORY" ] && + [ "$live_number" = "$TARGET_PR_NUMBER" ] && + [ "${live_head_sha,,}" = "${TARGET_PR_HEAD_SHA,,}" ] && { + [ "$TARGET_ACTION:$live_state" = "synchronize:open" ] || + [ "$TARGET_ACTION:$live_state" = "closed:closed" ] + } + } + + if ! live_target_matches; then + echo "Superseded-run cleanup skipped because the event no longer matches the live pull request." + exit 0 + fi + + inventory_dir="$(mktemp -d)" + trap 'rm -rf "$inventory_dir"' EXIT + : >"$inventory_dir/active-arrays.jsonl" + for active_status in requested waiting pending queued in_progress; do + gh api --paginate --slurp \ + "repos/${TARGET_REPOSITORY}/actions/workflows/pr-review-merge-scheduler.yml/runs?event=pull_request_target&status=${active_status}&per_page=100" \ + >"$inventory_dir/${active_status}.json" + jq -c '[.[] | .workflow_runs[]?]' \ + "$inventory_dir/${active_status}.json" \ + >>"$inventory_dir/active-arrays.jsonl" + done + workflow_runs="$(jq -sc 'add // [] | unique_by(.id)' "$inventory_dir/active-arrays.jsonl")" + mapfile -t superseded_run_ids < <( + jq -r \ + --argjson pr_number "$TARGET_PR_NUMBER" \ + --argjson current_run_id "$GITHUB_RUN_ID" \ + --arg target_head "${TARGET_PR_HEAD_SHA,,}" \ + --arg target_action "$TARGET_ACTION" \ + ' + .[] + | select(.id != $current_run_id) + | select( + .status == "requested" or + .status == "waiting" or + .status == "pending" or + .status == "queued" or + .status == "in_progress" + ) + | select( + any( + .pull_requests[]?; + (.head.sha // "" | ascii_downcase) as $run_pr_head + | .number == $pr_number + | select( + $target_action == "closed" or + ( + ($run_pr_head | test("^[0-9a-f]{40}$")) and + $run_pr_head != $target_head + ) + ) + ) + ) + | .id + ' <<<"$workflow_runs" + ) + + for run_id in "${superseded_run_ids[@]}"; do + if ! live_target_matches; then + echo "Superseded-run cleanup stopped because the target changed before cancellation." + exit 0 + fi + if gh api --method POST \ + "repos/${TARGET_REPOSITORY}/actions/runs/${run_id}/force-cancel" \ + >/dev/null 2>&1; then + cancellation_verified=false + for attempt in 1 2 3 4 5 6; do + IFS=$'\t' read -r run_status run_conclusion < <( + gh api "repos/${TARGET_REPOSITORY}/actions/runs/${run_id}" \ + --jq '[.status // "", .conclusion // ""] | @tsv' + ) + if [ "$run_status" = "completed" ] && + [ "$run_conclusion" = "cancelled" ]; then + cancellation_verified=true + break + fi + if [ "$attempt" -lt 6 ]; then + sleep 1 + fi + done + if [ "$cancellation_verified" != "true" ]; then + echo "::error::Scheduler run $run_id did not reach completed/cancelled after accepted force-cancel." + exit 1 + fi + echo "Verified cancelled scheduler run $run_id." + continue + fi + IFS=$'\t' read -r run_status run_conclusion < <( + gh api "repos/${TARGET_REPOSITORY}/actions/runs/${run_id}" \ + --jq '[.status // "", .conclusion // ""] | @tsv' + ) + if [ "$run_status" = "completed" ]; then + continue + fi + echo "::error::Could not force-cancel nonterminal scheduler run $run_id." + exit 1 + done + scan-pr-queue: # repository_dispatch review runs do not reliably carry pull_requests metadata. # Without this guard, one completed central review can wake a repo-wide scan. @@ -219,6 +366,7 @@ jobs: GH_TOKEN: ${{ secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN || steps.scheduler_app_token.outputs.token || github.token }} TARGET_REPOSITORY_INPUT: ${{ github.event.client_payload.target_repository || '' }} TARGET_PR_NUMBER: ${{ github.event.client_payload.pr_number || '' }} + TARGET_HEAD_SHA_INPUT: ${{ github.event.client_payload.pr_head_sha || '' }} TARGET_BASE_BRANCH_INPUT: ${{ github.event.client_payload.base_branch || '' }} ALLOWED_TARGET_REPOSITORIES: ${{ vars.OPENCODE_REPOSITORY_DISPATCH_TARGETS }} run: | @@ -238,8 +386,9 @@ jobs: exit 1 fi if ! [[ "$TARGET_REPOSITORY_INPUT" =~ ^ContextualWisdomLab/[A-Za-z0-9_.-]+$ ]] || - ! [[ "$TARGET_PR_NUMBER" =~ ^[1-9][0-9]*$ ]]; then - printf '::error::Targeted scheduler dispatch rejected an invalid repository or pull request number. target=%s pr=%s\n' "${TARGET_REPOSITORY_INPUT:-}" "${TARGET_PR_NUMBER:-}" + ! [[ "$TARGET_PR_NUMBER" =~ ^[1-9][0-9]*$ ]] || + ! [[ "$TARGET_HEAD_SHA_INPUT" =~ ^[0-9a-fA-F]{40}$ ]]; then + printf '::error::Targeted scheduler dispatch rejected invalid repository, pull request, or head identity. target=%s pr=%s head=%s\n' "${TARGET_REPOSITORY_INPUT:-}" "${TARGET_PR_NUMBER:-}" "${TARGET_HEAD_SHA_INPUT:-}" exit 1 fi @@ -281,6 +430,10 @@ jobs: printf '::error::Targeted scheduler dispatch base branch does not match the live PR. supplied=%s live=%s\n' "$TARGET_BASE_BRANCH_INPUT" "$live_base_branch" exit 1 fi + if [ "${TARGET_HEAD_SHA_INPUT,,}" != "${live_head_sha,,}" ]; then + printf '::error::Targeted scheduler dispatch head does not match the live PR. supplied=%s live=%s\n' "$TARGET_HEAD_SHA_INPUT" "$live_head_sha" + exit 1 + fi { printf 'repository=%s\n' "$TARGET_REPOSITORY_INPUT" diff --git a/CHANGELOG.d/20261001-opencode-same-head-dispatch-idempotency.md b/CHANGELOG.d/20261001-opencode-same-head-dispatch-idempotency.md index 5e2198e7c6..f239cb182f 100644 --- a/CHANGELOG.d/20261001-opencode-same-head-dispatch-idempotency.md +++ b/CHANGELOG.d/20261001-opencode-same-head-dispatch-idempotency.md @@ -8,3 +8,7 @@ acquires the exact-PR lease, revalidate live authority and the formal exact-head review receipt again so a completed prior receiver cannot trigger duplicate coverage or model execution. + Preserve every exact-head merge-scheduler admission with `queue: max`; a + dedicated metadata-only cleanup retires only live-head-revalidated + predecessor runs and proves each accepted cancellation reaches + `completed/cancelled`. diff --git a/CHANGELOG.md b/CHANGELOG.md index 7bfac92f0a..43c23fa330 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,8 +4,10 @@ evidence showed a later same-head wake cancelling the queued authoritative run. The required wake now inventories all five active states twice, retires only identity-validated older-head runs after live authority checks, and - continues current-head admission when GitHub has accepted but not completed - an asynchronous cancellation. The receiver atomically compare-and-swaps one + fails closed until every accepted cancellation is proven + `completed/cancelled`. The merge scheduler now preserves all exact-head + admissions with `queue: max` and retires predecessor heads only through a + metadata-only, live-head-revalidated cleanup job. The receiver atomically compare-and-swaps one repository/PR lease file on a dedicated central branch before source materialization, coverage, or model execution, closing the cross-producer check-then-POST race without lossy native concurrency. A dedicated minimal diff --git a/docs/doctoring/opencode-same-head-dispatch-idempotency.md b/docs/doctoring/opencode-same-head-dispatch-idempotency.md index 6c256e8daa..7fc996b682 100644 --- a/docs/doctoring/opencode-same-head-dispatch-idempotency.md +++ b/docs/doctoring/opencode-same-head-dispatch-idempotency.md @@ -31,8 +31,8 @@ Before posting a new dispatch, the trusted required-check path now: state transition during the inventory remains observable; 5. validates each returned run's identity fields and matches its protected exact `repository#PR@head` title plus workflow path and trigger; -6. records an exact-head execution without returning until canonical - older-head executions have been retired; and +6. records an exact-head execution only after every accepted older-head + cancellation reaches terminal `completed/cancelled`; and 7. re-fetches the PR immediately before dispatch, retiring the request if state, draft status, or head authority changed. @@ -42,10 +42,18 @@ when `cancel-in-progress` is false, so native concurrency cannot preserve every distinct callback payload. Producers deduplicate exact-head work from trusted inventory. Before a current-head POST, the required workflow revalidates live repository/PR/head authority and cancels only canonical older-head central -runs. A refused cancellation, failed status lookup, or invalid status fails -closed. GitHub may still report an accepted cancellation as active; that -asynchronous state no longer abandons the current-head dispatch or holds a -runner. A final live-authority read guards the POST. +runs. A refused cancellation, failed status lookup, invalid state, terminal +non-cancelled conclusion, or accepted cancellation that remains active after +the bounded poll fails closed without a new dispatch. A final live-authority +read guards the POST. + +The merge scheduler separately binds native concurrency to the exact PR head +and uses GitHub's bounded `queue: max` FIFO instead of lossy single-pending +replacement. `synchronize` and `closed` events run a metadata-only cleanup job +with `actions: write`; it inventories every active status, revalidates the live +PR/head immediately before each mutation, cancels only predecessor-head work +(or all final-head work on close), and accepts completion only after GitHub +reports `completed/cancelled`. Producer observation and POST are not atomic, so the receiver first authorizes the exact actor/sender pair, repository allowlist membership, and complete @@ -102,7 +110,10 @@ no-active-run dispatch, existing formal receipts, and a head movement between initial validation and the mutation boundary. Queue-contract tests prohibit lossy native receiver concurrency; execution fixtures prove exact older-head cancellation, deduplication, asynchronous cancellation continuation, and -fail-closure when a cancellation is refused or returns an invalid status. +fail-closure when a cancellation is refused, remains active, returns an invalid +state, or terminates with a non-cancelled conclusion. Scheduler cleanup +fixtures also prove concurrent-head-movement preservation and bounded terminal +cancellation verification. Receiver fixtures execute absent, active-owner, terminal-owner, self-rerun, different-head takeover, and branch-initialization-race lease paths. A two-process fixture starts simultaneous cache misses against an atomic fake @@ -117,12 +128,12 @@ independent byte-for-byte reviewer pin was regenerated from the repaired receiver as exact Git blob `5c87c74863ef6872c1ef7136d5b330071920c09e`. -Local focused verification on the stacked successor base -`5a91ce9f9c3e773aa1172f1055fd791ccde8fdaa`: +Local exact-tree verification on the stacked successor base +`813f16fad7528b035d6ae4386cd176c9a67413c1`: - required-workflow, nonblocking capacity, queue, receiver, and integrity-pin contracts after independent-review repair: `248 passed, 1 skipped`; -- complete Python 3.14 warnings-fatal suite: `5339 passed, 5 skipped, 40 +- complete Python 3.14 warnings-fatal suite: `5356 passed, 7 skipped, 40 subtests passed`, with owned production `18729/18729` statements and `7642/7642` branches, Docstring `100%`, and zero warnings. diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 24c6bda0f7..d08d9d2e4f 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -4,7 +4,7 @@ | Gap | Exact evidence | Action | Status | |---|---|---|---| -| A repeated required-check wake for unchanged `.github#2545@9a4af5e438283a31dc05814d6bc2818caee782a3` cancelled queued central dispatch `36776536447` when same-head run `36778773766` was created, resetting queue position without new source authority | [Run 36776536447](https://github.com/ContextualWisdomLab/.github/actions/runs/36776536447); [run 36778773766](https://github.com/ContextualWisdomLab/.github/actions/runs/36778773766); protected `main@37b10243cec3d160ecc9c1be75c71428b160a703` | At the central `.github` owner, inventory all five active admission states twice, validate run identity and fail closed on ambiguity, remove lossy native receiver concurrency, retire only canonical older-head central runs after live repository/PR/head validation, and continue current-head admission after an accepted asynchronous cancellation. Because producer inventory and POST are not atomic, authorize actor/sender/target and payload shape before OIDC, then acquire one repository/PR lease through the Contents API's blob-SHA compare-and-swap only after full live state/draft/base/head validation; active same-head losers terminate cheaply, self-reruns and terminal owners recover, and a freshly revalidated live head may replace a fully validated older-head owner. After lease acquisition or retention, revalidate live repository/PR/head authority and the trusted formal exact-head receipt; an existing receipt must return `admitted=false` before source materialization, coverage, or model execution, and an unavailable or malformed lookup fails closed. Isolate central `contents: write` in a dedicated lease job and keep later jobs read-only. After formal receipt, use repository-wide run inventory, bind PR number and `pull_requests[].head.sha` rather than the trusted-base run-level SHA, recursively partition the PR-lifetime `created` range below GitHub's 1,000-result ceiling, verify `total_count` completeness, and revalidate before each failed-job rerun so duplicate callbacks are unnecessary. See [RCA and executable acceptance](doctoring/opencode-same-head-dispatch-idempotency.md) | **Proposed / local RED→GREEN complete; exact-head hosted Checks and independent approval required** | +| A repeated required-check wake for unchanged `.github#2545@9a4af5e438283a31dc05814d6bc2818caee782a3` cancelled queued central dispatch `36776536447` when same-head run `36778773766` was created, resetting queue position without new source authority | [Run 36776536447](https://github.com/ContextualWisdomLab/.github/actions/runs/36776536447); [run 36778773766](https://github.com/ContextualWisdomLab/.github/actions/runs/36778773766); protected `main@37b10243cec3d160ecc9c1be75c71428b160a703` | At the central `.github` owner, inventory all five active admission states twice, validate run identity and fail closed on ambiguity, remove lossy native receiver concurrency, retire only canonical older-head central runs after live repository/PR/head validation, and require every accepted cancellation to reach `completed/cancelled` before current-head dispatch. Preserve merge-scheduler exact-head admissions with bounded `queue: max`; on `synchronize` or `closed`, a metadata-only job inventories active states, revalidates live PR/head authority before each mutation, retires only predecessor work, and proves terminal cancellation. Because producer inventory and POST are not atomic, authorize actor/sender/target and payload shape before OIDC, then acquire one repository/PR lease through the Contents API's blob-SHA compare-and-swap only after full live state/draft/base/head validation; active same-head losers terminate cheaply, self-reruns and terminal owners recover, and a freshly revalidated live head may replace a fully validated older-head owner. After lease acquisition or retention, revalidate live repository/PR/head authority and the trusted formal exact-head receipt; an existing receipt must return `admitted=false` before source materialization, coverage, or model execution, and an unavailable or malformed lookup fails closed. Isolate central `contents: write` in a dedicated lease job and keep later jobs read-only. After formal receipt, use repository-wide run inventory, bind PR number and `pull_requests[].head.sha` rather than the trusted-base run-level SHA, recursively partition the PR-lifetime `created` range below GitHub's 1,000-result ceiling, verify `total_count` completeness, and revalidate before each failed-job rerun so duplicate callbacks are unnecessary. See [RCA and executable acceptance](doctoring/opencode-same-head-dispatch-idempotency.md) | **Proposed / local RED→GREEN complete; exact-head hosted Checks and independent approval required** | ## 2026-10-01 Maturin response-lifecycle coverage closure diff --git a/tests/test_close_empty_pr_queue_pressure.py b/tests/test_close_empty_pr_queue_pressure.py index 6da88f63f1..5600a081c3 100644 --- a/tests/test_close_empty_pr_queue_pressure.py +++ b/tests/test_close_empty_pr_queue_pressure.py @@ -29,8 +29,19 @@ def test_closed_pull_request_does_not_allocate_a_noop_runner( assert "closed" in workflow assert "github.event.pull_request.number" in concurrency - assert "github.event.pull_request.head.sha" not in concurrency - assert re.search(r"(?m)^[ \t]+cancel-in-progress:[ \t]+\S", concurrency) - assert "cancel-closed-pr-runs:" not in workflow + if filename == "pr-review-merge-scheduler.yml": + assert "github.event.pull_request.head.sha" in concurrency + assert "queue: max" in concurrency + assert "cancel-in-progress:" not in concurrency + assert "cancel-superseded-pr-runs:" in workflow + cleanup = workflow.split(" cancel-superseded-pr-runs:", 1)[1].split( + " scan-pr-queue:", 1 + )[0] + assert "actions: write" in cleanup + assert "actions/checkout" not in cleanup + else: + assert "github.event.pull_request.head.sha" not in concurrency + assert re.search(r"(?m)^[ \t]+cancel-in-progress:[ \t]+\S", concurrency) + assert "cancel-closed-pr-runs:" not in workflow assert "github.event.action != 'closed'" in workflow assert evidence_job in workflow diff --git a/tests/test_current_head_coalescer_self_cancellation.py b/tests/test_current_head_coalescer_self_cancellation.py index e49193d4df..55de2228f2 100644 --- a/tests/test_current_head_coalescer_self_cancellation.py +++ b/tests/test_current_head_coalescer_self_cancellation.py @@ -9,8 +9,8 @@ ) -def test_current_head_coalescer_shares_pr_scoped_scheduler_admission() -> None: - """The integrated step reuses PR-scoped scheduler admission and its runner.""" +def test_current_head_coalescer_preserves_exact_head_scheduler_admission() -> None: + """The scheduler queues one exact head and retires predecessors explicitly.""" workflow_text = WORKFLOW_PATH.read_text(encoding="utf-8") coalescer = workflow_text.split("\n scan-pr-queue:\n", 1)[1] concurrency_block = workflow_text.split("\nconcurrency:\n", 1)[1].split( @@ -24,11 +24,18 @@ def test_current_head_coalescer_shares_pr_scoped_scheduler_admission() -> None: assert "Retire redundant queued exact-head runs" in coalescer assert "github.repository == 'ContextualWisdomLab/.github'" in coalescer - assert "github.event.pull_request.head.sha" not in concurrency_block + assert "github.event.pull_request.head.sha" in concurrency_block + assert "github.event.client_payload.pr_head_sha" in concurrency_block assert "github.event.pull_request.number" in concurrency_block - assert any( - line.startswith("cancel-in-progress:") - and "github.event_name == 'pull_request_target'" in line - for line in active_lines - ) - assert "queue: max" not in workflow_text + assert not any(line.startswith("cancel-in-progress:") for line in active_lines) + assert "queue: max" in concurrency_block + + cleanup = workflow_text.split("\n cancel-superseded-pr-runs:\n", 1)[1].split( + "\n scan-pr-queue:\n", 1 + )[0] + assert "github.event.action == 'synchronize'" in cleanup + assert "github.event.action == 'closed'" in cleanup + assert "actions: write" in cleanup + assert "actions/checkout" not in cleanup + assert "live_target_matches" in cleanup + assert "did not reach completed/cancelled" in cleanup diff --git a/tests/test_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index 148013e502..0bcc16d49a 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -2484,10 +2484,13 @@ def test_merge_scheduler_uses_escalating_mutation_credentials(): assert 'check_delay="$((check_attempt * 2))"' in workflow assert "steps.review_followup.outputs.proceed != 'false'" in workflow assert "Native events and the explicit org-sweep recovery remain authoritative." in workflow - assert ( - "github.event_name == 'pull_request_review' || " - "github.event_name == 'repository_dispatch'" in workflow - ) + concurrency = workflow.split("\nconcurrency:\n", 1)[1].split( + "\npermissions:\n", 1 + )[0] + assert "queue: max" in concurrency + assert "cancel-in-progress:" not in concurrency + assert "github.event.pull_request.head.sha" in concurrency + assert "github.event.client_payload.pr_head_sha" in concurrency def test_opencode_runs_merge_scheduler_after_review_without_repo_local_dispatch(): diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index e78a3fa2e4..f84155e1a0 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -1156,7 +1156,10 @@ def test_fail_closed_step_checks_once_for_a_non_draft_pr(tmp_path: Path) -> None ([], [active_dispatch(status="queued", head_sha="d" * 40)], None, HEAD, False, 0, 1), ([], [active_dispatch(status="queued", head_sha="d" * 40, run_id=45)], None, HEAD, False, 1, 0), ([], [active_dispatch(status="queued", head_sha="d" * 40, run_id=46)], None, HEAD, False, 1, 0), - ([], [active_dispatch(status="queued", head_sha="d" * 40, run_id=47)], None, HEAD, False, 0, 1), + ([], [active_dispatch(status="queued", head_sha="d" * 40, run_id=47)], None, HEAD, False, 1, 0), + ([], [active_dispatch(status="queued", head_sha="d" * 40, run_id=49)], None, HEAD, False, 0, 1), + ([], [active_dispatch(status="queued", head_sha="d" * 40, run_id=50)], None, HEAD, False, 1, 0), + ([], [active_dispatch(status="queued", head_sha="d" * 40, run_id=51)], None, "e" * 40, False, 0, 0), ([], [], [active_dispatch(status="pending")], HEAD, False, 0, 0), ( [], @@ -1230,12 +1233,21 @@ def test_scheduler_wake_reuses_trusted_receipt_predicate( elif [[ "$*" == *"repos/ContextualWisdomLab/.github/actions/runs/"* ]]; then [[ "$*" =~ actions/runs/([0-9]+) ]] || exit 91 run_id="${BASH_REMATCH[1]}" + count_file="$RUN_STATE_CALLS/$run_id" + count=0 + [[ ! -f "$count_file" ]] || count="$(cat "$count_file")" + count=$((count + 1)) + printf '%s' "$count" >"$count_file" if [[ "$run_id" == "46" ]]; then - printf 'mystery' + printf 'mystery\t' elif [[ "$run_id" == "47" ]]; then - printf 'queued' + printf 'queued\t' + elif [[ "$run_id" == "49" && "$count" -eq 1 ]]; then + printf 'in_progress\t' + elif [[ "$run_id" == "50" ]]; then + printf 'completed\tsuccess' else - printf 'completed' + printf 'completed\tcancelled' fi elif [[ "$*" == *"repos/ContextualWisdomLab/.github/dispatches"* ]]; then cat >/dev/null @@ -1253,6 +1265,11 @@ def test_scheduler_wake_reuses_trusted_receipt_predicate( encoding="utf-8", ) fake_curl.chmod(0o755) + fake_sleep = fake_bin / "sleep" + fake_sleep.write_text("#!/usr/bin/env bash\nexit 0\n", encoding="utf-8") + fake_sleep.chmod(0o755) + run_state_calls = tmp_path / "run-state-calls" + run_state_calls.mkdir() env = { **os.environ, "PATH": f"{fake_bin}{os.pathsep}{os.environ['PATH']}", @@ -1266,6 +1283,7 @@ def test_scheduler_wake_reuses_trusted_receipt_predicate( "ACTIVE_RUN_CALLS": str(tmp_path / "active-run-calls"), "CANCEL_CALLS": str(tmp_path / "cancel-calls"), "DISPATCH_CALLS": str(calls), + "RUN_STATE_CALLS": str(run_state_calls), "ACTIONS_ID_TOKEN_REQUEST_TOKEN": "request", "ACTIONS_ID_TOKEN_REQUEST_URL": "https://token.example", "OIDC_AUDIENCE": "opencode-github-action", @@ -1300,17 +1318,21 @@ def test_scheduler_wake_reuses_trusted_receipt_predicate( if cancel_calls.exists() else [] ) - expected_cancel_ids = sorted( - { - str(run["id"]) - for run in [*active_runs, *(later_active_runs or [])] - if isinstance(run.get("display_title"), str) - and str(run["display_title"]).startswith( - "OpenCode Review Dispatch owner/repo#7@" - ) - and str(run["display_title"]).lower() - != f"OpenCode Review Dispatch owner/repo#7@{HEAD}".lower() - } + expected_cancel_ids = ( + [] + if revalidated_head != HEAD + else sorted( + { + str(run["id"]) + for run in [*active_runs, *(later_active_runs or [])] + if isinstance(run.get("display_title"), str) + and str(run["display_title"]).startswith( + "OpenCode Review Dispatch owner/repo#7@" + ) + and str(run["display_title"]).lower() + != f"OpenCode Review Dispatch owner/repo#7@{HEAD}".lower() + } + ) ) assert actual_cancel_ids == expected_cancel_ids diff --git a/tests/test_opencode_workflow_shell_syntax.py b/tests/test_opencode_workflow_shell_syntax.py index 3e30633eb7..b18b207ca4 100644 --- a/tests/test_opencode_workflow_shell_syntax.py +++ b/tests/test_opencode_workflow_shell_syntax.py @@ -262,6 +262,7 @@ def test_merge_scheduler_targeted_dispatch_validates_live_exact_pr(tmp_path): "DEFAULT_BRANCH": "main", "TARGET_REPOSITORY_INPUT": "ContextualWisdomLab/naruon", "TARGET_PR_NUMBER": "1179", + "TARGET_HEAD_SHA_INPUT": "4afd4af7ad343660356791873d940aa2846f40c2", "TARGET_BASE_BRANCH_INPUT": "develop", "ALLOWED_TARGET_REPOSITORIES": ( "ContextualWisdomLab/.github, ContextualWisdomLab/naruon" @@ -302,6 +303,22 @@ def test_merge_scheduler_targeted_dispatch_validates_live_exact_pr(tmp_path): assert "absent from the configured exact allowlist" in rejected.stdout assert not output.exists() + stale_head_env = { + **env, + "TARGET_HEAD_SHA_INPUT": "a" * 40, + } + stale_head = subprocess.run( + [bash], + input=script, + text=True, + capture_output=True, + check=False, + env=stale_head_env, + ) + assert stale_head.returncode == 1 + assert "head does not match the live PR" in stale_head.stdout + assert not output.exists() + output.unlink(missing_ok=True) cross_repo_pull = { **pull, diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index 5bfd102f48..18adf853fb 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -37,7 +37,10 @@ def test_central_dispatch_and_control_jobs_use_dedicated_groups() -> None: def test_reusable_scheduler_keeps_consumer_runner_access() -> None: """Reusable trusted schedulers share control capacity without PR execution.""" text = workflow_text("pr-review-merge-scheduler.yml") - selector = next(line for line in text.splitlines() if line.strip().startswith("runs-on:")) + scan_job = text.split("\n scan-pr-queue:\n", 1)[1] + selector = next( + line for line in scan_job.splitlines() if line.strip().startswith("runs-on:") + ) assert selector.strip() == "runs-on:" assert " runs-on:\n group: CWL central control\n labels: [self-hosted, linux, x64]" in text assert "fromJSON" not in selector @@ -251,12 +254,23 @@ def test_merge_scheduler_uses_native_auto_merge_after_required_checks() -> None: assert "github.event_name == 'repository_dispatch' && github.run_id" not in ( concurrency_contract ) - # Anchored, not a substring: this workflow's value is an expression rather - # than a constant, so it cannot use the boolean helper, but a commented-out - # setting must not satisfy it either. - assert re.search(r"(?m)^[ \t]+cancel-in-progress:[ \t]+\$\{\{", concurrency_contract) + assert "queue: max" in concurrency_contract + assert "cancel-in-progress:" not in concurrency_contract + assert "github.event.pull_request.head.sha" in concurrency_contract + assert "github.event.client_payload.pr_head_sha" in concurrency_contract assert "github.event_name == 'repository_dispatch'" in concurrency_contract + cleanup_job = workflow.split(" cancel-superseded-pr-runs:", 1)[1].split( + " scan-pr-queue:", 1 + )[0] + assert "actions: write" in cleanup_job + assert "actions/checkout" not in cleanup_job + assert "github.event.pull_request.number" in cleanup_job + assert "github.event.pull_request.head.sha" in cleanup_job + assert ".pull_requests[]?" in cleanup_job + assert ".head.sha" in cleanup_job + assert "force-cancel" in cleanup_job + def test_merge_scheduler_provides_same_repository_dispatch_credential() -> None: """Guard the runner-token dispatch credential for central review workflows. @@ -1122,9 +1136,23 @@ def test_pull_request_close_events_cancel_superseded_runs_without_heavy_jobs() - assert "actions: write" in cleanup_job assert "actions/checkout" not in cleanup_job assert "cleanup skipped" not in cleanup_job + elif filename == "pr-review-merge-scheduler.yml": + assert "cancel-closed-pr-runs:" not in workflow + concurrency_contract = workflow.split("concurrency:", 1)[1].split( + "permissions:", 1 + )[0] + assert "github.event.pull_request.number" in concurrency_contract + assert "github.event.pull_request.head.sha" in concurrency_contract + assert "queue: max" in concurrency_contract + assert "cancel-in-progress:" not in concurrency_contract + assert "cancel-superseded-pr-runs:" in workflow + cleanup_job = workflow.split( + " cancel-superseded-pr-runs:", 1 + )[1].split(" scan-pr-queue:", 1)[0] + assert "actions: write" in cleanup_job + assert "actions/checkout" not in cleanup_job elif filename in { "codeql-pr.yml", - "pr-review-merge-scheduler.yml", "python-security.yml", "sast-semgrep.yml", "security-scan.yml", @@ -1508,6 +1536,136 @@ def test_merge_scheduler_has_no_workflow_run_trigger() -> None: assert "workflow_run:" not in workflow.split("workflow_call:", 1)[0] +def _run_merge_scheduler_cleanup( + tmp_path: Path, + pull_states: list[dict[str, object]], + run_states: list[dict[str, object]], +) -> tuple[subprocess.CompletedProcess[str], str]: + """Execute predecessor cleanup against stateful GitHub API fixtures.""" + if shutil.which("jq") is None: + pytest.skip("jq is required to execute the production cleanup") + step = workflow_step( + workflow_text("pr-review-merge-scheduler.yml"), + "Cancel revalidated predecessor scheduler runs", + ) + run_block = step.split(" run: |\n", 1)[1].split( + "\n scan-pr-queue:", 1 + )[0] + script = textwrap.dedent(run_block) + fake_bin = tmp_path / "bin" + 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( + "\n".join(json.dumps(state) for state in run_states) + "\n", + encoding="utf-8", + ) + fake_gh = fake_bin / "gh" + fake_gh.write_text( + '''#!/usr/bin/env bash +set -euo pipefail +printf '%s\n' "$*" >>"$FAKE_CALLS" +next_line() { + local source="$1" count_file="${1}.count" count=0 + [[ ! -f "$count_file" ]] || count="$(cat "$count_file")" + count=$((count + 1)) + printf '%s' "$count" >"$count_file" + sed -n "${count}p" "$source" +} +if [[ "$*" == *"/pulls/7"* ]]; then + next_line "$FAKE_PULLS" +elif [[ "$*" == *"actions/workflows/pr-review-merge-scheduler.yml/runs"* ]]; then + printf '%s\n' '[{"workflow_runs":[{"id":100,"status":"queued","pull_requests":[{"number":7,"head":{"sha":"bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"}}]}]}]' +elif [[ "$*" == *"actions/runs/100/force-cancel"* ]]; then + exit 0 +elif [[ "$*" == *"actions/runs/100"* ]]; then + state="$(next_line "$FAKE_RUNS")" + jq -r '[.status // "", .conclusion // ""] | @tsv' <<<"$state" +else + exit 1 +fi +''', + encoding="utf-8", + ) + fake_gh.chmod(0o755) + result = subprocess.run( + ["bash", "-c", script], + env={ + **os.environ, + "PATH": f"{fake_bin}{os.pathsep}{os.environ['PATH']}", + "FAKE_CALLS": str(calls), + "FAKE_PULLS": str(pulls), + "FAKE_RUNS": str(runs), + "GH_TOKEN": "synthetic-actions-token", + "GITHUB_RUN_ID": "999", + "TARGET_REPOSITORY": "owner/repo", + "TARGET_PR_NUMBER": "7", + "TARGET_PR_HEAD_SHA": "a" * 40, + "TARGET_ACTION": "synchronize", + }, + capture_output=True, + text=True, + check=False, + ) + return result, calls.read_text(encoding="utf-8") + + +def _live_scheduler_pull(*, head_sha: str = "a" * 40) -> dict[str, object]: + """Build one live pull-request response for scheduler cleanup evidence.""" + return { + "base": {"repo": {"full_name": "owner/repo"}}, + "number": 7, + "state": "open", + "head": {"sha": head_sha}, + } + + +def test_scheduler_cleanup_revalidates_target_after_run_selection(tmp_path: Path) -> None: + """A concurrent head advance after selection prevents cancellation.""" + result, calls = _run_merge_scheduler_cleanup( + tmp_path, + [_live_scheduler_pull(), _live_scheduler_pull(head_sha="c" * 40)], + [{"status": "completed", "conclusion": "cancelled"}], + ) + assert result.returncode == 0, result.stderr + assert calls.count("/pulls/7") == 2 + assert "/actions/runs/100/force-cancel" not in calls + + +def test_scheduler_cleanup_fails_when_accepted_cancel_never_finishes( + tmp_path: Path, +) -> None: + """An accepted POST is not terminal cancellation evidence.""" + result, calls = _run_merge_scheduler_cleanup( + tmp_path, + [_live_scheduler_pull()] * 8, + [{"status": "in_progress", "conclusion": None}] * 6, + ) + assert result.returncode == 1 + assert calls.count("actions/runs/100 --jq") == 6 + assert "did not reach completed/cancelled" in result.stdout + + +def test_scheduler_cleanup_verifies_accepted_cancelled_state(tmp_path: Path) -> None: + """Finish only after GitHub reports completed/cancelled.""" + result, calls = _run_merge_scheduler_cleanup( + tmp_path, + [_live_scheduler_pull()] * 8, + [ + {"status": "in_progress", "conclusion": None}, + {"status": "completed", "conclusion": "cancelled"}, + ], + ) + assert result.returncode == 0, result.stderr + assert calls.count("actions/runs/100 --jq") == 2 + assert "Verified cancelled scheduler run 100." in result.stdout + + def test_review_events_can_dispatch_after_threads_are_resolved() -> None: """Let the scheduler dispatch OpenCode when a review event clears its last blocker.""" workflow = workflow_text("pr-review-merge-scheduler.yml") From a44bbefeb7a0db0d85ebddba1f8001d691f9d542 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 1 Oct 2026 14:01:20 +0900 Subject: [PATCH 04/10] fix(scheduler): complete active-run inventory --- .../workflows/pr-review-merge-scheduler.yml | 36 ++++++++-- ...opencode-same-head-dispatch-idempotency.md | 7 +- CHANGELOG.md | 3 +- ...in-download-response-lifecycle-20261001.md | 10 ++- ...opencode-same-head-dispatch-idempotency.md | 23 +++--- docs/product-technical-gap-baseline.md | 2 +- .../test_required_workflow_queue_contract.py | 71 ++++++++++++++++++- ...test_verify_release_maturin_tool_assets.py | 12 ++-- 8 files changed, 137 insertions(+), 27 deletions(-) diff --git a/.github/workflows/pr-review-merge-scheduler.yml b/.github/workflows/pr-review-merge-scheduler.yml index f42f062799..5473e33f6f 100644 --- a/.github/workflows/pr-review-merge-scheduler.yml +++ b/.github/workflows/pr-review-merge-scheduler.yml @@ -161,13 +161,35 @@ jobs: inventory_dir="$(mktemp -d)" trap 'rm -rf "$inventory_dir"' EXIT : >"$inventory_dir/active-arrays.jsonl" - for active_status in requested waiting pending queued in_progress; do - gh api --paginate --slurp \ - "repos/${TARGET_REPOSITORY}/actions/workflows/pr-review-merge-scheduler.yml/runs?event=pull_request_target&status=${active_status}&per_page=100" \ - >"$inventory_dir/${active_status}.json" - jq -c '[.[] | .workflow_runs[]?]' \ - "$inventory_dir/${active_status}.json" \ - >>"$inventory_dir/active-arrays.jsonl" + for inventory_pass in 1 2; do + for active_status in requested waiting pending queued in_progress; do + inventory_path="$inventory_dir/${inventory_pass}-${active_status}.json" + gh api --paginate --slurp \ + "repos/${TARGET_REPOSITORY}/actions/workflows/pr-review-merge-scheduler.yml/runs?event=pull_request_target&status=${active_status}&per_page=100" \ + >"$inventory_path" + if ! inventory_counts="$(jq -er ' + if type == "array" and length > 0 + and all(.[]; (.total_count | type) == "number") + and all(.[]; (.workflow_runs | type) == "array") + then + [([.[] | .workflow_runs[]] | length), (map(.total_count) | max)] + | @tsv + else + error("invalid workflow-run inventory") + end + ' "$inventory_path")"; then + echo "::error::Scheduler workflow-run inventory was invalid." + exit 1 + fi + IFS=$'\t' read -r collected_count expected_count <<<"$inventory_counts" + if [ "$collected_count" -ne "$expected_count" ]; then + echo "::error::Scheduler workflow-run inventory was incomplete for ${active_status}: expected=${expected_count} collected=${collected_count}." + exit 1 + fi + jq -c '[.[] | .workflow_runs[]]' \ + "$inventory_path" \ + >>"$inventory_dir/active-arrays.jsonl" + done done workflow_runs="$(jq -sc 'add // [] | unique_by(.id)' "$inventory_dir/active-arrays.jsonl")" mapfile -t superseded_run_ids < <( diff --git a/CHANGELOG.d/20261001-opencode-same-head-dispatch-idempotency.md b/CHANGELOG.d/20261001-opencode-same-head-dispatch-idempotency.md index f239cb182f..d9bac48a92 100644 --- a/CHANGELOG.d/20261001-opencode-same-head-dispatch-idempotency.md +++ b/CHANGELOG.d/20261001-opencode-same-head-dispatch-idempotency.md @@ -9,6 +9,7 @@ exact-head review receipt again so a completed prior receiver cannot trigger duplicate coverage or model execution. Preserve every exact-head merge-scheduler admission with `queue: max`; a - dedicated metadata-only cleanup retires only live-head-revalidated - predecessor runs and proves each accepted cancellation reaches - `completed/cancelled`. + dedicated metadata-only cleanup inventories every active state twice, + rejects incomplete GitHub search results, retires only + live-head-revalidated predecessor runs, and proves each accepted + cancellation reaches `completed/cancelled`. diff --git a/CHANGELOG.md b/CHANGELOG.md index 43c23fa330..b2bccf21c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,7 +29,8 @@ - Refactor the bounded Maturin asset downloader so successful and rejected responses share one unconditional close path while `HTTPError` keeps its own explicit close path. A new regression exercises a non-200 response and an - opener-raised HTTP error. This removes an impossible optional-response branch + opener-raised HTTP error through a close-observing body without replacing the + error's real `close()` method. This removes an impossible optional-response branch without changing hosts, redirects, byte limits, hashes, or fail-closed error mapping; the focused suite is 17 passed with 100% statement and branch coverage. The trusted full-suite workflow now also tracks the verifier source diff --git a/docs/doctoring/maturin-download-response-lifecycle-20261001.md b/docs/doctoring/maturin-download-response-lifecycle-20261001.md index b6765daae8..cfe20cc5c6 100644 --- a/docs/doctoring/maturin-download-response-lifecycle-20261001.md +++ b/docs/doctoring/maturin-download-response-lifecycle-20261001.md @@ -27,7 +27,11 @@ still produced no gate because the workflow admitted only PRs whose base was The downloader now closes every returned response in one unconditional nested `finally` block. An opener-raised `HTTPError` remains independently closed by its handler. Tests assert closure for both an HTTP 503 response and an HTTP 502 -exception. A workflow contract now requires both verifier paths in pull-request +exception. The error fixture preserves the real `HTTPError.close()` and observes +its body closing; replacing the method with a spy had suppressed the actual +close and leaked pytest's temporary capture object until a later test, where +warnings-fatal execution correctly rejected the `ResourceWarning`. A workflow +contract now requires both verifier paths in pull-request and protected-branch triggers, and a separate contract admits stacked PR bases while the push trigger remains restricted to protected `main`. The repair does not change admitted hosts, the one-hop redirect @@ -37,7 +41,9 @@ contract, credentials, request timeout, byte bounds, digests, or error mapping. - Hosted RED: 5,314 passed, 5 skipped, 40 subtests; 18,775 statements with 5 missing, 7,652 branches with 2 partial; total 99%. -- Local owner GREEN: 17 focused tests; 107/107 statements and 34/34 branches; +- Local integrated GREEN: 18 lifecycle/prescreen tests; complete warnings-fatal + suite 5,362 passed, 5 skipped, 40 subtests; 18,767/18,767 statements and + 7,648/7,648 branches; verifier 107/107 statements and 34/34 branches; `git diff --check` clean. - Required before acceptance: complete exact-head hosted suite, security and CodeQL verdicts, qualifying independent approval, ordinary owner integration, diff --git a/docs/doctoring/opencode-same-head-dispatch-idempotency.md b/docs/doctoring/opencode-same-head-dispatch-idempotency.md index 7fc996b682..c4e7b91d69 100644 --- a/docs/doctoring/opencode-same-head-dispatch-idempotency.md +++ b/docs/doctoring/opencode-same-head-dispatch-idempotency.md @@ -44,8 +44,11 @@ inventory. Before a current-head POST, the required workflow revalidates live repository/PR/head authority and cancels only canonical older-head central runs. A refused cancellation, failed status lookup, invalid state, terminal non-cancelled conclusion, or accepted cancellation that remains active after -the bounded poll fails closed without a new dispatch. A final live-authority -read guards the POST. +the bounded status reads fails closed without a new dispatch. Those reads do +not sleep inside the required job: the nonblocking capacity contract releases +the runner and lets a later trusted scheduler admission retry instead of +holding scarce capacity while GitHub converges. A final live-authority read +guards the POST. The merge scheduler separately binds native concurrency to the exact PR head and uses GitHub's bounded `queue: max` FIFO instead of lossy single-pending @@ -53,7 +56,10 @@ replacement. `synchronize` and `closed` events run a metadata-only cleanup job with `actions: write`; it inventories every active status, revalidates the live PR/head immediately before each mutation, cancels only predecessor-head work (or all final-head work on close), and accepts completion only after GitHub -reports `completed/cancelled`. +reports `completed/cancelled`. Two complete status passes prevent a state +transition from escaping between filtered queries; every response reconciles +the collected row count with `total_count` and fails closed if GitHub's +filtered-search ceiling truncates the inventory. Producer observation and POST are not atomic, so the receiver first authorizes the exact actor/sender pair, repository allowlist membership, and complete @@ -112,7 +118,8 @@ lossy native receiver concurrency; execution fixtures prove exact older-head cancellation, deduplication, asynchronous cancellation continuation, and fail-closure when a cancellation is refused, remains active, returns an invalid state, or terminates with a non-cancelled conclusion. Scheduler cleanup -fixtures also prove concurrent-head-movement preservation and bounded terminal +fixtures also prove concurrent-head-movement preservation, transition-safe +two-pass discovery, inventory-completeness rejection, and bounded terminal cancellation verification. Receiver fixtures execute absent, active-owner, terminal-owner, self-rerun, different-head takeover, and branch-initialization-race lease paths. @@ -129,13 +136,13 @@ receiver as exact Git blob `5c87c74863ef6872c1ef7136d5b330071920c09e`. Local exact-tree verification on the stacked successor base -`813f16fad7528b035d6ae4386cd176c9a67413c1`: +`86ddef63ed306d4c7d56d051d9a72570e7d358a5`: - required-workflow, nonblocking capacity, queue, receiver, and integrity-pin - contracts after independent-review repair: `248 passed, 1 skipped`; -- complete Python 3.14 warnings-fatal suite: `5356 passed, 7 skipped, 40 + contracts after independent-review repair: `233 passed`; +- complete Python 3.14 warnings-fatal suite: `5362 passed, 5 skipped, 40 subtests passed`, with - owned production `18729/18729` statements and `7642/7642` branches, + owned production `18767/18767` statements and `7648/7648` branches, Docstring `100%`, and zero warnings. Protected merge still requires hosted exact-head security and quality Checks, diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index d08d9d2e4f..ecfe86f976 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -4,7 +4,7 @@ | Gap | Exact evidence | Action | Status | |---|---|---|---| -| A repeated required-check wake for unchanged `.github#2545@9a4af5e438283a31dc05814d6bc2818caee782a3` cancelled queued central dispatch `36776536447` when same-head run `36778773766` was created, resetting queue position without new source authority | [Run 36776536447](https://github.com/ContextualWisdomLab/.github/actions/runs/36776536447); [run 36778773766](https://github.com/ContextualWisdomLab/.github/actions/runs/36778773766); protected `main@37b10243cec3d160ecc9c1be75c71428b160a703` | At the central `.github` owner, inventory all five active admission states twice, validate run identity and fail closed on ambiguity, remove lossy native receiver concurrency, retire only canonical older-head central runs after live repository/PR/head validation, and require every accepted cancellation to reach `completed/cancelled` before current-head dispatch. Preserve merge-scheduler exact-head admissions with bounded `queue: max`; on `synchronize` or `closed`, a metadata-only job inventories active states, revalidates live PR/head authority before each mutation, retires only predecessor work, and proves terminal cancellation. Because producer inventory and POST are not atomic, authorize actor/sender/target and payload shape before OIDC, then acquire one repository/PR lease through the Contents API's blob-SHA compare-and-swap only after full live state/draft/base/head validation; active same-head losers terminate cheaply, self-reruns and terminal owners recover, and a freshly revalidated live head may replace a fully validated older-head owner. After lease acquisition or retention, revalidate live repository/PR/head authority and the trusted formal exact-head receipt; an existing receipt must return `admitted=false` before source materialization, coverage, or model execution, and an unavailable or malformed lookup fails closed. Isolate central `contents: write` in a dedicated lease job and keep later jobs read-only. After formal receipt, use repository-wide run inventory, bind PR number and `pull_requests[].head.sha` rather than the trusted-base run-level SHA, recursively partition the PR-lifetime `created` range below GitHub's 1,000-result ceiling, verify `total_count` completeness, and revalidate before each failed-job rerun so duplicate callbacks are unnecessary. See [RCA and executable acceptance](doctoring/opencode-same-head-dispatch-idempotency.md) | **Proposed / local RED→GREEN complete; exact-head hosted Checks and independent approval required** | +| A repeated required-check wake for unchanged `.github#2545@9a4af5e438283a31dc05814d6bc2818caee782a3` cancelled queued central dispatch `36776536447` when same-head run `36778773766` was created, resetting queue position without new source authority | [Run 36776536447](https://github.com/ContextualWisdomLab/.github/actions/runs/36776536447); [run 36778773766](https://github.com/ContextualWisdomLab/.github/actions/runs/36778773766); protected `main@37b10243cec3d160ecc9c1be75c71428b160a703` | At the central `.github` owner, inventory all five active admission states twice, validate run identity and fail closed on ambiguity, remove lossy native receiver concurrency, retire only canonical older-head central runs after live repository/PR/head validation, and require every accepted cancellation to reach `completed/cancelled` before current-head dispatch. Preserve merge-scheduler exact-head admissions with bounded `queue: max`; on `synchronize` or `closed`, a metadata-only job inventories every active state twice, rejects invalid or `total_count`-incomplete results, revalidates live PR/head authority before each mutation, retires only predecessor work, and proves terminal cancellation. Because producer inventory and POST are not atomic, authorize actor/sender/target and payload shape before OIDC, then acquire one repository/PR lease through the Contents API's blob-SHA compare-and-swap only after full live state/draft/base/head validation; active same-head losers terminate cheaply, self-reruns and terminal owners recover, and a freshly revalidated live head may replace a fully validated older-head owner. After lease acquisition or retention, revalidate live repository/PR/head authority and the trusted formal exact-head receipt; an existing receipt must return `admitted=false` before source materialization, coverage, or model execution, and an unavailable or malformed lookup fails closed. Isolate central `contents: write` in a dedicated lease job and keep later jobs read-only. After formal receipt, use repository-wide run inventory, bind PR number and `pull_requests[].head.sha` rather than the trusted-base run-level SHA, recursively partition the PR-lifetime `created` range below GitHub's 1,000-result ceiling, verify `total_count` completeness, and revalidate before each failed-job rerun so duplicate callbacks are unnecessary. See [RCA and executable acceptance](doctoring/opencode-same-head-dispatch-idempotency.md) | **Proposed / local RED→GREEN complete; exact-head hosted Checks and independent approval required** | ## 2026-10-01 Maturin response-lifecycle coverage closure diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index 18adf853fb..84549be9d9 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -1540,6 +1540,7 @@ def _run_merge_scheduler_cleanup( tmp_path: Path, pull_states: list[dict[str, object]], run_states: list[dict[str, object]], + inventory_responses: list[dict[str, object]] | None = None, ) -> tuple[subprocess.CompletedProcess[str], str]: """Execute predecessor cleanup against stateful GitHub API fixtures.""" if shutil.which("jq") is None: @@ -1557,6 +1558,7 @@ def _run_merge_scheduler_cleanup( calls = tmp_path / "calls" pulls = tmp_path / "pulls" runs = tmp_path / "runs" + inventories = tmp_path / "inventories" pulls.write_text( "\n".join(json.dumps(state) for state in pull_states) + "\n", encoding="utf-8", @@ -1565,6 +1567,27 @@ def _run_merge_scheduler_cleanup( "\n".join(json.dumps(state) for state in run_states) + "\n", encoding="utf-8", ) + if inventory_responses is None: + default_inventory = { + "total_count": 1, + "workflow_runs": [ + { + "id": 100, + "status": "queued", + "pull_requests": [ + { + "number": 7, + "head": {"sha": "b" * 40}, + } + ], + } + ], + } + inventory_responses = [default_inventory] * 10 + inventories.write_text( + "\n".join(json.dumps(response) for response in inventory_responses) + "\n", + encoding="utf-8", + ) fake_gh = fake_bin / "gh" fake_gh.write_text( '''#!/usr/bin/env bash @@ -1580,7 +1603,7 @@ def _run_merge_scheduler_cleanup( if [[ "$*" == *"/pulls/7"* ]]; then next_line "$FAKE_PULLS" elif [[ "$*" == *"actions/workflows/pr-review-merge-scheduler.yml/runs"* ]]; then - printf '%s\n' '[{"workflow_runs":[{"id":100,"status":"queued","pull_requests":[{"number":7,"head":{"sha":"bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"}}]}]}]' + printf '[%s]\n' "$(next_line "$FAKE_INVENTORIES")" elif [[ "$*" == *"actions/runs/100/force-cancel"* ]]; then exit 0 elif [[ "$*" == *"actions/runs/100"* ]]; then @@ -1601,6 +1624,7 @@ def _run_merge_scheduler_cleanup( "FAKE_CALLS": str(calls), "FAKE_PULLS": str(pulls), "FAKE_RUNS": str(runs), + "FAKE_INVENTORIES": str(inventories), "GH_TOKEN": "synthetic-actions-token", "GITHUB_RUN_ID": "999", "TARGET_REPOSITORY": "owner/repo", @@ -1666,6 +1690,51 @@ def test_scheduler_cleanup_verifies_accepted_cancelled_state(tmp_path: Path) -> assert "Verified cancelled scheduler run 100." in result.stdout +def test_scheduler_cleanup_second_inventory_pass_catches_state_transition( + tmp_path: Path, +) -> None: + """A run moving between filtered states remains visible on the second pass.""" + empty_inventory = {"total_count": 0, "workflow_runs": []} + transitioned_inventory = { + "total_count": 1, + "workflow_runs": [ + { + "id": 100, + "status": "in_progress", + "pull_requests": [ + {"number": 7, "head": {"sha": "b" * 40}} + ], + } + ], + } + result, calls = _run_merge_scheduler_cleanup( + tmp_path, + [_live_scheduler_pull()] * 8, + [{"status": "completed", "conclusion": "cancelled"}], + [empty_inventory] * 5 + + [empty_inventory] * 4 + + [transitioned_inventory], + ) + assert result.returncode == 0, result.stderr + assert calls.count("actions/workflows/pr-review-merge-scheduler.yml/runs") == 10 + assert "/actions/runs/100/force-cancel" in calls + + +def test_scheduler_cleanup_fails_closed_on_truncated_inventory( + tmp_path: Path, +) -> None: + """Never treat GitHub's filtered-search ceiling as a complete snapshot.""" + result, calls = _run_merge_scheduler_cleanup( + tmp_path, + [_live_scheduler_pull()], + [{"status": "completed", "conclusion": "cancelled"}], + [{"total_count": 1001, "workflow_runs": []}], + ) + assert result.returncode == 1 + assert "inventory was incomplete" in result.stdout + assert "/force-cancel" not in calls + + def test_review_events_can_dispatch_after_threads_are_resolved() -> None: """Let the scheduler dispatch OpenCode when a review event clears its last blocker.""" workflow = workflow_text("pr-review-merge-scheduler.yml") diff --git a/tests/test_verify_release_maturin_tool_assets.py b/tests/test_verify_release_maturin_tool_assets.py index 6aa84a0827..6dd581eea0 100644 --- a/tests/test_verify_release_maturin_tool_assets.py +++ b/tests/test_verify_release_maturin_tool_assets.py @@ -201,11 +201,14 @@ def open(self, _request, timeout): verifier._download("maturin-x86_64-pc-windows-msvc.zip") assert closed == ["response"] + class ErrorBody(io.BytesIO): + def close(self): + closed.append("http-error") + super().close() + + error_body = ErrorBody() transport_error = urllib.error.HTTPError( - "https://github.com/asset", 502, "Bad Gateway", {}, io.BytesIO() - ) - monkeypatch.setattr( - transport_error, "close", lambda: closed.append("http-error") + "https://github.com/asset", 502, "Bad Gateway", {}, error_body ) class ErrorOpener: @@ -221,6 +224,7 @@ def open(self, _request, timeout): with pytest.raises(ValueError, match="HTTP 502"): verifier._download("maturin-x86_64-pc-windows-msvc.zip") assert closed == ["response", "http-error"] + assert error_body.closed def test_maturin_download_rejects_unlisted_name_before_network(monkeypatch): From 3a7d92eccb5074bfb9794702835577bb2146c1d4 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 1 Oct 2026 14:40:39 +0900 Subject: [PATCH 05/10] fix(scheduler): retire review-event predecessors --- .../workflows/pr-review-merge-scheduler.yml | 10 ++++--- .../trusted-uv-materializer-quality-ci.yml | 2 +- AGENTS.md | 21 +++++++++------ ...opencode-same-head-dispatch-idempotency.md | 5 ++-- CHANGELOG.md | 12 ++++++--- ...in-download-response-lifecycle-20261001.md | 10 +++++-- ...opencode-same-head-dispatch-idempotency.md | 10 ++++--- docs/product-technical-gap-baseline.md | 4 +-- .../ci/verify_release_maturin_tool_assets.py | 2 ++ .../test_required_workflow_queue_contract.py | 26 +++++++++++++++++++ ..._materializer_quality_workflow_contract.py | 2 +- ...test_verify_release_maturin_tool_assets.py | 6 +++++ 12 files changed, 83 insertions(+), 27 deletions(-) diff --git a/.github/workflows/pr-review-merge-scheduler.yml b/.github/workflows/pr-review-merge-scheduler.yml index 5473e33f6f..b568bfa4c6 100644 --- a/.github/workflows/pr-review-merge-scheduler.yml +++ b/.github/workflows/pr-review-merge-scheduler.yml @@ -93,9 +93,11 @@ concurrency: github.event_name == 'pull_request_review' && format('head-{0}', github.event.pull_request.head.sha) || github.event_name == 'repository_dispatch' && github.event.client_payload.pr_head_sha != '' && format('head-{0}', github.event.client_payload.pr_head_sha) || 'no-head' }} - # Preserve every same-head admission in GitHub's bounded FIFO queue. A new - # head receives a distinct group; the metadata-only cleanup job below retires - # only revalidated predecessor work and proves terminal cancellation. + # Preserve same-head admissions up to GitHub's documented pending limit + # without single-pending replacement; dispatch ordering remains + # platform-controlled. A new head receives a distinct group; the metadata-only + # cleanup job below retires only revalidated predecessor work and proves + # terminal cancellation. queue: max # Scorecard Token-Permissions (alert #9): declare a least-privilege default at @@ -165,7 +167,7 @@ jobs: for active_status in requested waiting pending queued in_progress; do inventory_path="$inventory_dir/${inventory_pass}-${active_status}.json" gh api --paginate --slurp \ - "repos/${TARGET_REPOSITORY}/actions/workflows/pr-review-merge-scheduler.yml/runs?event=pull_request_target&status=${active_status}&per_page=100" \ + "repos/${TARGET_REPOSITORY}/actions/workflows/pr-review-merge-scheduler.yml/runs?status=${active_status}&per_page=100" \ >"$inventory_path" if ! inventory_counts="$(jq -er ' if type == "array" and length > 0 diff --git a/.github/workflows/trusted-uv-materializer-quality-ci.yml b/.github/workflows/trusted-uv-materializer-quality-ci.yml index ea78c30cc7..d38dcd855f 100644 --- a/.github/workflows/trusted-uv-materializer-quality-ci.yml +++ b/.github/workflows/trusted-uv-materializer-quality-ci.yml @@ -155,7 +155,7 @@ jobs: python -m coverage report - name: Enforce complete production docstrings - run: python -m interrogate --fail-under 100 scripts/ci/materialize_base_python_requirements.py + run: python -m interrogate --fail-under 100 scripts/ci - name: Compile production and quality contracts run: | diff --git a/AGENTS.md b/AGENTS.md index 0972af51c5..5087855f7b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -46,18 +46,23 @@ The materialization contract is also covered by [`docs/doctoring/exact-artifact- and use `github-robot-review-gate` plus `babysit-pr` when diagnosing or monitoring a protected PR. If a named skill is unavailable, preserve its fail-closed trust boundary and exact-current-head evidence rules manually. -- PR-triggered workflow concurrency must be trigger-aware. Group by workflow, - target repository, and pull request number with `cancel-in-progress: true`; - do not include the head SHA, because that prevents a new head from cancelling - its predecessor. Non-PR triggers need an explicit collision-safe fallback. +- PR-triggered workflow concurrency must be trigger-aware. For replaceable + current-state checks, group by workflow, target repository, and pull request + number with `cancel-in-progress: true`. When every same-head admission carries + distinct work that must survive, include the exact head SHA and use bounded + `queue: max`; retire predecessor heads only through a metadata-only cleanup + that inventories every PR-associated trigger, revalidates live authority, + and proves terminal cancellation. Non-PR triggers need an explicit + collision-safe fallback. - Put concurrency at workflow scope when queued jobs must be coalesced before a runner is admitted. Job-level concurrency cannot relieve a saturated runner queue because it is evaluated only after job admission. - Keep cleanup repository-local and event-driven. Do not restore an - organization-wide queue sweep, polling `sleep`, or another scheduled scan to - compensate for incorrect concurrency. Cancel only runs proven to belong to a - superseded head of the same PR, then verify each accepted cancellation - reaches `completed/cancelled`. + organization-wide queue sweep, long-lived polling wait, or another scheduled + scan to compensate for incorrect concurrency. A metadata-only cleanup may use + a few bounded status reads to prove an accepted cancellation reached + `completed/cancelled`; it must fail closed rather than hold a model or review + runner. Cancel only runs proven to belong to a superseded head of the same PR. - Classify a run's PR head by event-specific evidence before cancellation. `pull_request` may use the run's top-level `head_sha`, but `pull_request_target` records the trusted base there; use its PR association diff --git a/CHANGELOG.d/20261001-opencode-same-head-dispatch-idempotency.md b/CHANGELOG.d/20261001-opencode-same-head-dispatch-idempotency.md index d9bac48a92..8609c73085 100644 --- a/CHANGELOG.d/20261001-opencode-same-head-dispatch-idempotency.md +++ b/CHANGELOG.d/20261001-opencode-same-head-dispatch-idempotency.md @@ -8,8 +8,9 @@ acquires the exact-PR lease, revalidate live authority and the formal exact-head review receipt again so a completed prior receiver cannot trigger duplicate coverage or model execution. - Preserve every exact-head merge-scheduler admission with `queue: max`; a - dedicated metadata-only cleanup inventories every active state twice, + Preserve exact-head merge-scheduler admissions within GitHub's documented + `queue: max` bound; a dedicated metadata-only cleanup inventories every + active state twice across all PR-associated scheduler triggers, rejects incomplete GitHub search results, retires only live-head-revalidated predecessor runs, and proves each accepted cancellation reaches `completed/cancelled`. diff --git a/CHANGELOG.md b/CHANGELOG.md index b2bccf21c1..7b177ebd7e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,9 +5,11 @@ run. The required wake now inventories all five active states twice, retires only identity-validated older-head runs after live authority checks, and fails closed until every accepted cancellation is proven - `completed/cancelled`. The merge scheduler now preserves all exact-head - admissions with `queue: max` and retires predecessor heads only through a - metadata-only, live-head-revalidated cleanup job. The receiver atomically compare-and-swaps one + `completed/cancelled`. The merge scheduler now preserves exact-head + admissions within GitHub's documented `queue: max` bound and retires + predecessor heads from every PR-associated scheduler trigger only through a + metadata-only, + live-head-revalidated cleanup job. The receiver atomically compare-and-swaps one repository/PR lease file on a dedicated central branch before source materialization, coverage, or model execution, closing the cross-producer check-then-POST race without lossy native concurrency. A dedicated minimal @@ -37,7 +39,9 @@ and its focused test, so a future lifecycle change cannot omit the repository coverage gate that detected this regression. The pull-request trigger admits stacked canonical-owner bases as well as `main`; the protected-branch push - trigger remains restricted to `main`. + trigger remains restricted to `main`. Document the archive extractor and CLI + entry point, and expand the trusted docstring gate from one materializer file + to all `scripts/ci` production modules. ### Shared Strix lock advances beyond the PyJWT recursion DoS diff --git a/docs/doctoring/maturin-download-response-lifecycle-20261001.md b/docs/doctoring/maturin-download-response-lifecycle-20261001.md index cfe20cc5c6..cbe781f145 100644 --- a/docs/doctoring/maturin-download-response-lifecycle-20261001.md +++ b/docs/doctoring/maturin-download-response-lifecycle-20261001.md @@ -37,14 +37,20 @@ while the push trigger remains restricted to protected `main`. The repair does not change admitted hosts, the one-hop redirect contract, credentials, request timeout, byte bounds, digests, or error mapping. +Exact-tree verification then found that the verifier's archive extractor and +CLI entry point lacked docstrings even though the PR claimed complete +production docstring coverage. A RED regression now binds both symbols, the +two trust-boundary docstrings close the omission, and Trusted uv measures all +of `scripts/ci` instead of only the materializer module. + ## Evidence and remaining gates - Hosted RED: 5,314 passed, 5 skipped, 40 subtests; 18,775 statements with 5 missing, 7,652 branches with 2 partial; total 99%. - Local integrated GREEN: 18 lifecycle/prescreen tests; complete warnings-fatal - suite 5,362 passed, 5 skipped, 40 subtests; 18,767/18,767 statements and + suite 5,363 passed, 5 skipped, 40 subtests; 18,767/18,767 statements and 7,648/7,648 branches; verifier 107/107 statements and 34/34 branches; - `git diff --check` clean. + production docstrings 1,456/1,456; `git diff --check` clean. - Required before acceptance: complete exact-head hosted suite, security and CodeQL verdicts, qualifying independent approval, ordinary owner integration, then ordinary merge-forward into #1653 and fresh consumer Checks. diff --git a/docs/doctoring/opencode-same-head-dispatch-idempotency.md b/docs/doctoring/opencode-same-head-dispatch-idempotency.md index c4e7b91d69..af75351b3c 100644 --- a/docs/doctoring/opencode-same-head-dispatch-idempotency.md +++ b/docs/doctoring/opencode-same-head-dispatch-idempotency.md @@ -59,7 +59,10 @@ PR/head immediately before each mutation, cancels only predecessor-head work reports `completed/cancelled`. Two complete status passes prevent a state transition from escaping between filtered queries; every response reconciles the collected row count with `total_count` and fails closed if GitHub's -filtered-search ceiling truncates the inventory. +filtered-search ceiling truncates the inventory. The inventory spans every +event for this workflow and then binds candidates through PR association and +head SHA; filtering the API to `pull_request_target` would strand predecessor +`pull_request_review` runs in their old exact-head group. Producer observation and POST are not atomic, so the receiver first authorizes the exact actor/sender pair, repository allowlist membership, and complete @@ -119,8 +122,9 @@ cancellation, deduplication, asynchronous cancellation continuation, and fail-closure when a cancellation is refused, remains active, returns an invalid state, or terminates with a non-cancelled conclusion. Scheduler cleanup fixtures also prove concurrent-head-movement preservation, transition-safe -two-pass discovery, inventory-completeness rejection, and bounded terminal -cancellation verification. +two-pass discovery, review-event predecessor discovery, +inventory-completeness rejection, and bounded terminal cancellation +verification. Receiver fixtures execute absent, active-owner, terminal-owner, self-rerun, different-head takeover, and branch-initialization-race lease paths. A two-process fixture starts simultaneous cache misses against an atomic fake diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index ecfe86f976..380558d777 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -4,13 +4,13 @@ | Gap | Exact evidence | Action | Status | |---|---|---|---| -| A repeated required-check wake for unchanged `.github#2545@9a4af5e438283a31dc05814d6bc2818caee782a3` cancelled queued central dispatch `36776536447` when same-head run `36778773766` was created, resetting queue position without new source authority | [Run 36776536447](https://github.com/ContextualWisdomLab/.github/actions/runs/36776536447); [run 36778773766](https://github.com/ContextualWisdomLab/.github/actions/runs/36778773766); protected `main@37b10243cec3d160ecc9c1be75c71428b160a703` | At the central `.github` owner, inventory all five active admission states twice, validate run identity and fail closed on ambiguity, remove lossy native receiver concurrency, retire only canonical older-head central runs after live repository/PR/head validation, and require every accepted cancellation to reach `completed/cancelled` before current-head dispatch. Preserve merge-scheduler exact-head admissions with bounded `queue: max`; on `synchronize` or `closed`, a metadata-only job inventories every active state twice, rejects invalid or `total_count`-incomplete results, revalidates live PR/head authority before each mutation, retires only predecessor work, and proves terminal cancellation. Because producer inventory and POST are not atomic, authorize actor/sender/target and payload shape before OIDC, then acquire one repository/PR lease through the Contents API's blob-SHA compare-and-swap only after full live state/draft/base/head validation; active same-head losers terminate cheaply, self-reruns and terminal owners recover, and a freshly revalidated live head may replace a fully validated older-head owner. After lease acquisition or retention, revalidate live repository/PR/head authority and the trusted formal exact-head receipt; an existing receipt must return `admitted=false` before source materialization, coverage, or model execution, and an unavailable or malformed lookup fails closed. Isolate central `contents: write` in a dedicated lease job and keep later jobs read-only. After formal receipt, use repository-wide run inventory, bind PR number and `pull_requests[].head.sha` rather than the trusted-base run-level SHA, recursively partition the PR-lifetime `created` range below GitHub's 1,000-result ceiling, verify `total_count` completeness, and revalidate before each failed-job rerun so duplicate callbacks are unnecessary. See [RCA and executable acceptance](doctoring/opencode-same-head-dispatch-idempotency.md) | **Proposed / local RED→GREEN complete; exact-head hosted Checks and independent approval required** | +| A repeated required-check wake for unchanged `.github#2545@9a4af5e438283a31dc05814d6bc2818caee782a3` cancelled queued central dispatch `36776536447` when same-head run `36778773766` was created, resetting queue position without new source authority | [Run 36776536447](https://github.com/ContextualWisdomLab/.github/actions/runs/36776536447); [run 36778773766](https://github.com/ContextualWisdomLab/.github/actions/runs/36778773766); protected `main@37b10243cec3d160ecc9c1be75c71428b160a703` | At the central `.github` owner, inventory all five active admission states twice, validate run identity and fail closed on ambiguity, remove lossy native receiver concurrency, retire only canonical older-head central runs after live repository/PR/head validation, and require every accepted cancellation to reach `completed/cancelled` before current-head dispatch. Preserve merge-scheduler exact-head admissions with bounded `queue: max`; on `synchronize` or `closed`, a metadata-only job inventories every active state twice across all PR-associated scheduler triggers, rejects invalid or `total_count`-incomplete results, revalidates live PR/head authority before each mutation, retires only predecessor work, and proves terminal cancellation. Because producer inventory and POST are not atomic, authorize actor/sender/target and payload shape before OIDC, then acquire one repository/PR lease through the Contents API's blob-SHA compare-and-swap only after full live state/draft/base/head validation; active same-head losers terminate cheaply, self-reruns and terminal owners recover, and a freshly revalidated live head may replace a fully validated older-head owner. After lease acquisition or retention, revalidate live repository/PR/head authority and the trusted formal exact-head receipt; an existing receipt must return `admitted=false` before source materialization, coverage, or model execution, and an unavailable or malformed lookup fails closed. Isolate central `contents: write` in a dedicated lease job and keep later jobs read-only. After formal receipt, use repository-wide run inventory, bind PR number and `pull_requests[].head.sha` rather than the trusted-base run-level SHA, recursively partition the PR-lifetime `created` range below GitHub's 1,000-result ceiling, verify `total_count` completeness, and revalidate before each failed-job rerun so duplicate callbacks are unnecessary. See [RCA and executable acceptance](doctoring/opencode-same-head-dispatch-idempotency.md) | **Proposed / local RED→GREEN complete; exact-head hosted Checks and independent approval required** | ## 2026-10-01 Maturin response-lifecycle coverage closure | Gap | Exact evidence | Action | Status | |---|---|---|---| -| `.github#1653@5cd141ec2c33b631d164af936cd1c9de70e4c9a4` passed all 5,314 tests but failed the complete branch gate because the canonical Maturin downloader left five error-path statements and two branches unexecuted; the owner workflow omitted both verifier paths and stacked PR bases | Trusted uv Materializer run `36811202519`, job `110206427182`; `verify_release_maturin_tool_assets.py` 95%, missing lines 104 and 106-112 plus branch 114→116; no owner run at #2530 predecessor `8cf2ea5f73976d47b2267fb52ac28284323404b7` because its base was #2531 rather than `main` | Repair canonical successor `.github#2530`: exercise non-200 and opener-raised `HTTPError` closure, replace the impossible nullable-response finalizer with one unconditional response-owned close scope, add source/test and stacked-PR trigger contracts to the complete gate while retaining protected-main push scope, preserve all network and fail-closed boundaries, then ordinary-merge the accepted owner head into #1653 | **Proposed / hosted RED reproduced; focused verifier coverage GREEN locally; path and stacked-admission contracts RED→GREEN; exact-head hosted full-suite, security, CodeQL, and independent approval required** | +| `.github#1653@5cd141ec2c33b631d164af936cd1c9de70e4c9a4` passed all 5,314 tests but failed the complete branch gate because the canonical Maturin downloader left five error-path statements and two branches unexecuted; the owner workflow omitted both verifier paths and stacked PR bases | Trusted uv Materializer run `36811202519`, job `110206427182`; `verify_release_maturin_tool_assets.py` 95%, missing lines 104 and 106-112 plus branch 114→116; no owner run at #2530 predecessor `8cf2ea5f73976d47b2267fb52ac28284323404b7` because its base was #2531 rather than `main` | Repair canonical successor `.github#2530`: exercise non-200 and opener-raised `HTTPError` closure, replace the impossible nullable-response finalizer with one unconditional response-owned close scope, document the archive/CLI trust boundaries, expand the trusted docstring gate to all `scripts/ci`, add source/test and stacked-PR trigger contracts to the complete gate while retaining protected-main push scope, preserve all network and fail-closed boundaries, then ordinary-merge the accepted owner head into #1653 | **Proposed / hosted RED reproduced; focused verifier coverage and 100% production docstrings GREEN locally; path and stacked-admission contracts RED→GREEN; exact-head hosted full-suite, security, CodeQL, and independent approval required** | ## 2026-10-01 bounded Maturin release downloader SAST closure diff --git a/scripts/ci/verify_release_maturin_tool_assets.py b/scripts/ci/verify_release_maturin_tool_assets.py index a59507ac83..2aa9767d8a 100644 --- a/scripts/ci/verify_release_maturin_tool_assets.py +++ b/scripts/ci/verify_release_maturin_tool_assets.py @@ -120,6 +120,7 @@ def _download(filename: str) -> bytes: def _binary(raw: bytes, filename: str) -> bytes: + """Extract the single bounded Maturin executable from its reviewed archive.""" if filename.endswith(".zip"): with zipfile.ZipFile(io.BytesIO(raw)) as archive: members = archive.infolist() @@ -168,6 +169,7 @@ def verify_assets(evidence: dict, reader: str, fetch=_download) -> None: def main() -> None: + """Verify reviewed Maturin assets from the network or an explicit local root.""" parser = argparse.ArgumentParser() parser.add_argument("--asset-root", type=Path) args = parser.parse_args() diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index 84549be9d9..12560f0d05 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -1720,6 +1720,32 @@ def test_scheduler_cleanup_second_inventory_pass_catches_state_transition( assert "/actions/runs/100/force-cancel" in calls +def test_scheduler_cleanup_inventory_covers_review_event_runs(tmp_path: Path) -> None: + """A new head must retire predecessor runs triggered by PR reviews too.""" + review_inventory = { + "total_count": 1, + "workflow_runs": [ + { + "id": 100, + "event": "pull_request_review", + "status": "queued", + "pull_requests": [ + {"number": 7, "head": {"sha": "b" * 40}}, + ], + } + ], + } + result, calls = _run_merge_scheduler_cleanup( + tmp_path, + [_live_scheduler_pull()] * 8, + [{"status": "completed", "conclusion": "cancelled"}], + [review_inventory] * 10, + ) + assert result.returncode == 0, result.stderr + assert "event=pull_request_target" not in calls + assert "/actions/runs/100/force-cancel" in calls + + def test_scheduler_cleanup_fails_closed_on_truncated_inventory( tmp_path: Path, ) -> None: diff --git a/tests/test_trusted_uv_materializer_quality_workflow_contract.py b/tests/test_trusted_uv_materializer_quality_workflow_contract.py index ed4ad55681..7c27766c5d 100644 --- a/tests/test_trusted_uv_materializer_quality_workflow_contract.py +++ b/tests/test_trusted_uv_materializer_quality_workflow_contract.py @@ -146,7 +146,7 @@ def test_full_quality_gate_proves_tests_coverage_docstrings_and_compilation() -> assert "python -m coverage report" in workflow assert "python -m coverage run -m pytest tests -q" in workflow assert "unset COVERAGE_RCFILE" in workflow - assert "python -m interrogate --fail-under 100" in workflow + assert "run: python -m interrogate --fail-under 100 scripts/ci\n" in workflow assert "python -m compileall -q" in workflow required_tests = ( diff --git a/tests/test_verify_release_maturin_tool_assets.py b/tests/test_verify_release_maturin_tool_assets.py index 6dd581eea0..85e720152e 100644 --- a/tests/test_verify_release_maturin_tool_assets.py +++ b/tests/test_verify_release_maturin_tool_assets.py @@ -338,6 +338,12 @@ def verify(evidence, reader, fetch): assert captured["raw"] == b"asset" +def test_maturin_verifier_has_complete_docstrings(): + """Every production entry point explains its trust-boundary responsibility.""" + assert verifier._binary.__doc__ + assert verifier.main.__doc__ + + def test_maturin_process_entrypoint_uses_the_bounded_downloader(monkeypatch): archives, assets, _, evidence = _asset_case() from scripts.ci import scan_release_native_links as scanner From 8951c2c72e778c323257f08082b9b7554fa39b75 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 1 Oct 2026 14:43:09 +0900 Subject: [PATCH 06/10] docs(evidence): bind final exact-suite count --- docs/doctoring/maturin-download-response-lifecycle-20261001.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/doctoring/maturin-download-response-lifecycle-20261001.md b/docs/doctoring/maturin-download-response-lifecycle-20261001.md index cbe781f145..ec73c8b89a 100644 --- a/docs/doctoring/maturin-download-response-lifecycle-20261001.md +++ b/docs/doctoring/maturin-download-response-lifecycle-20261001.md @@ -48,7 +48,7 @@ of `scripts/ci` instead of only the materializer module. - Hosted RED: 5,314 passed, 5 skipped, 40 subtests; 18,775 statements with 5 missing, 7,652 branches with 2 partial; total 99%. - Local integrated GREEN: 18 lifecycle/prescreen tests; complete warnings-fatal - suite 5,363 passed, 5 skipped, 40 subtests; 18,767/18,767 statements and + suite 5,364 passed, 5 skipped, 40 subtests; 18,767/18,767 statements and 7,648/7,648 branches; verifier 107/107 statements and 34/34 branches; production docstrings 1,456/1,456; `git diff --check` clean. - Required before acceptance: complete exact-head hosted suite, security and From db81de7a09268eb0abfc5b5c675ddd9a0a38a3e2 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 1 Oct 2026 17:42:10 +0900 Subject: [PATCH 07/10] test(strix): align queue gate with lease admission --- scripts/ci/test_strix_quick_gate.sh | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index e53741bd51..e3d4ce826a 100755 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -587,11 +587,12 @@ assert_opencode_review_uses_codegraph_and_contextual_orchestrator() { record_failure "opencode required workflow bootstrap condition detection must survive a job block larger than the pipe buffer" fi rm -f "$large_bootstrap_fixture" - assert_file_contains "$workflow_file" 'needs.validate-pr-metadata.outputs.target_repository' "opencode review scopes concurrency by the live validated target repository" - assert_file_contains "$workflow_file" 'needs.validate-pr-metadata.outputs.pr_number || github.run_id' "opencode review scopes concurrency by the live validated PR with a non-PR fallback" + assert_file_contains "$workflow_file" 'admit-exact-head-dispatch:' "opencode review admits one exact-head receiver before expensive work" + assert_file_contains "$workflow_file" 'admitted: ${{ steps.single_flight.outputs.admitted }}' "opencode review exports its atomic exact-head lease decision" + assert_file_contains "$workflow_file" "needs.admit-exact-head-dispatch.outputs.admitted == 'true'" "opencode review gates expensive work on the exact-head lease" + assert_file_contains "$workflow_file" 'opencode-dispatch-leases' "opencode review uses a durable exact-head lease instead of lossy native concurrency" + assert_file_contains "$workflow_file" "Wake every failed exact-head Required OpenCode workflow" "opencode review preserves distinct same-head admissions for deterministic receipt recovery" assert_file_not_contains "$workflow_file" "format('pr-{0}-{1}'" "opencode review does not keep stale head-specific concurrency groups" - assert_file_contains "$workflow_file" 'opencode-review-${{' "opencode review uses the workflow-repository-PR group prefix" - assert_file_contains "$workflow_file" 'cancel-in-progress: true' "opencode review cancels stale in-progress review attempts when a newer PR event arrives" assert_file_contains "$workflow_file" "Materialize pull request merge tree for coverage measurement" "opencode pull_request coverage execution materializes the exact base/head merge tree" assert_file_contains "$workflow_file" "stale OpenCode run: event head=" "opencode review side effects are skipped for stale heads" assert_file_not_contains "$workflow_file" "github.event.pull_request.head.repo.full_name == github.event.pull_request.base.repo.full_name" "opencode never treats a same-repository pull_request_target head as authorization to execute PR-controlled code" @@ -614,7 +615,18 @@ assert_opencode_review_uses_codegraph_and_contextual_orchestrator() { assert_file_contains "$workflow_file" "actions: read" "opencode review workflow can read failed Actions logs without Actions write scope" assert_file_contains "$workflow_file" "checks: read" "opencode review workflow can read failed check-run annotations for line-specific findings" assert_file_contains "$workflow_file" "contents: read" "opencode review workflow uses read-only repository contents permission" - assert_file_not_contains "$workflow_file" "contents: write" "opencode review workflow does not need repository contents write scope" + local admission_job validation_job + admission_job="$(awk '/^ admit-exact-head-dispatch:$/ { emit=1 } emit && /^ [A-Za-z0-9_-]+:$/ && $0 !~ /^ admit-exact-head-dispatch:$/ { exit } emit { print }' "$workflow_file")" + validation_job="$(awk '/^ validate-pr-metadata:$/ { emit=1 } emit && /^ [A-Za-z0-9_-]+:$/ && $0 !~ /^ validate-pr-metadata:$/ { exit } emit { print }' "$workflow_file")" + if ! grep -Fq -- "contents: write" <<<"$admission_job"; then + record_failure "opencode review scopes repository contents write permission to the atomic lease job" + fi + if grep -Fq -- "contents: write" <<<"$validation_job"; then + record_failure "opencode review metadata validation must not receive repository contents write permission" + fi + if ! grep -Fq -- "contents: read" <<<"$validation_job"; then + record_failure "opencode review metadata validation keeps read-only repository contents permission" + fi assert_file_contains "$workflow_file" "pull-requests: write" "opencode review workflow may use github-actions[bot] for same-repository review-thread, update-branch, auto-merge, and merge follow-up" assert_file_contains "$workflow_file" "issues: write" "opencode review workflow can publish or update overview comments through the job token" assert_file_contains "$workflow_file" "statuses: write" "opencode review workflow can read status contexts and publish the repository_dispatch status evidence it owns" @@ -1593,7 +1605,10 @@ assert_pr_review_merge_scheduler_uses_github_actions_bot_token() { assert_file_contains "$workflow_file" "github.event_name == 'pull_request_target' && format('pr-{0}', github.event.pull_request.number)" "scheduler scopes pull_request_target concurrency to the active PR" assert_file_contains "$workflow_file" "github.event_name == 'schedule' && format('schedule-{0}', github.event.schedule)" "scheduler isolates repository-local recovery from PR runs" assert_file_contains "$workflow_file" "github.event_name == 'repository_dispatch' && github.event.client_payload.target_repository != '' && github.event.client_payload.pr_number != ''" "scheduler scopes targeted manual queue scans to the requested PR" - assert_file_contains "$workflow_file" "cancel-in-progress: \${{ github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review' || github.event_name == 'repository_dispatch' }}" "scheduler cancels stale PR/review/manual queue scans instead of accumulating merge/update attempts" + assert_file_contains "$workflow_file" "queue: max" "scheduler preserves distinct same-head admissions up to the documented pending limit" + assert_file_not_contains "$workflow_file" "cancel-in-progress:" "scheduler avoids native pending-run replacement" + assert_file_contains "$workflow_file" "cancel-superseded-pr-runs:" "scheduler retires only revalidated predecessor-head runs" + assert_file_contains "$workflow_file" "Cancel revalidated predecessor scheduler runs" "scheduler keeps predecessor cleanup metadata-only and explicit" assert_file_not_contains "$workflow_file" 'github.event.workflow_run' "scheduler does not poll required-check completion through follow-up workflow runs" assert_file_contains "$workflow_file" "github.event.client_payload.trigger_reviews != false" "scheduler enables review dispatch by default for default-branch dispatch events" assert_file_contains "$workflow_file" "github.event_name == 'schedule' || github.event_name == 'push'" "scheduler can dispatch a bounded OpenCode review from native or recovery events" From bd1a3d3b2e5a73f48fb83cb4f590886b411d050f Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 1 Oct 2026 19:16:43 +0900 Subject: [PATCH 08/10] fix: admit exact-head Draft semantic reviews --- .../workflows/opencode-review-dispatch.yml | 76 ++++- CHANGELOG.md | 16 + ...ncode-authorized-draft-review-admission.md | 50 ++++ docs/product-technical-gap-baseline.md | 6 + scripts/ci/pr_review_merge_scheduler_core.py | 38 ++- ...st_opencode_required_verdict_regression.py | 280 ++++++++++++++++++ ...t_pr_review_autofix_nvidia_nim_contract.py | 2 +- tests/test_pr_review_merge_scheduler.py | 95 ++++++ 8 files changed, 550 insertions(+), 13 deletions(-) create mode 100644 docs/doctoring/opencode-authorized-draft-review-admission.md diff --git a/.github/workflows/opencode-review-dispatch.yml b/.github/workflows/opencode-review-dispatch.yml index 5c87c74863..bd1c9894e9 100644 --- a/.github/workflows/opencode-review-dispatch.yml +++ b/.github/workflows/opencode-review-dispatch.yml @@ -43,6 +43,7 @@ jobs: SUPPLIED_BASE_SHA: ${{ github.event.client_payload.pr_base_sha || '' }} SUPPLIED_HEAD_REF: ${{ github.event.client_payload.pr_head_ref || '' }} SUPPLIED_HEAD_SHA: ${{ github.event.client_payload.pr_head_sha || '' }} + DRAFT_REVIEW_ONLY: ${{ toJSON(github.event.client_payload.draft_review_only || false) }} run: | set -euo pipefail [ "$EVENT_NAME" = "repository_dispatch" ] || { @@ -89,6 +90,10 @@ jobs: printf '::error::repository_dispatch admission rejected malformed PR identity metadata. target=%s pr=%s\n' "${TARGET_REPOSITORY:-}" "${PR_NUMBER:-}" exit 1 fi + if ! [[ "$DRAFT_REVIEW_ONLY" =~ ^(true|false)$ ]]; then + echo "::error::repository_dispatch admission rejected malformed draft-review authority." + exit 1 + fi printf 'Authorized exact repository_dispatch envelope for %s#%s.\n' "$TARGET_REPOSITORY" "$PR_NUMBER" - name: Exchange OpenCode app token for target repository metadata reads @@ -176,6 +181,7 @@ jobs: SUPPLIED_BASE_SHA: ${{ github.event.client_payload.pr_base_sha }} SUPPLIED_HEAD_REF: ${{ github.event.client_payload.pr_head_ref }} SUPPLIED_HEAD_SHA: ${{ github.event.client_payload.pr_head_sha }} + DRAFT_REVIEW_ONLY: ${{ toJSON(github.event.client_payload.draft_review_only || false) }} run: | set -euo pipefail exact_title="OpenCode Review Dispatch ${TARGET_REPOSITORY}#${PR_NUMBER}@${HEAD_SHA}" @@ -186,6 +192,31 @@ jobs: receipt_helper="$(mktemp)" trap 'rm -f "$receipt_helper"' EXIT + authorized_draft_marker_matches() { + local marker_name marker_inventory + [ "${DRAFT_REVIEW_ONLY:-false}" = "true" ] || return 1 + marker_name="cwl-draft-review-request-${TARGET_REPOSITORY//\//-}-${PR_NUMBER}-${HEAD_SHA}" + marker_inventory="$(gh api \ + "repos/${GITHUB_REPOSITORY}/actions/artifacts?name=${marker_name}&per_page=100")" || return 1 + jq -e --arg marker_name "$marker_name" ' + (.total_count | type) == "number" + and (.total_count | floor) == .total_count + and .total_count >= 0 + and (.artifacts | type) == "array" + and .total_count == (.artifacts | length) + and all(.artifacts[]; + type == "object" + and (.id | type) == "number" + and (.id | floor) == .id + and .id >= 1 + and (.name | type) == "string" + and .name == $marker_name + and (.expired | type) == "boolean" + ) + and any(.artifacts[]; .name == $marker_name and .expired == false) + ' <<<"$marker_inventory" >/dev/null + } + live_authority_matches() { local live_pr live_base_repo live_base_ref live_base_sha local live_head_repo live_head_ref live_head_sha live_draft live_state @@ -203,14 +234,18 @@ jobs: live_state="$(jq -r \ 'if (.state | type) == "string" then .state else empty end' \ <<<"$live_pr")" - [ "$live_state" = "open" ] && [ "$live_draft" = "false" ] && + [ "$live_state" = "open" ] && [ "$live_base_repo" = "$TARGET_REPOSITORY" ] && [[ "$live_head_repo" =~ ^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ ]] && [ "$live_base_ref" = "$SUPPLIED_BASE_REF" ] && [ "$live_base_sha" = "$SUPPLIED_BASE_SHA" ] && [ "$live_head_ref" = "$SUPPLIED_HEAD_REF" ] && [ "${live_head_sha,,}" = "${SUPPLIED_HEAD_SHA,,}" ] && - [ "${live_head_sha,,}" = "${HEAD_SHA,,}" ] + [ "${live_head_sha,,}" = "${HEAD_SHA,,}" ] && + { + { [ "$live_draft" = "false" ] && [ "${DRAFT_REVIEW_ONLY:-false}" = "false" ]; } || + { [ "$live_draft" = "true" ] && authorized_draft_marker_matches; } + } } if ! live_authority_matches; then echo "::error::Pull request authority changed before atomic OpenCode admission." @@ -224,8 +259,8 @@ jobs: echo "::error::Could not load the trusted OpenCode receipt helper." >&2 return 1 fi - GH_TOKEN="$TARGET_READ_TOKEN" python3 -c 'import runpy,sys; gate=runpy.run_path(sys.argv[1]); reviews=gate["fetch_reviews"](sys.argv[2],int(sys.argv[3])); receipt,_reason=gate["evaluate_receipts"](reviews,sys.argv[4],is_draft=False); print("present" if receipt is not None else "missing")' \ - "$receipt_helper" "$TARGET_REPOSITORY" "$PR_NUMBER" "$HEAD_SHA" + GH_TOKEN="$TARGET_READ_TOKEN" python3 -c 'import runpy,sys; gate=runpy.run_path(sys.argv[1]); reviews=gate["fetch_reviews"](sys.argv[2],int(sys.argv[3])); receipt,_reason=gate["evaluate_receipts"](reviews,sys.argv[4],is_draft=sys.argv[5] == "true"); print("present" if receipt is not None else "missing")' \ + "$receipt_helper" "$TARGET_REPOSITORY" "$PR_NUMBER" "$HEAD_SHA" "${DRAFT_REVIEW_ONLY:-false}" } complete_receipt_admission() { @@ -436,6 +471,7 @@ jobs: base_sha: ${{ steps.validate.outputs.base_sha }} head_ref: ${{ steps.validate.outputs.head_ref }} head_sha: ${{ steps.validate.outputs.head_sha }} + is_draft: ${{ steps.validate.outputs.is_draft }} is_private: ${{ steps.validate.outputs.is_private }} steps: - name: Exchange OpenCode app token for target repository metadata reads @@ -526,6 +562,7 @@ jobs: SUPPLIED_BASE_SHA: ${{ github.event.client_payload.pr_base_sha || '' }} SUPPLIED_HEAD_REF: ${{ github.event.client_payload.pr_head_ref || '' }} SUPPLIED_HEAD_SHA: ${{ github.event.client_payload.pr_head_sha || '' }} + DRAFT_REVIEW_ONLY: ${{ toJSON(github.event.client_payload.draft_review_only || false) }} run: | set -euo pipefail if [ "$EVENT_NAME" = "repository_dispatch" ]; then @@ -581,6 +618,9 @@ jobs: live_base_sha="$(jq -r '.base.sha // empty' <<<"$pull_request_json")" live_head_ref="$(jq -r '.head.ref // empty' <<<"$pull_request_json")" live_head_sha="$(jq -r '.head.sha // empty' <<<"$pull_request_json")" + live_draft="$(jq -r \ + 'if (.draft | type) == "boolean" then (.draft | tostring) else empty end' \ + <<<"$pull_request_json")" live_state="$(jq -r '.state // empty' <<<"$pull_request_json")" live_visibility="$(jq -r '.base.repo.visibility // empty | ascii_downcase' <<<"$pull_request_json")" case "$live_visibility" in @@ -594,6 +634,7 @@ jobs: ! [[ "$live_head_repository" =~ ^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ ]] || ! [[ "$live_base_sha" =~ ^[0-9a-fA-F]{40}$ ]] || ! [[ "$live_head_sha" =~ ^[0-9a-fA-F]{40}$ ]] || + ! [[ "$live_draft" =~ ^(true|false)$ ]] || ! [[ "$live_is_private" =~ ^(true|false)$ ]] || [ -z "$live_base_ref" ] || [ -z "$live_head_ref" ]; then @@ -607,6 +648,7 @@ jobs: [ "$SUPPLIED_BASE_SHA" = "$live_base_sha" ] || mismatches+=("base_sha") [ "$SUPPLIED_HEAD_REF" = "$live_head_ref" ] || mismatches+=("head_ref") [ "$SUPPLIED_HEAD_SHA" = "$live_head_sha" ] || mismatches+=("head_sha") + [ "$DRAFT_REVIEW_ONLY" = "$live_draft" ] || mismatches+=("draft_review_only") if [ "${#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' "${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 @@ -620,6 +662,7 @@ jobs: printf 'base_sha=%s\n' "$live_base_sha" printf 'head_ref=%s\n' "$live_head_ref" printf 'head_sha=%s\n' "$live_head_sha" + printf 'is_draft=%s\n' "$live_draft" printf 'is_private=%s\n' "$live_is_private" } >>"$GITHUB_OUTPUT" printf 'Validated current live metadata for %s#%s: base=%s/%s head=%s/%s.\n' "$TARGET_REPOSITORY" "$PR_NUMBER" "$live_base_ref" "$live_base_sha" "$live_head_ref" "$live_head_sha" @@ -4884,6 +4927,7 @@ jobs: && needs.coverage-evidence.result == 'success' && steps.opencode_review_model_pool.outputs.review_status == 'success' && steps.central_review_process_fallback_scope.outputs.eligible == 'true' + && needs.validate-pr-metadata.outputs.is_draft == 'false' continue-on-error: true # Keep the normal peer-check hold short, but leave bounded room for # dynamic image/package-build extensions and review publication overhead. @@ -5223,6 +5267,7 @@ jobs: NO_COLOR: "1" PR_NUMBER: ${{ needs.validate-pr-metadata.outputs.pr_number }} HEAD_SHA: ${{ needs.validate-pr-metadata.outputs.head_sha }} + PR_DRAFT: ${{ needs.validate-pr-metadata.outputs.is_draft }} RUN_ID: ${{ github.run_id }} RUN_ATTEMPT: ${{ github.run_attempt }} OPENCODE_MODEL_POOL_OUTCOME: ${{ steps.opencode_review_model_pool.outputs.review_status }} @@ -5251,6 +5296,10 @@ jobs: OPENCODE_EXPORT_TIMEOUT_SECONDS: "60" run: | set -euo pipefail + if ! [[ "$PR_DRAFT" =~ ^(true|false)$ ]]; then + echo "::error::OpenCode publication rejected malformed live Draft metadata." + exit 1 + fi quoted_coverage="${COVERAGE_EVIDENCE_RESULT:-}" COVERAGE_EVIDENCE_RESULT="$(python3 scripts/ci/opencode_coverage_identity.py \ --repo "$GH_REPOSITORY" \ @@ -5541,6 +5590,11 @@ jobs: local gh_error_file local review_payload_file local review_response_file + if [ "$event" = "APPROVE" ] && [ "$PR_DRAFT" = "true" ]; then + event="COMMENT" + body="${body//- Result: APPROVE/- Result: DRAFT_REVIEW_COMPLETE}" + body="${body}"$'\n\n'"Draft review-only request completed without publishing merge approval authority." + fi if [ -z "${review_write_token:-}" ]; then printf '::error::OPENCODE_REVIEW_IDENTITY_UNAVAILABLE: refusing to publish %s with a GitHub Actions or PAT identity for head %s.\n' "$event" "$HEAD_SHA" return 1 @@ -8138,6 +8192,7 @@ jobs: if: >- always() && needs.validate-pr-metadata.result == 'success' + && needs.validate-pr-metadata.outputs.is_draft == 'false' && needs.validate-pr-metadata.outputs.target_repository != '' && needs.validate-pr-metadata.outputs.pr_number != '' && needs.validate-pr-metadata.outputs.head_sha != '' @@ -8148,21 +8203,17 @@ jobs: PR_HEAD_SHA: ${{ needs.validate-pr-metadata.outputs.head_sha }} run: | set -euo pipefail - draft_args=() - if [ "$(gh api "repos/${GH_REPOSITORY}/pulls/${PR_NUMBER}" --jq '.draft')" = "true" ]; then - draft_args=(--draft) - fi python3 scripts/ci/opencode_review_receipt_gate.py \ --repo "$GH_REPOSITORY" \ --pr-number "$PR_NUMBER" \ - --head-sha "$PR_HEAD_SHA" \ - "${draft_args[@]}" + --head-sha "$PR_HEAD_SHA" - name: Wake every failed exact-head Required OpenCode workflow if: >- always() && github.event_name == 'repository_dispatch' && steps.formal_review_receipt.outcome == 'success' + && needs.validate-pr-metadata.outputs.is_draft == 'false' && needs.validate-pr-metadata.outputs.target_repository != '' && needs.validate-pr-metadata.outputs.head_sha != '' env: @@ -8329,6 +8380,7 @@ jobs: if: >- always() && github.event_name == 'repository_dispatch' + && needs.validate-pr-metadata.outputs.is_draft == 'false' && needs.validate-pr-metadata.outputs.target_repository != '' && needs.validate-pr-metadata.outputs.head_sha != '' env: @@ -8393,6 +8445,7 @@ jobs: if: >- always() && github.event_name == 'repository_dispatch' + && needs.validate-pr-metadata.outputs.is_draft == 'false' && needs.validate-pr-metadata.outputs.target_repository != '' && needs.validate-pr-metadata.outputs.pr_number != '' && needs.validate-pr-metadata.outputs.head_sha != '' @@ -8421,6 +8474,9 @@ jobs: --interval-seconds 10 - name: Run merge scheduler after approval + if: >- + always() + && needs.validate-pr-metadata.outputs.is_draft == 'false' continue-on-error: true env: GH_TOKEN: ${{ secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN || steps.opencode_app_token.outputs.token || github.token }} diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b177ebd7e..fd31639c93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,19 @@ +### Authorized Draft reviews reach the exact-head receiver + +- Carry an explicit `draft_review_only` boolean from the merge scheduler to the + central OpenCode receiver. The receiver accepts a live Draft only while an + exact repository/PR/head durable request artifact is present, non-expired, + structurally complete, and independently re-fetched with `actions: read`; + malformed payloads, missing markers, and every base/head/state mismatch fail + before the Contents lease can be mutated. Ready pull requests reject Draft + authority. A clean Draft publishes an exact-head formal comment with + `DRAFT_REVIEW_COMPLETE`, never an approval or a merge-authorizing receipt; + the scheduler recognizes only that source-backed comment as completion. + Review-only runs never publish Ready status, dispatch Noema, invoke the merge + scheduler, or wake merge-required OpenCode jobs. This reconnects the + already-authorized agent-mention/scheduler path without weakening ordinary + ready-for-review admission. + ### OpenCode preserves exact-head queue position - Remove the central receiver's lossy native concurrency group after live diff --git a/docs/doctoring/opencode-authorized-draft-review-admission.md b/docs/doctoring/opencode-authorized-draft-review-admission.md new file mode 100644 index 0000000000..61d82b75dd --- /dev/null +++ b/docs/doctoring/opencode-authorized-draft-review-admission.md @@ -0,0 +1,50 @@ +# Authorized Draft review admission + +## Incident + +At `.github#2546@db81de7a09268eb0abfc5b5c675ddd9a0a38a3e2`, the explicit Draft-review +path had two individually valid halves that did not compose. The +agent-mention workflow wrote a one-day artifact named +`cwl-draft-review-request---`, and the merge scheduler +used that exact marker to authorize review-only dispatch. The central receiver +then required `live_draft=false` unconditionally before acquiring its lease. +The authorized request therefore could not reach semantic review. + +This was a control-plane contract defect, not a reason to make the pull request +Ready or to weaken ordinary merge admission. + +## Invariants + +- The scheduler sends `draft_review_only` as a JSON boolean derived from live + PR metadata; the receiver rejects any non-boolean envelope before external + token exchange. +- Ready work is admitted only with `draft_review_only=false`. +- Draft work is admitted only with `draft_review_only=true` and a freshly + fetched, exact repository/PR/head, non-expired central artifact. +- Artifact pagination or schema ambiguity, missing/expired markers, and any + live state/base/head mismatch fail before Contents lease mutation. +- Every pre-mutation authority recheck also re-fetches the marker. +- A clean Draft emits an exact-head formal `COMMENTED` review with the explicit + `DRAFT_REVIEW_COMPLETE` result; it never emits `APPROVED` authority. +- Only that exact-head result plus its review-only explanation satisfies the + Draft scheduler, so stale and generic comments cannot suppress a new request. +- Draft review-only work does not create a merge receipt, publish Ready status, + dispatch Noema, invoke the merge scheduler, or wake a merge-required OpenCode + workflow. +- A Draft-to-Ready transition invalidates Draft authority before lease access; + a validated Draft that changes later still publishes only a non-authorizing + comment. + +## Executable acceptance + +The regression suite executes the extracted production shell. It proves that +an exact authorized Draft reaches lease validation, malformed/mismatched/ +expired artifacts stop before lease access, ordinary Drafts remain rejected, +the dispatch payload distinguishes Ready and Draft work, and the Required +workflow wake is structurally disabled for a validated Draft. It also proves +that the Draft path cannot publish approval authority or start any Ready-only +follow-up, while its exact-head formal completion comment prevents an unbounded +same-head redispatch loop. The complete affected scheduler and receiver suite +must pass both locally and on the exact published head. Hosted security, +provenance, and independent semantic review remain required before ordinary +protected integration. diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 380558d777..3f53b60295 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1,5 +1,11 @@ # Product and Technical Gap Baseline +## 2026-10-01 authorized Draft review admission + +| Gap | Exact evidence | Action | Status | +|---|---|---|---| +| The agent-mention workflow and scheduler created an exact repository/PR/head durable Draft-review request, but the central receiver unconditionally required `live_draft=false`; every explicitly authorized Draft review therefore stopped before lease admission and could never produce the semantic review it requested. Once admitted, the clean-verdict publisher also attempted `APPROVED` before its downstream Draft receipt guard, which could grant merge authority to review-only work | `.github#2546@db81de7a09268eb0abfc5b5c675ddd9a0a38a3e2`; `.github/workflows/agent-mention-opencode-dispatch.yml` marker `cwl-draft-review-request---`; `.github/workflows/opencode-review-dispatch.yml` former `live_authority_matches` and unconditional clean-verdict approval; local RED contracts in `tests/test_opencode_required_verdict_regression.py` and `tests/test_pr_review_merge_scheduler.py` | Carry a typed review-only boolean in the scheduler payload, reject malformed authority before token exchange, and admit a live Draft only after re-fetching a non-expired exact marker from the central Actions artifact API. Revalidate state/base/head plus marker before every lease mutation. Convert a clean Draft verdict to an exact-head `DRAFT_REVIEW_COMPLETE` formal comment before publication; accept only that source-backed comment as scheduler completion, and structurally skip formal merge receipt, Ready status, Noema, merge-scheduler, and Required-workflow wake follow-ups. See [RCA and executable acceptance](doctoring/opencode-authorized-draft-review-admission.md) | **Proposed / local RED→GREEN complete; exact-head hosted Checks and independent semantic review required** | + ## 2026-10-01 OpenCode same-head dispatch idempotency | Gap | Exact evidence | Action | Status | diff --git a/scripts/ci/pr_review_merge_scheduler_core.py b/scripts/ci/pr_review_merge_scheduler_core.py index 5a86bd24c8..8a1ac3fb5e 100644 --- a/scripts/ci/pr_review_merge_scheduler_core.py +++ b/scripts/ci/pr_review_merge_scheduler_core.py @@ -103,7 +103,14 @@ def reconcile_state(state): continue terminal = ( record.request.component == "opencode" - and (has_current_head_approval(pr) or has_current_head_changes_requested(pr)) + and ( + has_current_head_approval(pr) + or has_current_head_changes_requested(pr) + or ( + bool(pr.get("isDraft")) + and has_current_head_draft_review_completion(pr) + ) + ) ) or ( record.request.component == "strix" and strix_evidence_state(pr) == "complete" @@ -2296,6 +2303,28 @@ def has_current_head_changes_requested(pr: dict[str, Any]) -> bool: return current_head_review_state(pr, "CHANGES_REQUESTED") +def has_current_head_draft_review_completion(pr: dict[str, Any]) -> bool: + """Return whether OpenCode completed an exact-head Draft review-only request. + + A clean Draft cannot receive merge approval authority. The central reviewer + therefore publishes a formal COMMENTED review carrying an explicit result + marker and explanatory sentence. Both markers are required so an ordinary + status comment cannot suppress a later explicit Draft review request. + """ + for review in reversed((pr.get("reviews") or {}).get("nodes") or []): + if not is_opencode_review(review) or not review_matches_current_head(review, pr): + continue + body = review.get("body") or "" + if ( + (review.get("state") or "").upper() == "COMMENTED" + and "- Result: DRAFT_REVIEW_COMPLETE" in body + and "Draft review-only request completed without publishing merge approval authority." + in body + ): + return True + return False + + def latest_current_head_coverage_change_request( pr: dict[str, Any], ) -> dict[str, Any] | None: @@ -3862,6 +3891,7 @@ def dispatch_opencode_review(repo: str, workflow: str, pr: dict[str, Any], *, dr "pr_base_sha": base_sha, "pr_head_ref": head_ref, "pr_head_sha": head_sha, + "draft_review_only": bool(pr.get("isDraft")), } complete_paginated_pr_contexts(target_repo, pr) required_run_id = matching_actions_run_id(pr, is_opencode_check_run) @@ -4233,7 +4263,11 @@ def dispatch_draft_review_only( # alone as a verdict would make a failed dispatch attempt permanently # block every later explicit retry. Only an actual current-head formal # review is a verdict. - if has_current_head_approval(pr) or has_current_head_changes_requested(pr): + if ( + has_current_head_approval(pr) + or has_current_head_changes_requested(pr) + or has_current_head_draft_review_completion(pr) + ): return Decision( number, "skip", diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index f84155e1a0..147f9e1026 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -140,6 +140,7 @@ def test_opencode_dispatch_never_uses_lossy_native_concurrency() -> None: "rejected target=", ), ({"SUPPLIED_HEAD_SHA": "mutable"}, "malformed PR identity metadata"), + ({"DRAFT_REVIEW_ONLY": "yes"}, "malformed draft-review authority"), ), ) def test_central_authorization_rejects_before_any_token_or_lease_access( @@ -179,6 +180,7 @@ def test_central_authorization_rejects_before_any_token_or_lease_access( "SUPPLIED_BASE_SHA": "b" * 40, "SUPPLIED_HEAD_REF": "feature", "SUPPLIED_HEAD_SHA": HEAD, + "DRAFT_REVIEW_ONLY": "false", **override, } result = subprocess.run( @@ -193,6 +195,199 @@ def test_central_authorization_rejects_before_any_token_or_lease_access( assert not calls.exists() +def test_central_admission_accepts_only_an_exact_authorized_draft_marker( + tmp_path: Path, +) -> None: + """An explicit Draft review reaches lease admission only with its live marker.""" + calls = tmp_path / "calls" + marker_name = f"cwl-draft-review-request-owner-repo-7-{HEAD}" + fake_gh = tmp_path / "gh" + fake_gh.write_text( + """#!/usr/bin/env bash +set -euo pipefail +printf '%s\n' "$*" >>"$CALLS" +if [[ "$*" == "api repos/owner/repo/pulls/7" ]]; then + jq -cn --arg base "$SUPPLIED_BASE_SHA" --arg head "$HEAD_SHA" \ + '{state:"open",draft:true,base:{ref:"main",sha:$base,repo:{full_name:"owner/repo"}},head:{ref:"feature",sha:$head,repo:{full_name:"owner/repo"}}}' +elif [[ "$*" == "api repos/ContextualWisdomLab/.github/actions/artifacts?name=$MARKER_NAME&per_page=100" ]]; then + jq -cn --arg name "$MARKER_NAME" '{total_count:1,artifacts:[{id:91,name:$name,expired:false}]}' +elif [[ "$*" == *"git/ref/heads/opencode-dispatch-leases"* ]]; then + printf '{}' +else + exit 97 +fi +""", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + result = subprocess.run( + [shutil.which("bash") or "/bin/bash", "-c", central_single_flight_script()], + env={ + **os.environ, + "PATH": f"{tmp_path}{os.pathsep}{os.environ.get('PATH', '')}", + "CALLS": str(calls), + "MARKER_NAME": marker_name, + "GITHUB_OUTPUT": str(tmp_path / "github-output"), + "GITHUB_REPOSITORY": "ContextualWisdomLab/.github", + "GITHUB_RUN_ID": "42", + "GITHUB_SHA": "a" * 40, + "TARGET_REPOSITORY": "owner/repo", + "PR_NUMBER": "7", + "HEAD_SHA": HEAD, + "SUPPLIED_BASE_REF": "main", + "SUPPLIED_BASE_SHA": "b" * 40, + "SUPPLIED_HEAD_REF": "feature", + "SUPPLIED_HEAD_SHA": HEAD, + "DRAFT_REVIEW_ONLY": "true", + "GH_TOKEN": "token", + "TARGET_READ_TOKEN": "target-token", + }, + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 1 + assert "Central OpenCode lease branch identity was malformed" in result.stdout + assert calls.read_text(encoding="utf-8").splitlines()[:3] == [ + "api repos/owner/repo/pulls/7", + ( + "api repos/ContextualWisdomLab/.github/actions/artifacts?" + f"name={marker_name}&per_page=100" + ), + "api repos/ContextualWisdomLab/.github/git/ref/heads/opencode-dispatch-leases", + ] + + +@pytest.mark.parametrize( + "artifact_payload", + ( + '{"total_count":1,"artifacts":[{"id":91,"name":"wrong","expired":false}]}', + '{"total_count":1,"artifacts":[{"id":91,"name":"MARKER","expired":true}]}', + '{"total_count":"1","artifacts":[]}', + ), +) +def test_central_admission_rejects_draft_without_a_valid_live_marker( + tmp_path: Path, artifact_payload: str +) -> None: + """Malformed, mismatched, or expired Draft authority fails before lease access.""" + calls = tmp_path / "calls" + marker_name = f"cwl-draft-review-request-owner-repo-7-{HEAD}" + fake_gh = tmp_path / "gh" + fake_gh.write_text( + """#!/usr/bin/env bash +set -euo pipefail +printf '%s\n' "$*" >>"$CALLS" +if [[ "$*" == "api repos/owner/repo/pulls/7" ]]; then + jq -cn --arg base "$SUPPLIED_BASE_SHA" --arg head "$HEAD_SHA" \ + '{state:"open",draft:true,base:{ref:"main",sha:$base,repo:{full_name:"owner/repo"}},head:{ref:"feature",sha:$head,repo:{full_name:"owner/repo"}}}' +elif [[ "$*" == *"/actions/artifacts?name="* ]]; then + printf '%s' "$ARTIFACT_PAYLOAD" | sed "s/MARKER/$MARKER_NAME/g" +else + exit 97 +fi +""", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + result = subprocess.run( + [shutil.which("bash") or "/bin/bash", "-c", central_single_flight_script()], + env={ + **os.environ, + "PATH": f"{tmp_path}{os.pathsep}{os.environ.get('PATH', '')}", + "CALLS": str(calls), + "MARKER_NAME": marker_name, + "ARTIFACT_PAYLOAD": artifact_payload, + "GITHUB_OUTPUT": str(tmp_path / "github-output"), + "GITHUB_REPOSITORY": "ContextualWisdomLab/.github", + "GITHUB_RUN_ID": "42", + "GITHUB_SHA": "a" * 40, + "TARGET_REPOSITORY": "owner/repo", + "PR_NUMBER": "7", + "HEAD_SHA": HEAD, + "SUPPLIED_BASE_REF": "main", + "SUPPLIED_BASE_SHA": "b" * 40, + "SUPPLIED_HEAD_REF": "feature", + "SUPPLIED_HEAD_SHA": HEAD, + "DRAFT_REVIEW_ONLY": "true", + "GH_TOKEN": "token", + "TARGET_READ_TOKEN": "target-token", + }, + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 1 + assert "authority changed before atomic OpenCode admission" in result.stdout + assert all("opencode-dispatch-leases" not in call for call in calls.read_text().splitlines()) + + +def test_central_admission_revalidates_draft_marker_before_lease_mutation( + tmp_path: Path, +) -> None: + """A marker that expires after admission cannot authorize a Contents write.""" + calls = tmp_path / "calls" + marker_reads = tmp_path / "marker-reads" + marker_name = f"cwl-draft-review-request-owner-repo-7-{HEAD}" + fake_gh = tmp_path / "gh" + fake_gh.write_text( + """#!/usr/bin/env bash +set -euo pipefail +printf '%s\n' "$*" >>"$CALLS" +if [[ "$*" == "api repos/owner/repo/pulls/7" ]]; then + jq -cn --arg base "$SUPPLIED_BASE_SHA" --arg head "$HEAD_SHA" \ + '{state:"open",draft:true,base:{ref:"main",sha:$base,repo:{full_name:"owner/repo"}},head:{ref:"feature",sha:$head,repo:{full_name:"owner/repo"}}}' +elif [[ "$*" == *"/actions/artifacts?name="* ]]; then + read_count=0 + [[ ! -f "$MARKER_READS" ]] || read_count="$(cat "$MARKER_READS")" + read_count=$((read_count + 1)) + printf '%s' "$read_count" >"$MARKER_READS" + if [[ "$read_count" -eq 1 ]]; then marker_expired=false; else marker_expired=true; fi + jq -cn --arg name "$MARKER_NAME" --argjson expired "$marker_expired" \ + '{total_count:1,artifacts:[{id:91,name:$name,expired:$expired}]}' +elif [[ "$*" == *"git/ref/heads/opencode-dispatch-leases"* ]]; then + jq -cn --arg sha "$GITHUB_SHA" '{object:{type:"commit",sha:$sha}}' +elif [[ "$*" == *"contents/opencode-dispatch-leases/"* && "$*" != *"--method PUT"* ]]; then + exit 1 +else + exit 97 +fi +""", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + result = subprocess.run( + [shutil.which("bash") or "/bin/bash", "-c", central_single_flight_script()], + env={ + **os.environ, + "PATH": f"{tmp_path}{os.pathsep}{os.environ.get('PATH', '')}", + "CALLS": str(calls), + "MARKER_READS": str(marker_reads), + "MARKER_NAME": marker_name, + "GITHUB_OUTPUT": str(tmp_path / "github-output"), + "GITHUB_REPOSITORY": "ContextualWisdomLab/.github", + "GITHUB_RUN_ID": "42", + "GITHUB_SHA": "a" * 40, + "TARGET_REPOSITORY": "owner/repo", + "PR_NUMBER": "7", + "HEAD_SHA": HEAD, + "SUPPLIED_BASE_REF": "main", + "SUPPLIED_BASE_SHA": "b" * 40, + "SUPPLIED_HEAD_REF": "feature", + "SUPPLIED_HEAD_SHA": HEAD, + "DRAFT_REVIEW_ONLY": "true", + "GH_TOKEN": "token", + "TARGET_READ_TOKEN": "target-token", + }, + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 1 + assert "authority changed before the atomic OpenCode lease mutation" in result.stdout + assert marker_reads.read_text(encoding="utf-8") == "2" + assert all("--method PUT" not in call for call in calls.read_text().splitlines()) + + @pytest.mark.parametrize( "live_override", ( @@ -267,6 +462,55 @@ def test_central_admission_rejects_changed_live_identity_before_contents_mutatio assert calls.read_text().splitlines() == ["api repos/owner/repo/pulls/7"] +def test_central_admission_rejects_draft_request_after_pr_becomes_ready( + tmp_path: Path, +) -> None: + """Draft-only authority cannot survive a Draft-to-Ready state transition.""" + calls = tmp_path / "calls" + fake_gh = tmp_path / "gh" + fake_gh.write_text( + """#!/usr/bin/env bash +set -euo pipefail +printf '%s\n' "$*" >>"$CALLS" +if [[ "$*" == "api repos/owner/repo/pulls/7" ]]; then + jq -cn --arg base "$SUPPLIED_BASE_SHA" --arg head "$HEAD_SHA" \ + '{state:"open",draft:false,base:{ref:"main",sha:$base,repo:{full_name:"owner/repo"}},head:{ref:"feature",sha:$head,repo:{full_name:"owner/repo"}}}' + exit 0 +fi +exit 97 +""", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + result = subprocess.run( + [shutil.which("bash") or "/bin/bash", "-c", central_single_flight_script()], + env={ + **os.environ, + "PATH": f"{tmp_path}{os.pathsep}{os.environ.get('PATH', '')}", + "CALLS": str(calls), + "GITHUB_OUTPUT": str(tmp_path / "github-output"), + "GITHUB_RUN_ID": "42", + "GITHUB_SHA": "a" * 40, + "TARGET_REPOSITORY": "owner/repo", + "PR_NUMBER": "7", + "HEAD_SHA": HEAD, + "SUPPLIED_BASE_REF": "main", + "SUPPLIED_BASE_SHA": "b" * 40, + "SUPPLIED_HEAD_REF": "feature", + "SUPPLIED_HEAD_SHA": HEAD, + "DRAFT_REVIEW_ONLY": "true", + "GH_TOKEN": "token", + "TARGET_READ_TOKEN": "target-token", + }, + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 1 + assert "authority changed before atomic OpenCode admission" in result.stdout + assert calls.read_text().splitlines() == ["api repos/owner/repo/pulls/7"] + + @pytest.mark.parametrize( ( "lease_state", @@ -1355,6 +1599,7 @@ def test_formal_receipt_wake_reruns_the_immediately_failed_required_job() -> Non wake_step = dispatched.split( "Wake every failed exact-head Required OpenCode workflow", 1 )[1].split("\n\n - name:", 1)[0] + assert "needs.validate-pr-metadata.outputs.is_draft == 'false'" in wake_step target_job = dispatched.split(" opencode-review-target:\n", 1)[1] target_permissions = target_job.split(" env:\n", 1)[0] assert "actions: write" in target_permissions @@ -1374,6 +1619,41 @@ def test_formal_receipt_wake_reruns_the_immediately_failed_required_job() -> Non assert 'workflow_url | contains("/actions/required_workflows/")' not in wake_step +def test_authorized_draft_review_cannot_publish_approval_or_run_merge_followups() -> None: + """Review-only Draft work publishes prose but cannot create approval authority.""" + dispatched = DISPATCH_WORKFLOW.read_text(encoding="utf-8") + fast_approval = dispatched.split( + " - name: Publish central OpenCode fast approval", 1 + )[1].split(" - name: Publish OpenCode review outcome", 1)[0] + publication = dispatched.split( + " - name: Publish OpenCode review outcome", 1 + )[1].split(" - name: Enforce current-head formal OpenCode review receipt", 1)[0] + formal_receipt = dispatched.split( + " - name: Enforce current-head formal OpenCode review receipt", 1 + )[1].split(" - name: Wake every failed exact-head Required OpenCode workflow", 1)[0] + status_publication = dispatched.split( + " - name: Publish repository_dispatch OpenCode status", 1 + )[1].split(" - name: Dispatch Noema after current-head OpenCode approval", 1)[0] + noema_handoff = dispatched.split( + " - name: Dispatch Noema after current-head OpenCode approval", 1 + )[1].split(" - name: Run merge scheduler after approval", 1)[0] + merge_followup = dispatched.split( + " - name: Run merge scheduler after approval", 1 + )[1] + + assert "needs.validate-pr-metadata.outputs.is_draft == 'false'" in fast_approval + assert "PR_DRAFT: ${{ needs.validate-pr-metadata.outputs.is_draft }}" in publication + assert ( + 'if [ "$event" = "APPROVE" ] && [ "$PR_DRAFT" = "true" ]; then' + in publication + ) + assert 'event="COMMENT"' in publication + assert "needs.validate-pr-metadata.outputs.is_draft == 'false'" in formal_receipt + assert "needs.validate-pr-metadata.outputs.is_draft == 'false'" in status_publication + assert "needs.validate-pr-metadata.outputs.is_draft == 'false'" in noema_handoff + assert "needs.validate-pr-metadata.outputs.is_draft == 'false'" in merge_followup + + def wake_selector(run: dict[str, object], *, head: str = HEAD) -> str: """Execute the wake inventory's fail-closed jq program in isolation.""" jq = shutil.which("jq") diff --git a/tests/test_pr_review_autofix_nvidia_nim_contract.py b/tests/test_pr_review_autofix_nvidia_nim_contract.py index 2d0078242b..36e125b601 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 = "5c87c74863ef6872c1ef7136d5b330071920c09e" +REVIEW_DISPATCH_BLOB_SHA = "bd1c9894e98160e1b329470521c3a29c6ac77497" def _workflow_text(path: Path) -> str: diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index abe5a411c7..14492e2734 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -5606,11 +5606,45 @@ def fake_run_with_env(args, *, stdin=None, env=None): "pr_base_sha": base_sha, "pr_head_ref": "feature", "pr_head_sha": head_sha, + "draft_review_only": False, "required_run_id": 42, }, } +def test_draft_review_dispatch_carries_explicit_review_only_authority(monkeypatch): + """The receiver can distinguish an authorized Draft review from merge work.""" + monkeypatch.setenv("GITHUB_ACTIONS", "true") + monkeypatch.setenv("GH_TOKEN", "opencode-app-token") + monkeypatch.setattr(sched, "active_opencode_run_refs", lambda *args: ([], [])) + monkeypatch.setattr(sched, "_cancel_revalidated_review_run_refs", lambda *args: ([], [])) + monkeypatch.setattr(sched, "review_dispatch_admitted", lambda *args: True) + monkeypatch.setattr(sched, "live_dispatch_head_matches", lambda *args: True) + monkeypatch.setattr(sched, "complete_paginated_pr_contexts", lambda *args: None) + monkeypatch.setattr(sched, "matching_actions_run_id", lambda *args: None) + monkeypatch.setattr(sched, "discover_opencode_required_run_id", lambda *args: None) + monkeypatch.setattr(sched, "reset_active_workflow_runs_cache", lambda: None) + dispatch_payloads = [] + monkeypatch.setattr( + sched, + "run_github_dispatch", + lambda _args, stdin=None: dispatch_payloads.append(json.loads(stdin)), + ) + pull_request = make_pr( + isDraft=True, + baseRefOid="b" * 40, + headRefOid="a" * 40, + ) + + assert ( + sched.dispatch_opencode_review( + "owner/repo", "OpenCode Review", pull_request, dry_run=False + ) + == "dispatched" + ) + assert dispatch_payloads[0]["client_payload"]["draft_review_only"] is True + + def test_central_required_workflow_waits_without_cross_repo_dispatch_credential(monkeypatch): monkeypatch.setenv("SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", "ContextualWisdomLab/.github") monkeypatch.setenv("SCHEDULER_REQUIRED_WORKFLOW_REF", "main") @@ -8282,6 +8316,67 @@ def test_draft_pr_review_only_dispatch_skips_when_a_current_head_verdict_exists( "draft PR review-only dispatch; current-head OpenCode verdict already exists" ) + draft_completion = { + **opencode_review("COMMENTED", "head"), + "body": ( + "OpenCode review\n\n" + "- Result: DRAFT_REVIEW_COMPLETE\n" + "- Head SHA: `head`\n\n" + "Draft review-only request completed without publishing merge approval authority." + ), + } + completed_draft = make_pr(isDraft=True, reviews={"nodes": [draft_completion]}) + completed_decision = inspect(completed_draft, allow_draft_review_dispatch=True) + assert completed_decision.action == "skip" + assert completed_decision.reason == ( + "draft PR review-only dispatch; current-head OpenCode verdict already exists" + ) + + +def test_draft_review_completion_requires_exact_head_and_both_formal_markers(): + """A stale or generic comment cannot suppress an explicit Draft review.""" + complete_body = ( + "OpenCode review\n\n" + "- Result: DRAFT_REVIEW_COMPLETE\n\n" + "Draft review-only request completed without publishing merge approval authority." + ) + stale = { + **opencode_review("COMMENTED", "old-head"), + "body": complete_body, + } + missing_explanation = { + **opencode_review("COMMENTED", "head"), + "body": "OpenCode review\n\n- Result: DRAFT_REVIEW_COMPLETE", + } + generic = { + **opencode_review("COMMENTED", "head"), + "body": "OpenCode is still running.", + } + + assert not sched.has_current_head_draft_review_completion( + make_pr(isDraft=True, reviews={"nodes": [stale]}) + ) + assert not sched.has_current_head_draft_review_completion( + make_pr(isDraft=True, reviews={"nodes": [missing_explanation]}) + ) + assert not sched.has_current_head_draft_review_completion( + make_pr(isDraft=True, reviews={"nodes": [generic]}) + ) + assert sched.has_current_head_draft_review_completion( + make_pr( + isDraft=True, + reviews={ + "nodes": [ + { + **opencode_review("COMMENTED", "head"), + "body": complete_body, + }, + generic, + ] + }, + ) + ) + def test_draft_pr_review_only_dispatch_retries_a_failed_required_check_with_no_verdict(): """A completed-but-failed required-workflow check is not a posted review: From 6eb1174e4f9c5733bfa89266448ca33c839b4061 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 1 Oct 2026 19:52:00 +0900 Subject: [PATCH 09/10] fix: preserve Draft-to-Ready review admission --- .../workflows/opencode-review-dispatch.yml | 6 +-- .github/workflows/opencode-review.yml | 3 +- CHANGELOG.md | 4 ++ ...ncode-authorized-draft-review-admission.md | 9 +++- docs/product-technical-gap-baseline.md | 2 +- scripts/ci/pr_review_merge_scheduler_core.py | 32 ++++++++++++- ...st_opencode_required_verdict_regression.py | 10 +++++ ...t_pr_review_autofix_nvidia_nim_contract.py | 2 +- tests/test_pr_review_merge_scheduler.py | 45 +++++++++++++++++++ .../test_required_workflow_queue_contract.py | 2 + 10 files changed, 107 insertions(+), 8 deletions(-) diff --git a/.github/workflows/opencode-review-dispatch.yml b/.github/workflows/opencode-review-dispatch.yml index bd1c9894e9..ac80537dac 100644 --- a/.github/workflows/opencode-review-dispatch.yml +++ b/.github/workflows/opencode-review-dispatch.yml @@ -43,7 +43,7 @@ jobs: SUPPLIED_BASE_SHA: ${{ github.event.client_payload.pr_base_sha || '' }} SUPPLIED_HEAD_REF: ${{ github.event.client_payload.pr_head_ref || '' }} SUPPLIED_HEAD_SHA: ${{ github.event.client_payload.pr_head_sha || '' }} - DRAFT_REVIEW_ONLY: ${{ toJSON(github.event.client_payload.draft_review_only || false) }} + DRAFT_REVIEW_ONLY: ${{ toJSON(github.event.client_payload.draft_review_only) }} run: | set -euo pipefail [ "$EVENT_NAME" = "repository_dispatch" ] || { @@ -181,7 +181,7 @@ jobs: SUPPLIED_BASE_SHA: ${{ github.event.client_payload.pr_base_sha }} SUPPLIED_HEAD_REF: ${{ github.event.client_payload.pr_head_ref }} SUPPLIED_HEAD_SHA: ${{ github.event.client_payload.pr_head_sha }} - DRAFT_REVIEW_ONLY: ${{ toJSON(github.event.client_payload.draft_review_only || false) }} + DRAFT_REVIEW_ONLY: ${{ toJSON(github.event.client_payload.draft_review_only) }} run: | set -euo pipefail exact_title="OpenCode Review Dispatch ${TARGET_REPOSITORY}#${PR_NUMBER}@${HEAD_SHA}" @@ -562,7 +562,7 @@ jobs: SUPPLIED_BASE_SHA: ${{ github.event.client_payload.pr_base_sha || '' }} SUPPLIED_HEAD_REF: ${{ github.event.client_payload.pr_head_ref || '' }} SUPPLIED_HEAD_SHA: ${{ github.event.client_payload.pr_head_sha || '' }} - DRAFT_REVIEW_ONLY: ${{ toJSON(github.event.client_payload.draft_review_only || false) }} + DRAFT_REVIEW_ONLY: ${{ toJSON(github.event.client_payload.draft_review_only) }} run: | set -euo pipefail if [ "$EVENT_NAME" = "repository_dispatch" ]; then diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index ff791c5922..4918b5c641 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -635,7 +635,8 @@ jobs: --arg pr_head_ref "$HEAD_REF" \ --arg pr_head_sha "$HEAD_SHA" \ --arg required_run_id "$GITHUB_RUN_ID" \ - '{event_type:"opencode-review",client_payload:{target_repository:$target_repository,pr_number:$pr_number,pr_base_ref:$pr_base_ref,pr_base_sha:$pr_base_sha,pr_head_ref:$pr_head_ref,pr_head_sha:$pr_head_sha,required_run_id:$required_run_id}}' | + --argjson draft_review_only false \ + '{event_type:"opencode-review",client_payload:{target_repository:$target_repository,pr_number:$pr_number,pr_base_ref:$pr_base_ref,pr_base_sha:$pr_base_sha,pr_head_ref:$pr_head_ref,pr_head_sha:$pr_head_sha,draft_review_only:$draft_review_only,required_run_id:$required_run_id}}' | GH_TOKEN="$app_token" gh api -X POST repos/ContextualWisdomLab/.github/dispatches --input - - name: Fail closed without a current-head OpenCode verdict diff --git a/CHANGELOG.md b/CHANGELOG.md index fd31639c93..92110230a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ authority. A clean Draft publishes an exact-head formal comment with `DRAFT_REVIEW_COMPLETE`, never an approval or a merge-authorizing receipt; the scheduler recognizes only that source-backed comment as completion. + The Ready-only Required producer now sends an explicit boolean `false`, and + a Draft completion lease is retired when the same head becomes Ready so the + mode-independent admission identity cannot suppress the required approval + review. Review-only runs never publish Ready status, dispatch Noema, invoke the merge scheduler, or wake merge-required OpenCode jobs. This reconnects the already-authorized agent-mention/scheduler path without weakening ordinary diff --git a/docs/doctoring/opencode-authorized-draft-review-admission.md b/docs/doctoring/opencode-authorized-draft-review-admission.md index 61d82b75dd..247cf8eb79 100644 --- a/docs/doctoring/opencode-authorized-draft-review-admission.md +++ b/docs/doctoring/opencode-authorized-draft-review-admission.md @@ -34,6 +34,12 @@ Ready or to weaken ordinary merge admission. - A Draft-to-Ready transition invalidates Draft authority before lease access; a validated Draft that changes later still publishes only a non-authorizing comment. +- The Ready-only Required producer always sends typed `draft_review_only=false`; + omitted and malformed types fail closed at the receiver. +- Because the durable admission identity intentionally remains + repository/PR/head/component, reconciliation retires an exact-head Draft + completion lease when that pull request becomes Ready without an approving + verdict. The same head can then acquire a fresh Ready review lease. ## Executable acceptance @@ -44,7 +50,8 @@ the dispatch payload distinguishes Ready and Draft work, and the Required workflow wake is structurally disabled for a validated Draft. It also proves that the Draft path cannot publish approval authority or start any Ready-only follow-up, while its exact-head formal completion comment prevents an unbounded -same-head redispatch loop. The complete affected scheduler and receiver suite +same-head redispatch loop. A transition regression proves Draft completion, +same-head Ready retirement, and fresh Ready redispatch. The complete affected scheduler and receiver suite must pass both locally and on the exact published head. Hosted security, provenance, and independent semantic review remain required before ordinary protected integration. diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 3f53b60295..24dc6a4e09 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -4,7 +4,7 @@ | Gap | Exact evidence | Action | Status | |---|---|---|---| -| The agent-mention workflow and scheduler created an exact repository/PR/head durable Draft-review request, but the central receiver unconditionally required `live_draft=false`; every explicitly authorized Draft review therefore stopped before lease admission and could never produce the semantic review it requested. Once admitted, the clean-verdict publisher also attempted `APPROVED` before its downstream Draft receipt guard, which could grant merge authority to review-only work | `.github#2546@db81de7a09268eb0abfc5b5c675ddd9a0a38a3e2`; `.github/workflows/agent-mention-opencode-dispatch.yml` marker `cwl-draft-review-request---`; `.github/workflows/opencode-review-dispatch.yml` former `live_authority_matches` and unconditional clean-verdict approval; local RED contracts in `tests/test_opencode_required_verdict_regression.py` and `tests/test_pr_review_merge_scheduler.py` | Carry a typed review-only boolean in the scheduler payload, reject malformed authority before token exchange, and admit a live Draft only after re-fetching a non-expired exact marker from the central Actions artifact API. Revalidate state/base/head plus marker before every lease mutation. Convert a clean Draft verdict to an exact-head `DRAFT_REVIEW_COMPLETE` formal comment before publication; accept only that source-backed comment as scheduler completion, and structurally skip formal merge receipt, Ready status, Noema, merge-scheduler, and Required-workflow wake follow-ups. See [RCA and executable acceptance](doctoring/opencode-authorized-draft-review-admission.md) | **Proposed / local RED→GREEN complete; exact-head hosted Checks and independent semantic review required** | +| The agent-mention workflow and scheduler created an exact repository/PR/head durable Draft-review request, but the central receiver unconditionally required `live_draft=false`; every explicitly authorized Draft review therefore stopped before lease admission and could never produce the semantic review it requested. Once admitted, the clean-verdict publisher also attempted `APPROVED` before its downstream Draft receipt guard, which could grant merge authority to review-only work. Finally, a completed Draft lease shared the Ready identity and could suppress same-head Ready review forever | `.github#2546@db81de7a09268eb0abfc5b5c675ddd9a0a38a3e2`; `.github/workflows/agent-mention-opencode-dispatch.yml` marker `cwl-draft-review-request---`; `.github/workflows/opencode-review-dispatch.yml` former `live_authority_matches` and unconditional clean-verdict approval; mode-independent admission identity in `scripts/ci/review_admission_controller.py`; local RED contracts in `tests/test_opencode_required_verdict_regression.py` and `tests/test_pr_review_merge_scheduler.py` | Carry a typed review-only boolean in every producer payload, reject omitted/malformed authority before token exchange, and admit a live Draft only after re-fetching a non-expired exact marker from the central Actions artifact API. Revalidate state/base/head plus marker before every lease mutation. Convert a clean Draft verdict to an exact-head `DRAFT_REVIEW_COMPLETE` formal comment before publication; accept only that source-backed comment as Draft scheduler completion, retire that lease if the same head becomes Ready without approval, and structurally skip formal merge receipt, Ready status, Noema, merge-scheduler, and Required-workflow wake follow-ups. See [RCA and executable acceptance](doctoring/opencode-authorized-draft-review-admission.md) | **Proposed / local RED→GREEN complete; exact-head hosted Checks and independent semantic review required** | ## 2026-10-01 OpenCode same-head dispatch idempotency diff --git a/scripts/ci/pr_review_merge_scheduler_core.py b/scripts/ci/pr_review_merge_scheduler_core.py index 8a1ac3fb5e..370d43aeb1 100644 --- a/scripts/ci/pr_review_merge_scheduler_core.py +++ b/scripts/ci/pr_review_merge_scheduler_core.py @@ -70,6 +70,34 @@ def admit(self, component: str, repository: str, pr: dict[str, Any]) -> bool: def lease(state): """Apply this request to `state` and record any lease it wins.""" + prior_record = state.records.get(request.identity) + if ( + component == "opencode" + and not bool(pr.get("isDraft")) + and prior_record is not None + and has_current_head_draft_review_completion(pr) + and not has_current_head_approval(pr) + and not has_current_head_changes_requested(pr) + and ( + prior_record.status == "complete" + or ( + prior_record.status == "dispatched" + and opencode_progress_state( + pr, stale_after_minutes=DEFAULT_STALE_OPENCODE_MINUTES + ) + != "running" + ) + ) + ): + # The durable identity intentionally omits Draft/Ready mode. + # Retire only a terminal prior lease at the moment a Ready + # dispatch asks for admission; an in-flight Ready lease keeps + # its bounded worker slot on subsequent reconciliations. + updated_records = dict(state.records) + updated_records[request.identity] = RequestRecord( + prior_record.request, "stale" + ) + state = type(state)(updated_records, dict(state.latest_sequences)) plan = plan_dispatches( state, [request], @@ -94,10 +122,12 @@ def reconcile_state(state): records = dict(state.records) latest = dict(state.latest_sequences) for identity, record in tuple(records.items()): - if record.status != "dispatched" or record.request.repository != repository: + if record.request.repository != repository: continue pr = live_prs.get(record.request.pull_request) live_head = str((pr or {}).get("headRefOid") or "").lower() + if record.status != "dispatched": + continue if live_head != record.request.head_sha: records[identity] = RequestRecord(record.request, "stale") continue diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index 147f9e1026..9a080bb915 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -1008,6 +1008,8 @@ def test_required_workflow_cannot_succeed_with_an_echo_only_placeholder() -> Non assert "id-token: write" in target_job.split(" steps:\n", 1)[0] assert 'event_type:"opencode-review"' in workflow assert "required_run_id:$required_run_id" in workflow + assert "--argjson draft_review_only false" in workflow + assert "draft_review_only:$draft_review_only" in workflow dispatch_step = target_job.split( " - name: Request current-head OpenCode review execution", 1 )[1].split(" - name: Fail closed", 1)[0] @@ -1654,6 +1656,14 @@ def test_authorized_draft_review_cannot_publish_approval_or_run_merge_followups( assert "needs.validate-pr-metadata.outputs.is_draft == 'false'" in merge_followup +def test_dispatch_preserves_draft_review_authority_type_before_validation() -> None: + """Falsy non-booleans cannot be normalized into Ready review authority.""" + dispatched = DISPATCH_WORKFLOW.read_text(encoding="utf-8") + raw_binding = "toJSON(github.event.client_payload.draft_review_only)" + assert dispatched.count(raw_binding) == 3 + assert "draft_review_only || false" not in dispatched + + def wake_selector(run: dict[str, object], *, head: str = HEAD) -> str: """Execute the wake inventory's fail-closed jq program in isolation.""" jq = shutil.which("jq") diff --git a/tests/test_pr_review_autofix_nvidia_nim_contract.py b/tests/test_pr_review_autofix_nvidia_nim_contract.py index 36e125b601..a702f79f6c 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 = "bd1c9894e98160e1b329470521c3a29c6ac77497" +REVIEW_DISPATCH_BLOB_SHA = "ac80537dacf2bb0d20c0716c76b460da0b493f52" def _workflow_text(path: Path) -> str: diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 14492e2734..33c9237154 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -11084,6 +11084,51 @@ def test_bounded_admission_persists_leases_and_completes_only_current_head( assert [record.status for record in persisted.records.values()].count("dispatched") == 1 +def test_ready_transition_retires_same_head_draft_completion_lease(tmp_path): + """A Draft comment cannot permanently suppress Ready review on the same head.""" + state_path = tmp_path / "admission.json" + gate = sched.SchedulerAdmissionGate(state_path, sequence=78, dispatch_budget=1) + completion = { + **opencode_review("COMMENTED", "a" * 40), + "body": ( + "OpenCode review\n\n" + "- Result: DRAFT_REVIEW_COMPLETE\n\n" + "Draft review-only request completed without publishing merge approval authority." + ), + } + draft_pr = make_pr( + number=7, + isDraft=True, + headRefOid="a" * 40, + reviews={"nodes": [completion]}, + ) + assert gate.admit("opencode", "ContextualWisdomLab/example", draft_pr) + gate.reconcile("ContextualWisdomLab/example", [draft_pr]) + + from scripts.ci.review_admission_controller import load_state_file + + completed = next(iter(load_state_file(state_path).records.values())) + assert completed.status == "complete" + + ready_pr = { + **draft_pr, + "isDraft": False, + "statusCheckRollup": { + "contexts": {"nodes": [opencode_check(status="IN_PROGRESS")]} + }, + } + ready_gate = sched.SchedulerAdmissionGate( + state_path, sequence=79, dispatch_budget=1 + ) + assert ready_gate.admit("opencode", "ContextualWisdomLab/example", ready_pr) + admitted = next(iter(load_state_file(state_path).records.values())) + assert admitted.status == "dispatched" + + ready_gate.reconcile("ContextualWisdomLab/example", [ready_pr]) + preserved = next(iter(load_state_file(state_path).records.values())) + assert preserved.status == "dispatched" + + def test_actual_opencode_dispatch_path_obeys_one_shared_admission_budget( monkeypatch, tmp_path ): diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index 12560f0d05..93d0bba8c4 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -679,6 +679,8 @@ def test_required_opencode_dispatch_does_not_wait_on_merge_scheduler() -> None: assert 'event_type:"opencode-review"' in dispatch assert 'event_type:"merge-scheduler"' not in dispatch assert 'required_run_id:$required_run_id' in dispatch + assert "--argjson draft_review_only false" in dispatch + assert "draft_review_only:$draft_review_only" in dispatch for field in ( "target_repository", "pr_number", From 8b81426c6708b042bc3db51c3de1cfdc1d1a0a21 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 1 Oct 2026 20:02:16 +0900 Subject: [PATCH 10/10] fix: restore scheduler coverage guard --- scripts/ci/pr_review_merge_scheduler_core.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/scripts/ci/pr_review_merge_scheduler_core.py b/scripts/ci/pr_review_merge_scheduler_core.py index 370d43aeb1..09a248c3fb 100644 --- a/scripts/ci/pr_review_merge_scheduler_core.py +++ b/scripts/ci/pr_review_merge_scheduler_core.py @@ -122,12 +122,10 @@ def reconcile_state(state): records = dict(state.records) latest = dict(state.latest_sequences) for identity, record in tuple(records.items()): - if record.request.repository != repository: + if record.status != "dispatched" or record.request.repository != repository: continue pr = live_prs.get(record.request.pull_request) live_head = str((pr or {}).get("headRefOid") or "").lower() - if record.status != "dispatched": - continue if live_head != record.request.head_sha: records[identity] = RequestRecord(record.request, "stale") continue