Enhance Markdown artifact review in Wizard and SDD canvases - #29
Draft
vibeCoder500 wants to merge 4 commits into
Draft
vibeCoder500 wants to merge 4 commits into
vibeCoder500 wants to merge 4 commits into
Conversation
Add one shared reader and independently packaged assets inside the existing canvas preview surfaces. Preserve existing workflows and action contracts, and add scoped reads, revision-bound clarification validation, navigation, and regression coverage. Keep preview distribution and repository discovery outside this change.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Local-reference resolution can escape a scoped review context, while SDD polling and deep-link event handling also need correction.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a shared, independently packaged Markdown artifact reader to the Wizard and SDD canvases while preserving workflow execution behavior.
Changes:
- Adds responsive GFM rendering, navigation, history, freshness, and clarification controls.
- Integrates scoped review services into both canvases.
- Adds packaging verification and broad browser/security regression coverage.
File summaries
| File | Description |
|---|---|
.gitattributes |
Marks generated reader assets. |
.gitignore |
Ignores reader build output. |
.github/workflows/markdown-artifact-review.yml |
Adds reader CI verification. |
docs/markdown-artifact-review.md |
Documents architecture and maintenance. |
scripts/canvas-reader/playwright.config.mjs |
Configures browser tests. |
scripts/canvas-reader/serve-fixture.mjs |
Serves isolated canvas fixtures. |
scripts/canvas-reader/stage-app-fixture.mjs |
Stages standalone plugin payloads. |
scripts/canvas-reader/sync-assets.d.mts |
Declares synchronization types. |
scripts/canvas-reader/sync-assets.mjs |
Packages and verifies reader assets. |
scripts/canvas-reader/sync-domain.mjs |
Synchronizes shared domain code. |
scripts/canvas-reader/fixtures/workspace.mjs |
Creates owned fixture workspaces. |
scripts/canvas-reader/test/clarifications.spec.mjs |
Tests clarification workflows. |
scripts/canvas-reader/test/fixture-tools.test.mjs |
Tests fixture safeguards. |
scripts/canvas-reader/test/freshness.spec.mjs |
Tests revision refresh behavior. |
scripts/canvas-reader/test/package-security.test.mjs |
Tests package isolation. |
scripts/canvas-reader/test/performance.spec.mjs |
Tests bounded rendering performance. |
scripts/canvas-reader/test/reader-accessibility.spec.mjs |
Tests responsive accessibility. |
scripts/canvas-reader/test/sdd-shell.spec.mjs |
Tests SDD shell integration. |
scripts/canvas-reader/test/sync-assets.test.mjs |
Tests asset verification. |
scripts/canvas-reader/test/wizard-shell.spec.mjs |
Tests Wizard shell integration. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/README.md |
Documents reader commands. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/server.mjs |
Registers review endpoints. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/server/artifact-clarifications.mjs |
Parses clarification markers. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/server/artifact-clarifications.NOTICES.txt |
Records parser notices. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/server/artifact-discovery.mjs |
Discovers review artifacts. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/server/artifact-read.mjs |
Performs bounded artifact reads. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/server/artifact-review.mjs |
Manages review contexts. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/app.js |
Integrates reader state. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/artifact-review.js |
Adapts the shared reader. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/client.js |
Handles review events. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/handlers-phase.js |
Opens phase artifacts. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-card.js |
Adds artifact review actions. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-runtime.js |
Preserves workflow state. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/setup.js |
Wires reader setup. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/styles/artifact-review.css |
Styles review controls. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/styles/overlays.css |
Updates overlay layout. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/vendor/markdown-reader/markdown-reader.css |
Packages generated reader styles. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/package.json |
Defines reader tooling. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/package-lock.json |
Locks reader dependencies. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/eslint.config.mjs |
Configures reader linting. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/tsconfig.json |
Configures reader type checking. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/vite.config.ts |
Configures browser bundling. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/vite.parser.config.ts |
Configures parser bundling. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/src/bundle.ts |
Exposes the reader bundle. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/src/clarificationParser.ts |
Extracts clarification markers. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/src/mount.tsx |
Implements mounting lifecycle. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/src/types.ts |
Defines reader contracts. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/src/MarkdownReader/MarkdownReader.tsx |
Implements the reader UI. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/src/MarkdownReader/markdownOutline.ts |
Builds heading outlines. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/src/MarkdownReader/reader.css |
Styles responsive reading. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/src/MarkdownReader/useActiveMarkdownHeading.ts |
Tracks active headings. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/src/MarkdownReader/MarkdownTableOfContents/MarkdownTableOfContents.tsx |
Implements responsive TOC navigation. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/test/clarification-source.test.ts |
Tests marker parsing. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/test/clarifications.test.tsx |
Tests clarification controls. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/test/content-security.test.tsx |
Tests sanitized rendering. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/test/markdown-outline.test.ts |
Tests outline generation. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/test/markdown-reader.test.tsx |
Tests reader behavior. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/test/markdown-table-of-contents.test.tsx |
Tests TOC interaction. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/test/mount.test.tsx |
Tests mounting and updates. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/test/reader-fixture.ts |
Provides reader fixtures. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/test/reader-states.test.tsx |
Tests reader states. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/test/artifact-review-context.test.mjs |
Tests review context binding. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/test/modals.test.mjs |
Tests clarification modals. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/test/reader-probe.mjs |
Probes packaged reader behavior. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/test/fixtures/review-freshness-fixture.mjs |
Supplies freshness fixtures. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/test/fixtures/review-http-fixture.mjs |
Supplies review HTTP fixtures. |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/test/fixtures/server-integration-fixture.mjs |
Integrates server fixtures. |
plugins/spec-kit-copilot-sdd/extensions/sdd-canvas/README.md |
Documents SDD reader behavior. |
plugins/spec-kit-copilot-sdd/extensions/sdd-canvas/artifact-review.mjs |
Adapts SDD review contexts. |
plugins/spec-kit-copilot-sdd/extensions/sdd-canvas/extension.mjs |
Hosts review APIs and events. |
plugins/spec-kit-copilot-sdd/extensions/sdd-canvas/index.html |
Integrates the SDD reader UI. |
plugins/spec-kit-copilot-sdd/extensions/sdd-canvas/sdd.mjs |
Preserves SDD workflow behavior. |
plugins/spec-kit-copilot-sdd/extensions/sdd-canvas/ui/artifact-review.js |
Packages the browser adapter. |
plugins/spec-kit-copilot-sdd/extensions/sdd-canvas/vendor/artifact-clarifications.mjs |
Packages clarification parsing. |
plugins/spec-kit-copilot-sdd/extensions/sdd-canvas/vendor/artifact-clarifications.NOTICES.txt |
Records parser notices. |
plugins/spec-kit-copilot-sdd/extensions/sdd-canvas/vendor/artifact-discovery.mjs |
Packages artifact discovery. |
plugins/spec-kit-copilot-sdd/extensions/sdd-canvas/vendor/artifact-read.mjs |
Packages bounded reading. |
plugins/spec-kit-copilot-sdd/extensions/sdd-canvas/vendor/artifact-review.mjs |
Packages review services. |
plugins/spec-kit-copilot-sdd/extensions/sdd-canvas/vendor/markdown-reader/markdown-reader.css |
Packages reader styles. |
plugins/spec-kit-copilot-sdd/extensions/sdd-canvas/vendor/markdown-reader/markdown-reader.js |
Packages reader runtime. |
plugins/spec-kit-copilot-sdd/extensions/sdd-canvas/vendor/markdown-reader/manifest.json |
Records reader provenance. |
plugins/spec-kit-copilot-sdd/extensions/sdd-canvas/vendor/markdown-reader/THIRD_PARTY_NOTICES.txt |
Records reader dependencies. |
plugins/spec-kit-copilot-sdd/extensions/sdd-canvas/tests/artifact-review.test.mjs |
Tests SDD review services. |
plugins/spec-kit-copilot-sdd/extensions/sdd-canvas/tests/reader-probe.mjs |
Probes packaged SDD reader. |
Review details
- Files reviewed: 77/93 changed files
- Comments generated: 3
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+340
to
+344
| relativeTarget = path.posix.normalize(path.posix.join(path.posix.dirname(source.relativePath), relativeTarget)); | ||
| validateArtifactPath(relativeTarget); | ||
| } catch { return { kind: "inert", reason: "unsupported_target" }; } | ||
| if (relativeTarget === source.relativePath) return { kind: "fragment", artifactId: sourceArtifactId, fragment }; | ||
| const document = await readArtifact(workspacePath, relativeTarget); |
Comment on lines
+238
to
+240
| const state = currentState(); | ||
| const signature = await entry.review.signature(); | ||
| const sig = stateSignature(state, signature); |
Comment on lines
833
to
+839
| try { | ||
| const es = new EventSource(endpoint("/events")); | ||
| es.addEventListener("state", (e) => { try { render(JSON.parse(e.data)); } catch (_) {} }); | ||
| es.onerror = () => {}; | ||
| es.addEventListener("review", (event) => { | ||
| try { | ||
| const review = JSON.parse(event.data); | ||
| if (!review.contextHint || review.contextHint === artifactReview?.context?.contextId) void artifactReview?.refresh(); |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Relative links can escape the selected review scope, while clarification nesting, polling cost, and incomplete parser type-check coverage also remain unresolved.
Review details
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/server/artifact-review.mjs:367
- Each SDD poll calls
signature(), which recursively rediscovers and stats up to 10,000 entries for every retained review context. Contexts remain retained after the reader closes, so the 1.5-second timer can repeatedly perform tens or hundreds of thousands of filesystem operations for up to 15 minutes. Cache/incrementally update the signature or explicitly unregister closed reader contexts instead of rescanning them on every poll.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/src/clarificationParser.ts:24 - This check excludes only text whose immediate parent is a link. Markdown such as
[**[NEEDS CLARIFICATION: Q?]**](research.md)has anemphasis/strongparent, so it becomes a trusted clarification binding and the renderer can place a button inside a navigable anchor. Exclude text with any link ancestor in both the parser and renderer transform.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/tsconfig.json:20 npm run typecheckdoes not include the newly added parser Vite configuration, so type errors in that build path are silently skipped even though CI reports the reader configuration as typechecked.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/server/artifact-review.mjs:344
- Relative links are read before checking that the normalized target belongs to the current feature/composition scope. A client can POST
../../other-feature/private.md(with a valid source revision) and receive any workspace Markdown file, contradicting the scope boundary. Reject targets outside the scope’s explicit candidates and allowed roots before reading them.
const document = await readArtifact(workspacePath, relativeTarget);
- Files reviewed: 78/94 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Local-reference scope enforcement and bounded SDD polling must be corrected before approval.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/server/artifact-review.mjs:369
- The 1.5-second SDD poll calls
discoverCandidatesfor every still-valid selection, while closing the reader does not remove selections and up to 20 contexts remain valid for 15 minutes. Each discovery can inspect 10,000 entries before this loop incrementsinspectedonly for returned Markdown candidates, so repeated opens—or directories containing mostly non-Markdown files—can cause hundreds of thousands of filesystem operations per poll. Deduplicate/cache signatures by scope and retire selections on close/expiry; also account fordiscovered.inspectedCountrather than only candidate count.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/server/artifact-review.mjs:344
- Relative-link resolution reads and registers any Markdown path that normalizes inside the workspace, without checking that it belongs to the current feature/composition scope. For example, a link from
specs/001-feature/spec.mdto../002-other/private.mdnormalizes to a valid path and bypasses the membership boundary. Check the target against the current scope's candidates/roots beforereadArtifact, and return an inert result when it is outside that scope.
relativeTarget = path.posix.normalize(path.posix.join(path.posix.dirname(source.relativePath), relativeTarget));
validateArtifactPath(relativeTarget);
} catch { return { kind: "inert", reason: "unsupported_target" }; }
if (relativeTarget === source.relativePath) return { kind: "fragment", artifactId: sourceArtifactId, fragment };
const document = await readArtifact(workspacePath, relativeTarget);
- Files reviewed: 78/94 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Relative links can escape the active review scope, and SDD polling introduces potentially excessive recurring filesystem scans.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/markdown-reader/tsconfig.json:20
- The new parser Vite configuration is omitted from the TypeScript project, so
npm run typecheckdoes not check it despite the validation workflow's typecheck step. Includevite.parser.config.tsalongside the reader config so both build configurations are covered.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/server/artifact-review.mjs:344
- Relative links are normalized before authorization, so
../002-other/spec.mdfromspecs/001-fixture/spec.mdbecomesspecs/002-other/spec.md, passes path validation, and is read even though it is outside the current feature/composition scope. Revalidate the normalized target against the context's candidates or allowed roots before reading and registering it; the synchronized SDD copy needs the same correction.
relativeTarget = path.posix.normalize(path.posix.join(path.posix.dirname(source.relativePath), relativeTarget));
validateArtifactPath(relativeTarget);
} catch { return { kind: "inert", reason: "unsupported_target" }; }
if (relativeTarget === source.relativePath) return { kind: "fragment", artifactId: sourceArtifactId, fragment };
const document = await readArtifact(workspacePath, relativeTarget);
plugins/spec-kit-copilot-sdd/extensions/sdd-canvas/extension.mjs:240
- While an SSE client is connected, this runs every 1.5 seconds and
review.signature()rescans up to 10,000 entries and stats every candidate. Large feature trees will therefore cause continuous high filesystem load even when nothing changes. Cache discovery/signature state or invalidate it from scoped filesystem changes rather than performing a full scan on every poll.
const signature = await entry.review.signature();
const sig = stateSignature(state, signature);
- Files reviewed: 78/94 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
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.
Summary
Enhance Markdown viewing inside the existing Wizard and SDD canvas previews, without introducing another plugin or canvas.
Existing Offerings Preserved
Intentional behavior changes: existing documents are readable while setup or execution is gated; Run/Rerun remains gated. Clarification markers in code examples or HTML comments are inert, and stale reviewed answers require refresh/reconfirmation.
Scope And Review Guide
Reader-only changes for both canvases. No repository browsing/authentication/cloning, preview installers or distribution, generated agent files, historical release records, branding, version bumps, or changes to unrelated plugins/skills/presets.
linguist-generated; canonical implementation remains visible for review.Verification
Verified locally on Windows with Node 24.19.0 and Edge 153.0.4234.32:
Native And Hosted Acceptance
Final candidate:
5e3b459a1666ec28a3087f3595ca68e60efc44bc.Hosted Windows and Linux validation: both jobs passed on this exact candidate. These are fork-hosted checks; upstream PR check runs are not currently attached.
Tested in GitHub Copilot App 1.1.20 on Windows using an owned synthetic repository and real project-local Wizard and SDD providers. Served payloads were hash-verified; normal installed plugins and real projects were not modified.
30 days.: only the synthetic specification changed; 145 other tracked files remained unchanged, with no additions. Its resolved question disappeared.Native testing found and regression-tested two reader defects: compact drawer stacking and explicit refresh of expired contexts. Recovery revalidates selected-document membership and clears old context bindings; automatic updates do not renew the existing 15-minute expiry. A temporary App WebView loading/focus issue was resolved by reacquiring native controls and opening the preview normally; the final SDD recovery check then passed without another code change.
Limits: actual Windows App acceptance, not native macOS/Linux certification or marketplace-release validation. Raw local evidence and diagnostic captures remain local. PR remains draft; no merge or release performed.