From 9ddfe22ec78b1f62cff260a89ec1c672b08fc199 Mon Sep 17 00:00:00 2001 From: Giorgio Ughini Date: Mon, 21 Sep 2026 10:43:39 +0200 Subject: [PATCH 1/2] Add optional SKILL.md review and prepare 0.7.0 Prepare and cache exact skill text for optional review, synchronize modal and main placement state, and preserve the direct install/export flow. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- README.md | 7 + common/ipc.ts | 25 +- electron/ipc.ts | 48 +++- electron/preload.cjs | 7 +- electron/skillbuilder/builder.ts | 241 ++++++++++++++----- electron/skillbuilder/preview.test.ts | 328 ++++++++++++++++++++++++++ package-lock.json | 4 +- package.json | 4 +- src/App.css | 40 ++++ src/Library.tsx | 262 +++++++++++++++----- src/SkillReviewModal.tsx | 140 +++++++++++ src/skill-review-state.test.ts | 202 ++++++++++++++++ src/skill-review-state.ts | 119 ++++++++++ 13 files changed, 1297 insertions(+), 130 deletions(-) create mode 100644 electron/skillbuilder/preview.test.ts create mode 100644 src/SkillReviewModal.tsx create mode 100644 src/skill-review-state.test.ts create mode 100644 src/skill-review-state.ts diff --git a/README.md b/README.md index 87ca1e8..6d18797 100644 --- a/README.md +++ b/README.md @@ -40,6 +40,13 @@ form can teach the agent to submit *all* of them. 4. ✨ **Create.** From an approved analysis, generate a reusable **Skill** and/or a scheduled **Automation**. +For skills, **Review SKILL.md** optionally shows the complete, read-only file text +(including frontmatter) before installation or export. Preparing a preview writes +no skill files. Close and reopen it without regenerating; Add/Export from either +surface uses that exact preview. Editing the plan discards the preview. You can +still choose Add/Export directly without reviewing or adding a confirmation step. +Review provides visibility into the generated instructions, not a security guarantee. + ## Get started Skill Recorder is published as a **source release**: one command downloads a pinned Node.js diff --git a/common/ipc.ts b/common/ipc.ts index fac3499..50070a7 100644 --- a/common/ipc.ts +++ b/common/ipc.ts @@ -206,9 +206,23 @@ export interface SkillPlanResult { */ export type SkillPlacement = TargetPlacement; +/** Exact rendered file held in the main process until placement or discard. */ +export interface SkillPreview { + id: string; + markdown: string; +} + +export interface SkillPreviewResult { + ok: boolean; + preview?: SkillPreview; + error?: string; +} + /** Result of finalizing + placing a skill. */ export interface SkillCreateResult { ok: boolean; + /** The reviewed candidate was discarded or no longer matches the plan. */ + previewExpired?: boolean; skill?: BuiltSkill; /** Absolute path of the placed SKILL.md. */ path?: string; @@ -475,6 +489,8 @@ export const IPC = { deleteSession: "sessions:delete", exportDebugBundle: "sessions:export-debug", buildSkill: "skill:build", + prepareSkill: "skill:prepare", + discardSkillPreview: "skill:discard-preview", createSkill: "skill:create", getSkill: "skill:get", cancelSkill: "skill:cancel", @@ -602,7 +618,14 @@ export interface SkillRecorderApi { * skills folder (Scout); `"export"` prompts for a folder and downloads it there (the * only option for Cowork). Defaults to `"install"`. */ - createSkill(sessionId: string, plan: SkillPlan, placement?: SkillPlacement): Promise; + prepareSkill(sessionId: string, plan: SkillPlan): Promise; + discardSkillPreview(sessionId: string, previewId?: string): Promise<{ ok: boolean; error?: string }>; + createSkill( + sessionId: string, + plan: SkillPlan, + placement?: SkillPlacement, + previewId?: string, + ): Promise; /** Load a previously built skill for a session, if any. */ getSkill(sessionId: string): Promise; /** Abort an in-flight build. */ diff --git a/electron/ipc.ts b/electron/ipc.ts index 85b694f..c659c7b 100644 --- a/electron/ipc.ts +++ b/electron/ipc.ts @@ -26,6 +26,7 @@ import type { SkillCreateResult, SkillPlacement, SkillPlanResult, + SkillPreviewResult, } from "../common/ipc"; import { IPC } from "../common/ipc"; import type { AutomationPlan } from "../common/automation"; @@ -52,7 +53,7 @@ import { import type { SensitiveModelManager } from "./sensitive/model-manager"; import { buildRedactor, loadSensitiveReport, saveSensitiveReport, scanSession } from "./sensitive/scanner"; import { deleteSession, listSessions } from "./sessions"; -import { loadPersistedSkill, SkillBuilder, type SkillTarget } from "./skillbuilder/builder"; +import { loadPersistedSkill, SkillBuilder, SkillPreviewExpiredError, type SkillTarget } from "./skillbuilder/builder"; const log = createLogger("IPC"); @@ -428,6 +429,29 @@ export function registerIpc( } }); + ipcMain.handle( + IPC.prepareSkill, + async (_event, sessionId: string, plan: SkillPlan): Promise => { + if (!isValidSessionId(sessionId)) return { ok: false, error: "Unknown session." }; + try { + return { ok: true, preview: await builder.prepare(sessionId, plan) }; + } catch (err) { + const error = err instanceof Error ? err.message : String(err); + log.warn("prepare skill failed:", error); + return { ok: false, error }; + } + }, + ); + + ipcMain.handle(IPC.discardSkillPreview, (_event, sessionId: string, previewId?: string) => { + if (!isValidSessionId(sessionId)) return { ok: false, error: "Unknown session." }; + if (previewId !== undefined && typeof previewId !== "string") { + return { ok: false, error: "Invalid skill preview." }; + } + builder.discardPreview(sessionId, previewId); + return { ok: true }; + }); + ipcMain.handle( IPC.createSkill, async ( @@ -435,11 +459,18 @@ export function registerIpc( sessionId: string, plan?: SkillPlan, placement: SkillPlacement = "install", + previewId?: string, ): Promise => { if (!isValidSessionId(sessionId)) return { ok: false, error: "Unknown session." }; + if (placement !== "install" && placement !== "export") { + return { ok: false, error: "Unknown skill placement." }; + } + if (previewId !== undefined && typeof previewId !== "string") { + return { ok: false, error: "Invalid skill preview." }; + } try { - let target: SkillTarget = { kind: "install" }; - if (placement === "export") { + const placed = await builder.create(sessionId, plan, async (): Promise => { + if (placement === "install") return { kind: "install" }; // Export == download: let the user pick a destination folder; we drop a // ready-to-use /SKILL.md inside it. A dismissed dialog is a cancel, not an error. const win = BrowserWindow.fromWebContents(event.sender) ?? undefined; @@ -451,15 +482,16 @@ export function registerIpc( const result = win ? await dialog.showOpenDialog(win, opts) : await dialog.showOpenDialog(opts); - if (result.canceled || result.filePaths.length === 0) return { ok: false, canceled: true }; - target = { kind: "export", dir: result.filePaths[0] }; - } - const { skill, path: file } = await builder.create(sessionId, plan, target); + if (result.canceled || result.filePaths.length === 0) return null; + return { kind: "export", dir: result.filePaths[0] }; + }, previewId); + if (!placed) return { ok: false, canceled: true }; + const { skill, path: file } = placed; return { ok: true, skill, path: file, placement }; } catch (err) { const error = err instanceof Error ? err.message : String(err); log.warn("create skill failed:", error); - return { ok: false, error }; + return { ok: false, error, previewExpired: err instanceof SkillPreviewExpiredError }; } }, ); diff --git a/electron/preload.cjs b/electron/preload.cjs index dc4c798..b7dce6f 100644 --- a/electron/preload.cjs +++ b/electron/preload.cjs @@ -43,6 +43,8 @@ const IPC = { deleteSession: "sessions:delete", exportDebugBundle: "sessions:export-debug", buildSkill: "skill:build", + prepareSkill: "skill:prepare", + discardSkillPreview: "skill:discard-preview", createSkill: "skill:create", getSkill: "skill:get", cancelSkill: "skill:cancel", @@ -180,7 +182,10 @@ contextBridge.exposeInMainWorld("skillRecorder", { deleteSession: (sessionId) => ipcRenderer.invoke(IPC.deleteSession, sessionId), exportDebugBundle: (sessionId) => ipcRenderer.invoke(IPC.exportDebugBundle, sessionId), buildSkill: (input) => ipcRenderer.invoke(IPC.buildSkill, input), - createSkill: (sessionId, plan, placement) => ipcRenderer.invoke(IPC.createSkill, sessionId, plan, placement), + prepareSkill: (sessionId, plan) => ipcRenderer.invoke(IPC.prepareSkill, sessionId, plan), + discardSkillPreview: (sessionId, previewId) => ipcRenderer.invoke(IPC.discardSkillPreview, sessionId, previewId), + createSkill: (sessionId, plan, placement, previewId) => + ipcRenderer.invoke(IPC.createSkill, sessionId, plan, placement, previewId), getSkill: (sessionId) => ipcRenderer.invoke(IPC.getSkill, sessionId), cancelSkill: (sessionId) => ipcRenderer.invoke(IPC.cancelSkill, sessionId), revealSkill: (sessionId) => ipcRenderer.invoke(IPC.revealSkill, sessionId), diff --git a/electron/skillbuilder/builder.ts b/electron/skillbuilder/builder.ts index 116e121..744b7bb 100644 --- a/electron/skillbuilder/builder.ts +++ b/electron/skillbuilder/builder.ts @@ -1,4 +1,5 @@ import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; +import { randomUUID } from "node:crypto"; import os from "node:os"; import path from "node:path"; @@ -17,7 +18,7 @@ import { type SkillSubmission, } from "../../common/skill"; import { unresolvedTokens } from "../../common/values"; -import type { SkillBuildInput, SkillBuildProgress } from "../../common/ipc"; +import type { SkillBuildInput, SkillBuildProgress, SkillPreview } from "../../common/ipc"; import { requireCatalogue } from "../architectures/catalogue-registry"; import { AgentBuilder, type BaseLive } from "../builders/agent-builder"; import { createReadTools } from "../builders/read-tools"; @@ -30,6 +31,7 @@ import { createSkillBuilderTools } from "./tools"; const log = createLogger("SkillBuilder"); const TURN_TIMEOUT_MS = 180_000; +const MAX_PREVIEWS = 4; const KICKOFF_PROMPT = "Read get_analysis (and get_timeline where the tool mapping needs evidence), then call " + @@ -65,6 +67,23 @@ function isInside(root: string, dir: string): boolean { * - **export** — into a user-picked folder (a "download"), as `//SKILL.md`. */ export type SkillTarget = { kind: "install" } | { kind: "export"; dir: string }; +type SkillTargetPicker = () => Promise; + +interface PreparedSkill extends SkillPreview { + skill: BuiltSkill; + planKey: string; +} + +interface BuildOperation { + canceled: boolean; +} + +export class SkillPreviewExpiredError extends Error { + constructor() { + super("This skill preview is no longer current. Review the skill again before placing it."); + this.name = "SkillPreviewExpiredError"; + } +} interface LiveBuild extends BaseLive { sessionDir: string; @@ -101,6 +120,9 @@ export function loadPersistedSkill(sessionId: string): BuiltSkill | null { * callback and writes the final SKILL.md into the target agent's skills folder. */ export class SkillBuilder extends AgentBuilder { + private readonly previews = new Map(); + private readonly operations = new Map(); + constructor(private readonly emitProgress: (p: SkillBuildProgress) => void) { super("SkillBuilder"); } @@ -113,7 +135,8 @@ export class SkillBuilder extends AgentBuilder { const analysis = loadPersistedAnalysis(sessionId); if (!analysis) throw new Error("There is no analysis for this recording yet."); - this.active.add(sessionId); + const operation = this.begin(sessionId); + this.discardPreview(sessionId); try { const refining = Boolean(feedback && feedback.trim()); this.emit(sessionId, "start", refining ? "Refining the plan…" : "Planning the skill…"); @@ -123,82 +146,173 @@ export class SkillBuilder extends AgentBuilder { live = await this.createLive(sessionId, architecture); } const prompt = refining ? renderRefinePrompt(feedback!.trim(), live.lastPlan) : KICKOFF_PROMPT; - return await this.runProposeTurn(live, prompt); + this.requireActive(operation); + return await this.runProposeTurn(live, prompt, operation); + } finally { + this.finish(sessionId); + } + } + + /** Prepare the exact file for optional review, without writing or placing it. */ + async prepare(sessionId: string, editedPlan: SkillPlan): Promise { + const plan = SkillPlanSchema.parse(editedPlan); + const operation = this.begin(sessionId); + try { + let prepared = this.previews.get(sessionId); + if (!prepared || prepared.planKey !== JSON.stringify(plan)) { + this.discardPreview(sessionId); + prepared = await this.generate(sessionId, plan, operation); + this.requireActive(operation); + this.previews.set(sessionId, prepared); + while (this.previews.size > MAX_PREVIEWS) { + const oldest = this.previews.keys().next().value; + if (oldest === undefined) break; + this.previews.delete(oldest); + } + } + this.emit(sessionId, "done", "Skill ready for review. Nothing has been installed."); + return { id: prepared.id, markdown: prepared.markdown }; } finally { - this.active.delete(sessionId); + this.finish(sessionId); } } - /** Finalize the user-edited plan into a SKILL.md and place it. The edited plan is - * authoritative: its name/description/values/steps are used verbatim and only the - * markdown body is written by the agent (which references each fixed value by its - * `{{id}}` token; `renderSkillMarkdown` substitutes the literals). `target` picks the - * destination — installed into the agent's live skills folder, or exported (downloaded) - * to a user-picked dir. */ + discardPreview(sessionId: string, previewId?: string): void { + if (previewId === undefined || this.previews.get(sessionId)?.id === previewId) { + this.previews.delete(sessionId); + } + } + + override async cancel(sessionId: string): Promise { + const operation = this.operations.get(sessionId); + if (operation) operation.canceled = true; + await super.cancel(sessionId); + } + + override async forget(sessionId: string): Promise { + this.discardPreview(sessionId); + await this.cancel(sessionId); + await super.forget(sessionId); + } + + override async dispose(): Promise { + for (const operation of this.operations.values()) operation.canceled = true; + this.previews.clear(); + await super.dispose(); + } + + /** Place a reviewed candidate, or generate and place in one step for direct callers. + * The picker runs under the operation guard, before any direct-path generation. */ async create( sessionId: string, editedPlan?: SkillPlan, - target: SkillTarget = { kind: "install" }, - ): Promise<{ skill: BuiltSkill; path: string }> { - if (this.active.has(sessionId)) throw new Error("Wait for the current step to finish."); - let held = this.live.get(sessionId); - // Prefer the user's edited plan from the review tiles; fall back to the last - // proposed plan for older callers that don't pass one. - const plan = editedPlan ? SkillPlanSchema.parse(editedPlan) : held?.lastPlan ?? null; + target: SkillTarget | SkillTargetPicker = { kind: "install" }, + previewId?: string, + ): Promise<{ skill: BuiltSkill; path: string } | null> { + const plan = editedPlan + ? SkillPlanSchema.parse(editedPlan) + : this.live.get(sessionId)?.lastPlan ?? null; if (!plan) throw new Error("There is no plan to build from yet."); - requireTargetPlacement(plan.architecture, "skill", target.kind); - // The pool may have evicted the live conversation while the user edited the plan; - // recreate one so export always works. - if (!held) held = await this.createLive(sessionId, plan.architecture); - const live = held; - live.lastPlan = plan; - - this.active.add(sessionId); + const operation = this.begin(sessionId); try { - this.emit(sessionId, "drafting", "Writing the skill…"); - live.holder.submission = undefined; - try { - await live.copilot.sendAndWait(`${CREATE_PROMPT}\n\n${renderPlanForPrompt(plan)}`, TURN_TIMEOUT_MS); - } catch (err) { - await live.copilot.abort().catch(() => undefined); - throw new Error(`Skill build failed: ${msg(err)}`); + let prepared = previewId === undefined ? undefined : this.requirePreview(sessionId, plan, previewId); + const destination = typeof target === "function" ? await target() : target; + this.requireActive(operation); + if (!destination) return null; + requireTargetPlacement(plan.architecture, "skill", destination.kind); + if (previewId !== undefined) { + // The folder picker can outlive a discarded or replaced preview. + prepared = this.requirePreview(sessionId, plan, previewId); + } else { + this.discardPreview(sessionId); + prepared = await this.generate(sessionId, plan, operation); } - const submission = live.holder.submission as SkillSubmission | undefined; - if (!submission) throw new Error("The agent finished without submitting a skill."); - // Lint the authored body: the agent is given each value as `{{id}} — name` (never the - // literal), so it can only reference tokens. Any token that doesn't match a declared - // value would ship un-substituted, so surface it (the render leaves unknown tokens as-is). - const unknownTokens = unresolvedTokens(submission.body, plan.values); - if (unknownTokens.length) { - log.warn(`skill body references unknown value tokens: ${unknownTokens.map((t) => `{{${t}}}`).join(", ")}`); - } - // The frontmatter comes from the edited plan (authoritative); only the body is - // the agent's generated prose. allowed-tools may be tightened by the agent to the - // final steps, but never emptied below what the plan declared. - const finalSubmission: SkillSubmission = { - name: plan.name, - description: plan.description, - allowedTools: submission.allowedTools.length ? submission.allowedTools : plan.allowedTools, - body: submission.body, - }; - const built = toBuiltSkill(sessionId, plan.architecture, finalSubmission, plan); + this.requireActive(operation); + if (!prepared) throw new Error("There is no prepared skill to place."); const exportPath = - target.kind === "export" ? this.exportSkillTo(built, target.dir) : this.exportSkill(built); - const finalSkill: BuiltSkill = { ...built, exportedPath: exportPath, exportedAt: Date.now() }; - this.persist(live.sessionDir, finalSkill); + destination.kind === "export" + ? this.exportSkillTo(prepared.skill, destination.dir, prepared.markdown) + : this.exportSkill(prepared.skill, prepared.markdown); + const finalSkill: BuiltSkill = { ...prepared.skill, exportedPath: exportPath, exportedAt: Date.now() }; + this.persist(sessionDir(sessionId), finalSkill); + this.discardPreview(sessionId); this.emit( sessionId, "done", - target.kind === "export" ? `Skill exported to ${exportPath}` : `Skill added: ${exportPath}`, + destination.kind === "export" ? `Skill exported to ${exportPath}` : `Skill added: ${exportPath}`, ); return { skill: finalSkill, path: exportPath }; } finally { - this.active.delete(sessionId); + this.finish(sessionId); } } // --- internals ----------------------------------------------------------- + private begin(sessionId: string): BuildOperation { + if (this.active.has(sessionId)) throw new Error("Wait for the current step to finish."); + const operation: BuildOperation = { canceled: false }; + this.active.add(sessionId); + this.operations.set(sessionId, operation); + return operation; + } + + private finish(sessionId: string): void { + this.operations.delete(sessionId); + this.active.delete(sessionId); + } + + private requireActive(operation: BuildOperation): void { + if (operation.canceled) throw new Error("Skill build canceled."); + } + + private requirePreview(sessionId: string, plan: SkillPlan, id: string): PreparedSkill { + const prepared = this.previews.get(sessionId); + if (!prepared || prepared.id !== id || prepared.planKey !== JSON.stringify(plan)) { + throw new SkillPreviewExpiredError(); + } + return prepared; + } + + private async generate( + sessionId: string, + plan: SkillPlan, + operation: BuildOperation, + ): Promise { + let live = this.live.get(sessionId); + if (live && live.architecture !== plan.architecture) { + await this.disposeLive(sessionId); + live = undefined; + } + this.requireActive(operation); + if (!live) live = await this.createLive(sessionId, plan.architecture); + this.requireActive(operation); + live.lastPlan = plan; + this.emit(sessionId, "drafting", "Writing the skill…"); + live.holder.submission = undefined; + try { + await live.copilot.sendAndWait(`${CREATE_PROMPT}\n\n${renderPlanForPrompt(plan)}`, TURN_TIMEOUT_MS); + } catch (err) { + await live.copilot.abort().catch(() => undefined); + throw new Error(`Skill build failed: ${msg(err)}`); + } + this.requireActive(operation); + const submission = live.holder.submission as SkillSubmission | undefined; + if (!submission) throw new Error("The agent finished without submitting a skill."); + const unknownTokens = unresolvedTokens(submission.body, plan.values); + if (unknownTokens.length) { + log.warn(`skill body references unknown value tokens: ${unknownTokens.map((t) => `{{${t}}}`).join(", ")}`); + } + const finalSubmission: SkillSubmission = { + name: plan.name, + description: plan.description, + allowedTools: submission.allowedTools.length ? submission.allowedTools : plan.allowedTools, + body: submission.body, + }; + const skill = toBuiltSkill(sessionId, plan.architecture, finalSubmission, plan); + return { id: randomUUID(), skill, planKey: JSON.stringify(plan), markdown: renderSkillMarkdown(skill) }; + } + private emit(sessionId: string, phase: SkillBuildProgress["phase"], message: string): void { this.emitProgress({ sessionId, phase, message }); } @@ -254,7 +368,11 @@ export class SkillBuilder extends AgentBuilder { return live; } - private async runProposeTurn(live: LiveBuild, prompt: string): Promise { + private async runProposeTurn( + live: LiveBuild, + prompt: string, + operation: BuildOperation, + ): Promise { live.holder.plan = undefined; this.emit(live.sessionId, "working", "Thinking…"); try { @@ -263,6 +381,7 @@ export class SkillBuilder extends AgentBuilder { await live.copilot.abort().catch(() => undefined); throw new Error(`Planning failed: ${msg(err)}`); } + this.requireActive(operation); const plan = live.holder.plan; if (!plan) throw new Error("The agent finished without proposing a plan."); live.lastPlan = plan; @@ -271,7 +390,7 @@ export class SkillBuilder extends AgentBuilder { } /** Write the SKILL.md into the target agent's live skills folder; returns its path. */ - private exportSkill(skill: BuiltSkill): string { + private exportSkill(skill: BuiltSkill, markdown: string): string { const root = skillsRoot(); const name = slugifySkillName(skill.name); const prior = loadPersistedSkill(skill.sessionId); @@ -288,13 +407,13 @@ export class SkillBuilder extends AgentBuilder { } mkdirSync(dir, { recursive: true }); const file = path.join(dir, "SKILL.md"); - writeFileSync(file, renderSkillMarkdown(skill)); + writeFileSync(file, markdown); return file; } /** Export (download) the SKILL.md into a user-picked folder as `//SKILL.md`; * returns its path. Always picks a fresh, non-colliding subfolder within `baseDir`. */ - private exportSkillTo(skill: BuiltSkill, baseDir: string): string { + private exportSkillTo(skill: BuiltSkill, baseDir: string, markdown: string): string { const name = slugifySkillName(skill.name); let dir = path.join(baseDir, name); if (existsSync(dir)) { @@ -304,7 +423,7 @@ export class SkillBuilder extends AgentBuilder { } mkdirSync(dir, { recursive: true }); const file = path.join(dir, "SKILL.md"); - writeFileSync(file, renderSkillMarkdown(skill)); + writeFileSync(file, markdown); return file; } diff --git a/electron/skillbuilder/preview.test.ts b/electron/skillbuilder/preview.test.ts new file mode 100644 index 0000000..88e0c83 --- /dev/null +++ b/electron/skillbuilder/preview.test.ts @@ -0,0 +1,328 @@ +import assert from "node:assert/strict"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import test, { type TestContext } from "node:test"; + +import { CopilotClient, CopilotSession, type SessionConfig } from "@github/copilot-sdk"; + +import { AnalysisSchema } from "../../common/analysis"; +import type { SkillBuildProgress } from "../../common/ipc"; +import { renderSkillMarkdown, SkillPlanSchema, type SkillPlan } from "../../common/skill"; +import { loadPersistedSkill, SkillBuilder } from "./builder"; + +const sessionId = "preview-test"; +const plan = SkillPlanSchema.parse({ + architecture: "scout", + name: "reviewed-skill", + title: "Reviewed skill", + description: "A skill to review.", + allowedTools: ["Bash(gh *)"], + values: [{ id: "repo", name: "Repository", value: "example/repository" }], + steps: [{ kind: "calculation", title: "List issues", text: "List issues for {{repo}}.", tools: ["bash"] }], +}); + +function deferred() { + let resolve!: () => void; + const promise = new Promise((done) => { resolve = done; }); + return { promise, resolve }; +} + +function fixture( + t: TestContext, + options: { beforeTurn?: () => Promise; beforeSession?: () => Promise } = {}, +) { + const root = mkdtempSync(path.join(os.tmpdir(), "skill-preview-")); + const sessions = path.join(root, "sessions"); + const skills = path.join(root, "skills"); + const priorSessions = process.env.SKILL_RECORDER_SESSIONS_DIR; + const priorSkills = process.env.SKILL_RECORDER_SKILLS_DIR; + process.env.SKILL_RECORDER_SESSIONS_DIR = sessions; + process.env.SKILL_RECORDER_SKILLS_DIR = skills; + const progress: SkillBuildProgress[] = []; + const counts = { turns: 0, sessions: 0, aborts: 0 }; + const client = new CopilotClient(); + t.mock.method(client, "createSession", async (config: SessionConfig) => { + counts.sessions++; + await options.beforeSession?.(); + const session = new CopilotSession(); + t.mock.method(session, "abort", async () => { counts.aborts++; }); + t.mock.method(session, "disconnect", async () => undefined); + t.mock.method(session, "sendAndWait", async (input: string | { prompt: string }) => { + counts.turns++; + await options.beforeTurn?.(); + const prompt = typeof input === "string" ? input : input.prompt; + const toolName = prompt.includes("Call submit_skill") ? "submit_skill" : "propose_plan"; + const tool = config.tools?.find((candidate) => candidate.name === toolName); + assert.ok(tool); + assert.ok(tool.handler); + const payload = toolName === "submit_skill" + ? { name: "model-name", description: "Model description", allowedTools: ["Bash(gh issue *)"], body: "# Instructions\n\nList issues for {{repo}}.\n\n" } + : plan; + await tool.handler(payload, { sessionId, toolCallId: "test", toolName, arguments: payload }); + return undefined; + }); + return session; + }); + class TestBuilder extends SkillBuilder { + protected override async ensureClient(): Promise { + return client; + } + } + const builder = new TestBuilder((event) => progress.push(event)); + function seed(id: string) { + const dir = path.join(sessions, id); + mkdirSync(dir, { recursive: true }); + writeFileSync(path.join(dir, "analysis.json"), JSON.stringify(AnalysisSchema.parse({ + version: 1, sessionId: id, revision: 1, createdAt: 1, + intent: "List issues", intentConfidence: "high", intentRationale: "Used gh", steps: [], + }))); + } + seed(sessionId); + t.after(async () => { + await builder.dispose(); + if (priorSessions === undefined) delete process.env.SKILL_RECORDER_SESSIONS_DIR; + else process.env.SKILL_RECORDER_SESSIONS_DIR = priorSessions; + if (priorSkills === undefined) delete process.env.SKILL_RECORDER_SKILLS_DIR; + else process.env.SKILL_RECORDER_SKILLS_DIR = priorSkills; + rmSync(root, { recursive: true, force: true }); + }); + return { builder, root, sessions, skills, counts, progress, seed }; +} + +test("preview renders the entire final file without installing or marking the session complete", async (t) => { + const f = fixture(t); + const preview = await f.builder.prepare(sessionId, plan); + assert.match(preview.markdown, /^---\nname: reviewed-skill\n/); + assert.match(preview.markdown, /allowed-tools:\n - Bash\(gh issue \*\)/); + assert.match(preview.markdown, /List issues for example\/repository/); + assert.match(preview.markdown, /\n" }; +const reviewed = () => reduce( + reduce(initialSkillReviewState(false), { type: "visibility", open: true }), + { type: "preview", preview }, +); + +test("direct placement skips review and finishes on the shared done surface", () => { + const creating = reduce(initialSkillReviewState(false), { type: "start", phase: "creating", message: "Writing…" }); + assert.equal(creating.reviewOpen, false); + assert.equal(creating.preview, null); + const done = reduce(creating, { type: "done" }); + assert.equal(done.phase, "done"); + assert.equal(done.reviewOpen, false); + assert.equal(skillReviewBusy(done.phase), false); +}); + +test("close and reopen retain the exact candidate and do not change operation status", () => { + const closed = reduce(reviewed(), { type: "visibility", open: false }); + assert.equal(closed.preview, preview); + assert.equal(closed.phase, "plan"); + const opened = reduce(closed, { type: "visibility", open: true }); + assert.equal(opened.preview, preview); + assert.equal(opened.preview?.markdown, preview.markdown); + const preparing = reduce(opened, { type: "start", phase: "preparing", message: "Preparing…" }); + const hidden = reduce(preparing, { type: "visibility", open: false }); + assert.equal(hidden.phase, "preparing"); + assert.equal(skillReviewBusy(hidden.phase), true); + assert.equal(reduce(hidden, { type: "preview", preview }).reviewOpen, false); +}); + +test("export cancellation preserves the open review and candidate without an error", () => { + const creating = reduce(reviewed(), { type: "start", phase: "creating", message: "Exporting…" }); + const canceled = reduce(creating, { type: "settle", phase: "plan" }); + assert.equal(canceled.preview, preview); + assert.equal(canceled.reviewOpen, true); + assert.equal(canceled.error, null); + assert.equal(canceled.phase, "plan"); +}); + +test("placement failure retains the candidate and shared error for retry from either surface", () => { + for (const open of [true, false]) { + const ready = reduce(reviewed(), { type: "visibility", open }); + const failed = reduce(ready, { type: "settle", phase: "plan", error: "Write failed" }); + assert.equal(failed.preview, preview); + assert.equal(failed.error, "Write failed"); + assert.equal(failed.reviewOpen, open); + const retrying = reduce(failed, { type: "start", phase: "creating", message: "Writing…" }); + assert.equal(retrying.error, null); + const done = reduce(retrying, { type: "done" }); + assert.equal(done.phase, "done"); + assert.equal(done.reviewOpen, false); + assert.equal(done.preview, null); + } +}); + +test("plan invalidation drops the candidate without blocking edits", () => { + const invalidated = reduce(reviewed(), { type: "invalidate" }); + assert.equal(invalidated.preview, null); + assert.equal(invalidated.phase, "plan"); + assert.equal(skillReviewBusy(invalidated.phase), false); +}); + +test("expired placement clears the local candidate while retaining the error and explicit review retry", () => { + for (const open of [true, false]) { + const ready = reduce(reviewed(), { type: "visibility", open }); + const creating = reduce(ready, { type: "start", phase: "creating", message: "Writing…" }); + const expired = reduce(creating, { + type: "settle", phase: "plan", previewExpired: true, error: "Preview expired. Review again.", + }); + assert.equal(expired.preview, null); + assert.equal(expired.reviewOpen, open); + assert.equal(expired.error, "Preview expired. Review again."); + assert.equal(expired.phase, "plan"); + assert.equal(skillReviewBusy(expired.phase), false); + const reopened = reduce(reduce(expired, { type: "visibility", open: false }), { type: "visibility", open: true }); + assert.equal(reopened.preview, null); + assert.equal(reopened.error, expired.error); + const retry = reduce(reopened, { type: "start", phase: "preparing", message: "Preparing…" }); + assert.equal(retry.phase, "preparing"); + assert.equal(retry.error, null); + } +}); + +test("generation failure and cancellation return to the plan without accepting a preview", () => { + const preparing = reduce(initialSkillReviewState(false), { type: "start", phase: "preparing", message: "Preparing…" }); + const failed = reduce(preparing, { type: "settle", phase: "plan", error: "IPC failed" }); + assert.equal(failed.phase, "plan"); + assert.equal(failed.preview, null); + assert.equal(failed.error, "IPC failed"); + const stopping = reduce(preparing, { type: "start", phase: "stopping", message: "Stopping…" }); + assert.equal(skillReviewBusy(stopping.phase), true); + assert.equal(reduce(stopping, { type: "settle", phase: "plan" }).error, null); +}); + +test("synchronous operation guard rejects double clicks and releases only the matching operation", () => { + const guard = new SkillOperationGuard(); + const first = guard.begin()!; + assert.equal(guard.begin(), null); + assert.equal(guard.isCurrent(first), true); + guard.finish(Symbol()); + assert.equal(guard.busy, true); + guard.finish(first); + assert.equal(guard.busy, false); + assert.ok(guard.begin()); +}); + +test("cancellation or departure makes late results stale without unlocking newer work", () => { + const guard = new SkillOperationGuard(); + const old = guard.begin()!; + assert.equal(guard.invalidate(), true); + const next = guard.begin()!; + assert.equal(guard.isCurrent(old), false); + guard.finish(old); + assert.equal(guard.isCurrent(next), true); + assert.equal(guard.begin(), null); + guard.invalidate(); + assert.equal(guard.isCurrent(next), false); + assert.equal(guard.invalidate(), false); +}); + +test("cancellation acknowledgement cannot release the lock before the original request settles", async () => { + const guard = new SkillOperationGuard(); + const original = guard.begin()!; + const { originalSettled, operation: cancellation } = guard.beginCancellation()!; + let stopped = false; + const stop = (async () => { + await Promise.resolve(); // cancelSkill acknowledged, but generation/picker is still pending. + await originalSettled; + stopped = true; + guard.finish(cancellation); + })(); + await Promise.resolve(); + await Promise.resolve(); + assert.equal(stopped, false); + assert.equal(guard.busy, true); + assert.equal(guard.begin(), null); + assert.equal(guard.isCurrent(original), false); + guard.finish(original); + await stop; + assert.equal(stopped, true); + assert.equal(guard.busy, false); + assert.ok(guard.begin()); +}); + +test("committed placement wins a cancellation race and keeps the shared done screen", async () => { + const guard = new SkillOperationGuard(); + const placement = guard.begin()!; + let state = reduce(reviewed(), { type: "start", phase: "creating", message: "Writing…" }); + const cancellation = guard.beginCancellation()!; + state = reduce(state, { type: "start", phase: "stopping", message: "Stopping…" }); + assert.equal(guard.isCurrent(placement), false); + // createSkill reports success after the synchronous write beat cancellation. + assert.equal(guard.commit(placement), true); + state = reduce(state, { type: "done" }); + guard.finish(placement); + await cancellation.originalSettled; + // The cancellation continuation must not put the completed UI back on its plan. + if (guard.isCurrent(cancellation.operation)) { + state = reduce(state, { type: "settle", phase: "plan" }); + } + guard.finish(cancellation.operation); + assert.equal(state.phase, "done"); + assert.equal(state.preview, null); + assert.equal(state.reviewOpen, false); + assert.equal(guard.busy, false); +}); + +test("departed or superseded placements cannot report a committed success", () => { + const guard = new SkillOperationGuard(); + const old = guard.begin()!; + guard.beginCancellation(); + guard.invalidate(); + const next = guard.begin()!; + assert.equal(guard.commit(old), false); + assert.equal(guard.isCurrent(next), true); + guard.finish(old); + assert.equal(guard.isCurrent(next), true); +}); + +test("departed operation completion does not unlock a fresh builder instance", async () => { + const previousView = new SkillOperationGuard(); + const departed = previousView.begin()!; + const settled = previousView.whenSettled(); + previousView.invalidate(); + const freshView = new SkillOperationGuard(); + const current = freshView.begin()!; + previousView.finish(departed); + await settled; + assert.equal(previousView.isCurrent(departed), false); + assert.equal(freshView.isCurrent(current), true); + assert.equal(freshView.begin(), null); +}); diff --git a/src/skill-review-state.ts b/src/skill-review-state.ts new file mode 100644 index 0000000..63de041 --- /dev/null +++ b/src/skill-review-state.ts @@ -0,0 +1,119 @@ +import type { SkillPreview } from "../common/ipc"; + +export type SkillPhase = "loading" | "ready" | "planning" | "plan" | "preparing" | "creating" | "stopping" | "done"; + +export interface SkillReviewState { + phase: SkillPhase; + preview: SkillPreview | null; + reviewOpen: boolean; + statusLine: string; + error: string | null; +} + +export type SkillReviewAction = + | { type: "start"; phase: "planning" | "preparing" | "creating" | "stopping"; message: string } + | { type: "settle"; phase: "ready" | "plan"; error?: string; previewExpired?: boolean } + | { type: "preview"; preview: SkillPreview } + | { type: "visibility"; open: boolean } + | { type: "invalidate" } + | { type: "progress"; message: string } + | { type: "error"; error: string } + | { type: "done" }; + +export function initialSkillReviewState(hasSkill: boolean): SkillReviewState { + return { phase: hasSkill ? "loading" : "ready", preview: null, reviewOpen: false, statusLine: "", error: null }; +} + +export function skillReviewBusy(phase: SkillPhase): boolean { + return phase === "planning" || phase === "preparing" || phase === "creating" || phase === "stopping"; +} + +export function skillReviewReducer(state: SkillReviewState, action: SkillReviewAction): SkillReviewState { + switch (action.type) { + case "start": + return { ...state, phase: action.phase, statusLine: action.message, error: null }; + case "settle": + return { + ...state, + phase: action.phase, + preview: action.previewExpired ? null : state.preview, + statusLine: "", + error: action.error ?? null, + }; + case "preview": + return { ...state, phase: "plan", preview: action.preview, statusLine: "", error: null }; + case "visibility": + return { ...state, reviewOpen: action.open }; + case "invalidate": + return { ...state, preview: null }; + case "progress": + return { ...state, statusLine: action.message }; + case "error": + return { ...state, error: action.error }; + case "done": + return { ...state, phase: "done", preview: null, reviewOpen: false, statusLine: "", error: null }; + } +} + +// React state updates are deferred; acquire this guard before the first await. +export class SkillOperationGuard { + private active: symbol | null = null; + private canceledSource: symbol | null = null; + private pending = new Map; resolve: () => void }>(); + + begin(): symbol | null { + if (this.active) return null; + this.active = Symbol(); + let resolve!: () => void; + const promise = new Promise((done) => { resolve = done; }); + this.pending.set(this.active, { promise, resolve }); + return this.active; + } + + isCurrent(operation: symbol): boolean { + return this.active === operation; + } + + beginCancellation(): { operation: symbol; originalSettled: Promise } | null { + if (!this.active || this.canceledSource) return null; + const original = this.active; + const originalSettled = this.whenSettled(); + this.active = null; + const operation = this.begin()!; + this.canceledSource = original; + return { operation, originalSettled }; + } + + // Cancellation cannot undo a synchronous file write that already committed. + // Departure invalidates both identities, so it still rejects late successes. + commit(operation: symbol): boolean { + if (!this.isCurrent(operation) && this.canceledSource !== operation) return false; + this.active = null; + this.canceledSource = null; + return true; + } + + finish(operation: symbol): void { + this.pending.get(operation)?.resolve(); + this.pending.delete(operation); + if (this.isCurrent(operation)) { + this.active = null; + this.canceledSource = null; + } + } + + whenSettled(): Promise { + return (this.active && this.pending.get(this.active)?.promise) || Promise.resolve(); + } + + invalidate(): boolean { + const wasActive = this.active !== null; + this.active = null; + this.canceledSource = null; + return wasActive; + } + + get busy(): boolean { + return this.active !== null; + } +} From 8bd2b6eab6d3473ceb1061227ba0d48554874581 Mon Sep 17 00:00:00 2001 From: Giorgio Ughini Date: Mon, 21 Sep 2026 10:56:01 +0200 Subject: [PATCH 2/2] Avoid cold cmdlet startup in installer lock test Use Thread.Sleep while the helper owns the simulated scanner lock so PowerShell cmdlet discovery cannot consume the retry window. Keep the installer and retry assertions unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- scripts/install-windows.test.ps1 | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/install-windows.test.ps1 b/scripts/install-windows.test.ps1 index f4092e2..fcfe8fd 100644 --- a/scripts/install-windows.test.ps1 +++ b/scripts/install-windows.test.ps1 @@ -253,7 +253,8 @@ try { ) try { [IO.File]::WriteAllText('$escapedReadyFile', 'ready') - Start-Sleep -Milliseconds 500 + # Avoid cold cmdlet discovery extending the lock beyond the retry window. + [Threading.Thread]::Sleep(500) } finally { `$stream.Dispose() }