Pr Review

PR Quality Review

Perform a comprehensive quality review of a pull request or feature branch. This skill covers production readiness, code quality, UX, documentation, tests, and safety.

The review is structured as a checklist across seven dimensions. For each dimension, investigate the actual code and report specific findings -- not just "looks good" but concrete observations with file paths and line numbers.

Getting Started

First, understand the scope of the change:

# What branch are we on, what's the base?
git branch --show-current
git log main..HEAD --oneline

# What files changed?
git diff main...HEAD --stat

# Full diff for review
git diff main...HEAD

If reviewing a remote PR, fetch it first:

gh pr view <number> --json title,body,files
gh pr diff <number>

Read the PR description and all commits to understand the intent before diving into code.

Review Dimensions

Work through each dimension below. For each one, report a status:

• Pass -- meets the bar, no issues
• Needs work -- specific issues found (list them)
• N/A -- not applicable to this change

1. Production Readiness

Does this code behave correctly and handle failure gracefully?

• Error handling: Are all errors checked? Are they wrapped with context (fmt.Errorf("context: %w", err))? Do they surface actionable messages to users?
• Edge cases: Empty inputs, nil values, missing config, network failures, API rate limits, large datasets, pagination boundaries
• Safety checks: All mutating commands (create, edit, apply, delete, update) must include safety checks after LoadConfig() and before client operations. Pattern:

  checker, err := NewSafetyChecker(cfg)
  if err := checker.CheckError(safety.OperationXXX, safety.OwnershipUnknown); err != nil { return err }
  

  Verify correct operation type. Skip only in dry-run paths.
• No stdout in library code: pkg/ must return data, not print. Only cmd/ handles output.
• No hardcoded secrets or customer data: No real names, env IDs, tokens, or emails in code or tests. Use @example.invalid for emails (RFC 2606).

2. Code Quality

Is the code clean, idiomatic, and maintainable?

• Go idioms: Follows Effective Go and Go Code Review Comments
• Naming: Descriptive names, Go conventions (camelCase unexported, PascalCase exported), -er suffix for interfaces
• File size: Files should be under 500 lines. Large files should be split.
• Imports: Standard library first, then third-party, then internal (github.com/dynatrace-oss/dtctl)
• Duplication: Look for copy-pasted code that should be extracted into helpers
• Comments: Exported functions/types documented. Comments explain "why" not "what".
• Consistent patterns: New code should follow existing patterns in the codebase. Check similar resources in pkg/resources/ for reference implementations.

Run the linter to catch issues the eye might miss:

make lint-strict

3. User Experience

Does this feel right from the user's perspective?

• Command naming: Follows the dtctl <verb> <resource> pattern. No custom query flags -- use DQL passthrough.
• Output formatting: Table output is readable, columns make sense, no misalignment. JSON/YAML output is clean.
• Error messages: Clear, actionable, suggest next steps. In agent mode (-A), errors are structured JSON with machine-readable codes.
• Interactive behavior: Name resolution, disambiguation prompts work. --plain disables interactive behavior.
• Help text: Command has a Short description, Long description, and Example field. Parent verb commands have examples.
• Aliases: Resource has sensible aliases (e.g., wf for workflow, dash for dashboard).
• Agent mode: Commands support --agent envelope with contextual suggestions. Test with -A -o json.
• Color control: Respects NO_COLOR, FORCE_COLOR, --plain, and TTY detection.

Try running the actual commands to see how they feel:

# Does the help text look good?
dtctl <command> --help

# Does table output look right?
dtctl <command> --plain

# Does agent mode work?
dtctl <command> -A

4. Test Coverage

Are the changes well-tested?

• Unit tests: New functions have tests. Table-driven tests preferred.
• Coverage targets: 70% minimum overall, 80% for new packages, 90% for critical packages (pkg/client, pkg/config).
• Edge case tests: Not just happy paths -- test error conditions, empty inputs, boundary values.
• Mock server guards: Paginated mock servers must reject invalid parameter combinations (e.g., page-size + page-key). Settings API mocks must also reject schemaIds/scopes with nextPageKey.
• Golden tests: If output formatting changed or a new resource was added, golden files must be updated. Check:

  go test ./pkg/output/ -run TestGolden
  

  Golden tests use real production structs from pkg/resources/* -- never test-only duplicates.
• E2E tests: Integration scenarios in test/e2e/ for new resources or complex workflows.

# Run full suite
go test ./...

# Check coverage
make test-coverage

5. Documentation

Is the change properly documented for users and contributors?

Always required:

• Conventional commit title: The PR title (and squash-merge commit) must be a valid conventional commit (feat: ..., fix: ..., feat!: ... for breaking changes) — release-please derives the version bump and GitHub Release notes from it. There is no CHANGELOG.md to edit.

Required for new features:

• README.md: Updated if the feature is user-facing and significant (new resource type, new command category)
• docs/QUICK_START.md: Usage examples for major new features
• docs/dev/IMPLEMENTATION_STATUS.md: Feature matrix rows updated
• docs/dev/API_DESIGN.md: Design patterns documented if introducing new conventions
• docs/TOKEN_SCOPES.md: New scopes documented if the feature requires additional API permissions

Required for new resources:

• Resource-specific doc page in docs/ or docs/site/_docs/
• Command reference updated in docs/site/_docs/command-reference.md

Required for new AI agent support:

• README.md, docs/QUICK_START.md, docs/dev/API_DESIGN.md, docs/dev/IMPLEMENTATION_STATUS.md (all four)

6. GitHub Pages

If the change adds a new user-facing feature, is the documentation site updated?

The site lives in docs/site/ and deploys via GitHub Actions on pushes to main that touch docs/site/**.

Check:

• New doc page: Does the feature need a page in docs/site/_docs/? Use YAML frontmatter with title, layout: docs.
• Navigation: Is the new page added to docs/site/_includes/docs-nav.html in the correct section (Getting Started / Resources / Reference)?
• Landing page: Does docs/site/index.md need updating? (e.g., new resource in the feature table, new capability mentioned)
• Existing pages: Are related pages updated to mention the new feature? (e.g., a new output format should appear on the output-formats page)
• Links: All links work, relative paths are correct, no broken references.

7. PR Description

Is the PR itself well-described?

• Title: Clear, follows conventional commit style (feat: ..., fix: ...)
• Summary: Explains what changed and why (not just what files were touched)
• Related issues: References issues with Closes #NNN or Fixes #NNN
• Breaking changes: Called out explicitly if any
• Testing section: Describes how the change was tested
• UX examples: Before/after CLI output for user-facing changes

Review Output

After completing the review, provide a summary in this format:

## PR Review: <title>

| Dimension | Status | Notes |
|-----------|--------|-------|
| Production readiness | Pass/Needs work | ... |
| Code quality | Pass/Needs work | ... |
| User experience | Pass/Needs work | ... |
| Test coverage | Pass/Needs work | ... |
| Documentation | Pass/Needs work | ... |
| GitHub Pages | Pass/Needs work/N/A | ... |
| PR description | Pass/Needs work | ... |

### Issues Found
1. **[Dimension]** file:line -- description of issue
2. ...

### Suggestions (non-blocking)
1. ...

### Verdict
Ready to merge / Needs revisions (list blockers)

Be direct and specific. Reference exact file paths and line numbers. Distinguish between blocking issues (must fix) and suggestions (nice to have). Don't pad the review with praise -- focus on what needs attention.