From dcd47e65f50d9c40ce450bef90effe48dcc631b5 Mon Sep 17 00:00:00 2001 From: AgentSeal Date: Fri, 3 Jul 2026 11:20:59 +0200 Subject: [PATCH] fix(act): refuse to apply a plan whose target changed since it was built Plans serialize the full post-edit file content at build time, so a target edited between the preview and the interactive confirm would be silently overwritten with stale content (recoverable via backup, but a silent violation of the dry-run-shows-exactly-what-changes principle). - PlannedChange edit/create variants gain optional expectedHash: sha256 of the raw on-disk bytes the plan was built from (hashed before the BOM strip), null when the plan expects the file to be absent, undefined to skip validation (framework back-compat). - plans.ts sets it everywhere a target is read: ConfigDocs hashes the raw buffer it parses, and the marker builders hash the file behind a shared markerChange helper. - runAction validates all expected hashes after snapshotting and before the first mutation; a mismatch throws " changed since the plan was built; re-run codeburn optimize --apply", removes the backup dir, and journals nothing. No rollback is involved since nothing mutated. Tests: stale edit target rejected with no journal record and no backup dir left, expects-absent plan rejected when the file appeared, matching hash still applies, and a hash-less change still applies unchecked. --- src/act/apply.ts | 10 ++++++ src/act/plans.ts | 42 ++++++++++++++++------ src/act/types.ts | 9 +++-- tests/optimize-apply.test.ts | 67 ++++++++++++++++++++++++++++++++++++ 4 files changed, 115 insertions(+), 13 deletions(-) diff --git a/src/act/apply.ts b/src/act/apply.ts index 8f4b507..7daba0c 100644 --- a/src/act/apply.ts +++ b/src/act/apply.ts @@ -41,6 +41,16 @@ export async function runAction(plan: ActionPlan, actionsDir: string = defaultAc const done: number[] = [] try { + // Stale-plan guard: plans carry full post-edit content computed at + // build time, so refuse if a target changed between preview and + // confirm. Runs before any mutation; failure needs no rollback (the + // catch below only removes the backup dir). + for (const pc of plan.changes) { + if (pc.op === 'move' || pc.expectedHash === undefined) continue + if ((await sha256File(pc.path)) !== pc.expectedHash) { + throw new Error(`${pc.path} changed since the plan was built; re-run codeburn optimize --apply`) + } + } for (let i = 0; i < plan.changes.length; i++) { const pc = plan.changes[i]! if (pc.op === 'move') { diff --git a/src/act/plans.ts b/src/act/plans.ts index 9e12f66..861d4c8 100644 --- a/src/act/plans.ts +++ b/src/act/plans.ts @@ -2,6 +2,7 @@ import { existsSync, readFileSync } from 'fs' import { isAbsolute, join } from 'path' import { homedir } from 'os' import type { ActionKind, ActionPlan, PlannedChange } from './types.js' +import { sha256 } from './backup.js' import type { WasteFinding } from '../optimize.js' // Turns an optimize finding into a concrete, journaled file-mutation plan. @@ -107,7 +108,16 @@ function findServerKey(container: Record | undefined, server: s return null } -type DocState = { path: string; doc: Record; existed: boolean; dirty: boolean } +type DocState = { + path: string + doc: Record + existed: boolean + dirty: boolean + // sha256 of the raw bytes the doc was parsed from (before the BOM strip); + // null when the file did not exist. Becomes the change's expectedHash so + // runAction refuses to apply over a file edited after the plan was built. + rawHash: string | null +} // Reads each config file at most once, tracks parse errors, and emits one // PlannedChange per file it actually mutated. @@ -119,22 +129,24 @@ class ConfigDocs { load(path: string): DocState | null { if (this.docs.has(path)) return this.docs.get(path)! if (!existsSync(path)) { - const state: DocState = { path, doc: {}, existed: false, dirty: false } + const state: DocState = { path, doc: {}, existed: false, dirty: false, rawHash: null } this.docs.set(path, state) return state } - let raw: string + let buf: Buffer try { - raw = readFileSync(path, 'utf-8') + buf = readFileSync(path) } catch (e) { this.errors.set(path, `could not read ${shortPath(path, this.homeDir)}: ${errMessage(e)}`) this.docs.set(path, null) return null } + const rawHash = sha256(buf) + let raw = buf.toString('utf-8') if (raw.charCodeAt(0) === 0xfeff) raw = raw.slice(1) try { const doc = JSON.parse(raw) as Record - const state: DocState = { path, doc, existed: true, dirty: false } + const state: DocState = { path, doc, existed: true, dirty: false, rawHash } this.docs.set(path, state) return state } catch (e) { @@ -152,6 +164,7 @@ class ConfigDocs { op: state.existed ? 'edit' : 'create', path: state.path, content: JSON.stringify(state.doc, null, 2) + '\n', + expectedHash: state.rawHash, }) } } @@ -393,17 +406,26 @@ function upsertMarkerBlock(existing: string | null, id: string, text: string, st return existing.endsWith('\n') ? existing + block : existing + '\n' + block } +function markerChange(target: string, id: string, text: string, style: 'html' | 'hash'): PlannedChange { + const buf = existsSync(target) ? readFileSync(target) : null + const existing = buf === null ? null : buf.toString('utf-8') + return { + op: buf === null ? 'create' : 'edit', + path: target, + content: upsertMarkerBlock(existing, id, text, style), + expectedHash: buf === null ? null : sha256(buf), + } +} + function buildClaudeMdRule(finding: WasteFinding, r: ResolvedPaths): BuiltPlan { if (finding.fix.type !== 'paste') return { plan: null, notes: [] } const target = r.projectClaudeMd - const existing = existsSync(target) ? readFileSync(target, 'utf-8') : null - const content = upsertMarkerBlock(existing, finding.id, finding.fix.text, 'html') return { plan: { kind: 'claude-md-rule', findingId: finding.id, description: `Add the ${finding.id} rule block to ${shortPath(target, r.homeDir)}`, - changes: [{ op: existing === null ? 'create' : 'edit', path: target, content }], + changes: [markerChange(target, finding.id, finding.fix.text, 'html')], }, notes: [], } @@ -412,14 +434,12 @@ function buildClaudeMdRule(finding: WasteFinding, r: ResolvedPaths): BuiltPlan { function buildShellConfig(finding: WasteFinding, r: ResolvedPaths): BuiltPlan { if (finding.fix.type !== 'paste') return { plan: null, notes: [] } const target = r.shellRc - const existing = existsSync(target) ? readFileSync(target, 'utf-8') : null - const content = upsertMarkerBlock(existing, finding.id, finding.fix.text, 'hash') return { plan: { kind: 'shell-config', findingId: finding.id, description: `Set the bash output cap in ${shortPath(target, r.homeDir)}`, - changes: [{ op: existing === null ? 'create' : 'edit', path: target, content }], + changes: [markerChange(target, finding.id, finding.fix.text, 'hash')], }, notes: [], } diff --git a/src/act/types.ts b/src/act/types.ts index 38f1a0d..4e8eff9 100644 --- a/src/act/types.ts +++ b/src/act/types.ts @@ -26,9 +26,14 @@ export type ActionRecord = { baseline?: Record } +// expectedHash: sha256 of the raw on-disk bytes the plan's content was +// computed from (null when the plan expects the file to be absent). runAction +// refuses to apply when the target no longer matches, so a file edited +// between preview and confirm is never silently clobbered with stale +// content. undefined skips the check. export type PlannedChange = - | { op: 'edit'; path: string; content: string | Buffer } - | { op: 'create'; path: string; content: string | Buffer } + | { op: 'edit'; path: string; content: string | Buffer; expectedHash?: string | null } + | { op: 'create'; path: string; content: string | Buffer; expectedHash?: string | null } | { op: 'move'; path: string; movedTo: string } export type ActionPlan = { diff --git a/tests/optimize-apply.test.ts b/tests/optimize-apply.test.ts index 6c8c330..2b58228 100644 --- a/tests/optimize-apply.test.ts +++ b/tests/optimize-apply.test.ts @@ -586,3 +586,70 @@ describe('runOptimizeApply end-to-end', () => { expect(out).toContain('could not parse') }) }) + +describe('stale-plan detection', () => { + it('rejects when the target changed after the plan was built, leaving nothing behind', async () => { + const fx = await makeFixture() + const claudeJson = join(fx.home, '.claude.json') + await writeFile(claudeJson, JSON.stringify({ mcpServers: { s: {} } }, null, 2) + '\n') + const plan = planFor( + makeFinding('mcp-low-coverage', CMD_FIX, { kind: 'mcp-remove', servers: ['s'] }), + { homeDir: fx.home, cwd: fx.project }, + ) + expect(plan).not.toBeNull() + + const interim = JSON.stringify({ mcpServers: { s: {}, addedMeanwhile: {} } }, null, 2) + '\n' + await writeFile(claudeJson, interim) + + await expect(runAction(plan!, fx.actionsDir)).rejects.toThrow(/changed since the plan was built; re-run codeburn optimize --apply/) + expect(await readFile(claudeJson, 'utf-8')).toBe(interim) + expect(await readRecords(fx.actionsDir)).toHaveLength(0) + const backups = await readdir(join(fx.actionsDir, 'backups')).catch(() => []) + expect(backups).toEqual([]) + }) + + it('rejects a plan expecting an absent file when the file appeared', async () => { + const fx = await makeFixture() + const claudeMd = join(fx.project, 'CLAUDE.md') + const plan = planFor( + makeFinding('read-edit-ratio', { type: 'paste', destination: 'claude-md', label: '', text: 'rule' }), + { homeDir: fx.home, cwd: fx.project }, + ) + expect(plan!.changes[0]).toMatchObject({ op: 'create', expectedHash: null }) + + await writeFile(claudeMd, '# appeared meanwhile\n') + + await expect(runAction(plan!, fx.actionsDir)).rejects.toThrow(/changed since the plan was built/) + expect(await readFile(claudeMd, 'utf-8')).toBe('# appeared meanwhile\n') + expect(await readRecords(fx.actionsDir)).toHaveLength(0) + }) + + it('applies when the target still matches the expected hash', async () => { + const fx = await makeFixture() + const claudeJson = join(fx.home, '.claude.json') + await writeFile(claudeJson, JSON.stringify({ mcpServers: { s: {} } }, null, 2) + '\n') + const plan = planFor( + makeFinding('mcp-low-coverage', CMD_FIX, { kind: 'mcp-remove', servers: ['s'] }), + { homeDir: fx.home, cwd: fx.project }, + ) + expect(plan!.changes[0]!.op === 'move' ? undefined : plan!.changes[0]!.expectedHash).toMatch(/^[0-9a-f]{64}$/) + + const rec = await runAction(plan!, fx.actionsDir) + expect(rec.status).toBe('applied') + expect(JSON.parse(await readFile(claudeJson, 'utf-8')).mcpServers).toEqual({}) + }) + + it('a change without expectedHash skips validation (framework back-compat)', async () => { + const fx = await makeFixture() + const p = join(fx.home, 'free.txt') + await writeFile(p, 'anything at all') + + const rec = await runAction({ + kind: 'claude-md-rule', + description: 'no expected hash', + changes: [{ op: 'edit', path: p, content: 'overwritten' }], + }, fx.actionsDir) + expect(rec.status).toBe('applied') + expect(await readFile(p, 'utf-8')).toBe('overwritten') + }) +})