[johnny4young]

Review follow-up to #251

This is stacked on fix/app-review-2026-10 so the focused safety delta can be reviewed independently of the existing full-app review. Merge order, if approved: #251 first, then retarget this PR to main. No merge or release was performed.

Findings

1. --manifest or --skipped-report could name an image or sidecar produced by the same run. JSON was written after rendering and silently replaced that artifact, while the summary still reported success.
2. Both reports could name the same file, so the second write replaced the first.
3. --no-overwrite checked images and sidecars but not reports.
4. The source-protection set contained only successfully loaded text files. A skipped input could still be overwritten by another input's sidecar.
5. Batch input preflight canonicalized every source once per output, introducing quadratic filesystem work.
6. A report could replace a background/watermark input resource, including through a symlink alias.
7. Named reports are intentionally excluded from discovery, so an existing source named by --manifest/--skipped-report could be overwritten. Existing destinations inside the input tree now require bounded recognition as the matching prior report structure.

Changes

• Preflight report/image/sidecar collisions with canonical, case-folded filesystem keys
• Protect existing reports under --no-overwrite, including dry runs that still emit reports
• Validate before creating output directories or entering the render loop
• Protect every discovered input, including files that the loader skipped
• Canonicalize the source set once for the complete batch plan; preserve the single-render API
• Add regression tests for both flags, every sidecar type, case aliases, shared reports, report preservation and skipped-input preservation
• Include image-resource identities in report collision checks, with direct/symlink regression coverage
• Recognize existing in-input reports through the shared stable regular-file reader (maximum 5 MiB), strict matching keys/types and valid manifest status/dimensions. Reject unknown, malformed, wrong-kind and oversized targets before output; retain empty/nonempty report reruns and recursive/nested inputs
• Document that recognized structure is not cryptographic proof of origin

Review follow-ups (2026-10-06)

• Verified:
  • The guards run before any directory is created.
  • The report-structure validator matches the encoder: planned entries omit width/height, format is limited to png/pdf/heic/avif, and language is a raw value.
  • Symlink aliases to resources are caught.
  • Empty and matching non-empty reports can be re-used.
• Applied:
  • cca7677 merges fix: full app review — editor, menu bar, settings, domain, web/CLI, docs and screenshots #251's head, which carries the runtime outputConflict error and case-folded claim keys. The conflict in CLIOutputWriter is resolved by applying both to the batch (array) form of guardOutputsDoNotOverwriteInputs. The CHANGELOG keeps both entries.
  • 0c05fdc fix(cli): report batch output conflicts deterministically:
    • planned outputs are checked in source-key order instead of dictionary order, so the first reported conflict is the same on every run;
    • a batch report that collides with a resource or output now raises outputConflict (exit 1, message only), like the other filesystem conflicts.
• Validation: swiftc -parse and swift format lint --strict on the changed files (Swift 6.0.3, Linux) and the repository hygiene script pass. AppKit targets cannot be compiled on Linux; native build and tests rely on macOS CI for this head.

Earlier verification (head 2d3132e)

• CI run 37363266467: all five required jobs successful (Linux static, Sequoia/Tahoe build and tests, both native UI suites with strict visual tours). Tahoe logs confirmed 268 CLI, 158 domain, 53 rendering, 133 repository and 1,919 app tests.
• Critical changed-line coverage 101/103 (98.06%); Tahoe production coverage 59.28% (floor 47.93%).
• Website npm test passed: Astro check/build and 58/58 Node tests.

Scope

No dependency, entitlement, rendering-style or test-lane changes. The existing rerun behavior for separately named manifest/report files is retained.

Merge order

#251 first, then retarget this PR to main and re-run CI.

[johnny4young]

Review: protect batch artifacts and skipped inputs during preflight

This is stacked on #251, and I reviewed only the delta against fix/app-review-2026-10.

What it does: before any directory is created, the batch now checks that --manifest and --skipped-report don't collide with each other, with any planned image or sidecar, or with the background or watermark resources. It uses canonical, case-folded keys. --no-overwrite now also protects existing reports. Every discovered input, including skipped ones, is protected from sidecars, and the input set is canonicalized once per batch instead of once per output. An existing file inside the input tree may only be replaced if it is a recognized prior report of the same kind (≤ 5 MiB, strict keys and types).

Verdict: ready to merge after #251, then retarget to main and re-run CI.

What I checked

• Ordering: createDirectory moved after both guards, so a rejected run leaves nothing behind, as the tests assert with !fileExists(output).
• Report recognition (matchesReportStructure):
  • Manifest entries use synthesized Codable with encodeIfPresent, so planned entries omit width/height, exactly what the validator expects.
  • format comes from ExportFormat, which has only png/pdf/heic/avif.
  • language is Language.rawValue.
  • Empty arrays are accepted, so [] reruns work.
• Size limit: maximumExistingReportBytes reuses FileInputLoader.maximumByteCount (5 MiB), which matches the README.
• Symlinks: a symlink alias to a resource is caught through canonicalPath (resolvingSymlinksInPath).
• CI was green on 2d3132e (all 5 required jobs).

Nits (not blocking)

• 🟡 plannedOutputs = Array(outputURLs.values) iterates a dictionary, so when several outputs conflict, the path in the error message can differ between runs. Sorting by path would make CI logs reproducible.
• 🟡 guardOutputsDoNotOverwriteInputs (in CLIOutputWriter) still compares canonicalPath without case folding, while guardReportTargets folds case. On the default case-insensitive APFS, render notes.MD --out notes.png --markdown-sidecar still slips past the input guard. That's pre-existing, but both guards could share filesystemKey.
• 🟡 Report conflicts throw incompatibleOptions, which with #251 means exit code 2 and the full usage text, for what is really a filesystem conflict. A dedicated CLIError case (exit 1, no usage dump) would read better in CI logs.
• 🟡 A legitimate manifest larger than 5 MiB stored inside the input tree can't be re-used. That's documented, but a very large recursive batch could hit it.

---

Generated by Claude Code