From de9546cbf63cad43bd457a14e8f684c9a54ae49a Mon Sep 17 00:00:00 2001 From: Art Pai Date: Tue, 15 Sep 2026 21:51:11 +1000 Subject: [PATCH 1/2] Capture binary file changes as explainable review blocks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Binary sides keep their byte size and SHA-256 hash instead of aborting the capture, so a change set mixing text and binary files can be captured and explained. Binary changes render as metadata cards with their path, status, modes, and before/after sizes, and their hashes participate in capture identity. Text-only captures hash exactly as before, keeping existing walks and explanations matched. Co-Authored-By: ことね --- README.md | 2 + docs/architecture.md | 15 ++- docs/usage.md | 34 +++++- skills/diffwalk/SKILL.md | 15 ++- src/authoring/capture.ts | 97 +++++++++++++-- src/authoring/git.ts | 244 +++++++++++++++++++++++++------------ src/cli/commands/change.ts | 20 ++- src/cli/commands/file.ts | 15 ++- src/cli/output.ts | 26 ++-- src/format/schema.ts | 60 ++++++++- src/format/types.ts | 37 +++++- src/report/render.ts | 38 +++++- src/report/targets.ts | 7 +- test/authoring.test.ts | 156 +++++++++++++++++++++++- test/cli.test.ts | 232 +++++++++++++++++++++++++++++++++++ test/format.test.ts | 105 ++++++++++++++++ test/git.test.ts | 217 +++++++++++++++++++++++++++++++-- test/report.test.ts | 111 +++++++++++++++++ 18 files changed, 1300 insertions(+), 131 deletions(-) diff --git a/README.md b/README.md index 337e8d4..ff967b4 100644 --- a/README.md +++ b/README.md @@ -56,6 +56,8 @@ file contents and publication tokens: ## Documentation +## Documentation + - [Usage](https://github.com/minipai/diffwalk/blob/main/docs/usage.md): manual authoring, commands, YAML format, and review service settings. - [Agent skill](skills/diffwalk/SKILL.md): the workflow your agent follows. - [Development](https://github.com/minipai/diffwalk/blob/main/docs/development.md): local development, testing, and self-hosting. diff --git a/docs/architecture.md b/docs/architecture.md index b157f36..976f223 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -113,11 +113,14 @@ exact corresponding diffs in a deliberate order. - `src/format/types.ts`: independent TypeScript types used by internal logic. - `src/format/schema.ts`: boundary-only Zod schemas for the machine-owned capture and the author-edited explanations, plus the version 1 ExplainDocument and its optional attribution metadata. -- `src/authoring/git.ts`: captures staged, unstaged, deleted, renamed, and untracked UTF-8 - files from an immutable Git base commit, optionally reading the index or limiting the - capture to named paths. -- `src/authoring/capture.ts`: derives change blocks and the content `captureId`, and - materializes exact section patches from capture plus explanations. +- `src/authoring/git.ts`: captures staged, unstaged, deleted, renamed, and untracked text + and binary files from an immutable Git base commit, optionally reading the index or + limiting the capture to named paths. A text side keeps its UTF-8 content; a binary side + keeps only its byte size and SHA-256 content hash. +- `src/authoring/capture.ts`: derives text change blocks, file-level binary change blocks, + and the content `captureId`, and materializes exact section patches from capture plus + explanations. Text-only files hash into `captureId` exactly as before binary support, so + existing captures and their explanations stay matched. - `src/cli/explanations.ts`: strict safe YAML 1.2 parsing into the explanations schema. - `src/cli/commands/`: each command owns its option schema and validates inputs before calling internal logic. `cli.ts` registers commands and forwards their arguments. @@ -138,6 +141,8 @@ exact corresponding diffs in a deliberate order. - `src/report/index.ts`: atomic report writes and client-bundle loading. - `src/report/render.ts`: the one report shell, embedded-data escaping, and shell styles, rendered with inlined assets for the offline file or linked assets for the hosted page. + A binary change renders as a metadata card carrying its path, status, and before/after + sizes instead of a diff. - `src/cli/service.ts`: review service configuration and origin checks. - `src/publish/client.ts`: publish, update, and unpublish requests, and adding the Git user name as `metadata.publishedBy` without mutating the authoring files. diff --git a/docs/usage.md b/docs/usage.md index 9b6bdeb..a87eda6 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -14,8 +14,8 @@ diffwalk inspect diffwalk changes ``` -`inspect` captures staged, unstaged, and untracked UTF-8 file changes relative to -`HEAD`. It creates a walk under `.diffwalk/` containing: +`inspect` captures staged, unstaged, and untracked file changes relative to `HEAD`, +including binary assets. It creates a walk under `.diffwalk/` containing: - `capture.json` — generated capture data. Do not edit it. - `explanations.yaml` — the file you edit to explain and order the changes. @@ -122,11 +122,33 @@ diffwalk file src/a.ts --after # full captured new file `changes --json` includes change blocks, not full file snapshots. +Text changes list line coordinates and their before/after blocks. Binary changes keep +only an identity, never the bytes: `changes` shows the status and each existing side's +byte size, `changes --json` and `change ` add the content hash, and +`file --before/--after` prints that metadata instead of bytes. A file that +switches between text and binary keeps both side identities. Binary change IDs are +assigned to steps like any other change. + +## Binary assets + +Diffwalk represents binary files at file level. It records the path, change status, +file modes, and each existing side's byte size and SHA-256 content hash, and assigns +the file a change ID rendered as a metadata card instead of a textual patch. Because +the hash participates in `captureId`, two binary revisions of the same size are still +distinguished, and `check` re-validates the captured metadata before it passes. + +Diffwalk treats a file side as binary when its bytes contain a NUL byte or do not +decode as UTF-8, so a non-UTF-8 text file is captured as an opaque card rather than +shown as text. Diffwalk does not generate binary patches, preview images, or decode +binary contents, so a binary card shows identity rather than the changed bytes. +Symbolic links and non-file Git paths are still rejected at capture time. + ## Validation `diffwalk check` rejects stale capture IDs, malformed YAML, unknown change IDs, -unexplained changes, and blocks that cannot produce an exact patch. It reports -section, step, change, and file counts, including repeated changes. +unexplained changes, and blocks that cannot produce an exact patch or whose binary +metadata no longer matches the captured file. It reports section, step, change, and +file counts, including repeated changes. Explanations use YAML 1.2. Custom tags, duplicate keys, anchors, and aliases are not allowed; `yes` and `on` remain strings. @@ -143,6 +165,9 @@ HTML reviews are standalone files that work offline with JavaScript enabled. JSO export produces an ExplainDocument (format version 1) for integrations or archiving; its default filename is `diffwalk.json` in the current walk. +Both exports, and the hosted review, keep a binary change's card with its path, status, +and before/after sizes. + `text` and `summary` support Markdown and inline HTML. Use inline SVG or `data:` URIs for images; hosted reviews block remote image URLs. Authored HTML is not sanitized, so only preview, export, or publish explanations you or a trusted agent authored. @@ -228,4 +253,3 @@ diffwalk check --input path/to/capture.json --explanations path/to/explanations. `view`, `export`, and `publish` accept the same options. With explicit input files, `publish` saves `published.json` alongside the authoring pair. - diff --git a/skills/diffwalk/SKILL.md b/skills/diffwalk/SKILL.md index e33c904..d40cf1e 100644 --- a/skills/diffwalk/SKILL.md +++ b/skills/diffwalk/SKILL.md @@ -30,6 +30,9 @@ explained. - `diffwalk changes` for a concise summary, or `diffwalk changes --json` for structured IDs, paths, coordinates, before, and after. - `diffwalk change ` to read one captured change block. - `diffwalk file --before` / `diffwalk file --after` to read one captured file side. + - A binary change has no coordinates or bytes; `changes`, `change`, and `file` report its + path, status, and each existing side's byte size and hash instead. Assign its ID like + any other change. 4. Edit the generated explanations path printed by `inspect`. Diffwalk stores each authoring pair under `.diffwalk//` and records the selected walk in `.diffwalk/current`. When the user asks to switch or clean up walks, run @@ -163,8 +166,16 @@ hide ownership or order; otherwise let Diffwalk's exact diff carry the code. and never overwrites an authored `explanations.yaml`. - Executable modes are preserved for additions, deletions, renames, and content changes. A chmod-only change has no representable explanation block, so stop when - Diffwalk reports it. Also stop for binary files, symbolic links, or non-file Git - paths; do not bypass these boundaries. + Diffwalk reports it. Symbolic links and non-file Git paths are still rejected at + capture time; do not bypass that boundary. +- Binary files are represented at file level, not by their bytes. The capture records + the path, status, modes, and each side's byte size and SHA-256 hash; the review shows + a metadata card instead of a patch. A side counts as binary when its bytes contain a + NUL or do not decode as UTF-8, so a non-UTF-8 text file also becomes an opaque card. + Binary IDs must be assigned like any other change, and `check` re-validates the + captured metadata. Diffwalk does not generate binary patches or image previews, so + explain a binary change from its identity rather than expecting its contents in the + diff. Never reconstruct the missing bytes or paste them into `text`. - Treat a pure rename as a real assignable change. Diffwalk renders it as a move rather than an empty textual diff. - The capture contains full file contents. Treat it as potentially sensitive and do not publish or send it without the user's authorization. diff --git a/src/authoring/capture.ts b/src/authoring/capture.ts index 59b7f36..e1dae10 100644 --- a/src/authoring/capture.ts +++ b/src/authoring/capture.ts @@ -1,14 +1,17 @@ import { createHash } from 'node:crypto' import { diffLines, formatPatch, structuredPatch, type StructuredPatch } from 'diff' import type { + BinaryChangeBlock, CaptureSource, ChangeBlock, + ChangeSide, DraftFile, DocumentStep, ExplanationStep, ExplainCapture, ExplainDocument, Explanations, + TextChangeBlock, } from '../format/types' export function createExplainCapture(files: DraftFile[], source: CaptureSource): ExplainCapture { @@ -16,7 +19,13 @@ export function createExplainCapture(files: DraftFile[], source: CaptureSource): const changes: ChangeBlock[] = [] for (const file of [...files].sort((left, right) => left.path.localeCompare(right.path))) { + if (file.oldBinary !== undefined || file.newBinary !== undefined) { + changes.push(binaryChangeBlock(file, changeId(nextId++))) + continue + } + const fileChanges = changeBlocks(file).map((change) => ({ + kind: 'text' as const, id: changeId(nextId++), path: file.path, ...change, @@ -24,6 +33,7 @@ export function createExplainCapture(files: DraftFile[], source: CaptureSource): if (fileChanges.length === 0 && file.status !== 'modified') { fileChanges.push({ + kind: 'text', id: changeId(nextId++), path: file.path, oldStart: 1, @@ -46,6 +56,39 @@ export function createExplainCapture(files: DraftFile[], source: CaptureSource): } } +function binaryChangeBlock(file: DraftFile, id: string): BinaryChangeBlock { + const before = changeSide(file, 'old') + const after = changeSide(file, 'new') + return { + kind: 'binary', + id, + path: file.path, + status: file.status, + ...(file.oldPath === undefined ? {} : { oldPath: file.oldPath }), + oldMode: file.oldMode, + newMode: file.newMode, + ...(before === undefined ? {} : { before }), + ...(after === undefined ? {} : { after }), + } +} + +// Every existing side has an identity, whether its bytes decode as text or stay binary, so a +// text-to-binary or binary-to-text transition keeps both sides instead of dropping one. +function changeSide(file: DraftFile, side: 'old' | 'new'): ChangeSide | undefined { + const absent = side === 'old' ? file.status === 'added' : file.status === 'deleted' + if (absent) return undefined + + const binary = side === 'old' ? file.oldBinary : file.newBinary + if (binary !== undefined) return { kind: 'binary', size: binary.size, hash: binary.hash } + + const content = side === 'old' ? file.oldContent : file.newContent + return { + kind: 'text', + size: Buffer.byteLength(content, 'utf8'), + hash: createHash('sha256').update(content).digest('hex'), + } +} + export function captureIdFor(files: DraftFile[], includeModes = true): string { const hash = createHash('sha256') for (const file of [...files].sort((left, right) => left.path.localeCompare(right.path))) { @@ -65,6 +108,17 @@ export function captureIdFor(files: DraftFile[], includeModes = true): string { hash.update('\0') hash.update(file.newContent) hash.update('\0') + // Binary sides keep their bytes out of the capture, so their size and content hash must + // carry the identity instead. Text-only files hash exactly as they did before this field, + // keeping existing captures and their explanations matched. + if (file.oldBinary !== undefined) { + hash.update(`old-binary:${file.oldBinary.size}:${file.oldBinary.hash}`) + hash.update('\0') + } + if (file.newBinary !== undefined) { + hash.update(`new-binary:${file.newBinary.size}:${file.newBinary.hash}`) + hash.update('\0') + } } return hash.digest('hex') } @@ -128,8 +182,16 @@ function materializeStep( return change }) - const changesByPath = new Map() - for (const change of selected) { + const textChanges = selected.filter( + (change): change is TextChangeBlock => change.kind === 'text', + ) + const binaryChanges = selected.filter( + (change): change is BinaryChangeBlock => change.kind === 'binary', + ) + for (const change of binaryChanges) validateBinaryChange(change, filesByPath) + + const changesByPath = new Map() + for (const change of textChanges) { const fileChanges = changesByPath.get(change.path) ?? [] fileChanges.push(change) changesByPath.set(change.path, fileChanges) @@ -143,12 +205,33 @@ function materializeStep( return { text: step.text, - diff: patches.map(formatFilePatch).join('\n'), + ...(patches.length === 0 ? {} : { diff: patches.map(formatFilePatch).join('\n') }), + ...(binaryChanges.length === 0 ? {} : { binary: binaryChanges }), changes: step.changes, } } } +// A binary change carries no patch to re-derive, so materialization re-reads the file and +// confirms the captured sizes, hashes, status, and modes still describe it. +function validateBinaryChange(change: BinaryChangeBlock, filesByPath: Map): void { + const file = findFile(filesByPath, change.path) + if (file.oldBinary === undefined && file.newBinary === undefined) { + throw new Error(`Change block no longer matches captured file content: ${change.id}`) + } + const expected = binaryChangeBlock(file, change.id) + if ( + expected.status !== change.status || + expected.oldPath !== change.oldPath || + expected.oldMode !== change.oldMode || + expected.newMode !== change.newMode || + JSON.stringify(expected.before) !== JSON.stringify(change.before) || + JSON.stringify(expected.after) !== JSON.stringify(change.after) + ) { + throw new Error(`Change block no longer matches captured file content: ${change.id}`) + } +} + function findChange(changesById: Map, changeId: string): ChangeBlock { const change = changesById.get(changeId) if (!change) throw new Error(`Unknown change ID: ${changeId}`) @@ -180,9 +263,9 @@ function validateChangeCoverage(changes: ChangeBlock[], shown: Set): voi } } -function changeBlocks(file: DraftFile): Omit[] { +function changeBlocks(file: DraftFile): Omit[] { const parts = diffLines(file.oldContent, file.newContent) - const changes: Omit[] = [] + const changes: Omit[] = [] let oldIndex = 0 let newIndex = 0 @@ -224,7 +307,7 @@ function changeBlocks(file: DraftFile): Omit[] { return changes } -function createFilePatch(file: DraftFile, changes: ChangeBlock[]): StructuredPatch { +function createFilePatch(file: DraftFile, changes: TextChangeBlock[]): StructuredPatch { validateBlocks(file, changes) const nextLines = splitLines(file.oldContent) @@ -256,7 +339,7 @@ function formatFilePatch(patch: StructuredPatch): string { return formatted.replace('\nrename from ', '\nsimilarity index 100%\nrename from ') } -function validateBlocks(file: DraftFile, changes: ChangeBlock[]) { +function validateBlocks(file: DraftFile, changes: TextChangeBlock[]) { const oldLines = splitLines(file.oldContent) const newLines = splitLines(file.newContent) const sorted = [...changes].sort((left, right) => left.oldStart - right.oldStart) diff --git a/src/authoring/git.ts b/src/authoring/git.ts index 4087acb..9f98427 100644 --- a/src/authoring/git.ts +++ b/src/authoring/git.ts @@ -1,8 +1,16 @@ import { spawn } from 'node:child_process' +import { createHash } from 'node:crypto' import { lstat, mkdir, mkdtemp, rm } from 'node:fs/promises' import { tmpdir } from 'node:os' import { isAbsolute, join, relative, resolve } from 'node:path' -import type { DraftFile } from '../format/types' +import type { BinarySide, DraftFile } from '../format/types' + +// A captured file side is text when its bytes decode as UTF-8, and binary otherwise. +// Binary sides keep only an identity: byte size and a content hash, never the bytes. +interface CapturedSide { + content: string + binary?: BinarySide +} export interface GitCapture { root: string @@ -57,45 +65,51 @@ export async function captureGitChanges( for (const change of changes) { if (change.kind === 'R') { - files.push({ - path: change.path, - oldPath: change.oldPath, - status: 'renamed', - oldMode: change.oldMode, - newMode: change.newMode, - oldContent: await gitFile(baseCommit, change.oldPath!, root), - newContent: await newContentFor(change.path), - }) + files.push( + draftFile({ + path: change.path, + oldPath: change.oldPath, + status: 'renamed', + oldMode: change.oldMode, + newMode: change.newMode, + oldSide: await gitFile(baseCommit, change.oldPath!, root), + newSide: await newContentFor(change.path), + }), + ) } else if (change.kind === 'M') { - const oldContent = await gitFile(baseCommit, change.path, root) - const newContent = await newContentFor(change.path) - validateFileModeChange(change.path, change.oldMode, change.newMode, oldContent, newContent) - files.push({ - path: change.path, - status: 'modified', - oldMode: change.oldMode, - newMode: change.newMode, - oldContent, - newContent, - }) + const oldSide = await gitFile(baseCommit, change.path, root) + const newSide = await newContentFor(change.path) + validateFileModeChange(change.path, change.oldMode, change.newMode, oldSide, newSide) + files.push( + draftFile({ + path: change.path, + status: 'modified', + oldMode: change.oldMode, + newMode: change.newMode, + oldSide, + newSide, + }), + ) } else if (change.kind === 'A') { - files.push({ - path: change.path, - status: 'added', - oldMode: change.oldMode, - newMode: change.newMode, - oldContent: '', - newContent: await newContentFor(change.path), - }) + files.push( + draftFile({ + path: change.path, + status: 'added', + oldMode: change.oldMode, + newMode: change.newMode, + newSide: await newContentFor(change.path), + }), + ) } else if (change.kind === 'D') { - files.push({ - path: change.path, - status: 'deleted', - oldMode: change.oldMode, - newMode: change.newMode, - oldContent: await gitFile(baseCommit, change.path, root), - newContent: '', - }) + files.push( + draftFile({ + path: change.path, + status: 'deleted', + oldMode: change.oldMode, + newMode: change.newMode, + oldSide: await gitFile(baseCommit, change.path, root), + }), + ) } else { throw new Error(`Unsupported Git change status: ${change.kind}`) } @@ -114,32 +128,38 @@ export async function captureGitChanges( const existing = filesByPath.get(path) if (existing !== undefined) { if (existing.status !== 'deleted') continue - const newContent = await workingTreeFile(path, root) + const oldSide = storedSide(existing, 'old') + const newSide = await workingTreeFile(path, root) const newMode = await workingTreeMode(path, root) - validateFileModeChange(path, existing.oldMode, newMode, existing.oldContent, newContent) - if (existing.oldContent === newContent) { + validateFileModeChange(path, existing.oldMode, newMode, oldSide, newSide) + if (sidesMatch(oldSide, newSide)) { filesByPath.delete(path) } else { - filesByPath.set(path, { + filesByPath.set( path, - status: 'modified', - oldMode: existing.oldMode, - newMode, - oldContent: existing.oldContent, - newContent, - }) + draftFile({ + path, + status: 'modified', + oldMode: existing.oldMode, + newMode, + oldSide, + newSide, + }), + ) } continue } - filesByPath.set(path, { + filesByPath.set( path, - status: 'added', - oldMode: '000000', - newMode: await workingTreeMode(path, root), - oldContent: '', - newContent: await workingTreeFile(path, root), - }) + draftFile({ + path, + status: 'added', + oldMode: '000000', + newMode: await workingTreeMode(path, root), + newSide: await workingTreeFile(path, root), + }), + ) } } @@ -164,20 +184,29 @@ export async function captureGitRevisionChanges( const files: DraftFile[] = [] for (const change of changes) { - const oldContent = change.kind === 'A' ? '' : await gitFile(fromCommit, change.oldPath ?? change.path, root) - const newContent = change.kind === 'D' ? '' : await gitFile(toCommit, change.path, root) + const oldSide = change.kind === 'A' ? undefined : await gitFile(fromCommit, change.oldPath ?? change.path, root) + const newSide = change.kind === 'D' ? undefined : await gitFile(toCommit, change.path, root) if (change.kind === 'M') { - validateFileModeChange(change.path, change.oldMode, change.newMode, oldContent, newContent) + validateFileModeChange(change.path, change.oldMode, change.newMode, oldSide!, newSide!) } - files.push({ - path: change.path, - ...(change.oldPath === undefined ? {} : { oldPath: change.oldPath }), - status: change.kind === 'R' ? 'renamed' : change.kind === 'M' ? 'modified' : change.kind === 'A' ? 'added' : 'deleted', - oldMode: change.oldMode, - newMode: change.newMode, - oldContent, - newContent, - }) + files.push( + draftFile({ + path: change.path, + oldPath: change.oldPath, + status: + change.kind === 'R' + ? 'renamed' + : change.kind === 'M' + ? 'modified' + : change.kind === 'A' + ? 'added' + : 'deleted', + oldMode: change.oldMode, + newMode: change.newMode, + oldSide, + newSide, + }), + ) } return { @@ -206,7 +235,7 @@ export async function gitUserName(root = process.cwd()): Promise { +async function workingTreeFile(path: string, root: string): Promise { const absolutePath = await validateWorkingTreeFile(path, root) const scratch = await mkdtemp(join(tmpdir(), 'diffwalk-working-tree-')) try { @@ -216,7 +245,7 @@ async function workingTreeFile(path: string, root: string): Promise { const object = ( await gitText(['hash-object', '-w', `--path=${path}`, '--', absolutePath], root, environment) ).trim() - return decodeText(await gitBytes(['cat-file', 'blob', object], root, environment), path) + return decodeFile(await gitBytes(['cat-file', 'blob', object], root, environment)) } finally { await rm(scratch, { recursive: true, force: true }) } @@ -226,14 +255,54 @@ function validateFileModeChange( filePath: string, oldMode: DraftFile['oldMode'], newMode: DraftFile['newMode'], - oldContent: string, - newContent: string, + oldSide: CapturedSide, + newSide: CapturedSide, ): void { - if (oldMode !== newMode && oldContent === newContent) { + if (oldMode !== newMode && sidesMatch(oldSide, newSide)) { throw new Error(`File mode changes are not supported: ${filePath}`) } } +function draftFile(input: { + path: string + oldPath?: string + status: DraftFile['status'] + oldMode: DraftFile['oldMode'] + newMode: DraftFile['newMode'] + oldSide?: CapturedSide + newSide?: CapturedSide +}): DraftFile { + return { + path: input.path, + ...(input.oldPath === undefined ? {} : { oldPath: input.oldPath }), + status: input.status, + oldMode: input.oldMode, + newMode: input.newMode, + oldContent: input.oldSide?.content ?? '', + newContent: input.newSide?.content ?? '', + ...(input.oldSide?.binary === undefined ? {} : { oldBinary: input.oldSide.binary }), + ...(input.newSide?.binary === undefined ? {} : { newBinary: input.newSide.binary }), + } +} + +function storedSide(file: DraftFile, side: 'old' | 'new'): CapturedSide { + const binary = side === 'old' ? file.oldBinary : file.newBinary + const content = side === 'old' ? file.oldContent : file.newContent + return binary === undefined ? { content } : { content: '', binary } +} + +function sidesMatch(left: CapturedSide, right: CapturedSide): boolean { + if (left.binary !== undefined || right.binary !== undefined) { + return ( + left.binary !== undefined && + right.binary !== undefined && + left.binary.size === right.binary.size && + left.binary.hash === right.binary.hash + ) + } + return left.content === right.content +} + async function validateWorkingTreeFile(path: string, root: string): Promise { const absolutePath = resolve(root, path) const pathWithinRoot = relative(root, absolutePath) @@ -253,12 +322,12 @@ async function workingTreeMode(path: string, root: string): Promise { - return decodeText(await gitBytes(['show', `${commit}:${path}`], root), path) +async function gitFile(commit: string, path: string, root: string): Promise { + return decodeFile(await gitBytes(['show', `${commit}:${path}`], root)) } -async function indexFile(path: string, root: string): Promise { - return decodeText(await gitBytes(['show', `:${path}`], root), path) +async function indexFile(path: string, root: string): Promise { + return decodeFile(await gitBytes(['show', `:${path}`], root)) } async function gitText( @@ -304,7 +373,7 @@ async function gitBytes( } function decodeText(bytes: Uint8Array, label: string): string { - if (bytes.includes(0)) throw new Error(`Binary files are not supported: ${label}`) + if (bytes.includes(0)) throw new Error(`Unexpected binary output from ${label}`) try { return new TextDecoder('utf-8', { fatal: true }).decode(bytes) } catch { @@ -312,6 +381,29 @@ function decodeText(bytes: Uint8Array, label: string): string { } } +function decodeFile(bytes: Uint8Array): CapturedSide { + if (isBinary(bytes)) { + return { content: '', binary: { size: bytes.byteLength, hash: hashBytes(bytes) } } + } + return { content: new TextDecoder('utf-8', { fatal: true }).decode(bytes) } +} + +// A file is binary when it has a NUL byte or its bytes are not valid UTF-8, matching how the +// rest of Diffwalk decides that a side cannot be shown as text. +function isBinary(bytes: Uint8Array): boolean { + if (bytes.includes(0)) return true + try { + new TextDecoder('utf-8', { fatal: true }).decode(bytes) + return false + } catch { + return true + } +} + +function hashBytes(bytes: Uint8Array): string { + return createHash('sha256').update(bytes).digest('hex') +} + function splitNulls(bytes: Uint8Array): string[] { const value = new TextDecoder('utf-8', { fatal: true }).decode(bytes) return value === '' ? [] : value.slice(0, value.endsWith('\0') ? -1 : undefined).split('\0') diff --git a/src/cli/commands/change.ts b/src/cli/commands/change.ts index a7aa338..a2d11f4 100644 --- a/src/cli/commands/change.ts +++ b/src/cli/commands/change.ts @@ -1,7 +1,7 @@ import { z } from 'zod' -import type { ChangeBlock, ExplainCapture } from '../../format/types' +import type { BinaryChangeBlock, ChangeBlock, ExplainCapture, TextChangeBlock } from '../../format/types' import { captureInput, readCapture } from '../input' -import { coordinates } from '../output' +import { binarySideLine, coordinates } from '../output' const changeOptionsSchema = z.object({ input: z.string().optional(), @@ -25,6 +25,22 @@ function findChange(capture: ExplainCapture, changeId: string): ChangeBlock { } function printChangeDetails(change: ChangeBlock): void { + if (change.kind === 'binary') { + printBinaryChange(change) + return + } + printTextChange(change) +} + +function printBinaryChange(change: BinaryChangeBlock): void { + process.stdout.write(`${change.id} ${change.path} binary ${change.status} +modes: ${change.oldMode} → ${change.newMode} +${binarySideLine('before', change.before)} +${binarySideLine('after', change.after)} +`) +} + +function printTextChange(change: TextChangeBlock): void { // The template supplies one final newline for each block. const before = change.before.endsWith('\n') ? change.before.slice(0, -1) : change.before const after = change.after.endsWith('\n') ? change.after.slice(0, -1) : change.after diff --git a/src/cli/commands/file.ts b/src/cli/commands/file.ts index b96f62e..21c3879 100644 --- a/src/cli/commands/file.ts +++ b/src/cli/commands/file.ts @@ -1,5 +1,5 @@ import { z } from 'zod' -import type { DraftFile, ExplainCapture } from '../../format/types' +import type { BinarySide, DraftFile, ExplainCapture } from '../../format/types' import { captureInput, readCapture } from '../input' import { UsageError } from '../usage' @@ -14,6 +14,11 @@ export async function printFile(filePath: string, options: z.input step.text.trim() !== '' || step.diff !== undefined, { - message: 'a step needs text, a diff, or both', - }) - .refine((step) => step.changes === undefined || step.diff !== undefined, { - message: 'captured change IDs require a diff', + .refine((step) => step.text.trim() !== '' || step.diff !== undefined || step.binary !== undefined, { + message: 'a step needs text, a diff, a binary change, or a combination', }) + .refine( + (step) => step.changes === undefined || step.diff !== undefined || step.binary !== undefined, + { + message: 'captured change IDs require a diff', + }, + ) export const documentMetadataSchema = z .object({ @@ -171,3 +213,9 @@ function normalizeFileModes(value: unknown): unknown { newMode: file.newMode ?? (file.status === 'deleted' ? '000000' : '100644'), } } + +function normalizeChangeKind(value: unknown): unknown { + if (typeof value !== 'object' || value === null || Array.isArray(value)) return value + const change = value as Record + return 'kind' in change ? change : { ...change, kind: 'text' } +} diff --git a/src/format/types.ts b/src/format/types.ts index 10fd700..babfe8d 100644 --- a/src/format/types.ts +++ b/src/format/types.ts @@ -1,14 +1,30 @@ +export type GitMode = '000000' | '100644' | '100755' + +export interface BinarySide { + size: number + hash: string +} + export interface DraftFile { path: string oldPath?: string status: 'added' | 'modified' | 'deleted' | 'renamed' - oldMode: '000000' | '100644' | '100755' - newMode: '000000' | '100644' | '100755' + oldMode: GitMode + newMode: GitMode oldContent: string newContent: string + oldBinary?: BinarySide + newBinary?: BinarySide } -export interface ChangeBlock { +export interface ChangeSide { + kind: 'text' | 'binary' + size: number + hash: string +} + +export interface TextChangeBlock { + kind: 'text' id: string path: string oldStart: number @@ -19,6 +35,20 @@ export interface ChangeBlock { after: string } +export interface BinaryChangeBlock { + kind: 'binary' + id: string + path: string + status: 'added' | 'modified' | 'deleted' | 'renamed' + oldPath?: string + oldMode: GitMode + newMode: GitMode + before?: ChangeSide + after?: ChangeSide +} + +export type ChangeBlock = TextChangeBlock | BinaryChangeBlock + interface CommitEndpoint { revision: string commit: string @@ -51,6 +81,7 @@ export interface Explanations { export interface DocumentStep { text: string diff?: string + binary?: BinaryChangeBlock[] changes?: string[] } diff --git a/src/report/render.ts b/src/report/render.ts index 308ad1f..de262b6 100644 --- a/src/report/render.ts +++ b/src/report/render.ts @@ -1,4 +1,4 @@ -import type { ExplainDocument } from '../format/types' +import type { ExplainDocument, BinaryChangeBlock } from '../format/types' import { faviconDataUrl } from './favicon' import { renderMarkdown } from './markdown' import { fileDiffLabel, fileDiffStats, parseSectionPatch } from './patches' @@ -159,13 +159,13 @@ function renderSection( const actions = `
${renderPermalink(stepTarget.fragment, `Permalink to step ${stepIndex + 1} in ${section.title}`, 'LINK')}${changeTargets}
` - if (step.diff === undefined) { + if (step.diff === undefined && step.binary === undefined) { return `
${actions}${textMarkup}
` } - const files = parseStepDiff(step.diff, section.title) - fileCount += files.length - diffs.push({ section: index, step: stepIndex, diff: step.diff }) + const files = step.diff === undefined ? [] : parseStepDiff(step.diff, section.title) + fileCount += files.length + (step.binary?.length ?? 0) + if (step.diff !== undefined) diffs.push({ section: index, step: stepIndex, diff: step.diff }) const filesMarkup = files .map((file, fileIndex) => { @@ -184,8 +184,11 @@ function renderSection( }) .join('\n') + const binaryMarkup = (step.binary ?? []).map(renderBinaryCard).join('\n') + const stepFiles = [filesMarkup, binaryMarkup].filter((markup) => markup !== '').join('\n') + return `
${actions}${textMarkup} -
${filesMarkup}
+
${stepFiles}
` }) @@ -207,6 +210,25 @@ function parseStepDiff(diff: string, sectionTitle: string): FileDiffMetadata[] { } } +// A binary change has no patch to render, so its card carries the identity a reader needs: +// the path, the status, and each existing side's kind, byte size, and content hash. +function renderBinaryCard(change: BinaryChangeBlock): string { + const label = change.oldPath && change.oldPath !== change.path ? `${change.oldPath} → ${change.path}` : change.path + return `
+
${escapeHtml(label)} Binary · ${escapeHtml(change.status)}
+
+ ${renderBinarySide('Before', change.before)} + ${renderBinarySide('After', change.after)} +
+
` +} + +function renderBinarySide(label: string, side: BinaryChangeBlock['before']): string { + const detail = + side === undefined ? 'absent' : `${side.kind} · ${side.size} B · sha256 ${side.hash}` + return `
${label}
${escapeHtml(detail)}
` +} + function renderReviewMap( sections: { title: string; fragment: string }[], counts: { sections: number; files: number }, @@ -542,6 +564,10 @@ main { max-width: none; min-width: 0; margin: 0; padding: 22px 28px 72px; } .file > summary::before { content: "▸ "; } .file[open] > summary::before { content: "▾ "; } .file-stats { float: right; color: #6d7d72; } +.binary-sides { display: grid; gap: 4px; margin: 0; padding: 10px 12px; border-top: 1px solid #d2ddd4; } +.binary-side { display: grid; grid-template-columns: 58px minmax(0, 1fr); gap: 8px; font: 12px/1.5 ui-monospace, SFMono-Regular, Menlo, monospace; } +.binary-side dt { color: #7e8d82; font-weight: 600; } +.binary-side dd { min-width: 0; margin: 0; color: #314439; overflow-wrap: anywhere; } .file-diff { border-top: 1px solid #d2ddd4; } .file-diff:empty { border-top: none; } .diff-error { diff --git a/src/report/targets.ts b/src/report/targets.ts index 9a05182..16d65eb 100644 --- a/src/report/targets.ts +++ b/src/report/targets.ts @@ -107,7 +107,12 @@ function assignCanonicalChanges(targets: ReportSectionTarget[], changeIds: strin } function stepFingerprint(step: ExplainDocument['sections'][number]['steps'][number]): string { - return JSON.stringify({ text: step.text, diff: step.diff, changes: step.changes }) + return JSON.stringify({ + text: step.text, + diff: step.diff, + binary: step.binary, + changes: step.changes, + }) } interface FragmentSeed { diff --git a/test/authoring.test.ts b/test/authoring.test.ts index 2c350e1..1bd7da7 100644 --- a/test/authoring.test.ts +++ b/test/authoring.test.ts @@ -1,11 +1,12 @@ import { describe, expect, test } from 'bun:test' +import { createHash } from 'node:crypto' import { captureIdFor, createExplainCapture, duplicatedChangeIds, materializeExplainDocument, } from '../src/authoring/capture' -import type { CaptureSource, ExplainCapture } from '../src/format/types' +import type { CaptureSource, DraftFile, ExplainCapture } from '../src/format/types' import { fileDiffStats, parseSectionPatch } from '../src/report/patches' const source: CaptureSource = { @@ -486,3 +487,156 @@ describe('explain materialization', () => { ) }) }) + +describe('binary changes', () => { + function binaryFile(overrides: Partial = {}): DraftFile { + return { + path: 'logo.png', + status: 'modified', + oldMode: '100644', + newMode: '100644', + oldContent: '', + newContent: '', + oldBinary: { size: 3, hash: 'a'.repeat(64) }, + newBinary: { size: 5, hash: 'b'.repeat(64) }, + ...overrides, + } + } + + test('emits one binary change block that keeps both side identities', () => { + const capture = createExplainCapture([binaryFile()], source) + + expect(capture.changes).toEqual([ + { + kind: 'binary', + id: 'change-001', + path: 'logo.png', + status: 'modified', + oldMode: '100644', + newMode: '100644', + before: { kind: 'binary', size: 3, hash: 'a'.repeat(64) }, + after: { kind: 'binary', size: 5, hash: 'b'.repeat(64) }, + }, + ]) + }) + + test('keeps the identity of a text side and a binary side across a transition', () => { + const capture = createExplainCapture( + [ + { + path: 'logo.png', + status: 'modified', + oldMode: '100644', + newMode: '100644', + oldContent: 'old bytes\n', + newContent: '', + newBinary: { size: 5, hash: 'b'.repeat(64) }, + }, + ], + source, + ) + + expect(capture.changes[0]).toMatchObject({ + kind: 'binary', + before: { + kind: 'text', + size: Buffer.byteLength('old bytes\n', 'utf8'), + hash: createHash('sha256').update('old bytes\n').digest('hex'), + }, + after: { kind: 'binary', size: 5, hash: 'b'.repeat(64) }, + }) + }) + + test('includes binary content identity even when both sides keep the same size', () => { + const same = createExplainCapture([binaryFile()], source) + const differentHash = createExplainCapture( + [binaryFile({ newBinary: { size: 5, hash: 'c'.repeat(64) } })], + source, + ) + const differentSize = createExplainCapture( + [binaryFile({ newBinary: { size: 6, hash: 'b'.repeat(64) } })], + source, + ) + + expect(differentHash.captureId).not.toBe(same.captureId) + expect(differentSize.captureId).not.toBe(same.captureId) + expect(createExplainCapture([binaryFile()], source).captureId).toBe(same.captureId) + }) + + test('keeps the capture identity of a text-only capture stable for existing walks', () => { + expect( + captureIdFor([ + { + path: 'a.ts', + status: 'modified', + ...regularModes, + oldContent: 'old\n', + newContent: 'new\n', + }, + ]), + ).toBe('7b01637bb6902d7b475e38e88289653635981ad1e8c2484dc2b223856345edb5') + }) + + test('materializes a binary card instead of a textual patch', () => { + const capture = createExplainCapture([binaryFile()], source) + const document = materializeExplainDocument(capture, allChangesAssigned(capture)) + const step = document.sections[0]!.steps[0]! + + expect(step.diff).toBeUndefined() + expect(step.binary).toEqual([ + expect.objectContaining({ + kind: 'binary', + id: 'change-001', + path: 'logo.png', + status: 'modified', + before: { kind: 'binary', size: 3, hash: 'a'.repeat(64) }, + after: { kind: 'binary', size: 5, hash: 'b'.repeat(64) }, + }), + ]) + }) + + test('materializes a step that mixes textual and binary changes', () => { + const capture = createExplainCapture( + [ + { + path: 'example.ts', + status: 'modified', + ...regularModes, + oldContent: 'old\n', + newContent: 'new\n', + }, + binaryFile(), + ], + source, + ) + const document = materializeExplainDocument(capture, { + captureId: capture.captureId, + title: 'Mixed', + summary: '', + sections: [ + { + title: 'Mixed', + steps: [{ text: 'Both.', changes: ['change-001', 'change-002'] }], + }, + ], + }) + const step = document.sections[0]!.steps[0]! + + expect(step.diff).toContain('diff --git') + expect(step.binary).toHaveLength(1) + expect(step.binary![0]!.path).toBe('logo.png') + }) + + test('rejects a binary change whose captured metadata no longer matches its file', () => { + const capture = createExplainCapture([binaryFile()], source) + const explanations = allChangesAssigned(capture) + const tampered: ExplainCapture = structuredClone(capture) + const change = tampered.changes[0]! + if (change.kind !== 'binary') throw new Error('expected a binary change') + change.after = { kind: 'binary', size: 99, hash: 'c'.repeat(64) } + + expect(() => materializeExplainDocument(tampered, explanations)).toThrow( + 'no longer matches captured file content', + ) + }) +}) diff --git a/test/cli.test.ts b/test/cli.test.ts index 32b0b52..c04b868 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -1,4 +1,5 @@ import { afterEach, describe, expect, test } from 'bun:test' +import { createHash } from 'node:crypto' import { existsSync } from 'node:fs' import { chmod, mkdir, mkdtemp, readFile, readdir, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' @@ -854,6 +855,7 @@ describe('change', () => { expect(result.stdout).toContain(change.path) expect(result.stdout).toContain('before:') expect(result.stdout).toContain('after:') + if (change.kind !== 'text') throw new Error('expected a text change') expect(result.stdout).toContain(change.before) expect(result.stdout).toContain(change.after) }) @@ -1814,6 +1816,225 @@ describe('removed workflow', () => { }) }) +describe('binary captures', () => { + test('inspects a mixed working-tree change with text and binary files', async () => { + const repo = await mixedFixtureRepo() + + const result = await runCli(['inspect'], repo) + + expect(result.exitCode).toBe(0) + const capture = await readCapture(repo) + expect(capture.files.map((file) => file.path)).toEqual(['assets/logo.png', 'greeting.ts']) + const binary = capture.changes.find((change) => change.kind === 'binary')! + expect(binary).toMatchObject({ + kind: 'binary', + path: 'assets/logo.png', + status: 'modified', + before: { kind: 'binary', size: 4, hash: sha256([0, 1, 2, 3]) }, + after: { kind: 'binary', size: 5, hash: sha256([0, 1, 2, 3, 4]) }, + }) + }) + + test('inspects a staged binary addition', async () => { + const repo = await mixedFixtureRepo() + await writeBinary(join(repo, 'assets', 'added.bin'), [0, 7, 7]) + await git(['add', 'assets/added.bin'], repo) + + const result = await runCli(['inspect', '--staged'], repo) + + expect(result.exitCode).toBe(0) + const capture = await readCapture(repo) + expect(capture.files).toEqual([ + expect.objectContaining({ + path: 'assets/added.bin', + status: 'added', + newBinary: { size: 3, hash: sha256([0, 7, 7]) }, + }), + ]) + }) + + test('inspects a committed range with a binary addition and deletion', async () => { + const repo = await binaryLifecycleRepo() + const commits = await gitText(['log', '--format=%H'], repo) + const [newer, older] = commits.trim().split('\n') + + const result = await runCli(['inspect', '--from', older!, '--to', newer!], repo) + + expect(result.exitCode).toBe(0) + const capture = await readCapture(repo) + expect(capture.files).toEqual([ + expect.objectContaining({ + path: 'fresh.bin', + status: 'added', + newBinary: { size: 2, hash: sha256([0, 4]) }, + }), + expect.objectContaining({ + path: 'gone.bin', + status: 'deleted', + oldBinary: { size: 3, hash: sha256([0, 3, 3]) }, + }), + ]) + }) + + test('exposes binary metadata through changes, change, and file without raw bytes', async () => { + const repo = await mixedFixtureRepo() + await runCli(['inspect'], repo) + const capture = await readCapture(repo) + const binary = capture.changes.find((change) => change.kind === 'binary')! + const binaryFile = capture.files.find((file) => file.path === 'assets/logo.png')! + + const listed = await runCli(['changes'], repo) + expect(listed.exitCode).toBe(0) + expect(listed.stdout).toContain('assets/logo.png') + expect(listed.stdout).toContain('binary modified before 4 B → after 5 B') + + const json = await runCli(['changes', '--json'], repo) + expect(json.exitCode).toBe(0) + const data = JSON.parse(json.stdout) as { + changes: { + id: string + kind: string + path: string + status: string + oldMode: string + newMode: string + before?: { kind: string; size: number; hash: string } + after?: { kind: string; size: number; hash: string } + }[] + } + const jsonBinary = data.changes.find((change) => change.kind === 'binary')! + expect(jsonBinary).toEqual({ + kind: 'binary', + id: binary.id, + path: 'assets/logo.png', + status: 'modified', + oldMode: '100644', + newMode: '100644', + before: { kind: 'binary', size: 4, hash: sha256([0, 1, 2, 3]) }, + after: { kind: 'binary', size: 5, hash: sha256([0, 1, 2, 3, 4]) }, + }) + expect(Object.keys(jsonBinary.before as object).sort()).toEqual(['hash', 'kind', 'size']) + + const described = await runCli(['change', binary.id], repo) + expect(described.exitCode).toBe(0) + expect(described.stdout).toContain('assets/logo.png binary modified') + expect(described.stdout).toContain('modes: 100644 → 100644') + expect(described.stdout).toContain(`before: binary · 4 B · sha256 ${sha256([0, 1, 2, 3])}`) + expect(described.stdout).toContain(`after: binary · 5 B · sha256 ${sha256([0, 1, 2, 3, 4])}`) + + const after = await runCli(['file', 'assets/logo.png', '--after'], repo) + expect(after.exitCode).toBe(0) + expect(after.stdout).toContain('assets/logo.png binary modified after') + expect(after.stdout).toContain('size: 5 B') + expect(after.stdout).toContain(`sha256: ${binaryFile.newBinary!.hash}`) + + // The fixture's binary sides contain a NUL byte, so any raw-byte leak would surface here. + for (const output of [listed.stdout, json.stdout, described.stdout, after.stdout]) { + expect(output).not.toContain('\u0000') + expect(output).not.toContain('\uFFFD') + } + + const text = await runCli(['file', 'greeting.ts', '--after'], repo) + expect(text.exitCode).toBe(0) + expect(text.stdout).toBe('Hello\nUniverse\n') + }) + + test('check covers binary changes and rejects tampered captured metadata', async () => { + const repo = await mixedFixtureRepo() + await runCli(['inspect'], repo) + await authorEveryChange(repo) + + const checked = await runCli(['check'], repo) + expect(checked.exitCode).toBe(0) + expect(checked.stdout).toContain('cover') + + const capture = await readCapture(repo) + const binary = capture.changes.find((change) => change.kind === 'binary')! + const textChanges = capture.changes.filter((change) => change.kind === 'text') + await writeExplanations(repo, everyChangeYaml(capture.captureId, textChanges)) + const uncovered = await runCli(['check'], repo) + expect(uncovered.exitCode).not.toBe(0) + expect(uncovered.stderr).toContain(`Unassigned change IDs: ${binary.id}`) + + await authorEveryChange(repo) + const capturePath = join(await currentWalkDir(repo), 'capture.json') + const persisted = JSON.parse(await readFile(capturePath, 'utf8')) as { + changes: { kind: string; after?: { kind: string; size: number; hash: string } }[] + } + const tampered = persisted.changes.find((change) => change.kind === 'binary')! + tampered.after = { ...tampered.after!, size: 99 } + await writeFile(capturePath, `${JSON.stringify(persisted, null, 2)}\n`) + + const mismatch = await runCli(['check'], repo) + expect(mismatch.exitCode).not.toBe(0) + expect(mismatch.stderr).toContain('no longer matches captured file content') + }) + + test('exports HTML and JSON that retain the binary change card', async () => { + const repo = await mixedFixtureRepo() + await runCli(['inspect'], repo) + await authorEveryChange(repo) + const htmlPath = join(repo, 'out', 'report.html') + const jsonPath = join(repo, 'out', 'document.json') + + const html = await runCli(['export', 'html', '--output', htmlPath], repo) + expect(html.exitCode).toBe(0) + const markup = await readFile(htmlPath, 'utf8') + expect(markup).toContain('assets/logo.png') + expect(markup).toContain('Binary · modified') + expect(markup).toContain('4 B') + expect(markup).toContain('5 B') + + const json = await runCli(['export', 'json', '--output', jsonPath], repo) + expect(json.exitCode).toBe(0) + const document = JSON.parse(await readFile(jsonPath, 'utf8')) as { + sections: { steps: { binary?: { path: string; before?: { size: number }; after?: { size: number } }[] }[] }[] + } + const card = document.sections.flatMap((section) => section.steps).find((step) => step.binary !== undefined)! + expect(card.binary![0]).toMatchObject({ + path: 'assets/logo.png', + before: { kind: 'binary', size: 4 }, + after: { kind: 'binary', size: 5 }, + }) + }) +}) + +async function mixedFixtureRepo(): Promise { + const repo = await mkdtemp(join(tmpdir(), 'diffwalk-cli-')) + directories.push(repo) + await initializeRepository(repo) + await writeFile(join(repo, 'greeting.ts'), 'Hello\nWorld\n') + await mkdir(join(repo, 'assets'), { recursive: true }) + await writeBinary(join(repo, 'assets', 'logo.png'), [0, 1, 2, 3]) + await git(['add', '.'], repo) + await git(['commit', '-q', '-m', 'fixture'], repo) + await writeFile(join(repo, 'greeting.ts'), 'Hello\nUniverse\n') + await writeBinary(join(repo, 'assets', 'logo.png'), [0, 1, 2, 3, 4]) + return repo +} + +async function binaryLifecycleRepo(): Promise { + const repo = await mkdtemp(join(tmpdir(), 'diffwalk-cli-')) + directories.push(repo) + await initializeRepository(repo) + await writeBinary(join(repo, 'gone.bin'), [0, 3, 3]) + await git(['add', '.'], repo) + await git(['commit', '-q', '-m', 'one'], repo) + await rm(join(repo, 'gone.bin')) + await writeBinary(join(repo, 'fresh.bin'), [0, 4]) + await git(['add', '-A'], repo) + await git(['commit', '-q', '-m', 'two'], repo) + return repo +} + +async function writeBinary(path: string, bytes: number[]) { + await writeFile(path, new Uint8Array(bytes)) +} + +function sha256(bytes: number[]): string { + return createHash('sha256').update(new Uint8Array(bytes)).digest('hex') +} + async function initializeRepository(directory: string) { await git(['init', '-q'], directory) await git(['config', 'user.name', 'Test'], directory) @@ -1825,3 +2046,14 @@ async function git(args: string[], cwd: string) { const [exitCode, stderr] = await Promise.all([child.exited, new Response(child.stderr).text()]) if (exitCode !== 0) throw new Error(stderr) } + +async function gitText(args: string[], cwd: string): Promise { + const child = Bun.spawn(['git', ...args], { cwd, stdout: 'pipe', stderr: 'pipe' }) + const [stdout, stderr, exitCode] = await Promise.all([ + new Response(child.stdout).text(), + new Response(child.stderr).text(), + child.exited, + ]) + if (exitCode !== 0) throw new Error(stderr) + return stdout +} diff --git a/test/format.test.ts b/test/format.test.ts index d961c73..54dce25 100644 --- a/test/format.test.ts +++ b/test/format.test.ts @@ -464,3 +464,108 @@ describe('explanations schema', () => { } }) }) + +describe('binary capture and document cards', () => { + const binaryCapture = { + captureId: 'c'.repeat(64), + source: workingTreeSource, + files: [ + { + path: 'assets/logo.png', + status: 'modified', + oldMode: '100644', + newMode: '100644', + oldContent: '', + newContent: '', + oldBinary: { size: 3, hash: 'a'.repeat(64) }, + newBinary: { size: 5, hash: 'b'.repeat(64) }, + }, + ], + changes: [ + { + kind: 'binary', + id: 'change-001', + path: 'assets/logo.png', + status: 'modified', + oldMode: '100644', + newMode: '100644', + before: { kind: 'binary', size: 3, hash: 'a'.repeat(64) }, + after: { kind: 'binary', size: 5, hash: 'b'.repeat(64) }, + }, + ], + } as const + + test('reads captures written before binary support as textual changes', () => { + const parsed = captureSchema.parse(JSON.parse(JSON.stringify(capture))) + + expect(parsed.changes[0]).toMatchObject({ kind: 'text', before: 'old\n', after: 'new\n' }) + }) + + test('accepts file-level binary sides and a binary change with both identities', () => { + const parsed = captureSchema.parse(JSON.parse(JSON.stringify(binaryCapture))) + + expect(parsed.files[0]).toMatchObject({ + oldBinary: { size: 3, hash: 'a'.repeat(64) }, + newBinary: { size: 5, hash: 'b'.repeat(64) }, + }) + expect(parsed.changes[0]).toMatchObject({ + kind: 'binary', + before: { kind: 'binary', size: 3, hash: 'a'.repeat(64) }, + after: { kind: 'binary', size: 5, hash: 'b'.repeat(64) }, + }) + }) + + test('accepts a document step whose only captured change is binary', () => { + const document = explainDocumentSchema.parse({ + formatVersion: 1, + title: 'Binary card', + source: proposalSource, + sections: [{ title: 'Assets', steps: [{ text: 'A new logo.', binary: [...binaryCapture.changes] }] }], + }) + + expect(document.sections[0]!.steps[0]!.diff).toBeUndefined() + expect(document.sections[0]!.steps[0]!.binary).toEqual([...binaryCapture.changes]) + expect(explainDocumentSchema.parse(JSON.parse(JSON.stringify(document)))).toEqual(document) + }) + + test('requires a diff or binary card to carry captured change IDs', () => { + expect(() => + explainDocumentSchema.parse({ + formatVersion: 1, + title: 'Missing card', + source: proposalSource, + sections: [{ title: 'Bad', steps: [{ text: 'Only text.', changes: ['change-001'] }] }], + }), + ).toThrow('captured change IDs require a diff') + }) + + test('rejects malformed binary metadata and unknown card fields', () => { + const cases: unknown[] = [ + { size: -1, hash: 'a'.repeat(64) }, + { size: 3, hash: '' }, + { size: 3.5, hash: 'a'.repeat(64) }, + { size: 3, hash: 'a'.repeat(64), extra: 'x' }, + ] + for (const oldBinary of cases) { + expect(() => + captureSchema.parse({ + ...binaryCapture, + files: [{ ...binaryCapture.files[0], oldBinary }], + }), + ).toThrow() + } + expect(() => + explainDocumentSchema.parse({ + formatVersion: 1, + title: 'Bad card', + source: proposalSource, + sections: [ + { + title: 'Bad', + steps: [{ text: 'x', binary: [{ ...binaryCapture.changes[0], kind: 'text' }] }], + }, + ], + }), + ).toThrow() + }) +}) diff --git a/test/git.test.ts b/test/git.test.ts index 6b00620..6b0562c 100644 --- a/test/git.test.ts +++ b/test/git.test.ts @@ -1,4 +1,5 @@ import { afterEach, describe, expect, test } from 'bun:test' +import { createHash } from 'node:crypto' import { chmod, mkdir, mkdtemp, rename, rm, symlink, unlink, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' @@ -151,7 +152,7 @@ describe('captureGitChanges', () => { ]) }) - test('rejects a binary replacement after a staged deletion', async () => { + test('captures a binary replacement after a staged deletion with both side identities', async () => { const directory = await mkdtemp(join(tmpdir(), 'diffwalk-git-')) directories.push(directory) await initializeRepository(directory) @@ -162,9 +163,19 @@ describe('captureGitChanges', () => { await git(['rm', '--cached', '-q', 'replacement.dat'], directory) await writeFile(join(directory, 'replacement.dat'), new Uint8Array([0, 1, 2])) - await expect(captureGitChanges('HEAD', directory)).rejects.toThrow( - 'Binary files are not supported: replacement.dat', - ) + const capture = await captureGitChanges('HEAD', directory) + + expect(capture.files).toEqual([ + { + path: 'replacement.dat', + status: 'modified', + oldMode: '100644', + newMode: '100644', + oldContent: 'text\n', + newContent: '', + newBinary: { size: 3, hash: sha256(new Uint8Array([0, 1, 2])) }, + }, + ]) }) test('rejects a symbolic-link replacement after a staged deletion', async () => { @@ -417,7 +428,7 @@ describe('captureGitChanges', () => { ]) }) - test('rejects a binary edit in a CRLF checkout', async () => { + test('captures a binary edit in a CRLF checkout as a text-to-binary change', async () => { const directory = await mkdtemp(join(tmpdir(), 'diffwalk-git-')) directories.push(directory) await initializeRepository(directory) @@ -428,9 +439,19 @@ describe('captureGitChanges', () => { await writeFile(join(directory, 'data.dat'), new Uint8Array([0, 1, 2])) - await expect(captureGitChanges('HEAD', directory)).rejects.toThrow( - 'Binary files are not supported: data.dat', - ) + const capture = await captureGitChanges('HEAD', directory) + + expect(capture.files).toEqual([ + { + path: 'data.dat', + status: 'modified', + oldMode: '100644', + newMode: '100644', + oldContent: 'text\n', + newContent: '', + newBinary: { size: 3, hash: sha256(new Uint8Array([0, 1, 2])) }, + }, + ]) }) test('captures committed revisions without reading the working tree', async () => { @@ -712,6 +733,178 @@ describe('captureGitChanges selection', () => { }) }) +describe('binary capture', () => { + test('captures binary additions, modifications, deletions, and renames in the working tree', async () => { + const directory = await mkdtemp(join(tmpdir(), 'diffwalk-git-')) + directories.push(directory) + await initializeRepository(directory) + await writeBinary(directory, 'old.bin', [0, 1, 2, 3]) + await writeBinary(directory, 'delete.bin', [0, 9]) + await writeBinary(directory, 'rename.bin', [0, 5, 5]) + await git(['add', '.'], directory) + await git(['commit', '-q', '-m', 'fixture'], directory) + + await writeBinary(directory, 'old.bin', [0, 1, 2, 3, 4]) + await unlink(join(directory, 'delete.bin')) + await unlink(join(directory, 'rename.bin')) + await writeBinary(directory, 'moved.bin', [0, 5, 5]) + await writeBinary(directory, 'new.bin', [0, 7, 7, 7]) + await git(['add', '-A'], directory) + + const capture = await captureGitChanges('HEAD', directory) + + expect(capture.files).toEqual([ + { + path: 'delete.bin', + status: 'deleted', + oldMode: '100644', + newMode: '000000', + oldContent: '', + newContent: '', + oldBinary: { size: 2, hash: sha256(new Uint8Array([0, 9])) }, + }, + { + path: 'moved.bin', + oldPath: 'rename.bin', + status: 'renamed', + oldMode: '100644', + newMode: '100644', + oldContent: '', + newContent: '', + oldBinary: { size: 3, hash: sha256(new Uint8Array([0, 5, 5])) }, + newBinary: { size: 3, hash: sha256(new Uint8Array([0, 5, 5])) }, + }, + { + path: 'new.bin', + status: 'added', + oldMode: '000000', + newMode: '100644', + oldContent: '', + newContent: '', + newBinary: { size: 4, hash: sha256(new Uint8Array([0, 7, 7, 7])) }, + }, + { + path: 'old.bin', + status: 'modified', + oldMode: '100644', + newMode: '100644', + oldContent: '', + newContent: '', + oldBinary: { size: 4, hash: sha256(new Uint8Array([0, 1, 2, 3])) }, + newBinary: { size: 5, hash: sha256(new Uint8Array([0, 1, 2, 3, 4])) }, + }, + ]) + }) + + test('captures a binary-to-text transition with the identity of both sides', async () => { + const directory = await mkdtemp(join(tmpdir(), 'diffwalk-git-')) + directories.push(directory) + await initializeRepository(directory) + await writeBinary(directory, 'transition.dat', [0, 1, 2]) + await git(['add', '.'], directory) + await git(['commit', '-q', '-m', 'fixture'], directory) + + await writeFile(join(directory, 'transition.dat'), 'plain text\n') + + const capture = await captureGitChanges('HEAD', directory) + + expect(capture.files).toEqual([ + { + path: 'transition.dat', + status: 'modified', + oldMode: '100644', + newMode: '100644', + oldContent: '', + newContent: 'plain text\n', + oldBinary: { size: 3, hash: sha256(new Uint8Array([0, 1, 2])) }, + }, + ]) + }) + + test('captures binary additions and deletions between committed revisions', async () => { + const directory = await mkdtemp(join(tmpdir(), 'diffwalk-git-')) + directories.push(directory) + await initializeRepository(directory) + await writeBinary(directory, 'gone.bin', [0, 3, 3]) + await git(['add', '.'], directory) + await git(['commit', '-q', '-m', 'one'], directory) + const first = (await gitText(['rev-parse', 'HEAD'], directory)).trim() + + await unlink(join(directory, 'gone.bin')) + await writeBinary(directory, 'fresh.bin', [0, 4]) + await git(['add', '-A'], directory) + await git(['commit', '-q', '-m', 'two'], directory) + + const capture = await captureGitRevisionChanges(first, 'HEAD', directory) + + expect(capture.files).toEqual([ + { + path: 'fresh.bin', + status: 'added', + oldMode: '000000', + newMode: '100644', + oldContent: '', + newContent: '', + newBinary: { size: 2, hash: sha256(new Uint8Array([0, 4])) }, + }, + { + path: 'gone.bin', + status: 'deleted', + oldMode: '100644', + newMode: '000000', + oldContent: '', + newContent: '', + oldBinary: { size: 3, hash: sha256(new Uint8Array([0, 3, 3])) }, + }, + ]) + }) + + test('captures a binary rename between committed revisions', async () => { + const directory = await mkdtemp(join(tmpdir(), 'diffwalk-git-')) + directories.push(directory) + await initializeRepository(directory) + await writeBinary(directory, 'before.bin', [0, 5, 5]) + await git(['add', '.'], directory) + await git(['commit', '-q', '-m', 'one'], directory) + const first = (await gitText(['rev-parse', 'HEAD'], directory)).trim() + + await rename(join(directory, 'before.bin'), join(directory, 'after.bin')) + await git(['add', '-A'], directory) + await git(['commit', '-q', '-m', 'two'], directory) + + const capture = await captureGitRevisionChanges(first, 'HEAD', directory) + + expect(capture.files).toEqual([ + { + path: 'after.bin', + oldPath: 'before.bin', + status: 'renamed', + oldMode: '100644', + newMode: '100644', + oldContent: '', + newContent: '', + oldBinary: { size: 3, hash: sha256(new Uint8Array([0, 5, 5])) }, + newBinary: { size: 3, hash: sha256(new Uint8Array([0, 5, 5])) }, + }, + ]) + }) + + test('rejects a mode-only change to a binary file', async () => { + const directory = await mkdtemp(join(tmpdir(), 'diffwalk-git-')) + directories.push(directory) + await initializeRepository(directory) + await writeBinary(directory, 'script.bin', [0, 1]) + await git(['add', '.'], directory) + await git(['commit', '-q', '-m', 'fixture'], directory) + + await chmod(join(directory, 'script.bin'), 0o755) + + await expect(captureGitChanges('HEAD', directory)).rejects.toThrow( + 'File mode changes are not supported: script.bin', + ) + }) +}) + describe('gitUserName', () => { test('reads the configured Git user name', async () => { const directory = await mkdtemp(join(tmpdir(), 'diffwalk-git-')) @@ -760,6 +953,14 @@ async function initializeRepository(directory: string) { await git(['config', 'user.email', 'test@example.com'], directory) } +function sha256(bytes: Uint8Array): string { + return createHash('sha256').update(bytes).digest('hex') +} + +async function writeBinary(directory: string, name: string, bytes: number[]) { + await writeFile(join(directory, name), new Uint8Array(bytes)) +} + async function git(args: string[], cwd: string) { const child = Bun.spawn(['git', ...args], { cwd, stdout: 'ignore', stderr: 'pipe' }) const [exitCode, stderr] = await Promise.all([child.exited, new Response(child.stderr).text()]) diff --git a/test/report.test.ts b/test/report.test.ts index f90732c..d63bf07 100644 --- a/test/report.test.ts +++ b/test/report.test.ts @@ -790,3 +790,114 @@ describe('attribution metadata', () => { expect(html).not.toContain('onmouseover="alert(1)"') }) }) + +const binaryChange = { + kind: 'binary', + id: 'change-001', + path: 'assets/logo.png', + status: 'modified', + oldMode: '100644', + newMode: '100644', + before: { kind: 'binary', size: 3, hash: 'a'.repeat(64) }, + after: { kind: 'binary', size: 5, hash: 'b'.repeat(64) }, +} as const + +describe('binary change cards', () => { + test('renders a metadata card with the path, status, and both side sizes', () => { + const html = renderReport( + document([{ title: 'Assets', steps: [{ text: 'A new logo.', binary: [{ ...binaryChange }] }] }]), + stubClient, + ) + + expect(html).toContain('assets/logo.png') + expect(html).toContain('Binary · modified') + expect(html).toContain('3 B') + expect(html).toContain('5 B') + expect(html).toContain(binaryChange.before.hash) + expect(html).toContain(binaryChange.after.hash) + expect(html).not.toContain('data-diff-mount') + expect(html).not.toContain('
{ + const html = renderHostedReport( + document([{ title: 'Assets', steps: [{ text: 'A new logo.', binary: [{ ...binaryChange }] }] }]), + { stylesHref: '/report.css', clientSrc: '/report-client.js' }, + ) + + expect(html).toContain('assets/logo.png') + expect(html).toContain('Binary · modified') + expect(html).toContain('3 B') + expect(html).toContain('5 B') + }) + + test('renders a textual diff and a binary card inside one step', () => { + const html = renderReport( + document([ + { + title: 'Mixed', + steps: [ + { + text: 'Code and asset.', + diff: simplePatch(), + binary: [{ ...binaryChange }], + }, + ], + }, + ]), + stubClient, + ) + + expect(html).toContain('data-diff-mount="0-0-0"') + expect(html).toContain('assets/logo.png') + expect(html).toContain('Binary · modified') + }) + + test('counts binary cards as files in the review map', () => { + const html = renderReport( + document([ + { + title: 'Assets', + steps: [ + { + text: 'Two assets.', + binary: [ + { ...binaryChange }, + { ...binaryChange, id: 'change-002', path: 'assets/icon.png' }, + ], + }, + ], + }, + ]), + stubClient, + ) + + expect(html).toContain('1 section') + expect(html).toContain('2 files') + }) + + test('escapes binary paths and hashes', () => { + const html = renderReport( + document([ + { + title: 'Assets', + steps: [ + { + text: '', + binary: [ + { + ...binaryChange, + path: 'assets/.png', + }, + ], + }, + ], + }, + ]), + stubClient, + ) + + expect(html).not.toContain('') + expect(html).toContain('assets/<img src=x>.png') + }) +}) From e0deb934df7e0311aa8c8abbc5cab5d2c6f49bd0 Mon Sep 17 00:00:00 2001 From: Art Pai Date: Tue, 15 Sep 2026 23:23:11 +1000 Subject: [PATCH 2/2] Show binary change cards in the report preview sample MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dev:report sample only exercised textual diffs, so the new binary card had no public example to inspect. Add an explained section covering an addition, a same-size modification, a deletion, a rename, and both text/binary transitions. Co-Authored-By: ことね --- fixtures/report-preview.json | 136 +++++++++++++++++++++++++++++++++++ 1 file changed, 136 insertions(+) diff --git a/fixtures/report-preview.json b/fixtures/report-preview.json index 31cb8a4..f21a27b 100644 --- a/fixtures/report-preview.json +++ b/fixtures/report-preview.json @@ -49,6 +49,142 @@ } ] }, + { + "title": "Explain binary assets with metadata cards", + "steps": [ + { + "text": "### Binary changes carry identity, not text\n\nA binary asset has no lines to diff, so Diffwalk records each existing side's kind, byte size, and SHA-256 hash instead of decoding the bytes, then draws a metadata card in place of a patch. The card names the path, the status, and the before/after identity, so a reviewer can still separate an addition from a modification and can confirm that an edit changed the bytes even when it kept the byte size.\n\nNo raw bytes reach the page. The steps below show an addition, a same-size modification, a deletion, a rename, and both directions of a text/binary transition." + }, + { + "text": "The added asset ships beside an ordinary code change, so one step shows both card types at once. The text file still renders its patch; the PNG gets a card. Both `changes` entries belong to this step, and the review map counts one step with two files.", + "diff": "diff --git a/src/report/binary-card.ts b/src/report/binary-card.ts\n--- a/src/report/binary-card.ts\n+++ b/src/report/binary-card.ts\n@@ -1,3 +1,4 @@\n export function cardKind(): string {\n- return \"patch\"\n+ return \"metadata\"\n }\n+export const cardDelayMs = 0\n", + "binary": [ + { + "kind": "binary", + "id": "change-051", + "path": "assets/expressions-v2/afraid.box.png", + "status": "added", + "oldMode": "000000", + "newMode": "100644", + "after": { + "kind": "binary", + "size": 48211, + "hash": "4bf34f69726bfcda81145cd78a11d71581a58c69f7aeddcd19dec2497b007baa" + } + } + ], + "changes": [ + "change-050", + "change-051" + ] + }, + { + "text": "An edit can keep the byte size and still change the asset. Both sides report **51,204 B**, but the before and after hashes differ, which is why the content hash participates in capture identity. A deletion is the other extreme: only the before side exists, so the after row reads `absent`.", + "binary": [ + { + "kind": "binary", + "id": "change-052", + "path": "assets/expressions-v2/curious.box.png", + "status": "modified", + "oldMode": "100644", + "newMode": "100644", + "before": { + "kind": "binary", + "size": 51204, + "hash": "d30d18a489265bf6234144b7f0b6a613392307c3da8ebe3fb80aa565dded9c36" + }, + "after": { + "kind": "binary", + "size": 51204, + "hash": "3bc9aee6796dfde881c424f1e0ec7945fd107843416429d055dda2a3904a96c8" + } + }, + { + "kind": "binary", + "id": "change-053", + "path": "assets/expressions-v2/retired.box.png", + "status": "deleted", + "oldMode": "100644", + "newMode": "000000", + "before": { + "kind": "binary", + "size": 30122, + "hash": "be59af98df030c4738c2e74d332177b3948551248ec8fd91457ab910ec369828" + } + } + ], + "changes": [ + "change-052", + "change-053" + ] + }, + { + "text": "A rename moves the path without changing the bytes, so the card shows the old path, an arrow, and the new path. A transition changes the kind of the file itself: a readable config becomes a compiled blob, and a packed asset becomes readable source. Diffwalk keeps both sides in either direction, so the card shows what the file was and what it became.", + "binary": [ + { + "kind": "binary", + "id": "change-054", + "path": "assets/expressions-v2/excited.box.png", + "status": "renamed", + "oldPath": "assets/expressions-v1/excited.box.png", + "oldMode": "100644", + "newMode": "100644", + "before": { + "kind": "binary", + "size": 44987, + "hash": "3bc0977e7fc8fa58e79e43cc6e0d330c957586d0a555e68006d6d0bfaef8042c" + }, + "after": { + "kind": "binary", + "size": 44987, + "hash": "3bc0977e7fc8fa58e79e43cc6e0d330c957586d0a555e68006d6d0bfaef8042c" + } + }, + { + "kind": "binary", + "id": "change-055", + "path": "assets/loader.conf", + "status": "modified", + "oldMode": "100644", + "newMode": "100644", + "before": { + "kind": "text", + "size": 812, + "hash": "bbf096f2ae6671689f175738f04246ad1b23b7bfc3749fd4e1b04647393b33a5" + }, + "after": { + "kind": "binary", + "size": 6104, + "hash": "37418d6597fe0b07e72d44abc55a7162e8ee9371594c988ba55cf30ce1f6ee95" + } + }, + { + "kind": "binary", + "id": "change-056", + "path": "assets/theme.tokens", + "status": "modified", + "oldMode": "100644", + "newMode": "100644", + "before": { + "kind": "binary", + "size": 256, + "hash": "e4cd60c730974ce82775d6526d807c126616302a66281e5916dfe4fa5e4a56de" + }, + "after": { + "kind": "text", + "size": 318, + "hash": "3b084f8f3fe6b12ee44c9eccd1ff29f7eeac5efe8c8f43b09815b76329769eed" + } + } + ], + "changes": [ + "change-054", + "change-055", + "change-056" + ] + } + ] + }, { "title": "Review a code-only step without an explanation heading", "steps": [