Merge pull request #205 from gsd-build/fix/177-milestone-id-generation

fix: use max-based milestone ID generation instead of length+1

src/resources/extensions/gsd/guided-flow.ts

@@ -112,6 +112,19 @@ function findMilestoneIds(basePath: string): string[] {
   }
 }
 
+/** Return the highest numeric suffix among milestone IDs (0 when the list is empty or has no numeric IDs). */
+export function maxMilestoneNum(milestoneIds: string[]): number {
+  return milestoneIds.reduce((max, id) => {
+    const num = parseInt(id.replace(/^M/, ""), 10);
+    return num > max ? num : max;
+  }, 0);
+}
+
+/** Derive the next milestone ID from existing IDs using max-based approach to avoid collisions after deletions. */
+export function nextMilestoneId(milestoneIds: string[]): string {
+  return `M${String(maxMilestoneNum(milestoneIds) + 1).padStart(3, "0")}`;
+}
+
 // ─── Queue ─────────────────────────────────────────────────────────────────────
 
 /**
@@ -153,12 +166,9 @@ export async function showQueue(
   const existingContext = await buildExistingMilestonesContext(basePath, milestoneIds, state);
 
   // ── Determine next milestone ID ─────────────────────────────────────
-  const maxNum = milestoneIds.reduce((max, id) => {
-    const num = parseInt(id.replace(/^M/, ""), 10);
-    return num > max ? num : max;
-  }, 0);
-  const nextId = `M${String(maxNum + 1).padStart(3, "0")}`;
-  const nextIdPlus1 = `M${String(maxNum + 2).padStart(3, "0")}`;
+  const max = maxMilestoneNum(milestoneIds);
+  const nextId = `M${String(max + 1).padStart(3, "0")}`;
+  const nextIdPlus1 = `M${String(max + 2).padStart(3, "0")}`;
 
   // ── Build preamble ──────────────────────────────────────────────────
   const activePart = state.activeMilestone
@@ -508,7 +518,7 @@ export async function showSmartEntry(
     }
 
     const milestoneIds = findMilestoneIds(basePath);
-    const nextId = `M${String(milestoneIds.length + 1).padStart(3, "0")}`;
+    const nextId = nextMilestoneId(milestoneIds);
     const isFirst = milestoneIds.length === 0;
 
     if (isFirst) {
@@ -570,7 +580,7 @@ export async function showSmartEntry(
 
     if (choice === "new_milestone") {
       const milestoneIds = findMilestoneIds(basePath);
-      const nextId = `M${String(milestoneIds.length + 1).padStart(3, "0")}`;
+      const nextId = nextMilestoneId(milestoneIds);
 
       pendingAutoStart = { ctx, pi, basePath, milestoneId: nextId, step: stepMode };
       dispatchWorkflow(pi, buildDiscussPrompt(nextId,
@@ -638,7 +648,7 @@ export async function showSmartEntry(
         }));
       } else if (choice === "skip_milestone") {
         const milestoneIds = findMilestoneIds(basePath);
-        const nextId = `M${String(milestoneIds.length + 1).padStart(3, "0")}`;
+        const nextId = nextMilestoneId(milestoneIds);
         pendingAutoStart = { ctx, pi, basePath, milestoneId: nextId, step: stepMode };
         dispatchWorkflow(pi, buildDiscussPrompt(nextId,
           `New milestone ${nextId}.`,

src/resources/extensions/gsd/tests/next-milestone-id.test.ts

@@ -0,0 +1,87 @@
+// Tests for nextMilestoneId and maxMilestoneNum — milestone ID generation
+// using max-based approach to avoid collisions after deletions.
+//
+// Sections:
+//   (a) Empty array returns M001
+//   (b) Sequential IDs return next in sequence
+//   (c) IDs with gaps (deletion) use max, not fill
+//   (d) Non-numeric directory names mixed in are ignored
+
+import { nextMilestoneId, maxMilestoneNum } from '../guided-flow.ts';
+
+// ─── Assertion helpers ─────────────────────────────────────────────────────
+
+let passed = 0;
+let failed = 0;
+
+function assertEq<T>(actual: T, expected: T, message: string): void {
+  if (JSON.stringify(actual) === JSON.stringify(expected)) {
+    passed++;
+  } else {
+    failed++;
+    console.error(`  FAIL: ${message} — expected ${JSON.stringify(expected)}, got ${JSON.stringify(actual)}`);
+  }
+}
+
+// ─── Tests ─────────────────────────────────────────────────────────────────
+
+async function main(): Promise<void> {
+  console.log('nextMilestoneId / maxMilestoneNum tests');
+
+  // (a) Empty array → M001
+  {
+    assertEq(maxMilestoneNum([]), 0, 'maxMilestoneNum([]) === 0');
+    assertEq(nextMilestoneId([]), 'M001', 'nextMilestoneId([]) === "M001"');
+  }
+
+  // (b) Sequential IDs → next in sequence
+  {
+    assertEq(
+      nextMilestoneId(['M001', 'M002', 'M003']),
+      'M004',
+      'sequential IDs return M004',
+    );
+    assertEq(maxMilestoneNum(['M001', 'M002', 'M003']), 3, 'max of sequential is 3');
+  }
+
+  // (c) IDs with gaps (deletion scenario) → uses max, not fill
+  {
+    assertEq(
+      nextMilestoneId(['M001', 'M003']),
+      'M004',
+      'gap scenario returns M004, not M002',
+    );
+    assertEq(maxMilestoneNum(['M001', 'M003']), 3, 'max with gap is 3');
+  }
+
+  // (d) Non-numeric directory names mixed in are ignored
+  {
+    assertEq(
+      nextMilestoneId(['M001', 'notes', '.DS_Store', 'M003']),
+      'M004',
+      'non-numeric names ignored, returns M004',
+    );
+    assertEq(
+      maxMilestoneNum(['M001', 'notes', '.DS_Store', 'M003']),
+      3,
+      'max ignores non-numeric entries',
+    );
+  }
+
+  // ═══════════════════════════════════════════════════════════════════════════
+  // Results
+  // ═══════════════════════════════════════════════════════════════════════════
+
+  console.log(`\n${'='.repeat(40)}`);
+  console.log(`Results: ${passed} passed, ${failed} failed`);
+  if (failed > 0) {
+    process.exit(1);
+  } else {
+    console.log('All tests passed');
+  }
+}
+
+main().catch((error) => {
+  console.error(error);
+  process.exit(1);
+});