diff --git a/plugins/tend-ci-runner/skills/review/SKILL.md b/plugins/tend-ci-runner/skills/review/SKILL.md index 6375bb16..67137cf8 100644 --- a/plugins/tend-ci-runner/skills/review/SKILL.md +++ b/plugins/tend-ci-runner/skills/review/SKILL.md @@ -46,14 +46,31 @@ SUBSTANTIVE=$(gh api --paginate "repos/$REPO/pulls//comments" \ # IMPORTANT: REST reviews carry `.commit_id`, NOT the `.commit.oid` that # `gh pr view --json reviews` returns — don't confuse the two. `gh api --jq` # accepts no `--arg`/`--argjson`, so pipe to `jq` rather than using `--jq`. -LAST_REVIEW_SHA=$(gh api --paginate "repos/$REPO/pulls//reviews" \ - | jq -rs --argjson sub "$SUBSTANTIVE" --arg bot "$BOT_LOGIN" \ +LAST_REVIEW=$(gh api --paginate "repos/$REPO/pulls//reviews" \ + | jq -s --argjson sub "$SUBSTANTIVE" --arg bot "$BOT_LOGIN" \ 'add | [.[] | select(.user.login == $bot) | select((.body | length) > 0 or (.id | IN($sub[])) or .state == "APPROVED")] - | last | .commit_id // empty') + | last // {}') +LAST_REVIEW_SHA=$(jq -r '.commit_id // empty' <<<"$LAST_REVIEW") + +# A force-push does not just move HEAD — GitHub re-points the prior review's +# `.commit_id` at the NEW head, so `LAST_REVIEW_SHA == HEAD_SHA` reads true for +# code nothing ever reviewed and the run exits silently, leaving a stale APPROVE +# standing as the active verdict. (An ordinary push leaves the old anchor +# intact; only a rewrite re-points it, so `.commit_id` alone can't tell the two +# apart.) Probe the timeline for a rewrite newer than that review. +LAST_REVIEW_AT=$(jq -r '.submitted_at // empty' <<<"$LAST_REVIEW") +FORCE_PUSHED=0 +if [ -n "$LAST_REVIEW_AT" ]; then + FORCE_PUSHED=$(gh api --paginate "repos/$REPO/issues//timeline" \ + | jq -s --arg at "$LAST_REVIEW_AT" \ + 'add | [.[] | select(.event == "head_ref_force_pushed" and .created_at > $at)] | length') +fi ``` -If `LAST_REVIEW_SHA == HEAD_SHA`, this commit has already been reviewed — finish at step 9 without posting. Two exceptions: an unanswered conversation question directed at the bot (check below), or `EVENT_ACTION == "ready_for_review"` (the PR just transitioned out of draft, so any prior review was a draft-mode review and the author is now asking for a full one — proceed). +If `FORCE_PUSHED` is non-zero, the commit the bot reviewed was rewritten away: ignore `LAST_REVIEW_SHA` entirely and review `HEAD_SHA` in full. The incremental below can't run either — `LAST_REVIEW_SHA` now names the current head rather than anything the bot read, so `LAST_REVIEW_SHA..HEAD_SHA` is empty and every trivial-skip heuristic keyed on it under-reports. If that prior review was an `APPROVED` and the re-review lands on findings rather than an approval, dismiss it too — it is re-anchored onto the rewritten head, so posting a COMMENT alone leaves the PR reading as bot-approved. `jq -r '.state, .id' <<<"$LAST_REVIEW"` gives the state and the `$REVIEW_ID` for step 6's `reviews/$REVIEW_ID/dismissals` call. + +Otherwise, if `LAST_REVIEW_SHA == HEAD_SHA`, this commit has already been reviewed — finish at step 9 without posting. Two exceptions: an unanswered conversation question directed at the bot (check below), or `EVENT_ACTION == "ready_for_review"` (the PR just transitioned out of draft, so any prior review was a draft-mode review and the author is now asking for a full one — proceed). If the bot reviewed a previous commit (`LAST_REVIEW_SHA` exists but differs from `HEAD_SHA`), judge what was pushed since. Read two signals, both leak-free against base-merges: @@ -233,12 +250,22 @@ read -r CURRENT_HEAD PR_STATE < <(gh pr view --json commits,state \ # left on the current HEAD would read as "already reviewed" and discard this run's # review at the last step, after all the work. Shell state doesn't carry between # tool calls, so re-derive $SUBSTANTIVE here. +# +# The force-push re-anchoring from step 1 lands on this guard too, and harder: a +# review submitted against the rewritten-away commit now reports +# `.commit_id == $HEAD_SHA`, so it reads as "already reviewed" and discards the +# very re-review step 1's `FORCE_PUSHED` probe just unblocked. Drop reviews older +# than the newest rewrite; anything submitted after it really did anchor here. # NOTE: REST API uses .commit_id (not .commit.oid from gh pr view --json) SUBSTANTIVE=$(gh api --paginate "repos/$REPO/pulls//comments" \ --jq '.[] | select(.in_reply_to_id == null) | .pull_request_review_id' | jq -s 'unique') +LAST_FORCE_PUSH_AT=$(gh api --paginate "repos/$REPO/issues//timeline" \ + | jq -rs 'add | [.[] | select(.event == "head_ref_force_pushed") | .created_at] | max // ""') ALREADY_POSTED=$(gh api --paginate "repos/$REPO/pulls//reviews" \ | jq -rs --argjson sub "$SUBSTANTIVE" --arg bot "$BOT_LOGIN" --arg head "$HEAD_SHA" \ + --arg fp "$LAST_FORCE_PUSH_AT" \ 'add | [.[] | select(.user.login == $bot and .commit_id == $head) + | select($fp == "" or .submitted_at > $fp) | select((.body | length) > 0 or (.id | IN($sub[])) or .state == "APPROVED")] | last | .submitted_at // empty') [ -n "$ALREADY_POSTED" ] && echo "Already reviewed — finish at step 9" && exit 0 @@ -354,9 +381,19 @@ GitHub returns `422 Unprocessable Entity` with "Line could not be resolved" when # `gh api --paginate` applies `--jq` to each page separately, so a `| last` # inside `--jq` returns one id *per page* — pipe to `jq -rs 'add | …'` to reduce # over the merged array instead. +# +# The force-push filter from the pre-post guard is required here too, and the +# consequence of omitting it is worse than a skipped run: a body-bearing review +# from before a rewrite reports `.commit_id == $HEAD_SHA`, passes the body-length +# test, and the PUT below overwrites that older review's text with this run's +# findings — destroying a published review and leaving the new body over the old +# review's inline comments on code that no longer exists. +LAST_FORCE_PUSH_AT=$(gh api --paginate "repos/$REPO/issues//timeline" \ + | jq -rs 'add | [.[] | select(.event == "head_ref_force_pushed") | .created_at] | max // ""') ORPHAN_ID=$(gh api --paginate "repos/$REPO/pulls//reviews" \ - | jq -rs --arg bot "$BOT_LOGIN" --arg head "$HEAD_SHA" \ - 'add | [.[] | select(.user.login == $bot and .commit_id == $head and (.body | length) > 0)] + | jq -rs --arg bot "$BOT_LOGIN" --arg head "$HEAD_SHA" --arg fp "$LAST_FORCE_PUSH_AT" \ + 'add | [.[] | select(.user.login == $bot and .commit_id == $head and (.body | length) > 0) + | select($fp == "" or .submitted_at > $fp)] | last | .id // empty') ```