Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a .github/copilot-instructions.md file to guide GitHub Copilot's automated pull request reviews for the NVDA codebase. The goal is to improve the quality and relevance of Copilot's review feedback by codifying NVDA-specific review goals, Python coding standards, PR checklist expectations, security checks, and comment style guidelines.
Changes:
- Adds a new
.github/copilot-instructions.mdfile instructing Copilot on NVDA-specific review priorities, Python style conventions, PR checklist expectations, security checks, and preferred comment style.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Are you sure that the Copilot agent can read the files mentioned in the instructions? From what I know, the agent will only be able to read files that are touched by the pull request itself and not others. For this, we have to include the content of the needed files in the instructions, or a summary of them, including the most important information, or direct links, assuming the agent has the ability to browse context links, which is not always the case. |
|
@makhlwf - the PR includes a summary of the most important information. My understanding is the agent does gain wider context from your repo in general training, but the reviews themselves only check necessary related files. The agent does have to know about the rest of your repo to perform reviews already. |
seanbudd
added
the
conceptApproved
Co-authored-by: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com>
Co-authored-by: gerald-hartig <153188145+gerald-hartig@users.noreply.github.com>
Co-authored-by: Luke Davis <8139760+XLTechie@users.noreply.github.com>
Co-authored-by: Luke Davis <8139760+XLTechie@users.noreply.github.com>
XLTechie
left a comment
XLTechie
left a comment
There was a problem hiding this comment.
I am wondering if you guys have read this GitHub blog entry on this feature? There are a few things being done in here, that at least seem to be a bit outside what this article calls for.
| * All public functions, classes, and methods must have Sphinx-style docstrings (without type declarations in docstrings). | ||
| Most internal functions, classes, and methods should also have docstrings, except where their purpose is clear from their name or code. | ||
| * Flag unnecessary import-time side effects and new module-level globals where avoidable. | ||
| * Prefer inclusive language and avoid terms disallowed by NVDA guidelines. |
There was a problem hiding this comment.
Shouldn't "NVDA guidelines" be replaced with the path to the file containing them, rather than assuming it will naturally know which guidelines apply in this situation?
There was a problem hiding this comment.
Ah yes, I recall that one, though didn't realize the context.
All that aside, I'm seriously asking what makes us think it's going to read those referenced files at all? Not sure the whole "incorporated by reference" thing works here.
• Including external links. Copilot won’t follow them. You should copy relevant info into your instructions files instead.
No idea if they mean web links or things it recognizes as file paths, but I've not seen any clarity that would suggest that file links in the repo work any better than web links.
| * speech, braille, low vision | ||
| * browser differences where applicable | ||
| * localization implications | ||
| * API compatibility with add-ons is preserved unless explicitly intended and documented. |
There was a problem hiding this comment.
| * API compatibility with add-ons is preserved unless explicitly intended and documented. | |
| * API compatibility with add-ons is preserved, unless breaking API compatibility is explicitly intended and documented. |
| ## Architecture / performance checks | ||
|
|
||
| * Verify thread safety. | ||
| GUI changes, core state mutations and most COM/UI interactions must happen on the main thread. |
There was a problem hiding this comment.
"most"? How is it to know which are part of "most"?
Especially since it won't remember usages between PRs. At least, I hope it's not using PRs as training data.
| * Check for XSS e.g. from translators via translatable strings | ||
| * IPC and injected C++ code (`NVDAHelper`): | ||
| * Ensure data sent via RPC or IPC from injected processes to the main NVDA process is strictly validated for length and type. | ||
| * In C++ code, flag unsafe string handling, missing bounds checks or improper buffer allocations. |
There was a problem hiding this comment.
Are you guys aware of this, from the blog entry I will link at the top of this review?
• Place language-specific rules in .instructions.md files and then use the applyTo frontmatter property to target specific languages (e.g., applyTo: **/.py or applyTo: documentation/*.md).
My reading of that, is that preferably, such C-specific or Python-specific instructions, aught to be in separate files, attached as described.
| DEBUG level logging may include sensitive information such as a speech passed to a synthesizer. | ||
| * Untrusted input & web parsing: | ||
| * Validate that parsing of external structures (HTML, ARIA attributes, UIA/IA2 properties) handles malformed, excessively long or deeply nested inputs safely without causing infinite loops or memory crashes. | ||
| * Check for XSS e.g. from translators via translatable strings |
There was a problem hiding this comment.
That instruction would seem to need more elaboration. We know what all of those concepts mean in context. Does it?
In particular, I'm wondering if it understands how translators work, where/how they can be expected to inject their input, and to what lengths it should go to insure sanitization.
Co-authored-by: Luke Davis <8139760+XLTechie@users.noreply.github.com>
| * constants: `UPPER_SNAKE_CASE` | ||
| * scripts: `script_*` | ||
| * events: `event_*` | ||
| * User-facing strings must be translated via one of the gettext functions (`_()`, `pgettext()`, `ngettext()` or `npgettext()`); and must include an appropriate translators comment of the form "# Translators: comment.". |
There was a problem hiding this comment.
Instead of how I suggested that example, the doc I pointed to earlier, mentions providing examples with a code block. Should we therefore use an actual example here?, Something like:
```py
ui.message(_(
# Translators: A demonstration translatable string.
"This is a string being spoken with ui.message, and "
"is being translated using the _() function."
))
```
| Ensure the PR content supports NVDA’s review checklist (`.github/PULL_REQUEST_TEMPLATE.md`): | ||
|
|
||
| * Documentation impact considered: | ||
| * changelog entry when required (`user_docs/en/changes.md`) |
There was a problem hiding this comment.
Is it smart enough to know when to incorporate an external file as instructions (of which it can only take 1,000 lines before it gets confused), or the same format being used just to reference a file that it shouldn't read?
- If it does try to read the changelog, it's going to get deeply confused to the point of being useless, at least according to that blog post.
- If, on the other hand, it ignores such external references (suggested in the article, with the reason possibly explained by the syntax of this line), than all the external references may not be doing what you think.
There has to be some way to confirm this, apart from the ambiguous statement in that blog.
| * changelog entry when required (`user_docs/en/changes.md`) | |
| * changelog entry when required (the changelog is contained in the file "user_docs/en/changes.md") |
No idea if that would make a difference, but at least it's a different format than the "incorporate by reference" files.
I'm also concerned that "when required" is ambiguous, especially if it doesn't read in external files as I believe.
| * speech, braille, low vision | ||
| * browser differences where applicable | ||
| * localization implications | ||
| * API compatibility with add-ons is preserved unless explicitly intended and documented. |
There was a problem hiding this comment.
The reason I commented on this before, is that the grammar as used, literally means:
API compatibility with add-ons is preserved unless API compatibility is explicitly intended and documented.
It is supposed to mean that the compatibility should be preserved, unless incompatibility is intended (how does it know?) and documented. But it doesn't say that. It never indicates what the alternative to API compatibility is, which should probably lead the system to make the second subject agree with the first, which produces a very weird instruction.
However this is phrased, I would drop the word "intended". The AI has no idea if it was intended. Documentation is the only deciding factor.
Link to issue number:
None
Summary of the issue:
CoPilot PR reviews can be improved by writing a
.github/copilot-instructions.mdfile.This can minimize the work of NV Access, by automating some common review feedback we create.
Description of user facing changes:
None
Description of developer facing changes:
Improved CoPilot review instruction
Description of development approach:
Used CoPilot to generate, edited
Testing strategy:
None
Known issues with pull request:
None
Code Review Checklist: