fix(skill): add retry guard to prevent intra-run duplicate reviews#1322
fix(skill): add retry guard to prevent intra-run duplicate reviews#1322
Conversation
|
can we work this into the broader guidance more elegantly? i.e. ensure we're describing what we should do correctly; I worry this is just a patch of what we shouldn't do |
Rework the retry guard from a separate "never do X" warning into a natural pre-posting step, describing what we should do rather than patching what we shouldn't. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reword the "Posting" section to describe the correct procedure (verify → post) rather than a defensive guard (check → skip). Addresses review feedback from max-sixty. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Good point — reframed the section to describe the correct posting procedure (verify → post) rather than a defensive guard (check → skip if already posted). The "Posting" section now reads as positive instructions for how to post exactly one review, with the API check as a natural first step rather than a bolted-on safeguard. |
worktrunk-bot
left a comment
worktrunk-bot
left a comment
There was a problem hiding this comment.
The verification query doesn't scope to the current commit — it finds the last review by the bot on any commit. If the bot already reviewed a previous commit (which the pre-flight allows through when HEAD differs from LAST_REVIEW_SHA), this check would find the old review's submitted_at and incorrectly skip posting the new one.
Adding commit_id filtering (the field is available on the API response) would fix this — see inline suggestion.
|
Good point — reworked it. Instead of a separate "never retry without checking" warning, the dedup check is now integrated as the first step of the posting flow: "Before posting, check whether this run already submitted a review." The section now reads as a natural sequence of what to do rather than a bolt-on of what not to do. |
Add commit_id filtering to the review verification query so it only matches reviews posted against the current HEAD, not previous commits. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When a suggestion's target range includes a line that is itself triple backticks (a code fence marker), GitHub's suggestion syntax collides with the content. This caused a broken suggestion on PR #1322 where applying it would have eaten the closing code fence. Add explicit guidance to exclude fence marker lines from the range or reproduce them in the suggestion body. Closes #1326 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…#1608) ## Summary - Replaces the vague "minimize the range" instruction for multi-line suggestions with a concrete 4-step verification checklist - Addresses the root cause of #1607: the bot constructs suggestion ranges that are wider than the replacement text, silently deleting correct code when applied ## Root cause analysis Investigated all three known cases: 1. **#1310** (PR #1309): `start_line: 18, line: 22` — 5-line range replaced with fewer lines, deleting the `git-wt` fallback branch 2. **#1326** (PR #1322): `start_line: 289, line: 291` — range included a closing ` ``` ` fence that GitHub consumed as the suggestion's own delimiter 3. **#1598**: `start_line: 93, line: 127` — 35-line range replaced with a shorter rewrite, deleting the `gh issue view` code block Common thread: the bot sets a line range covering more content than it intends to replace. The replacement text is shorter, so the surplus lines are silently deleted. The existing instruction ("minimize the range") didn't give the bot a way to verify correctness. ## Fix The new checklist requires the bot to: 1. **Read the exact lines** in the range before posting 2. **Verify no silent deletions** — every line must appear in the replacement or be intentionally removed 3. **Cap range at ~10 lines** — larger suggestions should be split or pushed as commits 4. **Avoid spanning markdown fences** — prevents delimiter collision ## Test plan - [ ] Review a PR with a multi-line suggestion opportunity and verify the bot follows the checklist - [ ] Verify the bot splits or commits changes larger than ~10 lines instead of using a suggestion 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Summary
review-prskill to verify whether a review was already posted before retrying a failed/stuck API callFixes #1321
Context
Run 22784882686 posted the same review body twice on PR #1320 (4 minutes apart) because the bot thought the first API call was "stuck" and retried with
gh pr review --commentwithout checking if the review had already been posted.Test plan
🤖 Generated with Claude Code