diff --git a/.github/actions/comment-triggered-check/README.md b/.github/actions/comment-triggered-check/README.md index 06a13818..68391033 100644 --- a/.github/actions/comment-triggered-check/README.md +++ b/.github/actions/comment-triggered-check/README.md @@ -281,8 +281,10 @@ first version is the one notification emails carry. The reply is a view of the check-runs. Its hidden state holds only the command and the commit; which rows exist, their labels, status and run links all come from the checks on the commit stamped with this reply, newest attempt only. -Every writer lists those checks, redraws the whole reply, reads it back, and -writes again if it differs. A job that overwrites the reply with an older view, +Every writer lists those checks on the stored commit, its own commit and the +pull request's current head, redraws the whole reply, reads it back, and writes +again if it differs. Listing the head lets a writer from an older attempt find a +re-run on a newer commit. A job that overwrites the reply with an older view, such as a finish from a cancelled attempt, is corrected by the next redraw of any job, because nothing it wrote back is a source of truth. GitHub has no conditional comment edit, so this is what keeps parallel jobs from losing each diff --git a/.github/actions/comment-triggered-check/src/reply.sh b/.github/actions/comment-triggered-check/src/reply.sh index 1145d086..93c538d1 100755 --- a/.github/actions/comment-triggered-check/src/reply.sh +++ b/.github/actions/comment-triggered-check/src/reply.sh @@ -42,14 +42,25 @@ reply_checks_on() { printf '%s' "$listing" | jq -c -s '[.[] | .check_runs? // [] | .[]]' 2>/dev/null } +# reply_head_sha — the current head commit of the pull +# request the reply is on; empty when it cannot be read. +reply_head_sha() { + local number + number="$(jq -r '(.issue_url // "") | capture("/issues/(?[0-9]+)$").n // empty' <<< "$2" 2>/dev/null)" || return 0 + [[ -n "$number" ]] || return 0 + gh api "repos/${1}/pulls/${number}" 2>/dev/null | jq -r '.head.sha // empty' 2>/dev/null || true +} + # reply_refresh [sha] — redraws the reply from the checks on -# its commit and on sha, writing only when the body changed, until a read finds -# nothing to change. sha is the caller's own commit, which differs from the -# stored one after a re-run on a pull request that moved. +# its stored commit, on sha, and on the pull request's current head, writing +# only when the body changed, until a read finds nothing to change. sha is the +# caller's own commit. Every newer attempt runs on the head, so listing it lets +# even a writer from an older attempt draw the newest one. reply_refresh() { - local repo="$1" reply_id="$2" own_sha="${3-}" attempt current state body all checks sha + local repo="$1" reply_id="$2" own_sha="${3-}" attempt comment current state body all checks sha head_sha for ((attempt = 1; attempt <= REPLY_ATTEMPTS; attempt++)); do - if ! current="$(gh api "repos/${repo}/issues/comments/${reply_id}" 2>/dev/null | jq -r '.body // empty')" || + comment="$(gh api "repos/${repo}/issues/comments/${reply_id}" 2>/dev/null)" || comment="" + if ! current="$(jq -r '.body // empty' <<< "${comment:-null}" 2>/dev/null)" || [[ -z "$current" ]]; then echo "::warning::could not read reply comment ${reply_id}" return 0 @@ -58,6 +69,7 @@ reply_refresh() { echo "::warning::reply comment ${reply_id} has no readable state; leaving it as is" return 0 fi + head_sha="$(reply_head_sha "$repo" "$comment")" all='[]' while IFS= read -r sha; do [[ -n "$sha" ]] || continue @@ -66,7 +78,7 @@ reply_refresh() { return 0 fi all="$(jq -c --argjson more "$checks" '. + $more' <<< "$all")" - done < <(printf '%s\n%s\n' "$(jq -r '.sha // ""' <<< "$state")" "$own_sha" | awk 'NF && !seen[$0]++') + done < <(printf '%s\n%s\n%s\n' "$(jq -r '.sha // ""' <<< "$state")" "$own_sha" "$head_sha" | awk 'NF && !seen[$0]++') checks="$(reply_rows "$reply_id" "$all")" # The rows decide the commit, so a stale writer cannot pin an old one. state="$(jq -c --argjson rows "$checks" '.sha = ($rows[0].head_sha // .sha)' <<< "$state")" diff --git a/.github/actions/comment-triggered-check/test/gh_mock.bash b/.github/actions/comment-triggered-check/test/gh_mock.bash index 2093e01b..5c3838c3 100644 --- a/.github/actions/comment-triggered-check/test/gh_mock.bash +++ b/.github/actions/comment-triggered-check/test/gh_mock.bash @@ -33,6 +33,8 @@ # A PATCH to a seeded check-run stores its external_id in that check's file. # With GH_MOCK_CREATE_SEQ, a created check-run is seeded too. Seeded checks # answer the commit listing, filtered by head_sha. +# GH_MOCK_AFTER_FIRST_READ names a script run once, right after the first +# comment read is served. # A posted or edited comment body is kept in $MOCK_DIR/comment-, and GET # .../issues/comments/ serves it. $MOCK_DIR/checkrun-.json, when present, # answers GET .../check-runs/ ahead of GH_MOCK_CHECKRUN_JSON. @@ -98,7 +100,14 @@ case "$all" in *"/issues/comments/"*) id="${all##*/issues/comments/}"; id="${id%% *}" [ -f "$MOCK_DIR/comment-${id}" ] || { echo "mock: no comment ${id}" >&2; exit 1; } - jq -n --rawfile b "$MOCK_DIR/comment-${id}" --argjson id "$id" '{id: $id, body: $b}' + jq -n --rawfile b "$MOCK_DIR/comment-${id}" --argjson id "$id" \ + '{id: $id, body: $b, issue_url: "https://api.github.com/repos/loft-sh/demo/issues/7"}' + # Runs once, after the first read was served: another writer acting while + # this caller holds a stale copy. + if [ -n "${GH_MOCK_AFTER_FIRST_READ:-}" ] && [ ! -f "$MOCK_DIR/after-first-read" ]; then + touch "$MOCK_DIR/after-first-read" + bash "$GH_MOCK_AFTER_FIRST_READ" >/dev/null 2>&1 + fi ;; *"/issues/"*"/comments"*) [ -n "${GH_MOCK_COMMENTS_FAIL:-}" ] && { echo "mock: comments listing failed" >&2; exit 1; } diff --git a/.github/actions/comment-triggered-check/test/reply.bats b/.github/actions/comment-triggered-check/test/reply.bats index 9f9e8077..bbf44620 100644 --- a/.github/actions/comment-triggered-check/test/reply.bats +++ b/.github/actions/comment-triggered-check/test/reply.bats @@ -198,6 +198,41 @@ refresh() { [ "$(reply_state_decode "$(body)" | jq -r .sha)" = "def5678aaaa" ] } +@test "an old writer paused across a re-run on a newer commit still draws the newer attempt" { + # The old writer is attempt 1's finish: its check just completed as + # cancelled, so its view differs from the reply it read and it will write. + # Between its read and its write, a re-run on a newer commit opens its check + # and writes the reply. The old writer only knows the old commit. + seed_check 11 Pro completed cancelled https://e/old 1 + render_reply "$(state)" "[$(row 11 Pro in_progress "" https://e/old)]" https://github.com loft-sh/demo > "$MOCK_DIR/comment-555" + export GH_MOCK_PR_JSON='{"head":{"sha":"def5678aaaa","ref":"feature/x","repo":{"full_name":"loft-sh/demo"}},"base":{"ref":"main"},"state":"open"}' + cat > "$MOCK_DIR/rerun.sh" < "$MOCK_DIR/checkrun-20.json" +printf '%s' "\$(source "$LIB" && render_reply '{"v":2,"comment_id":777,"command":"/test-e2e snapshots","sha":"def5678aaaa"}' \ + '[{"id":20,"label":"Pro","status":"in_progress","conclusion":null,"head_sha":"def5678aaaa","url":"https://e/new"}]' \ + https://github.com loft-sh/demo)" > "$MOCK_DIR/comment-555" +EOF + export GH_MOCK_AFTER_FIRST_READ="$MOCK_DIR/rerun.sh" + run refresh abc1234def56 + [ "$status" -eq 0 ] + [ -f "$MOCK_DIR/after-first-read" ] + [[ "$(body)" == *"started 1 run on \`def5678\`"* ]] + [[ "$(body)" == *"- Pro: running, [watch the run](https://e/new)"* ]] + [[ "$(body)" != *"e/old"* ]] + [ "$(reply_state_decode "$(body)" | jq -r .sha)" = "def5678aaaa" ] +} + +@test "a redraw still works when the pull request cannot be read" { + seed_reply + seed_check 11 Pro completed success https://e/1 + export GH_MOCK_PR_FAIL=1 + run refresh + [ "$status" -eq 0 ] + [[ "$(body)" == *"- Pro: passed, [view the run](https://e/1)"* ]] +} + @test "a redraw leaves the reply alone when the checks cannot be listed" { seed_reply before="$(body)"