You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
My contribution adds a new instruction, prompt, agent, skill, workflow, or canvas extension file in the correct directory. (N/A: repository automation)
The file follows the required naming convention.
The content is clearly structured and follows the example format.
I have tested my instructions, prompt, agent, skill, workflow, or canvas extension with GitHub Copilot. (N/A: see Validation)
I have run npm start and verified that README.md is up to date.
I am targeting the main branch for this pull request.
Description
Implements Phase 3 of #4184: canvas review evidence (section 6) and weekly operating metrics (section 8), plus MOS3 portability notes. It is one of three PRs (Phase 1 #4189: ownership and routing; Phase 2 #4190: submission gate and risk tiers) and doesn't define CODEOWNERS, routing, submission-gate, merge-risk:*, or state labels.
Safe auto-merge (section 7) has been removed from this PR following maintainer review. Arming auto-merge from automation risks codifying workarounds to the required-review and Copilot-review policies and widens the attack surface (it relied on pull_request_target, which this repo doesn't use). It's deferred pending a security discussion with GitHub; the deferral is documented.
1. canvas-smoke-test check
New .github/workflows/canvas-smoke-test.yml (job canvas-smoke-test). It runs on pull_request to main for extensions/** and plugins/** (plus the checker and its workflow), with read-only permissions and no secrets. A detect-only step runs first, so plugin changes with no canvas extension skip the dependency install and the smoke test. validate-canvas-extensions.yml is unchanged from main.
eng/canvas-smoke-test.mjs (+ tests) checks each affected extension without executing it:
Module graph: parses every reachable module and follows static imports, literal dynamic import(), literal require(), and package.jsonimports aliases. Call-shaped text inside strings, templates, and regexes is ignored. Every reference must resolve to a local file, a builtin, the host SDK, or a declared runtime dependency.
Files: symlinks, node_modules, native binaries, executables, and unsafe paths fail.
Preview:assets/preview.png must be a structurally valid PNG (exactly one leading IHDR, CRCs, bounded decode) of at least 400×160.
Plugin: materializes the plugin, schema-validates it, and installs it into an isolated COPILOT_HOME.
Evidence: a job summary, the canvas-smoke-test-results artifact, and one sticky PR comment. The comment is posted by canvas-smoke-test-comment.yml (writer, workflow_run), which binds the artifact to the triggering run and never checks out PR code.
2. Weekly operating metrics
eng/review-metrics.mjs (+ tests), .github/review-metrics.yml, .github/workflows/review-metrics.yml. It runs Mondays at 14:00 UTC and on dispatch. It reports:
open contributions by state and risk tier
median/p90 time to first review and time to merge
reviewer load and concentration (HHI)
items past the 2- and 4-business-day targets
the automation failure rate
Publishes to a pinned review-metrics tracking issue and uploads an artifact. Missing Phase 1/2 labels show up as unknown buckets.
3. Docs
docs/maintainers/canvas-evidence-and-metrics.md covers canvas evidence, metric definitions, the auto-merge deferral, and Portability to MOS3, including the microsoft/azure-dev-tools marketplace.
Updated eng/README.md. setup-labels.yml appends only review-metrics.
Type of Contribution
New instruction file.
New prompt file.
New agent file.
New plugin.
New skill file.
New agentic workflow.
New canvas extension.
Update to existing instruction, prompt, agent, plugin, skill, workflow, or canvas extension.
node --test eng/canvas-smoke-test.test.mjs eng/review-metrics.test.mjs: 43/43 pass. Covers: reachable data: imports fail, deleted bundle manifests are read from the base SHA, the reviews connection is paginated, and workflow-runs API errors are reported as incomplete.
npm run build: passes, with no unrelated diffs.
npm run plugin:validate: passes.
All existing extensions/* pass the checker. Synthetic fixtures fail as expected: a bad or duplicate-IHDR PNG, an undersized preview, a .. path, an executable, a missing file, an import inside a string literal, and unresolvable # aliases.
Metrics were dry-run (read-only) against upstream data.
Maintainer follow-up
Run Setup Labels to create review-metrics.
Optionally set CANVAS_PREVIEW_MIN_WIDTH / CANVAS_PREVIEW_MIN_HEIGHT (default 400×160).
After Phase 2 lands, submission-gate picks up canvas-smoke-test as an optional check by name. Add it to the ruleset if desired.
After the first metrics run, confirm the tracking issue is pinned (pinning is best-effort).
Revisit auto-merge separately after the security discussion.
…ew metrics (github#4184)
Adds the canvas-smoke-test check with review evidence, a config-gated safe auto-merge workflow (disabled by default), and a weekly review operating metrics workflow, plus maintainer docs.
Refs github#4184
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Resolve CODEOWNERS team membership for ownership checks
eng/auto-merge.mjs:296
The Phase 1 ownership model uses CODEOWNERS teams, but this comparison only matches a literal owner token to the author's login. An author who belongs to @github/awesome-copilot-content-reviewers will never qualify through the codeowners source. Resolve team membership through the API (with a conservative failure mode), or document and configure this source as direct-user-only.
Read declared authors for non-plugin resources
eng/auto-merge.mjs:513
The PR description says recorded ownership comes from author/authors front matter or plugin.json, but non-plugin resources never have their front matter read; they fall back only to the oldest commit author. This both rejects declared authors (for example, skills/mentoring-juniors/SKILL.md:5-9) and may grant ownership to an importer instead. Parse the resource's declared GitHub authors before using commit history as a fallback.
Fail validation for missing reachable dynamic imports
eng/canvas-smoke-test.mjs:684
A missing file referenced by a reachable dynamic import is only a warning, so the required canvas-smoke-test still passes even though the extension will fail at runtime. Use the same strict/error path as static imports when the importing module is reachable.
Resolve CODEOWNERS team membership instead of matching users
eng/auto-merge.mjs:296
Phase 1’s CODEOWNERS design assigns paths to teams, but this equality check can only recognize direct user entries: a value such as @github/awesome-copilot-content-reviewers can never equal an author login. Consequently the configured codeowners ownership source will reject team members. Resolve team membership through GitHub or explicitly constrain/document this source as direct-user-only.
Parse resource front matter for declared ownership
eng/auto-merge.mjs:511
The PR description says recorded ownership comes from author/authors front matter, but this implementation never parses resource front matter; non-plugin resources use only the oldest commit author. For example, skills can declare GitHub authors in front matter, so those declared owners will not qualify unless they also authored the oldest commit. Implement the documented front-matter lookup or update the stated eligibility contract.
Reject runtime imports declared only in devDependencies
eng/canvas-smoke-test.mjs:661
A runtime import that exists only in devDependencies still allows the smoke test to pass, even though production plugin installation does not guarantee that package and the documented policy requires declared runtime dependencies. Make this a validation error for reachable modules.
Ignore reviews submitted before the latest review clock start
eng/review-metrics.mjs:131
This can select a maintainer review submitted before the last ready-for-review event. After a draft/re-ready cycle, subtracting the later clock start produces a negative time-to-first-review and also incorrectly marks the PR as no longer waiting. Restrict candidates to reviews submitted on or after the review clock starts.
- Bind canvas report writer to the triggering workflow_run PR (base/head repo, ref, SHA)
- Bound PNG inflation by IHDR-derived size and a decoded-bytes budget
- Classify file: URLs as unsafe manifest paths
- Follow CommonJS require() and literal dynamic imports; error on missing
reachable targets and devDependency-only runtime imports
- Validate removed extension paths and orphaned plugins instead of skipping
- Warn on .py/.rb/.pl scripts
- Auto-merge: support CODEOWNERS teams (fail closed), front matter authors,
list PRs by head SHA, only fall back to direct merge on clean status,
drop ineffective check_run trigger
- Metrics: ignore reviews before the PR was ready for review
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Addressed the Copilot review in 5462990. All inline threads have replies and are resolved. The same commit also fixes the four "previously missed" findings:
CODEOWNERS teams: team owners such as @org/team now count when the author is an active team member. Membership is checked with GET /orgs/{org}/teams/{slug}/memberships/{login}, and any lookup error counts as "not a member".
Front matter authors: recorded owners are also read from author/authors in resource front matter on the base branch: SKILL.md, hook README.md, and *.agent.md/*.instructions.md. Only a github field, a github.com URL or an @login counts.
Missing reachable dynamic imports are now errors.
devDependency-only runtime imports are now errors when the importing module is reachable from extension.mjs.
Validation: 43 unit tests pass. node eng/canvas-smoke-test.mjs --all --install never still passes on every existing extension. npm run build and npm run plugin:validate both pass.
This writer has no concurrency guard, so two completed runs for the same PR can both list comments before either creates one, producing duplicate “sticky” comments. The other reader/writer workflows serialize by head repository and branch (for example, .github/workflows/label-pr-intent-writer.yml:14); apply the same grouping here.
…Writer
Adds trusted_checks (default submission-gate -> external_id
submission-gate-writer). Any other check run or status with that name on the
head SHA blocks arming, so a PR-defined job cannot satisfy the gate.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Filter bytes are validated only for non-interlaced PNGs. An Adam7 PNG with an invalid filter value greater than 4 can therefore pass inspectPng even though a conforming decoder rejects it, undermining the requirement that previews decode as real PNGs. Walk each Adam7 pass and validate the leading filter byte for every pass row as well.
Target detection only considers plugin paths whose manifest still exists in the post-change tree. Deleting plugins/<id>/plugin.json while leaving extensions/<id>/extension.mjs, or deleting a bundling plugin, therefore makes canvas-smoke-test report success as skipped instead of validating the canvas removal. Use the base-tree manifest/change status to recognize deleted or formerly extension-bearing plugins and validate the resulting ownership/materialization state.
Limited label history miscalculates review wait times
eng/review-metrics.mjs:385
Only the last 20 label events are fetched, but normalizeIssue falls back to issue creation when the most recent ready-for-review/awaiting-approval event has fallen outside that slice. Long-running or re-reviewed external-plugin issues can then be reported as waiting since creation, substantially overstating the 2/4-business-day metrics. Paginate label events (or fetch until the current state's latest matching event is found) instead of silently using an incomplete timeline.
The reason will be displayed to describe this comment to others. Learn more.
I'm going to be honest, I'm not super comfortable with the notion of an auto-merge flow.
The concern I have is that we're attempting to do codification of workarounds on repo and org policies that are in place, such as required reviewers, or allowing CCR to be able to approve PRs.
The other issue is that we are creating quite a wide surface area for malicious actors to attempt to exploit. Yes, we're doing the right things like checking out the base branch to ensure a known-good on the workflow rather than the PR HEAD, but a little bit of absentmindedness and that can be wiped out easily.
I think we should first discuss with GitHub security to look at whether we can get an exception on using CCR for auto-approvals and see how we can leverage that, rather than trying to circumvent restrictions.
- Remove safe auto-merge (config, workflow, script, tests, labels) per
maintainer review; documented as deferred pending a security review.
- Restore validate-canvas-extensions.yml to main and move the smoke test
to its own canvas-smoke-test.yml workflow scoped to extensions/**.
- Reject duplicate IHDR chunks in preview PNGs.
- Mask string/template/regex literal contents before extracting dynamic
import() and require() specifiers.
- Resolve package.json imports aliases through the same classifier and
module graph; fail closed on alias shapes that cannot be analyzed.
- Rename docs to canvas-evidence-and-metrics.md and update references.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@aaronpowell thanks, agreed on auto-merge. I've removed it from this PR entirely (config, workflow, script, tests, and labels) in 4aebda3 and documented it as deferred until we've had the security conversation. It risks codifying workarounds to the review policies, and it relied on pull_request_target. I also restored validate-canvas-extensions.yml to main; the smoke test now has its own canvas-smoke-test.yml, scoped to extensions/**.
- Trigger canvas smoke tests for plugin changes while skipping expensive setup when target detection finds no canvas coverage.
- Fail reachable data: imports while preserving warnings for unreachable module data: imports.
- Use base plugin manifests for deleted bundle plugin detection and fail closed when base manifests cannot be read.
- Paginate GraphQL PR review pages before computing weekly review metrics.
- Report workflow run API collection errors as incomplete automation health instead of not found.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
Import scanning and canvas target detection have bypasses, while several reporting and infrastructure failures are misclassified or insufficiently sanitized.
PNG requires the IEND chunk to have a zero-length data field, but this accepts any length when its CRC is valid and reports the image as structurally valid. Reject nonzero-length IEND chunks before setting sawEnd.
Plugin listing failures are misreported as contribution failures
eng/canvas-smoke-test.mjs:1369
A failed or unparsable copilot plugin list --json is silently converted to listed = null. With newer CLIs that expose live marketplace installs only through this listing, the code then reports “installed plugin files not found” and exits as a contribution failure. Treat list-command and JSON-format failures as infra_error so CLI outages or output changes are not attributed to the contributor.
Retries can duplicate GitHub write operations
eng/lib/review-automation-github.mjs:30
This retries every HTTP method, including the POSTs that create the tracking issue and weekly comment. If GitHub processes a write but returns a transient 502/503, the retry can create duplicate issues or comments. Restrict automatic retries to idempotent reads, or add operation-specific idempotency handling for writes.
Unescaped titles allow Markdown links in metrics reports
eng/review-metrics.mjs:300
PR and issue titles are untrusted, but this escapes only table separators, newlines, and mentions. A title such as [review details](https://attacker.example) becomes a live link in the trusted metrics issue. Escape Markdown link/formatting characters and HTML delimiters before embedding titles in the report.
Resolve the setup-labels.yml and eng/README.md conflicts with Phase 2 as
additive unions. The submission gate's pluggable canvas-smoke-test slot is
already satisfied by canvas-smoke-test.yml's job name and paths.
- Treat a slash after a postfix ++/-- as division, so a real import on the
same line as x++ / y can no longer be masked as a regex literal and evade
the dependency, traversal, and capability checks.
- Union base and head extension references for an edited plugin.json, so
dropping a bundle's only registration of an extension is still validated.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Risk tier:merge-risk:high — Privileged execution, automation, or review-policy change Required to merge: passing submission-gate checks plus 2 approvals from reviewers with write access, including a maintainer with admin or maintain permission.
Why this tier
.github/review-metrics.yml is a high-risk path (automation, scripts, MCP config, hooks, or review policy)
.github/workflows/canvas-smoke-test-comment.yml is a high-risk path (automation, scripts, MCP config, hooks, or review policy)
.github/workflows/canvas-smoke-test.yml is a high-risk path (automation, scripts, MCP config, hooks, or review policy)
.github/workflows/review-metrics.yml is a high-risk path (automation, scripts, MCP config, hooks, or review policy)
.github/workflows/setup-labels.yml is a high-risk path (automation, scripts, MCP config, hooks, or review policy)
eng/README.md is a high-risk path (automation, scripts, MCP config, hooks, or review policy)
eng/canvas-smoke-test.mjs is a high-risk path (automation, scripts, MCP config, hooks, or review policy)
eng/canvas-smoke-test.test.mjs is a high-risk path (automation, scripts, MCP config, hooks, or review policy)
eng/lib/review-automation-github.mjs is a high-risk path (automation, scripts, MCP config, hooks, or review policy)
eng/review-metrics.mjs is a high-risk path (automation, scripts, MCP config, hooks, or review policy)
eng/review-metrics.test.mjs is a high-risk path (automation, scripts, MCP config, hooks, or review policy)
Label needs-review:HIGH flags a high contributor-risk signal
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
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.
github-actions
Bot
added
new-submission
aaronpowell
left a comment
Pull Request Checklist
npm startand verified thatREADME.mdis up to date.mainbranch for this pull request.Description
Implements Phase 3 of #4184: canvas review evidence (section 6) and weekly operating metrics (section 8), plus MOS3 portability notes. It is one of three PRs (Phase 1 #4189: ownership and routing; Phase 2 #4190: submission gate and risk tiers) and doesn't define CODEOWNERS, routing,
submission-gate,merge-risk:*, or state labels.Safe auto-merge (section 7) has been removed from this PR following maintainer review. Arming auto-merge from automation risks codifying workarounds to the required-review and Copilot-review policies and widens the attack surface (it relied on
pull_request_target, which this repo doesn't use). It's deferred pending a security discussion with GitHub; the deferral is documented.1.
canvas-smoke-testcheck.github/workflows/canvas-smoke-test.yml(jobcanvas-smoke-test). It runs onpull_requesttomainforextensions/**andplugins/**(plus the checker and its workflow), with read-only permissions and no secrets. A detect-only step runs first, so plugin changes with no canvas extension skip the dependency install and the smoke test.validate-canvas-extensions.ymlis unchanged frommain.eng/canvas-smoke-test.mjs(+ tests) checks each affected extension without executing it:import(), literalrequire(), andpackage.jsonimportsaliases. Call-shaped text inside strings, templates, and regexes is ignored. Every reference must resolve to a local file, a builtin, the host SDK, or a declared runtime dependency.node_modules, native binaries, executables, and unsafe paths fail.assets/preview.pngmust be a structurally valid PNG (exactly one leadingIHDR, CRCs, bounded decode) of at least 400×160.COPILOT_HOME.canvas-smoke-test-resultsartifact, and one sticky PR comment. The comment is posted bycanvas-smoke-test-comment.yml(writer,workflow_run), which binds the artifact to the triggering run and never checks out PR code.2. Weekly operating metrics
eng/review-metrics.mjs(+ tests),.github/review-metrics.yml,.github/workflows/review-metrics.yml. It runs Mondays at 14:00 UTC and on dispatch. It reports:review-metricstracking issue and uploads an artifact. Missing Phase 1/2 labels show up asunknownbuckets.3. Docs
docs/maintainers/canvas-evidence-and-metrics.mdcovers canvas evidence, metric definitions, the auto-merge deferral, and Portability to MOS3, including themicrosoft/azure-dev-toolsmarketplace.eng/README.md.setup-labels.ymlappends onlyreview-metrics.Type of Contribution
Additional Notes
Validation
node --test eng/canvas-smoke-test.test.mjs eng/review-metrics.test.mjs: 43/43 pass. Covers: reachabledata:imports fail, deleted bundle manifests are read from the base SHA, the reviews connection is paginated, and workflow-runs API errors are reported as incomplete.npm run build: passes, with no unrelated diffs.npm run plugin:validate: passes.extensions/*pass the checker. Synthetic fixtures fail as expected: a bad or duplicate-IHDR PNG, an undersized preview, a..path, an executable, a missing file, an import inside a string literal, and unresolvable#aliases.Maintainer follow-up
review-metrics.CANVAS_PREVIEW_MIN_WIDTH/CANVAS_PREVIEW_MIN_HEIGHT(default 400×160).submission-gatepicks upcanvas-smoke-testas an optional check by name. Add it to the ruleset if desired.Refs #4184
By submitting this pull request, I confirm that my contribution abides by the Code of Conduct and will be licensed under the MIT License.