Conversation
|
@claude /review |
|
We need to merge this to try it, it seems. |
| prompt: | | ||
| Please review this pull request and provide feedback on: | ||
| 1. Potential bugs, such as: | ||
| - Off-by-one | ||
| - Incorrect conditionals | ||
| - Use of wrong variable when multiple variables of same type are in scope | ||
| - `min` vs `max`, `first` vs `last`, flipped ordering | ||
| - Iterating over hashmap/hashset in order-sensitive operation | ||
| 2. Panic branches that cannot be locally proven to be unreachable: | ||
| - `unwrap` or `expect` | ||
| - Indexing operations | ||
| - Panicking operations on external data | ||
| 3. Dead code that is not caught by warnings: | ||
| - Overriding values that should be read first | ||
| - Silently dead code due to `pub` | ||
| - `todo!()` or `dbg!()` | ||
| 4. Performance: | ||
| - Blocking operations in async code | ||
| - DB connection with lifetimes that exceed a local scope | ||
| 5. Inconsistencies between comments and code | ||
| 6. Backwards compatibility: | ||
| - Changes to `Deserialize` structs that break existing data. | ||
| - Check that DB migrations should keep compatibility when possible: | ||
| - Use `IF NOT EXIST`. | ||
| - Avoid dropping columns or altering their data types. | ||
| - Check if migration can be made friendly to rollbacks. | ||
| - Breaking changes to HTTP APIs or CLIs. | ||
| 7. Documentation: | ||
| - The `config.sample.toml` should be kept up-to-date. | ||
| 8. Security concerns | ||
| 9. Testing: | ||
| - Reduced test coverage without justification | ||
| - Tests that don't actually test the intended behavior | ||
| - Tests with race conditions or non-deterministic behavior | ||
| - Integration tests that should be unit tests (or vice versa) | ||
| - Changes to existing tests that weaken assertions | ||
| - Changes to tests that are actually a symptom of breaking changes to user-visible behaviour. | ||
|
|
||
| Use `gh pr comment` with your Bash tool to leave your review as a comment on the PR. |
There was a problem hiding this comment.
It would be interesting to add this as a Claude command and instruct here to read that command's prompt/ This way, we can also use it offline—ideally, pre-push.
There was a problem hiding this comment.
@LNSD you mean extract this to a markdown in .patterns?
There was a problem hiding this comment.
Sorry, I thought it was a "standard" thing. I am referring to this "commands" thing:
- https://github.com/Effect-TS/effect-smol/tree/main/.claude/commands
- https://github.com/Effect-TS/effect-smol/blob/70b06e42a85b05408ad5ba1fcff47924e8ceb843/.claude/commands/new-feature.md
They are like preset procedures that you can ask Claude to follow. The flow would be something like: Hey, Claude, let's conduct a code review. And it would read the AGENTS.md file, and follow the .claude/commands/code-review.md procedure.
It differs from .patterns in that patterns are intended to describe code patterns. And commands are "reusable procedures".
There was a problem hiding this comment.
As a suggestion, one interesting thing to add to the "code-review" command would be:
- [ ] Review the changeset for `.patterns/` violations.
This is how I would relate "commands" with "patterns".
There was a problem hiding this comment.
Ah yhea that is the standard way to add a slash command, this was just my ignorance. Done.
Code ReviewThis PR adds a GitHub Actions workflow for automated Claude code reviews triggered by @claude /review comments. The implementation looks solid overall with appropriate security measures and clear documentation. ✅ Strengths
📝 Minor Suggestions
🔒 Security Assessment
The implementation follows security best practices and poses no immediate concerns. 🎯 OverallClean implementation of automated code review functionality. The review guidelines are comprehensive and well-thought-out, covering critical areas from bugs to architecture consistency. This should significantly enhance the code review process. |
2 participants
Testing out github workflow for claude code bot