Skip to content

Accept nested extensions/<name>/extension.mjs in external-plugin canvas checks#2403

Open
tlmii wants to merge 4 commits into
mainfrom
tlmii-fix-external-plugin-nested-canvas-extens
Open

Accept nested extensions/<name>/extension.mjs in external-plugin canvas checks#2403
tlmii wants to merge 4 commits into
mainfrom
tlmii-fix-external-plugin-nested-canvas-extens

Conversation

@tlmii

@tlmii tlmii commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Fixes #2402.

Problem

The external-plugin canvas-structure check required the canvas extension entry point to be a flat file at <pluginRoot>/extensions/extension.mjs. But the endorsed convention (AGENTS.md → Canvas Extensions) nests each extension in its own subfolder: <pluginRoot>/extensions/<name>/extension.mjs. Plugins following the nested convention — which install and run fine (e.g. upgrade-agent in #2394, whose install smoke test passes) — were falsely reported as "missing required canvas extension entry point."

This repo's own discovery helpers (eng/generate-marketplace.mjs, eng/generate-website-data.mjs) already probe the nested path; only the two external-plugin checks disagreed.

Fix

Both checks now locate the canvas entry point by accepting either the flat extensions/extension.mjs or at least one immediate subfolder containing extension.mjs, kept behaviorally aligned:

  • Quality gate (eng/external-plugin-quality-gates.mjs, git-object based): new locateCanvasEntryPoint checks the flat blob first, then enumerates extensions/ children via git ls-tree and probes each <name>/extension.mjs.
  • Intake (eng/external-plugin-intake.mjs, Contents-API based): new locateCanvasEntryPointViaApi checks the flat path, then walks the extensions/ directory listing for a subdir with extension.mjs.

A canvas plugin whose extensions/ contains no extension.mjs anywhere (flat or nested) still fails. eng/external-plugin-validation.mjs is unchanged.

Adversarial review hardening

A multi-model review surfaced three issues, addressed here:

  • Truncated tree listing (gate): the directory enumeration now uses git ls-tree -z via spawnSync (NUL-delimited, untruncated) instead of the 12 KB-capped runCommand, so large extensions/ directories no longer drop the real entry.
  • Coupled flat check (intake): the flat-path check is decoupled from the directory listing succeeding; a transient API error on the listing or a nested lookup now yields an unverifiable warning ("a maintainer should re-run intake") rather than a false rejection, matching the gate's infra-error philosophy.
  • Non-array listing (intake): the listing must be a JSON array (Array.isArray) before it is treated as a directory; otherwise a "must be a directory" error is raised.

Tests

  • eng/external-plugin-quality-gates.test.mjs: nested-subfolder pass, no-entry-anywhere fail, and a nested entry listed past the legacy output cap.
  • eng/external-plugin-intake.test.mjs (new): nested accept, flat accept, no-entry reject, unverifiable-on-listing-error warning, and flat-still-accepted when the listing errors.

All 11 tests pass; the new nested tests fail on the pre-fix code. npm run build, npm run plugin:validate, and npm run skill:validate all pass with no generated-file drift.

Note: distinct from #2397 / #2399 (tag-ref infra_error), though both touch the quality-gates file. Branched from main; #2399 was still open at push time, so no rebase was needed — expect only a light conflict if it lands first.

  Generated via Copilot (Claude Opus 4.8) on behalf of @tlmii

tlmii and others added 2 commits July 22, 2026 15:39
…as checks

The external-plugin canvas structure check (quality gate) and intake
validation both hardcoded a flat extensions/extension.mjs entry point,
falsely rejecting the documented nested extensions/<name>/extension.mjs
layout that installs and runs fine.

Scan the extensions/ directory for a nested subfolder containing
extension.mjs while still accepting the flat form for backward
compatibility. Applied to both runCanvasStructureGate (git-object
lookups) and validateCanvasPluginMetadata (Contents API), keeping them
behaviorally aligned. Added regression coverage for both paths.

Fixes #2402

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Address multi-model review findings on the nested canvas extension fix:

- Quality gate: enumerate extensions/ via 'git ls-tree -z' with spawnSync (NUL-delimited, untruncated) so large directories no longer drop the real entry past the 12KB output cap.

- Intake: decouple the flat extensions/extension.mjs check from the directory listing, require an array listing (Array.isArray) before treating it as a directory, and surface an unverifiable (warning) result instead of a false rejection when the listing or a nested lookup hits a transient API error.

- Add regression tests: nested entry beyond the legacy output cap (gate) and unverifiable/flat-still-accepted paths when the listing errors (intake).

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 22, 2026 22:57
@tlmii
tlmii requested a review from aaronpowell as a code owner July 22, 2026 22:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Updates external-plugin canvas validation to support nested extension entry points.

Changes:

  • Supports flat and nested extension.mjs layouts.
  • Handles lookup failures as unverifiable.
  • Adds regression tests for both validation paths.
Show a summary per file
File Description
eng/external-plugin-quality-gates.mjs Adds untruncated nested entry-point discovery.
eng/external-plugin-quality-gates.test.mjs Tests nested and large directory layouts.
eng/external-plugin-intake.mjs Adds API-based nested entry-point discovery.
eng/external-plugin-intake.test.mjs Tests intake acceptance and failure handling.

Review details

  • Files reviewed: 4/4 changed files
  • Comments generated: 2
  • Review effort level: Medium

Comment thread eng/external-plugin-intake.mjs Outdated
Comment thread eng/external-plugin-intake.mjs Outdated
…intake

Address PR review: the Contents API caps directory listings at 1,000 entries and required one request per extension subfolder, so a nested entry beyond the cap could be falsely rejected (the same truncation class the git gate avoids) and large repos risked latency / rate-limit exhaustion.

Replace the per-subfolder Contents API enumeration with one recursive 'git/trees/<locator>?recursive=1' fetch and inspect 'extensions/extension.mjs' and immediate 'extensions/<name>/extension.mjs' paths locally. A truncated tree without a located entry point is reported as unverifiable (warning) rather than rejected, and refs are normalized so 'refs/tags/<tag>' resolves as a tree-ish.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 23, 2026 02:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review details

  • Files reviewed: 4/4 changed files
  • Comments generated: 2
  • Review effort level: Medium

Comment thread eng/external-plugin-quality-gates.mjs Outdated
}

const nestedEntryPoint = toPosixPath(extensionsDir, entry.name, "extension.mjs");
const nestedCheck = checkPathExistsAtLocator(repoDir, locator, nestedEntryPoint, "blob");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Fixed in 437f916: locateCanvasEntryPoint now enumerates the extensions/ subtree with a single recursive git ls-tree -r -t -z and classifies the flat/nested entry points in memory, instead of spawning a git cat-file per immediate subfolder. Process count is now constant regardless of how many directories an untrusted repo packs under extensions/ — the 160-folder regression test dropped from ~10s to ~1.3s as a result.

  Generated via Copilot (Claude Opus 4.8) on behalf of @tlmii

Comment thread eng/external-plugin-intake.mjs Outdated
`submission: plugins tagged with "canvas" must include "extensions/extension.mjs" at ${releaseLocatorDescription}`,
);
} else if (extensionEntryResponse.kind === "apiError") {
const releaseTreeResponse = await fetchGitHubTree(repo, normalizeTreeish(releaseLocator), token);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed — a root recursive fetch made verifiability depend on total repo size, so a large unrelated monorepo could deterministically return truncated: true and permanently block a valid plugin. Fixed in 437f916: intake now walks to the plugin's extensions directory one level at a time (resolveDirectoryTreeSha, bounded by path depth) to resolve its tree SHA, then fetches only that subtree recursively. A truncated result is now only possible for an implausibly large extensions directory itself, not the rest of the repo.

  Generated via Copilot (Claude Opus 4.8) on behalf of @tlmii

Address reviewer feedback on unbounded scaling for untrusted/large repos:

- Quality gate: replace the per-candidate-directory git cat-file spawns in
  locateCanvasEntryPoint with a single recursive git ls-tree over the
  extensions subtree, classifying flat/nested entry points in memory. Process
  count is now constant regardless of how many folders live under extensions/.

- Intake: stop fetching the recursive git tree from the repo root (which a
  large unrelated monorepo can push past the Trees API truncation limit and
  never validate). Walk to the plugin's extensions directory one level at a
  time to resolve its tree SHA, then fetch only that subtree recursively, so
  verifiability depends on the plugin's own size, not the whole repository.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 23, 2026 23:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review details

  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Medium

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

External plugin canvas check requires flat extensions/extension.mjs, rejecting valid nested extensions/<name>/extension.mjs layout

2 participants