From 00e8006709ff2bd0e10fdaf5bd1918779695c4df Mon Sep 17 00:00:00 2001 From: opencode-debug Date: Sun, 27 Sep 2026 18:20:56 +0800 Subject: [PATCH] fix(phase2): never reset the baseline after a no-op consolidation A phase-2 run whose consolidation agent changed nothing was recorded as `succeeded`: the git baseline was reset and the watermark advanced, even though the new rollouts were never promoted into MEMORY.md or memory_summary.md. Because the baseline reset is what makes the workspace diff disappear, the next pass saw zero changes, returned `no_workspace_changes`, and never invoked the consolidator again. The un-consolidated memories were then stranded in the workspace permanently and silently, while every status field reported success. The consolidation contract explicitly allows this run: "No-op content updates are allowed and preferred when there is no meaningful, reusable learning worth saving." So the host cannot infer success from a clean `consolidateViaSubagent` return, and `validateConsolidationArtifactsForVersion` cannot catch it either - a stale MEMORY.md and a `v1`-headed summary are indistinguishable from fresh ones. Fingerprint the artifacts the consolidator owns before and after the helper runs. If nothing changed while the diff carried rollout summaries or extension resources, fail the job with the new `no_artifact_changes` status and skip `resetBaseline`, so the diff survives for the next attempt and the condition is visible in `memory_inspect` instead of silent. `raw_memories.md` is excluded from the "carries learning" test on purpose: `rebuildRawMemories([])` always writes a placeholder, so on a first INIT run it would look like new material and turn a legitimately empty consolidation into a permanent retry loop. A new or updated stage-1 output always changes the rollout_summaries set too, since the file stem embeds source_updated_at. --- src/phase2.ts | 31 +++++++++++++++++ src/workspace.ts | 72 ++++++++++++++++++++++++++++++++++++++ tests/phase2.test.ts | 76 +++++++++++++++++++++++++++++++++++++++++ tests/workspace.test.ts | 72 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 251 insertions(+) diff --git a/src/phase2.ts b/src/phase2.ts index b994e6a..7a8b5cd 100644 --- a/src/phase2.ts +++ b/src/phase2.ts @@ -8,6 +8,8 @@ import { writeWorkspaceDiff, validateConsolidationArtifactsForVersion, removeMemorySymlinks, + fingerprintConsolidationArtifacts, + diffCarriesNewLearning, } from "./workspace.js" import { ensureBaseline, captureWorkspaceDiff, resetBaseline, DIFF_ARTIFACT } from "./git-baseline.js" import { @@ -254,6 +256,12 @@ async function runVersionPhase2( writeWorkspaceDiff(diff) + // Snapshot the artifacts the consolidator owns before handing it the diff. + // See fingerprintConsolidationArtifacts: a clean return from + // consolidateViaSubagent does not prove the agent wrote anything, and the + // contract lets it no-op. Compared after the helper closes. + const artifactsBeforeConsolidator = fingerprintConsolidationArtifacts(root) + let heartbeatLost = false let heartbeatFailure: unknown = "ownership lost" const heartbeatOnce = (): boolean => { @@ -345,6 +353,29 @@ async function runVersionPhase2( return { status: "failed_invalid_artifacts" } } + // The consolidation agent returned successfully but left MEMORY.md / + // memory_summary.md byte-identical while the diff carried new rollouts, so + // the new memories were never promoted. Validation cannot catch this: the + // stale files still exist and the summary header is still "v1". + // + // Resetting the baseline here would be the damaging part. It would fold the + // un-consolidated rollout summaries into the baseline, the next pass would + // then see zero changes, take the no_workspace_changes early return above, + // and never invoke the consolidator again — the new memories would be + // stranded in the workspace permanently and silently, with the job reported + // as succeeded. Keep the diff instead and let the normal failure backoff + // retry, so the run is observable and recoverable. + if ( + fingerprintConsolidationArtifacts(root) === artifactsBeforeConsolidator && + diffCarriesNewLearning(diff.changes) + ) { + store.markPhase2Failed( + claim.ownershipToken, + "consolidation agent made no artifact changes while the workspace diff carried new rollouts", + ) + return { status: "no_artifact_changes" } + } + if (!await withHostTimeout(resetBaseline(diff.extensionSnapshot), GIT_TIMEOUT_MS, "resetBaseline")) { store.markPhase2Failed(claim.ownershipToken, "baseline reset failed") return { status: "baseline_reset_failed" } diff --git a/src/workspace.ts b/src/workspace.ts index da63c3d..ca83011 100644 --- a/src/workspace.ts +++ b/src/workspace.ts @@ -106,6 +106,78 @@ export function isValidV2Summary(summary: string): boolean { return V2_SUMMARY_HEADINGS.every((heading) => lines.includes(heading)) } +/** + * Artifacts the consolidation agent owns. `raw_memories.md` and + * `rollout_summaries/` are phase-1 prep output, not consolidation output, so + * they are deliberately excluded: a change there says nothing about whether the + * agent promoted anything into the durable artifacts. + */ +const CONSOLIDATION_ARTIFACTS = ["MEMORY.md", "memory_summary.md"] + +/** + * Fingerprint of the consolidation artifacts, for detecting a no-op run. + * + * `consolidateViaSubagent` resolves as soon as the helper session closes and + * throws only on prompt/timeout/shutdown failure. The consolidation contract + * also explicitly permits a no-op ("No-op content updates are allowed and + * preferred when there is no meaningful, reusable learning worth saving"), so a + * clean return is NOT evidence that MEMORY.md / memory_summary.md were updated. + * `validateConsolidationArtifactsForVersion` cannot close that gap either: it + * only proves the files exist and the summary header is intact, which stays true + * for artifacts that are merely stale. + * + * Comparing this fingerprint before and after the helper runs is the only host + * signal that a consolidation actually happened. Uses size + mtime rather than + * content hashing: it runs once per phase-2 attempt over two small files, and + * mtime is what git's own status matrix keys on. + */ +export function fingerprintConsolidationArtifacts(root: string = memoryRoot()): string { + const parts: string[] = [] + for (const name of CONSOLIDATION_ARTIFACTS) { + try { + const st = fs.lstatSync(path.join(root, name)) + parts.push(st.isFile() ? `${name}:${st.size}:${st.mtimeMs}` : `${name}:not-a-file`) + } catch { + // v2 does not use MEMORY.md, so "missing" is a legitimate steady state. + parts.push(`${name}:missing`) + } + } + try { + const skills = fs + .readdirSync(path.join(root, SKILLS_DIR), { withFileTypes: true }) + .map((entry) => entry.name) + .sort() + parts.push(`${SKILLS_DIR}:${skills.join(",")}`) + } catch { + parts.push(`${SKILLS_DIR}:none`) + } + return parts.join("|") +} + +/** + * True when the workspace diff carries learning material the consolidator was + * required to propagate. + * + * Scoped to the two input families the consolidation contract names as + * consolidation triggers: added/modified/deleted files under + * `rollout_summaries/`, and resources under `extensions//resources/`. + * `raw_memories.md` is deliberately excluded even though it is derived from the + * same inputs, because `rebuildRawMemories([])` always writes a placeholder + * ("No raw memories yet."), so on a first INIT run it would register as new learning material on its own and turn a legitimately + * empty consolidation into a permanent retry loop. A new or updated stage-1 + * output always changes the rollout_summaries set too (the file stem embeds + * source_updated_at), so nothing real is lost by keying on rollouts alone. + */ +export function diffCarriesNewLearning(changes: readonly { path: string }[]): boolean { + return changes.some(({ path: changed }) => { + if (changed.startsWith(`.${ROLLOUT_DIR}/`) || changed.startsWith(`${ROLLOUT_DIR}/.`)) return false + if (changed.startsWith(`${ROLLOUT_DIR}/`)) return true + // extensions//resources/.md + const parts = changed.split("/") + return parts.length >= 3 && parts[0] === EXTENSIONS_DIR && parts[1] !== "." && parts[2] === "resources" + }) +} + export function validateConsolidationArtifacts(root: string = memoryRoot()): { ok: true } | { ok: false; reason: string } { return validateConsolidationArtifactsForVersion(root, "v1") } diff --git a/tests/phase2.test.ts b/tests/phase2.test.ts index 436baff..9293fbe 100644 --- a/tests/phase2.test.ts +++ b/tests/phase2.test.ts @@ -149,6 +149,82 @@ describe("phase 2 orchestration", () => { expect(fs.existsSync(path.join(memoryRoot(), "phase2_workspace_diff.md"))).toBe(true) }) + it("fails a successful-but-empty consolidation and keeps the diff so the next run retries", async () => { + // The consolidation contract explicitly permits a no-op: "No-op content + // updates are allowed and preferred when there is no meaningful, reusable + // learning worth saving." The helper can therefore read the diff, change + // nothing, and exit cleanly. That is not a consolidation, and it must not be + // recorded as one. + const store = new MemoryStore() + const ts = Date.now() + store.upsertStage1Output({ + session_id: "ses_noop", + source_updated_at: ts, + raw_memory: "### Task 1: reproducible fact\n\n- the retry budget is per-job, not global", + rollout_summary: "# Reproducible fact\n\nThe stage-1 retry budget is per-job.", + rollout_slug: "reproducible-fact", + cwd: "/p", + generated_at: ts, + }) + setPluginInput({ + client: { + session: { + create: async () => ({ data: { id: "sub-phase2-noop" } }), + prompt: async () => ({ data: { info: {}, parts: [{ type: "text", text: "nothing worth saving" }] } }), + delete: async () => ({ data: {} }), + get: async (req: { path: { id: string } }) => ({ data: { id: req.path.id }, response: { status: 200 } }), + }, + config: { get: async () => ({ data: {} }) }, + }, + } as any) + + const result = await runPhase2(new MemoryStore()) + expect(result.status).toBe("no_artifact_changes") + + const job = openDb() + .prepare("SELECT status, lease_until, retry_at, last_error FROM memory_jobs WHERE kind='memory_consolidate_global'") + .get() as { status: string; lease_until: number | null; retry_at: number | null; last_error: string } + expect(job.status).toBe("failed") + expect(job.lease_until).toBeNull() + expect(job.last_error).toMatch(/no artifact changes/) + // Failure backoff, not a hot loop. + expect(job.retry_at).toBeGreaterThan(0) + + // The load-bearing assertion. If the baseline had been reset here, the next + // pass would see zero changes, return no_workspace_changes, and never invoke + // the consolidator again — stranding this rollout in the workspace forever + // while the job reported success. + const pending = await captureWorkspaceDiff() + expect(pending.changes.some((change) => change.path.startsWith("rollout_summaries/"))).toBe(true) + }) + + it("accepts a no-op consolidation when the diff carries no new learning material", async () => { + // No stage-1 outputs: prep writes the raw_memories.md placeholder and no + // rollout summaries, so there is genuinely nothing to promote and a no-op is + // the correct outcome. Pre-seeding a valid baseline keeps the diff to the + // placeholder file, which must not be treated as learning material. + const { resetBaseline } = require("../src/git-baseline.js") + const root = memoryRoot() + fs.mkdirSync(root, { recursive: true }) + fs.writeFileSync(path.join(root, "MEMORY.md"), "# MEMORY.md\n") + fs.writeFileSync(path.join(root, "memory_summary.md"), "v1\n\n## User Profile\n") + await resetBaseline(new Map()) + + setPluginInput({ + client: { + session: { + create: async () => ({ data: { id: "sub-phase2-noop-clean" } }), + prompt: async () => ({ data: { info: {}, parts: [{ type: "text", text: "nothing to add" }] } }), + delete: async () => ({ data: {} }), + }, + config: { get: async () => ({ data: {} }) }, + }, + } as any) + + const result = await runPhase2(new MemoryStore()) + expect(result.status).toBe("succeeded") + }) + it("v2 succeeds with only memory_summary.md and does not write raw_memories.md", async () => { const { applyPluginOptions } = require("../src/index.js") applyPluginOptions({ version: "v2" }) diff --git a/tests/workspace.test.ts b/tests/workspace.test.ts index 53bffbd..b4a7087 100644 --- a/tests/workspace.test.ts +++ b/tests/workspace.test.ts @@ -273,6 +273,78 @@ describe("validateConsolidationArtifacts", () => { }) }) +describe("fingerprintConsolidationArtifacts", () => { + it("is stable when nothing is written and changes as soon as an artifact is touched", async () => { + const { ensureLayout, fingerprintConsolidationArtifacts } = require("../src/workspace.js") + const { memoryRoot } = require("../src/paths.js") + const root = memoryRoot() + ensureLayout() + fs.writeFileSync(path.join(root, "MEMORY.md"), "# MEMORY.md\n") + fs.writeFileSync(path.join(root, "memory_summary.md"), "v1\n\nprofile\n") + + const before = fingerprintConsolidationArtifacts(root) + expect(fingerprintConsolidationArtifacts(root)).toBe(before) + + // Stale-but-valid artifacts must still fingerprint as unchanged: this is + // exactly the state a no-op consolidation leaves behind, and detecting it is + // the whole point of the helper. + expect(validateStaleStillValid(root)).toBe(true) + + // mtime granularity can hide a same-millisecond write, so bump it explicitly. + const future = new Date(Date.now() + 5000) + fs.utimesSync(path.join(root, "memory_summary.md"), future, future) + expect(fingerprintConsolidationArtifacts(root)).not.toBe(before) + }) + + it("reports a missing MEMORY.md as a steady state rather than throwing (v2)", () => { + const { fingerprintConsolidationArtifacts } = require("../src/workspace.js") + const { memoryRoot } = require("../src/paths.js") + const root = memoryRoot() + fs.mkdirSync(root, { recursive: true }) + expect(fingerprintConsolidationArtifacts(root)).toContain("MEMORY.md:missing") + }) + + it("notices a newly created skill directory", () => { + const { fingerprintConsolidationArtifacts } = require("../src/workspace.js") + const { memoryRoot } = require("../src/paths.js") + const root = memoryRoot() + fs.mkdirSync(path.join(root, "skills"), { recursive: true }) + const before = fingerprintConsolidationArtifacts(root) + fs.mkdirSync(path.join(root, "skills", "new-skill"), { recursive: true }) + expect(fingerprintConsolidationArtifacts(root)).not.toBe(before) + expect(fingerprintConsolidationArtifacts(root)).toContain("skills:new-skill") + }) +}) + +describe("diffCarriesNewLearning", () => { + it("flags rollout summary additions, modifications and deletions", () => { + const { diffCarriesNewLearning } = require("../src/workspace.js") + expect(diffCarriesNewLearning([{ path: "rollout_summaries/2026-07-03T05-11-22-abcd-fix.md" }])).toBe(true) + expect(diffCarriesNewLearning([{ path: "raw_memories.md" }])).toBe(false) + expect(diffCarriesNewLearning([{ path: "MEMORY.md" }])).toBe(false) + expect(diffCarriesNewLearning([])).toBe(false) + }) + + it("flags extension resources, which the contract also treats as consolidation input", () => { + const { diffCarriesNewLearning } = require("../src/workspace.js") + expect(diffCarriesNewLearning([{ path: "extensions/codex_memory_import/resources/a.md" }])).toBe(true) + // instructions.md is scaffolding, not learned content. + expect(diffCarriesNewLearning([{ path: "extensions/ad_hoc/instructions.md" }])).toBe(false) + }) + + it("ignores a raw_memories.md-only diff so a first INIT run cannot loop", () => { + // rebuildRawMemories([]) always writes the "No raw memories yet." placeholder, + // so on a fresh workspace that file alone must not count as new learning. + const { diffCarriesNewLearning } = require("../src/workspace.js") + expect(diffCarriesNewLearning([{ path: "raw_memories.md" }, { path: "skills/.keep" }])).toBe(false) + }) +}) + +function validateStaleStillValid(root: string): boolean { + const { validateConsolidationArtifactsForVersion } = require("../src/workspace.js") + return validateConsolidationArtifactsForVersion(root, "v1").ok +} + describe("pruneExtensionResources", () => { it("prunes old timestamped resources but never notes or instructions.md", () => { const { ensureLayout, pruneExtensionResources } = require("../src/workspace.js")