From 173ce39049c27d4fbf5eb21b5ae6229b2caecff4 Mon Sep 17 00:00:00 2001 From: Yehonal <147092+Yehonal@users.noreply.github.com> Date: Sat, 27 Jun 2026 16:25:07 +0000 Subject: [PATCH] fix(install): honor --force-drift for managed instruction blocks --- src/install/apply.ts | 13 ++++-- src/install/instructions-block.ts | 39 ++++++++++++------ src/install/plan.ts | 7 +++- src/install/uninstall.ts | 2 +- test/instructions-block.test.ts | 68 +++++++++++++++++++++++++++++++ 5 files changed, 111 insertions(+), 18 deletions(-) diff --git a/src/install/apply.ts b/src/install/apply.ts index 3653aa8..2013f6b 100644 --- a/src/install/apply.ts +++ b/src/install/apply.ts @@ -226,7 +226,7 @@ export async function uninstall(plan: InstallPlan, options: UninstallOptions | b const removable = plan.operations .filter((operation) => operation.action === "remove" || (resolvedOptions.force && isForceRemovableKeep(operation))) .map((operation) => operation.action === "keep" - ? { ...operation, action: "remove" as const, reason: `${operation.reason}; force removing drifted managed file` } + ? { ...operation, action: "remove" as const, overrideDrift: true, reason: `${operation.reason}; force removing drifted managed file` } : operation); const kept = plan.operations.filter((operation) => operation.action === "keep" && (!resolvedOptions.force || !isForceRemovableKeep(operation))); const skipped = plan.operations.filter((operation) => operation.action === "skip"); @@ -399,7 +399,7 @@ async function applyOperation( } if (operation.mode === managedInstructionBlockMode) { const selector = managedInstructionSelector(operation.logicalSelector, operation.artifactType, operation.artifactName); - const hash = await writeManagedInstructionBlock(operation.sourcePath, operation.destPath, selector, transport, operation.manifestHash); + const hash = await writeManagedInstructionBlock(operation.sourcePath, operation.destPath, selector, transport, managedBlockMutationOptions(operation)); if (hash !== operation.desiredHash) { throw new Error(`Managed block hash verification failed for ${operation.relativeDestPath}: expected ${operation.desiredHash}, got ${hash}`); } @@ -466,7 +466,7 @@ async function applyOperation( } if (operation.mode === managedInstructionBlockMode) { const selector = managedInstructionSelector(operation.logicalSelector, operation.artifactType, operation.artifactName); - await removeManagedInstructionBlock(operation.destPath, selector, transport, operation.manifestHash); + await removeManagedInstructionBlock(operation.destPath, selector, transport, managedBlockMutationOptions(operation)); } else { await transport.rm(operation.destPath); } @@ -662,6 +662,13 @@ function semanticInstallCommands(operation: InstallOperation): string[][] { return operation.semanticCommand ? [operation.semanticCommand] : []; } +function managedBlockMutationOptions(operation: InstallOperation): { expectedHash?: string; allowDrift?: boolean } { + return { + expectedHash: operation.overrideDrift ? undefined : operation.manifestHash, + allowDrift: operation.overrideDrift === true, + }; +} + async function entryForCompletedOperation( operation: InstallOperation, transport: TargetTransport, diff --git a/src/install/instructions-block.ts b/src/install/instructions-block.ts index 1613254..41c812c 100644 --- a/src/install/instructions-block.ts +++ b/src/install/instructions-block.ts @@ -17,6 +17,11 @@ export interface ManagedInstructionBlockState { drifted: boolean; } +export interface ManagedInstructionBlockMutationOptions { + expectedHash?: string; + allowDrift?: boolean; +} + interface LocatedBlock { start: number; end: number; @@ -55,12 +60,12 @@ export async function writeManagedInstructionBlock( destPath: string, selector: string, transport: TargetTransport, - expectedHash?: string, + options: ManagedInstructionBlockMutationOptions = {}, ): Promise { const source = await readFile(sourcePath, "utf8"); const desired = renderManagedInstructionBlock(selector, source); const existing = await readOptionalText(destPath, transport); - const merged = upsertManagedInstructionBlock(existing ?? "", selector, desired.block, expectedHash); + const merged = upsertManagedInstructionBlock(existing ?? "", selector, desired.block, options); await writeTextWithTransport(destPath, merged, transport); return desired.hash; } @@ -69,11 +74,11 @@ export async function removeManagedInstructionBlock( destPath: string, selector: string, transport: TargetTransport, - expectedHash?: string, + options: ManagedInstructionBlockMutationOptions = {}, ): Promise { if (!(await transport.pathExists(destPath))) return; const existing = await transport.readFile(destPath); - const updated = removeManagedBlockFromContent(existing, selector, expectedHash); + const updated = removeManagedBlockFromContent(existing, selector, options); await writeTextWithTransport(destPath, updated, transport); } @@ -123,26 +128,36 @@ function upsertManagedInstructionBlock( content: string, selector: string, block: string, - expectedHash: string | undefined, + options: ManagedInstructionBlockMutationOptions, ): string { + const { expectedHash, allowDrift = false } = options; const existing = findManagedInstructionBlock(content, selector); if (!existing) { if (expectedHash) throw new Error(`Managed instruction block missing for ${selector}`); return appendManagedInstructionBlock(content, block); } - assertCleanBlock(existing, selector); - if (expectedHash && hashText(existing.body) !== expectedHash) { - throw new Error(`Managed instruction block drift detected for ${selector}`); + if (!allowDrift) { + assertCleanBlock(existing, selector); + if (expectedHash && hashText(existing.body) !== expectedHash) { + throw new Error(`Managed instruction block drift detected for ${selector}`); + } } return `${content.slice(0, existing.start)}${block}${content.slice(existing.end)}`; } -function removeManagedBlockFromContent(content: string, selector: string, expectedHash: string | undefined): string { +function removeManagedBlockFromContent( + content: string, + selector: string, + options: ManagedInstructionBlockMutationOptions, +): string { + const { expectedHash, allowDrift = false } = options; const existing = findManagedInstructionBlock(content, selector); if (!existing) return content; - assertCleanBlock(existing, selector); - if (expectedHash && hashText(existing.body) !== expectedHash) { - throw new Error(`Managed instruction block drift detected for ${selector}`); + if (!allowDrift) { + assertCleanBlock(existing, selector); + if (expectedHash && hashText(existing.body) !== expectedHash) { + throw new Error(`Managed instruction block drift detected for ${selector}`); + } } return `${content.slice(0, existing.start)}${content.slice(existing.end)}`; } diff --git a/src/install/plan.ts b/src/install/plan.ts index c7117d8..606eb74 100644 --- a/src/install/plan.ts +++ b/src/install/plan.ts @@ -67,6 +67,7 @@ export interface InstallOperation { blockedDesiredHash?: string; blockedReason?: string; composedFromDiff?: string[]; + overrideDrift?: boolean; } export interface MigrationReport { @@ -283,6 +284,7 @@ async function createPlanFromOperations( action: "update", currentHash, manifestHash: existing.hash, + overrideDrift: true, reason: reasonWithComposedDiff("force replacing drifted managed destination", op.composedFrom, existing.composedFrom), composedFromDiff, }); @@ -345,7 +347,7 @@ async function createPlanFromOperations( composedFrom: entry.composedFrom, ...operationMetadataFromEntry(entry), ...(options.forceDrift - ? { action: "remove" as const, reason: "force removing drifted stale managed destination" } + ? { action: "remove" as const, reason: "force removing drifted stale managed destination", overrideDrift: true } : {}), }); } else { @@ -574,6 +576,7 @@ async function planManagedBlockOperation( action: "update", currentHash: state.hash, manifestHash: existing.hash, + overrideDrift: true, reason: reasonWithComposedDiff("force replacing drifted managed instruction block", op.composedFrom, existing.composedFrom), composedFromDiff, }]; @@ -611,7 +614,7 @@ async function currentEntryHash( if (entry.mode !== managedInstructionBlockMode) return transport.hashPath(destPath); const selector = managedInstructionSelector("logicalSelector" in entry ? entry.logicalSelector : undefined, entry.artifactType, entry.artifactName); const state = await readManagedInstructionBlockState(destPath, selector, transport); - return state.drifted ? state.markerHash : state.hash; + return state.hash; } async function canStrictlyAdoptLegacyEntry( diff --git a/src/install/uninstall.ts b/src/install/uninstall.ts index e2634cf..e9f4d9f 100644 --- a/src/install/uninstall.ts +++ b/src/install/uninstall.ts @@ -197,7 +197,7 @@ async function currentEntryHash(entry: ManifestEntry, destPath: string, transpor if (entry.mode !== managedInstructionBlockMode) return transport.hashPath(destPath); const selector = managedInstructionSelector("logicalSelector" in entry ? entry.logicalSelector : undefined, entry.artifactType, entry.artifactName); const state = await readManagedInstructionBlockState(destPath, selector, transport); - return state.drifted ? state.markerHash : state.hash; + return state.hash; } function operationMetadataFromEntry(entry: ManifestEntry, ownersOverride?: string[]): Pick< diff --git a/test/instructions-block.test.ts b/test/instructions-block.test.ts index f281dbf..273b828 100644 --- a/test/instructions-block.test.ts +++ b/test/instructions-block.test.ts @@ -67,6 +67,45 @@ describe("managed instruction blocks", () => { expect(manifestEntrySchema.parse(entry).mode).toBe("managed-block"); }); + it("keeps drifted managed blocks blocked unless force-drift is requested", async () => { + const sourceRoot = await tempRoot(); + const targetRoot = await tempRoot(); + const first = await instructionArtifact(sourceRoot, "Use the managed guidance.\n"); + await applyCombinedInstallPlan(await createCombinedInstallPlan([first], managedInstructionsAdapter, targetRoot)); + + const dest = join(targetRoot, "AGENTS.md"); + await driftManagedBlock(dest, "Use the managed guidance.\n"); + const updated = await instructionArtifact(sourceRoot, "Use the updated guidance.\n"); + const manifest = await readInstallManifest(targetRoot, managedInstructionsAdapter.name); + const driftPlan = await createCombinedInstallPlan([updated], managedInstructionsAdapter, targetRoot, manifest); + + expect(driftPlan.hasBlockingChanges).toBe(true); + expect(driftPlan.operations).toMatchObject([{ action: "drift", mode: "managed-block" }]); + await expect(applyCombinedInstallPlan(driftPlan)).rejects.toThrow(/Refusing to apply with blocking changes/); + }); + + it("force-drift replaces a drifted managed block", async () => { + const sourceRoot = await tempRoot(); + const targetRoot = await tempRoot(); + const first = await instructionArtifact(sourceRoot, "Use the managed guidance.\n"); + await applyCombinedInstallPlan(await createCombinedInstallPlan([first], managedInstructionsAdapter, targetRoot)); + + const dest = join(targetRoot, "AGENTS.md"); + await driftManagedBlock(dest, "Use the managed guidance.\n"); + const updated = await instructionArtifact(sourceRoot, "Use the updated guidance.\n"); + const manifest = await readInstallManifest(targetRoot, managedInstructionsAdapter.name); + const forcedPlan = await createCombinedInstallPlan([updated], managedInstructionsAdapter, targetRoot, manifest, localTransport, { forceDrift: true }); + + expect(forcedPlan.hasBlockingChanges).toBe(false); + expect(forcedPlan.operations).toMatchObject([{ action: "update", mode: "managed-block", overrideDrift: true }]); + + await applyCombinedInstallPlan(forcedPlan); + const content = await readFile(dest, "utf8"); + expect(content).toContain("Use the updated guidance."); + expect(content).not.toContain("manual edit"); + expect(content).not.toContain("Use the managed guidance."); + }); + it("skips Claude instruction writes when CLAUDE.md imports AGENTS.md", async () => { const sourceRoot = await tempRoot(); const targetRoot = await tempRoot(); @@ -160,6 +199,28 @@ describe("managed instruction blocks", () => { expect(content).not.toContain("openpack:include instructions/AGENTS.md"); expect(content).not.toContain("Managed guidance."); }); + + it("force uninstall removes a drifted managed block while preserving user content", async () => { + const sourceRoot = await tempRoot(); + const targetRoot = await tempRoot(); + const artifact = await instructionArtifact(sourceRoot, "Managed guidance.\n"); + await applyCombinedInstallPlan(await createCombinedInstallPlan([artifact], managedInstructionsAdapter, targetRoot)); + + const dest = join(targetRoot, "AGENTS.md"); + await writeFile(dest, `# Keep me\n\n${await readFile(dest, "utf8")}# Also keep me\n`, "utf8"); + await driftManagedBlock(dest, "Managed guidance.\n"); + const manifest = await readInstallManifest(targetRoot, managedInstructionsAdapter.name); + const uninstallPlan = await createUninstallPlan(manifest!); + expect(uninstallPlan.operations).toMatchObject([{ action: "keep", mode: "managed-block" }]); + + await uninstall(uninstallPlan, { force: true }); + const content = await readFile(dest, "utf8"); + expect(content).toContain("# Keep me"); + expect(content).toContain("# Also keep me"); + expect(content).not.toContain("openpack:include instructions/AGENTS.md"); + expect(content).not.toContain("Managed guidance."); + expect(content).not.toContain("manual edit"); + }); }); async function instructionArtifact(root: string, content: string, name = "AGENTS.md"): Promise { @@ -182,3 +243,10 @@ async function instructionArtifact(root: string, content: string, name = "AGENTS }, }; } + +async function driftManagedBlock(path: string, needle: string): Promise { + const content = await readFile(path, "utf8"); + const drifted = content.replace(needle, `${needle}manual edit\n`); + if (drifted === content) throw new Error(`test fixture did not contain '${needle.trim()}'`); + await writeFile(path, drifted, "utf8"); +}