From ce7bde14f362d6a2646f9df2e69bec868d74216e Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Sat, 14 Mar 2026 07:26:10 -0600 Subject: [PATCH] Fix execute-task loop detection: adaptive reconciliation instead of hard stop (#342) * Initial plan * Add adaptive reconciliation, concrete remediation steps, and xxHash32 JS fallback - auto.ts: final reconciliation pass before halting on loop detection (runs skipExecuteTask to write blocker artifacts and advance pipeline) - auto.ts: adaptive retry on prevCount>=2: write stub summary when still missing after two agent sessions, so the agent has a recovery context - auto.ts: buildLoopRemediationSteps() helper with concrete manual steps when automatic reconciliation cannot resolve the unit - xxhash/index.ts: pure-JS xxHash32 fallback when native addon does not export xxHash32 (prevents 'native.xxHash32 is not a function' crash) - idle-recovery.test.ts: tests for buildLoopRemediationSteps and loop-recovery path in skipExecuteTask Co-authored-by: glittercowboy <186001655+glittercowboy@users.noreply.github.com> * Address code review feedback - xxhash/index.ts: extract accumulate() helper to remove duplicate lane processing lines in the 16-byte loop - auto.ts: replace magic number 2 with named constant STUB_RECOVERY_THRESHOLD - idle-recovery.test.ts: add comment explaining the 3 (== MAX_UNIT_DISPATCHES) used in the loop-recovery test Co-authored-by: glittercowboy <186001655+glittercowboy@users.noreply.github.com> * Address PR review: verify post-state, fix stub wording, fix checkbox syntax, add fallback test - auto.ts (MAX branch): check verifyExpectedArtifact() after skipExecuteTask() before advancing; annotate both conditions explaining what each validates - auto.ts (self-repair branch): guard advance with verifyExpectedArtifact() so regex mismatches in the slice plan don't silently re-dispatch - auto.ts (stub-recovery): write stub summary directly with "PARTIAL RECOVERY" wording instead of using skipExecuteTask (which would claim "exhausted N attempts") - auto.ts (remediation): fix checkbox example to "- [x] **${tid}:" (no trailing **) - xxhash/index.ts: export xxHash32Fallback so CI can test JS path independently - native/src/index.ts: re-export xxHash32Fallback - xxhash.test.mjs: add "xxHash32Fallback (pure-JS path)" describe block that validates the JS implementation against the reference on all test cases Co-authored-by: glittercowboy <186001655+glittercowboy@users.noreply.github.com> * chore: retrigger CI --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: glittercowboy <186001655+glittercowboy@users.noreply.github.com> Co-authored-by: CI Trigger --- packages/native/src/__tests__/xxhash.test.mjs | 38 +++++- packages/native/src/index.ts | 2 +- packages/native/src/xxhash/index.ts | 84 ++++++++++++- src/resources/extensions/gsd/auto.ts | 118 +++++++++++++++++- .../gsd/tests/idle-recovery.test.ts | 101 +++++++++++++++ 5 files changed, 333 insertions(+), 10 deletions(-) diff --git a/packages/native/src/__tests__/xxhash.test.mjs b/packages/native/src/__tests__/xxhash.test.mjs index 1791b55ce..6feeba6f0 100644 --- a/packages/native/src/__tests__/xxhash.test.mjs +++ b/packages/native/src/__tests__/xxhash.test.mjs @@ -1,6 +1,6 @@ import { describe, it } from "node:test"; import assert from "node:assert/strict"; -import { xxHash32 } from "@gsd/native/xxhash"; +import { xxHash32, xxHash32Fallback } from "@gsd/native/xxhash"; /** * Reference values computed from the pure-JS xxHash32 implementation @@ -84,3 +84,39 @@ describe("xxHash32 native vs JS compatibility", () => { }); } }); + +describe("xxHash32Fallback (pure-JS path)", () => { + // These tests exercise the JS fallback directly, validating the path that + // runs when the native addon loads but does not export xxHash32. + const testCases = [ + ["empty string, seed 0", "", 0], + ["short string, seed 0", "hello", 0], + ["short string, seed 42", "hello", 42], + ["medium string, seed 0", "hello world!", 0], + ["long string (>16 bytes)", "abcdefghijklmnopqrstuvwxyz", 0], + ["whitespace only", " ", 0], + ["punctuation", "{}();", 0], + ["unicode", "\u{4e16}\u{754c}\u{1f600}", 0], + ["empty with nonzero seed", "", 7], + ["typical code line", " const x = 42;", 0], + ["typical code line with seed", " const x = 42;", 3], + ]; + + for (const [label, input, seed] of testCases) { + it(`JS fallback matches JS reference: ${label}`, () => { + const expected = jsXxHash32(input, seed); + const actual = xxHash32Fallback(input, seed); + assert.equal( + actual, + expected, + `Fallback mismatch for "${input}" seed=${seed}: fallback=${actual.toString(16)} reference=${expected.toString(16)}` + ); + }); + } + + it("JS fallback produces same result as xxHash32", () => { + const input = " const x = hashline;"; + const seed = 0; + assert.equal(xxHash32Fallback(input, seed), xxHash32(input, seed)); + }); +}); diff --git a/packages/native/src/index.ts b/packages/native/src/index.ts index cd8613f22..a2cc227d0 100644 --- a/packages/native/src/index.ts +++ b/packages/native/src/index.ts @@ -91,7 +91,7 @@ export type { export { parseImage, ImageFormat, SamplingFilter } from "./image/index.js"; export type { NativeImageHandle } from "./image/index.js"; -export { xxHash32 } from "./xxhash/index.js"; +export { xxHash32, xxHash32Fallback } from "./xxhash/index.js"; export { ttsrCompileRules, ttsrCheckBuffer, ttsrFreeRules } from "./ttsr/index.js"; export type { TtsrHandle, TtsrRuleInput } from "./ttsr/index.js"; diff --git a/packages/native/src/xxhash/index.ts b/packages/native/src/xxhash/index.ts index 1aac2adfe..55374707e 100644 --- a/packages/native/src/xxhash/index.ts +++ b/packages/native/src/xxhash/index.ts @@ -1,18 +1,98 @@ /** - * Native xxHash32 — Rust implementation via napi-rs. + * Native xxHash32 — Rust implementation via napi-rs, with a pure-JS fallback. * * Hashes the UTF-8 representation of the input string with the given seed. */ import { native } from "../native.js"; +// ─── Pure-JS xxHash32 fallback ──────────────────────────────────────────────── +// Used when the native addon is present but does not export xxHash32 +// (e.g. an older build or an unsupported platform variant). + +const PRIME32_1 = 0x9e3779b1; +const PRIME32_2 = 0x85ebca77; +const PRIME32_3 = 0xc2b2ae3d; +const PRIME32_4 = 0x27d4eb2f; +const PRIME32_5 = 0x165667b1; + +function rotl32(v: number, n: number): number { + return ((v << n) | (v >>> (32 - n))) >>> 0; +} + +/** Single xxHash32 lane accumulation step (processes one 32-bit word). */ +function accumulate(v: number, lane: number): number { + return Math.imul(rotl32((v + Math.imul(lane, PRIME32_2)) >>> 0, 13), PRIME32_1) >>> 0; +} + +function xxHash32JS(input: string, seed: number): number { + const data = Buffer.from(input, "utf-8"); + const len = data.length; + let h32: number; + let i = 0; + + if (len >= 16) { + let v1 = (seed + PRIME32_1 + PRIME32_2) >>> 0; + let v2 = (seed + PRIME32_2) >>> 0; + let v3 = seed >>> 0; + let v4 = (seed - PRIME32_1) >>> 0; + + while (i <= len - 16) { + v1 = accumulate(v1, data.readUInt32LE(i)); i += 4; + v2 = accumulate(v2, data.readUInt32LE(i)); i += 4; + v3 = accumulate(v3, data.readUInt32LE(i)); i += 4; + v4 = accumulate(v4, data.readUInt32LE(i)); i += 4; + } + + h32 = (rotl32(v1, 1) + rotl32(v2, 7) + rotl32(v3, 12) + rotl32(v4, 18)) >>> 0; + } else { + h32 = (seed + PRIME32_5) >>> 0; + } + + h32 = (h32 + len) >>> 0; + + while (i <= len - 4) { + h32 = Math.imul(rotl32((h32 + Math.imul(data.readUInt32LE(i), PRIME32_3)) >>> 0, 17), PRIME32_4) >>> 0; + i += 4; + } + + while (i < len) { + h32 = Math.imul(rotl32((h32 + Math.imul(data[i]!, PRIME32_5)) >>> 0, 11), PRIME32_1) >>> 0; + i++; + } + + h32 = Math.imul(h32 ^ (h32 >>> 15), PRIME32_2) >>> 0; + h32 = Math.imul(h32 ^ (h32 >>> 13), PRIME32_3) >>> 0; + h32 = (h32 ^ (h32 >>> 16)) >>> 0; + + return h32; +} + +/** + * Pure-JS xxHash32 implementation. Exposed for testing to allow CI to validate + * the fallback path independently of native addon availability. + */ +export function xxHash32Fallback(input: string, seed: number): number { + return xxHash32JS(input, seed); +} + +// Resolve once at module load: prefer native, fall back to JS. +const _xxHash32Impl: (input: string, seed: number) => number = + typeof native.xxHash32 === "function" + ? (input, seed) => native.xxHash32(input, seed) + : xxHash32JS; + /** * Compute xxHash32 of a UTF-8 string. * + * Uses the native Rust implementation when available; falls back to a + * pure-JS implementation if the loaded native addon does not export + * `xxHash32` (e.g. an older build). + * * @param input The string to hash (encoded as UTF-8 internally). * @param seed 32-bit seed value. * @returns 32-bit unsigned hash. */ export function xxHash32(input: string, seed: number): number { - return native.xxHash32(input, seed); + return _xxHash32Impl(input, seed); } diff --git a/src/resources/extensions/gsd/auto.ts b/src/resources/extensions/gsd/auto.ts index befc69e8c..e430e211f 100644 --- a/src/resources/extensions/gsd/auto.ts +++ b/src/resources/extensions/gsd/auto.ts @@ -135,6 +135,8 @@ let gitService: GitServiceImpl | null = null; /** Track total dispatches per unit to detect stuck loops (catches A→B→A→B patterns) */ const unitDispatchCount = new Map(); const MAX_UNIT_DISPATCHES = 3; +/** Retry index at which a stub summary placeholder is written when the summary is still absent. */ +const STUB_RECOVERY_THRESHOLD = 2; /** Tracks recovery attempt count per unit for backoff and diagnostics. */ const unitRecoveryCount = new Map(); @@ -1498,24 +1500,54 @@ async function dispatchNextUnit( } saveActivityLog(ctx, basePath, unitType, unitId); + // Final reconciliation pass for execute-task: write any missing durable + // artifacts (summary placeholder + [x] checkbox) so the pipeline can + // advance instead of stopping. This is the last resort before halting. + if (unitType === "execute-task") { + const [mid, sid, tid] = unitId.split("/"); + if (mid && sid && tid) { + const status = await inspectExecuteTaskDurability(basePath, unitId); + if (status) { + const reconciled = skipExecuteTask(basePath, mid, sid, tid, status, "loop-recovery", prevCount); + // reconciled: skipExecuteTask attempted to write missing artifacts. + // verifyExpectedArtifact: confirms physical artifacts (summary + [x]) now exist on disk. + // Both must pass before we clear the dispatch counter and advance. + if (reconciled && verifyExpectedArtifact(unitType, unitId, basePath)) { + ctx.ui.notify( + `Loop recovery: ${unitId} reconciled after ${prevCount + 1} dispatches — blocker artifacts written, pipeline advancing.\n Review ${status.summaryPath} and replace the placeholder with real work.`, + "warning", + ); + unitDispatchCount.delete(dispatchKey); + await new Promise(r => setImmediate(r)); + await dispatchNextUnit(ctx, pi); + return; + } + } + } + } + const expected = diagnoseExpectedArtifact(unitType, unitId, basePath); + const remediation = buildLoopRemediationSteps(unitType, unitId, basePath); await stopAuto(ctx, pi); ctx.ui.notify( - `Loop detected: ${unitType} ${unitId} dispatched ${prevCount + 1} times total. Expected artifact not found.${expected ? `\n Expected: ${expected}` : ""}\n Check branch state and .gsd/ artifacts.`, + `Loop detected: ${unitType} ${unitId} dispatched ${prevCount + 1} times total. Expected artifact not found.${expected ? `\n Expected: ${expected}` : ""}${remediation ? `\n\n Remediation steps:\n${remediation}` : "\n Check branch state and .gsd/ artifacts."}`, "error", ); return; } unitDispatchCount.set(dispatchKey, prevCount + 1); if (prevCount > 0) { - // Self-repair: if summary exists but checkbox not marked, fix it and re-derive + // Adaptive self-repair: each retry attempts a different remediation step. if (unitType === "execute-task") { const status = await inspectExecuteTaskDurability(basePath, unitId); - if (status?.summaryExists && !status.taskChecked) { - const [mid, sid, tid] = unitId.split("/"); - if (mid && sid && tid) { + const [mid, sid, tid] = unitId.split("/"); + if (status && mid && sid && tid) { + if (status.summaryExists && !status.taskChecked) { + // Retry 1+: summary exists but checkbox not marked — mark [x] and advance. const repaired = skipExecuteTask(basePath, mid, sid, tid, status, "self-repair", 0); - if (repaired) { + // repaired: skipExecuteTask updated metadata (returned early-true even if regex missed). + // verifyExpectedArtifact: confirms the physical artifact (summary + [x]) now exists. + if (repaired && verifyExpectedArtifact(unitType, unitId, basePath)) { ctx.ui.notify( `Self-repaired ${unitId}: summary existed but checkbox was unmarked. Marked [x] and advancing.`, "warning", @@ -1525,6 +1557,32 @@ async function dispatchNextUnit( await dispatchNextUnit(ctx, pi); return; } + } else if (prevCount >= STUB_RECOVERY_THRESHOLD && !status.summaryExists) { + // Retry STUB_RECOVERY_THRESHOLD+: summary still missing after multiple attempts. + // Write a minimal stub summary so the next agent session has a recovery artifact + // to overwrite, rather than starting from scratch again. + const tasksDir = resolveTasksDir(basePath, mid, sid); + const sDir = resolveSlicePath(basePath, mid, sid); + const targetDir = tasksDir ?? (sDir ? join(sDir, "tasks") : null); + if (targetDir) { + if (!existsSync(targetDir)) mkdirSync(targetDir, { recursive: true }); + const summaryPath = join(targetDir, buildTaskFileName(tid, "SUMMARY")); + if (!existsSync(summaryPath)) { + const stubContent = [ + `# PARTIAL RECOVERY — attempt ${prevCount + 1} of ${MAX_UNIT_DISPATCHES}`, + ``, + `Task \`${tid}\` in slice \`${sid}\` (milestone \`${mid}\`) has not yet produced a real summary.`, + `This placeholder was written by auto-mode after ${prevCount} dispatch attempts.`, + ``, + `The next agent session will retry this task. Replace this file with real work when done.`, + ].join("\n"); + writeFileSync(summaryPath, stubContent, "utf-8"); + ctx.ui.notify( + `Stub recovery (attempt ${prevCount + 1}/${MAX_UNIT_DISPATCHES}): ${unitId} stub summary placeholder written. Retrying with recovery context.`, + "warning", + ); + } + } } } } @@ -3178,3 +3236,51 @@ function diagnoseExpectedArtifact(unitType: string, unitId: string, base: string return null; } } + +/** + * Build concrete, manual remediation steps for a loop-detected unit failure. + * These are shown when automatic reconciliation is not possible. + */ +export function buildLoopRemediationSteps(unitType: string, unitId: string, base: string): string | null { + const parts = unitId.split("/"); + const mid = parts[0]; + const sid = parts[1]; + const tid = parts[2]; + switch (unitType) { + case "execute-task": { + if (!mid || !sid || !tid) break; + const planRel = relSliceFile(base, mid, sid, "PLAN"); + const summaryRel = relTaskFile(base, mid, sid, tid, "SUMMARY"); + return [ + ` 1. Write ${summaryRel} (even a partial summary is sufficient to unblock the pipeline)`, + ` 2. Mark ${tid} [x] in ${planRel}: change "- [ ] **${tid}:" → "- [x] **${tid}:"`, + ` 3. Run \`gsd doctor\` to reconcile .gsd/ state`, + ` 4. Resume auto-mode — it will pick up from the next task`, + ].join("\n"); + } + case "plan-slice": + case "research-slice": { + if (!mid || !sid) break; + const artifactRel = unitType === "plan-slice" + ? relSliceFile(base, mid, sid, "PLAN") + : relSliceFile(base, mid, sid, "RESEARCH"); + return [ + ` 1. Write ${artifactRel} manually (or with the LLM in interactive mode)`, + ` 2. Run \`gsd doctor\` to reconcile .gsd/ state`, + ` 3. Resume auto-mode`, + ].join("\n"); + } + case "complete-slice": { + if (!mid || !sid) break; + return [ + ` 1. Write the slice summary and UAT file for ${sid} in ${relSlicePath(base, mid, sid)}`, + ` 2. Mark ${sid} [x] in ${relMilestoneFile(base, mid, "ROADMAP")}`, + ` 3. Run \`gsd doctor\` to reconcile .gsd/ state`, + ` 4. Resume auto-mode`, + ].join("\n"); + } + default: + break; + } + return null; +} diff --git a/src/resources/extensions/gsd/tests/idle-recovery.test.ts b/src/resources/extensions/gsd/tests/idle-recovery.test.ts index 4ca52330e..213988089 100644 --- a/src/resources/extensions/gsd/tests/idle-recovery.test.ts +++ b/src/resources/extensions/gsd/tests/idle-recovery.test.ts @@ -7,6 +7,7 @@ import { writeBlockerPlaceholder, skipExecuteTask, verifyExpectedArtifact, + buildLoopRemediationSteps, } from "../auto.ts"; let passed = 0; @@ -490,6 +491,106 @@ const ROADMAP_COMPLETE = `# M001: Test Milestone } } +// ═══ buildLoopRemediationSteps ═══════════════════════════════════════════════ + +{ + console.log("\n=== buildLoopRemediationSteps: execute-task returns concrete steps ==="); + const base = mkdtempSync(join(tmpdir(), "gsd-loop-remediation-test-")); + try { + mkdirSync(join(base, ".gsd", "milestones", "M002", "slices", "S03", "tasks"), { recursive: true }); + const result = buildLoopRemediationSteps("execute-task", "M002/S03/T01", base); + assert(result !== null, "should return remediation steps"); + assert(result!.includes("T01-SUMMARY.md"), "steps mention the summary file"); + assert(result!.includes("S03-PLAN.md"), "steps mention the slice plan"); + assert(result!.includes("T01"), "steps mention the task ID"); + assert(result!.includes("gsd doctor"), "steps include gsd doctor command"); + // Exact slice plan checkbox syntax (no trailing **) + assert(result!.includes('"- [x] **T01:"'), "steps show exact checkbox syntax without trailing **"); + } finally { + rmSync(base, { recursive: true, force: true }); + } +} + +{ + console.log("\n=== buildLoopRemediationSteps: plan-slice returns concrete steps ==="); + const base = mkdtempSync(join(tmpdir(), "gsd-loop-remediation-test-")); + try { + mkdirSync(join(base, ".gsd", "milestones", "M001", "slices", "S01"), { recursive: true }); + const result = buildLoopRemediationSteps("plan-slice", "M001/S01", base); + assert(result !== null, "should return remediation steps for plan-slice"); + assert(result!.includes("S01-PLAN.md"), "steps mention the slice plan file"); + assert(result!.includes("gsd doctor"), "steps include gsd doctor command"); + } finally { + rmSync(base, { recursive: true, force: true }); + } +} + +{ + console.log("\n=== buildLoopRemediationSteps: research-slice returns concrete steps ==="); + const base = mkdtempSync(join(tmpdir(), "gsd-loop-remediation-test-")); + try { + mkdirSync(join(base, ".gsd", "milestones", "M001", "slices", "S01"), { recursive: true }); + const result = buildLoopRemediationSteps("research-slice", "M001/S01", base); + assert(result !== null, "should return remediation steps for research-slice"); + assert(result!.includes("S01-RESEARCH.md"), "steps mention the slice research file"); + assert(result!.includes("gsd doctor"), "steps include gsd doctor command"); + } finally { + rmSync(base, { recursive: true, force: true }); + } +} + +{ + console.log("\n=== buildLoopRemediationSteps: unknown type returns null ==="); + const base = mkdtempSync(join(tmpdir(), "gsd-loop-remediation-test-")); + try { + const result = buildLoopRemediationSteps("unknown-type", "M001/S01", base); + assertEq(result, null, "unknown type returns null"); + } finally { + rmSync(base, { recursive: true, force: true }); + } +} + +{ + console.log("\n=== skipExecuteTask: loop-recovery writes blocker when both summary and checkbox missing ==="); + const base = mkdtempSync(join(tmpdir(), "gsd-loop-recovery-test-")); + try { + mkdirSync(join(base, ".gsd", "milestones", "M002", "slices", "S03", "tasks"), { recursive: true }); + const planPath = join(base, ".gsd", "milestones", "M002", "slices", "S03", "S03-PLAN.md"); + writeFileSync(planPath, [ + "# S03: Harden guided session", + "", + "## Tasks", + "", + "- [ ] **T01: Harden contract usage** `est:30m`", + " Harden guided session contract usage in desktop flow.", + ].join("\n"), "utf-8"); + + const result = skipExecuteTask( + base, "M002", "S03", "T01", + { summaryExists: false, taskChecked: false }, + "loop-recovery", + // 3 == MAX_UNIT_DISPATCHES: represents the prevCount when the final + // reconciliation path runs (loop detected, reconciling before halting). + 3, + ); + + assert(result === true, "loop-recovery should succeed"); + + // Blocker summary written + const summaryPath = join(base, ".gsd", "milestones", "M002", "slices", "S03", "tasks", "T01-SUMMARY.md"); + assert(existsSync(summaryPath), "blocker summary should be written"); + const summaryContent = readFileSync(summaryPath, "utf-8"); + assert(summaryContent.includes("BLOCKER"), "summary should be a blocker placeholder"); + assert(summaryContent.includes("loop-recovery"), "summary should mention the recovery reason"); + + // Checkbox marked + const planContent = readFileSync(planPath, "utf-8"); + assert(planContent.includes("- [x] **T01:"), "T01 checkbox should be marked [x] after loop-recovery"); + } finally { + rmSync(base, { recursive: true, force: true }); + } +} + // ═════════════════════════════════════════════════════════════════════════════ // Results // ═════════════════════════════════════════════════════════════════════════════