ART transport core - #20386
Draft
SaschaCowley wants to merge 5 commits into
Draft
Conversation
Member
Author
|
Converted to draft pending opening an implementation roadmap |
SaschaCowley
marked this pull request as ready for review
July 17, 2026 01:30
seanbudd
reviewed
Jul 23, 2026
| :param localService: The service to expose to the add-on, defaults to ``None``. | ||
| :param name: Name of this connection, defaults to "unknown". | ||
| """ | ||
| self._name = name |
Member
There was a problem hiding this comment.
Is the name of the connection just used in internal logging?
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces the initial, feature-agnostic IPC transport layer for NVDA’s Add-on Runtime (ART), providing a restricted rpyc-over-pipes foundation (Connection/Service/Proxy) plus unit tests to validate core cross-boundary behaviors.
Changes:
- Added
_art.transportwith a restrictive rpyc protocol configuration and required global vinegar patching for exception deserialization. - Implemented
Servicelifecycle management (exposed-method allowlisting, termination, dependent connection/service teardown) andConnectionwrapper with background serving loops. - Added a
Proxybase and unit tests covering exposed calls, bidirectional callbacks, blocked non-exposed access, termination behavior, and dependent-service lifetime.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_art/init.py | Establishes the new ART unit test package. |
| tests/unit/test_art/test_artTransport.py | Adds unit coverage for the new transport primitives over an in-process pipe stream. |
| source/_art/init.py | Introduces the new _art package namespace for ART. |
| source/_art/transport/init.py | Exposes the transport public surface (PROTOCOL_CONFIG, Connection, Proxy, Service). |
| source/_art/transport/config.py | Defines security-restrictive rpyc protocol config and applies required global rpyc patches. |
| source/_art/transport/connection.py | Implements a bidirectional connection wrapper over an rpyc Stream, with serving loops and safe close semantics. |
| source/_art/transport/proxy.py | Provides a local-side base proxy that owns and closes dependent connections. |
| source/_art/transport/service.py | Adds a base service with explicit exposure, termination discipline, and dependent resource lifetime management. |
Comments suppressed due to low confidence (1)
source/_art/transport/proxy.py:41
- Docstring parameter name typo:
:param con:should be:param conn:to match the argument name.
def _holdConnection(self, conn: Connection) -> None:
"""Keep ``conn`` alive for at least as long as this proxy.
:param con: Connection to keep alive.
"""
Comment on lines
+10
to
+22
| from typing import TYPE_CHECKING | ||
|
|
||
| from rpyc.core.stream import Stream | ||
|
|
||
| from logHandler import log | ||
|
|
||
| if TYPE_CHECKING: | ||
| from .connection import Connection | ||
| from .service import Service | ||
|
|
||
|
|
||
| class Proxy[Service_t: Service]: | ||
| """Base class for the local-side wrapper around a remote :class:`.service.Service`. |
Comment on lines
+86
to
+87
| with self.assertRaises(Exception): | ||
| self.remote.echo("dead") |
Comment on lines
+23
to
+39
| #: ``allow_getattr`` is intentionally left at the rpyc default (``True``) because exposed methods are reached via attribute access on the netref; | ||
| #: with public, safe and all attribute access disabled, only the ``exposed`` allowlist remains reachable. | ||
| #: | ||
| #: .. warning:: | ||
| #: None of the security-critical flags below should be loosened without a security review.. | ||
| PROTOCOL_CONFIG: dict[str, object] = { | ||
| # Only explicitly exposed methods may cross the boundary. | ||
| "allow_public_attrs": False, | ||
| "allow_safe_attrs": False, | ||
| "allow_all_attrs": False, | ||
| "allow_setattr": False, | ||
| "allow_delattr": False, | ||
| # pickle would permit arbitrary code execution during deserialization. | ||
| "allow_pickle": False, | ||
| # A raising handler must not tear down the whole channel. | ||
| "close_catchall": True, | ||
| } |
Comment on lines
+66
to
+69
| """The service exposed by the add-on. | ||
|
|
||
| :raises RuntimeError: If the connection is closed. | ||
| """ |
seanbudd
marked this pull request as draft
July 24, 2026 03:43
Co-authored-by: Sean Budd <sean@nvaccess.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link to issue number:
None
Summary of the issue:
NVDA needs an out-of-process, sandboxed add-on runtime (ART) to help protect against malicious and/or buggy add-ons. Before any feature work can be built, ART needs a generic, feature-agnostic transport layer to carry calls across the NVDA—add-on process boundary.
Description of user facing changes:
None, just plumbing/architecture at this point.
Description of developer facing changes:
New
_artpackage and_art.transportsubpackage, which provide the rpyc-over-pipes core that ART will use for IPC:Connection: wraps a single bidirectional rpyc connection over aStream. Exposes a localServiceto the peer and reaches the peer's root service viaremoteService. Serves peer requests either in the calling thread (eventLoop) or a background thread (bgEventLoop).Service: base class for objects exposed across the boundary. Methods are opted-in with@Service.exposed. Owns the lifecycle of dependent connections it opens and dependant services it returns;terminate()cascades teardown to both.Proxy: base class for the local-side wrapper that re-presents a remote service through NVDA's normal interface. Usable as a mixin; closes any held connections on destruction.PROTOCOL_CONFIG: the shared, deliberately restrictive rpyc protocol config.Description of development approach:
Harvested the rpyc-over-pipes transport and Service/Proxy model from the synth driver host (
_bridge), but rebuilt it feature-generic from the start (nothing synth-specific) and added the explicit lifecycle/teardown discipline from the pyro5-based ART prototype.Notes:
_art.transport.config.PROTOCOL_CONFIGsetsallow_public_attrs,allow_safe_attrs,allow_all_attrs,allow_setattr,allow_delattrandallow_pickletoFalse, so only explicitly exposed attributes/methods are available to add-ons, and remote code execution via deserialization is prevented.allow_getattrisTruebecause exposed methods are themselves reached via attribute access; with everything else locked down theexposedallowlist is all that remains reachable. Changing these settings affects the security of ART._art.transportsetsrpyc.core.vinegar.exceptions_module = builtins. This is necessary because NVDA has its own levelexceptionsmodule, which rpyc would otherwise pick up overbuiltins, breaking remote exception deserialization._bridgepatches this on the host side, but it's done globally here so it applies to both sides of the connection. The patch is idempotent.Testing strategy:
tests/unit/test_art/test_artTransport.pyexercises the transport over an in-processPipeStreamduplex pair:AttributeError);vinegarpatch);Known issues with pull request:
Code Review Checklist: