diff --git a/review-pr/action.yml b/review-pr/action.yml index 8d365af..c52b7dd 100644 --- a/review-pr/action.yml +++ b/review-pr/action.yml @@ -975,27 +975,30 @@ runs: fi else STATUS="✅ **Review completed**" - # Defense-in-depth: if the log exists but no review was posted, post a fallback LGTM comment. - # This guards against the agent exiting 0 without calling gh api (e.g., zero-findings early exit). + # Defense-in-depth: if the log exists but no review was posted, post a neutral + # incomplete-run notice. Exit 0 without a posted review can be a degenerate agent + # run (e.g. a drafter loop), so the fallback must not read as a clean review and + # must not match the incremental-review completion markers. if [ -n "$VERBOSE_LOG_FILE" ] && [ -f "$VERBOSE_LOG_FILE" ] && ! grep -qE 'pullrequestreview-[0-9]+' "$VERBOSE_LOG_FILE" 2>/dev/null; then - # Dedup guard: skip posting if an identical fallback PR review already exists. - # Also check the old issue-comment endpoint for a one-time migration guard - # (prior runs posted to /issues/comments; this prevents a duplicate on the first - # run of this new code against PRs that already received an LGTM issue comment). - EXISTING=$(gh api "repos/$REPOSITORY/pulls/$PR_NUMBER/reviews" \ + # Dedup guard: skip posting if a fallback PR review already exists. Matches the + # current notice plus the legacy LGTM fallback so PRs that already received + # either kind never get a second one. Paginated to cover PRs with >30 reviews. + # The old issue-comment endpoint is a one-time migration guard (prior runs + # posted the LGTM fallback to /issues/comments). + EXISTING=$(gh api --paginate "repos/$REPOSITORY/pulls/$PR_NUMBER/reviews" \ + --jq '[.[] | select((.body // "") | (startswith("âš ī¸ **Review did not complete**") or startswith("đŸŸĸ **No issues found**")))] | length' \ + 2>/dev/null | jq -s 'add // 0' || true) + EXISTING_OLD=$(gh api --paginate "repos/$REPOSITORY/issues/$PR_NUMBER/comments" \ --jq '[.[] | select((.body // "") | startswith("đŸŸĸ **No issues found**"))] | length' \ - 2>/dev/null || echo "0") - EXISTING_OLD=$(gh api "repos/$REPOSITORY/issues/$PR_NUMBER/comments" \ - --jq '[.[] | select((.body // "") | startswith("đŸŸĸ **No issues found**"))] | length' \ - 2>/dev/null || echo "0") + 2>/dev/null | jq -s 'add // 0' || true) if [ "${EXISTING:-0}" -gt 0 ] || [ "${EXISTING_OLD:-0}" -gt 0 ]; then - echo "â„šī¸ Fallback LGTM review already exists — skipping duplicate post" + echo "â„šī¸ Fallback review already exists — skipping duplicate post" else - echo "::warning::Agent exited 0 but no review was posted — posting fallback LGTM review" - jq -n --arg body "đŸŸĸ **No issues found** — LGTM! [View logs]($RUN_URL)." --arg event "COMMENT" \ + echo "::warning::Agent exited 0 but no review was posted — posting incomplete-review notice" + jq -n --arg body "âš ī¸ **Review did not complete** — the agent finished without posting a review. Re-request a review from \`docker-agent\` to retry. [View logs]($RUN_URL)." --arg event "COMMENT" \ '{body: $body, event: $event, comments: []}' \ | gh api "repos/$REPOSITORY/pulls/$PR_NUMBER/reviews" --input - 2>&1 || \ - echo "::warning::Failed to post fallback LGTM review to PR" + echo "::warning::Failed to post incomplete-review notice to PR" fi fi fi diff --git a/src/incremental-review/__tests__/incremental-review.test.ts b/src/incremental-review/__tests__/incremental-review.test.ts index ebc00c7..a594e1c 100644 --- a/src/incremental-review/__tests__/incremental-review.test.ts +++ b/src/incremental-review/__tests__/incremental-review.test.ts @@ -53,8 +53,13 @@ describe('findLastReviewedSha', () => { expect(findLastReviewedSha(reviews)).toBe(SHA_B); }); - it('accepts the LGTM fallback body as a completed review', () => { - expect(findLastReviewedSha([review({ body: 'đŸŸĸ **No issues found** — LGTM!' })])).toBe(SHA_A); + it('ignores the incomplete-run fallback review (agent exited 0 without posting)', () => { + const body = 'âš ī¸ **Review did not complete** — the agent finished without posting a review.'; + expect(findLastReviewedSha([review({ body })])).toBeNull(); + }); + + it('ignores the legacy LGTM fallback review (never a completed run)', () => { + expect(findLastReviewedSha([review({ body: 'đŸŸĸ **No issues found** — LGTM!' })])).toBeNull(); }); it('accepts the GitHub App bot login variant', () => { diff --git a/src/incremental-review/incremental-review.ts b/src/incremental-review/incremental-review.ts index 63e4c34..ad5c562 100644 --- a/src/incremental-review/incremental-review.ts +++ b/src/incremental-review/incremental-review.ts @@ -13,9 +13,9 @@ * SHA at posting time. This survives across workflow runs, requires no extra * writes, and cannot be edited away like a marker embedded in a comment body. * Only reviews that represent a *completed* run count — an assessment body - * ("### Assessment:") or the zero-findings LGTM fallback. Timeout and failure - * fallback reviews do NOT mark commits as reviewed, so the next run re-covers - * them. + * ("### Assessment:", posted even for zero findings). Timeout, failure, and + * incomplete-run fallback reviews (including the retired zero-findings LGTM + * fallback) do NOT mark commits as reviewed, so the next run re-covers them. * * ## Fallbacks to a full review (see planIncrementalReview) * @@ -64,10 +64,12 @@ export interface IncrementalPlan { // looser (e.g. a body that starts with "-") must never get through. const SHA40 = /^[0-9a-f]{40}$/i; -// Bodies that mark a review run as completed. The timeout ("âąī¸ **PR Review -// Timed Out**") and failure ("❌ **PR Review Failed**") fallbacks match -// neither, so unreviewed commits stay unreviewed. -const COMPLETED_BODY_MARKERS = ['### Assessment:', 'đŸŸĸ **No issues found**']; +// Bodies that mark a review run as completed. Only the agent's own assessment +// body counts: the timeout ("âąī¸ **PR Review Timed Out**"), failure ("❌ **PR +// Review Failed**"), and incomplete-run ("âš ī¸ **Review did not complete**") +// fallbacks match nothing here, and neither does the retired "đŸŸĸ **No issues +// found**" LGTM fallback, so commits covered only by a fallback stay unreviewed. +const COMPLETED_BODY_MARKERS = ['### Assessment:']; // GitHub presents the bot identity as "docker-agent" when posting with a // machine user token, or "docker-agent[bot]" through a GitHub App installation diff --git a/src/rate-limit/index.ts b/src/rate-limit/index.ts index 9b04153..78e3fe8 100644 --- a/src/rate-limit/index.ts +++ b/src/rate-limit/index.ts @@ -16,7 +16,7 @@ * Counting is per LLM run, so each run contributes exactly one unit: * - Reviews are posted via the Reviews API (POST /pulls/{n}/reviews) with no * inline marker — a findings review, a zero-finding APPROVE, and the - * timeout/error/LGTM fallbacks all land there. They are counted from + * timeout/error/incomplete-run fallbacks all land there. They are counted from * `pulls.listReviews` by bot author (a real review run always carries an * assessment/status body); the inline finding comments such a review carries * are deliberately not counted, since that would be N units per single run. @@ -102,7 +102,7 @@ function isAgentReplyComment(c: CommentLike, botLogin: string, windowStartMs: nu function isAgentReview(r: ReviewLike, botLogin: string, windowStartMs: number): boolean { if (!matchesBotLogin(r.user?.login, botLogin)) return false; // A real review run always carries an assessment/status body ("### Assessment: - // â€Ļ", or a timeout/error/LGTM fallback). Standalone inline comments and replies + // â€Ļ", or a timeout/error/incomplete-run fallback). Standalone inline comments and replies // surface in this endpoint as empty-body review entries; skipping them keeps // each review run counted exactly once and avoids double-counting an inline // reply (already counted via its reply marker on the comment endpoints).