Skip to content

REVIEW.md

Latest commit

 

History

History
96 lines (68 loc) · 7.46 KB

REVIEW.md

File metadata and controls

96 lines (68 loc) · 7.46 KB

Code Review Guidelines

Review guidelines for PRs to Pup, whether authored by humans or AI agents. These rules are enforced during automated and manual review. Inspired by Vibe Maintainer principles — we welcome AI-assisted contributions but hold a high quality bar.

Audience

This project is built agent-first, human-second. Code should be easy for AI coding agents to read, navigate, and modify. It should also be clear to humans reviewing it. When the two goals conflict, favor explicitness and simplicity.

Review Checklist

Every PR is evaluated against the following criteria. A failure in any MUST item blocks merge.

1. Code Reuse (MUST)

  • Reuse existing utilities. Before writing a new helper, search src/ for an existing function that does the same thing. Common locations:
    • src/util.rs — time parsing, validation helpers
    • src/formatter.rs — output formatting (JSON, YAML, table, agent envelope)
    • src/client.rs — API client construction and request helpers
    • src/config.rs — configuration loading and precedence
  • No duplicate code. If two or more call sites need the same logic, extract it into a shared function in the appropriate module. Three identical lines is the threshold — extract on the third occurrence.
  • No reimplementation of standard library or dependency features. Use anyhow, serde, clap, and other existing dependencies rather than hand-rolling equivalents.

2. Test Coverage (MUST)

See TESTING.md for full test strategy, coverage thresholds, and conventions. Review-specific expectations:

  • Positive tests required. Every new public function or subcommand must have at least one test demonstrating correct behavior.
  • Negative tests required. Every error path, validation check, or fallible operation must have a test proving the failure mode works as intended (returns the right error, rejects bad input, etc.).
  • No test regressions. cargo test must pass with zero failures. Existing tests must not be deleted or weakened without justification.

3. Security (MUST)

See CONTRIBUTING.md security guidelines for full details. Review-specific expectations:

  • No malicious code. PRs containing obfuscated code, backdoors, exfiltration attempts, unauthorized network calls, or any code that does not serve the stated purpose of the PR will be rejected and the contributor banned.
  • No credential exposure. Never log, print, or include in error messages: API keys, tokens, secrets, or passwords. Grep your diff for DD_API_KEY, DD_APP_KEY, access tokens, and similar patterns.
  • Input validation at boundaries. All user-supplied input (CLI args, environment variables, config file values) must be validated before use. Prevent command injection, path traversal, and other OWASP Top 10 vulnerabilities.

4. Dependency Hygiene (MUST)

  • Latest stable versions only. New dependencies must use the latest stable release. Existing dependencies must not be pinned to versions with known CVEs.
  • No unnecessary dependencies. Do not add a crate for something the standard library or an existing dependency already handles. Justify every new entry in Cargo.toml.
  • Run cargo audit (or equivalent) before submitting. PRs that introduce dependencies with known vulnerabilities will be rejected.
  • Minimal version range. Use precise version constraints (e.g. "1.2" not "*") to avoid accidental breaking upgrades.

5. Complexity (MUST)

  • Keep functions small and focused. A function should do one thing. If it takes more than ~40 lines, consider splitting it.
  • Avoid deeply nested logic. More than 3 levels of indentation is a code smell. Use early returns, match arms, or extract helper functions.
  • No premature abstraction. Don't introduce traits, generics, or builder patterns unless there are already 2+ concrete implementations that justify the abstraction.
  • No dead code. Remove unused functions, imports, and commented-out blocks. cargo clippy catches most of these.
  • Prefer explicit over clever. A few extra lines of clear code is better than a one-liner that requires a comment to explain.

6. PR Hygiene (SHOULD)

  • One concern per PR. A bug fix, a new feature, and a refactor should be separate PRs. Mixed-concern PRs may be cherry-picked or split-merged.
  • Minimal diff. Change only what's needed. Don't reformat untouched code, rename unrelated variables, or add unrelated improvements.
  • Rebase before submitting. PRs based on stale forks create unnecessary merge conflicts.
  • No draft PRs. Submit only when the work is ready for review.
  • Descriptive title and body. Follow the conventional commit format: <type>(<scope>): <subject>. Include what changed and why.

7. Rust-Specific Standards (MUST)

See CONTRIBUTING.md code style and error handling sections for full conventions. Review-enforced minimums:

  • cargo fmt — code must be formatted. CI enforces this.
  • cargo clippy -- -D warnings — no clippy warnings. CI enforces this.
  • Use anyhow for errors. Wrap errors with context: .map_err(|e| anyhow::anyhow!("context: {e:?}")).
  • Follow Rust idioms. Use Option/Result instead of sentinel values. Use iterators over manual loops where appropriate. Use match over chains of if let.

Review Outcomes

Adapted from the Vibe Maintainer workflow. Listed in order of preference — we optimize for throughput and getting contributions merged:

Priority Outcome Description
1 Merge PR meets all standards — merge as-is.
2 Merge-fix PR is mergeable but has minor issues — merge as-is, then push a follow-up fix to main.
3 Fix-merge PR has value but is broken (fails CI, has bugs) — pull locally, fix, and push with contributor attribution.
4 Cherry-pick PR bundles multiple changes and we only want some — cherry-pick the good parts locally, fix as needed, commit with attribution, and close the PR.
5 Split-merge PR mixes separate concerns that should be independent — pull locally, split into separate commits with attribution, and push each.
6 Reimplement PR solves a real problem but the design is wrong — reimplement the solution with a better approach, close the PR with thanks and an explanation.
7 Retire PR is obsolete — superseded by another PR, already fixed, or no longer relevant. Close with a thank-you.
8 Reject PR adds tech debt, is too niche for core, contains malicious code, or violates security rules. Close with a polite explanation.
9 Request changes Last resort. Sending a PR back for rework delays landing and risks contributor attrition. Only use when the contributor is best positioned to make the fix.

For AI Agent Authors

If you are an AI agent generating a PR for this project:

  1. Read this file and CLAUDE.md before generating code.
  2. Search the codebase first for existing utilities, patterns, and conventions.
  3. Run cargo test, cargo clippy, and cargo fmt --check before submitting.
  4. Include both positive and negative tests for any new functionality.
  5. Do not introduce code that you cannot explain. Every line should serve a clear purpose.
  6. Do not add unnecessary files, comments, or documentation beyond what the change requires.