Rework agent code review guidance - #31969
Conversation
Add path-scoped C API instructions and a reusable review skill, with routing for GitHub Copilot and AGENTS.md-aware local agents. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Cover all append-only public API structs, release versioning, and C++ wrapper expectations. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Explicitly prohibit breaking changes to shipped API structs and describe new API functions without ambiguous terminology. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Reserve comments for non-obvious rationale, invariants, constraints, and subtle behavior rather than implementation history or narration. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Limit the tooling exclusion to formatting issues so reviewers still report substantive repository convention violations. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Explain where repository-wide, path-scoped, review, and domain-specific agent guidance belongs. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Present AGENTS.md as repository-wide guidance and use neutral terminology throughout the maintainer guide. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Include actionable clarity and maintainability concerns and generalize reporting requirements beyond correctness defects. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Direct Copilot surfaces to read and follow the repository-wide AGENTS.md guidance. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Hariharan Seshadri (hariharans29)
left a comment
There was a problem hiding this comment.
Overall LGTM
There was a problem hiding this comment.
Pull request overview
Reorganizes agent guidance into extensible repository-wide, path-scoped, and review-specific layers. No actionable findings found.
Changes:
- Adds guidance for extending agent instructions.
- Introduces a code-review skill and C API-specific instructions.
- Simplifies GitHub Copilot instructions to reference canonical guidance.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
docs/Agent_Coding_Guidance.md |
Documents guidance layers and extension practices. |
AGENTS.md |
Adds path-scoped review and comment guidance. |
.github/instructions/c-api.instructions.md |
Defines C API compatibility and versioning rules. |
.github/copilot-instructions.md |
Redirects GitHub Copilot to canonical instructions. |
.agents/skills/code-review/SKILL.md |
Adds the standard diff-review workflow. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Require complete Doxygen documentation and a since-version tag for new public C API members. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Add a dedicated agent skills section, call out the code-review skill, and remove repeated directory references. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Prefer separate inline findings when supported and make overall review summaries optional. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/skills/code-review/SKILL.md:3
- Both
.github/skills/and.agents/skills/are Copilot skill discovery roots, so this introduces two skills with the samecode-reviewname and description. Duplicate skills are indexed independently with no defined precedence; Copilot may expose/load both or activate this forwarding stub instead of the canonical workflow. Keep a single discoverable skill (the canonical.agents/skills/code-review/SKILL.mdis already in a supported location) and remove this adapter.
name: code-review
description: "Review ONNX Runtime pull requests, branches, commits, patches, and working-tree changes for actionable findings and missing tests."
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (3)
docs/Agent_Coding_Guidance.md:14
- The “Guidance Layers” table omits the
.github/skills/compatibility layer that this PR introduces. Since.github/skills/README.mdstates it’s needed for GitHub.com Copilot skill activation, listing it here will prevent maintainers from overlooking or removing it as “non-canonical”.
| Location | Purpose |
|---|---|
| [`AGENTS.md`](../AGENTS.md) | Repository-wide guidance. |
| [`.github/copilot-instructions.md`](../.github/copilot-instructions.md) | Thin GitHub Copilot adapter that points to canonical guidance. |
| [`.github/instructions/`](../.github/instructions/) | Guidance for agents working with specific paths. |
| [`.agents/skills/code-review/SKILL.md`](../.agents/skills/code-review/SKILL.md) | Generic diff-driven review workflow and finding format. |
| [`.agents/skills/`](../.agents/skills/) | Deeper workflows and knowledge for particular tasks or subsystems. |
.github/instructions/c-api.instructions.md:4
- The
applyToscope only matches the public headers, but this guidance explicitly references implementation updates inonnxruntime/core/session/onnxruntime_c_api.ccand related initializer .cc files. If a PR changes those .cc files without touching the headers, this instruction won’t load, reintroducing the exact review confusion this guidance is meant to prevent.
---
description: "C API implementation and review guidance for public C API updates."
applyTo: "include/onnxruntime/core/session/onnxruntime_c_api.h,include/onnxruntime/core/session/onnxruntime_ep_c_api.h"
---
.github/skills/code-review/SKILL.md:6
- The link text uses Windows-style backslashes (
.agents\skills\...), which can render oddly in Markdown and makes it harder to copy/paste as a repo path. Using forward slashes in the displayed path keeps it consistent with the link target and other docs.
Read the canonical code-review skill file at [.agents\skills\code-review\SKILL.md](../../../.agents/skills/code-review/SKILL.md).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/instructions/c-api.instructions.md:4
- The
applyToscope only matches the C API headers, but the guidance here also targets the implementation sites that usually change alongside them (e.g.,onnxruntime/core/session/onnxruntime_c_api.ccandplugin_ep/ep_api.cc). If an agent edits those.ccfiles without touching the headers, this instruction won’t be discovered, increasing the chance of incorrect version-table or ABI changes.
---
description: "C API implementation and review guidance for public C API updates."
applyTo: "include/onnxruntime/core/session/onnxruntime_c_api.h,include/onnxruntime/core/session/onnxruntime_ep_c_api.h"
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/instructions/c-api.instructions.md:3
applyToonly matches the public headers, but this guidance also covers the corresponding C API initializer/implementation files (e.g., appending toort_api_1_to_Ninonnxruntime_c_api.ccand updating companion API initializers). As written, edits to those.ccfiles without touching the headers won’t pick up these instructions; consider expandingapplyToto include the key implementation files so the guidance reliably applies to “C API implementation and review” changes.
applyTo: "include/onnxruntime/core/session/onnxruntime_c_api.h,include/onnxruntime/core/session/onnxruntime_ep_c_api.h"
### Description <!-- Describe your changes. --> Move `ort-release-notes` skill to new .github/skills location. ### Motivation and Context <!-- - Why is this change required? What problem does it solve? - If it fixes an open issue, please link to the issue here. --> Missed this skill in the migration in #31969
Description
Try to organize agent guidance so it is more extensible. E.g., add path-specific instructions, code review skill.
Also moved skills to
.github/skills. Added pointer to this location in AGENTS.md.Motivation and Context
Goal is to improve agent code reviewing by providing guidance to follow.