mirror of
https://github.com/AgentSeal/codeburn.git
synced 2026-07-10 01:29:41 +00:00
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 "<path> 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.
This commit is contained in:
parent
92d970196e
commit
dcd47e65f5
4 changed files with 115 additions and 13 deletions
|
|
@ -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') {
|
||||
|
|
|
|||
|
|
@ -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<string, unknown> | undefined, server: s
|
|||
return null
|
||||
}
|
||||
|
||||
type DocState = { path: string; doc: Record<string, unknown>; existed: boolean; dirty: boolean }
|
||||
type DocState = {
|
||||
path: string
|
||||
doc: Record<string, unknown>
|
||||
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<string, unknown>
|
||||
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: [],
|
||||
}
|
||||
|
|
|
|||
|
|
@ -26,9 +26,14 @@ export type ActionRecord = {
|
|||
baseline?: Record<string, number>
|
||||
}
|
||||
|
||||
// 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 = {
|
||||
|
|
|
|||
|
|
@ -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')
|
||||
})
|
||||
})
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue