Merge pull request #4231 from Kilo-Org/mark/unique-approval-file-numbers

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

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
@@ -195,6 +196,6 @@ 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

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

@@ -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", () => {

src/test-llm-autocompletion/approvals.ts

@@ -28,18 +28,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
@@ -136,7 +137,7 @@ 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)
 

src/test-llm-autocompletion/approvals/api-patterns/rest-api-client.approved.10.txt

@@ -7,9 +7,11 @@ class ApiClient {
 			},
 			body: JSON.stringify(data),
 		});
+
 		if (!response.ok) {
 			throw new Error(`HTTP error! status: ${response.status}`);
 		}
-		return response.json();
+
+		return await response.json();
 	}
 }

src/test-llm-autocompletion/approvals/api-patterns/rest-api-client.approved.11.txt

@@ -7,6 +7,9 @@ class ApiClient {
 			},
 			body: JSON.stringify(data),
 		});
-		return response.json();
+		if (!response.ok) {
+			throw new Error(`HTTP error! status: ${response.status}`);
+		}
+		return await response.json();
 	}
 }

src/test-llm-autocompletion/approvals/api-patterns/rest-api-client.approved.12.txt

@@ -7,6 +7,6 @@ class ApiClient {
 			},
 			body: JSON.stringify(data),
 		});
-		const result = await response.json();
-		return result;
-	}
+		return await response.json();
+	}
+}

src/test-llm-autocompletion/approvals/api-patterns/rest-api-client.approved.13.txt

@@ -10,5 +10,6 @@ class ApiClient {
 		if (!response.ok) {
 			throw new Error(`HTTP error! status: ${response.status}`);
 		}
-		return await response.json();
-	}
+		return response.json();
+	}
+}

src/test-llm-autocompletion/approvals/api-patterns/rest-api-client.approved.14.txt

@@ -7,6 +7,6 @@ class ApiClient {
 			},
 			body: JSON.stringify(data),
 		});
-		const json = await response.json();
-		return json;
-	}
+		return response.json();
+	}
+}

src/test-llm-autocompletion/approvals/api-patterns/rest-api-client.approved.15.txt

@@ -6,4 +6,7 @@ class ApiClient {
 				'Content-Type': 'application/json',
 			},
 			body: JSON.stringify(data),
-		});
+		});
+		const result = await response.json();
+		return result;
+	}

src/test-llm-autocompletion/approvals/api-patterns/rest-api-client.approved.16.txt

@@ -0,0 +1,14 @@
+class ApiClient {
+	async post(endpoint, data) {
+		const response = await fetch(this.baseUrl + endpoint, {
+			method: 'POST',
+			headers: {
+				'Content-Type': 'application/json',
+			},
+			body: JSON.stringify(data),
+		});
+		if (!response.ok) {
+			throw new Error(`HTTP error! status: ${response.status}`);
+		}
+		return await response.json();
+	}