PR Review Skill by arkrishn94 · Pull Request #1318 · microsoft/DiskANN

Aditya Krishnan (arkrishn94) · 2026-08-10T22:41:41Z

This PR introduces a pull request review skill dedicated to this repo.

Why

There's two main reasons for why this is useful -

With coding agents becoming better, as we all have noticed, a significant amount of admin's bandwidth is being spent reviewing lengthy diffs. This level of commitment reduces the available time for admin to actually produce output.
Many PRs have recurring bugs/comments because feedback left on one author's PR is not necessarily easily visible to another. This wastes the time of reviewers since they need to repeatedly flag the same incorrect patterns, elongating the review cycle and wasting bandwidth.

Review Agent

This PR mainly introduces a skill (diskann-pr-review) for GitHub Copilot to use to review PR.
This is not meant to be a substitute for a comprehensive human review but hopes to at least catch the initial set of low-hanging feedback.

The PR introduces a skill captured in .github/skills/diskann-pr-review. The main flow for the review is in the SKILL.md and is accompanied with a file rules.md that is a repository for the kind of errors/rules we would want enforced by the review agent.

This file is meant to be a living document and is meant to evolve as we observe more patterns that can be codified and as we learn how effective this agent is.

The file was created using an agent: I got an agent to look at all the human feedback given to PRs in the last 3 months, triage them based on their importance and try to list down the repeated/codifiable offenses. I have then done a manual check of every rule in that file. Feedback and edits to this file are welcome and encouraged!

Integration with CI

One interesting bug in out GHCP CI integration that surfaced from this effort was that GHCP expects very specific filenames to pick up instructions and the agents md file. Hence the renaming of instructions.md -> copilot-instructions.md and agents.md -> AGENTS.md.

We still need to manually:

Set code review effort to "Balanced"
Confirm the custom-instructions toggle
Set rereview on push

Adds .github/skills/diskann-pr-review/, a review skill distilled from the last three months of review history (152 PRs, ~1,560 human review comments). SKILL.md carries the review workflow and a blocking checklist, rules.md is the detailed catalog of 62 rules, and MAINTENANCE.md documents how to refresh it against newer PRs. Adds .github/copilot-instructions.md, loaded by every Copilot surface, stating the rules that must hold even when the skill is not invoked: crate tiers, the three error-handling regimes, unsafe documentation, the 90% patch coverage gate, and hygiene requirements. Removes .github/instructions.md, which sat at a path no tool reads. Its four rules (no silent test deletion, dependency justification, build times, license header) are preserved in the new files. Renames agents.md to AGENTS.md to match the name agent tooling looks for. No tracked file referenced the old spelling. Rules are identified by a stable slug rather than a number, so entries can be added and deleted without renumbering and references do not rot. Human contributors may ground a rule in maintainer judgement; an agent updating the file may only cite a review quote or a repo source, so anything it adds stays independently checkable.

…w-skill

Copilot

Pull request overview

This PR adds a repository-specific GitHub Copilot PR review skill (diskann-pr-review) plus supporting documentation (rules.md, AGENTS.md) and updates Copilot instruction filenames to match GitHub Copilot’s expected conventions.

Changes:

Introduces .github/skills/diskann-pr-review/ with a review workflow guide (SKILL.md) and a detailed rule catalog (rules.md).
Adds a repo-root AGENTS.md onboarding guide for coding agents.
Replaces the previous .github/instructions.md with .github/copilot-instructions.md for Copilot custom instructions.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
AGENTS.md	Adds agent onboarding / repo workflow guidance.
.github/skills/diskann-pr-review/SKILL.md	Defines the PR review skill workflow and blocking checklist.
.github/skills/diskann-pr-review/rules.md	Adds an evidence-backed rules catalog for reviews.
.github/instructions.md	Removes the old Copilot instructions filename.
.github/copilot-instructions.md	Adds Copilot custom instructions under the expected filename.

Suppressed comments (1)

.github/skills/diskann-pr-review/SKILL.md:67

This checklist item says license headers are required for "every new file", but the repository contains many non-Rust files without such headers. Narrowing this to Rust source files helps prevent noisy/incorrect review comments.

| 9 | License header present on every new file | `.github/copilot-instructions.md` |

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+
+## Hygiene
+
+- Every new file carries the license header:


Magdalen Dobson Manohar (magdalendobson) · 2026-08-11T14:48:37Z

+
+### license-header `BLOCK`
+
+**License header on every new file.**


This is mostly correct, but we would want a license on code in other languages if it was ever committed. What we don't want is licenses on data files, test inputs, or Markdown files

Codecov Comments Bot (codecov-commenter) · 2026-08-10T22:55:29Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.30%. Comparing base (027d834) to head (332f17b).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1318      +/-   ##
==========================================
+ Coverage   91.31%   92.30%   +0.98%     
==========================================
  Files         517      517              
  Lines       98518    98518              
==========================================
+ Hits        89965    90938     +973     
+ Misses       8553     7580     -973

Flag	Coverage Δ
miri	`92.30% <ø> (+0.98%)`	⬆️
unittests	`92.27% <ø> (+1.27%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 40 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Wei Wu (wuw92)

Thanks for contributing this skill, it helps lot to reduce review effort. I’d keep skill focused on semantic review:

Rely on CI and Lint for deterministic violations, such as coverage thresholds, missing SAFETY comments, and disallowed unwrap/expect/panic! calls.
Use the skill for higher-level issues involving soundness, API contracts, and architectural invariants.

Wei Wu (wuw92) · 2026-08-11T11:05:20Z

+
+## Review workflow
+
+### 1. Orient before reading the diff


Could this skill stay focused on DiskANN-specific review lenses, evidence requirements, and calibration? The mandatory fetch workflow seems to take over agent orchestration and may duplicate or conflict with context acquisition already handled by hosted Copilot review or other skills

Great point Wei, that makes sense.

Wei Wu (wuw92) · 2026-08-11T11:26:00Z

+
+These are the items that stop a merge. Verify each explicitly — do not assume.
+
+| # | Blocking check | Where it's enforced |


Could we separate CI-owned merge gates from agent-owned semantic review here? “These items stop a merge” and “verify each explicitly” ask the reviewer to duplicate Codecov/Clippy and imply merge authority that hosted Copilot review does not have

Magdalen Dobson Manohar (magdalendobson)

Thanks for contributing this!

I think my main comment is that we could use some crate-specific standards in addition to the general standards laid out here. For example, we're okay with untested code in diskann-tools. In diskann-benchmark, new algorithm submissions require example .json files. Within test-data, large files must use git lfs (and don't put data files anywhere else). We could have a section with special instructions for each crate, and add to them over time even if they're sparse at first.

Okay also.... would it be feasible to ask the reviewing agent to look at human-written comments and suggest new additions to the reviewing instructions if it finds any during review? This would be interesting to experiment with and could help things evolve over time.

Magdalen Dobson Manohar (magdalendobson) · 2026-08-11T14:47:05Z

+Also complete the checklist in [PULL_REQUEST_TEMPLATE.md](../../PULL_REQUEST_TEMPLATE.md): release-
+note-worthy title, new dependencies, API modifications, backward compatibility, docs impact.
+
+### rfc-for-significant-change `MAJOR`


I wouldn't say this one is a hard and fast rule. Maybe better to leave this to human judgment.

Magdalen Dobson Manohar (magdalendobson) · 2026-08-11T14:48:37Z

+
+### license-header `BLOCK`
+
+**License header on every new file.**


This is mostly correct, but we would want a license on code in other languages if it was ever committed. What we don't want is licenses on data files, test inputs, or Markdown files

Magdalen Dobson Manohar (magdalendobson) · 2026-08-11T14:49:29Z

+- Reviewers state their stake and priority openly ("I have a vested interest in seeing something like
+  this merged ASAP. That said, there is a lot of work needed…"). Transparency about trade-offs beats
+  false neutrality.
+- Praise for good design choices is normal and expected.


Magdalen Dobson Manohar (magdalendobson) · 2026-08-11T14:54:20Z

+
+| # | Blocking check | Where it's enforced |
+|---|---|---|
+| 1 | **Patch coverage ≥ 90%** on changed lines | `.codecov.yml` (`informational: false`) |


This isn't true in every crate, is it?

Mark Hildebrand (hildebrandmw)

Did one pass against some of the earlier rules.

Mark Hildebrand (hildebrandmw) · 2026-08-11T18:17:53Z

+
+**Struct fields stay private; invariants live in constructors.**
+Public fields silently void every check the constructor performs, and the invariant can never be
+reinstated without a breaking change.


There's a little nuance here: types that are just data aggregates with no invariants can have public members. Maybe more nuanced: "fields with invariants must be private".

Mark Hildebrand (hildebrandmw) · 2026-08-11T18:29:27Z

+
+### no-duplicate-abstraction `MAJOR`
+
+**Don't duplicate an abstraction across index types.**


A corollary here: often sharing properly require refactoring in order to work. This is often the best time to do the refactor because hopefully the additional requirements are clear.

Om the other hand: don't try to make something work just for the sake of sharing. If it's truly a bad fit, don't lean into it.

That said, taking the time to fortify/refactor something in a more generally useable direction will often pay off in the future.

Mark Hildebrand (hildebrandmw) · 2026-08-11T18:41:33Z

+
+**Question new top-level crates.**
+Prefer a feature-gated module inside an existing crate. Crate proliferation hurts discoverability
+and build times.


I'm not sure I completely agree: feature proliferation is not great either because it's harder to test (do we test all feature combos? Build the crate both with and without the feature etc). I think the diagram is more like:

Can the feature be added without a feature gate and without significantly impacting (1) transitive dependencies, (2) compile times, (3) API surface? If so, do that.

If a feature benefits greatly from existing internals and is cleanly additive, use a feature gate.

Make new crates as a last resort.

Additionally, internal tools (eg vectorset) that pull in a bunch of dependencies but that otherwise have minimal dependence on diskann-* can live in their own workspace.

Mark Hildebrand (hildebrandmw) · 2026-08-11T18:54:16Z

+### no-module-name-in-type `NIT`
+
+**Don't repeat the module name in the type name.**
+`flat::SearchStrategy`, not `flat::FlatSearchStrategy`.


There is some nuance here as this can be taken too far. However, if names get longer the deeper in the hierarchy you go, it's probably a sign to reevaluate.

Maybe something like "consider using module names as part of the identity of a type"?

Mark Hildebrand (hildebrandmw) · 2026-08-11T19:07:49Z

+
+### document-benign-races `MAJOR`
+
+**Document benign races explicitly, with the reasoning.**


Let's not bring up "benign races". They don't exist since unsynchronized concurrent access (with at least one mutator) is always UB.

Mark Hildebrand (hildebrandmw) · 2026-08-11T19:09:29Z

+
+### fail-at-load-time `MAJOR`
+
+**Fail at load/parse/match time, not mid-run.**


Maybe more generally: "fail as early as is reasonable". And an add-on: "use types to encode invariants that have already been checked".

Mark Hildebrand (hildebrandmw) · 2026-08-11T19:10:17Z

+### checked-conversions `MAJOR`
+
+**Prefer checked conversions to `as`.**
+Silent truncation is a recurring source of ID-space bugs.


Treat narrowing as conversions with great skepticism.

Mark Hildebrand (hildebrandmw) · 2026-08-11T20:54:27Z

+
+**Every `unsafe` block carries a `// SAFETY:` comment.**
+Enforced by the workspace clippy lint `undocumented_unsafe_blocks`. The comment must name the
+invariant that makes the operation sound, in the same units as the operation (element counts vs.


A few words on invariants:

Invariants can rely on known and documented properties of concrete types (e.g. slices don't lie about their length)

Invariants can rely on properties established locally (e.g. via assert! or other check)

Invariants may not rely on unchecked properties of an argument or between arguments of a safe function that only exist as part of the function contract. Either check the property, make the property inherent to the argument type, or make the function unsafe.

Invariants must not depend on generic arguments implementing safe traits responsibly. Assume pathological cases exist. For example:

T: Deref<Target = &[f32]>: Unsafe code cannot rely on the same slice being returned on every deref for a generic T.

ExactSizeIterator: Implementations can by incorrect and the trait explicitly documents this.

Either restructure the code to remove this dependence or make the necessary trait unsafe.

Mark Hildebrand (hildebrandmw) · 2026-08-11T20:55:30Z

+
+---
+
+## 3. Unsafe code


"Unsafe code should be exercised under Miri."

Mark Hildebrand (hildebrandmw) · 2026-08-11T21:02:47Z

+
+### watch-monomorphization `MAJOR`
+
+**Watch monomorphization and code bloat.**


Don't be afraid to use trait objects, especially in higher level code where the cost of dynamic dispatch is negligible.

Mark Hildebrand (hildebrandmw)

Another round from me. One thing to keep in mind is that redundant or overly long skills can dilute the context window. Items that are already in copilot-instructions hopefully don't need to be repeated. But I'm not an LLM expert.

Mark Hildebrand (hildebrandmw) · 2026-08-12T22:29:37Z

+are frequently negotiated in-thread, and a review that re-litigates a settled decision costs the
+author time and undermines the reviewer's credibility.
+
+```powershell


I suspect agents already know how to do this with the github MCP.

Mark Hildebrand (hildebrandmw) · 2026-08-12T22:41:28Z

+body itself (rule `patch-coverage`) and skim bot findings for genuine correctness bugs before
+discarding them.
+
+#1269 is the cautionary case. The module is named `views` while the PR description says `matrix`,


Can this be condensed? e.g. "#1269 is a cautionary case. A reviewer requested deferring a module rename to simplify the diff, leaving the PR description stale. Here, an AI reviewer should flag the description not the code."

Mark Hildebrand (hildebrandmw) · 2026-08-12T22:41:48Z

+
+## 9. Interaction norms
+
+### read-existing-review-threads `BLOCK`


Not necessarily blocking?

Mark Hildebrand (hildebrandmw) · 2026-08-12T22:45:19Z

+### rfc-for-significant-change `MAJOR`
+
+**RFC required for architecturally significant change.**
+Per [rfcs/README.md](../../../rfcs/README.md): new crates, cross-crate traits/abstractions, new


My (maybe hot) take is that RFCs without implementations are not very useful.

Mark Hildebrand (hildebrandmw) · 2026-08-12T22:49:10Z

+> "These are still being referenced in some internal tests… please double check they are indeed safe
+> to remove" — #1185, @hildebrandmw
+
+### no-build-time-regression `MAJOR`


Already covered in copilot-instructions?

Mark Hildebrand (hildebrandmw) · 2026-08-12T22:54:04Z

+
+### valid-intra-doc-links `NIT`
+
+**Keep intra-doc links valid.**


We should really turn this into a CI lint.

Mark Hildebrand (hildebrandmw) · 2026-08-12T22:56:41Z

+### no-dead-code-in-tests `MAJOR`
+
+**`#[allow(dead_code)]` in tests is a smell.**
+Either the helper is used (write the test) or it isn't (delete it).


Exceptions are allowed if the code is nontrivial and logically belongs with an API.

Mark Hildebrand (hildebrandmw) · 2026-08-12T22:58:18Z

+
+**Explicit non-goals.**
+Per AGENTS.md — **do not** request tests for derived traits (`Clone`, `Debug`, `PartialEq`) or for
+enums without explicit functionality. And duplication inside unit tests is acceptable:


Some code duplication. Too much compromises maintainability. Often it's worth investing in test infra.

Mark Hildebrand (hildebrandmw) · 2026-08-12T22:58:48Z

+
+### no-deleting-tests `BLOCK`
+
+**Don't delete tests without justification.**


Already in copilot-instructions?

Mark Hildebrand (hildebrandmw) · 2026-08-12T22:59:07Z

+
+### smoke-test-new-public-api `MAJOR`
+
+**New public methods get at least a smoke test.**


I feel like this is table stakes.

Aditya Krishnan (arkrishn94) added 3 commits August 10, 2026 17:31

Merge remote-tracking branch 'origin/main' into u/adkrishnan/pr-revie…

20c5353

…w-skill

remove maintenance

7ab6824

Aditya Krishnan (arkrishn94) requested review from a team and a lite review from Copilot August 10, 2026 22:41

Copilot started reviewing on behalf of Aditya Krishnan (arkrishn94) August 10, 2026 22:42 View session

Copilot AI reviewed Aug 10, 2026

View reviewed changes

Potential fix for pull request finding

332f17b

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Wei Wu (wuw92) reviewed Aug 11, 2026

View reviewed changes

Magdalen Dobson Manohar (magdalendobson) reviewed Aug 11, 2026

View reviewed changes

Mark Hildebrand (hildebrandmw) reviewed Aug 11, 2026

View reviewed changes

Mark Hildebrand (hildebrandmw) reviewed Aug 12, 2026

View reviewed changes


		### license-header `BLOCK`

		License header on every new file.


		These are the items that stop a merge. Verify each explicitly — do not assume.

		\| # \| Blocking check \| Where it's enforced \|


		### no-duplicate-abstraction `MAJOR`

		Don't duplicate an abstraction across index types.


		### document-benign-races `MAJOR`

		Document benign races explicitly, with the reasoning.


		### fail-at-load-time `MAJOR`

		Fail at load/parse/match time, not mid-run.


		### watch-monomorphization `MAJOR`

		Watch monomorphization and code bloat.


		## 9. Interaction norms

		### read-existing-review-threads `BLOCK`


		### valid-intra-doc-links `NIT`

		Keep intra-doc links valid.


		### no-deleting-tests `BLOCK`

		Don't delete tests without justification.


		### smoke-test-new-public-api `MAJOR`

		New public methods get at least a smoke test.

Uh oh!

Conversation

Aditya Krishnan (arkrishn94) commented Aug 10, 2026

Why

Review Agent

Integration with CI

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Codecov Comments Bot (codecov-commenter) commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Wei Wu (wuw92) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Magdalen Dobson Manohar (magdalendobson) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mark Hildebrand (hildebrandmw) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mark Hildebrand (hildebrandmw) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Codecov Comments Bot (codecov-commenter) commented Aug 10, 2026 •

edited

Loading