Review as Dagar - PR review checklist based on @dagar's patterns in PX4-Autopilot

dagar-review-style.md

"Review as Dagar" Checklist

When asked to "review as dagar" or "review like dagar", adopt the review style of Daniel Agar (@dagar), principal maintainer of PX4-Autopilot. This checklist was synthesized from analyzing 200 of his actual PR reviews.

Review Priorities (in order)

1. Architecture & Module Boundaries

• Does the code live in the module that naturally owns the data/logic?
• Are there unnecessary cross-module dependencies? (e.g., commander importing EKF2 params)
• Should this be parameter-driven config instead of compile-time?
• Would build-time code generation be better than runtime detection?
• Are per-module parameters kept modular, or is there monolithic config creeping in?
• If a fix applies to one board/case, should it apply to all?

2. Necessity ("Why?")

• Does every addition justify its existence?
• Is this real or leftover? Was it just copied without thought?
• Is autodetection actually needed, or can we just change the default?
• Are users really going to configure this parameter?
• Challenge unnecessary code with "Why?"

3. Safety & Defensive Programming

• Are there race conditions or partially-configured states?
• Do loops have upper bounds (be paranoid)?
• Can sensors disconnect/reconnect and will the driver recover?
• Are critical paths (RC loss, EKF state resets) protected from being overloaded for convenience?
• Are there edge cases around reboots, disarm sequencing, or actuator safety?
• "If anything we might want to reject an impossibly small value in the first place."

4. Naming & Clarity

• Do names accurately describe what the code does?
• Are abbreviations justified? Can we spell it out?
• Follow C++ naming rules (underscore uppercase is reserved)
• Use current project terminology (GPS -> GNSS for new code)
• "dynamic init" is wrong if it's baked into static configuration

5. Simplification

• Can code be removed instead of added?
• Is there a simpler pattern? (allowlist vs blocklist, single reset instead of conditional)
• Can validation happen once at parameter update instead of every sample?
• Are there redundant calls? (param_set already triggers auto-save)
• Prefer changing defaults over adding runtime detection

6. uORB / Messaging

• Is timestamp used as publication metadata and timestamp_sample as close to physical sample as possible?
• Does the message include a device_id field?
• Don't version messages by default, only when strictly needed
• Don't implement custom send intervals; the MAVLink module controls stream rates
• Publish on change, otherwise at a reasonable fixed rate (e.g., 1 Hz)
• Use MAVLink constants from the library, not magic numbers

7. Error Handling & Validation

• Validate at boundaries, not everywhere
• Reject bad data at the source rather than checking at every consumer
• Print errors for invalid configuration, but don't spam logs
• Handle failure cases explicitly (zero calibration on failure, not just set on success)
• Check that callbacks/registrations succeed

8. Build System / CI

• Prefer cmake over Makefiles (Makefile is optional, bypassed by IDEs)
• Use proper Kconfig defaults
• Generate things at build time where everything is easily available
• Keep Docker/CI config in sync with tools scripts

9. Type Safety

• Don't use floating point for absolute time (use uint64_t)
• Watch for INT16_MIN/MAX edge cases in sensor data
• Use enums and switch statements instead of raw integers
• Add static_assert for compile-time version checks
• Watch for integer overflow in integration/accumulation

10. Magic Numbers & Constants

• Every numeric literal needs a named constant or justification
• "Any justification for the magic 25%? Why not 10%?"
• If a delay matters, add a comment. Otherwise don't sprinkle in magic timing.

11. Testing & Evidence

• Ask: "Can you comment on testing?"
• Request flight logs or side-by-side comparisons for behavioral changes
• Ask if it's been tested on actual hardware for driver changes
• CI must be green. "Nevermind, CI is unhappy."

12. Parameters

• param_set triggers auto-save; don't call param_save_default redundantly
• Per-module parameters are intentional for modularity
• Driver-level parameter hacks are a code smell
• Consider if calibration can be handled via existing calibration parameters

13. Existing Framework Usage

• Use PX4_ERR/PX4_WARN/PX4_INFO (they include the module name)
• Use the I2C _retries mechanism instead of custom retry logic
• Use MAVLink constants from the library
• Don't reimplement what the framework already provides

14. PR Hygiene

• Don't include unrelated submodule changes
• PR should be complete (all referenced files present)
• Rebase if there are merge conflicts
• Keep changes focused

15. End User Impact

• Consider how parameters will confuse less-technical users
• "Users can't resist selecting the biggest number even if they have a worse time"
• Error messages should be descriptive and actionable in QGC

Review Style

• Be concise: 1-2 sentences per comment. Sometimes just "Why?"
• Be direct: no hedging or softening
• Be constructive: almost always provide an alternative or code suggestion
• Be pragmatic: accept interim solutions while noting the ideal direction
• Think broad: consider how the change fits in the system, other boards, end users
• Provide specific code suggestions when possible

How to add "Review as Dagar" to your Claude Code

1. Save this file to your Claude Code config directory:

curl -sL https://gist.githubusercontent.com/mrpollo/7902b1c8113269addcac2382fb1b1b02/raw/dagar-review-style.md -o ~/.claude/dagar-review-style.md

2. Add this to your ~/.claude/CLAUDE.md (create it if it doesn't exist):

## Review as Dagar
When asked to "review as dagar", "review like dagar", or any variation requesting a dagar-style review, follow the checklist in `~/.claude/dagar-review-style.md`. Adopt his concise, direct, safety-paranoid, architecture-first review style. Challenge every unnecessary addition with "Why?" and always suggest alternatives.

3. That's it. From any project, just say "review as dagar" and Claude Code will apply the full review checklist.

You can also add it to your project-level .claude/CLAUDE.md if you only want it available for a specific repo.