fix: parse cache collision causing false loop detection on complete-slice (#583)
cacheKey() used length + first/last 100 chars, which collides when a checkbox changes [ ] → [x] mid-file (same length, same endpoints). verifyExpectedArtifact() only cleared the path cache, not the parse cache, so parseRoadmap() returned stale data with done=false. - Add clearParseCache() to verifyExpectedArtifact alongside clearPathCache - Include middle 100-char sample in cacheKey to prevent interior collisions - Add regression test for the cache collision scenario
This commit is contained in:
parent
5a662c4655
commit
5866bb0b27
3 changed files with 63 additions and 4 deletions
|
|
@ -11,6 +11,7 @@ import type { ExtensionContext } from "@gsd/pi-coding-agent";
|
|||
import {
|
||||
clearUnitRuntimeRecord,
|
||||
} from "./unit-runtime.js";
|
||||
import { clearParseCache } from "./files.js";
|
||||
import {
|
||||
nativeConflictFiles,
|
||||
nativeCommit,
|
||||
|
|
@ -107,9 +108,13 @@ export function verifyExpectedArtifact(unitType: string, unitId: string, base: s
|
|||
// is managed by the hook engine, not the artifact verification system.
|
||||
if (unitType.startsWith("hook/")) return true;
|
||||
|
||||
// Clear stale directory listing cache so artifact checks see fresh disk state (#431).
|
||||
// Moved after hook check to avoid unnecessary cache clears for hook units.
|
||||
// Clear stale directory listing cache AND parse cache so artifact checks see
|
||||
// fresh disk state (#431). The parse cache must also be cleared because
|
||||
// cacheKey() uses length + first/last 100 chars — when a checkbox changes
|
||||
// from [ ] to [x], the key collides with the pre-edit version, returning
|
||||
// stale parsed results (e.g., slice.done = false when it's actually true).
|
||||
clearPathCache();
|
||||
clearParseCache();
|
||||
|
||||
if (unitType === "rewrite-docs") {
|
||||
const overridesPath = resolveGsdRootFile(base, "OVERRIDES");
|
||||
|
|
|
|||
|
|
@ -26,12 +26,16 @@ import { nativeParseRoadmap, nativeExtractSection, nativeParsePlanFile, nativePa
|
|||
|
||||
const CACHE_MAX = 50;
|
||||
|
||||
/** Fast composite key: length + first/last 100 chars. Unique enough for distinct markdown files. */
|
||||
/** Fast composite key: length + first/mid/last 100 chars. The middle sample
|
||||
* prevents collisions when only a few characters change in the interior of
|
||||
* a file (e.g., a checkbox [ ] → [x] that doesn't alter length or endpoints). */
|
||||
function cacheKey(content: string): string {
|
||||
const len = content.length;
|
||||
const head = content.slice(0, 100);
|
||||
const midStart = Math.max(0, Math.floor(len / 2) - 50);
|
||||
const mid = len > 200 ? content.slice(midStart, midStart + 100) : '';
|
||||
const tail = len > 100 ? content.slice(-100) : '';
|
||||
return `${len}:${head}:${tail}`;
|
||||
return `${len}:${head}:${mid}:${tail}`;
|
||||
}
|
||||
|
||||
const _parseCache = new Map<string, unknown>();
|
||||
|
|
|
|||
|
|
@ -7,6 +7,7 @@ import { randomUUID } from "node:crypto";
|
|||
|
||||
import {
|
||||
resolveExpectedArtifactPath,
|
||||
verifyExpectedArtifact,
|
||||
diagnoseExpectedArtifact,
|
||||
buildLoopRemediationSteps,
|
||||
completedKeysPath,
|
||||
|
|
@ -14,6 +15,7 @@ import {
|
|||
removePersistedKey,
|
||||
loadPersistedKeys,
|
||||
} from "../auto-recovery.ts";
|
||||
import { parseRoadmap, clearParseCache } from "../files.ts";
|
||||
|
||||
function makeTmpBase(): string {
|
||||
const base = join(tmpdir(), `gsd-test-${randomUUID()}`);
|
||||
|
|
@ -270,3 +272,51 @@ test("removePersistedKey is safe when file doesn't exist", () => {
|
|||
cleanup(base);
|
||||
}
|
||||
});
|
||||
|
||||
// ─── verifyExpectedArtifact: parse cache collision regression ─────────────
|
||||
|
||||
test("verifyExpectedArtifact detects roadmap [x] change despite parse cache", () => {
|
||||
// Regression test: cacheKey collision when [ ] → [x] doesn't change
|
||||
// file length or first/last 100 chars. Without the fix, parseRoadmap
|
||||
// returns stale cached data with done=false even though the file has [x].
|
||||
const base = makeTmpBase();
|
||||
try {
|
||||
// Build a roadmap long enough that the [x] change is outside the first/last 100 chars
|
||||
const padding = "A".repeat(200);
|
||||
const roadmapBefore = [
|
||||
`# M001: Test Milestone ${padding}`,
|
||||
"",
|
||||
"## Slices",
|
||||
"",
|
||||
"- [ ] **S01: First slice** `risk:low`",
|
||||
"",
|
||||
`## Footer ${padding}`,
|
||||
].join("\n");
|
||||
const roadmapAfter = roadmapBefore.replace("- [ ] **S01:", "- [x] **S01:");
|
||||
|
||||
// Verify lengths are identical (the key collision condition)
|
||||
assert.equal(roadmapBefore.length, roadmapAfter.length);
|
||||
|
||||
// Populate parse cache with the pre-edit roadmap
|
||||
const before = parseRoadmap(roadmapBefore);
|
||||
const sliceBefore = before.slices.find(s => s.id === "S01");
|
||||
assert.ok(sliceBefore);
|
||||
assert.equal(sliceBefore!.done, false);
|
||||
|
||||
// Now write the post-edit roadmap to disk and create required artifacts
|
||||
const roadmapPath = join(base, ".gsd", "milestones", "M001", "M001-ROADMAP.md");
|
||||
writeFileSync(roadmapPath, roadmapAfter);
|
||||
const summaryPath = join(base, ".gsd", "milestones", "M001", "slices", "S01", "S01-SUMMARY.md");
|
||||
writeFileSync(summaryPath, "# Summary\nDone.");
|
||||
const uatPath = join(base, ".gsd", "milestones", "M001", "slices", "S01", "S01-UAT.md");
|
||||
writeFileSync(uatPath, "# UAT\nPassed.");
|
||||
|
||||
// verifyExpectedArtifact should see the [x] despite the parse cache
|
||||
// having the [ ] version. The fix clears the parse cache inside verify.
|
||||
const verified = verifyExpectedArtifact("complete-slice", "M001/S01", base);
|
||||
assert.equal(verified, true, "verifyExpectedArtifact should return true when roadmap has [x]");
|
||||
} finally {
|
||||
clearParseCache();
|
||||
cleanup(base);
|
||||
}
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue