|
| 1 | +--- |
| 2 | +name: review |
| 3 | +description: Code review a pull request for Redpanda Connect, checking Go patterns, tests, component architecture, and commit policy |
| 4 | +argument-hint: "[pr-number]" |
| 5 | +disable-model-invocation: true |
| 6 | +allowed-tools: Bash(gh pr view *), Bash(gh pr diff *), Bash(gh pr comment *), Bash(git log *), Bash(git show *), Bash(git diff *), Read, Glob, Grep, Task |
| 7 | +--- |
| 8 | + |
| 9 | +Code review pull request $ARGUMENTS for Redpanda Connect. If no PR was specified, resolve the current branch's PR with `gh pr view --json number -q .number`. |
| 10 | + |
| 11 | +This review orchestrates specialized agents for domain-specific analysis. Do not duplicate the expertise of these agents -- delegate to them and synthesize their findings. |
| 12 | + |
| 13 | +## Workflow |
| 14 | + |
| 15 | +1. **Gather context** - Collect the information needed for review. Prefer running these in parallel when possible: |
| 16 | + - Collect paths to relevant CLAUDE.md files (root `CLAUDE.md`, `config/CLAUDE.md`, and any in directories touched by the PR) |
| 17 | + - Summarize the PR (files modified, change categories: component implementation, tests, configuration, CLI, etc.) |
| 18 | + |
| 19 | +2. **Review** - Launch review agents. Each receives the PR diff, change summary, and relevant CLAUDE.md content. Each returns a list of issues with a reason, confidence score 0-100, and (for scores 50-74) a brief explanation of why the reviewer is uncertain. Prefer running independent agents in parallel when possible. |
| 20 | + |
| 21 | + Confidence scale: |
| 22 | + - 0: False positive or pre-existing issue |
| 23 | + - 25: Possibly real, possibly false positive. Stylistic issues not in CLAUDE.md/skill files. |
| 24 | + - 50: Real but uncertain — reviewer lacks context to confirm severity or correctness. Include uncertainty reason (e.g., "unfamiliar domain pattern", "can't determine intent without runtime context", "possible edge case but depends on caller behavior"). |
| 25 | + - 75: Verified, will be hit in practice. Directly impacts functionality or mentioned in CLAUDE.md/skill files. |
| 26 | + - 100: Confirmed, will happen frequently. Evidence directly confirms this. |
| 27 | + |
| 28 | + **Go Patterns & Architecture** (`godev` agent): Component registration (single vs batch MustRegister*), ConfigSpec construction, field name constants, ParsedConfig extraction, Resources pattern, import organization, license headers, formatting/linting, error handling (wrapping with gerund form, %w), context propagation (no context.Background() in methods, no storing ctx on structs), concurrency patterns (mutex, goroutine lifecycle), shutdown/cleanup (idempotent Close, sync.Once), public wrappers, bundle registration, info.csv metadata, distribution classification. |
| 29 | + |
| 30 | + **Tests** (`tester` agent): Unit: table-driven tests with errContains, assert vs require, config parsing with MockResources, enterprise InjectTestService, processor/input/output/bloblang lifecycle tests, config linting, NewStreamBuilder pipelines, HTTP mock servers. Integration: integration.CheckSkip(t), Given-When-Then with t.Log(), testcontainers-go (module helpers preferred, GenericContainer fallback), NewStreamBuilder with AddBatchConsumerFunc, side-effect imports, async stream.Run with context.Canceled handling, assert.Eventually polling (no require inside), parallel subtest safety, cleanup with context.Background(). Flag changed code lacking tests and new components without integration tests. |
| 31 | + |
| 32 | + **Bugs and Security** (general-purpose agent): Logic errors, nil dereferences, race conditions, resource leaks, SQL/command injection, XSS, hardcoded secrets. Focus on real bugs, not nitpicks. |
| 33 | + |
| 34 | + **Commit Policy** (general-purpose agent): Uses `git log` and `git show --stat` on the PR commits. Checks: |
| 35 | + - **Granularity**: Each commit is one small, self-contained, logical change. Flag commits mixing unrelated work. |
| 36 | + - **Message format** (enforced): Must match one of these patterns: |
| 37 | + - `system: message` (e.g., `otlp: add authz support`, `kafka: fix consumer group rebalance`) |
| 38 | + - `system(subsystem): message` (e.g., `gateway(authz): add http middleware`, `cli(mcp): handle shutdown`) |
| 39 | + - Uppercase plain message for repo-wide/global changes (e.g., `Bump to Go 1.26`, `Update CI workflows`) |
| 40 | + - **Message quality** (enforced): Flag messages that are vague ("fix stuff", "updates", "WIP"), misleading (title doesn't match the actual changes), or incomprehensible. |
| 41 | + - **Fixup/squash**: Flag unsquashed `fixup!`/`squash!` commits. |
| 42 | + - Ignore PR number suffixes `(#1234)`. |
| 43 | + |
| 44 | +3. **Filter** - Drop issues scoring below 50. Separate remaining items into two buckets: |
| 45 | + - **Issues** (score >= 75): Confirmed problems to flag. |
| 46 | + - **Attention areas** (score 50-74): Areas where the reviewer is uncertain and a human should look. Each must include the uncertainty reason from step 2. |
| 47 | + |
| 48 | + If both buckets are empty, skip commenting. |
| 49 | + |
| 50 | +4. **Comment** - Post results via `gh pr comment`. Keep output brief, no emojis. Link and cite relevant code/files/URLs with full git SHA. |
| 51 | + |
| 52 | +## False Positives to Filter (steps 2 and 3) |
| 53 | + |
| 54 | +- Pre-existing issues not introduced in this PR |
| 55 | +- Code that looks wrong but is intentional |
| 56 | +- Pedantic nitpicks a senior engineer wouldn't flag |
| 57 | +- Issues that linters, typecheckers, or compilers catch (imports, types, formatting) |
| 58 | +- General quality issues unless explicitly required in CLAUDE.md or skill files |
| 59 | +- Issues called out in CLAUDE.md but silenced in code via lint ignore comments |
| 60 | +- Functionality changes that are clearly intentional |
| 61 | +- Real issues on lines the user did not modify |
| 62 | + |
| 63 | +## Comment Format |
| 64 | + |
| 65 | +If issues found (score >= 75): |
| 66 | + |
| 67 | +``` |
| 68 | +### Code review |
| 69 | +
|
| 70 | +Found N issues: |
| 71 | +
|
| 72 | +1. <brief description> (<source>: "<relevant guideline>") |
| 73 | +
|
| 74 | +<link to file and line with full SHA + line range> |
| 75 | +
|
| 76 | +2. ... |
| 77 | +
|
| 78 | +Generated with Claude Code |
| 79 | +``` |
| 80 | + |
| 81 | +If attention areas found (score 50-74), append after issues (or after the heading if no issues): |
| 82 | + |
| 83 | +``` |
| 84 | +### Areas for human review |
| 85 | +
|
| 86 | +The following areas had observations where the automated review was not confident enough to flag as issues. A human reviewer should verify these. |
| 87 | +
|
| 88 | +1. <brief description> — <uncertainty reason> |
| 89 | +
|
| 90 | +<link to file and line with full SHA + line range> |
| 91 | +
|
| 92 | +2. ... |
| 93 | +``` |
| 94 | + |
| 95 | +If neither issues nor attention areas found: |
| 96 | + |
| 97 | +``` |
| 98 | +### Code review |
| 99 | +
|
| 100 | +No issues found. Checked for bugs, Go patterns and component architecture (godev), test quality (tester), commit policy, and CLAUDE.md compliance. |
| 101 | +
|
| 102 | +Generated with Claude Code |
| 103 | +``` |
| 104 | + |
| 105 | +## Link Format |
| 106 | + |
| 107 | +Links must follow this exact format for GitHub Markdown rendering: |
| 108 | +``` |
| 109 | +https://github.com/redpanda-data/connect/blob/[full-sha]/path/file.ext#L[start]-L[end] |
| 110 | +``` |
| 111 | +- Full git SHA required (not abbreviated) |
| 112 | +- `#L` notation after filename |
| 113 | +- Line range format: `L[start]-L[end]` |
| 114 | +- Include at least 1 line of context before and after |
| 115 | + |
| 116 | +## Notes |
| 117 | + |
| 118 | +- Do not build, lint, or run tests. Those run separately in CI. |
| 119 | +- Use `gh` for all GitHub interactions, not web fetch. |
| 120 | +- Create a todo list first to track progress. |
| 121 | +- Cite and link every issue (if referring to a CLAUDE.md or skill file, link it). |
0 commit comments