Skip to content

tools: claude: use new review prompt to catch more issues#490

Merged
qdeslandes merged 1 commit intofacebook:mainfrom
pzmarzly:push-usptmmqtykvn
Mar 26, 2026
Merged

tools: claude: use new review prompt to catch more issues#490
qdeslandes merged 1 commit intofacebook:mainfrom
pzmarzly:push-usptmmqtykvn

Conversation

@pzmarzly
Copy link
Copy Markdown
Contributor

@pzmarzly pzmarzly commented Mar 25, 2026

Seems like telling AI to do more work makes it do more work. (And costs more money.) Shocked Pikachu.

Tested on #489 using my local testing scripts (pzmarzly/claude-pr-review#test/):

# Issue Upstream bot New bot
1 Path traversal: unvalidated cache_dir POST field flows into shutil.rmtree (bfoptimize-web:119) must-fix suggestion
2 git reset --hard silently destroys uncommitted work (bfoptimize:542) suggestion
3 bypassPermissions gives Claude agent unrestricted Bash (bfoptimize:259) suggestion suggestion
4 NO_TESTS option description is stale, now also disables benchmarks (CMakeLists.txt:33) suggestion
5 NO_TESTS doc description should mention benchmarks (build.rst:98) suggestion
6 No validation on model/effort string fields (bfoptimize-web:48) suggestion
7 daemon: cgen: commit prefix is stale — daemon was removed from project (bfoptimize:249) must-fix (5/5 reviewers)
8 -DNO_TESTS=1 contradicts make unit e2e integration — test targets won't exist (bfoptimize:246) must-fix (5/5 reviewers)
9 results.html.j2 deleted instead of movedbfbencher history --report-path broken at runtime (bfbencher:39) must-fix (4/5 reviewers)
10 DELETE /history crashes with FileNotFoundError if cache dir doesn't exist yet (bfoptimize-web:180) suggestion
11 --fail-on-benchmark-error dead code for history subcommand (bfbencher:1298) suggestion (3/5 reviewers)
12 CI build job may fail — find_package(benchmark REQUIRED) now runs without benchmark dep (ci.yaml:85) suggestion
13 git commit -am stages all tracked files, not just CGEN_DIR/ (bfoptimize:249) suggestion
14 No subprocess output captured on benchmark failure (bfoptimize:238) nit

And checked if workflow still works in: pzmarzly/claude-pr-review#16

@pzmarzly pzmarzly requested a review from qdeslandes as a code owner March 25, 2026 20:10
@meta-cla meta-cla bot added the cla signed label Mar 25, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 25, 2026

Claude review of PR #490 (4a027ab)

Nits

  • Stale "subagent" terminology.github/workflows/claude-pr-review.yaml:210 — One unchanged context line still says "not for subagent output" while all modified lines use "agent"/"reviewer"

Clean PR overall. YAML syntax is correct, --effort max and Agent tool additions are properly placed, the prompt instructions are clear and internally consistent, and the commit message follows project conventions (tools: claude:, imperative mood, under 72 chars). The read-only security posture of the review job is preserved since sub-agents inherit the same allowedTools restriction.

Workflow run

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

Thanks! Looking forward to try this out!

@qdeslandes qdeslandes merged commit 628a348 into facebook:main Mar 26, 2026
34 checks passed
@pzmarzly pzmarzly deleted the push-usptmmqtykvn branch March 26, 2026 14:10
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