Skip to content

tools: make Claude PR reviewer use subagents and maintain a summary post#440

Merged
qdeslandes merged 1 commit intofacebook:mainfrom
pzmarzly:push-kuxlolwzwqqx
Feb 25, 2026
Merged

tools: make Claude PR reviewer use subagents and maintain a summary post#440
qdeslandes merged 1 commit intofacebook:mainfrom
pzmarzly:push-kuxlolwzwqqx

Conversation

@pzmarzly
Copy link
Contributor

With Claude's help, I changed the prompt to address 2 problems:

  1. We want to have a tracking comment that will live beyond a single review loop. Let's mark it with a hidden HTML marker, and then Claude can update it the next time it runs.
  2. We noticed that on long PRs, a single run doesn't find all the issues. Let's explicitly instruct Claude how to use subagents, so that it has a better chance of fitting everything in context.

Demo: pzmarzly/demo--claude-bot-reviews#8 (comment)

@github-actions
Copy link

Claude: review of facebook/bpfilter #440 (6723086)

Must-fix (1): Task tool is not in --allowedTools — the entire subagent architecture from Phase 2 will not function at runtime.

Suggestions (3):

  • Rigid 5-subagent structure wastes tokens on non-code PRs (docs, YAML, etc.)
  • Reviews endpoint lost its jq filter for empty-body reviews (context noise regression)
  • Tracking comment update path is complex, untested in the demo, and may need issues: write permission for the PATCH API call

See inline comments for details.

@pzmarzly pzmarzly marked this pull request as draft February 24, 2026 18:29
@pzmarzly pzmarzly marked this pull request as ready for review February 24, 2026 18:58
@github-actions
Copy link

Claude: The previous review's must-fix (Task tool missing from --allowedTools) has been addressed in this revision — Task is now present at line 125.

One new suggestion: the /review-pr reference in the prompt is non-functional because Skill is not in --allowedTools. Since the Phase 2 categories are self-contained, dropping the reference resolves both the dead-letter issue and the duplication concern. See inline comment.

CLAUDE.md improvement: The commit message format says component: subcomponent: short description but recent tools: commits (including this one and its parent) omit the subcomponent. Consider documenting that subcomponents are optional when the component alone is specific enough.

@github-actions
Copy link

Claude: Previous must-fix (Task tool missing from --allowedTools) and suggestion (/review-pr Skill reference non-functional) are both resolved in this revision.

One new suggestion: the Read .claude/commands/review-pr.md instruction is wasteful since most of the file doesn't apply in CI, and the parts that do are duplicated by the Phase 2 categories. See inline comment with a concrete proposal addressing the duplication concern.

@github-actions
Copy link

Claude: Previous must-fix (Task tool missing) and suggestions (/review-pr Skill reference, review-pr.md duplication) from earlier reviews are all addressed or acknowledged in this revision.

New suggestions (2):

  • Add a concurrency group to prevent race conditions when rapid pushes trigger overlapping reviews that compete to update the tracking comment
  • Qualify the "read the full pre-patch file" subagent instruction to handle PRs that add new files (no pre-patch file on disk)

See inline comments for details.

@github-actions
Copy link

Claude: Previous must-fix (Task tool missing) and suggestions (/review-pr Skill reference, race condition) are all resolved in this revision.

One new suggestion: resolve the review-pr.md duplication concern by making that file the single source of truth for review categories, and keeping the workflow prompt focused on CI orchestration only (phases, subagents, tracking comment, dedup). See inline comment.

@github-actions
Copy link

Claude: Clean revision. Prior must-fix issues are resolved. The three-phase structure, flexible subagent count (2-8), verification requirements, and tracking comment mechanism address the stated goals.

One new suggestion: the CLAUDE.md compliance consolidation logic from the old prompt was removed without replacement — the tracking comment template has a "CLAUDE.md improvements" section but no instructions on when/how to populate it. See inline comment.

@github-actions
Copy link

Claude: Well-refined after 6 review rounds. Previous must-fixes (missing Task tool, broken /review-pr reference, race condition) are all resolved. The three-phase structure, flexible subagent count, verification requirements, and tracking comment mechanism address the stated goals.

New suggestions (2):

  • Conflicting output format: subagents are told to read review-pr.md (which defines a tables/PASS-FAIL report format) and also return a JSON array. Adding "ignore the Output format sections" would reduce ambiguity. See inline comment.
  • Phase 3 ordering: creating the tracking comment before posting inline comments would be more resilient to mid-run cancellation. See inline comment.

CLAUDE.md improvement: The commit message format says component: subcomponent: short description but recent tools: commits (including this one and its parent) omit the subcomponent. Consider documenting that subcomponents are optional when the component alone is specific enough.

@github-actions
Copy link

Claude: Review of revision 72da22c after 7 prior review rounds.

Previous must-fixes (missing Task tool, broken /review-pr reference, race condition) are all resolved. The three-phase structure, flexible subagent count, verification requirements, and tracking comment mechanism are well-designed.

New observations (2 nits/suggestions): Documentation section ambiguity in subagent instructions, and template placeholders that could use GHA expressions. See inline comments.

Still open from prior reviews: The conflicting output format concern (review-pr.md PASS/FAIL reports vs JSON array return) at line 73 hasn't been addressed or acknowledged.

@github-actions
Copy link

Claude: Review of revision 2354f55 after 8 prior review rounds.

Prior must-fixes (missing Task tool, broken /review-pr reference, race condition) are all resolved. The three-phase structure, flexible subagent count (2-8), verification requirements, tracking comment mechanism, and CLAUDE.md consolidation logic are well-designed.

New nits (2):

  • The PATCH command for tracking comment updates is fragile with complex markdown content — suggest using --input - pattern instead of -f body="..."
  • Subagent instructions at lines 67-69 don't propagate the skip directives from line 61, which is the root cause of the previously-raised conflicting output format concern

Still open from prior reviews: The conflicting output format concern (review-pr.md Report subsections vs JSON return format) is partially addressed by line 61 but the skip isn't forwarded to subagents. See inline comment with a concrete suggestion.

@github-actions
Copy link

Claude: Review of revision 3cf4f53 after 8 prior review rounds.

Previous must-fixes (missing Task tool, broken /review-pr reference, missing concurrency group) are all resolved. The three-phase structure, flexible subagent count (2-8), verification requirements, CLAUDE.md consolidation logic, and tracking comment mechanism are well-designed.

No new must-fix issues.

Remaining open items from prior reviews (not addressed or dismissed):

  • Subagent skip propagation is partial: "Output format" and "Report" are forwarded (line 70) but "Build", "Test", "Coverage", "Setup", "Cleanup" are not. Mitigated by the positive instruction at line 63 specifying which sections subagents should handle, and by subagents lacking Bash(make:*) permissions.
  • Template placeholders (<REPO>, <PR NUMBER>, <HEAD SHA>) in the tracking comment template (line 105) could be replaced with GHA expressions that auto-resolve before the agent sees the prompt.

CLAUDE.md improvement: The commit message format says component: subcomponent: short description but recent tools: commits (this PR and its parent) omit the subcomponent. Consider documenting that subcomponents are optional when the component alone is specific enough.

Copy link
Contributor

@qdeslandes qdeslandes left a comment

Choose a reason for hiding this comment

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

From your demo, I see there are multiple comments. Did Claude properly update the existing comment when instructed to do so?

@pzmarzly
Copy link
Contributor Author

Yes:

image

This last comment is the one that has <!-- claude-pr-review --> in it.

I should prepare a fresh demo setup at some point, but I was hoping this PR will be the last for now, so I got a bit lazy.

@qdeslandes
Copy link
Contributor

Great, LGTM! Thanks!

@qdeslandes qdeslandes merged commit bf228a0 into facebook:main Feb 25, 2026
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants