fix(skill): block @greptileai trigger until all comments replied#656
fix(skill): block @greptileai trigger until all comments replied#656carlos-alm wants to merge 2 commits intomainfrom
Conversation
Prevents the review skill from posting @greptileai re-triggers before replying to every inline Greptile comment. Adds a Step 2g verification script that checks for unanswered comments and blocks the trigger if any exist. Updates the Rules section to make reply-before-trigger the explicit invariant.
|
Claude finished @carlos-alm's task —— View job Reviewing PR: fix(skill): block @greptileai trigger until all comments repliedTodo List:
Root Cause AnalysisProblem Being Solved: Prevents review skill from starting new review cycles while old feedback is still unanswered. Addresses workflow issue where @greptileai was triggered prematurely without replying to all existing comments. Why This Approach: Adds mandatory verification script that exits with error code 1 if any Greptile comments lack replies. This enforces the reply-first invariant at the process level rather than relying on human discipline. Risk Assessment: Low risk - documentation/workflow change only, no code modification. Improves review process integrity. Backlog Compliance
Critical ConcernsNone. The verification script is well-structured with proper error handling and clear messaging. Code Quality Assessment
Final Recommendation
The verification script at lines 208-230 effectively prevents premature @greptileai triggers and the Rules update at line 305 clearly communicates the blocking invariant. This will reduce review noise and ensure feedback is properly addressed before new review cycles begin. |
Greptile SummaryThis PR adds a mandatory pre-trigger gate to the Previous round feedback (N+1 API calls, root-comment selection) has been fully addressed. Two issues remain:
Confidence Score: 4/5Safe to merge after fixing the multi-page jq reply_count bug, which silently bypasses the blocking check on PRs with >30 review comments. One P1 defect (broken reply_count on paginated responses) means the primary safety invariant — blocking @greptileai when comments are unanswered — can silently fail on busy PRs, defeating the whole point of this PR. The fix is a one-liner (jq -s + .[][]). Once addressed, the PR is straightforward and the prior round's concerns are fully resolved. .claude/skills/review/SKILL.md — specifically the reply_count jq expression at lines 217–218. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Step 2g: Re-trigger Greptile] --> B["Fetch all PR review comments via gh api --paginate"]
B --> C["Filter root Greptile comments where in_reply_to_id == null"]
C --> D{Any Greptile comments?}
D -- No --> G["All comments answered — safe to re-trigger"]
D -- Yes --> E["For each comment ID: count non-Greptile replies in all_comments"]
E --> F{unanswered count > 0?}
F -- Yes --> H["BLOCKED: echo unanswered IDs, exit 1"]
F -- No --> G
G --> I["Check positive emoji on last reply"]
I -- Positive reaction exists --> J[Skip re-trigger]
I -- No reaction --> K["Post @greptileai"]
Reviews (2): Last reviewed commit: "fix(skill): filter root Greptile comment..." | Re-trigger Greptile |
.claude/skills/review/SKILL.md
Outdated
| greptile_comment_ids=$(gh api repos/optave/codegraph/pulls/<number>/comments --paginate \ | ||
| --jq '[.[] | select(.user.login == "greptile-apps[bot]")] | .[].id') |
There was a problem hiding this comment.
Script selects all Greptile comments, including follow-up thread replies
greptile_comment_ids is populated by selecting every comment where user.login == "greptile-apps[bot]". This includes any follow-up replies Greptile might post inside an existing thread (those will have in_reply_to_id pointing to the thread root).
When the loop later checks for human replies via select(.in_reply_to_id == $cid ...), it uses each Greptile comment's own ID as $cid. But GitHub's threading model stores in_reply_to_id as the root comment of the thread — not the ID of an intermediate reply. As a result, any Greptile follow-up reply would never appear as an in_reply_to_id for other comments, and the script would permanently flag it as unanswered, blocking the trigger even when the thread has been fully addressed.
Fix by restricting the selection to root Greptile comments only (those that are not themselves replies):
| greptile_comment_ids=$(gh api repos/optave/codegraph/pulls/<number>/comments --paginate \ | |
| --jq '[.[] | select(.user.login == "greptile-apps[bot]")] | .[].id') | |
| greptile_comment_ids=$(gh api repos/optave/codegraph/pulls/<number>/comments --paginate \ | |
| --jq '[.[] | select(.user.login == "greptile-apps[bot]" and .in_reply_to_id == null)] | .[].id') |
There was a problem hiding this comment.
Fixed — added and .in_reply_to_id == null to the jq filter so only root Greptile comments are checked. Follow-up thread replies are now excluded, preventing false-positive blocks.
| for cid in $greptile_comment_ids; do | ||
| reply_count=$(gh api repos/optave/codegraph/pulls/<number>/comments --paginate \ | ||
| --jq "[.[] | select(.in_reply_to_id == $cid and .user.login != \"greptile-apps[bot]\")] | length") | ||
| if [ "$reply_count" -eq 0 ]; then | ||
| unanswered+=("$cid") | ||
| fi |
There was a problem hiding this comment.
N+1 API calls — fetch comments once and process in-memory
Each loop iteration issues a full paginated gh api call for all PR comments just to count replies for a single $cid. For a PR with 10 Greptile comments this means 11 round-trips each retrieving the full comment list. Consider fetching all comments once and passing the payload through jq entirely:
all_comments=$(gh api repos/optave/codegraph/pulls/<number>/comments --paginate)
greptile_comment_ids=$(echo "$all_comments" \
| jq -r '[.[] | select(.user.login == "greptile-apps[bot]" and .in_reply_to_id == null)] | .[].id')
unanswered=()
for cid in $greptile_comment_ids; do
reply_count=$(echo "$all_comments" \
| jq "[.[] | select(.in_reply_to_id == $cid and .user.login != \"greptile-apps[bot]\")] | length")
if [ "$reply_count" -eq 0 ]; then
unanswered+=("$cid")
fi
doneThere was a problem hiding this comment.
Fixed — refactored to fetch all comments once into all_comments and filter in-memory with jq. Eliminates the N+1 API call pattern.
| reply_count=$(echo "$all_comments" \ | ||
| | jq "[.[] | select(.in_reply_to_id == $cid and .user.login != \"greptile-apps[bot]\")] | length") |
There was a problem hiding this comment.
reply_count breaks on multi-page responses
gh api --paginate outputs each page as a separate JSON array, not a single merged array. When all_comments contains multiple pages (i.e., more than 30 PR review comments), the jq expression produces one number per page — e.g., 0\n2 — stored as a multi-line string in reply_count. The subsequent [ "$reply_count" -eq 0 ] bash comparison then receives a non-integer string, causing an "integer expression expected" error (exit code 2), which makes the if branch silently skip, treating every Greptile comment as answered. The check passes falsely and the block never fires.
Fix with jq -s to slurp all pages into one array before counting:
| reply_count=$(echo "$all_comments" \ | |
| | jq "[.[] | select(.in_reply_to_id == $cid and .user.login != \"greptile-apps[bot]\")] | length") | |
| reply_count=$(echo "$all_comments" \ | |
| | jq -s "[.[][] | select(.in_reply_to_id == $cid and .user.login != \"greptile-apps[bot]\")] | length") |
The greptile_comment_ids selection above is fine as-is — jq emits one ID per line there and the for loop splits on newlines correctly. Only reply_count is affected.
Summary
/reviewStep 2g that checks every Greptile inline comment has at least one reply before allowing@greptileaito be postedTest plan
/reviewon a PR with unanswered Greptile comments — verify the verification script blocks the trigger/reviewon a PR where all Greptile comments have replies — verify it proceeds to trigger normally