From 280653d925f70264e74c7031b13084e2fa5fd9b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?T=C3=82CHES?= Date: Sat, 21 Mar 2026 11:41:52 -0600 Subject: [PATCH] fix: share milestone ID reservation between preview and tool (#1569) (#1802) --- .../extensions/gsd/bootstrap/db-tools.ts | 18 +++-- src/resources/extensions/gsd/guided-flow.ts | 31 ++++++-- src/resources/extensions/gsd/milestone-ids.ts | 38 ++++++++++ .../tests/milestone-id-reservation.test.ts | 73 +++++++++++++++++++ 4 files changed, 147 insertions(+), 13 deletions(-) create mode 100644 src/resources/extensions/gsd/tests/milestone-id-reservation.test.ts diff --git a/src/resources/extensions/gsd/bootstrap/db-tools.ts b/src/resources/extensions/gsd/bootstrap/db-tools.ts index 2e55ff6cc..4b751abce 100644 --- a/src/resources/extensions/gsd/bootstrap/db-tools.ts +++ b/src/resources/extensions/gsd/bootstrap/db-tools.ts @@ -1,7 +1,7 @@ import { Type } from "@sinclair/typebox"; import type { ExtensionAPI } from "@gsd/pi-coding-agent"; -import { findMilestoneIds, nextMilestoneId } from "../guided-flow.js"; +import { findMilestoneIds, nextMilestoneId, claimReservedId, getReservedMilestoneIds } from "../guided-flow.js"; import { loadEffectiveGSDPreferences } from "../preferences.js"; import { ensureDbOpen } from "./dynamic-tools.js"; @@ -197,7 +197,6 @@ export function registerDbTools(pi: ExtensionAPI): void { }, }); - const reservedMilestoneIds = new Set(); pi.registerTool({ name: "gsd_generate_milestone_id", label: "Generate Milestone ID", @@ -215,15 +214,24 @@ export function registerDbTools(pi: ExtensionAPI): void { parameters: Type.Object({}), async execute(_toolCallId, _params, _signal, _onUpdate, _ctx) { try { + // Claim a reserved ID if the guided-flow already previewed one to the user. + // This guarantees the ID shown in the UI matches the one materialised on disk. + const reserved = claimReservedId(); + if (reserved) { + return { + content: [{ type: "text" as const, text: reserved }], + details: { operation: "generate_milestone_id", id: reserved, source: "reserved" } as any, + }; + } + const basePath = process.cwd(); const existingIds = findMilestoneIds(basePath); const uniqueEnabled = !!loadEffectiveGSDPreferences()?.preferences?.unique_milestone_ids; - const allIds = [...new Set([...existingIds, ...reservedMilestoneIds])]; + const allIds = [...new Set([...existingIds, ...getReservedMilestoneIds()])]; const newId = nextMilestoneId(allIds, uniqueEnabled); - reservedMilestoneIds.add(newId); return { content: [{ type: "text" as const, text: newId }], - details: { operation: "generate_milestone_id", id: newId, existingCount: existingIds.length, reservedCount: reservedMilestoneIds.size, uniqueEnabled } as any, + details: { operation: "generate_milestone_id", id: newId, existingCount: existingIds.length, uniqueEnabled } as any, }; } catch (err) { const msg = err instanceof Error ? err.message : String(err); diff --git a/src/resources/extensions/gsd/guided-flow.ts b/src/resources/extensions/gsd/guided-flow.ts index 983e42b4d..473b0e669 100644 --- a/src/resources/extensions/gsd/guided-flow.ts +++ b/src/resources/extensions/gsd/guided-flow.ts @@ -33,7 +33,7 @@ import { showProjectInit, offerMigration } from "./init-wizard.js"; import { validateDirectory } from "./validate-directory.js"; import { showConfirm } from "../shared/mod.js"; import { debugLog } from "./debug-logger.js"; -import { findMilestoneIds, nextMilestoneId } from "./milestone-ids.js"; +import { findMilestoneIds, nextMilestoneId, reserveMilestoneId, getReservedMilestoneIds } from "./milestone-ids.js"; import { parkMilestone, discardMilestone } from "./milestone-actions.js"; import { resolveModelWithFallbacksForUnit } from "./preferences-models.js"; @@ -42,6 +42,7 @@ export { MILESTONE_ID_RE, generateMilestoneSuffix, nextMilestoneId, extractMilestoneSeq, parseMilestoneId, milestoneIdSort, maxMilestoneNum, findMilestoneIds, + reserveMilestoneId, claimReservedId, getReservedMilestoneIds, clearReservedMilestoneIds, } from "./milestone-ids.js"; export { showQueue, handleQueueReorder, showQueueAdd, @@ -49,6 +50,20 @@ export { } from "./guided-flow-queue.js"; import { getErrorMessage } from "./error-utils.js"; +// ─── ID Generation with Reservation ───────────────────────────────────────── + +/** + * Generate the next milestone ID, accounting for reserved IDs, and reserve it. + * Ensures any preview ID shown in the UI matches what `gsd_generate_milestone_id` + * will later return. + */ +function nextMilestoneIdReserved(existingIds: string[], uniqueEnabled: boolean): string { + const allIds = [...new Set([...existingIds, ...getReservedMilestoneIds()])]; + const id = nextMilestoneId(allIds, uniqueEnabled); + reserveMilestoneId(id); + return id; +} + // ─── Commit Instruction Helpers ────────────────────────────────────────────── /** Build commit instruction for planning prompts. .gsd/ is managed externally and always gitignored. */ @@ -362,7 +377,7 @@ export async function showHeadlessMilestoneCreation( // Generate next milestone ID const existingIds = findMilestoneIds(basePath); const prefs = loadEffectiveGSDPreferences(); - const nextId = nextMilestoneId(existingIds, prefs?.preferences?.unique_milestone_ids ?? false); + const nextId = nextMilestoneIdReserved(existingIds, prefs?.preferences?.unique_milestone_ids ?? false); // Create milestone directory const milestoneDir = join(gsdRoot(basePath), "milestones", nextId, "slices"); @@ -552,7 +567,7 @@ export async function showDiscuss( } else if (choice === "skip_milestone") { const milestoneIds = findMilestoneIds(basePath); const uniqueMilestoneIds = !!loadEffectiveGSDPreferences()?.preferences?.unique_milestone_ids; - const nextId = nextMilestoneId(milestoneIds, uniqueMilestoneIds); + const nextId = nextMilestoneIdReserved(milestoneIds, uniqueMilestoneIds); pendingAutoStart = { ctx, pi, basePath, milestoneId: nextId, step: false }; await dispatchWorkflow(pi, buildDiscussPrompt(nextId, `New milestone ${nextId}.`, basePath), "gsd-run", ctx, "plan-milestone"); } @@ -793,7 +808,7 @@ async function handleMilestoneActions( if (choice === "skip") { const milestoneIds = findMilestoneIds(basePath); const uniqueMilestoneIds = !!loadEffectiveGSDPreferences()?.preferences?.unique_milestone_ids; - const nextId = nextMilestoneId(milestoneIds, uniqueMilestoneIds); + const nextId = nextMilestoneIdReserved(milestoneIds, uniqueMilestoneIds); pendingAutoStart = { ctx, pi, basePath, milestoneId: nextId, step: stepMode }; await dispatchWorkflow(pi, buildDiscussPrompt(nextId, `New milestone ${nextId}.`, @@ -933,7 +948,7 @@ export async function showSmartEntry( } const uniqueMilestoneIds = !!loadEffectiveGSDPreferences()?.preferences?.unique_milestone_ids; - const nextId = nextMilestoneId(milestoneIds, uniqueMilestoneIds); + const nextId = nextMilestoneIdReserved(milestoneIds, uniqueMilestoneIds); const isFirst = milestoneIds.length === 0; if (isFirst) { @@ -996,7 +1011,7 @@ export async function showSmartEntry( if (choice === "new_milestone") { const milestoneIds = findMilestoneIds(basePath); const uniqueMilestoneIds = !!loadEffectiveGSDPreferences()?.preferences?.unique_milestone_ids; - const nextId = nextMilestoneId(milestoneIds, uniqueMilestoneIds); + const nextId = nextMilestoneIdReserved(milestoneIds, uniqueMilestoneIds); pendingAutoStart = { ctx, pi, basePath, milestoneId: nextId, step: stepMode }; await dispatchWorkflow(pi, buildDiscussPrompt(nextId, @@ -1062,7 +1077,7 @@ export async function showSmartEntry( } else if (choice === "skip_milestone") { const milestoneIds = findMilestoneIds(basePath); const uniqueMilestoneIds = !!loadEffectiveGSDPreferences()?.preferences?.unique_milestone_ids; - const nextId = nextMilestoneId(milestoneIds, uniqueMilestoneIds); + const nextId = nextMilestoneIdReserved(milestoneIds, uniqueMilestoneIds); pendingAutoStart = { ctx, pi, basePath, milestoneId: nextId, step: stepMode }; await dispatchWorkflow(pi, buildDiscussPrompt(nextId, `New milestone ${nextId}.`, @@ -1146,7 +1161,7 @@ export async function showSmartEntry( } else if (choice === "skip_milestone") { const milestoneIds = findMilestoneIds(basePath); const uniqueMilestoneIds = !!loadEffectiveGSDPreferences()?.preferences?.unique_milestone_ids; - const nextId = nextMilestoneId(milestoneIds, uniqueMilestoneIds); + const nextId = nextMilestoneIdReserved(milestoneIds, uniqueMilestoneIds); pendingAutoStart = { ctx, pi, basePath, milestoneId: nextId, step: stepMode }; await dispatchWorkflow(pi, buildDiscussPrompt(nextId, `New milestone ${nextId}.`, diff --git a/src/resources/extensions/gsd/milestone-ids.ts b/src/resources/extensions/gsd/milestone-ids.ts index fdd26f7ab..286f16809 100644 --- a/src/resources/extensions/gsd/milestone-ids.ts +++ b/src/resources/extensions/gsd/milestone-ids.ts @@ -70,6 +70,44 @@ export function nextMilestoneId(milestoneIds: string[], uniqueEnabled?: boolean) return `M${seq}`; } +// ─── Reservation ───────────────────────────────────────────────────────────── + +/** + * Module-level set of milestone IDs that have been previewed/promised to the + * user but not yet materialised on disk. Both guided-flow (preview) and + * gsd_generate_milestone_id (tool) share this set so the ID shown in the UI + * matches the one the tool returns. + */ +const reservedMilestoneIds = new Set(); + +/** Reserve an ID so that subsequent calls to `claimReservedId` / `nextMilestoneId` account for it. */ +export function reserveMilestoneId(id: string): void { + reservedMilestoneIds.add(id); +} + +/** + * If any IDs have been reserved, shift one out and return it. + * Returns `undefined` when the reservation set is empty. + */ +export function claimReservedId(): string | undefined { + const first = reservedMilestoneIds.values().next().value; + if (first !== undefined) { + reservedMilestoneIds.delete(first); + return first; + } + return undefined; +} + +/** Return a snapshot of all currently reserved IDs (for merging into the "existing" list). */ +export function getReservedMilestoneIds(): ReadonlySet { + return reservedMilestoneIds; +} + +/** Clear all reservations (useful for tests). */ +export function clearReservedMilestoneIds(): void { + reservedMilestoneIds.clear(); +} + // ─── Discovery ────────────────────────────────────────────────────────────── /** Scan the milestones directory and return IDs sorted by queue order (or numeric fallback). */ diff --git a/src/resources/extensions/gsd/tests/milestone-id-reservation.test.ts b/src/resources/extensions/gsd/tests/milestone-id-reservation.test.ts new file mode 100644 index 000000000..814576205 --- /dev/null +++ b/src/resources/extensions/gsd/tests/milestone-id-reservation.test.ts @@ -0,0 +1,73 @@ +// milestone-id-reservation — Verifies that preview IDs from guided-flow +// match the IDs claimed by gsd_generate_milestone_id via the shared +// reservation mechanism in milestone-ids.ts. +// +// Regression test for #1569. + +import { describe, it, beforeEach } from 'node:test'; +import assert from 'node:assert/strict'; + +import { + nextMilestoneId, + reserveMilestoneId, + claimReservedId, + getReservedMilestoneIds, + clearReservedMilestoneIds, +} from '../milestone-ids.ts'; + +describe('milestone ID reservation (#1569)', () => { + beforeEach(() => { + clearReservedMilestoneIds(); + }); + + it('claimReservedId returns undefined when nothing is reserved', () => { + assert.equal(claimReservedId(), undefined); + }); + + it('reserved ID is returned by claimReservedId and removed from the set', () => { + const id = nextMilestoneId([], true); + reserveMilestoneId(id); + + assert.equal(getReservedMilestoneIds().size, 1); + assert.equal(claimReservedId(), id); + assert.equal(getReservedMilestoneIds().size, 0); + // Second claim returns undefined + assert.equal(claimReservedId(), undefined); + }); + + it('reserved IDs are visible in getReservedMilestoneIds', () => { + reserveMilestoneId('M001-abc123'); + reserveMilestoneId('M002-def456'); + const reserved = getReservedMilestoneIds(); + assert.equal(reserved.size, 2); + assert.ok(reserved.has('M001-abc123')); + assert.ok(reserved.has('M002-def456')); + }); + + it('clearReservedMilestoneIds empties the set', () => { + reserveMilestoneId('M001-abc123'); + clearReservedMilestoneIds(); + assert.equal(getReservedMilestoneIds().size, 0); + }); + + it('nextMilestoneId accounts for reserved IDs in sequence numbering', () => { + // Simulate: guided-flow previews M001, reserves it + const existing: string[] = []; + const preview = nextMilestoneId(existing, true); + assert.match(preview, /^M001-/); + reserveMilestoneId(preview); + + // Now generate the next one accounting for reservations + const allIds = [...new Set([...existing, ...getReservedMilestoneIds()])]; + const second = nextMilestoneId(allIds, true); + assert.match(second, /^M002-/); + }); + + it('claim returns IDs in insertion order (FIFO)', () => { + reserveMilestoneId('M001-aaa111'); + reserveMilestoneId('M002-bbb222'); + assert.equal(claimReservedId(), 'M001-aaa111'); + assert.equal(claimReservedId(), 'M002-bbb222'); + assert.equal(claimReservedId(), undefined); + }); +});