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")