fix(extract): check every document before writing, including targets they share (#43)

.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.

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.

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

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<Document>,
+  operation: ValidateOperation,
+  options: Pick<ValidateOptions, "filter" | "strict" | "ignoreAnonymous"> & { base: (document: Document) => string; }
+): Promise<{ results: Array<ValidatedDocument>; errors: Array<ResultError>; lines: Array<string>; }> {
+  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<ValidatedDocument> = [];
+  const errors: Array<ResultError> = [];
+  const lines: Array<string> = [];
+  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<ValidatedBlock>;
-};
-
 /**
  * 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<ExtractedDocument> = [];
         const errors: Array<ResultError> = [];
         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<ValidatedDocument> = [];
-        const errors: Array<ResultError> = [];
-        const lines: Array<string> = [];
-
         // 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);
 

packages/mdcode/src/commands/validate.ts

@@ -53,6 +53,13 @@ export interface ValidateResult {
   errors: Array<ResultError>;
 }
 
+/** 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<ValidatedBlock>;
+}
+
 /**
  * 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<Block>, outputDi
  * well formed there.
  */
 async function checkTarget({ display, items }: ExtractGroup): Promise<Array<ResultError>> {
-  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<Array<Resu
   });
 }
 
-/** Why several blocks cannot share one target, or undefined when they can. */
-function targetConflict(items: Array<ExtractItem>): 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<ValidatedDocument>): Promise<Array<ResultError>> {
+  const writers = new Map<string, Array<{ document: string | null; block: ValidatedBlock; }>>();
+  const keys = new Map<ValidatedBlock, string>();
+
+  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<string, string>();
+
+  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