Skip to content

fix(skills): approve after trivial follow-up to COMMENTED review - #1659

Closed
worktrunk-bot wants to merge 2 commits into
mainfrom
hourly/review-23402473159
Closed

worktrunk-bot wants to merge 2 commits into
mainfrom
hourly/review-23402473159

Conversation

@worktrunk-bot

Copy link
Copy Markdown
Collaborator

Summary

  • Extract LAST_REVIEW_STATE alongside LAST_REVIEW_SHA in the review-pr skill preflight
  • When trivial incremental changes follow a COMMENTED (non-APPROVED) review, submit an approval instead of assuming one already exists
  • Fixes a gap where PRs were left without bot approval after the author addressed review feedback

Root cause

The review-pr skill's incremental review path (lines 53-58) said "the existing
review stands" and explicitly instructed "do not submit a new approval." This
assumed the prior review was always APPROVED. When the prior review was
COMMENTED (i.e., the bot had requested changes), the bot correctly identified
the trivial follow-up but skipped submitting an approval, leaving the PR in
limbo.

Evidence

2 confirmed occurrences of "Bot falsely claims prior approval exists":

  1. Run 23401754507 (2026-03-22T11:08Z) — PR feat: add directional template vars to all two-worktree hooks #1655 (template-vars). Prior
    bot review was state COMMENTED. Bot concluded "existing approval stands"
    but no approval existed. PR left without bot approval.

  2. 1 historical occurrence tracked in review-reviewers tracking issue review-reviewers tracking: 2026-03 #1611
    (original detail entry lost to tracking comment overwrite, but the finding
    was carried forward in cumulative tallies across 15+ hourly runs).

Gate assessment:

  • Evidence level: High (consistent pattern across 2 sessions)
  • Change type: Targeted fix (adds a state check and conditional approval)
  • Both gates pass (2 occurrences meets the 2-3 threshold for High confidence)

Test plan

  • Verify the jq query correctly extracts {sha, state} from gh pr view --json reviews
  • On a PR where the bot's last review was COMMENTED, confirm the bot now submits an approval after trivial follow-up
  • On a PR where the bot's last review was APPROVED, confirm the bot still skips re-approval

🤖 Generated with Claude Code

The review-pr skill assumed the prior bot review was always APPROVED when
skipping trivial incremental changes. When the prior review was COMMENTED
(requesting changes), the bot said "existing approval stands" but no
approval existed, leaving the PR without bot approval.

Extract LAST_REVIEW_STATE alongside LAST_REVIEW_SHA in the preflight, and
after resolving threads on trivial follow-ups, submit an approval when the
prior state was non-APPROVED.

Evidence: 2 occurrences (PR #1655 run 23401754507, plus 1 historical).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@worktrunk-bot worktrunk-bot added the claude-behavior Issues with Claude CI bot behavior label Mar 22, 2026
…rror

The `(.body | length > 0 or .state == "APPROVED")` expression has a jq
pipe precedence bug: `|` has the lowest precedence in jq, so `.state`
resolves against the body string (pipe output), not the review object.
This causes `Cannot index string with string "state"` when the bot's
prior review has an empty body (every `--approve -b ""` approval).

Fix: `((.body | length) > 0 or .state == "APPROVED")` — parenthesizing
the pipe scopes it correctly.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@worktrunk-bot

Copy link
Copy Markdown
Collaborator Author

Pushed a fix for a jq pipe precedence bug in the LAST_REVIEW filter (4b2bd9c).

Bug: (.body | length > 0 or .state == "APPROVED") — jq's | has the lowest precedence, so after piping .body, the .state in the or branch resolves against the body string, not the review object. This causes Cannot index string with string "state" for every empty-body approval (--approve -b "").

Fix: ((.body | length) > 0 or .state == "APPROVED") — extra parens scope the pipe.

Reproduction
# Bug: errors on empty-body APPROVED review
echo '[{"author":{"login":"bot"},"body":"","state":"APPROVED","commit":{"oid":"abc"}}]' | \
jq '[.[] | select(.author.login == "bot" and (.body | length > 0 or .state == "APPROVED"))]'
# jq: error: Cannot index string with string "state"

# Fix: works correctly
echo '[{"author":{"login":"bot"},"body":"","state":"APPROVED","commit":{"oid":"abc"}}]' | \
jq '[.[] | select(.author.login == "bot" and ((.body | length) > 0 or .state == "APPROVED"))]'
# [{"author":{"login":"bot"},"body":"","state":"APPROVED","commit":{"oid":"abc"}}]

Found via review-reviewers analysis of run 23402604135, which identified the precedence issue during self-review of this PR. Related tracking entry: "jq pre-flight failure in LAST_REVIEW_SHA" (1 prior occurrence).

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The jq precedence fix is correct — | binds looser than or, so the original (.body | length > 0 or .state == "APPROVED") would error on empty-body approvals. The extra parens fix it.

The LAST_REVIEW_STATE logic correctly identifies the gap where trivial follow-ups to COMMENTED reviews left PRs without approval.

One concern: the new approve path (lines 63-68) doesn't account for self-authored PRs. Step 5 says "Do NOT attempt gh pr review --approve — GitHub rejects self-approvals," but the new trivial-approve path unconditionally says to submit an approval when LAST_REVIEW_STATE != APPROVED. A bot executing this on its own PR would hit a GitHub API error. Consider adding a PR_AUTHOR != BOT_LOGIN guard or a note to skip for self-authored PRs.

```bash
gh pr review <number> --approve -b ""
```
Then proceed to step 6 (monitor CI).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This path doesn't check for self-authored PRs. Step 5 says "Do NOT attempt gh pr review --approve — GitHub rejects self-approvals." Consider:

Suggested change
Then proceed to step 6 (monitor CI).
gh pr review <number> --approve -b ""

Skip this step for self-authored PRs (PR_AUTHOR == BOT_LOGIN) —
GitHub rejects self-approvals.

@max-sixty

Copy link
Copy Markdown
Owner

Superseded by tend extraction (#1676). Target file (.claude/skills/review-pr/SKILL.md) no longer exists. The improvement (approve after trivial follow-up to COMMENTED review) should be ported to the tend-review skill in tend.

@max-sixty max-sixty closed this Mar 24, 2026
@worktrunk-bot

Copy link
Copy Markdown
Collaborator Author

Understood — closing since the target file was removed in #1676. The fix (approve after trivial follow-up to COMMENTED review) and the self-approval guard from my review comment should both be ported to tend-review in tend.

@max-sixty
max-sixty deleted the hourly/review-23402473159 branch June 18, 2026 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

claude-behavior Issues with Claude CI bot behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants