From 2da1ecfd2072dd385683387f8a0204ccf86d8fa9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?T=C3=82CHES?= Date: Sat, 21 Mar 2026 09:46:27 -0600 Subject: [PATCH] fix: return retry from postUnitPreVerification when artifact verification fails (#1571) (#1782) When verifyExpectedArtifact returns false for a unit type with a known expected artifact, postUnitPreVerification now returns "retry" instead of "continue". This sets pendingVerificationRetry on the session so the next loop iteration re-dispatches with failure context, preventing 13+ blind re-dispatches of the same failed unit before the stuck-loop detector kicks in. Closes #1571 Co-authored-by: Claude Opus 4.6 (1M context) --- .../extensions/gsd/auto-post-unit.ts | 31 +++++++- .../extensions/gsd/auto/loop-deps.ts | 2 +- src/resources/extensions/gsd/auto/phases.ts | 11 +++ .../extensions/gsd/tests/auto-loop.test.ts | 78 +++++++++++++++++++ 4 files changed, 119 insertions(+), 3 deletions(-) diff --git a/src/resources/extensions/gsd/auto-post-unit.ts b/src/resources/extensions/gsd/auto-post-unit.ts index 4bd6812b0..6d7f054de 100644 --- a/src/resources/extensions/gsd/auto-post-unit.ts +++ b/src/resources/extensions/gsd/auto-post-unit.ts @@ -31,6 +31,7 @@ import { } from "./worktree.js"; import { verifyExpectedArtifact, + resolveExpectedArtifactPath, } from "./auto-recovery.js"; import { writeUnitRuntimeRecord, clearUnitRuntimeRecord } from "./unit-runtime.js"; import { runGSDDoctor, rebuildState, summarizeDoctorIssues } from "./doctor.js"; @@ -84,9 +85,12 @@ export interface PostUnitContext { * Pre-verification processing: parallel worker signal check, cache invalidation, * auto-commit, doctor run, state rebuild, worktree sync, artifact verification. * - * Returns "dispatched" if a signal caused stop/pause, "continue" to proceed. + * Returns: + * - "dispatched" — a signal caused stop/pause + * - "continue" — proceed normally + * - "retry" — artifact verification failed, s.pendingVerificationRetry set for loop re-iteration */ -export async function postUnitPreVerification(pctx: PostUnitContext, opts?: PreVerificationOpts): Promise<"dispatched" | "continue"> { +export async function postUnitPreVerification(pctx: PostUnitContext, opts?: PreVerificationOpts): Promise<"dispatched" | "continue" | "retry"> { const { s, ctx, pi, buildSnapshotOpts, stopAuto, pauseAuto } = pctx; // ── Parallel worker signal check ── @@ -347,6 +351,29 @@ export async function postUnitPreVerification(pctx: PostUnitContext, opts?: PreV } catch (e) { debugLog("postUnit", { phase: "artifact-verify", error: String(e) }); } + + // When artifact verification fails for a unit type that has a known expected + // artifact, return "retry" so the caller re-dispatches with failure context + // instead of blindly re-dispatching the same unit (#1571). + if (!triggerArtifactVerified) { + const hasExpectedArtifact = resolveExpectedArtifactPath(s.currentUnit.type, s.currentUnit.id, s.basePath) !== null; + if (hasExpectedArtifact) { + const retryKey = `${s.currentUnit.type}:${s.currentUnit.id}`; + const attempt = (s.verificationRetryCount.get(retryKey) ?? 0) + 1; + s.verificationRetryCount.set(retryKey, attempt); + s.pendingVerificationRetry = { + unitId: s.currentUnit.id, + failureContext: `Artifact verification failed: expected artifact for ${s.currentUnit.type} "${s.currentUnit.id}" was not found on disk after unit execution (attempt ${attempt}).`, + attempt, + }; + debugLog("postUnit", { phase: "artifact-verify-retry", unitType: s.currentUnit.type, unitId: s.currentUnit.id, attempt }); + ctx.ui.notify( + `Artifact missing for ${s.currentUnit.type} ${s.currentUnit.id} — retrying (attempt ${attempt})`, + "warning", + ); + return "retry"; + } + } } else { // Hook unit completed — finalize its runtime record try { diff --git a/src/resources/extensions/gsd/auto/loop-deps.ts b/src/resources/extensions/gsd/auto/loop-deps.ts index c016fa852..9b8961832 100644 --- a/src/resources/extensions/gsd/auto/loop-deps.ts +++ b/src/resources/extensions/gsd/auto/loop-deps.ts @@ -273,7 +273,7 @@ export interface LoopDeps { postUnitPreVerification: ( pctx: PostUnitContext, opts?: PreVerificationOpts, - ) => Promise<"dispatched" | "continue">; + ) => Promise<"dispatched" | "continue" | "retry">; runPostUnitVerification: ( vctx: VerificationContext, pauseAuto: (ctx?: ExtensionContext, pi?: ExtensionAPI) => Promise, diff --git a/src/resources/extensions/gsd/auto/phases.ts b/src/resources/extensions/gsd/auto/phases.ts index 6ede2fa67..5efff699d 100644 --- a/src/resources/extensions/gsd/auto/phases.ts +++ b/src/resources/extensions/gsd/auto/phases.ts @@ -1137,6 +1137,17 @@ export async function runFinalize( }); return { action: "break", reason: "pre-verification-dispatched" }; } + if (preResult === "retry") { + if (sidecarItem) { + // Sidecar artifact retries are skipped — just continue + debugLog("autoLoop", { phase: "sidecar-artifact-retry-skipped", iteration: ic.iteration }); + } else { + // s.pendingVerificationRetry was set by postUnitPreVerification. + // Continue the loop — next iteration will inject the retry context into the prompt. + debugLog("autoLoop", { phase: "artifact-verification-retry", iteration: ic.iteration }); + return { action: "continue" }; + } + } if (pauseAfterUatDispatch) { ctx.ui.notify( diff --git a/src/resources/extensions/gsd/tests/auto-loop.test.ts b/src/resources/extensions/gsd/tests/auto-loop.test.ts index ec10833cf..42e96393f 100644 --- a/src/resources/extensions/gsd/tests/auto-loop.test.ts +++ b/src/resources/extensions/gsd/tests/auto-loop.test.ts @@ -1762,3 +1762,81 @@ test("resolveAgentEndCancelled prevents orphaned promise after abort path", asyn const result = await resultPromise; assert.equal(result.status, "cancelled"); }); + +// ─── #1571: artifact verification retry ────────────────────────────────────── + +test("autoLoop re-iterates when postUnitPreVerification returns retry (#1571)", async () => { + _resetPendingResolve(); + + const ctx = makeMockCtx(); + ctx.ui.setStatus = () => {}; + const pi = makeMockPi(); + const s = makeLoopSession(); + + let preVerifyCallCount = 0; + + const deps = makeMockDeps({ + deriveState: async () => { + deps.callLog.push("deriveState"); + return { + phase: "executing", + activeMilestone: { id: "M001", title: "Test", status: "active" }, + activeSlice: { id: "S01", title: "Slice 1" }, + activeTask: { id: "T01" }, + registry: [{ id: "M001", status: "active" }], + blockers: [], + } as any; + }, + postUnitPreVerification: async () => { + deps.callLog.push("postUnitPreVerification"); + preVerifyCallCount++; + // First call returns "retry" (artifact missing), second returns "continue" + if (preVerifyCallCount === 1) { + return "retry" as const; + } + return "continue" as const; + }, + postUnitPostVerification: async () => { + deps.callLog.push("postUnitPostVerification"); + // After the retry succeeds (second iteration), stop the loop + s.active = false; + return "continue" as const; + }, + }); + + const loopPromise = autoLoop(ctx, pi, s, deps); + + // First iteration: runUnit completes → preVerification returns "retry" → loop continues + await new Promise((r) => setTimeout(r, 50)); + resolveAgentEnd(makeEvent()); + + // Second iteration: runUnit completes → preVerification returns "continue" → full finalize + await new Promise((r) => setTimeout(r, 50)); + resolveAgentEnd(makeEvent()); + + await loopPromise; + + // preVerification should have been called twice (retry + success) + assert.equal(preVerifyCallCount, 2, "preVerification should be called twice"); + + // When preVerification returns "retry", runPostUnitVerification and + // postUnitPostVerification should be skipped for that iteration. + // So we expect 1 call each (only the second iteration proceeds past pre-verification). + const postVerifyCalls = deps.callLog.filter( + (c: string) => c === "runPostUnitVerification", + ); + const postPostVerifyCalls = deps.callLog.filter( + (c: string) => c === "postUnitPostVerification", + ); + + assert.equal( + postVerifyCalls.length, + 1, + "runPostUnitVerification should only be called once (skipped on retry iteration)", + ); + assert.equal( + postPostVerifyCalls.length, + 1, + "postUnitPostVerification should only be called once (skipped on retry iteration)", + ); +});