docs: seed initial review rules for automated code review#1867
docs: seed initial review rules for automated code review#1867felixweinberger merged 4 commits intomainfrom
Conversation
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
felixweinberger
force-pushed
the
fweinberger/review-conventions
branch
from
6996f08 to
6b0adac
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — purely additive documentation file seeding review conventions, no code changes.
Extended reasoning...
Overview
This PR adds a single new file, REVIEW.md, containing review conventions for the repository. No source code, tests, or configuration files are modified.
Security risks
None. This is a documentation-only change.
Level of scrutiny
Minimal. The file contains prose guidelines and a checklist derived from past human review rounds. The rules are internally consistent and well-grounded in PR references.
Other factors
No bugs were found by the bug hunting system. No changeset is needed for a docs-only addition. The content is already serving as the repo_prompt for automated review, so it can be validated in practice immediately.
felixweinberger
force-pushed
the
fweinberger/review-conventions
branch
from
6b0adac to
9894555
Compare
felixweinberger
force-pushed
the
fweinberger/review-conventions
branch
from
9894555 to
a6a37ce
Compare
There was a problem hiding this comment.
LGTM — the stale and references I flagged have been corrected.
Extended reasoning...
Overview
Pure documentation change adding REVIEW.md with review conventions, guiding principles, checklist, and a recurring-catches section. No code changes.
Security Risks
None. This is a documentation file with no executable code.
Level of Scrutiny
Low. The only risk is stale or misleading guidance, which was the exact concern in my prior review. Both issues I raised (stale class name and stale Zod v3 API) were fixed in the follow-up commit . The current diff correctly uses / and throughout.
Other Factors
No code, no tests needed, no changeset needed. Content is consistent with the codebase conventions I can verify.
- Add docs-consistency and examples checks to Checklist - Add Reference section with spec MCP server and llms-full.txt - ProtocolError/ProtocolErrorCode (was McpError) - z.looseObject() (was .passthrough())
There was a problem hiding this comment.
LGTM — both previously flagged issues were addressed and the remaining nit was intentionally left.
Extended reasoning...
Overview
Pure documentation PR adding REVIEW.md with review conventions. No code changes.
Prior feedback resolution
Both substantive issues flagged in prior runs have been fixed in the current diff: McpError → ProtocolError/ProtocolErrorCode on line 36, and .passthrough() → z.looseObject() / .catchall(z.unknown()) in the Schema Compliance section. The third comment about migration-SKILL.md was acknowledged as a nit and explicitly left unchanged by the author.
Security risks
None — documentation only.
Level of scrutiny
Low. This is a review guideline document with no runtime impact. Content accuracy has been verified against the codebase across multiple review rounds.
Other factors
All three inline comments are marked resolved. The content is accurate and well-structured.
2 participants
Seed initial review rules for automated code review.
Types of changes
Checklist