UAV-develop/PX4-Autopilot
1
0
Fork 0
mirror of https://github.com/PX4/PX4-Autopilot.git synced 2026-07-24 15:27:41 +08:00
Code Issues Packages Projects Releases Wiki Activity

chore(claude): overhaul review-pr skill to focus on substance (#27892)

Browse Source 
* chore(claude): overhaul review-pr skill to focus on substance

The skill spent most of its 207 lines on process: merge-strategy
recommendations, commit-message auditing, formatting checks, and an
interactive posting menu. That feedback is nit-picky noise for a PR
author, and CI already enforces formatting.

Rewrite it (49 lines) around reviewing substance: merit and need (root
cause vs papering over), first-principles analysis for physics/math
changes, architecture consistency with the surrounding code, and an
alternatives assessment. Output is a debrief for the operator plus a
terse, actionable draft comment explicitly attributed as a Claude
review.

Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>

* chore(claude): generalize numeric-robustness check beyond float32

The physics/math check hardcoded float32; not all flight-critical math
uses it. Phrase the robustness check in terms of the given
representation, keeping float32 precision as a concrete example.

Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>

* chore(claude): fold maintainability into the architecture check

Rename the check to Architecture and maintainability and direct it at
hidden cross-module coupling — a change that silently breaks an
assumption in another module — asking for a contract-codifying unit
test when such coupling is unavoidable.

Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>

* chore(claude): add reliability to the correctness check

Reliability was not called out anywhere. Rename to Correctness and
reliability and add resource exhaustion and failure-path handling to
the list of defects to look for.

Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>

* chore(claude): make the debrief lead with findings

A per-file restatement of a well-described PR wastes tokens and time.
Have the debrief lead with actionable findings and only explain what
the change does when the PR description is missing, ambiguous, or
misleading.

Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>

* chore(claude): require provable, objectively-framed findings

Direct the draft comment to frame findings as engineering tradeoffs
rather than judgments, and to stay silent on any flaw it cannot
demonstrate via a concrete code path or first-principles derivation.

Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>

---------

Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
This commit is contained in:
Jacob Dahl
2026-07-14 10:03:37 -06:00
committed by GitHub
parent 25bb4a2224
commit 219fcf387d
1 changed files with 26 additions and 184 deletions
Download Patch File Download Diff File Expand all files Collapse all files

210
.claude/skills/review-pr/SKILL.md
View File

@@ -1,207 +1,49 @@
---
name: review-pr
description: Review a pull request with structured, domain-aware feedback
description: Substance-focused review of a PX4 pull request — merit, first-principles correctness, architecture fit. Produces a debrief for the user and a draft review comment.
argument-hint: "<PR number or URL>"
allowed-tools: Bash, Read, Glob, Grep, Agent
---
# PX4 Pull Request Review
Review a pull request with domain-aware checks based on which files are changed.
Goal: brief the user on a PR before they spend their own time on it, and draft a review comment they can choose to post. The review is by Claude and is presented as such — write from Claude's frame of reference, not the user's, unless the user explicitly requests a different voice.
**No Claude attribution anywhere.**
Focus on substance only. Do not review commit messages, do not recommend a merge strategy, do not audit formatting (CI enforces it). Be extremely thorough — the user wants a deep understanding of the problem, not just verdicts — but scale effort with impact: roughly proportional to lines/files changed and how flight-critical the touched code is. For diffs too large to hold at once, fan out Explore agents per subsystem.
## Steps
## Gather
1. **Fetch PR context.** Run these in parallel:
- `gh pr view <PR> --json number,title,body,baseRefName,headRefName,files,commits,reviewRequests,reviews,author`
- `gh pr checks <PR>` (exit code 8 means some checks are pending, this is normal, not an error)
- `gh pr diff <PR>` -- if this fails with HTTP 406 (300+ files), do NOT retry. Instead use `gh api repos/OWNER/REPO/pulls/NUMBER/files --paginate` to get the full file list in one call, then fetch patches for key infrastructure files individually and sample representative changes from each domain touched.
- `gh api repos/OWNER/REPO/pulls/NUMBER/comments --paginate --jq '.[] | {user: .user.login, body: .body, path: .path, created_at: .created_at}'` to get inline review comments
- `gh api repos/OWNER/REPO/issues/NUMBER/comments --paginate --jq '.[] | {user: .user.login, body: .body, created_at: .created_at}'` to get PR conversation comments
In parallel:
From the PR metadata, note:
- **Assigned reviewers**: who has been requested to review (from `reviewRequests`)
- **Existing reviews**: who has already reviewed and their verdict (from `reviews` -- approved, changes_requested, commented, dismissed)
- **PR comments and inline comments**: read all existing feedback to avoid duplicating points already raised by other reviewers, and to build on their discussion rather than ignoring it
- `gh pr view <PR> --json number,title,body,author,baseRefName,files,reviews`
- `gh pr diff <PR>` — on HTTP 406 (huge diff) do not retry; use `gh api repos/{owner}/{repo}/pulls/<PR>/files --paginate` and fetch key patches selectively
- `gh pr checks <PR>` (exit code 8 just means checks are pending)
- existing feedback: `gh api repos/{owner}/{repo}/pulls/<PR>/comments --paginate --jq '.[] | {user: .user.login, path, body}'` (inline) and `gh api repos/{owner}/{repo}/issues/<PR>/comments --paginate --jq '.[] | {user: .user.login, body}'` (conversation)
2. **Check CI status.** From the `gh pr checks` output in step 1, summarize pass/fail/pending. If there are failures, fetch logs with `gh run view <run-id> --log-failed`. Include CI status in the output.
Read any linked issue — the claimed problem is the baseline for judging merit. For full post-change file contents without touching the working tree: `git fetch origin pull/<PR>/head`, then `git show FETCH_HEAD:<path>`.
3. **Recommend merge strategy.** Analyze the commit history and recommend squash or rebase merge. This decision informs all subsequent commit hygiene feedback.
## Review
**Recommend rebase merge** when:
- Commits are atomic, each builds/works independently
- Each commit has a proper `type(scope): description` message
- The PR intentionally separates logical changes (e.g., refactor + feature, or one commit per module)
- The commit history tells a useful story that would be lost by squashing
Never judge a hunk from the diff alone. Read the enclosing function and file, and grep for callers and consumers — a PR is always reviewed in the context of the surrounding code.
**Recommend squash merge** when:
- There are WIP, fixup, or review-response commits
- Commit messages are messy or inconsistent
- The PR is a single logical change spread across multiple commits
- There are "oops" or "make format" commits mixed in
- **Merit and need.** Is this solving a real problem or papering over one? Trace the failure mechanism; check the fix addresses the root cause at the right layer. Ask whether a simpler change (different default, existing parameter, deleting code) would achieve the same.
- **Physics and math.** If the change involves physics, estimation, control, or nontrivial math, do a first-principles analysis: re-derive the result and check the implementation against your derivation, not against the PR description. Verify units, reference frames (FRD/NED, body/earth), signs, singularities and edge cases (division by zero, gimbal lock, low airspeed), timestep/discretization dependence, and numerical robustness in the given representation (NaN propagation, cancellation, float32 precision limits).
- **Architecture and maintainability.** Does the change follow the patterns of the code around it (uORB usage, scheduling, param handling, data ownership)? Does it reimplement something an existing library provides (AlphaFilter, SlewRate, mathlib)? Does the logic live in the module that owns the data? Watch for hidden coupling — a change that silently breaks an assumption in another module; if cross-module coupling is genuinely necessary, expect a unit test that codifies the new contract and fails if it is later broken. Embedded reality: flash is scarce, no dynamic allocation in flight code, mind CPU cost.
- **Alternatives.** Identify plausible alternative implementations and judge each: better, worse, or not worth mentioning. Raise only the ones the author should genuinely consider, and say why.
- **Correctness and reliability.** Edge cases, overflow, unsigned arithmetic, initialization, bounds, races, resource exhaustion, and failure-path handling. Flag real defects, not style.
- **Compatibility.** Changes to `msg/`, params, or MAVLink: impact on QGC, uLog tooling, and third-party integrations. uORB: `timestamp` vs `timestamp_sample`, `device_id`. Every new parameter is configuration burden on users — challenge it.
- **New board** (`boards/<mfr>/<board>/`): require a logs.px4.io flight log for the vehicle type, a docs page in `docs/en/flight_controller/`, the manufacturer's own USB VID/PID, a unique board_id in `boards.json`, CI build coverage, flash fit, and no copy-pasted template leftovers or board-local forks of common drivers.
Include the recommendation in the output. If recommending rebase, flag any commits that break atomicity or have bad messages. If recommending squash, don't bother flagging individual commit messages (they'll be discarded) but ensure the PR title is correct since it becomes the squash commit message.
Also, briefly: the PR title must be `type(scope): description` (it becomes the commit subject on squash); note CI status and read failure logs only when plausibly related to the change; do not repeat feedback other reviewers already gave — build on it or stay silent.
4. **Check conventional commit title.** Verify the PR title follows `type(scope): description` per CONTRIBUTING.md. The PR title becomes the commit message on squash-merge, so it must be accurate and descriptive. Verify the scope matches the primary area of changed files. If the PR introduces breaking changes, the title must include `!` before the colon. If rebase merge was recommended in step 3, also scan individual commit messages for anti-patterns: vague messages ("fix", "update"), missing type prefix, review-response noise ("apply suggestions from code review", "do make format"), or WIP markers. Flag these for rewording.
## Deliver
5. **Identify domains touched.** Classify changed files into domains based on paths (a PR may touch multiple):
- **Estimation**: `src/modules/ekf2/`, `src/lib/wind_estimator/`, `src/lib/world_magnetic_model/`
- **Control**: `src/modules/mc_*control*/`, `src/modules/fw_*control*/`, `src/modules/flight_mode_manager/`, `src/lib/rate_control/`, `src/lib/npfg/`, `src/modules/vtol_att_control/`
- **Drivers/CAN**: `src/drivers/`, `src/modules/cyphal/`, `src/drivers/uavcan*/`
- **Simulation**: `src/modules/simulation/`, `Tools/simulation/`
- **System**: `src/modules/commander/`, `src/modules/logger/`, `src/systemcmds/`, `platforms/`, `src/modules/dataman/`
- **Board Addition**: `boards/{manufacturer}/{board}/` (new directories only, not modifications to existing boards)
- **CI/Build**: `.github/`, `CMakeLists.txt`, `Makefile`, `cmake/`, `Tools/`, `Kconfig`
- **Messages/Protocol**: `msg/`, `src/modules/mavlink/`, `src/modules/uxrce_dds_client/`
End with two things, then stop — do not post anything.
6. **Apply core checks** (always):
- **Correctness**: logic errors, off-by-ones, unhandled edge cases
- **Type safety**: int16 overflow, float/double promotion, unsigned subtraction, use `uint64_t` for absolute time
- **Initialization**: uninitialized variables, missing default construction
- **Buffer safety**: unchecked array access, stack allocation of large buffers, snprintf bounds
- **Magic numbers**: every numeric literal needs a named constant or justification
- **Framework reuse**: use PX4_ERR/WARN/INFO, existing libraries (AlphaFilter, SlewRate, RateControl), MAVLink constants from the library
- **Naming**: accurate, no unjustified abbreviations, current terminology (GPS -> GNSS for new code)
- **Unnecessary complexity**: can code be removed instead of added? Is there a simpler pattern?
- **Test coverage**: new features should include unit or integration tests; bug fixes should include regression tests where practical. When automated testing is infeasible (hardware-specific), require a flight log link from https://logs.px4.io or bench test evidence.
- **PR hygiene**: focused scope, no unrelated formatting, no stale submodule changes. Commits should be atomic and independently revertable. Multiple WIP or review-response commits should be squashed. Clean, logical commits will be preserved individually on main via rebase merge. **Do NOT assume PRs are squash-merged. Both squash and rebase merge are enabled; merge commits are disabled.** Verify the PR targets `main` unless it is a backport or release-specific fix.
- **Formatting**: `make format` / `make check_format` (astyle) for C/C++ files; `clang-tidy` clean. Python files checked with `mypy` and `flake8`. PRs failing CI format or lint checks will not be merged.
- **Coding style**: C/C++ must follow the [PX4 coding style](https://docs.px4.io/main/en/contribute/code.html)
- **Necessity**: challenge every addition with "Why?" Is this actually needed or just copied? Can we change a default instead of adding runtime detection?
- **Root cause vs symptom**: is this fixing the real problem or masking it?
- **Ecosystem impact**: what does this change mean for QGC users, log analysis tools, and third-party integrations?
- **Sustainability**: who will maintain this? Does it create long-term burden?
- **Architecture fit**: does the code live in the module that naturally owns the data? Are there unnecessary cross-module dependencies?
- **End user impact**: will parameters confuse less-technical users? Are error messages actionable in QGC?
**1. Debrief for the user.** Lead with the findings — the user wants actionable information, not a play-by-play of the diff. Give only the framing they need to judge the change: whether the claimed problem is real, scope and risk (subsystems touched, flight-critical or not), and what CI and other reviewers already said. Restate what the change actually does and why only when the PR description is missing, ambiguous, or misleading. Order findings by severity (blocker / concern / nit) with `file:line`, and end with a one-or-two-sentence overall take.
7. **Apply domain checks** based on step 5:
**2. Draft review comment**, shown verbatim in a fenced block. First line: `**Claude review on behalf of @<login>**` (login via `gh api user --jq .login`). Then terse, actionable findings only — what is wrong, why it matters, what to do, with file:line references. Frame each as an objective engineering tradeoff, not a moral judgment. Never guess: if you cannot demonstrate a flaw via a concrete code path or a first-principles derivation, stay silent. No praise or "verified OK" notes unless they change what the author should do, no meta sections, no conversational filler, nothing already raised by others.
**Estimation:**
- Singularities in aerospace math (euler angles near gimbal lock, sideslip at low airspeed)
- Aliasing from downsampling sensor data without filtering
- Kalman filter correctness (Joseph form, innovation variance, covariance symmetry)
- CPU cost on embedded targets (avoid unnecessary sqrt, limit fusion rate)
- Frame/coordinate system correctness (FRD vs NED, body vs earth)
If the user asks to post it, post the draft verbatim with `gh pr comment <PR> --body "$(cat <<'EOF' ... EOF)"`.
**Control:**
- Phase margin: output filters consume margin for no benefit; prefer adjusting gyro/d-gyro cutoffs
- Circular dependencies: sensor data feeding back into its own control loop (e.g., throttle-based airspeed in TECS)
- NaN propagation in flight-critical math; check `PX4_ISFINITE` before magnitude checks
- Setpoint generation vs output-stage hacks: prefer proper setpoint smoothing over controller output filtering
- Yaw control edge cases: heading lock, drift, setpoint propagation
- Flight task inheritance chain: correct base class for the desired behavior
- Control allocation: actuator function ordering, motor index mapping
**Drivers/CAN:**
- CAN bus devices behave differently from serial/SPI; check driver assumptions
- ESC index mapping: telemetry index != channel when motors are disabled
- ESC hardware quirks: 4-in-1 ESCs may report current on only one channel
- device_id correctness and I2CSPIDriver patterns
- Time representation: prefer `hrt_abstime` over iteration counts
**Simulation:**
- Physics fidelity: noise models should match reality (GPS noise is not Gaussian)
- Keep gz_bridge generic; vehicle-specific logic belongs in plugins
- Prefer gz-transport over ROS2 dependencies when possible
- Wrench commands for physics correctness vs kinematic constraints
- Library generic/specific boundary: only base classes in common libs
**System:**
- Race conditions and concurrency: no partial fixes, demand complete solutions
- Semaphore/scheduling edge cases; understand RTOS guarantees
- State machine sequential-logic bugs (consecutive RTL, armed/disarmed alternation)
- uORB-driven scheduling (`SubscriptionCallback`), not extra threads
- param_set triggers auto-save; no redundant param_save_default
- Flash/memory efficiency: avoid `std::string` on embedded, minimize SubscriptionData usage
- Constructor initialization order matters
**CI/Build:**
- Pipeline race conditions (tag + branch push double-trigger, git describe correctness)
- Container image size (check layer bloat)
- Ubuntu LTS support policy (latest + one prior only)
- Build time impact
- CMake preferred over Makefiles
**Messages/Protocol:**
- Backwards compatibility: will this break QGC, post-flight tools, or uLog parsers?
- uORB: `timestamp` for publication metadata, `timestamp_sample` close to physical sample, include `device_id`
- Don't version messages unless strictly needed
- Parameter UX: will this confuse users in a GCS? Every new param is a configuration burden
- MAVLink: use library constants, don't implement custom stream rates
**Board Addition:**
- **Flight logs**: require a link to https://logs.px4.io demonstrating basic operation for the vehicle type (hover for multicopters, stable flight for fixed-wing, driving for rovers, etc.); short bench-only logs are insufficient
- **Documentation**: require a docs page in `docs/en/flight_controller/` with pinout, where-to-buy, connector types, version badge, and manufacturer-supported notice block
- **USB VID/PID**: must not reuse another manufacturer's Vendor ID; manufacturer must use their own
- **Board naming**: directory is `boards/{manufacturer}/{board}/`, both lowercase, hyphens for board name
- **Unique board_id**: registered in `boards/boards.json`, no collisions
- **Copied code cleanup**: check for leftover files, configs, or comments from the template board; "Is this real or leftover?"
- **RC configuration**: prefer `CONFIG_DRIVERS_COMMON_RC` over legacy `CONFIG_DRIVERS_RC_INPUT`
- **No board-specific custom modules**: reject copy-pasted drivers (e.g., custom heater) when existing infrastructure works
- **Bootloader**: expect a bootloader defconfig (`nuttx-config/bootloader/defconfig`) or explanation of shared bootloader
- **CI integration**: board must be added to CI compile workflows so it builds on every PR
- **Flash constraints**: verify enabled modules fit in flash; we are running low across all board targets
- **Port labels**: serial port labels must match what is physically printed on the board
- **Hardware availability**: for unknown manufacturers, verify the product exists and is purchasable (no vaporware)
8. **Format output** as:
- **CI status**: pass/fail summary, link to failed runs if any
- **Merge strategy**: recommend squash or rebase merge with reasoning
- **Title check**: pass/fail with suggestion
- **Review status**: list assigned reviewers and any existing reviews (who approved, who requested changes, key points already raised). Note if your review would duplicate feedback already given.
- **Domains detected**: list which domain checks were applied
- **Summary**: one paragraph on what the PR does and whether the approach is sound
- **Issues**: numbered list, each with file:line, severity (blocker/warning/nit), and explanation. Skip issues already raised by other reviewers unless you have something to add.
- **Verdict**: approve, request changes, or needs discussion
After the structured output, also display a **draft PR comment** formatted using the PR comment formatting rules from step 9. This gives the user a preview of what would be posted.
9. **Interactive dialog.** After displaying the review, present the user with these options:
Present options based on the verdict:
If verdict is **approve**:
```
What would you like to do?
1. Chat about this PR (ask questions, explore code) [default]
2. Approve this PR and post the review comment
3. Adjust the review or draft (tell me what to change)
4. Done for now
```
If verdict is **request changes**:
```
What would you like to do?
1. Chat about this PR (ask questions, explore code) [default]
2. Request changes on this PR and post the review comment
3. Adjust the review or draft (tell me what to change)
4. Done for now
```
If verdict is **needs discussion**:
```
What would you like to do?
1. Chat about this PR (ask questions, explore code) [default]
2. Post the review as a comment (no approval or rejection)
3. Adjust the review or draft (tell me what to change)
4. Done for now
```
Wait for the user to choose before proceeding. If they pick:
- **1 (chat)**: enter a free-form conversation about the PR. The user can ask about specific files, code paths, or decisions. When done, loop back to the options. This is the default if the user just presses enter.
- **2 (submit)**: use the draft PR comment already shown. Before posting, check if you have review permissions: run `gh api repos/OWNER/REPO/collaborators/$(gh api user --jq .login)/permission --jq .permission` -- if `admin` or `write`, submit as a formal review with `gh pr review <PR> --approve --body "..."` or `gh pr review <PR> --request-changes --body "..."` based on the verdict. If no write access, fall back to `gh pr comment <PR> --body "..."`. Always confirm with the user before posting.
- **3 (adjust)**: ask what to change, update the review and draft, then loop back to the options.
- **4 (done)**: stop.
**PR comment formatting rules** (for the draft):
When writing the GitHub comment, rewrite the review to sound like a human reviewer, not a structured report. Do NOT include the full skill output. Instead:
- Drop most meta-sections (CI status, title check, domains detected, severity labels) but keep the merge strategy recommendation (e.g., "I'd suggest a rebase merge here since the commits are clean and atomic" or "This should be squash-merged, the commit history is messy")
- Write conversationally: "Nice work on this. A few things I noticed:" not "Issues: 1. file:line (warning):"
- Lead with a brief take on the overall change (1-2 sentences)
- List only actionable feedback as natural review comments, not numbered checklists
- Skip nits unless they are particularly useful
- End with a clear stance: looks good to merge, needs a few changes, or let's discuss X
- Post with `gh pr comment <PR> --body "$(cat <<'EOF' ... EOF)"`. Do not post without explicit confirmation.
If the user provided arguments, use them as context: $ARGUMENTS
PR to review: $ARGUMENTS