Skip to content
Draft
91 changes: 91 additions & 0 deletions .github/copilot-instructions.md
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.
Comment thread
seanbudd marked this conversation as resolved.
* **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 NVDA guidance differs.
Comment thread
seanbudd marked this conversation as resolved.
Outdated
* **Indentation must use tabs**, not spaces.
* New/changed code must include **PEP 484 type hints** (prefer `X | Y` and `T | None`).
* Naming conventions:
Comment thread
SaschaCowley marked this conversation as resolved.
* functions/variables: `lowerCamelCase`
* classes: `UpperCamelCase`
* constants: `UPPER_SNAKE_CASE`
* scripts: `script_*`
* events: `event_*`
Comment thread
seanbudd marked this conversation as resolved.
* User-facing strings must be translatable via gettext (`_()`, `pgettext()`, `ngettext()` or `npgettext()`) and include an appropriate translators comment.
Comment thread
seanbudd marked this conversation as resolved.
Outdated
* 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.
Comment thread
seanbudd marked this conversation as resolved.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see discussion here: #19738 (comment)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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.

• 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.


## 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`)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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?

  • 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.

Suggested change
* 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.

* 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.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* 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.

Copy link
Copy Markdown
Collaborator

@XLTechie XLTechie Mar 11, 2026

Choose a reason for hiding this comment

The 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:

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.


## 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 a speech passed to a synthesizer.
Comment thread
seanbudd marked this conversation as resolved.
Outdated
* 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
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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?
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.

* 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.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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?

• 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.

* 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.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.

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.
Comment thread
seanbudd marked this conversation as resolved.

Comment thread
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.
Comment thread
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.
Loading