Skip to content

Commit 20fcc3d

Browse files
committed
Address review feedback: stale context, injection warning, failure visibility, detection robustness
- Use webhook payload for triggering comment instead of relying on API eventual consistency; deduplicate against API results by comment ID - Add explicit warning in agent instructions that jq --arg pattern is mandatory (never use shell string interpolation for JSON body) - Add agent-outcome output and confused reaction on failure so developers know their reply wasn't processed - Add user.type == Bot check on root comment author as defense-in-depth alongside the HTML marker detection
1 parent 61bb2c5 commit 20fcc3d

File tree

4 files changed

+92
-15
lines changed

4 files changed

+92
-15
lines changed

.github/workflows/review-pr.yml

Lines changed: 33 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -325,9 +325,15 @@ jobs:
325325
326326
parent=$(gh api "repos/$REPO/pulls/comments/$PARENT_ID" 2>/dev/null || echo "{}")
327327
body=$(echo "$parent" | jq -r '.body // ""')
328-
329-
# Match <!-- cagent-review --> but NOT <!-- cagent-review-reply --> (substring overlap)
330-
if echo "$body" | grep -q "<!-- cagent-review -->" && ! echo "$body" | grep -q "<!-- cagent-review-reply -->"; then
328+
parent_user_type=$(echo "$parent" | jq -r '.user.type // ""')
329+
330+
# Defense-in-depth: verify the root comment was posted by a Bot (agent) AND
331+
# contains the review marker but NOT the reply marker (substring overlap).
332+
# The user.type check prevents matching human comments that happen to contain
333+
# the marker text (e.g., in discussions about the review system).
334+
if [ "$parent_user_type" = "Bot" ] && \
335+
echo "$body" | grep -q "<!-- cagent-review -->" && \
336+
! echo "$body" | grep -q "<!-- cagent-review-reply -->"; then
331337
echo "is_agent=true" >> $GITHUB_OUTPUT
332338
echo "root_comment_id=$PARENT_ID" >> $GITHUB_OUTPUT
333339
@@ -369,6 +375,11 @@ jobs:
369375
REPO: ${{ github.repository }}
370376
FILE_PATH: ${{ steps.check.outputs.file_path }}
371377
LINE: ${{ steps.check.outputs.line }}
378+
# The triggering comment from the webhook payload — guaranteed fresh,
379+
# unlike the API which may have eventual consistency lag.
380+
TRIGGER_COMMENT_BODY: ${{ github.event.comment.body }}
381+
TRIGGER_COMMENT_AUTHOR: ${{ github.event.comment.user.login }}
382+
TRIGGER_COMMENT_ID: ${{ github.event.comment.id }}
372383
run: |
373384
# Fetch the root comment
374385
root=$(gh api "repos/$REPO/pulls/comments/$ROOT_ID")
@@ -377,13 +388,14 @@ jobs:
377388
# Fetch all review comments on this PR and filter to this thread.
378389
# Uses --paginate to handle PRs with >100 review comments.
379390
# Each page is processed by jq independently, then merged with jq -s.
391+
# Note: the triggering comment may not appear here due to eventual
392+
# consistency, so we append it from the webhook payload below.
380393
all_comments=$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/comments" \
381394
--jq "[.[] | select(.in_reply_to_id == $ROOT_ID)]" | jq -s 'add // [] | sort_by(.created_at)')
382395
383396
# Build the thread context and save as step output.
384397
# Use a randomized delimiter to prevent comment body content from
385398
# colliding with the GITHUB_OUTPUT heredoc terminator.
386-
reply_count=$(echo "$all_comments" | jq 'length')
387399
DELIM="THREAD_CONTEXT_$(openssl rand -hex 8)"
388400
389401
{
@@ -402,18 +414,31 @@ jobs:
402414
echo "$root_body"
403415
echo ""
404416
405-
# Add each reply in chronological order
417+
# Add earlier replies from the API (excludes the triggering comment
418+
# to avoid duplication if the API already has it)
419+
reply_count=$(echo "$all_comments" | jq 'length')
406420
for i in $(seq 0 $((reply_count - 1))); do
421+
comment_id=$(echo "$all_comments" | jq -r ".[$i].id")
422+
# Skip the triggering comment — we append it from the payload below
423+
if [ "$comment_id" = "$TRIGGER_COMMENT_ID" ]; then
424+
continue
425+
fi
407426
author=$(echo "$all_comments" | jq -r ".[$i].user.login")
408427
body=$(echo "$all_comments" | jq -r ".[$i].body")
409-
echo "[REPLY $((i + 1)) by @$author]"
428+
echo "[REPLY by @$author]"
410429
echo "$body"
411430
echo ""
412431
done
432+
433+
# Always append the triggering comment last — sourced directly from
434+
# the webhook payload so it's guaranteed to be present.
435+
echo "[REPLY by @$TRIGGER_COMMENT_AUTHOR] ← this is the reply you are responding to"
436+
echo "$TRIGGER_COMMENT_BODY"
437+
echo ""
413438
echo "$DELIM"
414439
} >> $GITHUB_OUTPUT
415440
416-
echo "✅ Built thread context with $reply_count replies"
441+
echo "✅ Built thread context with replies (triggering comment from webhook payload)"
417442
418443
# Safe to checkout PR head because the reply agent only READS files (no code execution)
419444
- name: Checkout PR head
@@ -439,6 +464,7 @@ jobs:
439464
uses: docker/cagent-action/review-pr/reply@latest
440465
with:
441466
thread-context: ${{ steps.thread.outputs.prompt }}
467+
comment-id: ${{ github.event.comment.id }}
442468
anthropic-api-key: ${{ secrets.ANTHROPIC_API_KEY }}
443469
openai-api-key: ${{ secrets.OPENAI_API_KEY }}
444470
google-api-key: ${{ secrets.GOOGLE_API_KEY }}

.github/workflows/self-review-pr.yml

Lines changed: 33 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -229,9 +229,15 @@ jobs:
229229
230230
parent=$(gh api "repos/$REPO/pulls/comments/$PARENT_ID" 2>/dev/null || echo "{}")
231231
body=$(echo "$parent" | jq -r '.body // ""')
232-
233-
# Match <!-- cagent-review --> but NOT <!-- cagent-review-reply --> (substring overlap)
234-
if echo "$body" | grep -q "<!-- cagent-review -->" && ! echo "$body" | grep -q "<!-- cagent-review-reply -->"; then
232+
parent_user_type=$(echo "$parent" | jq -r '.user.type // ""')
233+
234+
# Defense-in-depth: verify the root comment was posted by a Bot (agent) AND
235+
# contains the review marker but NOT the reply marker (substring overlap).
236+
# The user.type check prevents matching human comments that happen to contain
237+
# the marker text (e.g., in discussions about the review system).
238+
if [ "$parent_user_type" = "Bot" ] && \
239+
echo "$body" | grep -q "<!-- cagent-review -->" && \
240+
! echo "$body" | grep -q "<!-- cagent-review-reply -->"; then
235241
echo "is_agent=true" >> $GITHUB_OUTPUT
236242
echo "root_comment_id=$PARENT_ID" >> $GITHUB_OUTPUT
237243
@@ -273,6 +279,11 @@ jobs:
273279
REPO: ${{ github.repository }}
274280
FILE_PATH: ${{ steps.check.outputs.file_path }}
275281
LINE: ${{ steps.check.outputs.line }}
282+
# The triggering comment from the webhook payload — guaranteed fresh,
283+
# unlike the API which may have eventual consistency lag.
284+
TRIGGER_COMMENT_BODY: ${{ github.event.comment.body }}
285+
TRIGGER_COMMENT_AUTHOR: ${{ github.event.comment.user.login }}
286+
TRIGGER_COMMENT_ID: ${{ github.event.comment.id }}
276287
run: |
277288
# Fetch the root comment
278289
root=$(gh api "repos/$REPO/pulls/comments/$ROOT_ID")
@@ -281,13 +292,14 @@ jobs:
281292
# Fetch all review comments on this PR and filter to this thread.
282293
# Uses --paginate to handle PRs with >100 review comments.
283294
# Each page is processed by jq independently, then merged with jq -s.
295+
# Note: the triggering comment may not appear here due to eventual
296+
# consistency, so we append it from the webhook payload below.
284297
all_comments=$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/comments" \
285298
--jq "[.[] | select(.in_reply_to_id == $ROOT_ID)]" | jq -s 'add // [] | sort_by(.created_at)')
286299
287300
# Build the thread context and save as step output.
288301
# Use a randomized delimiter to prevent comment body content from
289302
# colliding with the GITHUB_OUTPUT heredoc terminator.
290-
reply_count=$(echo "$all_comments" | jq 'length')
291303
DELIM="THREAD_CONTEXT_$(openssl rand -hex 8)"
292304
293305
{
@@ -306,18 +318,31 @@ jobs:
306318
echo "$root_body"
307319
echo ""
308320
309-
# Add each reply in chronological order
321+
# Add earlier replies from the API (excludes the triggering comment
322+
# to avoid duplication if the API already has it)
323+
reply_count=$(echo "$all_comments" | jq 'length')
310324
for i in $(seq 0 $((reply_count - 1))); do
325+
comment_id=$(echo "$all_comments" | jq -r ".[$i].id")
326+
# Skip the triggering comment — we append it from the payload below
327+
if [ "$comment_id" = "$TRIGGER_COMMENT_ID" ]; then
328+
continue
329+
fi
311330
author=$(echo "$all_comments" | jq -r ".[$i].user.login")
312331
body=$(echo "$all_comments" | jq -r ".[$i].body")
313-
echo "[REPLY $((i + 1)) by @$author]"
332+
echo "[REPLY by @$author]"
314333
echo "$body"
315334
echo ""
316335
done
336+
337+
# Always append the triggering comment last — sourced directly from
338+
# the webhook payload so it's guaranteed to be present.
339+
echo "[REPLY by @$TRIGGER_COMMENT_AUTHOR] ← this is the reply you are responding to"
340+
echo "$TRIGGER_COMMENT_BODY"
341+
echo ""
317342
echo "$DELIM"
318343
} >> $GITHUB_OUTPUT
319344
320-
echo "✅ Built thread context with $reply_count replies"
345+
echo "✅ Built thread context with replies (triggering comment from webhook payload)"
321346
322347
# Safe to checkout PR head because the reply agent only READS files (no code execution)
323348
- name: Checkout PR head
@@ -343,6 +368,7 @@ jobs:
343368
uses: ./review-pr/reply
344369
with:
345370
thread-context: ${{ steps.thread.outputs.prompt }}
371+
comment-id: ${{ github.event.comment.id }}
346372
anthropic-api-key: ${{ secrets.ANTHROPIC_API_KEY }}
347373
openai-api-key: ${{ secrets.OPENAI_API_KEY }}
348374
google-api-key: ${{ secrets.GOOGLE_API_KEY }}

review-pr/agents/pr-review-reply.yaml

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,9 @@ agents:
5151
## Posting Your Reply
5252
5353
After formulating your response, post it using `gh api` with JSON input piped via stdin.
54-
This avoids shell quoting issues with special characters in the response body:
54+
IMPORTANT: You MUST use the `jq -n --arg` pattern shown below. NEVER construct the JSON
55+
body using shell string interpolation (e.g., `echo "{\"body\": \"$text\"}"`) — this creates
56+
a command injection vulnerability if the response contains quotes or special characters.
5557
5658
```bash
5759
jq -n \

review-pr/reply/action.yml

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,9 @@ inputs:
66
thread-context:
77
description: "Thread context (original comment + replies) to respond to"
88
required: true
9+
comment-id:
10+
description: "ID of the triggering comment (for failure notification reaction)"
11+
required: false
912
anthropic-api-key:
1013
description: "Anthropic API key"
1114
required: false
@@ -31,6 +34,11 @@ inputs:
3134
description: "GitHub token for API access"
3235
required: false
3336

37+
outputs:
38+
agent-outcome:
39+
description: "Outcome of the reply agent (success/failure/skipped)"
40+
value: ${{ steps.run-reply.outcome }}
41+
3442
runs:
3543
using: "composite"
3644
steps:
@@ -72,3 +80,18 @@ runs:
7280
with:
7381
path: ${{ github.workspace }}/.cache/pr-review-memory.db
7482
key: pr-review-memory-${{ github.repository }}-reply-${{ github.run_id }}
83+
84+
# Add a "confused" reaction to the triggering comment if the agent failed,
85+
# so the developer knows their reply wasn't processed.
86+
- name: React on failure
87+
if: always() && steps.run-reply.outcome == 'failure' && inputs.comment-id != ''
88+
continue-on-error: true
89+
shell: bash
90+
env:
91+
GH_TOKEN: ${{ inputs.github-token }}
92+
REPO: ${{ github.repository }}
93+
COMMENT_ID: ${{ inputs.comment-id }}
94+
run: |
95+
gh api "repos/$REPO/pulls/comments/$COMMENT_ID/reactions" \
96+
-f content="confused" --silent || true
97+
echo "😕 Added reaction to indicate reply failed"

0 commit comments

Comments
 (0)