-
-
Notifications
You must be signed in to change notification settings - Fork 769
add Copilot PR review instructions #19738
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
b7bf5a7
4e09892
fc06662
34a72d9
7bb947f
fd2c30c
13c82b8
f934ab5
d92a637
68e5661
8c6dcbf
dafec43
23b93b5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,91 @@ | ||||||
| # Copilot instructions for GitHub code reviews (NVDA) | ||||||
|
|
||||||
| Use these instructions when reviewing pull requests in this repository. | ||||||
|
|
||||||
| ## Review goals | ||||||
|
|
||||||
| * Prioritize correctness, avoiding regressions, positive accessibility impact, API compatibility, and security. | ||||||
| * Keep feedback specific and actionable; prefer concrete code suggestions over generic comments. | ||||||
| * Classify issues by severity: | ||||||
| * **Blocking**: likely bug, regression risk, security concern, API break, missing required tests/docs. | ||||||
| * **Non-blocking**: style consistency, readability, minor refactors. | ||||||
| * Avoid requesting unrelated refactors or broad redesigns outside PR scope. | ||||||
|
|
||||||
| ## NVDA Python style checks | ||||||
|
|
||||||
| When reviewing Python changes, verify alignment with `projectDocs/dev/codingStandards.md`: | ||||||
|
|
||||||
| * Follow PEP 8 unless guidance from the `codingStandards.md` file differs. | ||||||
| * **Indentation must use tabs**, not spaces. | ||||||
| * New/changed code must include **PEP 484 type hints** (prefer `X | Y` and `T | None`). | ||||||
| * Naming conventions: | ||||||
|
SaschaCowley marked this conversation as resolved.
|
||||||
| * functions/variables: `lowerCamelCase` | ||||||
| * classes: `UpperCamelCase` | ||||||
| * constants: `UPPER_SNAKE_CASE` | ||||||
| * scripts: `script_*` | ||||||
| * events: `event_*` | ||||||
|
seanbudd marked this conversation as resolved.
|
||||||
| * 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.". | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: |
||||||
| * 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. | ||||||
|
seanbudd marked this conversation as resolved.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see discussion here: #19738 (comment)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
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. |
||||||
|
|
||||||
| ## PR checklist expectations | ||||||
|
|
||||||
| 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`) | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
There has to be some way to confirm this, apart from the ambiguous statement in that blog.
Suggested change
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. |
||||||
| * user/developer docs updates where needed | ||||||
| * context-sensitive help for GUI options | ||||||
| * Testing strategy is clear and reproducible: | ||||||
| * unit/system/manual coverage discussed | ||||||
| * manual steps are concrete | ||||||
| * UX impact considered for: | ||||||
| * speech, braille, low vision | ||||||
| * browser differences where applicable | ||||||
| * localization implications | ||||||
| * API compatibility with add-ons is preserved unless explicitly intended and documented. | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason I commented on this before, is that the grammar as used, literally means:
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. |
||||||
|
|
||||||
| ## Security-specific review checks | ||||||
|
|
||||||
| NVDA operates with `UIAccess` privileges, injects code into other processes and handles untrusted data. | ||||||
| Scrutinize code for privilege escalation and data leaks. | ||||||
|
|
||||||
| * Secure mode, lock screens & installer limitations: | ||||||
| * Check lock-screen object handling safeguards (e.g., secure object filtering). | ||||||
| * Ensure `globalVars.appArgs.secure` is respected and blocked actions fail gracefully. | ||||||
| * Check `NVDAState.shouldWriteToDisk`; do not write to disk/config if running securely or from the launcher. | ||||||
| * Ensure no system or personal information is unintentionally exposed in secure screens or the lock screen. | ||||||
| * Subprocesses & file execution: | ||||||
| * Flag any use of `subprocess`, `os.system`, or `os.startfile`. | ||||||
| Because NVDA has UIAccess, these must use strictly sanitized arguments and absolute paths to prevent path/binary hijacking. | ||||||
| * Sensitive Data Logging: | ||||||
| * Ensure new logging statements that are INFO level or higher do not capture sensitive user data, particularly from `protected` or password text fields, API keys, or secure desktop states. | ||||||
| DEBUG level logging may include sensitive information such as the 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 | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That instruction would seem to need more elaboration. We know what all of those concepts mean in context. Does it? |
||||||
| * 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. | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you guys aware of this, from the blog entry I will link at the top of this review?
My reading of that, is that preferably, such C-specific or Python-specific instructions, aught to be in separate files, attached as described. |
||||||
| * Network / Updates: | ||||||
| * Any new HTTP/network requests must enforce secure connections (HTTPS) and validate server certificates except when updating certificates. | ||||||
|
|
||||||
| ## Architecture / performance checks | ||||||
|
|
||||||
| * Verify thread safety. | ||||||
| GUI changes, core state mutations and most COM/UI interactions must happen on the main thread. | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "most"? How is it to know which are part of "most"? |
||||||
| Ensure `wx.CallLater`, `wx.CallAfter`, `utils.schedule`, `core.callLater` or `queueHandler` are used when passing execution from background threads. | ||||||
| * Flag expensive operations (such as heavy computations, blocking I/O, complex loops) inside performance-critical hot paths like focus changes, key presses, or text iteration. | ||||||
| Watch for excessive COM calls (e.g. fetching properties individually inside a large loop instead of caching) and deep UIA tree walks on the main thread. | ||||||
| * For `ctypes` and COM interactions, ensure memory buffers, handles, and variants are safely freed to prevent memory leaks (e.g. using `ole32.CoTaskMemFree` or `kernel32.CloseHandle`). | ||||||
| * If an API change breaks compatibility, ensure it follows NVDA’s deprecation cycle (using `utils._deprecate`) and is noted in the API changelog as per `projectDocs/dev/deprecations.md`. | ||||||
| * For C++ changes, prioritise RAII, smart pointers for COM objects and lightweight execution inside injected hooks to prevent crashing target apps. | ||||||
|
seanbudd marked this conversation as resolved.
|
||||||
|
|
||||||
|
seanbudd marked this conversation as resolved.
|
||||||
| ## Comment style for Copilot reviews | ||||||
|
|
||||||
| * Focus comments on changed lines and clear user/developer impact. | ||||||
| * Explain **why** something is a problem and **what** an acceptable fix looks like. | ||||||
|
seanbudd marked this conversation as resolved.
|
||||||
| * If uncertain, ask a precise question instead of making a weak assertion. | ||||||
| * Acknowledge valid trade-offs; do not insist on preferences when standards are satisfied. | ||||||
Uh oh!
There was an error while loading. Please reload this page.