mirror of
https://github.com/openclaw/openclaw.git
synced 2026-03-12 07:20:45 +00:00
Merged via squash.
Prepared head SHA: b1fcfba868
Co-authored-by: benjipeng <11394934+benjipeng@users.noreply.github.com>
Co-authored-by: BunsDev <68980965+BunsDev@users.noreply.github.com>
Reviewed-by: @BunsDev
135 lines
5.5 KiB
Markdown
135 lines
5.5 KiB
Markdown
---
|
||
description: Review a PR thoroughly without merging
|
||
---
|
||
|
||
Input
|
||
|
||
- PR: $1 <number|url>
|
||
- If missing: use the most recent PR mentioned in the conversation.
|
||
- If ambiguous: ask.
|
||
|
||
Do (review-only)
|
||
Goal: produce a thorough review and a clear recommendation (READY FOR /landpr vs NEEDS WORK vs INVALID CLAIM). Do NOT merge, do NOT push, do NOT make changes in the repo as part of this command.
|
||
|
||
0. Truthfulness + reality gate (required for bug-fix claims)
|
||
- Do not trust the issue text or PR summary by default; verify in code and evidence.
|
||
- If the PR claims to fix a bug linked to an issue, confirm the bug exists now (repro steps, logs, failing test, or clear code-path proof).
|
||
- Prove root cause with exact location (`path/file.ts:line` + explanation of why behavior is wrong).
|
||
- Verify fix targets the same code path as the root cause.
|
||
- Require a regression test when feasible (fails before fix, passes after fix). If not feasible, require explicit justification + manual verification evidence.
|
||
- Hallucination/BS red flags (treat as BLOCKER until disproven):
|
||
- claimed behavior not present in repo,
|
||
- issue/PR says "fixes #..." but changed files do not touch implicated path,
|
||
- only docs/comments changed for a runtime bug claim,
|
||
- vague AI-generated rationale without concrete evidence.
|
||
|
||
1. Identify PR meta + context
|
||
|
||
```sh
|
||
gh pr view <PR> --json number,title,state,isDraft,author,baseRefName,headRefName,headRepository,url,body,labels,assignees,reviewRequests,files,additions,deletions --jq '{number,title,url,state,isDraft,author:.author.login,base:.baseRefName,head:.headRefName,headRepo:.headRepository.nameWithOwner,additions,deletions,files:.files|length}'
|
||
```
|
||
|
||
2. Read the PR description carefully
|
||
- Summarize the stated goal, scope, and any "why now?" rationale.
|
||
- Call out any missing context: motivation, alternatives considered, rollout/compat notes, risk.
|
||
|
||
3. Read the diff thoroughly (prefer full diff)
|
||
|
||
```sh
|
||
gh pr diff <PR>
|
||
# If you need more surrounding context for files:
|
||
gh pr checkout <PR> # optional; still review-only
|
||
git show --stat
|
||
```
|
||
|
||
4. Validate the change is needed / valuable
|
||
- What user/customer/dev pain does this solve?
|
||
- Is this change the smallest reasonable fix?
|
||
- Are we introducing complexity for marginal benefit?
|
||
- Are we changing behavior/contract in a way that needs docs or a release note?
|
||
|
||
5. Evaluate implementation quality + optimality
|
||
- Correctness: edge cases, error handling, null/undefined, concurrency, ordering.
|
||
- Design: is the abstraction/architecture appropriate or over/under-engineered?
|
||
- Performance: hot paths, allocations, queries, network, N+1s, caching.
|
||
- Security/privacy: authz/authn, input validation, secrets, logging PII.
|
||
- Backwards compatibility: public APIs, config, migrations.
|
||
- Style consistency: formatting, naming, patterns used elsewhere.
|
||
|
||
6. Tests & verification
|
||
- Identify what's covered by tests (unit/integration/e2e).
|
||
- Are there regression tests for the bug fixed / scenario added?
|
||
- Missing tests? Call out exact cases that should be added.
|
||
- If tests are present, do they actually assert the important behavior (not just snapshots / happy path)?
|
||
|
||
7. Follow-up refactors / cleanup suggestions
|
||
- Any code that should be simplified before merge?
|
||
- Any TODOs that should be tickets vs addressed now?
|
||
- Any deprecations, docs, types, or lint rules we should adjust?
|
||
|
||
8. Key questions to answer explicitly
|
||
- Is the core claim substantiated by evidence, or is it likely invalid/hallucinated?
|
||
- Can we fix everything ourselves in a follow-up, or does the contributor need to update this PR?
|
||
- Any blocking concerns (must-fix before merge)?
|
||
- Is this PR ready to land, or does it need work?
|
||
|
||
9. Output (structured)
|
||
Produce a review with these sections:
|
||
|
||
A) TL;DR recommendation
|
||
|
||
- One of: READY FOR /landpr | NEEDS WORK | INVALID CLAIM (issue/bug not substantiated) | NEEDS DISCUSSION
|
||
- 1–3 sentence rationale.
|
||
|
||
B) Claim verification matrix (required)
|
||
|
||
- Fill this table:
|
||
|
||
| Field | Evidence |
|
||
| ----------------------------------------------- | -------- |
|
||
| Claimed problem | ... |
|
||
| Evidence observed (repro/log/test/code) | ... |
|
||
| Root cause location (`path:line`) | ... |
|
||
| Why this fix addresses that root cause | ... |
|
||
| Regression coverage (test name or manual proof) | ... |
|
||
|
||
- If any row is missing/weak, default to `NEEDS WORK` or `INVALID CLAIM`.
|
||
|
||
C) What changed
|
||
|
||
- Brief bullet summary of the diff/behavioral changes.
|
||
|
||
D) What's good
|
||
|
||
- Bullets: correctness, simplicity, tests, docs, ergonomics, etc.
|
||
|
||
E) Concerns / questions (actionable)
|
||
|
||
- Numbered list.
|
||
- Mark each item as:
|
||
- BLOCKER (must fix before merge)
|
||
- IMPORTANT (should fix before merge)
|
||
- NIT (optional)
|
||
- For each: point to the file/area and propose a concrete fix or alternative.
|
||
- If evidence for the core bug claim is missing, add a `BLOCKER` explicitly.
|
||
|
||
F) Tests
|
||
|
||
- What exists.
|
||
- What's missing (specific scenarios).
|
||
- State clearly whether there is a regression test for the claimed bug.
|
||
|
||
G) Follow-ups (optional)
|
||
|
||
- Non-blocking refactors/tickets to open later.
|
||
|
||
H) Suggested PR comment (optional)
|
||
|
||
- Offer: "Want me to draft a PR comment to the author?"
|
||
- If yes, provide a ready-to-paste comment summarizing the above, with clear asks.
|
||
|
||
Rules / Guardrails
|
||
|
||
- Review only: do not merge (`gh pr merge`), do not push branches, do not edit code.
|
||
- If you need clarification, ask questions rather than guessing.
|