Fix Spec Kit wizard artifact and phase handling - #24
Merged
Conversation
Fix wizard phase and artifact handling across task, checklist, converge, extension, and inline clarification flows. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 23ff38e2-a233-493e-a8b7-c885652f57bc
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
One or more issues must be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 4
New issues introduced by this change (4)
| Severity | Finding |
|---|---|
 |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/wizard-phases.mjs — The new Converge phase advertises specs/<slug>/tasks.md, but scanner hydration only replaces the… |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/fs-helpers.mjs — This length cutoff still misclassifies valid multi-digit task markers such as [US10] and… |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/spec-phases.mjs — The persisted artifactPath is checked before the newly selected formValues.checklistFile. After… |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/modals.js — markPhaseRunning installs a five-minute run lock before submission is confirmed, but the failure… |
What changed in this PR
Fixes wizard regressions in artifact detection, phase tracking, command rendering, and clarification reruns.
Changes:
- Adds complete
convergephase support. - Improves checklist/task artifact handling and extension run-lock tracking.
- Adds behavior-focused regression tests.
| File | Description |
|---|---|
| plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/state.js | Updated as part of this pull request. |
| plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-runtime.js | Updated as part of this pull request. |
| plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/modals.js | Updated as part of this pull request. |
| plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/test/state-and-scanner.test.mjs | Updated as part of this pull request. |
| plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/test/server-integration.test.mjs | Updated as part of this pull request. |
| plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/test/composition.test.mjs | Updated as part of this pull request. |
| plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/prompts/pipeline.mjs | Updated as part of this pull request. |
| plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/spec-phases.mjs | Updated as part of this pull request. |
| plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/fs-helpers.mjs | Updated as part of this pull request. |
| plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/wizard-phases.mjs | Updated as part of this pull request. |
Suppressed comments (1)
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-runtime.js:765
- Making
coreCandidatesunconditional leavescustomizedIds(lines 749–760) completely unused, while the preceding explanation still says it prevents customized canonicals from appearing in both sections. Remove that dead computation and stale comment so this new intentional duplication has one clear source of truth.
// CORE group: the full canonical Spec Kit surface, including commands
// customized by presets. The Core list is an explicit escape hatch for
// adding the canonical command back to the pipeline even when the active
// preset also contributes a customized version.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
One or more issues must be addressed before approval.
Review tier: Balanced
Findings: 2
Pre-existing issues (2)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/modals.js — markPhaseRunning installs a five-minute run lock before submission is confirmed, but the failure… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/fs-helpers.mjs — This length cutoff still misclassifies valid multi-digit task markers such as [US10] and… View comment |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/spec-phases.mjs — The persisted artifactPath is checked before the newly selected formValues.checklistFile. After… View resolved comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/wizard-phases.mjs — The new Converge phase advertises specs/<slug>/tasks.md, but scanner hydration only replaces the… View resolved comment |
Suppressed comments (3)
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/fs-helpers.mjs:41
- The length cutoff still classifies valid multi-digit user-story markers such as
[US10]and[US11]as placeholders. A task list containing tasks for two such stories therefore has two distinct matches and is downgraded toempty; explicitly exclude task markers (P,ID, andUS\d+) rather than relying only on token length.
export const PLACEHOLDER_TOKEN_RE = /\[[A-Z][A-Z0-9_]{3,}\]/g;
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/spec-phases.mjs:106
artifactPathis checked beforeformValues.checklistFile, so a previously persisted checklist folder/file wins and the explicitly configured checklist is ignored. This contradicts the intended resolution order and can open the newest unrelated checklist instead of the user's selected file when both fields are present; prioritizechecklistFilebefore falling back toartifactPath.
const configuredSources = [
phases.checklist?.formValues?.checklistFile,
phases.checklist?.artifactPath,
];
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/modals.js:448
- The run lock is acquired before submission, but the failure path returns without releasing it. Because
postJsonconverts HTTP/network errors toundefined, every failed clarification submit leaves the phase marked running (and its regular controls disabled) until the five-minute safety timer expires; importclearPhaseRunningand call it in the catch path while preserving the queued answers.
markPhaseRunning(p.commandName);
const result = await __postJson("/api/phase/submit", { commandName: p.commandName, args });
if (!result) throw new Error("phase submit did not return a queued response");
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
One or more issues must be addressed before approval.
Review tier: Balanced
Findings: 2
Pre-existing issues (2)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/modals.js — markPhaseRunning installs a five-minute run lock before submission is confirmed, but the failure… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/fs-helpers.mjs — This length cutoff still misclassifies valid multi-digit task markers such as [US10] and… View comment |
Suppressed comments (5)
Previously missed (3) — in code that hasn't changed since the last review.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/spec-phases.mjs:87
- A persisted checklist path is accepted even when it belongs to a different spec slug. Because the scanner selects the newest slug but merges global state first, switching from
oldtonewcan leavespecs/old/checklists/security.mdas the first existing candidate and the viewer continues opening the old feature’s checklist. Reject candidates outside the activechecklistsDirso resolution falls back to the newest file for the selected slug.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/prompts/pipeline.mjs:40 - Converge appends remediation tasks to the existing
tasks.md, whose provenance remainsspeckit:tasks; routing it through this generic branch instructs the agent to replace the first line withspeckit:converge. That conflicts with the command’s append-only contract and mislabels the shared Tasks artifact. Give Converge a dedicated artifact instruction that preserves the existing first line.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/modals.js:514 - The artifact viewer is also called from
ui/composition-artifacts.js:272-276with onlyartifactPath,shortLabel, andid, sop.commandNameis undefined for those markdown files. Command sources can contain literal[NEEDS CLARIFICATION: …]examples, and after these imports fix the prior runtime error they now render Clarify pills whose answers are queued underundefined;flushClarificationsimmediately returns false and Apply and Rerun can never work. Only enable clarification parsing for viewers with a rerunnable command, or pass an unambiguous command name from every caller.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/fs-helpers.mjs:41
- The length cutoff still treats valid multi-digit user-story markers such as
[US10]and[US11]as placeholders. BecauselooksLikeUnfilledTemplatedeclares a file unfilled after two distinct matches, a normal task list with those markers is downgraded toempty. Exclude theUS<digits>marker grammar explicitly rather than relying only on token length.
export const PLACEHOLDER_TOKEN_RE = /\[[A-Z][A-Z0-9_]{3,}\]/g;
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/modals.js:448
- When submission fails,
markPhaseRunninghas already installed a five-minute run lock, but this catch path preserves it. The clarification button is re-enabled, yet the phase card remains falsely running/disabled until its safety timer expires. Mark the phase running only after the server confirms that the rerun was queued (or explicitly clear the lock in the failure path).
markPhaseRunning(p.commandName);
const result = await __postJson("/api/phase/submit", { commandName: p.commandName, args });
if (!result) throw new Error("phase submit did not return a queued response");
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
One or more issues must be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/spec-phases.mjs — When the phase is done but no Markdown candidate exists (for example, a configured file was not… |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/modals.js — markPhaseRunning installs a five-minute run lock before submission is confirmed, but the failure… View resolved comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/fs-helpers.mjs — This length cutoff still misclassifies valid multi-digit task markers such as [US10] and… View resolved comment |
Suppressed comments (1)
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/modals.js:448
markPhaseRunningstarts a five-minute run lock before the fallible submit, but the failure path never clears it. SincepostJsonconverts network/HTTP errors toundefined, the new error branch leaves the phase rendered as running and disables its normal Run/Rerun controls until the safety timeout; the failure test currently masks this by clearing the lock infinally. Start the lock only after validating the queued response (or explicitly clear it incatch).
try {
markPhaseRunning(p.commandName);
const result = await __postJson("/api/phase/submit", { commandName: p.commandName, args });
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
One or more issues must be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 2
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/modals.js — flushClarifications has no in-flight guard even though it can be invoked both automatically after… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/spec-phases.mjs — When the phase is done but no Markdown candidate exists (for example, a configured file was not… View comment |
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/prompts/pipeline.mjs:40
- Routing Converge through the generic artifact branch tells the agent that the first line of the existing
tasks.mdmust become<!-- speckit:converge v1 -->. Converge is defined to append remediation work to the task list, so this conflicts with the existing Tasks provenance header and can cause the shared artifact to be rewritten incorrectly. Special-case this prompt to preserve the existing document/header while appending remediation tasks.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/spec-phases.mjs:140
- When Checklist is marked done but the configured/persisted file is missing and the directory contains no Markdown files,
checklistArtifactPathreturnsnulland this block leaves the stale path (oftenspecs/<slug>/checklists/) untouched. That contradicts the promised folder fallback and still produces an unresolved or missing artifact target. Persist the concrete checklist directory whenever no file can be resolved.
const artifactPath = await checklistArtifactPath(checklistsDir);
if (artifactPath) {
phases.checklist = {
...phases.checklist,
artifactPath,
};
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
One or more issues must be addressed before approval.
Review tier: Balanced
Findings: None
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/modals.js — flushClarifications has no in-flight guard even though it can be invoked both automatically after… View resolved comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/spec-phases.mjs — When the phase is done but no Markdown candidate exists (for example, a configured file was not… View resolved comment |
Suppressed comments (5)
Previously missed (4) — in code that hasn't changed since the last review.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/fs-helpers.mjs:40
- The
{3,}cutoff suppresses every 1–3 character placeholder, not only task markers. For example, a template containing distinct unresolved[API]and[URL]tokens is now considered filled bylooksLikeUnfilledTemplate. Preserve the original token-length grammar and explicitly exclude the valid task markers instead.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/prompts/pipeline.mjs:40 - Putting Converge in this shared branch instructs it to make the first line of the existing task list
<!-- speckit:converge v1 -->. Converge is defined as appending remediation totasks.md, whose existing provenance header is<!-- speckit:tasks v1 -->; replacing that header contradicts the append-only behavior and can misidentify the task artifact. Give Converge an artifact instruction that preserves the existing header.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/modals.js:451 - The pending queue remains mutable while this request is in flight, but the artifact viewer does not lock all clarification controls. The automatic all-answered path leaves its Apply button enabled, allowing the same arguments to be submitted twice; the manual path leaves unanswered pills active, and answers added after
argswas built are then discarded byclearClarifications()on success. Add a per-command in-flight guard or disable/re-enable all clarification controls, and clear only the submitted snapshot.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-runtime.js:123 - Resolving this extension slice still does not provide a valid completion signal for reruns. Extension artifact hydration derives
statusfrom file existence and never advanceslastRunAt, so a command whose artifact already exists starts atdone; on the next snapshot,observePhaseProgress()sees that pre-existing terminal status and immediately clears the newly mapped lock before the rerun finishes. The test only coversempty → donewith a fabricated timestamp. Track a baseline transition/file mtime or emit an explicit extension completion update before clearing the lock.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/spec-phases.mjs:106
- Including the persisted
artifactPathhere can bypass the required newest-file fallback. If a completed phase points torequirements.mdand a newer checklist was generated—or the scanner has selected a newer feature slug—the old existing path is returned beforenewestChecklistFile(checklistsDir)runs. Treat onlychecklistFileas an explicit selection, then choose the newest file from the active checklist directory.
const configuredSources = [
phases.checklist?.formValues?.checklistFile,
phases.checklist?.artifactPath,
];
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical and moderate issues affect rerun locking, command identity, artifact viewing, and queued-answer retention.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-runtime.js — Mapping extension commands to their phase slice makes reruns unlock too early when that slice is… |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/modals.js — Answers can still be queued or edited while this request is awaiting the server, but a successful… |
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-runtime.js:777
- These synthesized Core cards use the same canonical
idandcommandNameas the preset-customized cards. The add endpoint stores only that bare id, andresolvePipelineEntry()resolves it through the active preset winner, so clicking the advertised Core escape hatch adds another customized phase rather than the canonical implementation. Preserve a core-layer identity through the card/pipeline/dispatch path, or avoid presenting this as a Core bypass.
.map((id) => __synthesizeCanonicalPhase(id))
.sort((a, b) => collator.compare(a.shortLabel || a.id, b.shortLabel || b.id))
.map((p) => renderCard(p, "core"))
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/spec-phases.mjs:122
- Returning the directory as
artifactPathstill produces a broken View action for a completed checklist with no Markdown file.renderGraphPhaseCard()treats every nonempty done-phase artifact path as a file andopenArtifactViewer()sends it to/api/artifact, whosereadFile()call returns 404 for directories; the existing folder-browser path is not invoked. Represent this fallback as a folder target and open the folder browser, or suppress the file View action until a file exists.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-runtime.js:773
- Making the Core candidates unconditional leaves the preceding
customizedIdscomputation completely unused, and its comment still claims customized commands are routed away from Core to prevent duplicates—the opposite of the new behavior. Remove that obsolete block and update the surrounding explanation so this render path has one consistent policy.
const coreCandidates = [
...canonicalSpine(),
...CANONICAL_UNSEEDED,
];
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 23ff38e2-a233-493e-a8b7-c885652f57bc
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
One or more issues must be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 2
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs — For skipped or error, the new prompt intentionally sends no reportExecution, but this… |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/fs-helpers.mjs — Returning realCandidate breaks callers that later compute a workspace-relative UI path. If the… |
 |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/prompts.mjs — The generated prompt tells the agent that the Run button remains locked until this callback or a… |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/run-tracker.mjs — The duplicate guard has no recovery path if a queued turn is cancelled, crashes, or otherwise omits… View resolved comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/extension-artifacts.mjs — For an absolute writesTo located inside the workspace, the new containment branch accepts the… View resolved comment |
Suppressed comments (4)
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs:94
- A terminal callback with no
runIdbypasses this stale-run check, thenclearRun(..., null)deletes whichever token is currently active. BecauserunIdremains optional in the action schema, an omitted-ID callback from an older overlapping run can still persist its status and clear the newer token. Require a matching ID whenever an active token exists while preserving any intentionally supported untracked status-update path.
if (["done", "skipped", "error"].includes(status)
&& runId
&& !activeRunMatches(inst.instanceId, `speckit.${phase}`, runId)) {
return { ok: false, error: "stale phase run" };
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/run-tracker.mjs:29
- This unconditionally replaces an existing same-command token, and the new test explicitly expects overlapping runs. That conflicts with the PR description's claim that a second canonical run is rejected until the terminal callback clears the guard. Since the code and README document launcher semantics, update the PR description or restore the stated duplicate-run rejection contract.
};
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/extension-artifacts.mjs:145
- An absolute
writesTothat is still inside the workspace passes this check, but line 149 exposes it unchanged asartifactPath; the artifact and reveal endpoints reject absolute UI paths. Reject absolute cache targets here (as below), or convert them to workspace-relative paths before storing them.
const abs = isAbsolute(resolvedPath) ? resolvedPath : join(cwd, resolvedPath);
const lexicallyInsideWorkspace = isPathContained(canonicalizePath(cwd), canonicalizePath(abs));
if (!lexicallyInsideWorkspace) {
phases[key] = next;
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/prompts.mjs:127
- This lifecycle description contradicts the new three-second local acknowledgement:
phase-runtime.jsno longer waits for snapshots or a terminal callback to unlock the button. Update the API documentation to describe status persistence/token cleanup rather than a callback-driven UI lock.
* `setPhaseStatus` with a terminal status before returning. The wizard keeps
* the Run button locked until that callback lands, with only a timeout as a
* last-resort fallback. Kept as a short, plain-English preamble so it doesn't
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Moderate issues remain in run-token validation, command classification, artifact visibility, and duplicate-run feedback.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 5
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/dispatch.mjs — phaseIdForCommandName only checks the command's shape, so valid non-canonical single-segment… |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-card.js — A failed rerun now persists status: "error", but the scanner still supplies the previously… |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-card.js — The local acknowledgement re-enables this button after three seconds while the server token can… |
Pre-existing issues (3)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/fs-helpers.mjs — Returning realCandidate breaks callers that later compute a workspace-relative UI path. If the… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs — For skipped or error, the new prompt intentionally sends no reportExecution, but this… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/prompts.mjs — The generated prompt tells the agent that the Run button remains locked until this callback or a… View comment |
Suppressed comments (3)
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs:94
- A terminal callback without
runIdbypasses this stale-run check, thenclearRun(..., undefined)acts as a wildcard and deletes the currently active token. A delayed or malformed callback can therefore persist stale completion and unlock a newer run. When a tracked run exists, require a supplied matching ID before persisting; keep tokenless updates only for phases with no active run.
if (["done", "skipped", "error"].includes(status)
&& runId
&& !activeRunMatches(inst.instanceId, `speckit.${phase}`, runId)) {
return { ok: false, error: "stale phase run" };
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/prompts.mjs:159
- This instruction is inaccurate after the local acknowledgement change: the Run button unlocks after three seconds regardless of the callback; only the server-side duplicate guard remains active. Describe the token guard instead so the generated prompt matches runtime behavior.
`Do not leave the phase in progress, and do not omit this terminal callback because the wizard's Run button stays locked until it receives one or the safety timeout expires.`,
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/server/handlers-phase.mjs:78
- These comments still describe a witness window, but this path now only adds a terminal-status preamble and asks the agent to self-report execution on success. Keeping the removed lifecycle mechanism in the handler documentation makes the new dispatch contract misleading.
// `track: true` prepends the wizard tracking preamble so the agent
// reports a terminal phase status, and opens a witness window so the
// extension records which artifacts actually fired on successful runs.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 23ff38e2-a233-493e-a8b7-c885652f57bc
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Stale or tokenless callbacks can still corrupt current phase status and execution evidence.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 6
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/prompts.mjs — A stale run whose setPhaseStatus(done) call is rejected can still call reportExecution, which… |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/prompts.mjs — This docstring still says the Run button remains locked until the terminal callback, but the new… |
Pre-existing issues (5)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-card.js — The local acknowledgement re-enables this button after three seconds while the server token can… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-card.js — A failed rerun now persists status: "error", but the scanner still supplies the previously… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/dispatch.mjs — phaseIdForCommandName only checks the command's shape, so valid non-canonical single-segment… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/fs-helpers.mjs — Returning realCandidate breaks callers that later compute a workspace-relative UI path. If the… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs — For skipped or error, the new prompt intentionally sends no reportExecution, but this… View comment |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/prompts.mjs — The generated prompt tells the agent that the Run button remains locked until this callback or a… View resolved comment |
Suppressed comments (2)
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs:94
- Terminal callbacks without
runIdbypass this stale-run check. SincerunIdremains optional, a delayed or legacy callback can persist its terminal status and thenclearRun(..., null)removes whichever newer token is active, defeating the guard this PR adds. When an active token exists, require a matchingrunId; allow tokenless terminal updates only when no run is tracked.
if (["done", "skipped", "error"].includes(status)
&& runId
&& !activeRunMatches(inst.instanceId, `speckit.${phase}`, runId)) {
return { ok: false, error: "stale phase run" };
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/app.js:249
observePhaseProgress()is now an empty function, so invoking it on every snapshot refresh leaves dead lifecycle plumbing after the simplification. Remove this call, its import, and the no-op export rather than retaining an API that suggests snapshots still drive running feedback.
observePhaseProgress();
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Two critical stale-token defects and two moderate issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 2
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs — Terminal callbacks can omit runId, bypass the stale-token check, persist their status, and then… |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs — The stale-token check is not atomic with the status write. persistAndBroadcast yields for disk… |
Pre-existing issues (4)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-card.js — The local acknowledgement re-enables this button after three seconds while the server token can… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-card.js — A failed rerun now persists status: "error", but the scanner still supplies the previously… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/dispatch.mjs — phaseIdForCommandName only checks the command's shape, so valid non-canonical single-segment… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/fs-helpers.mjs — Returning realCandidate breaks callers that later compute a workspace-relative UI path. If the… View comment |
Issues resolved since last review (3)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/prompts.mjs — This docstring still says the Run button remains locked until the terminal callback, but the new… View resolved comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/prompts.mjs — A stale run whose setPhaseStatus(done) call is rejected can still call reportExecution, which… View resolved comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs — For skipped or error, the new prompt intentionally sends no reportExecution, but this… View resolved comment |
Suppressed comments (2)
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/spec-phases.mjs:145
- The new folder fallback is dropped before the phase card can use it.
buildStateSnapshotomitsfolderPathfrom core phase slices (canvas-runtime/snapshot-builder.mjs:43-56) andbuildCommandsalso omits it (snapshot-builder.mjs:196-209), whilerenderPhaseCardprefers that command entry. Thus this completed-checklist case still renders no browse-folder link; forwardfolderPaththrough both projections and add a scanner-to-snapshot test.
phases.checklist = {
...phases.checklist,
artifactPath: target.artifactPath,
...(target.folderPath ? { folderPath: target.folderPath } : {}),
};
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/prompts.mjs:128
- This comment describes a callback-driven lock and timeout that no longer exist.
markPhaseRunningnow clears a local acknowledgement after three seconds regardless ofsetPhaseStatus, so keeping this text will mislead future lifecycle changes.
* then call `setPhaseStatus` with a terminal status before returning. The local
* Run button state is only a short acknowledgement animation: chat owns live
* progress, `setPhaseStatus` persists terminal phase state, and the scanner
* confirms files before artifact buttons become available. Kept as a short,
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Run-token handling, non-phase command tracking, and clarification failure-retention coverage remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 2
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/test/modals.test.mjs — The new tests only exercise successful submissions, so the advertised failure-retention contract is… |
Pre-existing issues (6)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs — The stale-token check is not atomic with the status write. persistAndBroadcast yields for disk… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs — Terminal callbacks can omit runId, bypass the stale-token check, persist their status, and then… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-card.js — The local acknowledgement re-enables this button after three seconds while the server token can… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-card.js — A failed rerun now persists status: "error", but the scanner still supplies the previously… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/dispatch.mjs — phaseIdForCommandName only checks the command's shape, so valid non-canonical single-segment… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/fs-helpers.mjs — Returning realCandidate breaks callers that later compute a workspace-relative UI path. If the… View comment |
Suppressed comments (2)
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs:109
- A terminal callback without
runIdreaches this branch and callsclearRun(..., null), which deletes the currently active token regardless of which run produced the callback. Thus a legacy or token-omitting older turn can still overwrite status and clear a newer run, bypassing the stale-callback protection. Preserve untagged compatibility only when no active token exists; reject an untagged terminal callback while a run is active.
if (["done", "skipped", "error"].includes(status)) {
if (runId) {
finishRun(inst.instanceId, `speckit.${phase}`, runId, { allowReport: status === "done" });
} else {
clearRun(inst.instanceId, `speckit.${phase}`, runId);
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/dispatch.mjs:155
phaseIdForCommandName()classifies any one-segmentspeckit.*name as a phase, but presets can add non-canonical commands such asspeckit.review(test/composition.test.mjs:1101-1123). This line therefore creates a token and an invalidsetPhaseStatus({ phase: "review" })preamble for that command instead of treating it as untracked; the callback cannot pass the phase enum. Gate the candidate againstPHASE_BY_IDbefore callingbeginRun.
const run = phaseId ? beginRun(inst?.instanceId, commandName) : null;
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 23ff38e2-a233-493e-a8b7-c885652f57bc
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Run-token races can persist stale status or reports, and setup/preset remain incorrectly classified as canonical commands.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 2
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs — consumeReportableRun removes the only run-id guard before the asynchronous report merge is… |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/prompts.mjs — This registry check still classifies the non-command setup and preset pseudo-phases as… |
Pre-existing issues (5)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs — The stale-token check is not atomic with the status write. persistAndBroadcast yields for disk… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-card.js — The local acknowledgement re-enables this button after three seconds while the server token can… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-card.js — A failed rerun now persists status: "error", but the scanner still supplies the previously… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/dispatch.mjs — phaseIdForCommandName only checks the command's shape, so valid non-canonical single-segment… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/fs-helpers.mjs — Returning realCandidate breaks callers that later compute a workspace-relative UI path. If the… View comment |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/test/modals.test.mjs — The new tests only exercise successful submissions, so the advertised failure-retention contract is… View resolved comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs — Terminal callbacks can omit runId, bypass the stale-token check, persist their status, and then… View resolved comment |
Suppressed comments (1)
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs:105
- The run-token check is separated from the terminal status write by
await persistAndBroadcast(...). Because a newer same-phase dispatch is allowed,beginRuncan replace the token while this write is awaiting disk I/O; the old callback then persists its terminal status,finishRunfails, but the handler still returns{ ok: true }. This breaks the stated stale-callback guarantee. SerializebeginRunwith terminal commits per run key, or atomically reserve the matching token before the awaited write and prevent replacement until the commit finishes.
await persistAndBroadcast(inst, {
phases: {
[phase]: {
status,
artifactPath: artifactPath ?? undefined,
Gate phase tracking on the canonical command inventory so setup and preset pseudo-phases are not treated as runnable skills while keeping canonical commands like converge tracked. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
One or more issues must be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs — runPhase still accepts the setup and preset meta-phase IDs because both are in… |
Pre-existing issues (5)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs — The stale-token check is not atomic with the status write. persistAndBroadcast yields for disk… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-card.js — The local acknowledgement re-enables this button after three seconds while the server token can… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-card.js — A failed rerun now persists status: "error", but the scanner still supplies the previously… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/dispatch.mjs — phaseIdForCommandName only checks the command's shape, so valid non-canonical single-segment… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/fs-helpers.mjs — Returning realCandidate breaks callers that later compute a workspace-relative UI path. If the… View comment |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/prompts.mjs — This registry check still classifies the non-command setup and preset pseudo-phases as… View resolved comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs — consumeReportableRun removes the only run-id guard before the asynchronous report merge is… View resolved comment |
Suppressed comments (2)
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs:95
- The stale-token check is not atomic with the status write. While this handler awaits
persistAndBroadcast, a newer same-phase dispatch can replace the active token; the older callback then persists its terminal status beforefinishRunnotices the mismatch. That violates the stated supersession guarantee and can show the old run as done/error while the newer run is active. Serialize status persistence with token replacement, or revalidate the token within the same guarded critical section that commits the patch.
if (runId) {
if (!activeRunMatches(inst.instanceId, commandName, runId)) {
return { ok: false, error: "stale phase run" };
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/fs-helpers.mjs:43
securePathWithinreturns the realpath rather than the validated workspace spelling. If the workspace itself was opened through a symlink (for example/work/link -> /data/project), checklist callers later computerelative(cwd, securedPath)as../data/project/...; that path is then rejected by/api/artifactas outside the workspace. Preserve a workspace-relative/lexical path for UI serialization while still performing the realpath checks for containment.
return realCandidate;
Exclude setup and preset meta screens from phase action schemas and runtime validation so agent-triggered phase runs cannot dispatch non-phase slash commands. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 23ff38e2-a233-493e-a8b7-c885652f57bc
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A critical stale-token race can persist outdated terminal status, and dispatch latency plus missing canonical checklist folder projection remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 2
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs — The run token is checked before persistAndBroadcast(), which performs multiple awaited… |
Pre-existing issues (5)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs — The stale-token check is not atomic with the status write. persistAndBroadcast yields for disk… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-card.js — The local acknowledgement re-enables this button after three seconds while the server token can… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-card.js — A failed rerun now persists status: "error", but the scanner still supplies the previously… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/dispatch.mjs — phaseIdForCommandName only checks the command's shape, so valid non-canonical single-segment… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/fs-helpers.mjs — Returning realCandidate breaks callers that later compute a workspace-relative UI path. If the… View comment |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs — runPhase still accepts the setup and preset meta-phase IDs because both are in… View resolved comment |
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/canvas-runtime/dispatch.mjs:166
- Awaiting this promise delays the HTTP 202/action response until the Copilot turn finishes, because the SDK's
session.send()promise resolves with the final assistant message rather than when the prompt is queued. As a result the UI's three-second acknowledgement expires whilepostJsonis still pending, and clarification flushes keep the viewer locked for the whole run. Return after scheduling/accepting the send and handle the eventual send rejection asynchronously (including token cleanup) instead.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/spec-phases.mjs:145
- This folder-only fallback is dropped before reaching the browser.
buildStateSnapshotcopies onlyartifactPathfor canonical phases (canvas-runtime/snapshot-builder.mjs:43-56), unlike its extension projection, so a completed checklist with no markdown file losesfolderPathand renders neither a viewer nor a browse-folder action. CarryfolderPaththrough the canonical snapshot projection and cover the scanner-to-snapshot path.
phases.checklist = {
...phases.checklist,
artifactPath: target.artifactPath,
...(target.folderPath ? { folderPath: target.folderPath } : {}),
};
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
One or more issues must be addressed before approval.
Review tier: Balanced
Findings: 2
Pre-existing issues (2)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-card.js — A failed rerun now persists status: "error", but the scanner still supplies the previously… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/dispatch.mjs — phaseIdForCommandName only checks the command's shape, so valid non-canonical single-segment… View comment |
Issues resolved since last review (4)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs — The run token is checked before persistAndBroadcast(), which performs multiple awaited… View resolved comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs — The stale-token check is not atomic with the status write. persistAndBroadcast yields for disk… View resolved comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-card.js — The local acknowledgement re-enables this button after three seconds while the server token can… View resolved comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/fs-helpers.mjs — Returning realCandidate breaks callers that later compute a workspace-relative UI path. If the… View resolved comment |
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/canvas-runtime/run-tracker.mjs:10
- These module-level maps are never cleared when a canvas instance closes;
onCloseremoves the instance record but leaves any active/reportable tokens keyed by that instance ID. Reopening instances will steadily retain stale token entries (and a reused instance ID can inherit stale run state). Add an instance-scoped cleanup function and invoke it from the canvas teardown path.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs:104
- The freshness check is separated from the status write by an
await. A newer same-phase dispatch can callbeginRunwhile this callback is insidepersistAndBroadcast; the older callback then persists its terminal status, and the laterfinishRunmerely returns false (which is ignored). This allows exactly the stale overwrite these tokens are intended to prevent. Make token validation and status persistence serialized/atomic per phase, and add a test that pauses persistence while starting the newer run.
if (runId) {
if (!activeRunMatches(inst.instanceId, commandName, runId)) {
return { ok: false, error: "stale phase run" };
}
} else if (hasActiveRun(inst.instanceId, commandName)) {
return { ok: false, error: "stale phase run" };
}
}
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/spec-phases.mjs:145
- The folder-only fallback is lost before it reaches the UI.
hydrateSpecPhasessetsphases.checklist.folderPath, butbuildStateSnapshotprojects core phases withoutfolderPath(andbuildCommandsalso omits it), so a completed checklist with no matching file still renders neither a folder link nor a usable artifact action. Please carry this field through the core phase/command snapshot projections and cover the scanner → snapshot path in the test.
phases.checklist = {
...phases.checklist,
artifactPath: target.artifactPath,
...(target.folderPath ? { folderPath: target.folderPath } : {}),
};
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 23ff38e2-a233-493e-a8b7-c885652f57bc
Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 23ff38e2-a233-493e-a8b7-c885652f57bc
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 23ff38e2-a233-493e-a8b7-c885652f57bc
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 23ff38e2-a233-493e-a8b7-c885652f57bc
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 23ff38e2-a233-493e-a8b7-c885652f57bc
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The critical dispatch regression and unresolved token, artifact, and command-rendering issues must be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/dispatch.mjs — session.send() completes with the agent turn, not merely when the prompt is accepted. Since both… |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/spec-phases.mjs — A failed checklist rerun sets the persisted status to error, so this gate stops resolving the… |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-card.js — artifactPath is also populated with expected/template paths for phases whose files do not exist… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/dispatch.mjs — phaseIdForCommandName only checks the command's shape, so valid non-canonical single-segment… View comment |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-card.js — A failed rerun now persists status: "error", but the scanner still supplies the previously… View resolved comment |
Suppressed comments (3)
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs:108
- The freshness check occurs before
await persistAndBroadcast(), leaving a time-of-check/time-of-use race. If a newer same-phase dispatch callsbeginRun()while this persistence is awaiting disk I/O/rescan, the older callback still writes its terminal status and only afterward failsfinishRun(), so stale status can overwrite the newer run despite the token guard. Token acceptance and terminal persistence need per-phase serialization or another atomic ordering mechanism.
if (runId) {
if (!activeRunMatches(inst.instanceId, commandName, runId)) {
return { ok: false, error: "stale phase run" };
}
} else if (hasActiveRun(inst.instanceId, commandName)) {
return { ok: false, error: "stale phase run" };
}
}
await persistAndBroadcast(inst, {
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/spec-phases.mjs:145
- The new folder fallback is lost before rendering: this function stores
folderPathon the scanned core checklist slice, butbuildStateSnapshot()only forwardsfolderPathforcommands/*extension slices, not for core phases. Consequently a completed checklist with no matching markdown file never gets the advertised Browse folder action. Forward this field for core phase snapshots as well.
// The checklist directory is fixed for a feature's lifetime, so
// keeping an existing folder fallback alongside a later file
// target is safe and still points at the same artifact area.
phases.checklist = {
...phases.checklist,
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-runtime.js:667
- This fallback now derives overrides only from
composition.artifacts. When that cached/generated composition payload is absent but the on-disk preset graph still emits a canonical command withsource: "preset:…", the command is grouped under the preset yet is not added tooverriddenCanonicals, so the same canonical is also offered as a Core card. Retain the command-list source fallback for snapshots where winner metadata is unavailable.
const overriddenCanonicals = new Set();
for (const id of [...canonicalSpine(), ...CANONICAL_UNSEEDED]) {
const winner = winnerByCmdId.get(id) || winnerByCmdId.get(`speckit.${id}`);
if (winner && winner.layer !== "core") overriddenCanonicals.add(id);
}
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
One or more issues must be addressed before approval.
Review tier: Balanced
Findings: 3
Pre-existing issues (3)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-card.js — artifactPath is also populated with expected/template paths for phases whose files do not exist… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/spec-phases.mjs — A failed checklist rerun sets the persisted status to error, so this gate stops resolving the… View comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/dispatch.mjs — phaseIdForCommandName only checks the command's shape, so valid non-canonical single-segment… View comment |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/dispatch.mjs — session.send() completes with the agent turn, not merely when the prompt is accepted. Since both… View resolved comment |
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-contributors.js:579
- A
donestatus does not establish that this report belongs to the latest run.beginRunonly clears the in-memory reportable token, while the persisted execution report has no run ID; if a successful rerun omits or failsreportExecution, this lookup keeps showing the previous run's verdict instead of the documented fallback. Persist/compare the report's run ID, or clear the prior report when a new run starts.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs:104
- The stale-token check happens before
persistAndBroadcast, which awaits a disk read, write, and rescan. A newer same-phase dispatch can callbeginRunduring any of those awaits, after this callback has passed the check; the older callback then still persists its terminal status andfinishRunmerely fails afterward. Make token validation and the status commit atomic for the phase (or serialize bothbeginRunand terminal commits on the same per-phase guard) so an older callback cannot overwrite a newer run.
if (runId) {
if (!activeRunMatches(inst.instanceId, commandName, runId)) {
return { ok: false, error: "stale phase run" };
}
} else if (hasActiveRun(inst.instanceId, commandName)) {
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/spec-phases.mjs:138
- A failed checklist rerun has status
error, so this gate skips dynamic resolution even when a prior checklist file still exists. Because the error callback omitsartifactPath, normalization restores the defaultspecs/<slug>/checklists/directory path; the UI then attempts to open that directory as a file instead of retaining the prior checklist. Treaterroras an attempted checklist run for artifact resolution while preserving the error status.
const hasChecklistRun = phases.checklist?.status === "done";
const hasConfiguredChecklist = typeof phases.checklist?.formValues?.checklistFile === "string"
&& !!phases.checklist.formValues.checklistFile.trim();
if ((hasChecklistRun || hasConfiguredChecklist) && await deps.pathExists(checklistsDir)) {
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-card.js:492
artifactPathis not necessarily scanner-confirmed: canonical phase slices seed it from the declared output template, and extension scanning assigns it before existence/realpath validation. Gating only on truthiness therefore exposes “View artifact” for unrun or missing outputs (for example, an untouched checklist points atspecs/<slug>/checklists/and the file endpoint returns an error). More seriously, an extension path rejected because it resolves through a symlink outside the workspace is still exposed here, while/api/artifactperforms only a lexical containment check and follows that symlink. Keep the expected writes-to path separate from a scanner-verified artifact path (or add an explicit verified flag), and enable this action only for the verified value.
const canViewArtifact = !!p.artifactPath;
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Three moderate artifact-visibility, checklist-resolution, and stale-status race issues remain unresolved.
Review tier: Balanced
Findings: None
Issues resolved since last review (3)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-card.js — artifactPath is also populated with expected/template paths for phases whose files do not exist… View resolved comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/spec-phases.mjs — A failed checklist rerun sets the persisted status to error, so this gate stops resolving the… View resolved comment |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/dispatch.mjs — phaseIdForCommandName only checks the command's shape, so valid non-canonical single-segment… View resolved comment |
Suppressed comments (3)
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/actions/phase.mjs:104
- The token check is separated from persistence by several awaits. An old callback can pass this check, then a newer same-phase dispatch can replace the active token (and even persist its terminal result) while the old
persistAndBroadcastis pending; the old callback can subsequently write stale status and still return{ ok: true }. The write queue only serializes already-computed payloads, so validation and commit need a shared per-phase serialized/conditional operation that rechecks the active token at commit time.
if (runId) {
if (!activeRunMatches(inst.instanceId, commandName, runId)) {
return { ok: false, error: "stale phase run" };
}
} else if (hasActiveRun(inst.instanceId, commandName)) {
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/project-scanner/spec-phases.mjs:137
- The new
errorterminal status is excluded from checklist artifact resolution. After a checklist rerun fails,setPhaseStatusomits an artifact path and normalization restores the phase's folder template; withoutformValues.checklistFile(the normal graph form does not persist it), this branch is skipped, so an existing checklist file is no longer selected and the failed-rerun View action targets the folder instead. Treat an errored checklist as having run for artifact lookup while preserving itserrorstatus.
const hasChecklistRun = phases.checklist?.status === "done";
const hasConfiguredChecklist = typeof phases.checklist?.formValues?.checklistFile === "string"
&& !!phases.checklist.formValues.checklistFile.trim();
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-card.js:492
artifactPathis not an existence signal. Core slices seed it with the expected template before a run (wizard-phases.mjs:198-207), and the extension scanner intentionally retains a missingwritesTopath (extension-artifacts.mjs:149-157;state-and-scanner.test.mjs:1206-1210). Consequently, every unrun phase with an expected path now gets a View button that opens a missing or<slug>path and fails. Track scanner-confirmed existence separately (or separate expected and resolved paths) and use that signal here while still allowing confirmed artifacts when status iserror.
const canViewArtifact = !!p.artifactPath;
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 23ff38e2-a233-493e-a8b7-c885652f57bc
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Artifact actions must be gated on scanner-confirmed file existence rather than an expected artifact path.
Review tier: Balanced
Findings: None
Suppressed comments (1)
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/ui/phase-card.js:493
artifactPathdoes not mean that the scanner found a file. Every empty canonical phase is initialized with its expected template path (for example, Plan getsspecs/<slug>/plan.mdfromemptyPhaseSlice), and extension inference also retains missing targets. Consequently an untouched phase now renders View artifact and sends a placeholder/nonexistent path to/api/artifact. Please separate the expected writes-to path from scanner-confirmed file existence (or expose an explicit existence flag) and gate this action on the latter.
const canViewArtifact = !!p.artifactPath;
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
Fixes several Spec Kit Wizard canvas regressions around artifact detection, phase tracking, command lists, inline clarification reruns, and phase run feedback.
Bug fixes
[P],[ID], and[US1]are treated as task content, not unresolved template placeholders.formValues.checklistFile, then choosing the most recently modified generated checklist file when the phase has run, with the checklist folder as fallback.convergeto the wizard phase registry and fallback phase order sosetPhaseStatusand execution reporting accept it consistently.speckit.*commands tocommands/<full-command-id>for status and artifact lookup.speckit.reviewremain untracked.writesTotargets back to workspace-relative artifact paths before exposing them to the viewer.error./api/phase/submit, marks the phase running, updates last-submitted args only on success, and keeps queued answers visible on failed submit.Running…to a short local dispatch acknowledgement. The chat window is the source of truth for execution progress, and scanner-confirmed files are the source of truth for artifact buttons.runIdtokens so older terminal callbacks are rejected after a newer same-phase dispatch, including tokenless terminal callbacks while a tracked run is active, without adding server-side lifecycle locks.done.Run-feedback scope
The wizard is a launcher plus artifact dashboard: users monitor the chat for live execution, while the wizard briefly acknowledges dispatch and then relies on scanner-confirmed workspace files for artifact buttons. Canonical phases keep only a lightweight status-token guard to reject stale callbacks. Broader session lifecycle tracking, duplicate-run locking, turn correlation, extension run locking, and active-run snapshot state are intentionally not part of this model.
Tests
Validation:
npm testfromplugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas, plus a local Playwright walkthrough of the reloaded wizard runtime.