diff --git a/.github/actions/dismiss-stale-approvals/check-manual-merge-resolutions.sh b/.github/actions/dismiss-stale-approvals/check-manual-merge-resolutions.sh index 2fcae70..64f998a 100755 --- a/.github/actions/dismiss-stale-approvals/check-manual-merge-resolutions.sh +++ b/.github/actions/dismiss-stale-approvals/check-manual-merge-resolutions.sh @@ -29,12 +29,25 @@ run_check_manual_merge_resolutions() { return 1 fi - if ! run_git "$repository_path" merge-base --is-ancestor "$approval_sha" "$head_sha"; then - printf 'stale=true\n' - printf 'reason=Latest approval commit %s is not an ancestor of %s, indicating rewritten history after approval\n' "$approval_sha" "$head_sha" + local merge_base_status + if run_git "$repository_path" merge-base --is-ancestor "$approval_sha" "$head_sha"; then + merge_base_status=0 + else + merge_base_status=$? + fi + + if [[ $merge_base_status -eq 1 ]]; then + # Rebases and restacks rewrite commit IDs. Non-ancestor alone does not prove + # that the reviewed patch series changed, so let range-diff decide that. + printf 'stale=false\nreason=\n' return 0 fi + if [[ $merge_base_status -ne 0 ]]; then + echo "merge-base --is-ancestor failed for ${approval_sha}..${head_sha}" >&2 + return "$merge_base_status" + fi + local merge_commit merge_commits merge_tree_output merge_tree_status conflict_files reason if ! merge_commits="$(run_git "$repository_path" rev-list --merges "${approval_sha}..${head_sha}")"; then diff --git a/.github/actions/dismiss-stale-approvals/self-test.sh b/.github/actions/dismiss-stale-approvals/self-test.sh index c9d0e5f..b75aded 100755 --- a/.github/actions/dismiss-stale-approvals/self-test.sh +++ b/.github/actions/dismiss-stale-approvals/self-test.sh @@ -171,7 +171,7 @@ test_decision_keeps_successful_no_approval_path_safe() { fail "Expected a successful no-approval decision to stay non-stale, got:"$'\n'"$output" } -test_rewritten_history_is_stale() { +test_rewritten_history_defers_to_range_diff() { local repo_path repo_path="$(mktemp -d)" register_temp_path "$repo_path" @@ -197,8 +197,100 @@ test_rewritten_history_is_stale() { local output output="$(run_check_manual_merge_resolutions "$repo_path" "$approval_sha" "$head_sha")" - assert_contains "$output" "stale=true" - assert_contains "$output" "rewritten history" + [[ "$output" == $'stale=false\nreason=' ]] || + fail "Expected rewritten history without descendant ancestry to defer to range-diff, got:"$'\n'"$output" +} + +test_rewritten_history_with_changed_patch_series_is_stale_via_range_diff() { + local repo_path output decision_output + repo_path="$(mktemp -d)" + register_temp_path "$repo_path" + + create_repo "$repo_path" + + printf 'base\n' >"$repo_path/file.txt" + git -C "$repo_path" add file.txt + git -C "$repo_path" commit -qm "base" + local prev_base_sha + prev_base_sha="$(git -C "$repo_path" rev-parse HEAD)" + + printf 'approved\n' >"$repo_path/file.txt" + git -C "$repo_path" commit -qam "approved" + local approval_sha prev_head_sha + approval_sha="$(git -C "$repo_path" rev-parse HEAD)" + prev_head_sha="$approval_sha" + + git -C "$repo_path" reset --hard HEAD~1 >/dev/null + + printf 'rewritten and changed\n' >"$repo_path/file.txt" + git -C "$repo_path" commit -qam "rewritten" + local head_sha base_sha + head_sha="$(git -C "$repo_path" rev-parse HEAD)" + base_sha="$(git -C "$repo_path" rev-parse HEAD~1)" + + output="$(run_check_manual_merge_resolutions "$repo_path" "$approval_sha" "$head_sha")" + [[ "$output" == $'stale=false\nreason=' ]] || + fail "Expected rewritten history check to defer to range-diff, got:"$'\n'"$output" + + local range_diff_output + range_diff_output="$( + run_check_range_diff_stale \ + --repository-path "$repo_path" \ + --prev-base-sha "$prev_base_sha" \ + --prev-head-sha "$prev_head_sha" \ + --base-sha "$base_sha" \ + --head-sha "$head_sha" + )" + assert_contains "$range_diff_output" "stale=true" + + decision_output="$(run_decide_stale_approvals "success" "success" "false" "" "success" "true" "0" "https://example.test/run")" + assert_contains "$decision_output" "stale=true" + assert_contains "$decision_output" 'See the output of `git range-diff`' +} + +test_merge_base_failure_returns_nonzero() { + local repo_path output status + repo_path="$(mktemp -d)" + register_temp_path "$repo_path" + + create_repo "$repo_path" + + printf 'base\n' >"$repo_path/file.txt" + git -C "$repo_path" add file.txt + git -C "$repo_path" commit -qm "base" + + printf 'approved\n' >"$repo_path/file.txt" + git -C "$repo_path" commit -qam "approved" + local approval_sha + approval_sha="$(git -C "$repo_path" rev-parse HEAD)" + + printf 'head\n' >"$repo_path/file.txt" + git -C "$repo_path" commit -qam "head" + local head_sha + head_sha="$(git -C "$repo_path" rev-parse HEAD)" + + run_git() { + local repository_path="$1" + shift + + if [[ "$1" == "merge-base" ]]; then + echo "forced merge-base failure" >&2 + return 42 + fi + + git -C "$repository_path" "$@" + } + + if output="$(run_check_manual_merge_resolutions "$repo_path" "$approval_sha" "$head_sha" 2>/dev/null)"; then + status=0 + else + status=$? + fi + + # shellcheck disable=SC1091 + source "$ACTION_DIR/check-manual-merge-resolutions.sh" + + [[ $status -ne 0 ]] || fail "Expected merge-base failure to propagate as non-zero, got output:"$'\n'"$output" } test_rev_list_failure_returns_nonzero() { @@ -521,7 +613,9 @@ main() { test_no_approval_is_not_stale test_decision_marks_approval_lookup_failure_stale test_decision_keeps_successful_no_approval_path_safe - test_rewritten_history_is_stale + test_rewritten_history_defers_to_range_diff + test_rewritten_history_with_changed_patch_series_is_stale_via_range_diff + test_merge_base_failure_returns_nonzero test_rev_list_failure_returns_nonzero test_missing_approval_commit_is_stale test_bare_repo_conflict_merge_is_stale diff --git a/.github/workflows/preflight-stale-approvals.yml b/.github/workflows/preflight-stale-approvals.yml index 067f0b4..644f348 100644 --- a/.github/workflows/preflight-stale-approvals.yml +++ b/.github/workflows/preflight-stale-approvals.yml @@ -34,6 +34,7 @@ jobs: steps: - name: Resolve reusable workflow ref id: workflow-ref + if: github.event_name != 'pull_request' uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0 with: script: | @@ -43,7 +44,17 @@ jobs: const ref = job_workflow_ref.split('@')[1]; core.setOutput('ref', ref); - - name: Checkout action + - name: Checkout action from pull request + if: github.event_name == 'pull_request' + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + repository: ${{ github.event.pull_request.head.repo.full_name }} + ref: ${{ github.event.pull_request.head.sha }} + sparse-checkout: | + .github/actions/dismiss-stale-approvals + + - name: Checkout action from reusable workflow ref + if: github.event_name != 'pull_request' uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: repository: hashintel/.github