Npm plugin loading#242
Conversation
Accept object entries in the 'scans' input ({name, package, version}) and install/import them at runtime via npm install --ignore-scripts. Loading is gated to first-party allowlist. Includes unit tests.
There was a problem hiding this comment.
Pull request overview
This PR adds support for installing and loading curated first-party accessibility scanner plugins from NPM, driven by object entries in the scans input (alongside existing built-in and local plugins). This addresses consumers who want to use a first-party plugin without vendoring plugin source into their repo.
Changes:
- Extend
scansinput parsing to accept{name, package, version?}entries and forward requested NPM plugins into plugin loading. - Add an NPM plugin loader that installs packages at runtime and dynamically imports them.
- Document the new NPM plugin mechanism and update the action input description; add unit tests for the NPM loader and NPM-plugin loading path.
Show a summary per file
| File | Description |
|---|---|
| PLUGINS.md | Documents how to request and load first-party NPM-published plugins via scans. |
| .github/actions/find/action.yml | Expands scans input description to include object-form entries for NPM plugins. |
| .github/actions/find/src/scansContextProvider.ts | Parses scans entries into scan names + a list of requested NPM plugins. |
| .github/actions/find/src/pluginManager/types.ts | Introduces NpmPluginRequest type for NPM plugin requests. |
| .github/actions/find/src/pluginManager/npmPluginLoader.ts | Adds runtime npm install + dynamic import for NPM plugin modules. |
| .github/actions/find/src/pluginManager/index.ts | Loads curated first-party NPM plugins after built-in and local plugins, with validation/precedence rules. |
| .github/actions/find/src/findForUrl.ts | Passes parsed npmPlugins into plugin loading so NPM plugins can be installed/loaded for the scan run. |
| .github/actions/find/tests/npmPluginLoader.test.ts | Adds unit tests for NPM install flags and NPM plugin loading/skip behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 5
| } | ||
|
|
||
| // exported for mocking/testing. not for actual use | ||
| // export to be used for mocking/testing. not for actual use |
There was a problem hiding this comment.
OK, I see these "not for actual" use comments are increasing in number; are they still accurate?
There was a problem hiding this comment.
It was an existing convention I noticed, for functions that needed to be exported since it was used in testing files, but the export wouldn't actually be used anywhere in production. Technically its accurate but it can be misleading. Do we need the label here or should I clean it up?
There was a problem hiding this comment.
Technically its accurate but it can be misleading.
Feel free to do this in a followup pull request, but yeah, I'm not sure these comments are particularly useful here since they're not programmatically enforced + nothing crazy should happen if, for some reason, you import and use one of these functions individually. But cc @abdulahmad307 in the event I'm missing something.
If we just want to separate out exports which solely exist for testing, maybe we could move these function definitions into a separate file imported both here and in tests? 🤷♀️
|
Should've thought of this earlier, but cc @abdulahmad307 if you also wanted to give this PR a review |
Scanner-side fixes for https://github.com/github/accessibility/issues/10755.
Consumers request a first-party NPM plugin by passing an object in the
scansinput: