Skip to content

Commit 782d067

Browse files
docs: Add comprehensive code review feedback guidance to AGENTS.md (#202)
Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: William Easton <strawgate@users.noreply.github.com>
1 parent cacb180 commit 782d067

File tree

1 file changed

+145
-2
lines changed

1 file changed

+145
-2
lines changed

AGENTS.md

Lines changed: 145 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,139 @@ to the async package. You will need to include the generated code in your pull
112112
request. Nobody will generate it for you. This also means pull requests will contain
113113
two copies of your changes, this is intentional!
114114

115+
## Working with Code Review Feedback
116+
117+
This project uses AI-assisted code review (CodeRabbit) and development (Claude).
118+
This section provides guidance for both AI agents and human developers on how
119+
to handle automated code review feedback effectively.
120+
121+
### For AI Coding Agents (Claude)
122+
123+
When responding to CodeRabbit feedback on pull requests:
124+
125+
#### 1. Triage Before Acting
126+
127+
Always categorize feedback before implementation:
128+
129+
- **Critical**: Security issues, data corruption, resource leaks, production bugs
130+
- **Important**: Type safety, error handling, performance issues, test failures
131+
- **Optional**: Style preferences, nitpicks, suggestions that conflict with
132+
existing patterns
133+
134+
Document your triage in the response to the user.
135+
136+
#### 2. Evaluate Against Existing Patterns
137+
138+
Before accepting suggestions:
139+
140+
1. Search the codebase for similar patterns
141+
2. Check if other stores/wrappers handle this differently
142+
3. Preserve consistency over isolated "best practices"
143+
4. If uncertain, ask the repository owner
144+
145+
**Example**: Test patterns should match existing `ContextManagerStoreTestMixin`
146+
usage rather than one-off suggestions for individual test files.
147+
148+
#### 3. Consider Context and Scope
149+
150+
Not all code has the same requirements:
151+
152+
- **Production stores**: Prioritize correctness, performance, security
153+
- **Debug/development tools**: Can defer async optimization, extensive validation
154+
- **Test code**: Clarity and coverage over production patterns
155+
- **Examples**: Simplicity and readability over comprehensive error handling
156+
157+
#### 4. Verify Completion
158+
159+
Before claiming work is "ready to merge" or "complete":
160+
161+
- [ ] All critical issues addressed or documented as out-of-scope
162+
- [ ] All important issues addressed or explicitly deferred with rationale
163+
- [ ] No unrelated changes from bad merges
164+
- [ ] `make precommit` passes (lint, typecheck, codegen)
165+
- [ ] Tests pass
166+
167+
Never claim completion with unresolved critical or important issues.
168+
169+
#### 5. Document Deferrals
170+
171+
If feedback is not implemented, explain why:
172+
173+
- Conflicts with established pattern (cite similar code)
174+
- Out of scope for this component's purpose
175+
- Trade-off not worth the complexity
176+
- Requires design decision from repository owner
177+
178+
### For AI Code Reviewers (CodeRabbit)
179+
180+
When reviewing pull requests, please consider:
181+
182+
#### Project-Specific Patterns
183+
184+
- **Async-first architecture**: All changes originate in `key-value/key-value-aio/`
185+
and are code-generated into `key-value/key-value-sync/`. PRs will naturally
186+
contain duplicate changes - this is intentional.
187+
- **Test patterns**: The project uses `ContextManagerStoreTestMixin` for store
188+
tests. Look for consistency with existing test implementations.
189+
- **ManagedEntry wrapper**: Values are never stored directly but are wrapped in
190+
`ManagedEntry` objects. This is by design, not a mistake.
191+
192+
#### Prioritization Guidance
193+
194+
When providing feedback, please categorize suggestions by severity:
195+
196+
- **Critical**: Issues that could cause data loss, security vulnerabilities,
197+
resource leaks, or production failures
198+
- **Important**: Type safety issues, missing error handling, performance problems,
199+
test coverage gaps
200+
- **Minor/Optional**: Style preferences, minor optimizations, suggestions that
201+
may conflict with existing patterns
202+
203+
This helps human developers and AI agents prioritize their work effectively.
204+
205+
#### Context Awareness
206+
207+
Consider the purpose and scope of the code being reviewed:
208+
209+
- **Production stores** (DynamoDB, Redis, PostgreSQL, etc.): Apply strict
210+
standards for correctness, performance, security, and resource management
211+
- **Debug/development tools** (FileTreeStore, LoggingWrapper): More lenient on
212+
performance optimizations; prioritize clarity and simplicity
213+
- **Test code**: Focus on clarity, coverage, and maintainability over production
214+
patterns
215+
- **Example code**: Prioritize readability and educational value over
216+
comprehensive error handling
217+
218+
#### Pattern Consistency
219+
220+
Before suggesting changes:
221+
222+
1. Check if similar patterns exist elsewhere in the codebase
223+
2. If the pattern exists in multiple stores/wrappers, it's likely intentional
224+
3. Suggest consistency improvements across all implementations rather than
225+
one-off changes
226+
227+
### Common Feedback Categories
228+
229+
**Clock usage**: Prefer monotonic clocks (`time.monotonic()`) for intervals,
230+
especially in wrappers like rate limiters and circuit breakers. Wall-clock time
231+
(`time.time()`) is vulnerable to system clock changes.
232+
233+
**Connection ownership**: Track whether stores own their client connections to
234+
avoid closing externally-provided resources. Use flags like `_owns_client` to
235+
distinguish between internally-created and externally-provided connections.
236+
237+
**Async patterns**: Production stores should use actual async I/O (not
238+
`asyncio.sleep()` or `run_in_executor()`). Debug-only tools may use simpler
239+
patterns for clarity.
240+
241+
**Test isolation**: Ensure tests clean up resources (connections, temp files,
242+
etc.) and don't interfere with each other. Use context managers and proper
243+
teardown.
244+
245+
**Type safety**: This project uses strict type checking (Basedpyright). Address
246+
type annotation issues to maintain type safety guarantees.
247+
115248
## Make Commands Reference
116249

117250
| Command | Purpose |
@@ -244,5 +377,15 @@ make bump-version-dry VERSION=1.2.3 # Dry run
244377

245378
## Radical Honesty
246379

247-
Agents should be honest! Properly document any problems encountered, share
248-
feedback, and be transparent about your AI-assisted work.
380+
Agents should be honest! When working with code review feedback:
381+
382+
- **Document unresolved items**: List any feedback that wasn't addressed and why
383+
- **Acknowledge uncertainty**: If unclear whether to implement a suggestion, ask
384+
- **Report problems**: Document issues encountered during implementation
385+
- **Share trade-offs**: Explain reasoning for rejecting or deferring feedback
386+
- **Admit limitations**: If unable to verify a fix works correctly, say so
387+
388+
Never claim work is complete if you have doubts about correctness or completeness.
389+
390+
Properly document any problems encountered, share feedback, and be transparent
391+
about your AI-assisted work.

0 commit comments

Comments
 (0)