chore: add copilot review instructions #407
Conversation
Add instructions for co-pilot for what to check for when doing reviews.
marcoscaceres
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive review guidelines for GitHub Copilot when reviewing pull requests to the Digital Credentials W3C specification. The instructions document establishes clear standards for editorial quality, ReSpec syntax, normative references, algorithm design, and various quality attributes (security, accessibility, i18n).
Key Changes:
- Added detailed Copilot review instructions covering editorial quality, ReSpec linking conventions, and W3C standards compliance
- Documented normative dependencies and browser interoperability requirements
- Established guidelines for accessibility, internationalization, security, and RFC 2119 keyword usage
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
marcoscaceres
changed the title
Plus additional fixes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/copilot-instructions.md
Outdated
| - `BREAKING CHANGE:` | ||
|
|
||
| For normative changes, **do not use any prefix**. |
There was a problem hiding this comment.
Contradiction in commit message conventions: BREAKING CHANGE: is listed under "Allowed prefixes (non-normative changes only)" but breaking changes are inherently normative (they affect WebIDL, algorithms, observable behavior, etc. as described in lines 320-324).
Consider either:
- Removing
BREAKING CHANGE:from the non-normative prefixes list and treating it as a special case, OR - Clarifying that
BREAKING CHANGE:is an exception to the "non-normative changes only" rule
The <BreakingChange> section (lines 316-325) makes it clear that breaking changes are normative, so the categorization here is misleading.
| - `BREAKING CHANGE:` | |
| For normative changes, **do not use any prefix**. | |
| For normative changes, **do not use any prefix**. | |
| Exception: For breaking normative changes, the commit message MUST begin with `BREAKING CHANGE:` (see <BreakingChange> below). |
Updated the document title and refined review instructions for clarity and consistency. Adjusted various sections to improve language precision and adherence to W3C standards.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 4. **WebIDL** is valid and matches algorithm behavior. | ||
| 5. **Security, privacy, a11y, and i18n** concerns are not obviously violated. | ||
| 6. **Commit messages** follow the project’s conventions. | ||
| If you can only do a few things on a PR, focus on these. |
There was a problem hiding this comment.
[nitpick] Potential inconsistency: Line 17 states "If you can only do a few things on a PR, focus on these" which suggests selective review, but the overall instructions appear to expect comprehensive review of all aspects. Consider clarifying whether this is guidance for resource-constrained scenarios or general prioritization advice.
| If you can only do a few things on a PR, focus on these. | |
| If you are unable to perform a comprehensive review of a PR, prioritize checking these key areas. |
|
|
||
| <RFCChecks> | ||
| - Check that MUST, MUST NOT, SHOULD, SHOULD NOT, MAY appear only in normative contexts. | ||
| - Conformance requirements using RFC2119 keywords only apply to a "user agent" implementing the specification (i.e., "A user agent MUST..." or where the context is obvious). There are no other conformance classes/objects (e.g., "Developers MUST..." would be wrong). |
There was a problem hiding this comment.
The phrase "Conformance requirements using RFC2119 keywords only apply to a 'user agent' implementing the specification" is unclear about the scope. The statement appears to be defining a single conformance class but the phrasing could be misinterpreted. Consider clarifying whether this means RFC2119 keywords should ONLY ever reference user agents, or if other contexts are acceptable.
| - Conformance requirements using RFC2119 keywords only apply to a "user agent" implementing the specification (i.e., "A user agent MUST..." or where the context is obvious). There are no other conformance classes/objects (e.g., "Developers MUST..." would be wrong). | |
| - In this specification, RFC2119 keywords (MUST, SHOULD, MAY, etc.) are only to be used in conformance requirements that apply to the "user agent" implementing the specification (e.g., "A user agent MUST..."). No other conformance classes or objects (such as "Developer") are defined, so using these keywords for other entities is incorrect. |
| Use caret syntax: | ||
| - `[^iframe^]` | ||
| - `[^input^]` |
There was a problem hiding this comment.
Inconsistent bracketing notation: Line 82 shows [^iframe/allow^] but this doesn't follow the pattern established by [^iframe^] and [^input^]. If "allow" is an attribute of the iframe element, the syntax should clarify whether a slash is the correct delimiter for element/attribute references in ReSpec.
| Use caret syntax: | |
| - `[^iframe^]` | |
| - `[^input^]` | |
| Use caret syntax for HTML elements: | |
| - `[^iframe^]` | |
| - `[^input^]` | |
| To reference an attribute of an element, use `[^element/attribute^]`: |
| - `fix stuff` | ||
| - `update spec` | ||
| - `should work now` | ||
| - `edit: tweaks` |
There was a problem hiding this comment.
The example "edit: tweaks" on line 327 uses "edit:" which isn't mentioned as a valid prefix in the Prefixes section. According to lines 293-299, non-normative changes should use either "chore:" or "editorial:". Consider replacing this example with one that uses the documented prefixes, or add "edit:" to the list of acceptable prefixes if it's intentionally valid.
| - `edit: tweaks` | |
| - `chore: tweaks` |
| - [credential-management] — Credential Management Level 1 | ||
| - [dom] — DOM Standard | ||
| - [html] — HTML Standard | ||
| - [infra] — Infra Standard | ||
| - [permissions] — Permissions API | ||
| - [permissions-policy] — Permissions Policy | ||
| - [RFC2119] | ||
| - [RFC8174] | ||
| - [vc-data-model] — Verifiable Credentials Data Model v2.0 | ||
| - [WebIDL] — Web IDL Standard |
There was a problem hiding this comment.
Inconsistent reference formatting in the NormativeReferences section. Lines 114-123 use lowercase bracket notation (e.g., [credential-management], [dom]) while earlier examples in the References section (lines 86-91) use uppercase notation (e.g., [[RFC2119]], [[HTML]]). The actual ReSpec syntax uses double brackets with typically uppercase shortnames. Consider updating these to match the standard [[shortname]] format for consistency.
| - [credential-management] — Credential Management Level 1 | |
| - [dom] — DOM Standard | |
| - [html] — HTML Standard | |
| - [infra] — Infra Standard | |
| - [permissions] — Permissions API | |
| - [permissions-policy] — Permissions Policy | |
| - [RFC2119] | |
| - [RFC8174] | |
| - [vc-data-model] — Verifiable Credentials Data Model v2.0 | |
| - [WebIDL] — Web IDL Standard | |
| - [[CREDENTIAL-MANAGEMENT]] — Credential Management Level 1 | |
| - [[DOM]] — DOM Standard | |
| - [[HTML]] — HTML Standard | |
| - [[INFRA]] — Infra Standard | |
| - [[PERMISSIONS]] — Permissions API | |
| - [[PERMISSIONS-POLICY]] — Permissions Policy | |
| - [[RFC2119]] | |
| - [[RFC8174]] | |
| - [[VC-DATA-MODEL]] — Verifiable Credentials Data Model v2.0 | |
| - [[WEBIDL]] — Web IDL Standard |
Add instructions for copilot for what to check for when doing reviews.
This pull request adds a comprehensive set of review instructions for Copilot to use when reviewing pull requests to the Digital Credentials Specification. The new
.github/copilot-instructions.mdfile establishes clear editorial, technical, and procedural guidelines to ensure high-quality, standards-compliant contributions. The instructions cover editorial quality, ReSpec conventions, normative references, algorithm requirements, accessibility, internationalization, security, conformance, and more.