Update instructions for agents #1014
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| When performing a code review, check that: | ||
| - No unit tests were eliminated without a strong reason. | ||
| - Additional dependencies introduced have a strong justification. | ||
| - Changes are not likely to increase build times. | ||
| - Each file has a license header. | ||
| ``` | ||
| /* | ||
| * Copyright (c) Microsoft Corporation. | ||
| * Licensed under the MIT license. | ||
| */ | ||
| ``` | ||
|
|
||
| ## SemVer and API Compatibility | ||
|
|
||
| - The workspace obeys SemVer. Removing or changing public API signatures (functions, types, re-exports) is a breaking change and requires a major version bump or a deprecated compatibility shim. | ||
| - Re-exports are part of the public API surface — removing them is also a breaking change. | ||
| - If changing public behavior, explain migration impact in the PR description. | ||
|
|
||
| ## Error Handling | ||
|
|
||
| - Do not introduce `panic!` paths for recoverable errors — propagate with `Result` instead. | ||
|
Contributor
There was a problem hiding this comment. This probably deserves a mention/move to the top-level AGENTS.md |
||
| - Keep error types small. Avoid large enums/structs that blow up the stack; look for ways to reduce field sizes (e.g., compute derivable fields, use enums instead of `&'static str`). | ||
| - Prefer `ANNError::new(ANNErrorKind::…, e)` over the old `log_*`-style constructors, which force eager string formatting and double-log errors. | ||
|
Contributor
There was a problem hiding this comment. Nit: There is not |
||
| - When using `thiserror`, rely on `#[from]` for automatic `Error::source` chaining — do not format the inner error in the `#[error("…")]` display string. | ||
| - Include relevant context values (e.g., the kind, key, or dimension) in error messages for debuggability. | ||
|
|
||
| ## Documentation | ||
|
|
||
| - Doc comments and README examples must match actual API signatures and serialized shapes. Stale examples that fail to compile or deserialize are treated as bugs. | ||
| - Do not leave dead references to APIs that no longer exist. | ||
| - When changing a function signature or removing a parameter, update all doc comments that mention the old signature. | ||
|
Contributor
There was a problem hiding this comment. More suggestions on documentation:
|
||
|
|
||
| ## Constants and Assumptions | ||
|
|
||
| - Do not hardcode magic values — make them configurable with sensible defaults and document the rationale. | ||
| - If using `wrapping_add` or other wrapping arithmetic, justify why overflow is expected or acceptable. | ||
| - Add assertions for invariants that callers or maintainers would otherwise have to discover by reading the implementation. | ||
|
|
||
| ## SIMD and Platform Portability | ||
|
|
||
| - Do not assume specific SIMD lane widths (e.g., `f32s::LANES == 8`). Code must be correct on AVX2, AVX-512, and ARM/NEON. | ||
| - When touching architecture-specific intrinsics, verify cross-platform behavior per `diskann-wide/README.md`. | ||
|
|
||
| ## Testing | ||
|
|
||
| - Keep test helpers close to the code they exercise, typically in a `mod tests` at the bottom of the file or in an adjacent test module, guarded with `#[cfg(test)]`. | ||
| - Do not add tests for derived traits (`Clone`, `Debug`, `PartialEq`) or enums unless they have explicit behavior beyond the derive. | ||
| - Test edge cases like empty inputs (e.g., empty iterators) to lock in defined behavior and prevent divide-by-zero or NaN results. | ||
|
|
||
| ## Rayon and Parallelism | ||
|
|
||
| - Never use the global Rayon thread pool. Always execute parallel work within the provided `RayonThreadPool` or `RayonThreadPoolRef`. | ||
|
Contributor
There was a problem hiding this comment. This guidance is more for |
||
| - Preserve deadlock-avoidance intent when modifying nested parallel loops. Be aware that combining blocking synchronization (e.g., mutex acquisition) with Rayon work-stealing can cause deadlocks. | ||
|
|
||
| ## Unsafe Code and Safety | ||
|
|
||
| - Every `unsafe` block must have a `// SAFETY:` comment directly above it explaining why the operation is sound. This is enforced by the `undocumented_unsafe_blocks = "warn"` workspace lint. | ||
| - Safety comments must be specific and verifiable — state the concrete precondition that makes the operation safe (e.g., `// SAFETY: i + width <= len ensures this read is in-bounds`). Do not use vague justifications like `// SAFETY: this is safe`. | ||
| - Safety contracts on `unsafe fn` signatures must be internally consistent — if the documented precondition says `scratch.len() >= n`, ensure the implementation does not write beyond `n` elements (e.g., due to rounding up to a panel/block size). | ||
| - Prefer safe abstractions over raw `unsafe` when possible. Use `unsafe` only when there is a measurable performance benefit or when interfacing with FFI/intrinsics. | ||
| - For pointer arithmetic, prefer `offset_from` to express bounds rather than `wrapping_add` unless wrapping behavior is intentionally needed — and document why. | ||
| - When calling SIMD intrinsics or FFI, list the specific preconditions being satisfied (alignment, length, non-null, valid initialization). | ||
|
|
||
| ## Naming | ||
|
|
||
| - Use names that reflect the current architecture, not historical ones. Rename outdated terms when refactoring. | ||
| - Struct and type names should map clearly to their domain concepts for easier mental mapping. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can copilot instructions point to the existing AGENTS.md files where applicable? It looks like a lot of instructions are manually duplicated here, so we risk them getting out of sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1.
Do we really need
.github/copilot-instructions.md? if so, how is it semantically different from/AGENTS.md? Why can't we keep all agent-related instructions in/AGENTS.md?