ci(copilot): add Copilot custom instructions for public-disclosure review - #323
ci(copilot): add Copilot custom instructions for public-disclosure review#323vivekkhandelwal1 wants to merge 3 commits into
Conversation
…view Copilot code review already runs on every PR and push here (the "CoPilot Review" repository ruleset), but the repo ships no custom instructions, so Copilot reviews each diff with no knowledge that this repository is the public half of a public/private split. Its own review footer has been asking for these files. PR #285 is the case this is meant to catch: an internal design doc and a contributor-specific demo guide, including a lab machine hostname, reached public main and had to be deleted after the fact. Reverting does not unpublish anything, so the check has to happen before merge. - .github/copilot-instructions.md: repository-wide. States the public/private boundary and that the plugin entry-point group means customer-specific material is never required here, then enumerates six categories to flag. Mirrors aorta-internal's CLAUDE.md rule 4 onto the public side. - .github/instructions/docs.instructions.md: scoped to docs/** and READMEs via applyTo, since prose is where this has actually gone wrong. Audience test plus checks for example commands, pasted output, cross-links, and deletions that leave dangling references. Both files carry an explicit "do not add real names, hostnames, or internal URLs here" rule and an allow-list of things that are already public (product and arch names, public ROCm links, aorta-internal issue pointers, docs/plans/, built-in mitigation names, artifactory-cdn.amd.com URLs in docker/), because a reviewer that flags established practice trains people to ignore it. These instructions are advisory: Copilot posts comments and cannot block a merge, and it reads instructions from the PR head branch. Enforcement would need a required status check, which this PR does not add. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
Adds repository-specific Copilot review instructions focused on pre-merge public-disclosure review for ROCm/aorta, reflecting the public/private split with aorta-internal and enumerating concrete categories of content that must not land in the public repo.
Changes:
- Introduces repository-wide Copilot instructions enumerating disclosure categories to flag and a short “how to raise a finding” rubric.
- Adds doc-scoped instructions (via
applyTo) to apply the disclosure categories more rigorously to prose-heavy files likedocs/**andREADME.md.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
.github/copilot-instructions.md |
Adds repo-wide Copilot custom instructions focused on preventing customer/internal material from landing in the public repo. |
.github/instructions/docs.instructions.md |
Adds prose-focused disclosure-review checks and guidance scoped to documentation and READMEs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - **Reproducible by anyone** -- flag nothing. Setup guides, CLI walkthroughs, | ||
| schema references, and design notes for published features are the point of | ||
| `docs/`. |
…checks Copilot's own review of this PR caught it: the "reproducible by anyone" bullet said "flag nothing", which reads as a blanket waiver on the whole page rather than on its audience. A broadly useful doc can still leak a hostname, an absolute path, or an internal image tag in one example, so that wording would have suppressed exactly the findings the file exists to produce. Scope the audience test to whether the page belongs in a public repository, and say explicitly that it never waives the category checks on the contents. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/instructions/docs.instructions.md:12
- The PR description says both instruction files include an explicit reminder that the instructions themselves are public, but this docs-scoped file currently does not. Since this file is likely to be edited when adding examples, it’s worth repeating the “this file is public too” guidance here (or update the PR description if you intentionally want it only in the repo-wide instructions).
Narrative files are where private material has actually reached public `main` in
this repository, because prose carries the incidental detail that code does not:
the machine someone ran it on, the internal document that explains why, the
customer the investigation was really about. Apply the categories in
`.github/copilot-instructions.md`, and additionally check the following.
…structions Second catch from Copilot's review of this PR: the rule only lived in the repo-wide instructions, but the docs-scoped file is the one that will be edited when someone adds a worked example of a check -- which is exactly the moment a real hostname or customer name gets pasted in to illustrate the point. Co-authored-by: Cursor <cursoragent@cursor.com>
amd-vivekag
left a comment
There was a problem hiding this comment.
Review summary
Verdict: ship — two optional Low suggestions; nothing blocking.
This is a careful, well-scoped guardrail. It reads as policy (not enforcement), the noise-floor discipline is genuinely good, and the allow-list is honest: I verified every listed identifier is already public in this repo — rocjitsu, mirage, and rocm-systems are referenced across src/, docs/, and recipes/, and artifactory-cdn.amd.com is already on main in five docker/ files — so listing them discloses nothing new. The file also correctly protects itself ("this file is public too", now mirrored into the docs-scoped file).
Copilot's inline comment is resolved correctly. The "reproducible by anyone → flag nothing" wording that Copilot flagged (and its suppressed "repeat the public-too rule in docs" note) are both fixed in 9042a3d / 5a4476f: the audience test now only decides page presence and explicitly says "Still review its individual lines."
Your open question (artifactory-cdn.amd.com): keeping the allow-list entry is the right call for this PR — it's established practice already on main, and re-flagging it would train reviewers to ignore the bot. But the underlying question (can a public user actually pull amdgpu-install-internal from that host? the package name and :8443/artifactory/ path suggest maybe not) is a real pre-existing reproducibility/disclosure question — worth a separate tracked issue rather than resolving it inside an instructions PR. So: allow-list stays, open a follow-up.
Findings: 0 High · 0 Medium · 2 Low (both optional)
| 6. **Internal-only planning material.** Design documents, roadmaps, escalation | ||
| post-mortems, and status reports written for an internal audience. Public | ||
| planning documents are welcome (see the "expected here" list below); the test | ||
| is the intended audience and the content, not the file's location. |
There was a problem hiding this comment.
[Low] Consider a first-class "secrets & credentials" category. Tokens, API keys, private keys, and passwords are the highest-impact thing that can hit a public repo, but they only appear here as a side-note under the docker/ allow-list ("newly added credentials, tokens, or auth headers ... are in scope"). Secret scanning is the real control, but it misses non-standard/internal token shapes, so having Copilot flag an obviously-pasted credential in any diff is cheap coverage.
Fix: add a short category (e.g. 7) — "Secrets & credentials: tokens, keys, passwords, auth headers — never in code, fixtures, logs, or docs; describe the shape, never the value."
| @@ -0,0 +1,65 @@ | |||
| --- | |||
| applyTo: "docs/**,**/README.md" | |||
There was a problem hiding this comment.
[Low, optional] applyTo: "docs/**,**/README.md" gives the prose-specific checks to docs/ and READMEs only. A future top-level CONTRIBUTING.md, SECURITY.md, or other non-README .md gets the repo-wide categories but not the audience test / pasted-output / dangling-link checks. Deliberate scoping is reasonable ("prose is where this has gone wrong"); flagging only so it's a conscious choice.
Fix (if you want broader prose coverage): applyTo: "docs/**,**/*.md".
Summary
Copilot code review already runs on every PR and push in this repository (the
CoPilot Reviewruleset, withreview_on_pushandreview_draft_pull_requestsboth enabled), but we ship no custom instructions. So Copilot reviews every diff
without knowing that
ROCm/aortais the public half of a public/private split.Its own review footer has been asking for these files on each PR: "Add Copilot
custom instructions for smarter, more guided reviews."
This adds them, aimed at one job: catching customer-specific and internal-only
material before it reaches public
main.The motivating case is #285, which deleted an internal design doc and a
contributor-specific demo guide (including a lab machine hostname) after they
had already landed on
main. Reverting does not unpublish anything, so the checkhas to happen pre-merge.
aorta-internal'sCLAUDE.mdrule 4 already states thepolicy; nothing on the public side could see it until now.
.github/copilot-instructions.md— repository-wide. States the boundary,notes that the
aorta.workloadsentry-point group means customer-specificmaterial is never required here, then enumerates six categories to flag:
customer identity, customer workload material, real captured environments and
logs, internal infrastructure identifiers, non-public container images, and
internal-only planning material.
.github/instructions/docs.instructions.md— scoped todocs/**and**/README.mdviaapplyTo, because prose is where this has actually gonewrong. Adds an audience test ("could a ROCm user outside AMD act on this?")
plus checks for example commands, pasted output, cross-links, and deletions
that leave dangling references.
Both files include an allow-list of things that are already public and must
not be flagged — AMD product and arch names, public ROCm links,
aorta-internalissue pointers,docs/plans/, built-in mitigation names, andthe existing
artifactory-cdn.amd.comURLs indocker/. A reviewer that flagsestablished practice teaches people to ignore it, so the noise floor matters as
much as the coverage.
Both also carry an explicit rule that the instructions files are public too,
so no one later adds real customer names or hostnames as a deny-list and
publishes the exact values the rules exist to protect.
Limits — please read before treating this as a control
This is a guardrail, not enforcement. Three specific gaps:
COMMENTED, never ablocking
REQUEST_CHANGES, and it is not a status check.deletes these files changes the rules of its own review.
main protectioncurrently hasrequired_approving_review_count: 0, withrequire_code_owner_review: falseandrequired_review_thread_resolution: false— a PR can merge with no humanapproval and with Copilot's comments unread.
Follow-ups worth deciding on separately, not included here:
leak-scanworkflow as a required status check — the only layer thatactually prevents a merge. Note the deny-list of customer names cannot live in
plaintext in a public repo; it would need an Actions variable or org-level
secret scanning custom patterns.
(scanning itself is on). Enabling it rejects content at
git push, before a PRexists.
required_approving_review_countto 1 and/or enabling CODEOWNERSreview, so a human signs off on public-facing diffs.
Open question for reviewers
docker/Dockerfiles fetchamdgpu-install-internalpackages fromartifactory-cdn.amd.com. That predates this PR and is onmain, so theinstructions explicitly tell Copilot not to raise it as a new finding. Flagging
it here in case that was never a deliberate decision — if it should be treated as
in-scope, the allow-list entry should come out.
Test plan
No code changes; nothing executable is added.
pre-commitpasses on both files (trailing-whitespace, end-of-file-fixer, check-yaml); full CI green.applyTofrontmatter parses as valid YAML.aorta-internalsplit — information available only from these files — and the "Add Copilot custom instructions" footer nag is gone from the latest review.9042a3d— the "reproducible by anyone" bullet said "flag nothing", which read as a blanket waiver on a whole page rather than on its audience. That would have suppressed exactly the findings this file exists to produce. The audience test is now explicitly scoped to whether a page belongs in a public repo, and never waives the line-level category checks.5a4476f— the "this file is public too" rule lived only in the repo-wide file, while the docs-scoped file is the one that will be edited to add a worked example of a check. That is precisely when a real hostname gets pasted in to illustrate the point, so the rule is now repeated there.