Skip to content

fix(skill): block @greptileai trigger until all comments replied#656

Open
carlos-alm wants to merge 2 commits intomainfrom
fix/review-skill-greptile-guard
Open

fix(skill): block @greptileai trigger until all comments replied#656
carlos-alm wants to merge 2 commits intomainfrom
fix/review-skill-greptile-guard

Conversation

@carlos-alm
Copy link
Contributor

Summary

  • Adds a mandatory pre-trigger verification script to /review Step 2g that checks every Greptile inline comment has at least one reply before allowing @greptileai to be posted
  • Updates the Rules section: replaces "always re-trigger after replying" with "never trigger without replying to every comment first" — making the reply-before-trigger order an explicit blocking invariant
  • Prevents the review skill from starting new review cycles while old feedback is still unanswered

Test plan

  • Run /review on a PR with unanswered Greptile comments — verify the verification script blocks the trigger
  • Run /review on a PR where all Greptile comments have replies — verify it proceeds to trigger normally

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
Copy link

claude bot commented Mar 27, 2026

Claude finished @carlos-alm's task —— View job


Reviewing PR: fix(skill): block @greptileai trigger until all comments replied

Todo List:

  • Read and analyze the modified file
  • Phase 1: Root cause analysis
  • Phase 2: Configuration change detection
  • Phase 3: Test integrity check
  • Phase 4: Code quality review
  • Phase 5: Backlog compliance analysis
  • Provide final assessment and rating

Root Cause Analysis

Problem 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

  • Zero-dep: ✓ — No new dependencies, pure bash script in skill documentation
  • Foundation-aligned: ✓ — Workflow improvement, no violation of core principles
  • Problem-fit: 2 — Workflow optimization tangential to core "lost AI" problem
  • Breaking: No — Purely additive workflow guard, no API/CLI changes
  • Tier: 1

Critical Concerns

None. The verification script is well-structured with proper error handling and clear messaging.

Code Quality Assessment

  • Script logic: Sound - correctly identifies unanswered comments and blocks trigger
  • Error handling: Proper exit codes and informative error messages
  • Shell practices: Good use of arrays, proper variable handling, clear control flow
  • Integration: Fits seamlessly into existing Step 2g workflow

Final Recommendation

  • Rating: ⭐⭐⭐⭐☆ (4/5)
  • Action: APPROVE
  • Reasoning: Solid workflow improvement that addresses a real process issue. Well-implemented guard script with proper error handling. Only loses one star due to low problem-fit score (workflow optimization rather than core AI navigation problem).

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-apps
Copy link
Contributor

greptile-apps bot commented Mar 27, 2026

Greptile Summary

This PR adds a mandatory pre-trigger gate to the /review skill's Step 2g: a bash script that queries all PR review comments, filters for root-level Greptile comments (in_reply_to_id == null), and blocks @greptileai from being posted unless every one of those comments has at least one non-Greptile reply. The Rules section is updated in parallel to make this a hard invariant.

Previous round feedback (N+1 API calls, root-comment selection) has been fully addressed. Two issues remain:

  • reply_count is broken on multi-page responses (P1): gh api --paginate emits one JSON array per page; piping multiple arrays through the existing jq expression produces one number per page. The resulting multi-line string in reply_count causes [ "$reply_count" -eq 0 ] to error as a non-integer, which can silently skip the block and let the trigger proceed even when comments are unanswered. Reproducible on any PR with >30 review comments total. Fix requires jq -s to slurp pages into a single array.
  • Stale "only exception" language (P2): Line 204 still claims the positive-emoji reaction is the only exception to re-triggering, which now conflicts with the new blocking pre-condition immediately below it. An AI agent may interpret "only exception" literally and skip the all-replies check.

Confidence Score: 4/5

Safe 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

Filename Overview
.claude/skills/review/SKILL.md Adds a pre-trigger verification script that checks all Greptile inline comments have replies before @greptileai is posted; reply_count is broken for PRs with >30 review comments due to multi-page jq output, and line 204's "only exception" language now conflicts with the new mandatory check.

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"]
Loading

Reviews (2): Last reviewed commit: "fix(skill): filter root Greptile comment..." | Re-trigger Greptile

Comment on lines +210 to +211
greptile_comment_ids=$(gh api repos/optave/codegraph/pulls/<number>/comments --paginate \
--jq '[.[] | select(.user.login == "greptile-apps[bot]")] | .[].id')
Copy link
Contributor

Choose a reason for hiding this comment

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

P2 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):

Suggested change
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')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment on lines +214 to +219
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
Copy link
Contributor

Choose a reason for hiding this comment

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

P2 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
done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed — refactored to fetch all comments once into all_comments and filter in-memory with jq. Eliminates the N+1 API call pattern.

@carlos-alm
Copy link
Contributor Author

@greptileai

Comment on lines +217 to +218
reply_count=$(echo "$all_comments" \
| jq "[.[] | select(.in_reply_to_id == $cid and .user.login != \"greptile-apps[bot]\")] | length")
Copy link
Contributor

Choose a reason for hiding this comment

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

P1 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:

Suggested change
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant