feat(test-llm-autocompletion): use globally unique file numbers for approvals

- Modified getNextFileNumber to use globally unique numbers across both
  approved and rejected files (e.g., approved.1.txt, rejected.2.txt)
- Added renumberApprovals function to fix duplicate file numbers
- Updated clean command to include renumbering functionality
- Added tests for the new global numbering and renumbering features
- Updated README to document the changes

Author: markijbema

src/test-llm-autocompletion/README.md

@@ -21,7 +21,8 @@ This test suite uses approval testing instead of regex pattern matching to valid
 
     - Display the test input and output
     - Ask you whether the output is acceptable
-    - Save your decision to `approvals/{category}/{test-name}/approved.N.txt` or `rejected.N.txt`
+    - Save your decision to `approvals/{category}/{test-name}.approved.N.txt` or `{test-name}.rejected.N.txt`
+    - File numbers are globally unique across approved and rejected files (e.g., `approved.1.txt`, `rejected.2.txt`, `approved.3.txt`)
 
 2. **Subsequent Runs**:
     - If the output matches a previously approved file, the test passes
@@ -95,7 +96,7 @@ LLM_MODEL=anthropic/claude-3-haiku pnpm run test
 
 ### Clean Command
 
-The `clean` command removes approval files for test cases that no longer exist:
+The `clean` command removes approval files for test cases that no longer exist and renumbers files to fix duplicates:
 
 ```bash
 pnpm run clean
@@ -106,7 +107,14 @@ This is useful when you've deleted or renamed test cases and want to clean up th
 - Scan all approval files in the `approvals/` directory
 - Check if each approval corresponds to an existing test case
 - Remove approvals for test cases that no longer exist
-- Report how many files were cleaned
+- **Renumber files** to fix duplicate numbers and gaps (e.g., if you have `approved.1.txt` and `rejected.1.txt`, they will be renumbered to `approved.1.txt` and `rejected.2.txt`)
+- Report how many files were cleaned and renumbered
+
+The renumbering feature is particularly useful when:
+
+- You've manually moved files between approved and rejected
+- You have legacy files with duplicate numbers from before the global numbering was implemented
+- You want to ensure all file numbers are sequential and unique
 
 ### Skip Approval Mode
 
@@ -155,6 +163,7 @@ Is this acceptable? (y/n):
 ## Notes
 
 - The `approvals/` directory is gitignored
-- Each approved/rejected output gets a unique numbered file
+- Each approved/rejected output gets a globally unique numbered file (numbers are unique across both approved and rejected files for the same test case)
 - Tests only prompt for input in the terminal when output is new
 - The test summary at the end shows how many passed/failed
+- To move a file from approved to rejected (or vice versa), simply rename it and run `pnpm run clean` to fix the numbering

src/test-llm-autocompletion/approvals.spec.ts

@@ -2,7 +2,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"
 import fs from "fs"
 import path from "path"
 import readline from "readline"
-import { checkApproval } from "./approvals.js"
+import { checkApproval, renumberApprovals } from "./approvals.js"
 
 const TEST_APPROVALS_DIR = "approvals"
 const TEST_CATEGORY = "test-category"
@@ -80,6 +80,30 @@ describe("approvals", () => {
 
 			expect(files).toContain(`${TEST_NAME}.rejected.1.txt`)
 		})
+
+		it("should use globally unique numbers across approved and rejected", async () => {
+			let callCount = 0
+			vi.spyOn(readline, "createInterface").mockReturnValue({
+				question: (_prompt: string, callback: (answer: string) => void) => {
+					// First call: approve, second call: reject, third call: approve
+					callCount++
+					callback(callCount === 2 ? "n" : "y")
+				},
+				close: () => {},
+			} as any)
+
+			await checkApproval(TEST_CATEGORY, TEST_NAME, "input1", "output1") // approved.1
+			await checkApproval(TEST_CATEGORY, TEST_NAME, "input2", "output2") // rejected.2
+			await checkApproval(TEST_CATEGORY, TEST_NAME, "input3", "output3") // approved.3
+
+			const categoryDir = path.join(TEST_APPROVALS_DIR, TEST_CATEGORY)
+			const files = fs.readdirSync(categoryDir)
+
+			expect(files).toContain(`${TEST_NAME}.approved.1.txt`)
+			expect(files).toContain(`${TEST_NAME}.rejected.2.txt`)
+			expect(files).toContain(`${TEST_NAME}.approved.3.txt`)
+			expect(files).toHaveLength(3)
+		})
 	})
 
 	describe("matching existing files", () => {
@@ -171,6 +195,71 @@ describe("approvals", () => {
 		})
 	})
 
+	describe("renumberApprovals", () => {
+		it("should renumber files with duplicate numbers", () => {
+			const categoryDir = path.join(TEST_APPROVALS_DIR, TEST_CATEGORY)
+			fs.mkdirSync(categoryDir, { recursive: true })
+
+			// Create files with duplicate numbers (approved.1 and rejected.1)
+			fs.writeFileSync(path.join(categoryDir, `${TEST_NAME}.approved.1.txt`), "output1", "utf-8")
+			fs.writeFileSync(path.join(categoryDir, `${TEST_NAME}.rejected.1.txt`), "output2", "utf-8")
+
+			const result = renumberApprovals(TEST_APPROVALS_DIR)
+
+			expect(result.renamedCount).toBeGreaterThan(0)
+
+			const files = fs.readdirSync(categoryDir)
+			const numbers = files.map((f) => {
+				const match = f.match(/\.(\d+)\.txt$/)
+				return match ? parseInt(match[1], 10) : 0
+			})
+
+			// All numbers should be unique
+			const uniqueNumbers = new Set(numbers)
+			expect(uniqueNumbers.size).toBe(numbers.length)
+		})
+
+		it("should renumber files with gaps", () => {
+			const categoryDir = path.join(TEST_APPROVALS_DIR, TEST_CATEGORY)
+			fs.mkdirSync(categoryDir, { recursive: true })
+
+			// Create files with gaps (1, 3, 5)
+			fs.writeFileSync(path.join(categoryDir, `${TEST_NAME}.approved.1.txt`), "output1", "utf-8")
+			fs.writeFileSync(path.join(categoryDir, `${TEST_NAME}.approved.3.txt`), "output2", "utf-8")
+			fs.writeFileSync(path.join(categoryDir, `${TEST_NAME}.approved.5.txt`), "output3", "utf-8")
+
+			const result = renumberApprovals(TEST_APPROVALS_DIR)
+
+			expect(result.renamedCount).toBeGreaterThan(0)
+
+			const files = fs.readdirSync(categoryDir)
+			expect(files).toContain(`${TEST_NAME}.approved.1.txt`)
+			expect(files).toContain(`${TEST_NAME}.approved.2.txt`)
+			expect(files).toContain(`${TEST_NAME}.approved.3.txt`)
+		})
+
+		it("should not rename files that are already sequential", () => {
+			const categoryDir = path.join(TEST_APPROVALS_DIR, TEST_CATEGORY)
+			fs.mkdirSync(categoryDir, { recursive: true })
+
+			// Create files that are already sequential
+			fs.writeFileSync(path.join(categoryDir, `${TEST_NAME}.approved.1.txt`), "output1", "utf-8")
+			fs.writeFileSync(path.join(categoryDir, `${TEST_NAME}.rejected.2.txt`), "output2", "utf-8")
+			fs.writeFileSync(path.join(categoryDir, `${TEST_NAME}.approved.3.txt`), "output3", "utf-8")
+
+			const result = renumberApprovals(TEST_APPROVALS_DIR)
+
+			expect(result.renamedCount).toBe(0)
+		})
+
+		it("should return zero counts for non-existent directory", () => {
+			const result = renumberApprovals("non-existent-dir")
+
+			expect(result.renamedCount).toBe(0)
+			expect(result.totalFiles).toBe(0)
+		})
+	})
+
 	describe("skip-approval mode", () => {
 		it("should mark new outputs as unknown with newOutput=true", async () => {
 			const result = await checkApproval(TEST_CATEGORY, TEST_NAME, "input", "output", true)

src/test-llm-autocompletion/approvals.ts

@@ -13,18 +13,19 @@ function getCategoryPath(category: string): string {
 	return path.join(APPROVALS_DIR, category)
 }
 
-function getNextFileNumber(categoryDir: string, testName: string, type: "approved" | "rejected"): number {
+function getNextFileNumber(categoryDir: string, testName: string): number {
 	if (!fs.existsSync(categoryDir)) {
 		return 1
 	}
 
 	const files = fs.readdirSync(categoryDir)
-	const pattern = new RegExp(`^${testName}\\.${type}\\.(\\d+)\\.txt$`)
+	// Match both approved and rejected files to get globally unique numbers
+	const pattern = new RegExp(`^${testName}\\.(approved|rejected)\\.(\\d+)\\.txt$`)
 	const numbers = files
 		.filter((f) => pattern.test(f))
 		.map((f) => {
 			const match = f.match(pattern)
-			return match ? parseInt(match[1], 10) : 0
+			return match ? parseInt(match[2], 10) : 0
 		})
 
 	return numbers.length > 0 ? Math.max(...numbers) + 1 : 1
@@ -112,11 +113,118 @@ export async function checkApproval(
 
 	fs.mkdirSync(categoryDir, { recursive: true })
 
-	const nextNumber = getNextFileNumber(categoryDir, testName, type)
+	const nextNumber = getNextFileNumber(categoryDir, testName)
 	const filename = `${testName}.${type}.${nextNumber}.txt`
 	const filePath = path.join(categoryDir, filename)
 
 	fs.writeFileSync(filePath, output, "utf-8")
 
 	return { isApproved, newOutput: true }
 }
+
+export interface RenumberResult {
+	renamedCount: number
+	totalFiles: number
+}
+
+export function renumberApprovals(approvalsDir: string = "approvals"): RenumberResult {
+	let renamedCount = 0
+	let totalFiles = 0
+
+	if (!fs.existsSync(approvalsDir)) {
+		return { renamedCount, totalFiles }
+	}
+
+	// Get all category directories
+	const categories = fs.readdirSync(approvalsDir, { withFileTypes: true }).filter((d) => d.isDirectory())
+
+	for (const category of categories) {
+		const categoryDir = path.join(approvalsDir, category.name)
+		const files = fs.readdirSync(categoryDir).filter((f) => f.endsWith(".txt"))
+
+		// Group files by test name
+		const filesByTestName = new Map<string, string[]>()
+		const pattern = /^(.+)\.(approved|rejected)\.(\d+)\.txt$/
+
+		for (const file of files) {
+			const match = file.match(pattern)
+			if (match) {
+				totalFiles++
+				const testName = match[1]
+				if (!filesByTestName.has(testName)) {
+					filesByTestName.set(testName, [])
+				}
+				filesByTestName.get(testName)!.push(file)
+			}
+		}
+
+		// Renumber files for each test name
+		for (const [testName, testFiles] of filesByTestName) {
+			// Sort files by their current number
+			const sortedFiles = testFiles.sort((a, b) => {
+				const matchA = a.match(pattern)!
+				const matchB = b.match(pattern)!
+				return parseInt(matchA[3], 10) - parseInt(matchB[3], 10)
+			})
+
+			// Check if renumbering is needed
+			const numbers = sortedFiles.map((f) => {
+				const match = f.match(pattern)!
+				return parseInt(match[3], 10)
+			})
+
+			// Check for duplicates or gaps
+			const uniqueNumbers = new Set(numbers)
+			const needsRenumber = uniqueNumbers.size !== numbers.length || !isSequential(numbers)
+
+			if (needsRenumber) {
+				// Renumber all files sequentially
+				for (let i = 0; i < sortedFiles.length; i++) {
+					const oldFile = sortedFiles[i]
+					const match = oldFile.match(pattern)!
+					const type = match[2]
+					const newNumber = i + 1
+					const newFile = `${testName}.${type}.${newNumber}.txt`
+
+					if (oldFile !== newFile) {
+						const oldPath = path.join(categoryDir, oldFile)
+						const newPath = path.join(categoryDir, newFile)
+
+						// Use a temp file to avoid conflicts
+						const tempPath = path.join(categoryDir, `${testName}.${type}.temp_${i}.txt`)
+						fs.renameSync(oldPath, tempPath)
+						sortedFiles[i] = `${testName}.${type}.temp_${i}.txt`
+					}
+				}
+
+				// Now rename from temp to final
+				for (let i = 0; i < sortedFiles.length; i++) {
+					const tempFile = sortedFiles[i]
+					if (tempFile.includes(".temp_")) {
+						const match = tempFile.match(/^(.+)\.(approved|rejected)\.temp_(\d+)\.txt$/)!
+						const type = match[2]
+						const newNumber = i + 1
+						const newFile = `${testName}.${type}.${newNumber}.txt`
+
+						const tempPath = path.join(categoryDir, tempFile)
+						const newPath = path.join(categoryDir, newFile)
+						fs.renameSync(tempPath, newPath)
+						renamedCount++
+					}
+				}
+			}
+		}
+	}
+
+	return { renamedCount, totalFiles }
+}
+
+function isSequential(numbers: number[]): boolean {
+	const sorted = [...numbers].sort((a, b) => a - b)
+	for (let i = 0; i < sorted.length; i++) {
+		if (sorted[i] !== i + 1) {
+			return false
+		}
+	}
+	return true
+}

src/test-llm-autocompletion/runner.ts

@@ -4,7 +4,7 @@ import fs from "fs"
 import path from "path"
 import { GhostProviderTester } from "./ghost-provider-tester.js"
 import { testCases, getCategories, TestCase } from "./test-cases.js"
-import { checkApproval } from "./approvals.js"
+import { checkApproval, renumberApprovals } from "./approvals.js"
 
 interface TestResult {
 	testCase: TestCase
@@ -395,6 +395,16 @@ export class TestRunner {
 		} else {
 			console.log("No orphaned approval files found.")
 		}
+
+		// Renumber files to fix duplicates and gaps
+		console.log("\n🔢 Renumbering approval files to fix duplicates and gaps...\n")
+		const renumberResult = renumberApprovals(approvalsDir)
+
+		if (renumberResult.renamedCount > 0) {
+			console.log(`✅ Renumbered ${renumberResult.renamedCount} files to ensure unique sequential numbering.`)
+		} else {
+			console.log("No renumbering needed - all files already have unique sequential numbers.")
+		}
 	}
 }