From 63e44dad918afb4b6c8526ff1a24eae5cc4a316b Mon Sep 17 00:00:00 2001 From: Art Pai Date: Sun, 13 Sep 2026 09:37:21 +1000 Subject: [PATCH] Keep validation at entry points and make execution flow explicit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: ことね --- docs/architecture.md | 20 ++- infra/setup.sh | 83 +++++---- scripts/dev-report.ts | 36 ++-- scripts/release.mjs | 90 +++++++--- src/authoring/capture.ts | 233 ++++++++++++++----------- src/authoring/git.ts | 56 +++--- src/authoring/walk.ts | 36 ++-- src/cli.ts | 68 ++++---- src/cli/commands/change.ts | 46 +++-- src/cli/commands/changes.ts | 32 ++-- src/cli/commands/check.ts | 27 ++- src/cli/commands/delete.ts | 12 +- src/cli/commands/export.ts | 48 +++-- src/cli/commands/file.ts | 29 +-- src/cli/commands/inspect.ts | 111 ++++++------ src/cli/commands/publish.ts | 75 +++++--- src/cli/commands/unpublish.ts | 20 ++- src/cli/commands/use.ts | 12 +- src/cli/commands/view.ts | 21 ++- src/cli/commands/walks.ts | 13 +- src/{authoring => cli}/config.ts | 26 +-- src/{authoring => cli}/explanations.ts | 21 ++- src/{authoring => cli}/input.ts | 70 ++++---- src/cli/options.ts | 17 -- src/cli/output.ts | 12 +- src/{authoring => cli}/published.ts | 7 +- src/cli/service.ts | 29 +++ src/{format.ts => format/schema.ts} | 34 ++-- src/format/types.ts | 64 +++++++ src/{publish.ts => publish/client.ts} | 78 ++++----- src/report/client.ts | 45 ++--- src/{report.ts => report/index.ts} | 12 +- src/report/markdown.ts | 8 +- src/report/patches.ts | 10 +- src/report/render.ts | 82 ++++----- src/report/targets.ts | 9 +- src/report/view.ts | 63 ++++--- test/authoring.test.ts | 2 +- test/cli.test.ts | 2 +- test/config.test.ts | 2 +- test/explanations.test.ts | 2 +- test/format.test.ts | 2 +- test/publish.test.ts | 6 +- test/published.test.ts | 2 +- test/report-dom.test.ts | 2 +- test/report-targets.test.ts | 2 +- test/report.test.ts | 2 +- test/visual/fixtures.ts | 2 +- website/public/index.html | 189 ++++++++++---------- worker/build-assets.ts | 23 ++- worker/index.test.ts | 2 +- worker/index.ts | 124 +++++++------ worker/reports.ts | 8 +- 53 files changed, 1183 insertions(+), 844 deletions(-) rename src/{authoring => cli}/config.ts (95%) rename src/{authoring => cli}/explanations.ts (79%) rename src/{authoring => cli}/input.ts (88%) delete mode 100644 src/cli/options.ts rename src/{authoring => cli}/published.ts (92%) create mode 100644 src/cli/service.ts rename src/{format.ts => format/schema.ts} (82%) create mode 100644 src/format/types.ts rename src/{publish.ts => publish/client.ts} (61%) rename src/{report.ts => report/index.ts} (84%) diff --git a/docs/architecture.md b/docs/architecture.md index 84bc4a0..b157f36 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -110,22 +110,24 @@ exact corresponding diffs in a deliberate order. ## Source map -- `src/format.ts`: Zod schemas for the machine-owned capture and the author-edited +- `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/explanations.ts`: strict safe YAML 1.2 parsing into the explanations schema. -- `src/cli/commands/`: one typed handler module per CLI command. -- `src/authoring/input.ts`: shared capture and explanations path resolution, schemas, +- `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. +- `src/cli/input.ts`: shared capture and explanations path resolution, schemas, validation, and persistence. - `src/authoring/walk.ts`: timestamped walk IDs, per-walk paths, listing and deleting walks, and the current-walk pointer. -- `src/authoring/config.ts`: the optional project-level `.diffwalk/config.json` lookup and +- `src/cli/config.ts`: the optional project-level `.diffwalk/config.json` lookup and schema for the review service origin. -- `src/authoring/published.ts`: the locally retained published review (ID, URL, service, +- `src/cli/published.ts`: the locally retained published review (ID, URL, service, and revocation token), written by `publish` and read by `publish --update`. - `src/cli.ts`: executable entry point for `inspect`, `walks`, `use`, `delete`, `changes`, `change`, `file`, `check`, `view`, `export`, `publish`, and `unpublish`. @@ -133,11 +135,11 @@ exact corresponding diffs in a deliberate order. - `src/report/patches.ts`: shared Pierre parse seam used by the generator, the browser client, and tests. - `src/report/markdown.ts`: Markdown rendering with inline HTML passed through. -- `src/report.ts`: atomic report writes and client-bundle loading. +- `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. -- `src/publish.ts`: review service origin checks, publish credential lookup, the - publish, update, and unpublish requests, and adding the Git user name as +- `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. - `src/report/client.ts`: browser entry that mounts a `FileDiff` per file and switches unified/split through `setOptions`. diff --git a/infra/setup.sh b/infra/setup.sh index 3a7a33e..f144adb 100755 --- a/infra/setup.sh +++ b/infra/setup.sh @@ -18,38 +18,45 @@ set -euo pipefail BUCKET="${R2_BUCKET:-diffwalk-reports}" API="https://api.cloudflare.com/client/v4" -for tool in curl jq npx; do - command -v "$tool" >/dev/null || { echo "Missing required tool: $tool" >&2; exit 1; } -done -: "${CLOUDFLARE_API_TOKEN:?Set CLOUDFLARE_API_TOKEN}" -: "${CLOUDFLARE_ZONE_ID:?Set CLOUDFLARE_ZONE_ID}" +main() { + validate_prerequisites + create_report_bucket + disable_public_bucket_url + configure_managed_rules + configure_publish_rate_limit -wrangler() { npx wrangler "$@"; } + echo + echo "Zone configuration is up to date. Deploy the Worker with \`pnpm deploy\`." +} -cloudflare_api() { - local method=$1 path=$2 body=$3 - curl --silent --show-error --fail-with-body \ - --request "$method" \ - --header "Authorization: Bearer ${CLOUDFLARE_API_TOKEN}" \ - --header "Content-Type: application/json" \ - --data "$body" \ - "${API}${path}" +validate_prerequisites() { + local tool + for tool in curl jq npx; do + command -v "$tool" >/dev/null || { echo "Missing required tool: $tool" >&2; exit 1; } + done + : "${CLOUDFLARE_API_TOKEN:?Set CLOUDFLARE_API_TOKEN}" + : "${CLOUDFLARE_ZONE_ID:?Set CLOUDFLARE_ZONE_ID}" } -echo "==> R2 bucket ${BUCKET}" -if wrangler r2 bucket info "$BUCKET" >/dev/null 2>&1; then - echo " already exists" -else - wrangler r2 bucket create "$BUCKET" -fi +create_report_bucket() { + echo "==> R2 bucket ${BUCKET}" + if wrangler r2 bucket info "$BUCKET" >/dev/null 2>&1; then + echo " already exists" + else + wrangler r2 bucket create "$BUCKET" + fi +} # Reports are served only through the Worker, so the bucket must never answer directly. -echo "==> Disabling the r2.dev public URL" -wrangler r2 bucket dev-url disable "$BUCKET" --force >/dev/null -wrangler r2 bucket dev-url get "$BUCKET" +disable_public_bucket_url() { + echo "==> Disabling the r2.dev public URL" + wrangler r2 bucket dev-url disable "$BUCKET" --force >/dev/null + wrangler r2 bucket dev-url get "$BUCKET" +} -echo "==> WAF managed rules" -cloudflare_api PUT "/zones/${CLOUDFLARE_ZONE_ID}/rulesets/phases/http_request_firewall_managed/entrypoint" '{ +configure_managed_rules() { + echo "==> WAF managed rules" + cloudflare_api PUT "/zones/${CLOUDFLARE_ZONE_ID}/rulesets/phases/http_request_firewall_managed/entrypoint" '{ "rules": [ { "action": "execute", @@ -59,12 +66,14 @@ cloudflare_api PUT "/zones/${CLOUDFLARE_ZONE_ID}/rulesets/phases/http_request_fi } ] }' | jq -e '.success' >/dev/null -echo " Cloudflare Free Managed Ruleset deployed" + echo " Cloudflare Free Managed Ruleset deployed" +} # The Free plan provides one rate limiting rule. Spend it on anonymous writes; report reads # remain protected by Cloudflare's network-level DDoS mitigation. -echo "==> Publish rate limit" -cloudflare_api PUT "/zones/${CLOUDFLARE_ZONE_ID}/rulesets/phases/http_ratelimit/entrypoint" '{ +configure_publish_rate_limit() { + echo "==> Publish rate limit" + cloudflare_api PUT "/zones/${CLOUDFLARE_ZONE_ID}/rulesets/phases/http_ratelimit/entrypoint" '{ "rules": [ { "action": "block", @@ -79,7 +88,19 @@ cloudflare_api PUT "/zones/${CLOUDFLARE_ZONE_ID}/rulesets/phases/http_ratelimit/ } ] }' | jq -e '.success' >/dev/null -echo " publish and revoke limited to 5 per 10 seconds per IP" + echo " publish and revoke limited to 5 per 10 seconds per IP" +} + +wrangler() { npx wrangler "$@"; } + +cloudflare_api() { + local method=$1 path=$2 body=$3 + curl --silent --show-error --fail-with-body \ + --request "$method" \ + --header "Authorization: Bearer ${CLOUDFLARE_API_TOKEN}" \ + --header "Content-Type: application/json" \ + --data "$body" \ + "${API}${path}" +} -echo -echo "Zone configuration is up to date. Deploy the Worker with \`pnpm deploy\`." +main diff --git a/scripts/dev-report.ts b/scripts/dev-report.ts index 059e915..09b35bc 100644 --- a/scripts/dev-report.ts +++ b/scripts/dev-report.ts @@ -1,8 +1,8 @@ import { renderReport } from '../src/report/render' -import { explainDocumentSchema } from '../src/format' +import { explainDocumentSchema } from '../src/format/schema' import sample from '../fixtures/report-preview.json' -const doc = explainDocumentSchema.parse(sample) +const document = explainDocumentSchema.parse(sample) const server = Bun.serve({ hostname: '0.0.0.0', @@ -11,21 +11,25 @@ const server = Bun.serve({ if (new URL(request.url).pathname !== '/') { return new Response('Not found', { status: 404 }) } - const build = await Bun.build({ - entrypoints: [new URL('../src/report/client.ts', import.meta.url).pathname], - target: 'browser', - format: 'iife', - minify: true, - }) - if (!build.success) { - console.error(build.logs) - return new Response('Report client build failed', { status: 500 }) - } - const client = await build.outputs[0]!.text() - return new Response(renderReport(doc, client), { - headers: { 'Content-Type': 'text/html; charset=utf-8', 'Cache-Control': 'no-store' }, - }) + return previewReport() }, }) console.log(`Report preview: http://localhost:${server.port}/ (LAN: http://192.168.88.8:${server.port}/)`) + +async function previewReport(): Promise { + const build = await Bun.build({ + entrypoints: [new URL('../src/report/client.ts', import.meta.url).pathname], + target: 'browser', + format: 'iife', + minify: true, + }) + if (!build.success) { + console.error(build.logs) + return new Response('Report client build failed', { status: 500 }) + } + const clientBundle = await build.outputs[0]!.text() + return new Response(renderReport(document, clientBundle), { + headers: { 'Content-Type': 'text/html; charset=utf-8', 'Cache-Control': 'no-store' }, + }) +} diff --git a/scripts/release.mjs b/scripts/release.mjs index 71d2c34..001bc42 100644 --- a/scripts/release.mjs +++ b/scripts/release.mjs @@ -21,23 +21,15 @@ and the npm version afterward.`); } function prepareRelease(version) { - if (!isStable(version)) throw new Error('Usage: pnpm release , e.g. pnpm release 0.1.8'); - if (git('status', '--porcelain')) throw new Error('Commit or stash working-tree changes before preparing a release.'); - run('gh', ['auth', 'status']); - if (run('gh', ['api', 'user', '--jq', '.login']).trim() !== 'claudecafe') { - throw new Error('GitHub CLI must be authenticated as claudecafe.'); - } + validateReleaseVersion(version); + validateCleanTree('preparing a release'); + validateGitHubAccount(); run('git', ['fetch', 'origin', 'main']); const current = packageVersion('origin/main'); - if (!isStable(current) || compareVersions(version, current) <= 0) { - throw new Error(`Release version must be newer than ${current}.`); - } + validateNewerVersion(version, current); const tag = `v${version}`; const branch = `release/${tag}`; - if (git('tag', '--list', tag) || git('branch', '--list', branch) || - git('ls-remote', 'origin', `refs/tags/${tag}`, `refs/heads/${branch}`)) { - throw new Error(`${tag} or ${branch} already exists.`); - } + validateNewReleaseBranch(tag, branch); run('git', ['switch', '-c', branch, 'origin/main']); const pkg = JSON.parse(readFileSync('package.json', 'utf8')); @@ -53,30 +45,78 @@ function prepareRelease(version) { console.log(`${url}\nAfter CI passes and this PR is merged, run pnpm release:publish .`); } -function publishRelease(number) { - if (!/^[1-9]\d*$/.test(number ?? '')) throw new Error('Usage: pnpm release:publish '); - if (git('status', '--porcelain')) throw new Error('Commit or stash working-tree changes before publishing.'); +function publishRelease(prNumber) { + validatePrNumber(prNumber); + validateCleanTree('publishing'); run('gh', ['auth', 'status']); - const pr = JSON.parse(run('gh', ['pr', 'view', number, '--json', 'state,baseRefName,headRefName,mergeCommit'])); + const pr = JSON.parse(run('gh', ['pr', 'view', prNumber, '--json', 'state,baseRefName,headRefName,mergeCommit'])); + validateMergedReleasePr(pr); + const version = pr.headRefName.replace(/^release\/v/, ''); + validateReleaseBranch(pr.headRefName, version); + run('git', ['fetch', 'origin', 'main']); + const commit = pr.mergeCommit.oid; + validateReleaseCommit(commit, version); + const tag = `v${version}`; + validateNewReleaseTag(tag); + run('git', ['tag', tag, commit]); + run('git', ['push', 'origin', `refs/tags/${tag}`]); + console.log(`Pushed ${tag} at ${commit}. Check the Publish workflow in GitHub Actions.`); +} + +function validateReleaseVersion(version) { + if (!isStable(version)) throw new Error('Usage: pnpm release , e.g. pnpm release 0.1.8'); +} + +function validateCleanTree(operation) { + if (git('status', '--porcelain')) throw new Error(`Commit or stash working-tree changes before ${operation}.`); +} + +function validateGitHubAccount() { + run('gh', ['auth', 'status']); + if (run('gh', ['api', 'user', '--jq', '.login']).trim() !== 'claudecafe') { + throw new Error('GitHub CLI must be authenticated as claudecafe.'); + } +} + +function validateNewerVersion(version, current) { + if (!isStable(current) || compareVersions(version, current) <= 0) { + throw new Error(`Release version must be newer than ${current}.`); + } +} + +function validateNewReleaseBranch(tag, branch) { + if (git('tag', '--list', tag) || git('branch', '--list', branch) || + git('ls-remote', 'origin', `refs/tags/${tag}`, `refs/heads/${branch}`)) { + throw new Error(`${tag} or ${branch} already exists.`); + } +} + +function validatePrNumber(prNumber) { + if (!/^[1-9]\d*$/.test(prNumber ?? '')) throw new Error('Usage: pnpm release:publish '); +} + +function validateMergedReleasePr(pr) { if (pr.state !== 'MERGED' || pr.baseRefName !== 'main' || !pr.mergeCommit?.oid) { throw new Error('The release PR must be merged into main before publishing.'); } - const version = pr.headRefName.replace(/^release\/v/, ''); - if (!pr.headRefName.startsWith('release/v') || !isStable(version)) { +} + +function validateReleaseBranch(branch, version) { + if (!branch.startsWith('release/v') || !isStable(version)) { throw new Error('Expected a release/v PR branch.'); } - run('git', ['fetch', 'origin', 'main']); - const commit = pr.mergeCommit.oid; +} + +function validateReleaseCommit(commit, version) { if (!/^[0-9a-f]{40}$/.test(commit)) throw new Error('GitHub returned an invalid merge commit.'); git('merge-base', '--is-ancestor', commit, 'origin/main'); if (packageVersion(commit) !== version) throw new Error('The merged package version does not match the release branch.'); - const tag = `v${version}`; +} + +function validateNewReleaseTag(tag) { if (git('tag', '--list', tag) || git('ls-remote', 'origin', `refs/tags/${tag}`)) { throw new Error(`${tag} already exists.`); } - run('git', ['tag', tag, commit]); - run('git', ['push', 'origin', `refs/tags/${tag}`]); - console.log(`Pushed ${tag} at ${commit}. Check the Publish workflow in GitHub Actions.`); } function isStable(version) { diff --git a/src/authoring/capture.ts b/src/authoring/capture.ts index 99fd2ec..59b7f36 100644 --- a/src/authoring/capture.ts +++ b/src/authoring/capture.ts @@ -1,15 +1,15 @@ import { createHash } from 'node:crypto' import { diffLines, formatPatch, structuredPatch, type StructuredPatch } from 'diff' -import { - captureSchema, - explainDocumentSchema, - type CaptureSource, - type ChangeBlock, - type DraftFile, - type ExplainCapture, - type ExplainDocument, - type Explanations, -} from '../format' +import type { + CaptureSource, + ChangeBlock, + DraftFile, + DocumentStep, + ExplanationStep, + ExplainCapture, + ExplainDocument, + Explanations, +} from '../format/types' export function createExplainCapture(files: DraftFile[], source: CaptureSource): ExplainCapture { let nextId = 1 @@ -38,56 +38,12 @@ export function createExplainCapture(files: DraftFile[], source: CaptureSource): changes.push(...fileChanges) } - return captureSchema.parse({ + return { captureId: captureIdFor(files), source, files, changes, - }) -} - -function changeBlocks(file: DraftFile): Omit[] { - const parts = diffLines(file.oldContent, file.newContent) - const changes: Omit[] = [] - let oldIndex = 0 - let newIndex = 0 - - for (let index = 0; index < parts.length;) { - const part = parts[index]! - if (!part.added && !part.removed) { - oldIndex += part.count ?? 0 - newIndex += part.count ?? 0 - index++ - continue - } - - const oldStart = oldIndex + 1 - const newStart = newIndex + 1 - let before = '' - let after = '' - let oldCount = 0 - let newCount = 0 - - while (index < parts.length) { - const changed = parts[index]! - if (!changed.added && !changed.removed) break - const count = changed.count ?? 0 - if (changed.removed) { - before += changed.value - oldCount += count - oldIndex += count - } else { - after += changed.value - newCount += count - newIndex += count - } - index++ - } - - changes.push({ oldStart, oldCount, newStart, newCount, before, after }) } - - return changes } export function captureIdFor(files: DraftFile[], includeModes = true): string { @@ -122,9 +78,9 @@ export function duplicatedChangeIds(explanations: Explanations): string[] { const repeated = new Set() for (const section of explanations.sections) { for (const step of section.steps) { - for (const id of step.changes ?? []) { - if (seen.has(id)) repeated.add(id) - seen.add(id) + for (const changeId of step.changes ?? []) { + if (seen.has(changeId)) repeated.add(changeId) + seen.add(changeId) } } } @@ -135,68 +91,137 @@ export function materializeExplainDocument( capture: ExplainCapture, explanations: Explanations, ): ExplainDocument { + validateCapturePair(capture, explanations) + const filesByPath = new Map(capture.files.map((file) => [file.path, file])) + const changesById = new Map(capture.changes.map((change) => [change.id, change])) + + const shown = new Set() + const sections = explanations.sections.map((section) => ({ + title: section.title, + steps: section.steps.map((step) => materializeStep(step, filesByPath, changesById, shown)), + })) + + validateChangeCoverage(capture.changes, shown) + + return { + formatVersion: 1, + title: explanations.title, + summary: explanations.summary, + source: capture.source, + ...(explanations.metadata === undefined ? {} : { metadata: explanations.metadata }), + sections, + } +} + +function materializeStep( + step: ExplanationStep, + filesByPath: Map, + changesById: Map, + shown: Set, +): DocumentStep { + if (step.changes === undefined) { + return { text: step.text } + } else { + const selected = step.changes.map((changeId) => { + const change = findChange(changesById, changeId) + shown.add(changeId) + return change + }) + + const changesByPath = new Map() + for (const change of selected) { + const fileChanges = changesByPath.get(change.path) ?? [] + fileChanges.push(change) + changesByPath.set(change.path, fileChanges) + } + + const patches: StructuredPatch[] = [] + for (const [path, fileChanges] of changesByPath) { + const file = findFile(filesByPath, path) + patches.push(createFilePatch(file, fileChanges)) + } + + return { + text: step.text, + diff: patches.map(formatFilePatch).join('\n'), + changes: step.changes, + } + } +} + +function findChange(changesById: Map, changeId: string): ChangeBlock { + const change = changesById.get(changeId) + if (!change) throw new Error(`Unknown change ID: ${changeId}`) + return change +} + +function findFile(filesByPath: Map, filePath: string): DraftFile { + const file = filesByPath.get(filePath) + if (!file) throw new Error(`Change references missing file: ${filePath}`) + return file +} + +function validateCapturePair(capture: ExplainCapture, explanations: Explanations): void { if (capture.captureId !== explanations.captureId) { throw new Error(stalePairingMessage(capture, explanations)) } - if (capture.changes.length === 0 && explanations.sections.length === 0) { throw new Error('No captured changes or authored sections to materialize; nothing to view, export, or publish.') } - - const filesByPath = new Map(capture.files.map((file) => [file.path, file])) - const changesById = new Map(capture.changes.map((change) => [change.id, change])) - if (changesById.size !== capture.changes.length) { + if (new Set(capture.changes.map((change) => change.id)).size !== capture.changes.length) { throw new Error('Capture contains duplicate change IDs') } +} - const shown = new Set() - const sections = explanations.sections.map((section) => ({ - title: section.title, - steps: section.steps.map((step) => { - if (step.changes === undefined) return { text: step.text } - - const selected = step.changes.map((id) => { - const change = changesById.get(id) - if (!change) throw new Error(`Unknown change ID: ${id}`) - shown.add(id) - return change - }) +function validateChangeCoverage(changes: ChangeBlock[], shown: Set): void { + const unshown = changes.filter((change) => !shown.has(change.id)) + if (unshown.length > 0) { + throw new Error(`Unassigned change IDs: ${unshown.map((change) => change.id).join(', ')}`) + } +} - const changesByPath = new Map() - for (const change of selected) { - const fileChanges = changesByPath.get(change.path) ?? [] - fileChanges.push(change) - changesByPath.set(change.path, fileChanges) - } +function changeBlocks(file: DraftFile): Omit[] { + const parts = diffLines(file.oldContent, file.newContent) + const changes: Omit[] = [] + let oldIndex = 0 + let newIndex = 0 - const patches: StructuredPatch[] = [] - for (const [path, fileChanges] of changesByPath) { - const file = filesByPath.get(path) - if (!file) throw new Error(`Change references missing file: ${path}`) - patches.push(createFilePatch(file, fileChanges)) - } + for (let index = 0; index < parts.length;) { + const part = parts[index]! + if (!part.added && !part.removed) { + oldIndex += part.count ?? 0 + newIndex += part.count ?? 0 + index++ + continue + } - return { - text: step.text, - diff: patches.map(formatFilePatch).join('\n'), - changes: step.changes, + const oldStart = oldIndex + 1 + const newStart = newIndex + 1 + let before = '' + let after = '' + let oldCount = 0 + let newCount = 0 + + while (index < parts.length) { + const changed = parts[index]! + if (!changed.added && !changed.removed) break + const count = changed.count ?? 0 + if (changed.removed) { + before += changed.value + oldCount += count + oldIndex += count + } else { + after += changed.value + newCount += count + newIndex += count } - }), - })) + index++ + } - const unshown = capture.changes.filter((change) => !shown.has(change.id)) - if (unshown.length > 0) { - throw new Error(`Unassigned change IDs: ${unshown.map((change) => change.id).join(', ')}`) + changes.push({ oldStart, oldCount, newStart, newCount, before, after }) } - return explainDocumentSchema.parse({ - formatVersion: 1, - title: explanations.title, - summary: explanations.summary, - source: capture.source, - ...(explanations.metadata === undefined ? {} : { metadata: explanations.metadata }), - sections, - }) + return changes } function createFilePatch(file: DraftFile, changes: ChangeBlock[]): StructuredPatch { diff --git a/src/authoring/git.ts b/src/authoring/git.ts index af9597d..4087acb 100644 --- a/src/authoring/git.ts +++ b/src/authoring/git.ts @@ -2,7 +2,7 @@ import { spawn } from 'node:child_process' 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' +import type { DraftFile } from '../format/types' export interface GitCapture { root: string @@ -66,15 +66,10 @@ export async function captureGitChanges( oldContent: await gitFile(baseCommit, change.oldPath!, root), newContent: await newContentFor(change.path), }) - continue - } - - if (change.kind === 'M') { + } else if (change.kind === 'M') { const oldContent = await gitFile(baseCommit, change.path, root) const newContent = await newContentFor(change.path) - if (change.oldMode !== change.newMode && oldContent === newContent) { - throw new Error(`File mode changes are not supported: ${change.path}`) - } + validateFileModeChange(change.path, change.oldMode, change.newMode, oldContent, newContent) files.push({ path: change.path, status: 'modified', @@ -121,9 +116,7 @@ export async function captureGitChanges( if (existing.status !== 'deleted') continue const newContent = await workingTreeFile(path, root) const newMode = await workingTreeMode(path, root) - if (existing.oldMode !== newMode && existing.oldContent === newContent) { - throw new Error(`File mode changes are not supported: ${path}`) - } + validateFileModeChange(path, existing.oldMode, newMode, existing.oldContent, newContent) if (existing.oldContent === newContent) { filesByPath.delete(path) } else { @@ -173,8 +166,8 @@ export async function captureGitRevisionChanges( 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) - if (change.kind === 'M' && change.oldMode !== change.newMode && oldContent === newContent) { - throw new Error(`File mode changes are not supported: ${change.path}`) + if (change.kind === 'M') { + validateFileModeChange(change.path, change.oldMode, change.newMode, oldContent, newContent) } files.push({ path: change.path, @@ -214,16 +207,7 @@ export async function gitUserName(root = process.cwd()): Promise { - const absolutePath = resolve(root, path) - const pathWithinRoot = relative(root, absolutePath) - if (isAbsolute(pathWithinRoot) || pathWithinRoot.startsWith('..')) { - throw new Error(`Git path escapes the repository: ${path}`) - } - - const file = await lstat(absolutePath) - if (file.isSymbolicLink()) throw new Error(`Symbolic links are not supported: ${path}`) - if (!file.isFile()) throw new Error(`Non-file Git paths are not supported: ${path}`) - + const absolutePath = await validateWorkingTreeFile(path, root) const scratch = await mkdtemp(join(tmpdir(), 'diffwalk-working-tree-')) try { const objectDirectory = join(scratch, 'objects') @@ -238,6 +222,32 @@ async function workingTreeFile(path: string, root: string): Promise { } } +function validateFileModeChange( + filePath: string, + oldMode: DraftFile['oldMode'], + newMode: DraftFile['newMode'], + oldContent: string, + newContent: string, +): void { + if (oldMode !== newMode && oldContent === newContent) { + throw new Error(`File mode changes are not supported: ${filePath}`) + } +} + +async function validateWorkingTreeFile(path: string, root: string): Promise { + const absolutePath = resolve(root, path) + const pathWithinRoot = relative(root, absolutePath) + if (isAbsolute(pathWithinRoot) || pathWithinRoot.startsWith('..')) { + throw new Error(`Git path escapes the repository: ${path}`) + } + + const file = await lstat(absolutePath) + if (file.isSymbolicLink()) throw new Error(`Symbolic links are not supported: ${path}`) + if (!file.isFile()) throw new Error(`Non-file Git paths are not supported: ${path}`) + + return absolutePath +} + async function workingTreeMode(path: string, root: string): Promise { const file = await lstat(resolve(root, path)) return (file.mode & 0o100) === 0 ? '100644' : '100755' diff --git a/src/authoring/walk.ts b/src/authoring/walk.ts index 98a3f44..83fad62 100644 --- a/src/authoring/walk.ts +++ b/src/authoring/walk.ts @@ -20,11 +20,11 @@ export function walkId(capturedAt: string, captureId: string): string { return `${timestamp}-${captureId.slice(0, 8)}` } -export function walkPaths(id: string, root = diffwalkDirectory): WalkPaths { - if (!walkIdPattern.test(id)) throw new Error(`Invalid Diffwalk walk ID: ${id}`) - const directory = join(root, id) +export function walkPaths(walkId: string, root = diffwalkDirectory): WalkPaths { + validateWalkId(walkId) + const directory = join(root, walkId) return { - id, + id: walkId, directory, capture: join(directory, 'capture.json'), explanations: join(directory, 'explanations.yaml'), @@ -43,8 +43,8 @@ export async function currentWalk(root = diffwalkDirectory): Promise } export async function currentWalkIfPresent(root = diffwalkDirectory): Promise { - const id = await currentWalkIdIfPresent(root) - return id === null ? null : walkPaths(id, root) + const currentWalkId = await currentWalkIdIfPresent(root) + return currentWalkId === null ? null : walkPaths(currentWalkId, root) } export async function currentWalkIdIfPresent(root = diffwalkDirectory): Promise { @@ -57,10 +57,10 @@ export async function currentWalkIdIfPresent(root = diffwalkDirectory): Promise< } } -export async function setCurrentWalk(id: string, root = diffwalkDirectory): Promise { - walkPaths(id, root) +export async function setCurrentWalk(walkId: string, root = diffwalkDirectory): Promise { + validateWalkId(walkId) await mkdir(root, { recursive: true }) - await writeFile(join(root, 'current'), `${id}\n`) + await writeFile(join(root, 'current'), `${walkId}\n`) } export async function listWalkIds(root = diffwalkDirectory): Promise { @@ -78,8 +78,8 @@ export async function listWalkIds(root = diffwalkDirectory): Promise { .reverse() } -export async function walkExists(id: string, root = diffwalkDirectory): Promise { - const paths = walkPaths(id, root) +export async function walkExists(walkId: string, root = diffwalkDirectory): Promise { + const paths = walkPaths(walkId, root) try { return (await lstat(paths.directory)).isDirectory() } catch (error) { @@ -88,14 +88,18 @@ export async function walkExists(id: string, root = diffwalkDirectory): Promise< } } -export async function deleteWalk(id: string, root = diffwalkDirectory): Promise { - const paths = walkPaths(id, root) - const wasCurrent = (await currentWalkIdIfPresent(root)) === id - if (await walkExists(id, root)) { +export async function deleteWalk(walkId: string, root = diffwalkDirectory): Promise { + const paths = walkPaths(walkId, root) + const wasCurrent = (await currentWalkIdIfPresent(root)) === walkId + if (await walkExists(walkId, root)) { await rm(paths.directory, { recursive: true }) } else if (!wasCurrent) { - throw new Error(`No Diffwalk walk ${id} to delete.`) + throw new Error(`No Diffwalk walk ${walkId} to delete.`) } if (wasCurrent) await rm(join(root, 'current'), { force: true }) return wasCurrent } + +function validateWalkId(walkId: string): void { + if (!walkIdPattern.test(walkId)) throw new Error(`Invalid Diffwalk walk ID: ${walkId}`) +} diff --git a/src/cli.ts b/src/cli.ts index 088309b..08b7d6e 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -1,20 +1,18 @@ #!/usr/bin/env node import { Command } from 'commander' -import { authoringOptionsSchema, captureOptionsSchema } from './authoring/input' -import { changeCommand } from './cli/commands/change' -import { changesCommand, changesOptionsSchema } from './cli/commands/changes' -import { checkCommand } from './cli/commands/check' -import { deleteCommand } from './cli/commands/delete' -import { exportCommand, exportOptionsSchema } from './cli/commands/export' -import { fileCommand, fileOptionsSchema } from './cli/commands/file' -import { inspectCommand, inspectOptionsSchema } from './cli/commands/inspect' -import { publishCommand, publishOptionsSchema } from './cli/commands/publish' -import { unpublishCommand, unpublishOptionsSchema } from './cli/commands/unpublish' -import { useCommand } from './cli/commands/use' -import { viewCommand } from './cli/commands/view' -import { walksCommand } from './cli/commands/walks' -import { withArgument, withOptions } from './cli/options' +import { printChange } from './cli/commands/change' +import { printChanges } from './cli/commands/changes' +import { checkReview } from './cli/commands/check' +import { removeWalk } from './cli/commands/delete' +import { exportReview } from './cli/commands/export' +import { printFile } from './cli/commands/file' +import { inspectChanges } from './cli/commands/inspect' +import { publishReview } from './cli/commands/publish' +import { removeReview } from './cli/commands/unpublish' +import { selectWalk } from './cli/commands/use' +import { viewReview } from './cli/commands/view' +import { printWalks } from './cli/commands/walks' import { UsageError } from './cli/usage' import packageJson from '../package.json' @@ -49,9 +47,9 @@ function createCli(): Command { .addHelpText('after', '\nLimit a working-tree capture with --staged or a `-- ...` list.') .action((_revision: string | undefined, options: Record, command: Command) => { const positionals = inspectPositionals(command) - return inspectCommand( + return inspectChanges( positionals.revision, - inspectOptionsSchema.parse(options), + options, positionals.paths, ) }) @@ -59,30 +57,30 @@ function createCli(): Command { cli .command('walks') .description('List timestamped walks and mark the current one') - .action(() => walksCommand()) + .action(() => printWalks()) cli .command('use ') .description('Select a local walk as current') - .action((id: string) => useCommand(id)) + .action(selectWalk) cli .command('delete ') .description('Delete a local walk') - .action((id: string) => deleteCommand(id)) + .action(removeWalk) cli .command('changes') .description('List captured change blocks') .option('--json', 'Print structured JSON change data') .option('--input ', 'Use an explicit capture path') - .action(withOptions(changesOptionsSchema, changesCommand)) + .action(printChanges) cli .command('change ') .description('Read one captured change block') .option('--input ', 'Use an explicit capture path') - .action(withArgument(captureOptionsSchema, changeCommand)) + .action(printChange) cli .command('file ') @@ -90,21 +88,21 @@ function createCli(): Command { .option('--before', 'Print the captured old side') .option('--after', 'Print the captured new side') .option('--input ', 'Use an explicit capture path') - .action(withArgument(fileOptionsSchema, fileCommand)) + .action(printFile) cli .command('check') .description('Validate capture and explanations') .option('--input ', 'Use an explicit capture path') .option('--explanations ', 'Use an explicit explanations path') - .action(withOptions(authoringOptionsSchema, checkCommand)) + .action(checkReview) cli .command('view') .description('Preview the review in a local browser') .option('--input ', 'Use an explicit capture path') .option('--explanations ', 'Use an explicit explanations path') - .action(withOptions(authoringOptionsSchema, viewCommand)) + .action(viewReview) cli .command('export ') @@ -112,7 +110,7 @@ function createCli(): Command { .option('--input ', 'Use an explicit capture path') .option('--explanations ', 'Use an explicit explanations path') .option('--output ', 'Write to an explicit output path') - .action(withArgument(exportOptionsSchema, exportCommand)) + .action(exportReview) cli .command('publish') @@ -121,14 +119,14 @@ function createCli(): Command { .option('--explanations ', 'Use an explicit explanations path') .option('--service ', 'Use an explicit review service origin') .option('--update', 'Replace the content behind the retained review link') - .action(withOptions(publishOptionsSchema, publishCommand)) + .action(publishReview) cli .command('unpublish ') .description('Remove a published review') .option('--token ', 'Use the review revocation token') .option('--service ', 'Use an explicit review service origin') - .action(withArgument(unpublishOptionsSchema, unpublishCommand)) + .action(removeReview) return cli } @@ -137,18 +135,22 @@ function inspectPositionals(command: Command): { revision: string | undefined; p const operands = command.args const separator = process.argv.indexOf('--', 2) if (separator === -1) { - if (operands.length > 1) { - throw new UsageError('Pass at most one revision; separate paths from options with `--`') - } + validateRevisionCount(operands.length, false) return { revision: operands[0], paths: [] } } const paths = process.argv.slice(separator + 1) - if (operands.length - paths.length > 1) { - throw new UsageError('Pass at most one revision before `--`') - } + validateRevisionCount(operands.length - paths.length, true) return { revision: operands.length > paths.length ? operands[0] : undefined, paths } } +function validateRevisionCount(count: number, hasPathSeparator: boolean): void { + if (count > 1) { + throw new UsageError(hasPathSeparator + ? 'Pass at most one revision before `--`' + : 'Pass at most one revision; separate paths from options with `--`') + } +} + function reportError(error: unknown): void { console.error(error instanceof Error ? error.message : String(error)) process.exitCode = 1 diff --git a/src/cli/commands/change.ts b/src/cli/commands/change.ts index 368cd21..a7aa338 100644 --- a/src/cli/commands/change.ts +++ b/src/cli/commands/change.ts @@ -1,15 +1,37 @@ -import { captureInput, readCapture, type CaptureOptions } from '../../authoring/input' +import { z } from 'zod' +import type { ChangeBlock, ExplainCapture } from '../../format/types' +import { captureInput, readCapture } from '../input' import { coordinates } from '../output' -export async function changeCommand(id: string, options: CaptureOptions): Promise { - const capture = await readCapture(await captureInput(options)) - const change = capture.changes.find((candidate) => candidate.id === id) - if (!change) throw new Error(`Unknown change ID: ${id}`) - console.log(`${change.id} ${change.path} ${coordinates(change)}`) - console.log('before:') - process.stdout.write(change.before) - if (!change.before.endsWith('\n')) console.log() - console.log('after:') - process.stdout.write(change.after) - if (!change.after.endsWith('\n')) console.log() +const changeOptionsSchema = z.object({ + input: z.string().optional(), +}) + +export async function printChange(changeId: string, options: z.input): Promise { + const { input } = changeOptionsSchema.parse(options) + const capture = await loadCapture(input) + const change = findChange(capture, changeId) + printChangeDetails(change) +} + +async function loadCapture(input: string | undefined): Promise { + return readCapture(await captureInput({ input })) +} + +function findChange(capture: ExplainCapture, changeId: string): ChangeBlock { + const change = capture.changes.find((candidate) => candidate.id === changeId) + if (!change) throw new Error(`Unknown change ID: ${changeId}`) + return change +} + +function printChangeDetails(change: ChangeBlock): 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 + process.stdout.write(`${change.id} ${change.path} ${coordinates(change)} +before: +${before} +after: +${after} +`) } diff --git a/src/cli/commands/changes.ts b/src/cli/commands/changes.ts index d05b46c..4dad7ec 100644 --- a/src/cli/commands/changes.ts +++ b/src/cli/commands/changes.ts @@ -1,21 +1,29 @@ import { z } from 'zod' -import { captureInput, captureOptionsSchema, readCapture, shortId } from '../../authoring/input' +import type { ExplainCapture } from '../../format/types' +import { captureInput, readCapture, shortId } from '../input' import { changeLine } from '../output' -export const changesOptionsSchema = captureOptionsSchema.extend({ +const changesOptionsSchema = z.object({ + input: z.string().optional(), json: z.boolean().optional(), }) -type ChangesOptions = z.infer -export async function changesCommand(options: ChangesOptions): Promise { - const capture = await readCapture(await captureInput(options)) - if (options.json === true) { - console.log(JSON.stringify({ captureId: capture.captureId, changes: capture.changes }, null, 2)) - return +export async function printChanges(options: z.input): Promise { + const { input, json = false } = changesOptionsSchema.parse(options) + const capture = await readCapture(await captureInput({ input })) + switch (json) { + case true: + console.log(JSON.stringify({ captureId: capture.captureId, changes: capture.changes }, null, 2)) + break + case false: + printCaptureChanges(capture) + break } +} + +function printCaptureChanges(capture: ExplainCapture): void { const fileCount = new Set(capture.files.map((file) => file.path)).size - console.log( - `${capture.changes.length} changes across ${fileCount} files · capture ${shortId(capture.captureId)}`, - ) - for (const change of capture.changes) console.log(changeLine(change)) + const heading = `${capture.changes.length} changes across ${fileCount} files · capture ${shortId(capture.captureId)}` + const lines = capture.changes.map(changeLine) + console.log([heading, ...lines].join('\n')) } diff --git a/src/cli/commands/check.ts b/src/cli/commands/check.ts index 24e445e..ed5c906 100644 --- a/src/cli/commands/check.ts +++ b/src/cli/commands/check.ts @@ -1,18 +1,29 @@ +import { z } from 'zod' import { duplicatedChangeIds } from '../../authoring/capture' -import { materialize, shortId, type AuthoringOptions } from '../../authoring/input' +import { materialize, shortId, type MaterializedAuthoring } from '../input' -export async function checkCommand(options: AuthoringOptions): Promise { - const { capture, explanations, document } = await materialize(options) +const checkOptionsSchema = z.object({ + input: z.string().optional(), + explanations: z.string().optional(), +}) + +export async function checkReview(options: z.input): Promise { + const { input, explanations } = checkOptionsSchema.parse(options) + const review = await materialize({ input, explanations }) + printCheckResult(review) +} + +function printCheckResult({ capture, explanations, document }: MaterializedAuthoring): void { const fileCount = new Set(capture.files.map((file) => file.path)).size const steps = document.sections.reduce((total, section) => total + section.steps.length, 0) - console.log( - `OK: ${document.sections.length} sections and ${steps} steps cover ${capture.changes.length} of ${capture.changes.length} changes across ${fileCount} files · capture ${shortId(capture.captureId)}`, - ) + const summary = `OK: ${document.sections.length} sections and ${steps} steps cover ${capture.changes.length} of ${capture.changes.length} changes across ${fileCount} files · capture ${shortId(capture.captureId)}` // Showing a change twice is a legitimate way to build an argument, so it is reported // rather than rejected. Only an unexplained change fails the check. const repeated = duplicatedChangeIds(explanations) + const lines = [summary] if (repeated.length > 0) { - console.log(`${repeated.length} changes are shown more than once: ${repeated.join(', ')}`) + lines.push(`${repeated.length} changes are shown more than once: ${repeated.join(', ')}`) } - console.log('Next: `diffwalk view`, `diffwalk export html`, or `diffwalk publish`.') + lines.push('Next: `diffwalk view`, `diffwalk export html`, or `diffwalk publish`.') + console.log(lines.join('\n')) } diff --git a/src/cli/commands/delete.ts b/src/cli/commands/delete.ts index 416391e..d212a97 100644 --- a/src/cli/commands/delete.ts +++ b/src/cli/commands/delete.ts @@ -1,9 +1,9 @@ import { deleteWalk } from '../../authoring/walk' -export async function deleteCommand(id: string): Promise { - const clearedCurrent = await deleteWalk(id) - console.log(`Deleted walk ${id}.`) - if (clearedCurrent) { - console.log('Cleared the current walk; run `diffwalk use ` to select another.') - } +export async function removeWalk(walkId: string): Promise { + const clearedCurrent = await deleteWalk(walkId) + const next = clearedCurrent + ? '\nCleared the current walk; run `diffwalk use ` to select another.' + : '' + console.log(`Deleted walk ${walkId}.${next}`) } diff --git a/src/cli/commands/export.ts b/src/cli/commands/export.ts index 27c4ea3..e0d3a8b 100644 --- a/src/cli/commands/export.ts +++ b/src/cli/commands/export.ts @@ -1,29 +1,45 @@ import { z } from 'zod' -import { authoringOptionsSchema, materialize, writeJson } from '../../authoring/input' +import type { ExplainDocument } from '../../format/types' +import { materialize, writeJson } from '../input' import { loadReportClient, renderReport, writeReport } from '../../report' import { UsageError } from '../usage' -export const exportOptionsSchema = authoringOptionsSchema.extend({ +const exportOptionsSchema = z.object({ + input: z.string().optional(), + explanations: z.string().optional(), output: z.string().optional(), }) -type ExportOptions = z.infer -export async function exportCommand(format: string, options: ExportOptions): Promise { +export async function exportReview(format: string, options: z.input): Promise { + const { input, explanations, output } = exportOptionsSchema.parse(options) + validateExportFormat(format) + const { document, paths } = await materialize({ input, explanations }) + switch (format) { + case 'html': + await exportHtml(document, output ?? paths.html) + break + case 'json': + await exportJson(document, output ?? paths.json) + break + } +} + +function validateExportFormat(format: string): asserts format is 'html' | 'json' { if (format !== 'html' && format !== 'json') { throw new UsageError(`Unknown export format: ${format}`) } - const { document, paths } = await materialize(options) - if (format === 'html') { - const output = options.output ?? paths.html - const clientBundle = await loadReportClient() - const html = renderReport(document, clientBundle) - await writeReport(output, html) - console.log( - `Wrote a ${html.length} byte review for ${document.sections.length} sections to ${output}`, - ) - return - } - const output = options.output ?? paths.json +} + +async function exportHtml(document: ExplainDocument, output: string): Promise { + const clientBundle = await loadReportClient() + const html = renderReport(document, clientBundle) + await writeReport(output, html) + console.log( + `Wrote a ${html.length} byte review for ${document.sections.length} sections to ${output}`, + ) +} + +async function exportJson(document: ExplainDocument, output: string): Promise { await writeJson(output, document) console.log(`Wrote ${document.sections.length} explanation sections to ${output}`) } diff --git a/src/cli/commands/file.ts b/src/cli/commands/file.ts index 4775a75..b96f62e 100644 --- a/src/cli/commands/file.ts +++ b/src/cli/commands/file.ts @@ -1,21 +1,30 @@ import { z } from 'zod' -import { captureInput, captureOptionsSchema, readCapture } from '../../authoring/input' +import type { DraftFile, ExplainCapture } from '../../format/types' +import { captureInput, readCapture } from '../input' import { UsageError } from '../usage' -export const fileOptionsSchema = captureOptionsSchema.extend({ +const fileOptionsSchema = z.object({ + input: z.string().optional(), before: z.boolean().optional(), after: z.boolean().optional(), }) -type FileOptions = z.infer -export async function fileCommand(path: string, options: FileOptions): Promise { - const before = options.before === true - const after = options.after === true +export async function printFile(filePath: string, options: z.input): Promise { + const { input, before = false, after = false } = fileOptionsSchema.parse(options) + validateFileSide(before, after) + const capture = await readCapture(await captureInput({ input })) + const file = findFile(capture, filePath) + process.stdout.write(before ? file.oldContent : file.newContent) +} + +function validateFileSide(before: boolean, after: boolean): void { if (before === after) { throw new UsageError('Choose exactly one side with --before or --after') } - const capture = await readCapture(await captureInput(options)) - const file = capture.files.find((candidate) => candidate.path === path) - if (!file) throw new Error(`Unknown file path: ${path}`) - process.stdout.write(before ? file.oldContent : file.newContent) +} + +function findFile(capture: ExplainCapture, filePath: string): DraftFile { + const file = capture.files.find((candidate) => candidate.path === filePath) + if (!file) throw new Error(`Unknown file path: ${filePath}`) + return file } diff --git a/src/cli/commands/inspect.ts b/src/cli/commands/inspect.ts index d6e9ee5..723468f 100644 --- a/src/cli/commands/inspect.ts +++ b/src/cli/commands/inspect.ts @@ -8,13 +8,13 @@ import { writeCapture, writeJson, writeText, -} from '../../authoring/input' -import type { ExplainCapture } from '../../format' +} from '../input' +import type { ExplainCapture } from '../../format/types' import { captureGitChanges, captureGitRevisionChanges, commitForRevision } from '../../authoring/git' import { UsageError } from '../usage' import { currentWalkIfPresent, setCurrentWalk, walkId, walkPaths } from '../../authoring/walk' -export const inspectOptionsSchema = z.object({ +const inspectOptionsSchema = z.object({ staged: z.boolean().default(false), base: z.string().optional(), from: z.string().optional(), @@ -24,13 +24,23 @@ export const inspectOptionsSchema = z.object({ }) type InspectOptions = z.infer -export async function inspectCommand( +export async function inspectChanges( revision: string | undefined, - options: InspectOptions, + options: z.input, paths: string[] = [], ): Promise { + const { base, from, to, staged, output, explanations } = inspectOptionsSchema.parse(options) + validateCaptureOptions(revision, { base, from, to, staged }, paths) const capturedAt = new Date().toISOString() - const { base, from, to, staged } = options + const capture = await captureChanges(revision, { base, from, to, staged }, paths, capturedAt) + await saveCapture(capture, { output, explanations }, capturedAt) +} + +function validateCaptureOptions( + revision: string | undefined, + { base, from, to, staged }: Pick, + paths: string[], +): void { if (from !== undefined || to !== undefined) { if (from === undefined || to === undefined) { throw new UsageError('Pass both --from and --to for a committed revision range') @@ -44,67 +54,70 @@ export async function inspectCommand( if (staged) { throw new UsageError('Do not combine --staged with --from/--to') } - const git = await captureGitRevisionChanges(from, to) - const capture = createExplainCapture(git.files, { - kind: 'commit-diff', - from: { revision: from, commit: git.fromCommit }, - to: { revision: to, commit: git.toCommit }, - capturedAt, - }) - await finishInspect(options, capture, capturedAt) - return - } - if (revision !== undefined && base !== undefined) { - throw new UsageError('Do not combine a positional commit revision with --base') - } - if (revision !== undefined) { + } else if (revision !== undefined) { + if (base !== undefined) { + throw new UsageError('Do not combine a positional commit revision with --base') + } if (paths.length > 0) { throw new UsageError('Path limiting applies only to working-tree captures') } if (staged) { throw new UsageError('Do not combine --staged with a positional commit revision') } + } +} + +async function captureChanges( + revision: string | undefined, + { base, from, to, staged }: Pick, + paths: string[], + capturedAt: string, +): Promise { + if (from !== undefined && to !== undefined) { + const git = await captureGitRevisionChanges(from, to) + return createExplainCapture(git.files, { + kind: 'commit-diff', + from: { revision: from, commit: git.fromCommit }, + to: { revision: to, commit: git.toCommit }, + capturedAt, + }) + } else if (revision !== undefined) { const commit = await commitForRevision(revision) const parent = await firstParent(commit) const git = await captureGitRevisionChanges(`${revision}^1`, revision) - const capture = createExplainCapture(git.files, { + return createExplainCapture(git.files, { kind: 'commit-diff', from: { revision: `${revision}^1`, commit: parent }, to: { revision, commit }, capturedAt, }) - await finishInspect(options, capture, capturedAt) - return + } else { + const resolvedBase = base ?? 'HEAD' + const git = await captureGitChanges(resolvedBase, process.cwd(), { staged, paths }) + return createExplainCapture(git.files, { + kind: 'working-tree', + from: { revision: resolvedBase, commit: git.baseCommit }, + capturedAt, + }) } - const resolvedBase = base ?? 'HEAD' - const git = await captureGitChanges(resolvedBase, process.cwd(), { staged, paths }) - const capture = createExplainCapture(git.files, { - kind: 'working-tree', - from: { revision: resolvedBase, commit: git.baseCommit }, - capturedAt, - }) - await finishInspect(options, capture, capturedAt) } -async function finishInspect( - options: InspectOptions, +async function saveCapture( capture: ExplainCapture, + { output: outputOverride, explanations: explanationsOverride }: Pick, capturedAt: string, ): Promise { - const { output: outputOverride, explanations: explanationsOverride } = options - if (outputOverride !== undefined || explanationsOverride !== undefined) { const paths = authoringFiles(outputOverride, explanationsOverride) await writeCapture(paths, capture) - console.log( - `Captured ${capture.changes.length} change blocks across ${capture.files.length} files to ${paths.capture}`, - ) - console.log( - `Next: edit ${paths.explanations}, then run \`diffwalk check --input ${paths.capture} --explanations ${paths.explanations}\`.`, - ) - return + console.log(`Captured ${capture.changes.length} change blocks across ${capture.files.length} files to ${paths.capture} +Next: edit ${paths.explanations}, then run \`diffwalk check --input ${paths.capture} --explanations ${paths.explanations}\`.`) + } else { + await saveWalk(capture, capturedAt) } +} +async function saveWalk(capture: ExplainCapture, capturedAt: string): Promise { const previous = await currentWalkIfPresent() if (previous !== null) { const previousCapture = await readCapture(previous.capture) @@ -126,10 +139,8 @@ async function finishInspect( await writeText(previous.explanations, explanationsSkeleton(capture.captureId)) console.log(`Wrote a ${previous.explanations} skeleton to author`) } - console.log( - `${capture.source.kind === 'working-tree' ? 'Working tree' : 'Capture'} is unchanged; kept current walk ${previous.id}`, - ) - console.log(`Next: edit ${previous.explanations}, then run \`diffwalk check\`.`) + console.log(`${capture.source.kind === 'working-tree' ? 'Working tree' : 'Capture'} is unchanged; kept current walk ${previous.id} +Next: edit ${previous.explanations}, then run \`diffwalk check\`.`) return } } @@ -152,11 +163,9 @@ async function finishInspect( console.log(`Wrote a ${paths.explanations} skeleton to author`) } await setCurrentWalk(id) - console.log( - `Captured ${capture.changes.length} change blocks across ${capture.files.length} files to ${paths.capture}`, - ) - console.log(`Current walk: ${id}`) - console.log(`Next: edit ${paths.explanations}, then run \`diffwalk check\`.`) + console.log(`Captured ${capture.changes.length} change blocks across ${capture.files.length} files to ${paths.capture} +Current walk: ${id} +Next: edit ${paths.explanations}, then run \`diffwalk check\`.`) } function sourceIdentity(source: ExplainCapture['source']): string { diff --git a/src/cli/commands/publish.ts b/src/cli/commands/publish.ts index f01ae36..bd4e627 100644 --- a/src/cli/commands/publish.ts +++ b/src/cli/commands/publish.ts @@ -1,39 +1,52 @@ import { z } from 'zod' import { gitUserName } from '../../authoring/git' -import { authoringOptionsSchema, materialize } from '../../authoring/input' -import { readPublishedReview, writePublishedReview } from '../../authoring/published' -import { publishDocument, reportService, updateDocument, withPublisher } from '../../publish' -import type { ExplainDocument } from '../../format' +import { materialize } from '../input' +import { readPublishedReview, writePublishedReview, type PublishedReview } from '../published' +import { publishDocument, updateDocument, withPublisher } from '../../publish/client' +import type { ExplainDocument } from '../../format/types' +import { reportService } from '../service' import { UsageError } from '../usage' -export const publishOptionsSchema = authoringOptionsSchema.extend({ +const publishOptionsSchema = z.object({ + input: z.string().optional(), + explanations: z.string().optional(), service: z.string().optional(), - update: z.boolean().optional(), + update: z.boolean().default(false), }) -type PublishOptions = z.infer -export async function publishCommand(options: PublishOptions): Promise { - const { document, paths } = await materialize(options) +export async function publishReview(options: z.input): Promise { + const { input, explanations, service, update } = publishOptionsSchema.parse(options) + const { document, paths } = await materialize({ input, explanations }) const outgoing = withPublisher(document, await gitUserName()) - if (options.update === true) { - await updatePublishedReview(outgoing, paths.published, options.service) - return + switch (update) { + case true: + await updatePublishedReview(outgoing, paths.published, service) + break + case false: + await createPublishedReview(outgoing, paths.published, service) + break } +} - const service = reportService(options.service) - const published = await publishDocument(outgoing, service) +async function createPublishedReview( + document: ExplainDocument, + path: string, + serviceOption: string | undefined, +): Promise { + const service = reportService(serviceOption) + const published = await publishDocument(document, service) // The token is shown before the retention write so a failed write can never leave a // live review whose only credential was never surfaced. - console.log(`Published ${outgoing.sections.length} explanation sections to ${published.url}`) - console.log(`Revocation token: ${published.revocationToken}`) - await writePublishedReview(paths.published, { + console.log(`Published ${document.sections.length} explanation sections to ${published.url} +Revocation token: ${published.revocationToken}`) + await writePublishedReview(path, { id: published.id, url: published.url, service, revocationToken: published.revocationToken, }) console.log( - `Retained at ${paths.published} (keep it out of version control). Remove the review with \`diffwalk unpublish ${published.id} --token ${published.revocationToken}\`, or replace its content later with \`diffwalk publish --update\`.`, + `Retained at ${path} (keep it out of version control). Remove the review with \`diffwalk unpublish ${published.id} --token ${published.revocationToken}\`, or replace its content later with \`diffwalk publish --update\`.`, ) } @@ -43,22 +56,30 @@ async function updatePublishedReview( serviceOption: string | undefined, ): Promise { const retained = await readPublishedReview(path) + validateRetainedReview(retained) + + // Validate the stored service too, so a damaged retained file can never aim the + // revocation token at an arbitrary or plaintext host. + const service = reportService(serviceOption ?? retained.service) + validateUpdateService(service, retained.service) + + await updateDocument(document, retained.id, service, retained.revocationToken) + console.log(`Updated ${document.sections.length} explanation sections at ${retained.url} +Review ${retained.id} keeps its link and revocation token.`) +} + +function validateRetainedReview(retained: PublishedReview | null): asserts retained is PublishedReview { if (retained === null) { throw new UsageError( 'No published review is retained for this walk. Run `diffwalk publish` first.', ) } +} - // Validate the stored service too, so a damaged retained file can never aim the - // revocation token at an arbitrary or plaintext host. - const service = reportService(serviceOption ?? retained.service) - if (service !== retained.service) { +function validateUpdateService(service: string, retainedService: string): void { + if (service !== retainedService) { throw new UsageError( - `The retained review is hosted at ${retained.service}; update it there by passing only --update.`, + `The retained review is hosted at ${retainedService}; update it there by passing only --update.`, ) } - - await updateDocument(document, retained.id, service, retained.revocationToken) - console.log(`Updated ${document.sections.length} explanation sections at ${retained.url}`) - console.log(`Review ${retained.id} keeps its link and revocation token.`) } diff --git a/src/cli/commands/unpublish.ts b/src/cli/commands/unpublish.ts index 4be76ab..cbb018e 100644 --- a/src/cli/commands/unpublish.ts +++ b/src/cli/commands/unpublish.ts @@ -1,19 +1,23 @@ import { z } from 'zod' -import { reportService, unpublishDocument } from '../../publish' +import { unpublishDocument } from '../../publish/client' +import { reportService } from '../service' import { UsageError } from '../usage' -export const unpublishOptionsSchema = z.object({ +const unpublishOptionsSchema = z.object({ token: z.string().optional(), service: z.string().optional(), }) -type UnpublishOptions = z.infer -export async function unpublishCommand(id: string, options: UnpublishOptions): Promise { - const { token } = options +export async function removeReview(reportId: string, options: z.input): Promise { + const { token, service: serviceOption } = unpublishOptionsSchema.parse(options) + validateRevocationToken(token) + const service = reportService(serviceOption) + await unpublishDocument(reportId, service, token) + console.log(`Removed review ${reportId} from ${service}`) +} + +function validateRevocationToken(token: string | undefined): asserts token is string { if (token === undefined) { throw new UsageError('Pass the review\'s revocation token with --token') } - const service = reportService(options.service) - await unpublishDocument(id, service, token) - console.log(`Removed review ${id} from ${service}`) } diff --git a/src/cli/commands/use.ts b/src/cli/commands/use.ts index c4f48b5..2de2663 100644 --- a/src/cli/commands/use.ts +++ b/src/cli/commands/use.ts @@ -1,7 +1,11 @@ import { setCurrentWalk, walkExists } from '../../authoring/walk' -export async function useCommand(id: string): Promise { - if (!(await walkExists(id))) throw new Error(`No Diffwalk walk ${id}.`) - await setCurrentWalk(id) - console.log(`Current walk: ${id}`) +export async function selectWalk(walkId: string): Promise { + await validateWalkExists(walkId) + await setCurrentWalk(walkId) + console.log(`Current walk: ${walkId}`) +} + +async function validateWalkExists(walkId: string): Promise { + if (!(await walkExists(walkId))) throw new Error(`No Diffwalk walk ${walkId}.`) } diff --git a/src/cli/commands/view.ts b/src/cli/commands/view.ts index 00d7cd1..d7250d2 100644 --- a/src/cli/commands/view.ts +++ b/src/cli/commands/view.ts @@ -1,18 +1,25 @@ -import { materialize, type AuthoringOptions } from '../../authoring/input' +import { z } from 'zod' +import { materialize } from '../input' import { loadReportClient, renderReport } from '../../report' import { openBrowser, startReportPreview } from '../../report/view' -export async function viewCommand(options: AuthoringOptions): Promise { - const { document } = await materialize(options) +const viewOptionsSchema = z.object({ + input: z.string().optional(), + explanations: z.string().optional(), +}) + +export async function viewReview(options: z.input): Promise { + const { input, explanations } = viewOptionsSchema.parse(options) + const { document } = await materialize({ input, explanations }) const clientBundle = await loadReportClient() const html = renderReport(document, clientBundle) const preview = await startReportPreview(html) - console.log(`Viewing ${document.sections.length} sections at ${preview.url}`) - console.log('Press Ctrl+C to stop the local review.') + console.log(`Viewing ${document.sections.length} sections at ${preview.url} +Press Ctrl+C to stop the local review.`) try { await openBrowser(preview.url) } catch (error) { - console.log(`Could not open a browser automatically: ${(error as Error).message}`) - console.log(`Open ${preview.url} yourself.`) + console.log(`Could not open a browser automatically: ${(error as Error).message} +Open ${preview.url} yourself.`) } } diff --git a/src/cli/commands/walks.ts b/src/cli/commands/walks.ts index 2dc8891..7b0915b 100644 --- a/src/cli/commands/walks.ts +++ b/src/cli/commands/walks.ts @@ -1,12 +1,13 @@ import { currentWalkIdIfPresent, listWalkIds } from '../../authoring/walk' -export async function walksCommand(): Promise { - const ids = await listWalkIds() - if (ids.length === 0) { +export async function printWalks(): Promise { + const walkIds = await listWalkIds() + if (walkIds.length === 0) { console.log('No Diffwalk walks. Run `diffwalk inspect` first.') return } - const current = await currentWalkIdIfPresent() - console.log(`${ids.length} ${ids.length === 1 ? 'walk' : 'walks'}`) - for (const id of ids) console.log(`${id}${id === current ? ' (current)' : ''}`) + const currentWalkId = await currentWalkIdIfPresent() + const heading = `${walkIds.length} ${walkIds.length === 1 ? 'walk' : 'walks'}` + const lines = walkIds.map((walkId) => `${walkId}${walkId === currentWalkId ? ' (current)' : ''}`) + console.log([heading, ...lines].join('\n')) } diff --git a/src/authoring/config.ts b/src/cli/config.ts similarity index 95% rename from src/authoring/config.ts rename to src/cli/config.ts index f3c2f07..d36341c 100644 --- a/src/authoring/config.ts +++ b/src/cli/config.ts @@ -1,7 +1,7 @@ import { lstatSync, readFileSync } from 'node:fs' import { dirname, join, resolve } from 'node:path' import { z } from 'zod' -import { diffwalkDirectory } from './walk' +import { diffwalkDirectory } from '../authoring/walk' export const projectConfigFileName = 'config.json' @@ -12,16 +12,12 @@ export const projectConfigSchema = z service: z.string().optional(), }) .strict() -export type ProjectConfig = z.infer +export interface ProjectConfig { + service?: string +} -// The project is the Git work tree, so its config only sits at the work-tree root. A -// `.diffwalk/config.json` above the root belongs to some outer directory, not this -// project, and is ignored; outside a work tree there is no project config at all. -export function findProjectConfig(directory = process.cwd()): string | null { - const root = workTreeRoot(directory) - if (root === null) return null - const candidate = join(root, diffwalkDirectory, projectConfigFileName) - return entryExists(candidate) ? candidate : null +export function configuredService(directory = process.cwd()): string | undefined { + return readProjectConfig(directory)?.service } export function readProjectConfig(directory = process.cwd()): ProjectConfig | null { @@ -46,8 +42,14 @@ export function readProjectConfig(directory = process.cwd()): ProjectConfig | nu } } -export function configuredService(directory = process.cwd()): string | undefined { - return readProjectConfig(directory)?.service +// The project is the Git work tree, so its config only sits at the work-tree root. A +// `.diffwalk/config.json` above the root belongs to some outer directory, not this +// project, and is ignored; outside a work tree there is no project config at all. +export function findProjectConfig(directory = process.cwd()): string | null { + const root = workTreeRoot(directory) + if (root === null) return null + const candidate = join(root, diffwalkDirectory, projectConfigFileName) + return entryExists(candidate) ? candidate : null } function workTreeRoot(directory: string): string | null { diff --git a/src/authoring/explanations.ts b/src/cli/explanations.ts similarity index 79% rename from src/authoring/explanations.ts rename to src/cli/explanations.ts index 823da00..c106b5f 100644 --- a/src/authoring/explanations.ts +++ b/src/cli/explanations.ts @@ -1,16 +1,11 @@ import { parseDocument } from 'yaml' import { ZodError } from 'zod' -import { explanationsSchema, type Explanations } from '../format' +import { explanationsSchema } from '../format/schema' +import type { Explanations } from '../format/types' export function parseExplanations(text: string): Explanations { const document = parseDocument(text, { strict: true, schema: 'core' }) - const problems = [...document.errors, ...document.warnings] - if (problems.length > 0) { - throw new Error(`Invalid explanations YAML: ${problems[0]!.message}`) - } - if (document.contents === null) { - throw new Error('Invalid explanations YAML: the document is empty') - } + validateExplanationsYaml(document) const value: unknown = document.toJS({ maxAliasCount: 0 }) try { return explanationsSchema.parse(value) @@ -24,3 +19,13 @@ export function parseExplanations(text: string): Explanations { throw error } } + +function validateExplanationsYaml(document: ReturnType): void { + const problems = [...document.errors, ...document.warnings] + if (problems.length > 0) { + throw new Error(`Invalid explanations YAML: ${problems[0]!.message}`) + } + if (document.contents === null) { + throw new Error('Invalid explanations YAML: the document is empty') + } +} diff --git a/src/authoring/input.ts b/src/cli/input.ts similarity index 88% rename from src/authoring/input.ts rename to src/cli/input.ts index e22ba60..2c33389 100644 --- a/src/authoring/input.ts +++ b/src/cli/input.ts @@ -1,21 +1,19 @@ import { existsSync } from 'node:fs' import { mkdir, readFile, writeFile } from 'node:fs/promises' import { dirname, join, resolve } from 'node:path' -import { z } from 'zod' -import { materializeExplainDocument } from './capture' +import { materializeExplainDocument } from '../authoring/capture' import { parseExplanations } from './explanations' -import { captureSchema, type ExplainCapture, type ExplainDocument, type Explanations } from '../format' -import { currentWalk } from './walk' +import { captureSchema } from '../format/schema' +import type { ExplainCapture, ExplainDocument, Explanations } from '../format/types' +import { currentWalk } from '../authoring/walk' -export const captureOptionsSchema = z.object({ - input: z.string().optional(), -}) -export type CaptureOptions = z.infer +export interface CaptureOptions { + input?: string +} -export const authoringOptionsSchema = captureOptionsSchema.extend({ - explanations: z.string().optional(), -}) -export type AuthoringOptions = z.infer +export interface AuthoringOptions extends CaptureOptions { + explanations?: string +} export interface AuthoringFiles { directory: string @@ -84,30 +82,6 @@ export async function readCapture(path: string): Promise { return captureSchema.parse(JSON.parse(await readInput(path, 'capture.json', true))) } -async function readExplanations(path: string): Promise { - const text = await readInput(path, 'explanations.yaml', false) - try { - return parseExplanations(text) - } catch (error) { - throw new Error(`${(error as Error).message} (in ${path})`) - } -} - -async function readInput(path: string, label: string, isCapture: boolean): Promise { - const absolutePath = resolve(path) - try { - return await readFile(absolutePath, 'utf8') - } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') { - const hint = isCapture - ? `No capture at ${path}. Run \`diffwalk inspect\` first, or pass --input.` - : `No explanations at ${path}. Run \`diffwalk inspect\` first, or pass --explanations.` - throw new Error(hint) - } - throw new Error(`Could not read ${label}: ${(error as Error).message}`) - } -} - export function explanationsSkeleton(captureId: string): string { return `captureId: ${captureId} title: Name this change set @@ -132,3 +106,27 @@ export async function writeText(path: string, text: string): Promise { await mkdir(dirname(absolutePath), { recursive: true }) await writeFile(absolutePath, text) } + +async function readExplanations(path: string): Promise { + const text = await readInput(path, 'explanations.yaml', false) + try { + return parseExplanations(text) + } catch (error) { + throw new Error(`${(error as Error).message} (in ${path})`) + } +} + +async function readInput(path: string, label: string, isCapture: boolean): Promise { + const absolutePath = resolve(path) + try { + return await readFile(absolutePath, 'utf8') + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + const hint = isCapture + ? `No capture at ${path}. Run \`diffwalk inspect\` first, or pass --input.` + : `No explanations at ${path}. Run \`diffwalk inspect\` first, or pass --explanations.` + throw new Error(hint) + } + throw new Error(`Could not read ${label}: ${(error as Error).message}`) + } +} diff --git a/src/cli/options.ts b/src/cli/options.ts deleted file mode 100644 index f06cc0b..0000000 --- a/src/cli/options.ts +++ /dev/null @@ -1,17 +0,0 @@ -import { z } from 'zod' - -type ActionResult = void | Promise - -export function withOptions( - schema: TSchema, - action: (options: z.output) => ActionResult, -): (options: Record) => ActionResult { - return (options) => action(schema.parse(options)) -} - -export function withArgument( - schema: TSchema, - action: (argument: TArgument, options: z.output) => ActionResult, -): (argument: TArgument, options: Record) => ActionResult { - return (argument, options) => action(argument, schema.parse(options)) -} diff --git a/src/cli/output.ts b/src/cli/output.ts index b1e736d..804b39b 100644 --- a/src/cli/output.ts +++ b/src/cli/output.ts @@ -1,19 +1,19 @@ -export function coordinates(change: { +export function changeLine(change: { + id: string + path: string oldStart: number oldCount: number newStart: number newCount: number }): string { - return `old ${change.oldStart}:${change.oldCount} → new ${change.newStart}:${change.newCount} (+${change.newCount} −${change.oldCount})` + return `${change.id} ${change.path} ${coordinates(change)}` } -export function changeLine(change: { - id: string - path: string +export function coordinates(change: { oldStart: number oldCount: number newStart: number newCount: number }): string { - return `${change.id} ${change.path} ${coordinates(change)}` + return `old ${change.oldStart}:${change.oldCount} → new ${change.newStart}:${change.newCount} (+${change.newCount} −${change.oldCount})` } diff --git a/src/authoring/published.ts b/src/cli/published.ts similarity index 92% rename from src/authoring/published.ts rename to src/cli/published.ts index 220a4ae..ab49d80 100644 --- a/src/authoring/published.ts +++ b/src/cli/published.ts @@ -13,7 +13,12 @@ export const publishedReviewSchema = z revocationToken: z.string().min(1), }) .strict() -export type PublishedReview = z.infer +export interface PublishedReview { + id: string + url: string + service: string + revocationToken: string +} export async function readPublishedReview(path: string): Promise { let text: string diff --git a/src/cli/service.ts b/src/cli/service.ts new file mode 100644 index 0000000..97e5553 --- /dev/null +++ b/src/cli/service.ts @@ -0,0 +1,29 @@ +import { configuredService } from './config' + +const defaultService = 'https://review.diffwalk.dev' + +// New publications and revocations resolve the service in order: the explicit flag, the +// project config, then the hosted default. The environment is deliberately not consulted, +// so a stray DIFFWALK_SERVICE_URL cannot redirect a review or its token. `publish --update` +// passes the retained service as `explicit`, so a changed config can never redirect an +// existing review or send its token elsewhere. +export function reportService(explicit: string | undefined, directory = process.cwd()): string { + const value = explicit ?? configuredService(directory) ?? defaultService + const url = parseServiceUrl(value) + validateServiceProtocol(url, value) + return url.origin +} + +function parseServiceUrl(value: string): URL { + try { + return new URL(value) + } catch { + throw new Error(`Not a valid review service URL: ${value}`) + } +} + +function validateServiceProtocol(url: URL, value: string): void { + if (url.protocol !== 'https:' && url.hostname !== 'localhost' && url.hostname !== '127.0.0.1') { + throw new Error(`The review service must be reached over HTTPS: ${value}`) + } +} diff --git a/src/format.ts b/src/format/schema.ts similarity index 82% rename from src/format.ts rename to src/format/schema.ts index 59076f1..3067684 100644 --- a/src/format.ts +++ b/src/format/schema.ts @@ -1,17 +1,10 @@ import { z } from 'zod' +import type { ExplainCapture, ExplainDocument, Explanations } from './types' const gitModeSchema = z.enum(['000000', '100644', '100755']) export const draftFileSchema = z.preprocess( - (value) => { - if (typeof value !== 'object' || value === null || Array.isArray(value)) return value - const file = value as Record - return { - ...file, - oldMode: file.oldMode ?? (file.status === 'added' ? '000000' : '100644'), - newMode: file.newMode ?? (file.status === 'deleted' ? '000000' : '100644'), - } - }, + normalizeFileModes, z.object({ path: z.string().min(1), oldPath: z.string().min(1).optional(), @@ -92,7 +85,7 @@ export const captureSchema = z files: z.array(draftFileSchema), changes: z.array(changeBlockSchema), }) - .strict() + .strict() satisfies z.ZodType export const explanationStepSchema = z .object({ @@ -125,7 +118,7 @@ export const explanationsSchema = z metadata: explanationMetadataSchema.optional(), sections: z.array(explanationSectionSchema), }) - .strict() + .strict() satisfies z.ZodType export const documentStepSchema = z .object({ @@ -167,13 +160,14 @@ export const explainDocumentSchema = z ) .min(1), }) - .strict() + .strict() satisfies z.ZodType -export type DraftFile = z.infer -export type ChangeBlock = z.infer -export type CaptureSource = z.infer -export type ExplainCapture = z.infer -export type ExplanationStep = z.infer -export type Explanations = z.infer -export type DocumentStep = z.infer -export type ExplainDocument = z.infer +function normalizeFileModes(value: unknown): unknown { + if (typeof value !== 'object' || value === null || Array.isArray(value)) return value + const file = value as Record + return { + ...file, + oldMode: file.oldMode ?? (file.status === 'added' ? '000000' : '100644'), + newMode: file.newMode ?? (file.status === 'deleted' ? '000000' : '100644'), + } +} diff --git a/src/format/types.ts b/src/format/types.ts new file mode 100644 index 0000000..10fd700 --- /dev/null +++ b/src/format/types.ts @@ -0,0 +1,64 @@ +export interface DraftFile { + path: string + oldPath?: string + status: 'added' | 'modified' | 'deleted' | 'renamed' + oldMode: '000000' | '100644' | '100755' + newMode: '000000' | '100644' | '100755' + oldContent: string + newContent: string +} + +export interface ChangeBlock { + id: string + path: string + oldStart: number + oldCount: number + newStart: number + newCount: number + before: string + after: string +} + +interface CommitEndpoint { + revision: string + commit: string +} + +export type CaptureSource = + | { kind: 'working-tree'; capturedAt: string; from: CommitEndpoint } + | { kind: 'commit-diff'; capturedAt: string; from: CommitEndpoint; to: CommitEndpoint } + +export interface ExplainCapture { + captureId: string + source: CaptureSource + files: DraftFile[] + changes: ChangeBlock[] +} + +export interface ExplanationStep { + text: string + changes?: string[] +} + +export interface Explanations { + captureId: string + title: string + summary: string + metadata?: { explainedBy?: string } + sections: { title: string; steps: ExplanationStep[] }[] +} + +export interface DocumentStep { + text: string + diff?: string + changes?: string[] +} + +export interface ExplainDocument { + formatVersion: 1 + title: string + summary: string + source: CaptureSource | { kind: 'proposal'; capturedAt: string } + metadata?: { explainedBy?: string; publishedBy?: string; publishedAt?: string } + sections: { title: string; steps: DocumentStep[] }[] +} diff --git a/src/publish.ts b/src/publish/client.ts similarity index 61% rename from src/publish.ts rename to src/publish/client.ts index 2402bfa..435c088 100644 --- a/src/publish.ts +++ b/src/publish/client.ts @@ -1,5 +1,4 @@ -import { configuredService } from './authoring/config' -import type { ExplainDocument } from './format' +import type { ExplainDocument } from '../format/types' export interface PublishedReport { id: string @@ -7,37 +6,6 @@ export interface PublishedReport { revocationToken: string } -// The publisher is self-reported attribution, not a credential. It is read at publish time -// so the authoring files never carry it, and omitted cleanly when Git has no user name. -export function withPublisher( - document: ExplainDocument, - publishedBy: string | undefined, -): ExplainDocument { - if (publishedBy === undefined) return document - return { ...document, metadata: { ...document.metadata, publishedBy } } -} - -const defaultService = 'https://review.diffwalk.dev' - -// New publications and revocations resolve the service in order: the explicit flag, the -// project config, then the hosted default. The environment is deliberately not consulted, -// so a stray DIFFWALK_SERVICE_URL cannot redirect a review or its token. `publish --update` -// passes the retained service as `explicit`, so a changed config can never redirect an -// existing review or send its token elsewhere. -export function reportService(explicit: string | undefined, directory = process.cwd()): string { - const value = explicit ?? configuredService(directory) ?? defaultService - let url: URL - try { - url = new URL(value) - } catch { - throw new Error(`Not a valid review service URL: ${value}`) - } - if (url.protocol !== 'https:' && url.hostname !== 'localhost' && url.hostname !== '127.0.0.1') { - throw new Error(`The review service must be reached over HTTPS: ${value}`) - } - return url.origin -} - export async function publishDocument( document: ExplainDocument, service: string, @@ -47,14 +15,10 @@ export async function publishDocument( headers: { 'content-type': 'application/json' }, body: JSON.stringify(document), }) - if (!response.ok) { - throw new Error(`Could not publish the review: ${await failureDetail(response)}`) - } + await validateServiceResponse(response, 'publish') const value = (await response.json()) as { id?: unknown; revocationToken?: unknown } - if (typeof value.id !== 'string' || typeof value.revocationToken !== 'string') { - throw new Error('The review service returned a response this version does not understand') - } + validatePublishedReport(value) return { id: value.id, url: `${service}/r/${value.id}`, @@ -64,11 +28,11 @@ export async function publishDocument( export async function updateDocument( document: ExplainDocument, - id: string, + reportId: string, service: string, revocationToken: string, ): Promise { - const response = await fetch(`${service}/api/reports/${encodeURIComponent(id)}`, { + const response = await fetch(`${service}/api/reports/${encodeURIComponent(reportId)}`, { method: 'PUT', headers: { 'content-type': 'application/json', @@ -76,22 +40,42 @@ export async function updateDocument( }, body: JSON.stringify(document), }) - if (!response.ok) { - throw new Error(`Could not update the review: ${await failureDetail(response)}`) - } + await validateServiceResponse(response, 'update') } export async function unpublishDocument( - id: string, + reportId: string, service: string, revocationToken: string, ): Promise { - const response = await fetch(`${service}/api/reports/${encodeURIComponent(id)}`, { + const response = await fetch(`${service}/api/reports/${encodeURIComponent(reportId)}`, { method: 'DELETE', headers: { authorization: `Bearer ${revocationToken}` }, }) + await validateServiceResponse(response, 'remove') +} + +// The publisher is self-reported attribution, not a credential. It is read at publish time +// so the authoring files never carry it, and omitted cleanly when Git has no user name. +export function withPublisher( + document: ExplainDocument, + publishedBy: string | undefined, +): ExplainDocument { + if (publishedBy === undefined) return document + return { ...document, metadata: { ...document.metadata, publishedBy } } +} + +async function validateServiceResponse(response: Response, action: 'publish' | 'update' | 'remove'): Promise { if (!response.ok) { - throw new Error(`Could not remove the review: ${await failureDetail(response)}`) + throw new Error(`Could not ${action} the review: ${await failureDetail(response)}`) + } +} + +function validatePublishedReport( + value: { id?: unknown; revocationToken?: unknown }, +): asserts value is { id: string; revocationToken: string } { + if (typeof value.id !== 'string' || typeof value.revocationToken !== 'string') { + throw new Error('The review service returned a response this version does not understand') } } diff --git a/src/report/client.ts b/src/report/client.ts index cf5832a..159e1fe 100644 --- a/src/report/client.ts +++ b/src/report/client.ts @@ -21,6 +21,29 @@ type FileDiffFactory = (options: FileDiffOptions) => FileDiff const initialRenderTimeoutMs = 10_000 +export function mountReport( + createFileDiff: FileDiffFactory = (options) => new FileDiff(options), +) { + reserveGeneratedIds() + const data = readReportData() + if (!data) return + let layout = initialLayout() + if (isNarrowViewport() && layout === 'split') { + layout = 'unified' + reflectLayout(layout) + } + let finishInitialRender!: () => void + const initialRender = new Promise((resolve) => (finishInitialRender = resolve)) + wireSectionFolds() + wireFragments(initialRender) + const mountedDiffs = mountDiffs(data, layout, createFileDiff) + void mountedDiffs.initialRender.then(finishInitialRender) + const { mounted } = mountedDiffs + wireLayout(mounted) + wireGlobalFolds() + prepareForPrint() +} + function readReportData(): ReportData | null { const element = document.querySelector( 'body > script#diffwalk-report-data[type="application/json"]', @@ -369,28 +392,6 @@ function prepareForPrint() { }) } -export function mountReport( - createFileDiff: FileDiffFactory = (options) => new FileDiff(options), -) { - reserveGeneratedIds() - const data = readReportData() - if (!data) return - let layout = initialLayout() - if (isNarrowViewport() && layout === 'split') { - layout = 'unified' - reflectLayout(layout) - } - let finishInitialRender!: () => void - const initialRender = new Promise((resolve) => (finishInitialRender = resolve)) - wireSectionFolds() - wireFragments(initialRender) - const mountedDiffs = mountDiffs(data, layout, createFileDiff) - void mountedDiffs.initialRender.then(finishInitialRender) - const { mounted } = mountedDiffs - wireLayout(mounted) - wireGlobalFolds() - prepareForPrint() -} if (typeof document !== 'undefined') { mountReport() diff --git a/src/report.ts b/src/report/index.ts similarity index 84% rename from src/report.ts rename to src/report/index.ts index 5d130e2..761fd79 100644 --- a/src/report.ts +++ b/src/report/index.ts @@ -1,10 +1,9 @@ import { mkdir, readFile, rename, unlink, writeFile } from 'node:fs/promises' import { randomUUID } from 'node:crypto' import { basename, dirname, join, resolve } from 'node:path' -import { fileURLToPath } from 'node:url' -export { renderHostedReport, renderReport, shellStyles } from './report/render' -export type { HostedAssets, ReportLayout, ReportOptions } from './report/render' +export { renderHostedReport, renderReport, shellStyles } from './render' +export type { HostedAssets, ReportLayout, ReportOptions } from './render' export async function writeReport(output: string, html: string): Promise { const absolutePath = resolve(output) @@ -30,8 +29,11 @@ export function loadReportClient(): Promise { } async function loadReportClientUncached(): Promise { - const root = resolve(dirname(fileURLToPath(import.meta.url)), '..') - const prebuilt = join(root, 'dist', 'report-client.js') + // Source runs from src/report; the bundled CLI runs beside report-client.js. + const prebuilt = new URL( + import.meta.url.endsWith('.ts') ? '../../dist/report-client.js' : './report-client.js', + import.meta.url, + ) try { return await readFile(prebuilt, 'utf8') } catch (error) { diff --git a/src/report/markdown.ts b/src/report/markdown.ts index ad1091c..6e342e1 100644 --- a/src/report/markdown.ts +++ b/src/report/markdown.ts @@ -1,5 +1,9 @@ import { Marked, type RendererObject, type Tokens } from 'marked' +export function renderMarkdown(markdown: string): string { + return parser.parse(markdown, { async: false }) +} + const renderer: RendererObject = { // Authored text is trusted, so inline HTML passes through: that is how a diagram // reaches the page. The report origin's Content Security Policy is what contains it. @@ -21,10 +25,6 @@ const renderer: RendererObject = { const parser = new Marked({ gfm: true, async: false, renderer }) -export function renderMarkdown(markdown: string): string { - return parser.parse(markdown, { async: false }) -} - function isSafeLinkHref(href: string): boolean { const scheme = /^[a-z][a-z0-9+.-]*:/i.exec(href)?.[0]?.toLowerCase() return ( diff --git a/src/report/patches.ts b/src/report/patches.ts index 10963c2..948b1f6 100644 --- a/src/report/patches.ts +++ b/src/report/patches.ts @@ -9,9 +9,7 @@ export interface FileDiffStats { export function parseSectionPatch(patch: string): FileDiffMetadata[] { const parsed = parsePatchFiles(patch, undefined, true) const files = parsed.flatMap((result) => result.files) - if (files.length === 0) { - throw new Error('The section patch contains no parseable file diffs') - } + validatePatchFiles(files) const structured = parsePatch(patch) for (const [index, file] of files.entries()) { const source = structured[index] @@ -39,3 +37,9 @@ export function fileDiffLabel(file: FileDiffMetadata): string { ? `${file.prevName} → ${file.name}` : file.name } + +function validatePatchFiles(files: FileDiffMetadata[]): void { + if (files.length === 0) { + throw new Error('The section patch contains no parseable file diffs') + } +} diff --git a/src/report/render.ts b/src/report/render.ts index 8559a8e..308ad1f 100644 --- a/src/report/render.ts +++ b/src/report/render.ts @@ -1,4 +1,4 @@ -import type { ExplainDocument } from '../format' +import type { ExplainDocument } from '../format/types' import { faviconDataUrl } from './favicon' import { renderMarkdown } from './markdown' import { fileDiffLabel, fileDiffStats, parseSectionPatch } from './patches' @@ -34,6 +34,30 @@ interface ReportBody { data: ReportData } +export function renderReport( + document: ExplainDocument, + clientBundle: string, + options: ReportOptions = {}, +): string { + return renderShell( + renderReportBody(document, options), + ``, + ``, + ) +} + +export function renderHostedReport( + document: ExplainDocument, + assets: HostedAssets, + options: ReportOptions = {}, +): string { + return renderShell( + renderReportBody(document, options, true), + ``, + ``, + ) +} + function renderReportBody( document: ExplainDocument, options: ReportOptions = {}, @@ -90,30 +114,6 @@ ${sections.map((section) => section.markup).join('\n')} } } -export function renderReport( - document: ExplainDocument, - clientBundle: string, - options: ReportOptions = {}, -): string { - return renderShell( - renderReportBody(document, options), - ``, - ``, - ) -} - -export function renderHostedReport( - document: ExplainDocument, - assets: HostedAssets, - options: ReportOptions = {}, -): string { - return renderShell( - renderReportBody(document, options, true), - ``, - ``, - ) -} - function renderShell(body: ReportBody, styles: string, client: string): string { return ` @@ -163,13 +163,7 @@ function renderSection( return `
${actions}${textMarkup}
` } - let files: FileDiffMetadata[] - try { - files = parseSectionPatch(step.diff) - } catch (error) { - const detail = error instanceof Error ? error.message : String(error) - throw new Error(`Section "${section.title}" has an unparseable diff: ${detail}`) - } + const files = parseStepDiff(step.diff, section.title) fileCount += files.length diffs.push({ section: index, step: stepIndex, diff: step.diff }) @@ -204,6 +198,15 @@ ${steps.join('\n')} return { markup, fileCount, diffs } } +function parseStepDiff(diff: string, sectionTitle: string): FileDiffMetadata[] { + try { + return parseSectionPatch(diff) + } catch (error) { + const detail = error instanceof Error ? error.message : String(error) + throw new Error(`Section "${sectionTitle}" has an unparseable diff: ${detail}`) + } +} + function renderReviewMap( sections: { title: string; fragment: string }[], counts: { sections: number; files: number }, @@ -263,18 +266,19 @@ function renderAttribution(metadata: ExplainDocument['metadata'], hosted: boolea } function renderSourceMetadata(source: ExplainDocument['source']): string { - if (source.kind === 'commit-diff') { - return `
From
${renderEndpoint(source.from)}
+ switch (source.kind) { + case 'commit-diff': + return `
From
${renderEndpoint(source.from)}
To
${renderEndpoint(source.to)}
Captured at
${escapeHtml(source.capturedAt)}
` - } - if (source.kind === 'working-tree') { - return `
From
${renderEndpoint(source.from)}
+ case 'working-tree': + return `
From
${renderEndpoint(source.from)}
To
Working tree
Captured at
${escapeHtml(source.capturedAt)}
` - } - return `
Source
Proposal
+ case 'proposal': + return `
Source
Proposal
Captured at
${escapeHtml(source.capturedAt)}
` + } } function renderEndpoint(endpoint: { revision: string; commit: string }): string { diff --git a/src/report/targets.ts b/src/report/targets.ts index 085a03c..9a05182 100644 --- a/src/report/targets.ts +++ b/src/report/targets.ts @@ -1,4 +1,4 @@ -import type { ExplainDocument } from '../format' +import type { ExplainDocument } from '../format/types' export interface ReportChangeTarget { id: string @@ -85,6 +85,12 @@ export function reportTargets(document: ExplainDocument): ReportSectionTarget[] } } + assignCanonicalChanges(targets, changeIds) + + return targets +} + +function assignCanonicalChanges(targets: ReportSectionTarget[], changeIds: string[]): void { for (const id of changeIds) { const occurrences = targets .flatMap((section) => @@ -98,7 +104,6 @@ export function reportTargets(document: ExplainDocument): ReportSectionTarget[] occurrences[0]!.change.canonical = true } - return targets } function stepFingerprint(step: ExplainDocument['sections'][number]['steps'][number]): string { diff --git a/src/report/view.ts b/src/report/view.ts index 7b5deac..8867b66 100644 --- a/src/report/view.ts +++ b/src/report/view.ts @@ -1,5 +1,5 @@ import { spawn } from 'node:child_process' -import { createServer } from 'node:http' +import { createServer, type IncomingMessage, type ServerResponse } from 'node:http' import type { AddressInfo } from 'node:net' export interface ReportPreview { @@ -8,26 +8,7 @@ export interface ReportPreview { } export async function startReportPreview(html: string): Promise { - const server = createServer((request, response) => { - if (request.method !== 'GET') { - response.writeHead(405, { Allow: 'GET' }).end() - return - } - const path = new URL(request.url ?? '/', 'http://localhost').pathname - if (path === '/favicon.ico') { - response.writeHead(204).end() - return - } - if (path !== '/') { - response.writeHead(404).end('Not found') - return - } - response.writeHead(200, { - 'Cache-Control': 'no-store', - 'Content-Type': 'text/html; charset=utf-8', - }) - response.end(html) - }) + const server = createServer((request, response) => serveReport(request, response, html)) await new Promise((accept, reject) => { server.once('error', reject) @@ -44,12 +25,7 @@ export async function startReportPreview(html: string): Promise { } export async function openBrowser(url: string): Promise { - const command = - process.platform === 'darwin' - ? { file: 'open', args: [url] } - : process.platform === 'win32' - ? { file: 'cmd', args: ['/d', '/s', '/c', 'start', '', url] } - : { file: 'xdg-open', args: [url] } + const command = browserCommand(url) const child = spawn(command.file, command.args, { detached: true, stdio: 'ignore' }) await new Promise((accept, reject) => { child.once('error', reject) @@ -57,3 +33,36 @@ export async function openBrowser(url: string): Promise { }) child.unref() } + +function serveReport(request: IncomingMessage, response: ServerResponse, html: string): void { + if (request.method !== 'GET') { + response.writeHead(405, { Allow: 'GET' }).end() + return + } + const path = new URL(request.url ?? '/', 'http://localhost').pathname + switch (path) { + case '/': + response.writeHead(200, { + 'Cache-Control': 'no-store', + 'Content-Type': 'text/html; charset=utf-8', + }) + response.end(html) + break + case '/favicon.ico': + response.writeHead(204).end() + break + default: + response.writeHead(404).end('Not found') + } +} + +function browserCommand(url: string): { file: string; args: string[] } { + switch (process.platform) { + case 'darwin': + return { file: 'open', args: [url] } + case 'win32': + return { file: 'cmd', args: ['/d', '/s', '/c', 'start', '', url] } + default: + return { file: 'xdg-open', args: [url] } + } +} diff --git a/test/authoring.test.ts b/test/authoring.test.ts index 78b3916..2c350e1 100644 --- a/test/authoring.test.ts +++ b/test/authoring.test.ts @@ -5,7 +5,7 @@ import { duplicatedChangeIds, materializeExplainDocument, } from '../src/authoring/capture' -import type { CaptureSource, ExplainCapture } from '../src/format' +import type { CaptureSource, ExplainCapture } from '../src/format/types' import { fileDiffStats, parseSectionPatch } from '../src/report/patches' const source: CaptureSource = { diff --git a/test/cli.test.ts b/test/cli.test.ts index 31da69c..32b0b52 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -4,7 +4,7 @@ import { chmod, mkdir, mkdtemp, readFile, readdir, rm, writeFile } from 'node:fs import { tmpdir } from 'node:os' import { join } from 'node:path' import { captureIdFor } from '../src/authoring/capture' -import { captureSchema } from '../src/format' +import { captureSchema } from '../src/format/schema' const directories: string[] = [] diff --git a/test/config.test.ts b/test/config.test.ts index c957cc2..d47b406 100644 --- a/test/config.test.ts +++ b/test/config.test.ts @@ -6,7 +6,7 @@ import { configuredService, findProjectConfig, readProjectConfig, -} from '../src/authoring/config' +} from '../src/cli/config' const directories: string[] = [] diff --git a/test/explanations.test.ts b/test/explanations.test.ts index ab76ad2..5665aac 100644 --- a/test/explanations.test.ts +++ b/test/explanations.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from 'bun:test' -import { parseExplanations } from '../src/authoring/explanations' +import { parseExplanations } from '../src/cli/explanations' const captureId = 'a'.repeat(64) const head = `captureId: ${captureId}\ntitle: A change set\n` diff --git a/test/format.test.ts b/test/format.test.ts index 2056ab6..d961c73 100644 --- a/test/format.test.ts +++ b/test/format.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from 'bun:test' -import { captureSchema, explainDocumentSchema, explanationsSchema } from '../src/format' +import { captureSchema, explainDocumentSchema, explanationsSchema } from '../src/format/schema' function diff(patchText = 'diff --git a/a.ts b/a.ts\n--- a/a.ts\n+++ b/a.ts\n@@ -1 +1 @@\n-old\n+new\n') { return patchText diff --git a/test/publish.test.ts b/test/publish.test.ts index 4c3c5ba..2d95df8 100644 --- a/test/publish.test.ts +++ b/test/publish.test.ts @@ -2,14 +2,14 @@ import { afterEach, describe, expect, test } from 'bun:test' import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' -import type { ExplainDocument } from '../src/format' +import type { ExplainDocument } from '../src/format/types' import { publishDocument, - reportService, unpublishDocument, updateDocument, withPublisher, -} from '../src/publish' +} from '../src/publish/client' +import { reportService } from '../src/cli/service' const originalFetch = globalThis.fetch const originalEnvironment = { ...process.env } diff --git a/test/published.test.ts b/test/published.test.ts index 9c2789c..df94874 100644 --- a/test/published.test.ts +++ b/test/published.test.ts @@ -2,7 +2,7 @@ import { afterEach, describe, expect, test } from 'bun:test' import { mkdtemp, readFile, rm, stat, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' -import { readPublishedReview, writePublishedReview } from '../src/authoring/published' +import { readPublishedReview, writePublishedReview } from '../src/cli/published' const directories: string[] = [] diff --git a/test/report-dom.test.ts b/test/report-dom.test.ts index b320dc3..3285707 100644 --- a/test/report-dom.test.ts +++ b/test/report-dom.test.ts @@ -1,7 +1,7 @@ import { afterEach, describe, expect, test } from 'bun:test' import type { FileDiff } from '@pierre/diffs' import { Window } from 'happy-dom' -import type { ExplainDocument } from '../src/format' +import type { ExplainDocument } from '../src/format/types' import { loadReportClient, renderReport } from '../src/report' import { mountReport } from '../src/report/client' import { reportTargets } from '../src/report/targets' diff --git a/test/report-targets.test.ts b/test/report-targets.test.ts index 3adb917..48fc2a9 100644 --- a/test/report-targets.test.ts +++ b/test/report-targets.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from 'bun:test' -import type { ExplainDocument } from '../src/format' +import type { ExplainDocument } from '../src/format/types' import { reportTargets } from '../src/report/targets' function document(sections: ExplainDocument['sections']): ExplainDocument { diff --git a/test/report.test.ts b/test/report.test.ts index 20692e4..f90732c 100644 --- a/test/report.test.ts +++ b/test/report.test.ts @@ -2,7 +2,7 @@ import { afterEach, describe, expect, test } from 'bun:test' import { mkdir, mkdtemp, readFile, readdir, rm } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' -import type { ExplainDocument } from '../src/format' +import type { ExplainDocument } from '../src/format/types' import { renderMarkdown } from '../src/report/markdown' import { fileDiffStats, parseSectionPatch } from '../src/report/patches' import { loadReportClient, renderHostedReport, renderReport, writeReport } from '../src/report' diff --git a/test/visual/fixtures.ts b/test/visual/fixtures.ts index 53ab9c8..bd39eac 100644 --- a/test/visual/fixtures.ts +++ b/test/visual/fixtures.ts @@ -1,4 +1,4 @@ -import type { ExplainDocument } from '../../src/format' +import type { ExplainDocument } from '../../src/format/types' export function simplePatch(oldLine = 'old', newLine = 'new'): string { return [ diff --git a/website/public/index.html b/website/public/index.html index 5ef7010..ec68c28 100644 --- a/website/public/index.html +++ b/website/public/index.html @@ -200,16 +200,22 @@

Follow the explanation

diffwalk

Review code in human order.

diff --git a/worker/build-assets.ts b/worker/build-assets.ts index 5bc07fd..2661815 100644 --- a/worker/build-assets.ts +++ b/worker/build-assets.ts @@ -8,14 +8,19 @@ await mkdir(publicDirectory, { recursive: true }) await writeFile(join(publicDirectory, 'report.css'), shellStyles.trimStart()) -const built = await Bun.build({ - entrypoints: [join(root, 'src', 'report', 'client.ts')], - target: 'browser', - format: 'iife', - minify: true, -}) -const bundle = built.outputs[0] -if (!bundle) throw new Error('The report client bundle produced no output') -await writeFile(join(publicDirectory, 'report-client.js'), await bundle.text()) +const clientBundle = await buildReportClient() +await writeFile(join(publicDirectory, 'report-client.js'), clientBundle) console.log(`Wrote report.css and report-client.js to ${publicDirectory}`) + +async function buildReportClient(): Promise { + const build = await Bun.build({ + entrypoints: [join(root, 'src', 'report', 'client.ts')], + target: 'browser', + format: 'iife', + minify: true, + }) + const bundle = build.outputs[0] + if (!bundle) throw new Error('The report client bundle produced no output') + return bundle.text() +} diff --git a/worker/index.test.ts b/worker/index.test.ts index 92d6aed..03a9115 100644 --- a/worker/index.test.ts +++ b/worker/index.test.ts @@ -1,5 +1,5 @@ import { beforeEach, describe, expect, test } from 'bun:test' -import type { ExplainDocument } from '../src/format' +import type { ExplainDocument } from '../src/format/types' import worker, { type Env } from './index' interface StoredObject { diff --git a/worker/index.ts b/worker/index.ts index d69758f..33e72cf 100644 --- a/worker/index.ts +++ b/worker/index.ts @@ -1,4 +1,5 @@ -import { explainDocumentSchema, type ExplainDocument } from '../src/format' +import { explainDocumentSchema } from '../src/format/schema' +import type { ExplainDocument } from '../src/format/types' import { faviconSvg } from '../src/report/favicon' import { renderHostedReport } from '../src/report/render' import { @@ -40,16 +41,7 @@ export default { async fetch(request: Request, env: Env): Promise { const path = new URL(request.url).pathname - if (path === '/favicon.svg') { - if (request.method !== 'GET' && request.method !== 'HEAD') return methodNotAllowed('GET, HEAD') - return new Response(request.method === 'HEAD' ? null : faviconSvg, { - headers: { - 'content-type': 'image/svg+xml', - 'cache-control': 'public, max-age=86400', - 'x-content-type-options': 'nosniff', - }, - }) - } + if (path === '/favicon.svg') return showFavicon(request) if (path === '/api/reports') { if (request.method !== 'POST') return methodNotAllowed('POST') @@ -57,13 +49,7 @@ export default { } const apiReport = /^\/api\/reports\/([^/]+)$/.exec(path) - if (apiReport) { - const id = apiReport[1]! - if (request.method === 'GET') return readReport(id, env) - if (request.method === 'PUT') return updateReport(id, request, env) - if (request.method === 'DELETE') return revokeReport(id, request, env) - return methodNotAllowed('GET, PUT, DELETE') - } + if (apiReport) return handleReportRequest(apiReport[1]!, request, env) const reader = /^\/r\/([^/]+)$/.exec(path) if (reader) { @@ -77,35 +63,50 @@ export default { }, } +function showFavicon(request: Request): Response { + if (request.method !== 'GET' && request.method !== 'HEAD') return methodNotAllowed('GET, HEAD') + return new Response(request.method === 'HEAD' ? null : faviconSvg, { + headers: { + 'content-type': 'image/svg+xml', + 'cache-control': 'public, max-age=86400', + 'x-content-type-options': 'nosniff', + }, + }) +} + +function handleReportRequest(reportId: string, request: Request, env: Env): Promise | Response { + switch (request.method) { + case 'GET': + return readReport(reportId, env) + case 'PUT': + return updateReport(reportId, request, env) + case 'DELETE': + return revokeReport(reportId, request, env) + default: + return methodNotAllowed('GET, PUT, DELETE') + } +} + async function publishReport(request: Request, env: Env): Promise { const parsed = await readReportDocument(request) if (!parsed.ok) return parsed.response const document = withPublishedAt(parsed.document) - const id = createReportId() + const reportId = createReportId() const revocationToken = createRevocationToken() - await env.REPORTS.put(reportKey(id), JSON.stringify(document), { + await env.REPORTS.put(reportKey(reportId), JSON.stringify(document), { httpMetadata: { contentType: 'application/json' }, customMetadata: { revocation: await hashToken(revocationToken) }, }) // The link is built by the caller, which knows the origin it reached. Deriving it here would // mean trusting the request's Host header. - return json(201, { id, revocationToken }) + return json(201, { id: reportId, revocationToken }) } -async function updateReport(id: string, request: Request, env: Env): Promise { - if (!isReportId(id)) return problem(404, 'No such report') - const token = bearerToken(request) - if (token === null) return problem(401, 'A revocation credential is required') - - const object = await env.REPORTS.head(reportKey(id)) - if (!object) return problem(404, 'No such report') - - const expected = object.customMetadata?.['revocation'] - if (expected === undefined || !secretsMatch(await hashToken(token), expected)) { - return problem(403, 'That credential does not update this report') - } +async function updateReport(reportId: string, request: Request, env: Env): Promise { + const authorization = await authorizeReport(reportId, request, env, 'update') + if (!authorization.ok) return authorization.response const parsed = await readReportDocument(request) if (!parsed.ok) return parsed.response @@ -113,11 +114,11 @@ async function updateReport(id: string, request: Request, env: Env): Promise { - if (!isReportId(id)) return problem(404, 'No such report') - const object = await env.REPORTS.get(reportKey(id)) +async function readReport(reportId: string, env: Env): Promise { + if (!isReportId(reportId)) return problem(404, 'No such report') + const object = await env.REPORTS.get(reportKey(reportId)) if (!object) return problem(404, 'No such report') return new Response(object.body, { headers: { @@ -178,29 +179,46 @@ async function readReport(id: string, env: Env): Promise { }) } -async function revokeReport(id: string, request: Request, env: Env): Promise { - if (!isReportId(id)) return problem(404, 'No such report') - const token = bearerToken(request) - if (token === null) return problem(401, 'A revocation credential is required') +async function revokeReport(reportId: string, request: Request, env: Env): Promise { + const authorization = await authorizeReport(reportId, request, env, 'revoke') + if (!authorization.ok) return authorization.response - const object = await env.REPORTS.head(reportKey(id)) - if (!object) return problem(404, 'No such report') + await env.REPORTS.delete(reportKey(reportId)) + return new Response(null, { status: 204, headers: { 'cache-control': 'no-store' } }) +} + +type ReportAuthorization = + | { ok: true; revocationHash: string } + | { ok: false; response: Response } - const expected = object.customMetadata?.['revocation'] - if (expected === undefined || !secretsMatch(await hashToken(token), expected)) { - return problem(403, 'That credential does not revoke this report') +async function authorizeReport( + reportId: string, + request: Request, + env: Env, + operation: 'update' | 'revoke', +): Promise { + if (!isReportId(reportId)) return { ok: false, response: problem(404, 'No such report') } + const token = bearerToken(request) + if (token === null) { + return { ok: false, response: problem(401, 'A revocation credential is required') } } - await env.REPORTS.delete(reportKey(id)) - return new Response(null, { status: 204, headers: { 'cache-control': 'no-store' } }) + const object = await env.REPORTS.head(reportKey(reportId)) + if (!object) return { ok: false, response: problem(404, 'No such report') } + + const revocationHash = object.customMetadata?.['revocation'] + if (revocationHash === undefined || !secretsMatch(await hashToken(token), revocationHash)) { + return { ok: false, response: problem(403, `That credential does not ${operation} this report`) } + } + return { ok: true, revocationHash } } -async function showReport(id: string, env: Env): Promise { - if (!isReportId(id)) return errorPage(404, 'No such report', 'This link does not name a report.') +async function showReport(reportId: string, env: Env): Promise { + if (!isReportId(reportId)) return errorPage(404, 'No such report', 'This link does not name a report.') let object: R2ObjectBody | null try { - object = await env.REPORTS.get(reportKey(id)) + object = await env.REPORTS.get(reportKey(reportId)) } catch { return errorPage( 503, diff --git a/worker/reports.ts b/worker/reports.ts index 5b4c5ad..4d07ca7 100644 --- a/worker/reports.ts +++ b/worker/reports.ts @@ -1,13 +1,13 @@ export const maxDocumentBytes = 1024 * 1024 -const idPattern = /^[A-Za-z0-9_-]{22}$/ +const reportIdPattern = /^[A-Za-z0-9_-]{22}$/ export function isReportId(value: string): boolean { - return idPattern.test(value) + return reportIdPattern.test(value) } -export function reportKey(id: string): string { - return `reports/${id}.json` +export function reportKey(reportId: string): string { + return `reports/${reportId}.json` } export function createReportId(): string {