fix(plugins): a node:-prefixed builtin must disclose what the bare one does - #72
Merged
Merged
Conversation
…e does
The capability scanner detected `child_process` and not
`node:child_process`. They are the same module, and the filesystem
detectors two lines below already accepted both spellings — so the
inconsistency lived inside a single object literal, one rule written two
ways, which is precisely the kind of thing nothing notices.
It was not theoretical. A plugin built on this machine imports
`node:child_process` and shells out to python3. Its build reported:
Detected: filesystem, env-access
with no spawn-process. The two call-shape patterns did not cover it
either: execFile/spawn/exec only match when the first argument is a quoted
literal, and a plugin resolving its binary from a variable or process.env
never has one. That plugin declared spawn-process by hand, so the union
model granted it — but a plugin that stayed quiet would have shipped a
shell-out with no disclosure at the consent modal, and no cost to its
trust tier.
Scanning that same plugin before and after:
before: env-access, filesystem
after: env-access, filesystem, spawn-process
Adds a parity ratchet over every builtin the import detectors recognise:
both spellings must produce the same capability set. That test fails when
a new builtin is added in one form only, which is how this gap arrived.
25 tests; 5 of them fail without this change.
`net` has a method-shape detector but no import detector in EITHER
spelling — a missing module rather than a missing prefix. Left alone and
asserted, so it stays visible instead of silent.
…requires
backend/plugins/cli/build-plugin.js refuses to package a manifest without
a structured `permissions` block:
❌ manifest.json must contain structured permissions.capabilities and
permissions.domains arrays
The agnt-plugin-builder skill — the instructions an agent follows to build
a plugin from chat — contained the word "permissions" zero times. Its
manifest example omitted the block entirely, so a plugin built by
following the skill exactly fails at step 3, and the only way forward is
to read the build script.
Adds the block to the example, a rule stating it is required, a section
explaining the six capabilities and what `domains` means, the verbatim
error string so it can be searched for, and the draft-permissions.js
escape hatch. Notes that declared and detected need not match, because
effective permissions are the union — over-declaring is safe and is the
right call when the plugin does something a regex scan cannot see.
The capability table is derived from CAPABILITY_DETECTORS rather than
invented, and three contract tests now assert the skill keeps matching the
code: every detectable capability is documented, the failure message is
quoted as the build script actually emits it, and the example carries a
permissions block. Documentation drifts silently; these make it drift
loudly.
draft-permissions.js takes no arguments and writes one draft per dev
plugin — verified against the script rather than assumed, having first
documented it wrongly.
Mutation testing found them. Both strings occur twice in SKILL.md, both
assertions searched the whole document, so deleting the copy that matters
left the other one holding the test green.
- "shows a permissions block in the manifest example" passed with the
block deleted from the manifest example, because the explanatory
section further down still had one. Now extracts the first json block
under the manifest.json heading and asserts inside it.
- "quotes the build failure verbatim" passed with the quotation
paraphrased, because the pitfalls table row kept the phrase alive. Now
scopes to the fenced error block, flattens the wrapped lines, and
compares against the exact string build-plugin.js emits.
Mutants 5 and 7 were survivors before this and are killed after. Same 25
tests; the two that lied now fail when they should.
…n unbroken copy too The scoped assertion added in 03edc59 pins fidelity: the fenced block must quote build-plugin.js exactly. But that block wraps mid-sentence, so nobody pasting the error into a search box can find it. The contiguous phrase lives only in the pitfalls table, and paraphrasing it there was still invisible to the suite. Adds a deliberately unscoped check for the unbroken fragment, with the reason stated: the contract is that the message is findable, not that it is findable in one particular place.
There was a problem hiding this comment.
🟡 Changes recommended
The newly added parity/ratchet tests have correctness and portability issues (multi-scan directory contamination and non-portable import.meta.url pathname handling) that can allow false passes or fail on Windows.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR tightens AGNT’s plugin capability-disclosure pipeline by ensuring node:-prefixed builtins are detected the same as their bare equivalents, and by updating the plugin-builder skill documentation to reflect the build-time requirement for a structured permissions block.
Changes:
- Fix
spawn-processcapability detection to recognize bothchild_processandnode:child_processimports. - Add a capability detector parity/ratchet test suite and documentation contract tests to prevent scanner/doc drift.
- Update
backend/skills/agnt-plugin-builder/SKILL.mdto require and explainpermissions(capabilities + domains), including the exact build failure message anddraft-permissions.jsusage.
File summaries
| File | Description |
|---|---|
| backend/plugins/lib/validate-core.js | Expands the spawn-process import detectors to match both bare and node:-prefixed child_process. |
| backend/src/plugins/capabilityDetectors.test.js | Adds parity/ratchet tests for builtin import spelling and contract tests tying scanner capabilities and build failure messaging to documentation. |
| backend/skills/agnt-plugin-builder/SKILL.md | Documents required permissions block, capability vocabulary, domains semantics, and the builder’s exact refusal message + drafting helper. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1
to
+11
| import { afterEach, beforeEach, describe, expect, it } from 'vitest'; | ||
| import fsp from 'fs/promises'; | ||
| import os from 'os'; | ||
| import path from 'path'; | ||
|
|
||
| import { scanCapabilities } from '../../plugins/lib/validate-core.js'; | ||
|
|
||
| const REPO_ROOT = path.resolve(path.dirname(new URL(import.meta.url).pathname), '../../..'); | ||
| const SKILL_MD = path.join(REPO_ROOT, 'backend/skills/agnt-plugin-builder/SKILL.md'); | ||
| const BUILD_SCRIPT = path.join(REPO_ROOT, 'backend/plugins/cli/build-plugin.js'); | ||
| const VALIDATE_CORE = path.join(REPO_ROOT, 'backend/plugins/lib/validate-core.js'); |
Comment on lines
+60
to
+69
| async function scan(source, filename = 'index.js') { | ||
| await fsp.writeFile(path.join(dir, filename), source, 'utf8'); | ||
| const result = await scanCapabilities(dir); | ||
| return { | ||
| caps: Object.keys(result.capabilities).sort(), | ||
| detail: result.capabilities, | ||
| filesScanned: result.filesScanned, | ||
| scanFailed: result.scanFailed, | ||
| }; | ||
| } |
`new URL(import.meta.url).pathname` is a URL path. On Windows it is `/C:/Users/...`, and the leading slash makes `path.resolve` treat it as drive-relative, so REPO_ROOT came out as `C:\C:\Users\...` and all three reads that back the skill/scanner parity assertions failed with ENOENT. Linux CI cannot catch this: there the URL path and the filesystem path happen to be identical, so the suite is green on every runner and red on every Windows dev machine. Uses `fileURLToPath`, which is what server.js and every script under plugins/cli/ already does. sign-plugin.js carries a hand-rolled `.replace(/^\/([A-Za-z]:)/, '$1')` against the same trap — this avoids needing that workaround at all. Verified on Windows: 25/25 in this file, where 3 failed before.
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.
Two defects in the plugin capability-disclosure path, found while building a plugin from the skill and watching the build reject it.
Independent of #70 and #71 — no shared files.
1. A
node:-prefixed builtin disclosed nothingCAPABILITY_DETECTORS['spawn-process']matchedchild_processbut notnode:child_process. They are the same module, and thefilesystemdetectors two lines below already accept both spellings (fsandnode:fs) — so the inconsistency lived inside a single object literal, one rule written two ways.It is not theoretical. A plugin on this machine imports
node:child_processand shells out topython3. Its build reported:No
spawn-process. The call-shape patterns did not cover it either —execFile(...)/spawn(...)only match when the first argument is a quoted literal, and a plugin resolving its binary from a variable orprocess.envnever has one:Scanning that same plugin directory before and after:
env-access, filesystemenv-access, filesystem, **spawn-process**That plugin declared
spawn-processby hand, so the union model granted it. A plugin that stayed quiet would have shipped a shell-out with no disclosure at the consent modal and no cost to its trust tier — which is the whole point of the scan.2. The plugin-builder skill never mentioned the block the build requires
cli/build-plugin.js:216refuses to package a manifest without a structuredpermissionsblock.backend/skills/agnt-plugin-builder/SKILL.md— the instructions an agent follows to build a plugin from chat — contained the wordpermissionszero times, and its manifest example omitted the block entirely.So a plugin built by following the skill exactly fails at step 3, and the only way forward is to read the build script. That is how I found it.
Adds: the block in the manifest example, a rule stating it is required, a section covering all six capabilities and what
domainsmeans, the verbatim error string, and thedraft-permissions.jsescape hatch. It also states that declared and detected need not match — effective permissions are the union, so over-declaring is safe and is the right call when the plugin does something a regex scan cannot see.The capability table is derived from
CAPABILITY_DETECTORSrather than invented.draft-permissions.jstakes no arguments and writes one draft per dev plugin — verified against the script, after I first documented it wrongly.Testing
25 tests. 8 fail without this change (5 scanner, 3 documentation contract).
The suite includes a parity ratchet over every builtin the import detectors recognise: both spellings must yield the same capability set. It fails when a new builtin is added in one form only — which is how this gap arrived.
Three contract tests keep the skill honest against the code: every detectable capability is documented, the failure message is quoted as
build-plugin.jsactually emits it, and the manifest example carries a permissions block.Mutation tested — 8 mutants, 7 killed, 1 equivalent:
node:fs)The last one changes the document without violating the contract: the message must be findable, two contiguous copies exist, and removing one leaves it findable. Killing it would require asserting an exact occurrence count, which breaks on any legitimate third mention.
Two of these mutants initially survived, and both were real weaknesses in my own tests — fixed in
03edc590and55b37622. Both assertions searched the whole document while their names claimed a specific location, so deleting the copy that mattered left the other holding them green. One now extracts the manifest example and asserts inside it; the other scopes to the fenced error block and compares against the exact string the build script emits, plus a deliberately unscoped check for an unbroken copy — because the fenced block wraps mid-sentence and cannot be found by pasting the error into a search box.Full backend suite: 290 passed / 291 files. The single failure,
UpdateScheduler.status.test.js > replaces the previous pass rather than accumulating, is pre-existing and unrelated — it passes in isolation, imports nothing here, and does not fire on CI.Notes for the reviewer
netis a known remaining gap, asserted rather than fixed. It has a method-shape detector (net.connect(...)) but no import detector in either spelling, so it is a missing module rather than a missing prefix. A test documents it so it stays visible.backend/plugins/tests/disclosure.test.jsreports19/20both before and after this change (D4a spicy legacy plugin backfilled → unverified) — pre-existing, verified on pristineorigin/main. Those files are excluded from vitest as standalone scripts.validate-core.jsis vendored into the api.agnt.gg server repo viacli/sync-validate-core.js, and no CI job checks the copies agree. This change alters the canonical file, so the server copy needs re-vendoring for the publish gate to enforce the same rule. I cannot reach that repo.