From 65fb5c7bb0eafd626a6ea8923d57cf904bb42b55 Mon Sep 17 00:00:00 2001 From: Adrian Elton-Browning Date: Tue, 6 Oct 2026 14:51:05 +0100 Subject: [PATCH 1/2] fix(extract): check every document before writing, including targets they share (#43) --- .bumpy/cross-document-targets.md | 5 + CLAUDE.md | 2 +- packages/mdcode/README.md | 17 +++- packages/mdcode/src/cli.ts | 112 ++++++++++++++++------- packages/mdcode/src/commands/validate.ts | 88 ++++++++++++++++-- packages/usage/tests/validate.test.ts | 100 +++++++++++++++++++- 6 files changed, 274 insertions(+), 50 deletions(-) create mode 100644 .bumpy/cross-document-targets.md diff --git a/.bumpy/cross-document-targets.md b/.bumpy/cross-document-targets.md new file mode 100644 index 0000000..5aa0788 --- /dev/null +++ b/.bumpy/cross-document-targets.md @@ -0,0 +1,5 @@ +--- +mdcode-ts: minor +--- + +`extract` with several documents now checks all of them before writing anything. Blocks in different documents that write one file follow the same rule as blocks within one document: each needs its own `region=`, in one language. Otherwise they are refused as `ambiguous_target`, each error naming its document, and nothing is written for any document. Previously the first document's version won and the second was skipped, depending on document order. A rule broken in a later document no longer leaves the earlier documents' files written. `validate --for extract` reports the same cross-document conflicts. diff --git a/CLAUDE.md b/CLAUDE.md index 736b803..0e1db92 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -18,7 +18,7 @@ TypeScript port of [szkiba/mdcode](https://github.com/szkiba/mdcode): keeps Mark ## Where things live - **Parser** (`src/parser.ts`): a custom line-by-line state machine instead of remark, so in-place updates keep exact character offsets. `scanFences()` is shared by `parse()` and `updateInfoStrings()` so block indices always agree. -- **Mapping rules** (`src/commands/validate.ts`): `extract()` runs `planExtract()` before writing anything, and `update()` reads every `file=` through `readSource()`. `mdcode validate` reports the same rules without writing. +- **Mapping rules** (`src/commands/validate.ts`): `extract()` runs `planExtract()` before writing anything, and `update()` reads every `file=` through `readSource()`. `mdcode validate` reports the same rules without writing. With several documents, the CLI's `validateDocuments()` validates them all, plus `sharedTargetErrors()` across them, before `extract` writes any. - **Writes**: `update()` never writes; the CLI decides, and only `--apply` writes the markdown. `watch()` writes only with `apply`. - **Config** (`src/config.ts`): `mdcode.config.json` is loaded only with `--project` or `--config`. Without either, input defaults to stdin. - **Contract** (`src/result.ts`): `COMMAND_NAMES` and `ERROR_CODES` are the source of truth for commands and error codes. diff --git a/packages/mdcode/README.md b/packages/mdcode/README.md index 692ca94..b5a1f43 100644 --- a/packages/mdcode/README.md +++ b/packages/mdcode/README.md @@ -331,7 +331,10 @@ exits 1 with an error per block: are in the same language. Two whole-file blocks for one file, even identical ones, a whole-file block beside a region block, a repeated `region=`, or regions in different languages are refused as `ambiguous_target`. Two spellings of one file (`./a.ts` and `a.ts`, or a symlinked directory - inside `--dir`) count as one file. + inside `--dir`) count as one file. With several documents, the same rule applies to blocks in + different documents that write one file, so `docs/a.md` and `docs/b.md` cannot both write + `src/x.ts` whole, nor both write region `one` of it. Anonymous blocks count too: `block-1.sh` from + two documents is one file. - **An existing file's markers for a declared region are broken** → a region that is never closed or overlaps another is `malformed_region`, one opened more than once is `duplicate_region`, and one marked only in another language's comment syntax is `region_language_mismatch`. An invalid @@ -770,7 +773,7 @@ block's line and name, the file concerned and the rule, which is the error code: | `duplicate_region` | `region=` is opened more than once in the file | Same, in an existing target | | `malformed_region` | invalid `region=` name, or its markers are unclosed or do not nest | Same, or two declared regions overlap | | `region_language_mismatch` | `region=` is marked only in another language's comment syntax | Same, in an existing target | -| `ambiguous_target` | - | Several blocks write one file, but not each with its own `region=` in one language | +| `ambiguous_target` | - | Several blocks write one file, but not each with its own `region=` in one language; with several documents, this includes blocks in different documents | | `missing_file_metadata` | `--strict`: a selected block has no `file=` | Same | `update` and `extract` enforce every rule here except `--strict`, which only `validate` applies, so a @@ -1275,7 +1278,10 @@ output starts with the document it is about. `--stdout` needs exactly one docume `update --apply` writes every document or none of them: when one fails, nothing is written. With `--continue-on-error`, `update` carries on past a failed document and applies the others. `extract` -writes as it goes, so it stops at the first document that fails, after extracting the ones before it. +writes nothing until every document has been checked: when any document breaks a rule, or two +documents write one file in a way that rule refuses, nothing is written for any of them. It still stops +at a document that fails while writing, after extracting the ones before it. `validate --for extract` +runs the same check across its documents. Under `--json`, `result.documents` holds one entry per document, and each error names its `document`; see [JSON Contract](#json-contract). @@ -2055,6 +2061,11 @@ Exit codes are the same with and without `--json`: refused as `ambiguous_target`, `malformed_region`, `duplicate_region` or `region_language_mismatch` and nothing is written. They used to skip that one file with `extract_skipped` and exit 2. Two identical whole-file blocks for one file used to be written once; they are now refused too. +- `extract` with several documents checks them all before writing any, including blocks in different + documents that write one file. It used to write each document in turn, so a later document's + refusal left the earlier ones' files written, and a file two documents both wrote ended up with + whichever came first, the second being skipped with `extract_skipped`. Both are now refused before + anything is written, and `validate --for extract` reports them. - `update` reports a missing, duplicated, unclosed or wrongly marked `region=` as `missing_region`, `duplicate_region`, `malformed_region` or `region_language_mismatch` instead of `read_failed`. A region found more than once in its file used to have its bodies joined; it is now refused. An empty diff --git a/packages/mdcode/src/cli.ts b/packages/mdcode/src/cli.ts index 1f36110..c73550d 100644 --- a/packages/mdcode/src/cli.ts +++ b/packages/mdcode/src/cli.ts @@ -15,8 +15,8 @@ import { formatList, list } from "./commands/list.ts"; import { formatRunBlock, run } from "./commands/run.ts"; import type { UpdateResult } from "./commands/update.ts"; import { describeChange, formatUpdate, update } from "./commands/update.ts"; -import type { ValidatedBlock, ValidateOperation } from "./commands/validate.ts"; -import { validate } from "./commands/validate.ts"; +import type { ValidatedDocument, ValidateOperation, ValidateOptions } from "./commands/validate.ts"; +import { sharedTargetErrors, validate } from "./commands/validate.ts"; import type { WatchEvent, WatchTarget } from "./commands/watch.ts"; import { formatWatch, watch } from "./commands/watch.ts"; import type { ProjectConfig } from "./config.ts"; @@ -119,6 +119,56 @@ function extractDir(dir: string | undefined, config: ProjectConfig | undefined): return dir ?? config?.outputRoot ?? "."; } +/** + * validate() every document for one operation, reporting every problem rather + * than stopping at the first. For extract with several documents, blocks in + * different documents that write one file are checked against each other too. + */ +async function validateDocuments( + documents: Array, + operation: ValidateOperation, + options: Pick & { base: (document: Document) => string; } +): Promise<{ results: Array; errors: Array; lines: Array; }> { + const { base, ...shared } = options; + const several = documents.length > 1; + const at = (label: string | null | undefined, text: string): string => several ? `${label}: ${text}` : text; + const results: Array = []; + const errors: Array = []; + const lines: Array = []; + const report = (error: ResultError): void => { + lines.push(styleText("red", at(error.document, `✗ ${describeError(error)} (${error.code})`))); + }; + + for (const document of documents) { + try { + const checked = await validate({ ...shared, source: await readInput(document.file), operation, base: base(document) }); + const found = inDocument(document, checked.errors); + + results.push({ document: document.label, blocks: checked.blocks }); + errors.push(...found); + found.forEach(report); + } + catch (error: unknown) { + errors.push(...inDocument(document, errorsFrom(error))); + lines.push(...errorLines(error).map(line => `Error: ${at(document.label, line)}`)); + } + } + + if (operation === "extract" && several) { + for (const error of await sharedTargetErrors(results)) { + const block = results.find(({ document }) => document === (error.document ?? null))?.blocks.find(({ line }) => line === error.line); + + if (block) { + block.valid = false; + } + errors.push(error); + report(error); + } + } + + return { results, errors, lines }; +} + /** What `mdcode update` does with the updated markdown; only apply writes it. */ const UPDATE_MODES = [ "plan", "apply", "diff", "check", "stdout" ] as const; @@ -167,12 +217,6 @@ type ValidateCliOptions = FilterCliOptions & ProjectCliOptions & { ignoreAnonymous?: boolean; }; -/** What validate found for one document, as it appears in the result's documents. */ -type ValidatedDocument = { - document: string | null; - blocks: Array; -}; - /** * Parse filter options from command-line flags */ @@ -375,6 +419,25 @@ export async function Execute( const filter = mergeFilters(config?.filter, parseFilterOptions(options)); const outputDir = extractDir(options.dir, config); const several = documents.length > 1; + + // Plan every document before writing to any, so that one refusal, or two + // documents writing one file, leaves every document's targets untouched. + // One document plans for itself inside extract(). + if (several) { + const planned = await validateDocuments(documents, "extract", { filter, base: () => outputDir, ignoreAnonymous: options.ignoreAnonymous }); + + if (planned.errors.length > 0) { + return { + result: null, + errors: planned.errors, + human: () => writeLines(stderr, [ + ...planned.errors.map(error => `Error: ${error.document}: ${describeError(error)}`), + styleText("yellow", `Nothing was written for any of the ${documents.length} documents.`), + ]), + }; + } + } + const results: Array = []; const errors: Array = []; const reports: Array<() => void> = []; @@ -761,34 +824,13 @@ export async function Execute( const config = await loadProject(options); const documents = selectDocuments(files, config); const filter = mergeFilters(config?.filter, parseFilterOptions(options)); - const several = documents.length > 1; - const results: Array = []; - const errors: Array = []; - const lines: Array = []; - // Every document is checked, so one run reports every problem. - for (const document of documents) { - const at = (text: string): string => several ? `${document.label}: ${text}` : text; - - try { - const checked = await validate({ - source: await readInput(document.file), - operation, - filter, - base: operation === "extract" ? extractDir(options.dir, config) : updateBase(options.base, config, document), - strict: options.strict, - ignoreAnonymous: options.ignoreAnonymous, - }); - - results.push({ document: document.label, blocks: checked.blocks }); - errors.push(...inDocument(document, checked.errors)); - lines.push(...checked.errors.map(error => styleText("red", at(`✗ ${describeError(error)} (${error.code})`)))); - } - catch (error: unknown) { - errors.push(...inDocument(document, errorsFrom(error))); - lines.push(...errorLines(error).map(line => `Error: ${at(line)}`)); - } - } + const { results, errors, lines } = await validateDocuments(documents, operation, { + filter, + base: document => operation === "extract" ? extractDir(options.dir, config) : updateBase(options.base, config, document), + strict: options.strict, + ignoreAnonymous: options.ignoreAnonymous, + }); const checkedBlocks = results.reduce((count, result) => count + result.blocks.length, 0); diff --git a/packages/mdcode/src/commands/validate.ts b/packages/mdcode/src/commands/validate.ts index 09f2ca7..e2c21a4 100644 --- a/packages/mdcode/src/commands/validate.ts +++ b/packages/mdcode/src/commands/validate.ts @@ -53,6 +53,13 @@ export interface ValidateResult { errors: Array; } +/** One document's validate() result, labelled with the document it came from. */ +export interface ValidatedDocument { + /** The document as named on the command line or relative to the current directory; null for stdin. */ + document: string | null; + blocks: Array; +} + /** * Check how the selected blocks map onto files for one operation, reading but * never writing. @@ -288,7 +295,8 @@ export async function planExtract(source: string, blocks: Array, outputDi * well formed there. */ async function checkTarget({ display, items }: ExtractGroup): Promise> { - const conflict = targetConflict(items); + const lines = `blocks on lines ${items.map(({ block }) => block.position?.line ?? 0).join(", ")} all write this file`; + const conflict = targetConflict(items.map(({ block }) => ({ region: block.meta.region, lang: block.lang })), lines); if (conflict !== undefined) { return items.map(({ block }) => blockError(block, { code: "ambiguous_target", message: conflict, path: display })); @@ -313,31 +321,91 @@ async function checkTarget({ display, items }: ExtractGroup): Promise): string | undefined { - if (items.length < 2) { +/** + * Why several blocks cannot share one target, or undefined when they can. + * `where` names the blocks, e.g. "blocks on lines 3, 7 all write this file". + */ +function targetConflict(blocks: Array<{ region?: string | undefined; lang: string; }>, where: string): string | undefined { + if (blocks.length < 2) { return undefined; } - const lines = `blocks on lines ${items.map(({ block }) => block.position?.line ?? 0).join(", ")} all write this file`; - const regions = items.map(({ block }) => block.meta.region); - const langs = [ ...new Set(items.map(({ block }) => block.lang.toLowerCase())) ]; + const regions = blocks.map(({ region }) => region); + const langs = [ ...new Set(blocks.map(({ lang }) => lang.toLowerCase())) ]; if (regions.includes(undefined)) { - return `${lines}, but not every one declares region=; give each block a region= of its own, or a file of its own`; + return `${where}, but not every one declares region=; give each block a region= of its own, or a file of its own`; } if (new Set(regions).size !== regions.length) { - return `${lines} and repeat a region=; give each block a region= of its own`; + return `${where} and repeat a region=; give each block a region= of its own`; } if (langs.length > 1) { - return `${lines} in different languages (${langs.map(lang => lang || "none").join(", ")}); blocks that share a file must share its language`; + return `${where} in different languages (${langs.map(lang => lang || "none").join(", ")}); blocks that share a file must share its language`; } return undefined; } +/** + * Check the targets that blocks in different documents of one extract run + * share, under the rule that applies within one document: only blocks that + * each declare a distinct region=, all in one language, may write one file. + * Pass validate()'s extract result for every document in the run. Blocks + * already invalid in their own document are left out, since validate() + * reported them there. Every error names its document. + */ +export async function sharedTargetErrors(documents: Array): Promise> { + const writers = new Map>(); + const keys = new Map(); + + for (const { document, blocks } of documents) { + for (const block of blocks) { + if (!block.valid || block.path === null) { + continue; + } + + const key = await resolveTarget(block.path); + keys.set(block, key); + writers.set(key, [ ...writers.get(key) ?? [], { document, block } ]); + } + } + + const conflicts = new Map(); + + for (const [ key, sharing ] of writers) { + if (new Set(sharing.map(({ document }) => document)).size < 2) { + continue; + } + + const where = `blocks at ${sharing.map(({ document, block }) => `${document ?? "stdin"}:${block.line}`).join(", ")} all write this file`; + const conflict = targetConflict(sharing.map(({ block }) => block), where); + + if (conflict !== undefined) { + conflicts.set(key, conflict); + } + } + + // In document order, then block order, as each document's own errors are. + return documents.flatMap(({ document, blocks }) => blocks.flatMap(block => { + const conflict = conflicts.get(keys.get(block) ?? ""); + + if (conflict === undefined) { + return []; + } + + return [{ + ...(document === null ? {} : { document }), + code: "ambiguous_target" as const, + message: conflict, + line: block.line, + ...(block.name === null ? {} : { name: block.name }), + path: block.path!, + }]; + })); +} + /** * The text of an existing target whose regions extract would splice, or * undefined when there is none to check: the file does not exist yet, or is a diff --git a/packages/usage/tests/validate.test.ts b/packages/usage/tests/validate.test.ts index f6cae0f..806c9ed 100644 --- a/packages/usage/tests/validate.test.ts +++ b/packages/usage/tests/validate.test.ts @@ -3,7 +3,7 @@ * exit code, and that it never writes. */ import assert from "node:assert/strict"; -import { mkdtemp, readdir, readFile, rm, writeFile } from "node:fs/promises"; +import { mkdir, mkdtemp, readdir, readFile, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { after, describe, it } from "node:test"; @@ -126,3 +126,101 @@ describe("mdcode validate", () => { assert.equal(await readFile(doc, "utf-8"), markdown, "the fresh block must not land while its neighbour is broken"); }); }); + +describe("extract across several documents", () => { + /** A directory holding docs/.md for each entry. */ + async function docs(files: Record): Promise { + const dir = await mkdtemp(join(tmpdir(), "mdcode-shared-target-")); + dirs.push(dir); + await mkdir(join(dir, "docs")); + + for (const [ name, markdown ] of Object.entries(files)) { + await writeFile(join(dir, "docs", `${name}.md`), markdown, "utf-8"); + } + + return dir; + } + + const fence = (info: string, code: string): string => `\`\`\`${info}\n${code}\n\`\`\`\n\n`; + + it("validate --for extract reports blocks in different documents that write one file, however it is spelled", async () => { + const dir = await docs({ + a: fence("ts file=src/x.ts", "export const a = 1;"), + b: fence("ts file=src/y.ts", "export const y = 1;") + fence("ts name=other file=./src/../src/x.ts", "export const b = 2;"), + }); + + const { exitCode, stdout } = await execCli([ "validate", "--for", "extract", "--json", "docs/a.md", "docs/b.md" ], { cwd: dir }); + const envelope = JSON.parse(stdout); + + assert.equal(exitCode, 1); + assert.deepEqual(envelope.errors.map(({ document, code, line, name }: Record) => ({ document, code, line, name })), [ + { document: "docs/a.md", code: "ambiguous_target", line: 1, name: undefined }, + { document: "docs/b.md", code: "ambiguous_target", line: 5, name: "other" }, + ]); + assert.match(envelope.errors[0].message, /^blocks at docs\/a\.md:1, docs\/b\.md:5 all write this file, but not every one declares region=/); + assert.deepEqual(envelope.result.documents.map(({ blocks }: { blocks: Array<{ valid: boolean; }>; }) => blocks.map(({ valid }) => valid)), [[ false ], [ true, false ]]); + }); + + it("extract writes nothing in any document when two of them write one file", async () => { + const dir = await docs({ + a: fence("ts file=src/x.ts", "export const a = 1;"), + b: fence("ts file=src/y.ts", "export const y = 1;") + fence("ts file=src/x.ts", "export const b = 2;"), + }); + + const { exitCode, stderr } = await execCli([ "extract", "docs/a.md", "docs/b.md" ], { cwd: dir }); + + assert.equal(exitCode, 1); + assert.match(stderr, /^Error: docs\/a\.md: line 1: src\/x\.ts: blocks at docs\/a\.md:1, docs\/b\.md:5 all write this file/m); + assert.match(stderr, /^Error: docs\/b\.md: line 5: src\/x\.ts: blocks at docs\/a\.md:1, docs\/b\.md:5 all write this file/m); + assert.deepEqual(await readdir(dir), [ "docs" ], "neither src/x.ts nor the unrelated src/y.ts may be written"); + }); + + it("extract writes nothing in any document when a later one breaks a rule of its own", async () => { + const dir = await docs({ + a: fence("ts file=src/a.ts", "export const a = 1;"), + b: fence("ts file=../outside.ts", "export const b = 2;"), + }); + + const { exitCode, stdout } = await execCli([ "extract", "--json", "docs/a.md", "docs/b.md" ], { cwd: dir }); + + assert.equal(exitCode, 1); + assert.deepEqual(JSON.parse(stdout).errors.map(({ document, code }: Record) => ({ document, code })), [{ document: "docs/b.md", code: "unsafe_path" }]); + assert.deepEqual(await readdir(dir), [ "docs" ], "docs/a.md's target must not be written before docs/b.md is refused"); + }); + + it("lets documents share a file when each block has its own region= in one language", async () => { + const dir = await docs({ + a: fence("ts file=src/x.ts region=one", "export const one = 1;"), + b: fence("ts file=src/x.ts region=two", "export const two = 2;"), + }); + + const checked = await execCli([ "validate", "--for", "extract", "docs/a.md", "docs/b.md" ], { cwd: dir }); + const extracted = await execCli([ "extract", "docs/a.md", "docs/b.md" ], { cwd: dir }); + const written = await readFile(join(dir, "src", "x.ts"), "utf-8"); + + assert.equal(checked.exitCode, 0, checked.stderr); + assert.equal(extracted.exitCode, 0, extracted.stderr); + assert.match(written, /#region one\nexport const one = 1;\n.*#endregion/s); + assert.match(written, /#region two\nexport const two = 2;\n.*#endregion/s); + }); + + it("refuses a shared file whose blocks repeat a region= or differ in language", async () => { + const dir = await docs({ + a: fence("ts file=src/x.ts region=one", "1"), + b: fence("ts file=src/x.ts region=one", "2"), + c: fence("ts file=src/y.ts region=one", "1"), + d: fence("py file=src/y.ts region=two", "2"), + }); + + const { exitCode, stdout } = await execCli([ "validate", "--for", "extract", "--json", "docs/a.md", "docs/b.md", "docs/c.md", "docs/d.md" ], { cwd: dir }); + const messages = JSON.parse(stdout).errors.map(({ document, message }: Record) => `${document}: ${message}`); + + assert.equal(exitCode, 1); + assert.deepEqual(messages, [ + "docs/a.md: blocks at docs/a.md:1, docs/b.md:1 all write this file and repeat a region=; give each block a region= of its own", + "docs/b.md: blocks at docs/a.md:1, docs/b.md:1 all write this file and repeat a region=; give each block a region= of its own", + "docs/c.md: blocks at docs/c.md:1, docs/d.md:1 all write this file in different languages (ts, py); blocks that share a file must share its language", + "docs/d.md: blocks at docs/c.md:1, docs/d.md:1 all write this file in different languages (ts, py); blocks that share a file must share its language", + ]); + }); +}); From 77c9d0d2ee9f02b4f16162902d8f90d8fd9c9da7 Mon Sep 17 00:00:00 2001 From: Adrian Elton-Browning Date: Tue, 6 Oct 2026 14:51:41 +0100 Subject: [PATCH 2/2] fix(extract): check every document before writing, including targets they share (#43) --- packages/mdcode/src/commands/validate.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/mdcode/src/commands/validate.ts b/packages/mdcode/src/commands/validate.ts index e2c21a4..0c0f11a 100644 --- a/packages/mdcode/src/commands/validate.ts +++ b/packages/mdcode/src/commands/validate.ts @@ -368,7 +368,7 @@ export async function sharedTargetErrors(documents: Array): P const key = await resolveTarget(block.path); keys.set(block, key); - writers.set(key, [ ...writers.get(key) ?? [], { document, block } ]); + writers.set(key, [ ...writers.get(key) ?? [], { document, block }]); } }