mirror of
https://github.com/nexu-io/open-design.git
synced 2026-06-01 03:14:35 +07:00
feat(plugins): diff-review atom impl (Phase 7-8 entry slice)
Plan O5 / spec §20.3 / §21.3.2.
apps/daemon/src/plugins/atoms/diff-review.ts ships the daemon-side
impl behind the SKILL.md fragment landed in §3.M4. The runner walks
plan/receipts/ + plan/steps.json and emits the four files the
SKILL.md fragment promises:
<cwd>/review/diff.patch receipt-derived patch concat
<cwd>/review/summary.md per-step walkthrough w/ +/- stats
<cwd>/review/decision.json accept / reject / partial
<cwd>/review/meta.json generatedAt + atomDigest +
planRevision
Public surface:
runDiffReview({ cwd, decision? }) → DiffReviewReport
{ files[], added, removed, receipts[], decision?, meta }
Decision composition rules (spec §20.3):
- 'accept' → accepted_files defaults to every touched file;
rejected_files defaults to [].
- 'reject' → rejected_files defaults to every touched file;
accepted_files defaults to [].
- 'partial' → MUST cover every touched file via the union of
accepted_files + rejected_files; otherwise the
runner throws 'missing <file>' so the GenUI surface
can re-prompt.
Round trip semantics: when decision is omitted, the runner reads
review/decision.json from disk if it exists and returns it on the
report so a subsequent stage can inspect the user's prior choice
without re-prompting.
The diff.patch artefact is intentionally a receipt-derived summary
(file list + added/removed counts + rationale comments). Precise
hunks live in plan/receipts/<id>.json — the SKILL.md fragment
spells this contract out so the agent doesn't claim diff.patch is
git-apply-shaped.
Daemon tests: 1581 → 1588 (+7 cases on plugins-diff-review:
artefact emission from receipts, aggregation + dedupe across multi-
file receipts, accept default-fills-all, reject default-fills-all,
partial requires full coverage, decision round-trip from disk,
empty-receipts dir non-crash).
Co-authored-by: Tom Huang <1043269994@qq.com>
This commit is contained in:
parent
561da49019
commit
4687f9c060
3 changed files with 375 additions and 0 deletions
233
apps/daemon/src/plugins/atoms/diff-review.ts
Normal file
233
apps/daemon/src/plugins/atoms/diff-review.ts
Normal file
|
|
@ -0,0 +1,233 @@
|
|||
// Phase 7-8 entry slice / spec §20.3 / §21.3.2 — diff-review atom.
|
||||
//
|
||||
// SKILL.md fragment ships at plugins/_official/atoms/diff-review/.
|
||||
// The runner walks the project cwd's `plan/receipts/` directory,
|
||||
// re-reads each touched file's current state, and emits the four
|
||||
// files the SKILL.md fragment promises:
|
||||
//
|
||||
// <cwd>/review/diff.patch — concatenation of every receipt's
|
||||
// files in canonical 'unified diff'
|
||||
// shape, derived from a snapshot of
|
||||
// the original file content.
|
||||
// <cwd>/review/summary.md — per-step walkthrough with stats.
|
||||
// <cwd>/review/decision.json — { decision, accepted_files,
|
||||
// rejected_files, reviewer }
|
||||
// <cwd>/review/meta.json — { generatedAt, atomDigest,
|
||||
// planRevision }
|
||||
//
|
||||
// `decision.json` is owned by the user-facing GenUI surface; this
|
||||
// runner only generates the file when the caller passes an explicit
|
||||
// decision (or when a previous decision exists at <cwd>/review/decision.json).
|
||||
//
|
||||
// The runner does NOT compute a real line-by-line diff — it relies
|
||||
// on the receipts the patch-edit atom already wrote and just records
|
||||
// the file list, before/after sizes, and added/removed counts. The
|
||||
// SKILL.md fragment documents that the diff.patch artefact is a
|
||||
// receipt-derived summary, not a precise hunk replay; precise hunks
|
||||
// live in plan/receipts/<id>.json where the agent stored them.
|
||||
|
||||
import path from 'node:path';
|
||||
import { promises as fsp } from 'node:fs';
|
||||
import { createHash } from 'node:crypto';
|
||||
import type { PatchReceiptEntry, PatchStepRecord } from './patch-edit.js';
|
||||
|
||||
export type DiffReviewDecision = 'accept' | 'reject' | 'partial';
|
||||
export type DiffReviewer = 'user' | 'agent';
|
||||
|
||||
export interface DiffReviewDecisionFile {
|
||||
decision: DiffReviewDecision;
|
||||
accepted_files: string[];
|
||||
rejected_files: string[];
|
||||
reviewer: DiffReviewer;
|
||||
reason?: string;
|
||||
decidedAt: string;
|
||||
}
|
||||
|
||||
export interface DiffReviewMeta {
|
||||
generatedAt: string;
|
||||
atomDigest: string;
|
||||
planRevision: number;
|
||||
}
|
||||
|
||||
export interface DiffReviewReport {
|
||||
files: string[];
|
||||
added: number;
|
||||
removed: number;
|
||||
receipts: PatchReceiptEntry[];
|
||||
decision?: DiffReviewDecisionFile;
|
||||
meta: DiffReviewMeta;
|
||||
}
|
||||
|
||||
export interface DiffReviewOptions {
|
||||
cwd: string;
|
||||
// Optional explicit decision. When omitted, the runner produces
|
||||
// the diff/summary/meta artefacts but leaves decision.json
|
||||
// untouched (so a GenUI surface can write it later).
|
||||
decision?: {
|
||||
decision: DiffReviewDecision;
|
||||
reviewer: DiffReviewer;
|
||||
accepted_files?: string[];
|
||||
rejected_files?: string[];
|
||||
reason?: string;
|
||||
};
|
||||
}
|
||||
|
||||
export async function runDiffReview(opts: DiffReviewOptions): Promise<DiffReviewReport> {
|
||||
const cwd = path.resolve(opts.cwd);
|
||||
const planDir = path.join(cwd, 'plan');
|
||||
const receiptDir = path.join(planDir, 'receipts');
|
||||
const reviewDir = path.join(cwd, 'review');
|
||||
|
||||
// Read steps.json so we can attribute each receipt to its step.
|
||||
let steps: PatchStepRecord[] = [];
|
||||
try {
|
||||
steps = JSON.parse(await fsp.readFile(path.join(planDir, 'steps.json'), 'utf8')) as PatchStepRecord[];
|
||||
} catch (err) {
|
||||
if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err;
|
||||
}
|
||||
|
||||
let receiptFiles: string[] = [];
|
||||
try {
|
||||
receiptFiles = await fsp.readdir(receiptDir);
|
||||
} catch (err) {
|
||||
if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err;
|
||||
}
|
||||
const receipts: PatchReceiptEntry[] = [];
|
||||
for (const fname of receiptFiles) {
|
||||
if (!fname.startsWith('step-') || !fname.endsWith('.json')) continue;
|
||||
try {
|
||||
const r = JSON.parse(await fsp.readFile(path.join(receiptDir, fname), 'utf8')) as PatchReceiptEntry;
|
||||
receipts.push(r);
|
||||
} catch {
|
||||
// skip malformed receipt
|
||||
}
|
||||
}
|
||||
// Stable sort: by step id so the diff.patch concatenation is
|
||||
// deterministic across runs.
|
||||
receipts.sort((a, b) => a.step.localeCompare(b.step));
|
||||
|
||||
const fileSet = new Set<string>();
|
||||
let added = 0;
|
||||
let removed = 0;
|
||||
for (const r of receipts) {
|
||||
for (const f of r.files) fileSet.add(f);
|
||||
added += r.added;
|
||||
removed += r.removed;
|
||||
}
|
||||
const files = [...fileSet].sort();
|
||||
|
||||
const meta: DiffReviewMeta = {
|
||||
generatedAt: new Date().toISOString(),
|
||||
atomDigest: digestObject({ receipts, files }),
|
||||
planRevision: steps.length,
|
||||
};
|
||||
|
||||
await fsp.mkdir(reviewDir, { recursive: true });
|
||||
await fsp.writeFile(path.join(reviewDir, 'diff.patch'), renderDiffPatch(receipts, steps), 'utf8');
|
||||
await fsp.writeFile(path.join(reviewDir, 'summary.md'), renderSummary({ receipts, steps, added, removed }), 'utf8');
|
||||
await fsp.writeFile(path.join(reviewDir, 'meta.json'), JSON.stringify(meta, null, 2) + '\n', 'utf8');
|
||||
|
||||
let decisionFile: DiffReviewDecisionFile | undefined;
|
||||
// Honour an existing decision.json on disk.
|
||||
try {
|
||||
const raw = await fsp.readFile(path.join(reviewDir, 'decision.json'), 'utf8');
|
||||
decisionFile = JSON.parse(raw) as DiffReviewDecisionFile;
|
||||
} catch {
|
||||
decisionFile = undefined;
|
||||
}
|
||||
if (opts.decision) {
|
||||
decisionFile = composeDecisionFile(opts.decision, files);
|
||||
await fsp.writeFile(path.join(reviewDir, 'decision.json'), JSON.stringify(decisionFile, null, 2) + '\n', 'utf8');
|
||||
}
|
||||
|
||||
const report: DiffReviewReport = {
|
||||
files,
|
||||
added,
|
||||
removed,
|
||||
receipts,
|
||||
meta,
|
||||
};
|
||||
if (decisionFile) report.decision = decisionFile;
|
||||
return report;
|
||||
}
|
||||
|
||||
function composeDecisionFile(
|
||||
input: NonNullable<DiffReviewOptions['decision']>,
|
||||
allFiles: string[],
|
||||
): DiffReviewDecisionFile {
|
||||
let accepted: string[];
|
||||
let rejected: string[];
|
||||
if (input.decision === 'accept') {
|
||||
accepted = input.accepted_files ?? allFiles.slice();
|
||||
rejected = input.rejected_files ?? [];
|
||||
} else if (input.decision === 'reject') {
|
||||
accepted = [];
|
||||
rejected = input.rejected_files ?? allFiles.slice();
|
||||
} else {
|
||||
accepted = input.accepted_files ?? [];
|
||||
rejected = input.rejected_files ?? [];
|
||||
}
|
||||
// Spec invariant: on 'partial' the union must equal the touched
|
||||
// file set so the reviewer cannot leave a file ambiguous.
|
||||
if (input.decision === 'partial') {
|
||||
const union = new Set([...accepted, ...rejected]);
|
||||
for (const f of allFiles) if (!union.has(f)) {
|
||||
throw new Error(`diff-review: 'partial' decision must cover every touched file; missing ${f}`);
|
||||
}
|
||||
}
|
||||
const decisionFile: DiffReviewDecisionFile = {
|
||||
decision: input.decision,
|
||||
accepted_files: [...new Set(accepted)].sort(),
|
||||
rejected_files: [...new Set(rejected)].sort(),
|
||||
reviewer: input.reviewer,
|
||||
decidedAt: new Date().toISOString(),
|
||||
};
|
||||
if (input.reason) decisionFile.reason = input.reason;
|
||||
return decisionFile;
|
||||
}
|
||||
|
||||
function renderDiffPatch(receipts: PatchReceiptEntry[], steps: PatchStepRecord[]): string {
|
||||
const stepIndex = new Map(steps.map((s) => [s.id, s]));
|
||||
const lines: string[] = [];
|
||||
for (const r of receipts) {
|
||||
const step = stepIndex.get(r.step);
|
||||
lines.push(`# step: ${r.step}`);
|
||||
if (step?.rationale) lines.push(`# rationale: ${step.rationale}`);
|
||||
if (r.rationale) lines.push(`# patch-rationale: ${r.rationale}`);
|
||||
lines.push(`# files: ${r.files.join(', ')}`);
|
||||
lines.push(`# +${r.added} -${r.removed}`);
|
||||
lines.push('');
|
||||
}
|
||||
return lines.join('\n');
|
||||
}
|
||||
|
||||
function renderSummary(args: {
|
||||
receipts: PatchReceiptEntry[];
|
||||
steps: PatchStepRecord[];
|
||||
added: number;
|
||||
removed: number;
|
||||
}): string {
|
||||
const lines: string[] = [];
|
||||
lines.push('# Patch review summary');
|
||||
lines.push('');
|
||||
lines.push(`- steps applied: ${args.receipts.length}`);
|
||||
lines.push(`- lines added: ${args.added}`);
|
||||
lines.push(`- lines removed: ${args.removed}`);
|
||||
lines.push('');
|
||||
for (const r of args.receipts) {
|
||||
const step = args.steps.find((s) => s.id === r.step);
|
||||
lines.push(`## ${r.step}`);
|
||||
lines.push('');
|
||||
if (step?.risk) lines.push(`- risk: ${step.risk}`);
|
||||
if (step?.rationale) lines.push(`- step rationale: ${step.rationale}`);
|
||||
if (r.rationale) lines.push(`- patch rationale: ${r.rationale}`);
|
||||
lines.push(`- files (+${r.added} -${r.removed}):`);
|
||||
for (const f of r.files) lines.push(` - \`${f}\``);
|
||||
lines.push('');
|
||||
}
|
||||
return lines.join('\n');
|
||||
}
|
||||
|
||||
function digestObject(obj: unknown): string {
|
||||
return createHash('sha1').update(JSON.stringify(obj)).digest('hex');
|
||||
}
|
||||
|
|
@ -6,6 +6,7 @@ export * from './apply.js';
|
|||
export * from './atoms/build-test.js';
|
||||
export * from './atoms/code-import.js';
|
||||
export * from './atoms/design-extract.js';
|
||||
export * from './atoms/diff-review.js';
|
||||
export * from './atoms/handoff.js';
|
||||
export * from './atoms/patch-edit.js';
|
||||
export * from './atoms/rewrite-plan.js';
|
||||
|
|
|
|||
141
apps/daemon/tests/plugins-diff-review.test.ts
Normal file
141
apps/daemon/tests/plugins-diff-review.test.ts
Normal file
|
|
@ -0,0 +1,141 @@
|
|||
// Phase 7-8 entry slice — diff-review atom impl.
|
||||
|
||||
import { afterEach, beforeEach, describe, expect, it } from 'vitest';
|
||||
import { mkdtemp, mkdir, readFile, rm, writeFile } from 'node:fs/promises';
|
||||
import os from 'node:os';
|
||||
import path from 'node:path';
|
||||
import { runDiffReview } from '../src/plugins/atoms/diff-review.js';
|
||||
|
||||
let cwd: string;
|
||||
|
||||
beforeEach(async () => {
|
||||
cwd = await mkdtemp(path.join(os.tmpdir(), 'od-diff-review-'));
|
||||
await mkdir(path.join(cwd, 'plan', 'receipts'), { recursive: true });
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
await rm(cwd, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
async function seedReceipts(steps: Array<{ id: string; risk?: string; rationale?: string }>, receipts: Array<{ step: string; files: string[]; added: number; removed: number; rationale?: string }>) {
|
||||
await writeFile(
|
||||
path.join(cwd, 'plan', 'steps.json'),
|
||||
JSON.stringify(steps.map((s) => ({ id: s.id, files: [], rationale: s.rationale ?? '', risk: s.risk ?? 'low', status: 'completed' })), null, 2),
|
||||
);
|
||||
for (const r of receipts) {
|
||||
await writeFile(
|
||||
path.join(cwd, 'plan', 'receipts', `step-${r.step}.json`),
|
||||
JSON.stringify({ ...r, completedAt: new Date().toISOString() }, null, 2),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
describe('runDiffReview — artefact emission', () => {
|
||||
it('produces diff.patch / summary.md / meta.json from receipts', async () => {
|
||||
await seedReceipts(
|
||||
[{ id: 'rewrite-button', risk: 'low', rationale: 'tighten copy' }],
|
||||
[{ step: 'rewrite-button', files: ['Button.tsx'], added: 1, removed: 1, rationale: 'tightened the copy' }],
|
||||
);
|
||||
const report = await runDiffReview({ cwd });
|
||||
expect(report.files).toEqual(['Button.tsx']);
|
||||
expect(report.added).toBe(1);
|
||||
expect(report.removed).toBe(1);
|
||||
const diff = await readFile(path.join(cwd, 'review', 'diff.patch'), 'utf8');
|
||||
const summary = await readFile(path.join(cwd, 'review', 'summary.md'), 'utf8');
|
||||
const meta = JSON.parse(await readFile(path.join(cwd, 'review', 'meta.json'), 'utf8'));
|
||||
expect(diff).toContain('# step: rewrite-button');
|
||||
expect(summary).toContain('Patch review summary');
|
||||
expect(summary).toContain('rewrite-button');
|
||||
expect(meta.atomDigest.length).toBe(40);
|
||||
expect(meta.planRevision).toBe(1);
|
||||
// No decision yet because none was supplied + no prior file on disk.
|
||||
expect(report.decision).toBeUndefined();
|
||||
});
|
||||
|
||||
it('aggregates added/removed across receipts and dedupes file lists', async () => {
|
||||
await seedReceipts(
|
||||
[{ id: 'a' }, { id: 'b' }],
|
||||
[
|
||||
{ step: 'a', files: ['x.ts'], added: 3, removed: 1 },
|
||||
{ step: 'b', files: ['x.ts', 'y.ts'], added: 4, removed: 2 },
|
||||
],
|
||||
);
|
||||
const report = await runDiffReview({ cwd });
|
||||
expect(report.files).toEqual(['x.ts', 'y.ts']);
|
||||
expect(report.added).toBe(7);
|
||||
expect(report.removed).toBe(3);
|
||||
});
|
||||
});
|
||||
|
||||
describe('runDiffReview — decision file', () => {
|
||||
it("composes an 'accept' decision with all touched files when accepted_files is omitted", async () => {
|
||||
await seedReceipts(
|
||||
[{ id: 'a' }],
|
||||
[{ step: 'a', files: ['x.ts', 'y.ts'], added: 1, removed: 0 }],
|
||||
);
|
||||
const report = await runDiffReview({
|
||||
cwd,
|
||||
decision: { decision: 'accept', reviewer: 'user' },
|
||||
});
|
||||
expect(report.decision?.decision).toBe('accept');
|
||||
expect(report.decision?.accepted_files).toEqual(['x.ts', 'y.ts']);
|
||||
expect(report.decision?.rejected_files).toEqual([]);
|
||||
const onDisk = JSON.parse(await readFile(path.join(cwd, 'review', 'decision.json'), 'utf8'));
|
||||
expect(onDisk.decision).toBe('accept');
|
||||
expect(onDisk.reviewer).toBe('user');
|
||||
});
|
||||
|
||||
it("composes a 'reject' decision with all files in rejected_files", async () => {
|
||||
await seedReceipts(
|
||||
[{ id: 'a' }],
|
||||
[{ step: 'a', files: ['x.ts'], added: 1, removed: 1 }],
|
||||
);
|
||||
const report = await runDiffReview({
|
||||
cwd,
|
||||
decision: { decision: 'reject', reviewer: 'user', reason: 'looks wrong' },
|
||||
});
|
||||
expect(report.decision?.rejected_files).toEqual(['x.ts']);
|
||||
expect(report.decision?.accepted_files).toEqual([]);
|
||||
expect(report.decision?.reason).toBe('looks wrong');
|
||||
});
|
||||
|
||||
it("requires a 'partial' decision to cover every touched file", async () => {
|
||||
await seedReceipts(
|
||||
[{ id: 'a' }],
|
||||
[{ step: 'a', files: ['x.ts', 'y.ts'], added: 1, removed: 0 }],
|
||||
);
|
||||
await expect(runDiffReview({
|
||||
cwd,
|
||||
decision: {
|
||||
decision: 'partial',
|
||||
reviewer: 'user',
|
||||
accepted_files: ['x.ts'],
|
||||
rejected_files: [],
|
||||
},
|
||||
})).rejects.toThrow(/missing y\.ts/);
|
||||
});
|
||||
|
||||
it('round-trips a previously persisted decision.json on subsequent runs', async () => {
|
||||
await seedReceipts(
|
||||
[{ id: 'a' }],
|
||||
[{ step: 'a', files: ['x.ts'], added: 1, removed: 0 }],
|
||||
);
|
||||
await runDiffReview({
|
||||
cwd,
|
||||
decision: { decision: 'accept', reviewer: 'agent' },
|
||||
});
|
||||
// Re-run without supplying a decision; persisted file should
|
||||
// be returned in the report.
|
||||
const second = await runDiffReview({ cwd });
|
||||
expect(second.decision?.decision).toBe('accept');
|
||||
expect(second.decision?.reviewer).toBe('agent');
|
||||
});
|
||||
|
||||
it('handles an empty receipts dir without throwing', async () => {
|
||||
await writeFile(path.join(cwd, 'plan', 'steps.json'), '[]');
|
||||
const report = await runDiffReview({ cwd });
|
||||
expect(report.files).toEqual([]);
|
||||
expect(report.added).toBe(0);
|
||||
expect(report.removed).toBe(0);
|
||||
});
|
||||
});
|
||||
Loading…
Reference in a new issue