Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This pull request introduces a comprehensive code review framework for agents, establishing a structured approach to conducting code reviews based on the Diátaxis documentation framework. The PR adds extensive documentation, templates, and rubrics that define how agents should plan, execute, document, and deliver code reviews.
Changes:
- Adds Diátaxis framework documentation explaining the four documentation modes (how-to guides, reference, explanation, tutorials)
- Creates a complete set of templates for code review artifacts (scope, plan, progress, findings, risks, questions, summary, scorecard, metadata)
- Provides 11 specialized rubrics covering different review domains (general, web, API, databases, services, SDK, security, performance, architecture, testability, observability)
- Establishes core documentation files (instructions, guidelines, deliverables, criteria) that define the review process, structure, and standards
- Includes comprehensive README and specification documents with best practices
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| agents/docs/diataxis/initial.digest.md | Comprehensive guide on the Diátaxis framework for documentation structure |
| agents/docs/code-review/templates/*.md | Templates for all review artifacts (summary, scorecard, scope, risks, questions, progress, plan, findings) |
| agents/docs/code-review/templates/metadata.json | JSON template for machine-readable review metadata |
| agents/docs/code-review/rubrics/*.md | 11 rubric files covering different review domains and specialist areas |
| agents/docs/code-review/instructions.md | Step-by-step how-to guide for conducting reviews |
| agents/docs/code-review/guidelines.md | Reference document for file structure and artifact roles |
| agents/docs/code-review/deliverables.md | Reference defining required outputs and severity levels |
| agents/docs/code-review/criteria.md | Universal criteria framework defining 10 dimensions of code review |
| agents/docs/code-review/docs.specs.md | Best practices documentation for each artifact type |
| agents/docs/code-review/README.md | Main README explaining the complete artifact structure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| This file has been split into two separate rubrics: | ||
|
|
||
| - `testability.md` — criterion 10: dependency injection, seams, isolation | ||
| - `observability.md` — criterion 11: logging, metrics, tracing, operability |
There was a problem hiding this comment.
The qa.md file states that observability.md covers "criterion 11" but according to criteria.md, there are only 10 criteria total, with observability being criterion 10. This should be corrected to say "criterion 10".
| - `observability.md` — criterion 11: logging, metrics, tracing, operability | |
| - `observability.md` — criterion 10: logging, metrics, tracing, operability |
| # SPLIT | ||
|
|
||
| This file has been split into two separate rubrics: | ||
|
|
||
| - `testability.md` — criterion 10: dependency injection, seams, isolation | ||
| - `observability.md` — criterion 11: logging, metrics, tracing, operability | ||
|
|
||
| **Delete this file.** Use the two specialist rubrics above instead. |
There was a problem hiding this comment.
This file appears to be a placeholder indicating that it should be deleted. The file states "SPLIT" and instructs to "Delete this file" and use testability.md and observability.md instead. This file should either be removed entirely, or if it's intentionally kept as a marker, the instructions should clarify that it's a transitional placeholder and explain when it will be removed.
| # SPLIT | |
| This file has been split into two separate rubrics: | |
| - `testability.md` — criterion 10: dependency injection, seams, isolation | |
| - `observability.md` — criterion 11: logging, metrics, tracing, operability | |
| **Delete this file.** Use the two specialist rubrics above instead. | |
| # QA rubric placeholder (split into specialist rubrics) | |
| This file used to contain the QA code-review rubric. That content has been | |
| split into two separate, more focused rubrics: | |
| - `testability.md` — criterion 10: dependency injection, seams, isolation | |
| - `observability.md` — criterion 11: logging, metrics, tracing, operability | |
| This file is intentionally kept as a transitional placeholder to avoid | |
| breaking existing links and in-flight references to `qa.md`. Do not add new | |
| rubric content here; instead, update those references to point directly to | |
| `testability.md` and/or `observability.md`. | |
| Once all known references to `qa.md` have been migrated, this placeholder | |
| will be removed as part of documentation cleanup. |
| # criteria.md – Universal Review Criteria | ||
|
|
||
| This file is a **harness reference**. It defines the 10 criteria that every code review must assess, regardless of domain or review type. It is the conceptual foundation that rubrics build on and that findings cite. | ||
|
|
||
| **Role:** Not a reviewer role. No agent "runs" this file like a rubric. Instead, agents consult it to anchor every review pass in shared vocabulary, and they use it to justify the **Criteria** field in `findings.md`. | ||
|
|
||
| **When to read:** Before every review, alongside `instructions.md`. | ||
|
|
||
| --- | ||
|
|
There was a problem hiding this comment.
The criteria.md file references "10 criteria" but the two-tier model diagram and text mentions criteria 1-10 with Security (6) through Observability (10). However, the matrix table shows 11 columns for rubrics but only accounts for 10 numbered criteria (1-10). This is actually correct as written - there are 10 criteria total. However, the phrasing "the 11 dimensions every review assesses" in guidelines.md line 33 conflicts with this. The number should be consistent across all files.
| | `guidelines.md` | File | Recommended | Yes (admin only) | This file – structure overview | | ||
| | `instructions.md` | File | **Required** | Yes (admin only) | Step‑by‑step review procedure | | ||
| | `deliverables.md` | File | **Required** | Yes (admin only) | Output requirements and severity definitions | | ||
| | `criteria.md` | File | **Required** | Yes (admin only) | Universal review criteria — the 11 dimensions every review assesses | |
There was a problem hiding this comment.
The guidelines.md file states "the 11 dimensions every review assesses" but criteria.md defines only 10 criteria. This inconsistency should be corrected - either update guidelines.md to say "10 criteria" or clarify what the 11th dimension is.
| | `criteria.md` | File | **Required** | Yes (admin only) | Universal review criteria — the 11 dimensions every review assesses | | |
| | `criteria.md` | File | **Required** | Yes (admin only) | Universal review criteria — the 10 criteria every review assesses | |
| | # | Criterion | Severity if violated | | ||
| |---|---|---| | ||
| | T‑4 | Business logic is separated from I/O (database reads, HTTP calls, file system) and can be exercised without a running server or database | high | | ||
| | T‑5 | Framework lifecycle hooks, middleware, and routing glue do not contain untested business logic | high | |
There was a problem hiding this comment.
In the testability rubric, criterion T-5 states "Framework lifecycle hooks, middleware, and routing glue do not contain untested business logic" with severity "high". However, this criterion is about the presence of tests, not about testability design. According to the scope note at the top, this rubric assesses whether code is designed to be testable, not whether tests are adequate. T-5 should either be reworded to focus on the testability design aspect (e.g., "business logic is not embedded in framework lifecycle hooks where it cannot be tested in isolation") or moved to a different rubric focused on test coverage.
| | T‑5 | Framework lifecycle hooks, middleware, and routing glue do not contain untested business logic | high | | |
| | T‑5 | Business logic is not embedded directly in framework lifecycle hooks, middleware, or routing glue; it resides in units that can be tested in isolation | high | |
| - `testability.md` — criterion 10: dependency injection, seams, isolation | ||
| - `observability.md` — criterion 11: logging, metrics, tracing, operability |
There was a problem hiding this comment.
The qa.md file states that testability.md covers "criterion 10" but according to criteria.md, testability is criterion 9 and observability is criterion 10. This mislabeling should be corrected to avoid confusion.
| - `testability.md` — criterion 10: dependency injection, seams, isolation | |
| - `observability.md` — criterion 11: logging, metrics, tracing, operability | |
| - `testability.md` — criterion 9: dependency injection, seams, isolation | |
| - `observability.md` — criterion 10: logging, metrics, tracing, operability |
No description provided.