Skip to content

EPIC-14: Extension points — first-party contributions + text filters - #54

Merged
Joncallim merged 16 commits into
masterfrom
claude/pr-49-deep-review-75lhq0
Sep 6, 2026
Merged

EPIC-14: Extension points — first-party contributions + text filters#54
Joncallim merged 16 commits into
masterfrom
claude/pr-49-deep-review-75lhq0

Conversation

@Joncallim

@Joncallim Joncallim commented Sep 2, 2026

Copy link
Copy Markdown
Owner

What this changes

Implements issue #15 (EPIC-14) end to end for its first shipped contribution: a renderer-neutral first-party contribution SPI/registry, the [TOC] table-of-contents contribution, and its Preview and HTML/PDF Export integration. Text-filter commands (the other half of issue #15) are not in this PR — see "Out of scope" below.

This PR head also carries a full remediation of ten contract defects (four P1, four P2, one P3, one CI-only) found by an adversarial architecture review after the Preview/Export integration first landed. The authoritative coding hand-off is PR #54#issuecomment-5538790625 ("Architecture takeover — implementation lock"), which supersedes the earlier review pass at #issuecomment-5538260720. planning/epic-14-implementation.md §19 reconciles the architecture document with what actually shipped.

Also fixes three pre-existing, unrelated failures discovered while verifying this PR's own app-test suite — including a real recovery-data-loss bug — at the reporter's request, rather than splitting them into a separate PR. See "Also fixed" below.

Why

planning/epics/README.md places E14 immediately after the Settings epic (#52) and before the math/diagram epics (E19/E20), which both need this exact seam — a way for first-party code to contribute rendered content into Preview and Export without each epic inventing its own renderer.

How it works

  • Contribution SPI (Packages/MacDownKit/Sources/Contributions): Contributing, ContributionResult/ContributionContent/ContributionPlacement/ContributionRepresentation/ContributionDiagnostic, and ContributionRegistry (fault-isolating per contribution; cancellation propagates via try Task.checkCancellation() before each contribution, immediately after each one returns, and once more before the registry returns — never converted into an empty successful result). sourceGeneration is an opaque, caller-selected snapshot token compared only for equality — Preview keys it to MarkdownDocument.revision; Export continues using the FileDocument.mutationGeneration of its captured export snapshot.
  • TOCContribution: replaces every line whose trimmed content is exactly [TOC] with a nested Markdown list built from the document's headings, but only when that line is the entire content of exactly one top-level, parsed .paragraph block (non-recursive document.blocks check). A marker inside fenced/indented code, a list item, a block quote, a heading, front matter, or an HTML block is literal text — in both Preview and Export, since both call this one discovery path.
  • Preview integration (PreviewContributionAdapter.compose(...), split across PreviewContributionAdmission.swift/PreviewContributionComposer.swift): a pure function — admits each contribution result (generation match, Markdown-only content, non-empty, in-bounds UTF-16 range, contained in exactly one preview block, placement-capability check), resolves overlap and the explicit Preview budget, then rebuilds the affected blocks by walking the containing block's source left to right with a UTF-16 cursor — .inline splices in place, .block replaces either the whole block or complete physical line(s) inside a paragraph. Untouched blocks pass through with their original IDs unchanged; affected/generated blocks get a deterministic ID derived from CryptoKit.SHA256 over their role/span/contribution-id (never Hasher/a random UUID()).
  • Preview task ownership (PreviewContributionSession, mirrors MarkdownParseSession's shape): keyed by document/tab identity + parsed revision (PreviewContributionTaskID), holds one atomic PreviewContributionComposition (blocks + diagnostics published together), and only publishes when its task ID is still current — a cancelled or superseded refresh performs no state mutation, and a non-text FileDocument change (save, rename, encoding) can no longer make a valid composed TOC disappear.
  • Preview diagnostics: a compact, non-modal PreviewContributionDiagnosticsBadge beside the existing busy indicator surfaces both producer- and adapter-raised diagnostics (highest severity + count; activation opens a deterministic list), gated to the currently displayed parsed revision.
  • Export integration (ExportContributionAdapter.adapt(_:) -> Adaptation): renders each placeable .markdown result via ExportService.renderMarkdownFragment into an ExportDerivedContribution for E12's existing DerivedContentComposer; a diagnostic-only result (nothing to anchor to) becomes a standaloneDiagnostics entry instead of being dropped. ExportCoordinator.combinedDiagnostics(_:_:) merges standaloneDiagnostics before the export service's own diagnostics, identically for the HTML and PDF paths (one helper, both call it, tested directly).
  • Budgets: Preview's is new and explicit (PreviewContributionBudget.standard: 64 accepted placements, 64 KiB generated Markdown, overflow-safe) — an omitted candidate leaves its source authored/visible and produces one aggregate warning, replacing a silent cap. It is deliberately smaller than, and independent of, Export's existing ExportResourceBudget (4096 fragments / 32 MiB), which is unchanged.

Also fixed (pre-existing, unrelated to EPIC-14)

Discovered while running this PR's own full app-test suite to verify it; confirmed unrelated via git diff (none of these files were part of the EPIC-14 work) and reproducible with the EPIC-14 commits stashed out:

  • Recovery data-loss bug (RecoveryBuffer+StorageSupport.swift, migrateLegacyFenceLedger): unconditionally swept every legacy (non-generation-encoded) recovery epoch into the durable retired set on ledger load, even when that epoch was still the recorded current owner for its document. isRetired's legacy branch checks only retired membership, never the current-owner map — so a document using a legacy, plain-UUID recovery epoch had its still-current crash-recovery snapshot silently and permanently marked unrecoverable the next time the ledger loaded (e.g. app relaunch), even with valid content still on disk. Now skips retiring a legacy lifetime that is still its document's current owner.
  • NSEvent construction crash (CommandStateRefreshTests): NSEvent.mouseEvent(with:...) asserts its type is an actual mouse event; the test constructed an .appKitDefined (system-defined) event through it, which macOS 26's AppKit now enforces strictly (crashing the whole test process) where a prior macOS silently tolerated it. Fixed by using NSEvent.otherEvent(with:...) instead.
  • Three flaky/broken test fixtures (ExternalFileControllerRecoveryTests, ExternalFileControllerCloseRecoveryTests, ScriptedRecoveryExecutor): a weak var model deallocated before assertions ran, a fixture relying on a 300ms debounce timer racing the assertion, and a scripted test executor that never touched real recovery storage so its own on-disk assertions could never pass.

Out of scope

  • Text-filter commands (the second half of issue [EPIC-14] Extension points: first-party contribution seam + user text-filter commands #15) — not implemented in this PR.
  • The .html(ContributionRepresentation) case remains real, typed, and unhandled — Preview has no HTML-fragment rendering path today (every block goes through PreviewMarkupParser/Textual as Markdown-shaped text); a contribution producing .html gets an explicit diagnostic and its authored source is preserved.
  • TOC's generated list entries are plain text, not clickable links — no part of today's export pipeline generates a stable heading id, and Preview hands every link to NSWorkspace.shared.open rather than scrolling to it.

Both are pre-existing, named residual risks (planning/epic-14-implementation.md §18), unchanged by this remediation.

Commands run and observed outcomes

swiftformat --lint MacDown2         → 0/389 files require formatting
swiftlint lint --strict MacDown2    → 0 violations, 0 serious

cd MacDown2/Packages/MacDownKit && swift build && swift test --no-parallel
  → Build complete; 1054 tests in 116 suites passed

xcodegen generate                   → project regenerated

xcodebuild -scheme MacDown2 -destination 'platform=macOS' build
  → BUILD SUCCEEDED
xcodebuild -scheme macdown2 -destination 'platform=macOS' build
  → BUILD SUCCEEDED
xcodebuild -scheme MacDown2 -destination 'platform=macOS' -enableCodeCoverage NO build-for-testing
  → TEST BUILD SUCCEEDED (this is the exact command CI's "Build UI tests" step runs;
    it was failing at this PR's original pinned head before Slice 0's `import Foundation` fix)
xcodebuild -scheme MacDown2 -configuration Release -destination 'platform=macOS' build
  → BUILD SUCCEEDED

xcodebuild -scheme MacDown2 -destination 'platform=macOS' -enableCodeCoverage NO -only-testing:MacDown2Tests test
  → Test run with 71 tests in 10 suites passed (the FULL app-target test suite — every
    test file, not a filtered subset; green only after the "Also fixed" items above)

CI (lint + build-and-test) is green on this PR's current head.

Manual matrix — verified

The reporter (@Joncallim) ran the Debug build locally and confirmed:

  • a standalone [TOC] and a [TOC] written between two lines of one paragraph both render correctly in Preview, with surrounding text intact;
  • a [TOC] inside a fenced code block stays literal text;
  • rapid editing and Save (⌘S) do not make a valid composed TOC flicker or disappear;
  • HTML and PDF export both produce a correct TOC and open cleanly outside the app.

The remaining edge cases in planning/epic-14-implementation.md's full matrix (LF/CRLF equivalence, non-BMP/emoji offsets, 65+ marker budget overflow, malformed/overlapping synthetic contributions, exact diagnostics-badge content) are covered by the automated test suite referenced above rather than re-walked by hand.

Risk and rollback

  • The EPIC-14 contribution work is additive/internal to Contributions, the two app-target adapters, and one new session/view pair — no public ExportService/E12 API changed, no new SPM dependency or target, no CI/workflow change. ContributionRegistry.standard's membership and DerivedContentComposer are unchanged.
  • The recovery-ledger fix ("Also fixed") is a 3-line, additive guard in one existing function (migrateLegacyFenceLedger) — it only prevents an incorrect retirement that was already user-visible as silent data loss; it does not change any other ledger/persistence behavior.
  • Revert is a plain git revert of this PR's commits.

Links

🤖 Generated with Claude Code

PR #52 merged; the table still said "open".

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015pVm6rWERNoNgDBpAnoQXv
… filters)

Reconciles issue #15 against live master (bbfb010): E12's contribution seam
(ExportDerivedContribution/DerivedContentComposer) and Preview's
pre-computed-blocks parameter are both already built and unused, so the
new Contributions/TextFilters modules and their app-layer adapters are the
only new surface needed. Covers all 18 EPIC_STANDARD.md sections, including
the renderer-neutral markdown/html result shape, the text-filter process
safety contract, and eight dependency-ordered implementation slices.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015pVm6rWERNoNgDBpAnoQXv
…contribution

New Contributions SwiftPM target (depends only on MarkdownEngine, per
epic-14-implementation.md §5): the renderer-neutral result types
(ContributionResult/Content/Representation/Diagnostic), the Contributing
protocol, and ContributionRegistry, which runs every registered
contribution and isolates one contribution's thrown error from the rest
while letting cancellation propagate. DeterministicTestContribution makes
that isolation/cancellation behaviour deterministically testable without
depending on the real TOC contribution, which lands in the next slice.
ContributionRegistry.standard is empty until then.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015pVm6rWERNoNgDBpAnoQXv
TOCContribution replaces every line consisting only of [TOC] with a
Markdown list nested from document.headings, using the same level-stack
algorithm most TOC generators use for skipped heading levels. Registered
into ContributionRegistry.standard.

Corrects the architecture doc's original claim that export would produce
"real anchor links": verified directly that no part of the export
pipeline generates a stable id for a heading, and that Preview hands
every link (anchor or not) to NSWorkspace.shared.open rather than
scrolling to it - both pre-existing gaps, not something a link generated
by this slice could paper over. TOC's list is plain text for now, with
the anchor work named as explicit follow-up in epic-14-implementation.md
Section 18 rather than attempted speculatively.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015pVm6rWERNoNgDBpAnoQXv
Build: a switch used as run(...)'s implicit return mixed throwing cases
with an empty [] literal, which the compiler could not infer a type for
without context ("empty collection literal requires an explicit type").
Made every non-throwing case an explicit `return` instead of relying on
switch-expression inference.

Lint: DeterministicTestContributionError.errorDescription's single-line
body needed wrapping onto its own line (wrapPropertyBodies), the same
SwiftFormat rule EPIC-13 hit for the same reason.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015pVm6rWERNoNgDBpAnoQXv
DocumentEditorSplitView now runs the standard contribution registry
alongside its existing reparse (a new .task(id: parseSession.document),
cancelled and restarted automatically by SwiftUI on every reparse rather
than a hand-rolled generation counter) and merges placeable results into
the blocks TextualMarkdownPreview already accepts as a pre-computed
parameter - no change to the Preview package itself.

PreviewContributionAdapter is a standalone, stateless type rather than an
extension on DocumentEditorSplitView: that view's body was already at
238 effective lines against SwiftLint's 250-line type_body_length budget,
with too little headroom for this feature's glue code. Bought back real
margin by extracting the pre-existing divider(in:) drag handler into
DocumentEditorSplitView+Divider.swift too (dragOriginFraction,
currentSplitFraction and coordinator are internal rather than private so
that extension can reach them, matching how
DocumentEditorSplitView+AppSettings.swift already split out unrelated
content for the same budget reason).

Substitution works at PreviewBlock granularity: a contribution's marker
occupies an entire base block by itself in the expected case (TOCContribution
only recognises a marker on its own line); one sharing a multi-line
paragraph with other text would replace that whole paragraph, a documented,
accepted limitation of Preview having no inline-splicing mechanism.

project.yml: added the new Contributions package product to the MacDown2
app target (needed to import it at all) and to MacDown2Tests, plus Preview
to MacDown2Tests for the new adapter test's PreviewBlock fixtures.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015pVm6rWERNoNgDBpAnoQXv
ContributionRegistry.run: SwiftLint's trailing_comma is stricter than
SwiftFormat's - it flags a multi-line collection literal with no trailing
comma even when it holds a single element, if that element's own call
spans multiple lines. Bound the diagnostic to a local let first, the same
fix EPIC-13 used for the identical rule.

PreviewContributionAdapter.results: SwiftFormat's hoistAwait rejected
(try? await foo()) ?? bar - await nested inside a parenthesized try?
combined with ??. Replaced with an explicit do/catch, which is
unambiguous and matches the try-await pattern already used everywhere
else in this codebase, rather than relying on try?/await/?? precedence.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015pVm6rWERNoNgDBpAnoQXv
ExportService.renderMarkdownFragment renders a standalone Markdown
fragment to HTML, independent of the whole-document metadata/theme/
resource pipeline - the one missing piece an export-side adapter needed
(no existing public entry point did this). Deliberately omits
CMARK_OPT_UNSAFE: a contribution-generated fragment has no legitimate
need to embed raw HTML, so this is more conservative than the main
document pipeline.

ExportContributionAdapter turns contribution results into
ExportDerivedContribution, the type E12's already-shipped
DerivedContentComposer has accepted (unused) since #49. Its switch over
ContributionRepresentation has no default: case, so a third
representation variant fails to compile here until this adapter decides
what it means; .html specifically constructs an empty-html contribution
plus a diagnostic rather than silently dropping it, so
DerivedContentComposer's existing empty-html rejection preserves
authored source while the diagnostic stays visible.

ExportCoordinator.performExport now computes contributions via its own
extra parse of the same text ExportComposer.prepare will parse again
moments later internally - one redundant parse per export, accepted as
negligible next to export's other costs, and avoids threading Preview's
live parse session into the export path.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015pVm6rWERNoNgDBpAnoQXv
…nt list

SwiftFormat's hoistAwait/hoistTry flagged contributions: try await
exportContributions(for: document) as one named argument buried inside
the larger multi-line ExportRequest(...) call, rather than the
throwing/async work being its own statement. Computed it into a local
let first, matching every other try await call site in this file.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015pVm6rWERNoNgDBpAnoQXv

Copy link
Copy Markdown
Owner Author

[P1] Preview contributions are invalidated by non-text document mutations

DocumentEditorSplitView starts contribution work with .task(id: parseSession.document), stamps the result with the live document.mutationGeneration, and later rejects it unless that generation still equals the live document generation. Those clocks do not describe the same thing: MarkdownParseSession.document changes only when a parse publishes, while FileDocument.mutationGeneration deliberately advances for non-text state transitions too (markClean, save, rename/rebind, etc.).

Deterministic failure path:

  1. Open a Markdown document containing [TOC]; contribution result is computed at generation g and renders.
  2. Save it. The source text and parse snapshot do not change, but FileDocument.saving(...) advances mutationGeneration to g+1.
  3. .task(id: parseSession.document) does not restart because the parsed document did not change.
  4. PreviewContributionAdapter.merged(... currentGeneration: g+1) rejects the still-correct result stamped g, so the TOC disappears/reverts to authored source until another parse occurs.

This defeats the intended stale-result guard. The generation used to compute, trigger, and validate a contribution needs to belong to the same immutable source snapshot. Prefer a dedicated text/parse generation captured with publishedText/MarkdownDocument; at minimum the task ID must include the exact generation it validates. Add an integration test covering render TOC -> save/mark clean without editing -> TOC remains rendered.

Copy link
Copy Markdown
Owner Author

[P1] A physical [TOC] line can delete adjacent authored Markdown from Preview

PreviewContributionAdapter.merged replaces the entire PreviewBlock when a contribution's source line falls anywhere inside that block. The comment treats “marker is on its own line” as sufficient, but CommonMark paragraphs can span multiple non-blank physical lines. For example:

Alpha
[TOC]
Omega

is one paragraph block. Preview therefore replaces that whole paragraph with the TOC, hiding Alpha and Omega. Export does not have this behavior: DerivedContentComposer splices only the exact UTF-16 contribution range, preserving the neighboring authored text. Two [TOC] lines inside one paragraph also collapse to the first placement because the merge uses first(where:) per block.

This is a cross-surface correctness defect, not just a display limitation. CommonMark §4.8 explicitly defines a sequence of non-blank lines as one paragraph. Please split/splice Preview by source range (or represent contribution markers as proper parsed block nodes) rather than letting one physical line own a whole top-level block. Add regressions for adjacent text on both sides and for two markers sharing one paragraph.

Reference: https://spec.commonmark.org/spec#paragraphs

Copy link
Copy Markdown
Owner Author

[P1] [TOC] is rewritten inside literal code blocks

TOCContribution.findMarkers scans raw physical lines and matches after trimmingCharacters(in: .whitespacesAndNewlines). It never consults the parsed Markdown block containing the line. Consequently both of these are treated as active TOC markers:

```text
[TOC]
```

and

    [TOC]

CommonMark specifies fenced and indented code-block contents as literal text, not Markdown to be interpreted. This means the contribution system currently mutates authored code in both Preview and Export.

The repository already exposes parsed block context via MarkdownDocument.block(atLine:); marker discovery should only accept semantically eligible Markdown context and explicitly exclude fenced/indented code (and future literal/raw contexts as appropriate). Add fenced-code and indented-code regressions.

Reference: https://spec.commonmark.org/spec#fenced-code-blocks and https://spec.commonmark.org/spec#indented-code-blocks

Copy link
Copy Markdown
Owner Author

[P1] Restore the existing build-and-test CI gate before merge

The current PR head a7aac6e44254817bb6679950cd24499fe92c669d is red in the repository's existing build-and-test workflow (failure occurs at the UI-test build stage). The same head's lint check succeeds, and the preceding base commit bbfb010074fbe44201ab6a8b1c4d6244c51ecf18 has both lint and build-and-test green.

That establishes this as a PR regression rather than a pre-existing flaky/red baseline. The PR's own verification checklist requires the app/test build gates to pass, so this should block merge even if the package-level contribution tests are green. Please restore the UI-test build and add/retain the failing configuration as a required gate; do not waive it solely because the new package tests pass.

Copy link
Copy Markdown
Owner Author

[P2] Preview ignores ContributionPlacement and promotes inline contributions to whole-block replacements

The public contribution contract distinguishes .inline and .block, and ExportContributionAdapter faithfully maps that distinction into ExportDerivedPlacement. PreviewContributionAdapter.placements, however, never checks content.placement; any .markdown result becomes a Placement, and merged replaces the containing PreviewBlock wholesale.

A valid future contribution returning placement: .inline for a small source range would therefore replace the entire paragraph in Preview while Export replaces only the inline range. This makes the newly introduced extension API internally inconsistent even before a second first-party contribution is added.

Preview must either implement inline splicing or explicitly reject/preserve unsupported inline contributions with a surfaced diagnostic. Do not silently reinterpret .inline as .block. Add a contract test asserting identical placement semantics across Preview and Export.

Copy link
Copy Markdown
Owner Author

[P2] Validate extension-provided source ranges before mapping them into Preview

ContributionContent documents sourceRange as non-empty source UTF-16, but its initializer cannot enforce that contract. Preview immediately feeds sourceRange.lowerBound into SourceMap.line(atUTF16Offset:); that helper intentionally clamps negative offsets to line 1 and offsets past EOF to the last line. A malformed contribution such as -1..<0, 0..<0, or a past-end range can therefore replace an unrelated first/last Preview block.

The export path already has the correct defensive behavior: DerivedContentComposer validates bounds, non-empty ranges, staleness, overlaps, and budget before splicing, preserves authored source on rejection, and emits a diagnostic. Preview should apply the same boundary validation rather than relying on a clamping line-lookup helper.

Please centralize/reuse the validation contract where possible and add negative, zero-width, past-end, and overlapping-range Preview tests. Extension points should fail closed at this boundary.

Copy link
Copy Markdown
Owner Author

[P2] Preview silently truncates valid contributions at 64 and diverges from Export

PreviewContributionAdapter.placements applies contributions.prefix(maxMergedResults) with maxMergedResults = 64 before checking generation/content/representation. Export's established resource budget allows up to 4096 derived fragments and explicitly diagnoses/rejects over-budget work.

Two concrete consequences:

  • A document containing 65 valid standalone [TOC] markers renders only the first 64 in Preview, while Export can place all 65.
  • Diagnostic-only, stale, or otherwise non-renderable results in the first 64 consume Preview's allowance and can starve later valid results.

A safety budget is appropriate, but silent, surface-specific truncation is not. Apply any Preview cap after eligibility validation, align it with a deliberate shared resource policy where practical, and surface an explicit diagnostic when work is rejected. Add >64-marker and ineligible results before valid result parity tests.

Copy link
Copy Markdown
Owner Author

[P2] Cancellation is converted into a successful empty result, allowing superseded tasks to publish

ContributionRegistry.run correctly treats CancellationError specially and propagates it, but PreviewContributionAdapter.results immediately catches every error and returns []. The caller then unconditionally executes contributionResults = await ... inside .task(id:).

That defeats structured-cancellation semantics: when SwiftUI cancels an old task because the ID changes, the canceled task can catch cancellation as an ordinary empty success and still mutate shared UI state. With future contributors that suspend or do longer CPU work, an older canceled task can clear/overwrite state after a newer task has started. The registry also checks cancellation only before each contributor; Swift cancellation is cooperative, so a contributor that does not observe cancellation can run to completion.

Keep cancellation distinct from ordinary failure: propagate it out of the adapter or return an explicit canceled state, check cancellation immediately before publishing results, and require long-running contributors to cooperate/check cancellation. Add a deterministic superseded-task race test.

Apple documents that task cancellation is cooperative and does not automatically stop arbitrary functions: https://developer.apple.com/documentation/swift/task/cancel()

Copy link
Copy Markdown
Owner Author

[P2] Registry failure diagnostics are silently lost at both UI boundaries

ContributionResult explicitly defines the contract that content == nil may carry .error diagnostics and says callers still surface those diagnostics. ContributionRegistry.run relies on that contract to isolate a throwing contribution without aborting every other contribution.

Neither integration boundary currently honors it:

  • Preview keeps the result array but merged only consumes placeable content and has no diagnostic presentation/logging path.
  • ExportContributionAdapter explicitly compactMaps away every content == nil result, so the diagnostic cannot reach DerivedContentComposer or export feedback.

The result is a silent extension failure: authored source is preserved, but neither the user nor a developer gets the reason the extension failed. That contradicts the new API's own documented failure semantics and the export composer's existing “nothing a renderer reported is silently dropped” policy.

Please add an anchorless diagnostic channel (UI status/logging for Preview; export diagnostics independent of placement for Export), or revise the registry/result contract so it does not promise observability that callers cannot provide. Add an integration test with a throwing contribution and assert that its diagnostic survives both surfaces.

Copy link
Copy Markdown
Owner Author

[P3] PR description/test plan is materially stale relative to the actual head

The current PR body still says this push contains only Slice 1 / the protocol-registry architecture, that ContributionRegistry.standard is intentionally empty, that TOC/Preview/Export wiring will arrive in later pushes, and that no user-visible behavior has landed. The head now contains TOCContribution, a non-empty standard registry, Preview wiring in DocumentEditorSplitView, Export wiring in ExportCoordinator, adapter tests, and the first-party behavior itself.

That mismatch is not cosmetic for a draft of this size: it tells reviewers not to test exactly the code that is now present, and the verification section remains entirely unchecked despite a currently red build gate. Please reconcile the PR body with the head, list the actual landed slices/files and concrete manual cases, and record the current CI/test evidence before hand-off/merge.

Copy link
Copy Markdown
Owner Author

Deep-review hand-off index — head a7aac6e44254817bb6679950cd24499fe92c669d

I ran repeated passes across functional correctness, CommonMark semantics, Preview↔Export parity, extension/API contracts, snapshot/concurrency/cancellation, resource bounds, failure observability, trust/safety boundaries, CI/build integration, test coverage, and PR/review hygiene. After de-duplicating downstream symptoms, the remaining validated findings are:

P1 — merge blockers

  1. Preview contribution generation is invalidated by non-text FileDocument mutations (save/mark-clean etc.).
  2. A standalone physical [TOC] line can replace a whole multi-line paragraph and hide adjacent authored text in Preview.
  3. [TOC] is interpreted inside fenced/indented literal code blocks.
  4. Current head regresses the existing build-and-test CI gate from green base to red head.

P2 — correctness/contract defects
5. Preview ignores .inline vs .block placement semantics.
6. Preview does not validate extension-provided source ranges and can clamp malformed offsets onto unrelated blocks.
7. Preview silently truncates at 64 before eligibility filtering, diverging from Export's explicit 4096-fragment budget/diagnostics.
8. Preview converts cancellation into an empty successful result, allowing superseded tasks to publish state.
9. Diagnostic-only registry failures are dropped at the Preview/Export integration boundaries despite the API contract promising they are surfaced.

P3 — hand-off/review integrity
10. PR description and verification plan are materially stale relative to the actual head.

Each item is posted above as its own remediation comment with reproduction/contract evidence and requested regression tests. I did not promote intentional/explicit residual limitations (e.g. clickable TOC anchors and unsupported HTML preview rendering) into duplicate findings. On the reviewed head, this PR is not merge-ready until the P1s are closed and CI is green; the P2s should be addressed before treating this extension seam as reusable by later contributions.

@Joncallim Joncallim left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architecture passes are being prepared against head a7aac6e44254817bb6679950cd24499fe92c669d. I am keeping the existing module graph and public contribution protocol intact; each finding will get an implementation-ready plan plus an immediate compatibility review before the next pass.

@Joncallim Joncallim left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architecture pass 1/10 — P1: Preview source-generation clock

Original finding: issue comment 5534694929.

Required invariant

A Preview contribution is valid iff it was computed from the same parsed source snapshot currently being rendered. File lifecycle transitions that do not alter parsed source — save, mark-clean, rename, recovery-state changes, etc. — must neither invalidate nor recompute it.

Do not introduce another mutable generation counter. The repository already has the correct snapshot identity: MarkdownParseSession publishes MarkdownDocument and publishedText together, and each accepted parse has a monotonically increasing MarkdownDocument.revision inside that session.

Files to change

  • MacDown2/MacDown2/DocumentEditorSplitView.swift
  • MacDown2/Packages/MacDownKit/Sources/Contributions/ContributionResult.swift — documentation semantics only
  • MacDown2/MacDown2Tests/PreviewContributionAdapterTests.swift
  • add an app-level integration/state test only if the existing test harness has a practical DocumentEditorSplitView seam; otherwise make the adapter test deterministic and cover the save/mark-clean behavior at the smallest existing model seam.

Final data model

Keep ContributionResult.sourceGeneration: UInt unchanged. Change its documentation from “the FileDocument.mutationGeneration this result was computed from” to an opaque renderer-supplied token identifying the exact source snapshot passed into Contributing.run. Export may continue using FileDocument.mutationGeneration because an export operates on one captured FileDocument value; Preview must not.

In DocumentEditorSplitView, replace the current contribution state with composed snapshot state:

@State private var contributedPreviewBlocks: [PreviewBlock]?
@State private var contributionDiagnostics: [PreviewContributionAdapter.Diagnostic] = []

Remove @State private var contributionResults once the Preview composition foundation is in place.

Add a private task key:

private struct ContributionTaskID: Hashable {
    let identity: String
    let revision: Int?
}

private var contributionTaskID: ContributionTaskID {
    ContributionTaskID(identity: identity, revision: parseSession.document?.revision)
}

Use .task(id: contributionTaskID), not .task(id: parseSession.document) and never key it on document.mutationGeneration.

Exact refresh sequence

When parseSession.document changes:

  1. Run the existing refreshPreviewBlocks() and refreshOutline().
  2. Immediately set contributedPreviewBlocks = nil and contributionDiagnostics = []. This prevents old derived blocks from being displayed against a newly published parse while the replacement task is running.
  3. The task keyed by {identity, revision} starts for the new snapshot.

Inside that task:

private func refreshPreviewContributions() async {
    guard let parsed = parseSession.document,
          let sourceText = parseSession.publishedText
    else {
        contributedPreviewBlocks = nil
        contributionDiagnostics = []
        return
    }

    let revision = parsed.revision
    guard let generation = UInt(exactly: revision) else {
        assertionFailure("Markdown parse revision must be non-negative")
        contributedPreviewBlocks = nil
        contributionDiagnostics = []
        return
    }

    do {
        let results = try await PreviewContributionAdapter.results(
            document: parsed,
            text: sourceText,
            generation: generation
        )
        try Task.checkCancellation()

        let base = PreviewBlock.blocks(from: parsed, text: sourceText)
        let composition = PreviewContributionAdapter.compose(
            base: base,
            sourceText: sourceText,
            sourceMap: parsed.sourceMap,
            contributions: results,
            currentGeneration: generation
        )
        try Task.checkCancellation()

        guard parseSession.document?.revision == revision,
              parseSession.publishedText == sourceText
        else { return }

        contributedPreviewBlocks = composition.blocks
        contributionDiagnostics = composition.diagnostics
    } catch is CancellationError {
        return
    } catch {
        // Preserve the ordinary Preview; this is an adapter/system failure,
        // not permission to blank authored content.
        contributedPreviewBlocks = nil
        contributionDiagnostics = [
            .init(
                contributionID: nil,
                severity: .error,
                message: "Preview contribution processing failed: \(error.localizedDescription)"
            ),
        ]
    }
}

Implementation refinement: avoid the parseSession.publishedText == sourceText whole-document comparison if profiling shows it material on the hot path. The mandatory stale guard is {identity, revision}; MarkdownParseSession already publishes document and publishedText atomically and rejects stale parse revisions. It is acceptable to omit the string comparison and use only identity + revision in production.

Preview rendering becomes:

blocks: contributedPreviewBlocks ?? previewBlocks

There must be no live FileDocument.mutationGeneration check anywhere in the Preview contribution path after this change.

Required regression tests

  1. sourceGeneration matching the parse-revision token is accepted.
  2. A stale parse-revision token is rejected and authored Preview blocks are preserved.
  3. Model-level sequence: parse [TOC] -> compose derived Preview -> perform FileDocument.markClean() or a save-success transition that advances mutationGeneration without changing text -> the same composed Preview remains valid because its task/source revision did not change.
  4. New parse revision for changed text invalidates old derived blocks before publishing the new result.
  5. Switching identity with the same numeric revision cannot publish the former document's result.

Acceptance criteria

  • Saving a Markdown file containing a rendered TOC does not make the TOC disappear.
  • Rename/mark-clean/recovery-state-only transitions do not trigger contribution recomputation.
  • A real text edit that produces a new parse revision does recompute contributions.
  • A superseded task cannot publish against another document identity.
  • Export behavior and FileDocument.mutationGeneration semantics are unchanged.

Immediate hostile compatibility review

Module/API: PASS. This changes no public signature and does not make Contributions depend on app, Preview, FileCore, or ExportService.

File/recovery state machine: PASS. FileDocument.mutationGeneration remains untouched for its existing ABA/save/recovery duties; we simply stop misusing it as a Preview parse clock.

Markdown parse lifecycle: PASS. MarkdownParseSession already guarantees that document and publishedText are published together and that older revisions do not replace newer ones. Reusing revision is therefore narrower and more correct than adding a second clock.

SwiftUI task identity: PASS with one constraint: include identity in the task ID so two document sessions that both happen to be on revision n cannot alias if the view is reused.

Scroll sync / Preview identity: PASS. This pass does not alter source line ranges. Composed blocks are stored once per parse snapshot rather than rebuilt in body, which also stabilizes synthetic block IDs during unrelated SwiftUI body recomputations.

Performance: PASS. A small {String, Int?} task key is cheaper to compare than the full MarkdownDocument value. No additional parse is introduced.

Conclusion: approved as final architecture. Do not solve this by modifying FileDocument.advanceMutation(), suppressing mutation advances on save, or introducing a second mutable Preview generation counter.

@Joncallim Joncallim left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architecture pass 2/10 — P1: precise Preview block splicing for [TOC]

Original finding: issue comment 5534696594.

Required invariant

A .block contribution replaces exactly the authored source range it owns. It may change block boundaries around that range — just as Export deliberately surrounds a block sentinel with blank lines — but it must never discard neighboring authored source merely because that source happened to parse into the same CommonMark top-level block.

For:

Alpha
[TOC]
Omega

Preview must become three source-mapped Preview blocks:

  1. authored paragraph, line 1, Alpha
  2. custom toc block, line 2, generated TOC Markdown
  3. authored paragraph, line 3, Omega

Two marker lines in one paragraph must produce two custom blocks, with every authored fragment between/around them preserved.

Files to change

  • MacDown2/MacDown2/PreviewContributionAdapter.swift
  • MacDown2/MacDown2Tests/PreviewContributionAdapterTests.swift
  • planning/epic-14-implementation.md — remove the current “accepted whole-paragraph replacement” limitation after tests pass

No change is required to PreviewBlock, TextualMarkdownPreview, ScrollSyncMap, MarkdownEngine, or ExportService.

Replace merged(...) with one composition pass

The final adapter API should be:

static func compose(
    base: [PreviewBlock]?,
    sourceText: String,
    sourceMap: SourceMap,
    contributions: [ContributionResult],
    currentGeneration: UInt
) -> Composition

Composition carries both blocks and diagnostics; later architecture passes define the diagnostic plumbing. Do not keep base.map { first placement wins }.

After validation/sorting, group accepted placements by the index of the single base block they affect. Walk base once in original order:

  • no accepted placement for this block -> append the original PreviewBlock value unchanged;
  • accepted inline-only placements -> issue 5's inline algorithm;
  • one or more block placements -> call splitBlock(...) below.

Exact block-placement normalization

For every non-empty valid block contribution range:

let startLine = sourceMap.line(atUTF16Offset: range.lowerBound)
let endLine = sourceMap.line(atUTF16Offset: range.upperBound - 1)
let lineRange = startLine ... endLine
let expected = sourceMap.utf16Range(ofLines: lineRange)

A Preview .block placement is accepted only when its sourceRange is exactly the UTF-16 range returned by sourceMap.utf16Range(ofLines: lineRange).

That rule is intentional. Preview scroll sync owns source by line, so a partial-line .block range has no unambiguous line ownership. Reject it fail-closed instead of guessing. TOCContribution already emits the full physical marker line (excluding LF, including CR for CRLF), so this does not narrow current TOC behavior.

Exact split algorithm

Add app-private helpers; naming can follow local style, but behavior must match this pseudocode:

private static func splitBlock(
    _ block: PreviewBlock,
    blockPlacements: [ValidatedPlacement],
    sourceText: String,
    sourceMap: SourceMap
) -> [PreviewBlock] {
    // placements are pre-sorted, valid, non-overlapping, and within block.lineRange
    var output: [PreviewBlock] = []
    var nextAuthoredLine = block.lineRange.lowerBound

    for placement in blockPlacements {
        if nextAuthoredLine < placement.lineRange.lowerBound {
            appendAuthoredFragment(
                block: block,
                lines: nextAuthoredLine ... (placement.lineRange.lowerBound - 1),
                sourceText: sourceText,
                sourceMap: sourceMap,
                to: &output
            )
        }

        output.append(PreviewBlock(
            kind: .custom(placement.contributionID),
            source: placement.markdown,
            lineRange: placement.lineRange
        ))

        nextAuthoredLine = placement.lineRange.upperBound + 1
    }

    if nextAuthoredLine <= block.lineRange.upperBound {
        appendAuthoredFragment(
            block: block,
            lines: nextAuthoredLine ... block.lineRange.upperBound,
            sourceText: sourceText,
            sourceMap: sourceMap,
            to: &output
        )
    }

    return output
}

appendAuthoredFragment must use the same source extraction convention as PreviewBlock.blocks(from:text:):

let nsRange = sourceMap.utf16Range(ofLines: lines)
let source = (sourceText as NSString).substring(with: nsRange)

Then construct:

PreviewBlock(kind: block.kind, source: source, lineRange: lines)

Do not attempt to splice by Swift String.Index; the contribution contract and SourceMap are UTF-16. Do not manually strip CR/LF. Using SourceMap preserves the repository's established CRLF behavior.

Container rule

A block placement that covers the entire base block line range may replace that block regardless of its kind.

A block placement that covers only part of a base block is allowed only when block.kind == .paragraph in this epic. Partial splitting of list/table/blockquote/code/HTML containers would require preserving nested container syntax and is outside this seam. Reject such a placement with a diagnostic and preserve authored source.

This restriction does not interfere with TOC: pass 3 excludes literal/code contexts semantically, and a standalone [TOC] participating in a multi-line CommonMark paragraph is exactly the paragraph case this algorithm handles.

Identity and scroll-sync requirements

  • Untouched base blocks must be appended as their existing values, retaining their deterministic IDs.
  • New split/custom blocks may use PreviewBlock's normal initializer. They are composed once per parse snapshot (pass 1), so their IDs remain stable until source changes.
  • Output line ranges must be strictly ordered and non-overlapping.
  • Every original source line owned by an affected base block must be owned by exactly one emitted block.
  • The generated custom block owns the marker's original line range; authored prefix/suffix blocks do not claim that line.

Required regression tests

Add deterministic tests using real PreviewBlock.blocks(from:text:) where possible rather than only synthetic one-line blocks:

  1. Alpha\n[TOC]\nOmega parses as one paragraph but composes to three Preview blocks and preserves Alpha/Omega.
  2. [TOC]\nMiddle\n[TOC] in one paragraph produces two TOCs plus the Middle authored fragment.
  3. Prefix only and suffix only variants.
  4. CRLF input preserves the correct authored text and disjoint line ranges.
  5. A marker that already occupies its own base block still yields one custom replacement with no empty fragments.
  6. Untouched surrounding blocks compare equal to the originals, including IDs.
  7. Resulting line ranges are sorted/non-overlapping and ScrollSyncMap.previewIndex(forSourceLine:) maps prefix/TOC/suffix to the expected block indices.
  8. Partial block placement in a non-paragraph container is rejected and original block remains unchanged.

Acceptance criteria

  • Preview and Export preserve the same authored source around the replaced range.
  • No path uses first(where:) to let one placement consume an entire containing block.
  • Multiple block placements in one paragraph work deterministically.
  • Scroll-sync line ownership remains ordered and unambiguous.
  • Existing oversize handling remains active because every generated PreviewBlock initializer recomputes isOversize from its own source.

Immediate hostile compatibility review

CommonMark semantics: PASS. Splitting authored prefix/suffix into separate preview paragraphs is deliberate for a .block replacement; Export already forces its block sentinel to standalone block context with surrounding blank lines. Preview therefore matches the contribution's declared placement instead of retaining the pre-replacement paragraph boundary.

Scroll sync: PASS with the exact full-physical-line constraint above. ScrollSyncMap binary-searches ordered lineRanges, so non-overlapping fragments are mandatory. The proposed line-oriented split satisfies it without changing ScrollSyncMap.

CRLF / Unicode: PASS. All source coordinates/extraction continue through SourceMap/NSString UTF-16 APIs already used by Preview and TOC.

View identity: PASS. Existing untouched PreviewBlock values are reused. New fragment IDs are stable for the composed parse snapshot because composition moves out of the SwiftUI body in pass 1.

Nested block structures: PASS by failing closed. We explicitly do not partially split list/table/code/HTML container blocks.

Export: PASS. No ExportService code changes; its existing exact-range composer remains the reference semantics.

Conclusion: approved as final architecture. The old “whole paragraph replacement is an accepted limitation” documentation must be deleted rather than retained as a residual risk once these regressions pass.

@Joncallim Joncallim left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architecture pass 3/10 — P1: [TOC] semantic eligibility

Original finding: issue comment 5534698236.

Required invariant

[TOC] is active contribution syntax only when the parsed Markdown document says the matching physical line is ordinary paragraph content. Literal/raw/container contexts must remain authored text.

The detector therefore has two stages:

  1. lexical eligibility — after trimming surrounding whitespace/newline characters, the physical line is exactly [TOC];
  2. semantic eligibilityMarkdownDocument.block(atLine:) reports .paragraph for that line.

Both conditions are required.

Files to change

  • MacDown2/Packages/MacDownKit/Sources/Contributions/TOCContribution.swift
  • MacDown2/Packages/MacDownKit/Tests/ContributionsTests/TOCContributionTests.swift
  • update any direct test calls to the helper's new signature

No changes to ParseEngine, MarkdownBlock, or MarkdownDocument are required.

Exact API change

Change:

static func findMarkers(in text: String, sourceMap: SourceMap) -> [Range<Int>]

to:

static func findMarkers(in text: String, document: MarkdownDocument) -> [Range<Int>]

In run(...) call:

let markers = Self.findMarkers(in: sourceText, document: document)

Do not add a second parser and do not perform ad-hoc fence tracking in TOCContribution; the caller already supplied the authoritative parsed document.

Exact detector

static func findMarkers(in text: String, document: MarkdownDocument) -> [Range<Int>] {
    let sourceMap = document.sourceMap
    let nsText = text as NSString
    var ranges: [Range<Int>] = []

    for line in 1 ... sourceMap.lineCount {
        let nsRange = sourceMap.utf16Range(ofLines: line ... line)
        let lineText = nsText.substring(with: nsRange)

        guard lineText.trimmingCharacters(in: .whitespacesAndNewlines) == "[TOC]" else {
            continue
        }

        guard let block = document.block(atLine: line),
              block.kind == .paragraph
        else {
            continue
        }

        ranges.append(nsRange.location ..< (nsRange.location + nsRange.length))
    }

    return ranges
}

If BlockKind does not support direct equality for the associated-value cases in the compiler configuration, use pattern matching instead:

guard case .paragraph = block.kind else { continue }

Prefer pattern matching because it makes the intended whitelist obvious.

Why this is a whitelist, not a blacklist

Do not implement:

if case .codeBlock = block.kind { continue }

and otherwise accept the marker. That would fix today's fenced/indented code example while leaving future raw/custom/literal block kinds accidentally active. The contribution syntax should fail closed: only the semantic context we explicitly know how to transform is eligible.

The paragraph whitelist automatically excludes:

  • fenced code blocks;
  • indented code blocks;
  • HTML blocks;
  • front matter (block(atLine:) is nil there);
  • headings / Setext-heading source;
  • thematic breaks;
  • tables;
  • custom block kinds;
  • list/blockquote syntax that does not lexically equal a plain [TOC] line in the first place.

Important retained behavior

Do not require the marker to be its own parsed paragraph block. A line can be lexically standalone while participating in a multi-line CommonMark paragraph:

Alpha
[TOC]
Omega

block(atLine:) is still .paragraph, so the marker remains eligible. Architecture pass 2 then performs precise block splicing without deleting Alpha/Omega.

Whitespace behavior also remains:

  [TOC]  

is active when parsed as a paragraph. Four-space indentation is an indented code block and is therefore correctly inactive.

Required regression tests

Update the existing findMarkers tests to parse a real document and pass that document into the helper. Add:

  1. fenced code:
    ```text
    [TOC]
    ```
    -> zero results.
  2. indented code: [TOC] -> zero results.
  3. HTML block containing an exact [TOC] line -> zero results.
  4. front matter containing [TOC] -> zero results if front matter parsing is enabled by the standard options.
  5. normal paragraph marker -> one result.
  6. two normal markers -> two results.
  7. Alpha\n[TOC]\nOmega -> marker remains detected even though it shares the paragraph block.
  8. two/three spaces of indentation -> detected when parser classifies as paragraph; four spaces -> not detected.
  9. CRLF marker -> still detected and range includes the same CR behavior as the current implementation.
  10. run(...) end-to-end on a fenced-code marker returns no contribution result.

Acceptance criteria

  • Literal code/raw text is byte-for-byte preserved by both Preview and Export.
  • TOCContribution uses the already parsed MarkdownDocument; no new parsing or fence-state implementation exists.
  • Existing valid marker syntax, whitespace tolerance, CRLF handling, and multiple markers still work.
  • Future non-paragraph block kinds are inactive by default.

Immediate hostile compatibility review

Dependency graph: PASS. Contributions already depends on MarkdownEngine; MarkdownDocument is already an input to Contributing.run, so no new module dependency is introduced.

Parser/source-map agreement: PASS. The semantic query and range extraction come from the exact same MarkdownDocument.sourceMap that was produced from sourceText; no coordinate translation is added.

CommonMark: PASS. Literal code-block contents remain literal. Whitespace up to the parser's paragraph rules is delegated to the parser instead of reimplemented.

Pass 2 interaction: PASS. Paragraph-only semantic eligibility intentionally retains the multi-line-paragraph case that precise Preview splitting now supports.

Export parity: PASS. Because marker discovery occurs before either adapter, Preview and Export receive the same eligible results and therefore stop rewriting code blocks together.

Performance: PASS. The current implementation already loops over every physical line. block(atLine:) walks parsed block structure; for normal documents this remains bounded, but if profiling later shows pathological cost, optimize MarkdownDocument lookup centrally rather than adding a second TOC parser. No optimization is required for this correctness patch.

Conclusion: approved as final architecture. Implement semantic eligibility as a positive .paragraph whitelist; do not maintain a growing blacklist of forbidden Markdown constructs.

@Joncallim Joncallim left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architecture pass 4/10 — P1: restore the build-and-test gate deterministically

Original finding: issue comment 5534700275.

Required invariant

The PR is not mergeable until the repository's existing canonical CI commands pass from a clean checkout at the PR head. Do not weaken, skip, conditionalize, or remove the UI-test/build-for-testing stage to make the check green.

The evidence boundary is clear:

  • base bbfb010074fbe44201ab6a8b1c4d6244c51ecf18: lint + build-and-test green;
  • head a7aac6e44254817bb6679950cd24499fe92c669d: lint green, build-and-test red;
  • the failure is in the app/UI-test build portion, after package work that otherwise passes.

The GitHub job log currently available through the connector does not expose a trustworthy first compiler/linker diagnostic. Therefore this pass deliberately does not guess the root cause.

Sources of truth

  1. .github/workflows/ci.yml — exact CI command sequence and environment contract.
  2. MacDown2/project.yml — XcodeGen source of truth for targets/dependencies/settings.
  3. Generated .xcodeproj — an output; never make a durable project fix only here.
  4. PR head source — code under test.

Exact implementation procedure

The coding agent should perform these steps in this order from a clean worktree at the PR head.

Step A — reproduce the failing stage exactly

  1. Read .github/workflows/ci.yml and copy the exact XcodeGen/bootstrap and xcodebuild ... build-for-testing/test command used by build-and-test.
  2. Remove any locally generated project/build products that the workflow recreates.
  3. Regenerate the Xcode project exactly as CI does from project.yml.
  4. Run the exact first failing CI command, without adding flags that suppress diagnostics.
  5. Capture the first deterministic compiler/linker/project-generation error plus 20–30 surrounding lines.

Do not start by running an alternate Xcode scheme or only swift test; those have already failed to reproduce the gate that is red.

Step B — classify the first error

Use this decision table and fix only the corresponding source of truth:

1. Swift source import/name-resolution error in a changed app-test file

Example class: cannot find 'URL' in scope, missing type/module visible only when MacDown2Tests compiles.

  • Fix the source file itself with the explicit required import.
  • One high-probability file to check first is MacDown2/MacDown2Tests/ExportContributionAdapterTests.swift: it uses URL(fileURLWithPath:) and, at this head, does not explicitly import Foundation.
  • This is a candidate, not a pre-proven root cause. Add import Foundation only if the reproduced compiler error confirms that namespace/import failure (or the compiler otherwise requires it under the repository's explicit-import policy).
  • Do not change target dependencies for a source-level import error.

2. No such module Contributions / No such module Preview / product-not-linked error for MacDown2Tests

  • Inspect MacDown2/project.yml target MacDown2Tests dependencies.
  • The PR already adds Contributions and Preview; correct omissions or target placement in project.yml only.
  • Regenerate the project.
  • Do not hand-edit project.pbxproj as the durable fix.

3. Duplicate product/framework/link symbol error

  • Compare direct dependencies of MacDown2 and MacDown2Tests with the SwiftPM product graph.
  • Remove only genuinely redundant direct target dependency entries from project.yml; do not remove a product merely because the app target also links it if the test target imports that product directly and Xcode requires explicit linkage.
  • Regenerate and rerun the exact command.

4. XcodeGen schema/project-generation failure

  • Repair project.yml syntax/product reference.
  • Regenerate before attempting any Xcode build.

5. UI-test host/runner configuration error unrelated to module compilation

  • Compare the generated UI-test target settings between base and head using project.yml-generated projects.
  • The changed project.yml dependency additions are the likely delta; do not alter bundle IDs, host app, signing, deployment target, or test-host settings unless the generated diff proves they changed.

6. Failure is transient/environmental and exact rerun passes

  • Rerun the exact failing command at least once from a fresh generated project.
  • Then run the whole canonical workflow. Only classify as transient if the full workflow is green without source/config changes and there is evidence the original failure was infrastructure-related.
  • Do not waive a consistently reproducible failure as flaky.

Repair loop

After each minimal repair:

  1. regenerate project if project.yml changed;
  2. rerun the exact formerly failing build-for-testing command;
  3. stop and inspect the new first error if one remains;
  4. do not batch speculative fixes.

When the targeted command is green, run the complete gate set in this order:

  1. all SwiftPM package tests used by CI;
  2. formatter check;
  3. strict SwiftLint check;
  4. app build in the exact CI configuration;
  5. CLI build if the workflow builds it separately;
  6. app unit tests / build-for-testing;
  7. UI tests or test-without-building stage exactly as CI specifies;
  8. finally the full workflow command sequence from a clean generated state.

Required evidence to record

Update the PR verification section with:

  • failing head SHA and original failing check/run;
  • reproduced command;
  • exact first error category;
  • minimal fix made and file(s);
  • green local command output summary;
  • green GitHub Actions run URL/check after push.

If the actual root cause is different from all decision-table categories, record it explicitly rather than forcing it into the nearest guess.

Regression protection

No new test is necessary merely to test an import/project-linking correction if the existing CI build stage is the regression test. If the failure reveals a dependency-generation defect that could recur silently, add the smallest project-generation assertion/check only if the repository already has a pattern for such checks; do not create a new build-system test framework in this PR.

Acceptance criteria

  • build-and-test is green on the PR head after the repair.
  • lint remains green.
  • no CI stage has been skipped/weakened.
  • project.yml and generated project are in sync if project structure changed.
  • clean-checkout reproduction passes.
  • package tests and app/CLI builds still pass.

Immediate hostile compatibility review

Evidence integrity: PASS. The plan explicitly separates proven regression boundaries from an unproven root cause, preventing a coding agent from implementing a plausible but wrong project edit.

Build-system source of truth: PASS. Durable target/dependency changes go through project.yml; this preserves XcodeGen regeneration and avoids pbxproj drift.

Existing CI contract: PASS. No workflow relaxation is allowed; the existing red gate remains the acceptance test.

Changed test dependencies: PASS. The decision tree acknowledges that the PR intentionally added Contributions/Preview to the app/test graph and only removes/changes them if the exact build diagnostic proves the graph wrong.

Scope control: PASS. A source import error stays a source fix; a target graph error stays a project.yml fix. This prevents a one-line compiler problem becoming an architectural dependency change.

Potential Foundation import: CONDITIONALLY PASS only when reproduced. ExportContributionAdapterTests.swift is worth checking first because it directly uses URL, but the architecture forbids applying that hypothesis without compiler evidence.

Conclusion: approved as final architecture. The coding agent's first action is exact CI reproduction, not code editing; after the first real diagnostic appears, the repair path is deterministic.

@Joncallim Joncallim left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architecture pass 5/10 — P2: honor ContributionPlacement.inline in Preview

Original finding: issue comment 5534701855.

Required invariant

Preview must never reinterpret .inline as .block.

For a valid inline contribution, Preview replaces exactly that UTF-16 source range inside the existing Preview block source and preserves:

  • the containing block's source line ownership;
  • its PreviewBlock.Kind;
  • all authored source outside the range;
  • the surrounding block structure.

If Preview cannot safely represent a particular inline result, it must preserve authored source and surface a diagnostic. Silent promotion to whole-block replacement is forbidden.

Files to change

  • MacDown2/MacDown2/PreviewContributionAdapter.swift
  • MacDown2/MacDown2Tests/PreviewContributionAdapterTests.swift

No public ContributionPlacement change is required. No Preview-package change is required.

Supported inline capability for this epic

An inline placement is Preview-placeable only when all of the following hold after the general range validation in pass 6:

  1. the range is non-empty and within the source;
  2. lower and upper-1 map to the same physical source line;
  3. the whole range belongs to exactly one existing top-level PreviewBlock line range;
  4. the generated representation is .markdown;
  5. the generated Markdown contains no \n or \r;
  6. it does not overlap any accepted block or inline contribution.

The single-line/no-line-break rules are adapter capability limits, not changes to the renderer-neutral public protocol. A future Preview implementation that can safely support multi-line inline content may widen them later.

Internal validated model

Inside PreviewContributionAdapter, normalize accepted candidates into a renderer-private type; do not repeatedly reinterpret raw ContributionResult values during composition:

private struct ValidatedPlacement {
    let resultIndex: Int
    let contributionID: String
    let sourceRange: Range<Int>
    let lineRange: ClosedRange<Int>
    let baseBlockIndex: Int
    let placement: ContributionPlacement
    let markdown: String
}

For .inline, lineRange is always line ... line.

Exact source-coordinate calculation

For a candidate assigned to base[baseBlockIndex]:

let block = base[baseBlockIndex]
let blockNSRange = sourceMap.utf16Range(ofLines: block.lineRange)
let blockRange = blockNSRange.location ..< (blockNSRange.location + blockNSRange.length)

Require:

blockRange.lowerBound <= candidate.sourceRange.lowerBound
candidate.sourceRange.upperBound <= blockRange.upperBound

Then convert to block-relative UTF-16 offsets:

let relativeRange =
    (candidate.sourceRange.lowerBound - blockRange.lowerBound)
        ..<
    (candidate.sourceRange.upperBound - blockRange.lowerBound)

Never derive relative coordinates from Swift character counts. The public contract is UTF-16.

Exact inline splice helper

Implement one pass over UTF-16 units. Do not repeatedly mutate a Swift String, because multiple replacements would invalidate later indexes and can become quadratic.

private struct InlineSplice {
    let range: Range<Int>     // block-relative UTF-16
    let markdown: String
}

private static func applyingInlineSplices(
    _ splices: [InlineSplice],
    to source: String
) -> String {
    guard !splices.isEmpty else { return source }

    let units = Array(source.utf16)
    var output = ""
    output.reserveCapacity(source.utf8.count)
    var cursor = 0

    for splice in splices { // pre-sorted, non-overlapping
        if cursor < splice.range.lowerBound {
            output += String(
                decoding: units[cursor ..< splice.range.lowerBound],
                as: UTF16.self
            )
        }
        output += splice.markdown
        cursor = splice.range.upperBound
    }

    if cursor < units.count {
        output += String(decoding: units[cursor ..< units.count], as: UTF16.self)
    }
    return output
}

Use the same UTF-16 strategy already established by DerivedContentComposer; do not introduce grapheme-cluster indexing into this path.

Composition behavior — inline-only block

When a base block has one or more accepted inline placements and no accepted block placement that divides it:

  1. sort inline placements by sourceRange.lowerBound, then upper bound, then original result index;
  2. convert them to block-relative InlineSplices;
  3. create one replacement PreviewBlock:
PreviewBlock(
    kind: block.kind,
    source: applyingInlineSplices(splices, to: block.source),
    lineRange: block.lineRange
)

Do not set .custom; this remains the same authored block with renderer-neutral inline content substituted into it.

Composition behavior when the same original paragraph also has block placements

Perform composition in source order, not in two independent passes.

Pass 2 splits a paragraph around accepted block-placement line ranges. For each authored prefix/middle/suffix fragment it emits, apply only the inline placements whose sourceRange is wholly inside that fragment's source-line range before constructing the fragment PreviewBlock.

An inline placement that intersects a block placement is already rejected by the global overlap validator in pass 6. It must never be partially applied.

This ordering ensures, for example:

Before *INLINE*
[TOC]
After *INLINE*

can produce authored prefix-with-inline, custom TOC, authored suffix-with-inline without losing line ownership.

Unsupported inline results

Reject and preserve source with an adapter diagnostic when:

  • source range spans more than one physical line;
  • generated Markdown contains \r or \n;
  • source range spans/lands outside a single base block;
  • source lies in a blank/inter-block region;
  • representation is HTML;
  • range overlaps another accepted placement;
  • general source validation fails.

Suggested message form:

<id> inline preview contribution spans multiple lines; authored source preserved

Use deterministic, testable messages; do not include memory addresses or localized parser descriptions.

Required regression tests

Add tests with manually constructed ContributionResults because TOC itself is block-only:

  1. inline range in Hello **marker** world replaces only the marker and preserves the rest.
  2. two ordered inline ranges in one paragraph both apply.
  3. Unicode before the range proves UTF-16 coordinates are honored (😀 is two UTF-16 code units).
  4. CRLF document with an inline placement still produces correct source.
  5. generated inline Markdown containing \n is rejected and original block is unchanged.
  6. source range spanning two physical lines is rejected.
  7. range crossing two top-level blocks is rejected.
  8. .inline never produces .custom kind.
  9. output lineRange equals the original containing block's lineRange.
  10. inline + block contributions in the same multi-line paragraph compose correctly around the block split.
  11. overlapping inline-inline and inline-block cases preserve the rejected authored source according to pass 6's deterministic overlap policy.
  12. stale inline generation does not place.

Acceptance criteria

  • Preview and Export both honor the public inline/block distinction.
  • Inline replacement changes only the declared source range.
  • No whole-block replacement occurs merely because placement is .inline.
  • Scroll-sync line ownership is unchanged for inline-only blocks.
  • Unsupported inline shapes fail closed with a visible diagnostic.
  • Unicode/CRLF behavior is covered.

Immediate hostile compatibility review

Public API: PASS. ContributionPlacement remains unchanged; this only makes Preview implement the semantics it already advertises.

Preview renderer: PASS. TextualMarkdownPreview already renders each PreviewBlock.source as Markdown. Reconstructing the containing block source is therefore sufficient; no inline-renderer API needs to be added.

Scroll sync: PASS. Inline-only composition retains the original lineRange, so source-line-to-preview-block mapping is unchanged.

CommonMark/block safety: PASS with the single-line/no-generated-linebreak constraints. We do not allow an inline contribution to create a new physical line and thereby claim block semantics through the inline channel.

Unicode: PASS. Relative ranges and splicing remain UTF-16, matching SourceMap, ContributionContent, and Export's existing composer.

Mixed inline/block: PASS only with one global source-ordered validation/composition plan. Do not implement separate independent inline and block mutation passes that could apply overlapping edits twice.

Future extensions: PASS. Single-line inline Markdown covers the expected math-like inline use case. Wider multi-line inline behavior remains additive rather than being silently approximated now.

Performance: PASS. One UTF-16 materialization per affected block and one linear splice is bounded. No reparse beyond the normal per-block Textual rendering is introduced.

Conclusion: approved as final architecture. .inline must either be precisely spliced under these constraints or rejected; it must never be promoted to .block.

@Joncallim Joncallim left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architecture pass 6/10 — P2: fail-closed Preview source-range validation

Original finding: issue comment 5534703764.

Required invariant

No untrusted/buggy contribution coordinate reaches SourceMap.line(atUTF16Offset:) or Preview block lookup until it has passed strict integer/source bounds validation. Every invalid, stale, unsupported, or overlapping placement preserves the authored source it would otherwise affect and produces one deterministic adapter diagnostic.

Preview should follow the same safety posture as DerivedContentComposer: malformed derived content is rejected; authored Markdown remains authoritative.

Files to change

  • MacDown2/MacDown2/PreviewContributionAdapter.swift
  • MacDown2/MacDown2Tests/PreviewContributionAdapterTests.swift

Do not change SourceMap.line(atUTF16Offset:) to stop clamping. Its documented clamping behavior is valid for general callers; the defect is that the Preview adapter currently treats it as validation.

One normalization pipeline

compose(...) must have exactly one validation/admission stage before block mutation. Do not scatter ad-hoc guards through splitBlock, inline splicing, and SwiftUI code.

Recommended private types:

private struct Candidate {
    let resultIndex: Int
    let contributionID: String
    let sourceRange: Range<Int>
    let placement: ContributionPlacement
    let markdown: String
}

private struct ValidatedPlacement {
    let resultIndex: Int
    let contributionID: String
    let sourceRange: Range<Int>
    let lineRange: ClosedRange<Int>
    let baseBlockIndex: Int
    let placement: ContributionPlacement
    let markdown: String
}

Candidate exists only after source-independent checks; ValidatedPlacement exists only after coordinate and Preview ownership checks.

Stage 0 — source-map integrity

Before validating any placement:

let sourceUTF16Length = sourceText.utf16.count
guard sourceUTF16Length == sourceMap.utf16Length else {
    return Composition(
        blocks: base,
        diagnostics: forwardedDiagnostics + [
            .error(
                contributionID: nil,
                message: "Preview contribution source map does not match source text; authored source preserved"
            )
        ]
    )
}

Do not attempt partial placement against mismatched source/map snapshots.

Stage 1 — always forward renderer diagnostics

For every ContributionResult, append all existing result.diagnostics to the Composition.diagnostics output before inspecting content.

This is necessary for issue 9 and also ensures a result with content == nil remains observable.

If content == nil, stop processing that result after forwarding diagnostics. It creates no placement.

Stage 2 — snapshot and representation checks

For each content-bearing result, in original result order:

  1. result.sourceGeneration == currentGeneration; otherwise reject with an error diagnostic and preserve source.
  2. representation must be .markdown; .html is unsupported by current Preview and must be rejected with an explicit error diagnostic, not silently ignored.
  3. capture a Candidate with original resultIndex.

Suggested stale message:

<id> preview contribution is stale; authored source preserved

Suggested HTML message:

<id> produced HTML that Preview does not support; authored source preserved

Stage 3 — integer/source range validation

For every candidate, check in this exact order before any line lookup:

let range = candidate.sourceRange

guard range.lowerBound >= 0 else { reject("range starts before source") }
guard range.lowerBound < range.upperBound else { reject("range is empty") }
guard range.upperBound <= sourceMap.utf16Length else { reject("range exceeds source") }

An Int overflow is not introduced by these comparisons; do not add/subtract until the bounds are proven.

Only after these guards may code call:

let startLine = sourceMap.line(atUTF16Offset: range.lowerBound)
let endLine = sourceMap.line(atUTF16Offset: range.upperBound - 1)

Stage 4 — Preview ownership validation

Find a single top-level base block whose lineRange contains both startLine and endLine:

let matchingBlockIndices = base.indices.filter {
    base[$0].lineRange.contains(startLine)
        && base[$0].lineRange.contains(endLine)
}

guard matchingBlockIndices.count == 1,
      let blockIndex = matchingBlockIndices.first
else { reject("range is not owned by one preview block") }

In production, this can be implemented as one firstIndex(where:) plus a debug assertion that no second block matches; top-level parsed block line ranges are expected to be disjoint. The semantic requirement is exactly one owner.

This rejects:

  • blank/inter-block source regions;
  • a contribution spanning two top-level blocks;
  • front matter or any source region not represented by Preview blocks;
  • coordinates that would otherwise clamp onto an unrelated boundary block.

Then derive the base block's physical UTF-16 span:

let blockNSRange = sourceMap.utf16Range(ofLines: base[blockIndex].lineRange)
let blockRange = blockNSRange.location ..< (blockNSRange.location + blockNSRange.length)

guard blockRange.lowerBound <= range.lowerBound,
      range.upperBound <= blockRange.upperBound
else { reject("range exceeds preview block source") }

The second guard is intentionally redundant with line ownership: it protects against future changes in line-range semantics and makes relative offset arithmetic safe.

Stage 5 — placement-specific capability validation

For .block:

  • derive lineRange = startLine ... endLine;
  • calculate expected = sourceMap.utf16Range(ofLines: lineRange);
  • require sourceRange to equal that full physical-line UTF-16 range exactly;
  • if placement covers only part of the containing base block's lineRange, require base[blockIndex].kind == .paragraph;
  • otherwise reject and preserve source.

For .inline: use pass 5 requirements:

  • startLine == endLine;
  • generated Markdown contains neither CR nor LF;
  • source range remains wholly inside one block;
  • lineRange = startLine ... startLine.

Only now construct ValidatedPlacement.

Stage 6 — deterministic ordering and overlap rejection

Sort validated placements by:

  1. sourceRange.lowerBound ascending;
  2. sourceRange.upperBound ascending;
  3. original resultIndex ascending.

Then scan once:

var accepted: [ValidatedPlacement] = []
var previousUpperBound: Int?

for placement in sorted {
    if let previousUpperBound,
       placement.sourceRange.lowerBound < previousUpperBound {
        reject placement with "overlaps another preview contribution"
        continue
    }
    accepted.append(placement)
    previousUpperBound = placement.sourceRange.upperBound
}

Adjacent ranges (lower == previousUpperBound) are valid.

Do not merge overlapping contributions and do not choose by contribution ID. Earliest source-ordered valid range wins deterministically; later overlap is rejected.

If an invalid candidate was rejected before this stage, it does not participate in overlap state and cannot block a later valid result.

Stage 7 — resource-budget admission

Only the valid, non-overlapping accepted array proceeds to pass 7's Preview budget. Invalid/stale/unsupported results must not consume budget slots.

Diagnostics severity policy

Use .error for:

  • malformed/out-of-bounds/empty range;
  • unsupported representation or placement shape;
  • overlap;
  • stale source generation;
  • source/map integrity mismatch.

Use .warning for resource-budget truncation (pass 7), because the contribution is otherwise valid and authored source is deliberately preserved.

Do not localize these internal diagnostic strings in this epic unless the existing contribution diagnostic infrastructure is already localized; tests need deterministic messages.

Required regression tests

Add focused cases proving both rejection and source preservation:

  1. negative lower bound, e.g. -1 ..< 4;
  2. zero-width 0 ..< 0;
  3. upper bound beyond sourceMap.utf16Length;
  4. fully beyond EOF;
  5. valid bounds that land on a blank line between Preview blocks;
  6. range spanning two top-level blocks;
  7. block placement that covers only part of a physical line;
  8. partial block placement inside a non-paragraph container;
  9. unsupported HTML representation;
  10. stale generation;
  11. overlapping inline-inline;
  12. overlapping block-block;
  13. overlapping inline-block;
  14. adjacent non-overlapping placements both succeed;
  15. malformed first result followed by valid result: valid result still places and invalid one does not consume overlap/budget state;
  16. sourceText/sourceMap mismatch causes all placement to fail closed with base unchanged;
  17. Unicode around range boundaries proves bounds use UTF-16 length, not character count.

For every rejection test assert both:

  • original authored Preview source remains present/unmodified at that location;
  • expected diagnostic is emitted exactly once.

Acceptance criteria

  • No invalid range is passed into SourceMap.line(...).
  • No malformed contribution can be clamped into the first/last Preview block.
  • Every placeable contribution belongs to exactly one base block.
  • Overlap handling is deterministic and source preserving.
  • Invalid results neither consume the Preview budget nor prevent later valid results.
  • Preview safety semantics are at least as fail-closed as Export's existing composer.

Immediate hostile compatibility review

SourceMap behavior: PASS. We leave its documented clamping API intact and put strict validation at the derived-content trust boundary where it belongs.

Contribution public API: PASS. ContributionContent remains a lightweight public value and does not become failable/throwing. This mirrors Export's current approach: producers may be buggy; renderer adapters validate before placement.

Preview block model: PASS. Validation uses existing top-level line ownership and never requires new fields on PreviewBlock.

ExportService dependency direction: PASS. We intentionally do not extract DerivedContentComposer or ExportResourceBudget into Contributions; Export has body-relative offsets and HTML-specific budgets, while Preview has block ownership constraints. Sharing those concrete validators would create the wrong abstraction/dependency pressure.

Overlap determinism: PASS. Explicit source/upper/index sorting is stable independent of Swift sort behavior for equal source starts.

Failure semantics: PASS. Authored Markdown is always the fallback. No rejected result causes an empty synthetic block.

Performance: PASS. Validation is linear after sorting (O(n log n) for at most the contribution result set), and Preview budget limits final placement count. Base block owner lookup can remain a small linear scan now; if future result counts justify it, add a binary lookup using ordered line ranges without changing semantics.

Conclusion: approved as final architecture. Treat the validation/admission pipeline as the single trust boundary; composition helpers must accept only ValidatedPlacement, never raw ContributionResult.

@Joncallim Joncallim left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architecture pass 7/10 — P2: Preview contribution resource policy

Original finding: issue comment 5534706013.

Required invariant

Preview may impose a tighter resource budget than Export because Preview is a hot-path, repeatedly recomputed surface while Export is a one-shot operation. However:

  1. invalid, stale, diagnostic-only, unsupported, or overlapping results must not consume Preview budget;
  2. budget rejection must preserve the authored source range;
  3. budget rejection must be observable, never silent;
  4. the budget must bound generated payload as well as placement count;
  5. do not raise Preview's count limit to Export's 4096 merely for numerical symmetry.

Export's existing policy is the design precedent, not the numeric policy: it validates first, admits within count/aggregate-byte limits, preserves authored source on rejection, and emits diagnostics.

Files to change

  • MacDown2/MacDown2/PreviewContributionAdapter.swift
  • MacDown2/MacDown2Tests/PreviewContributionAdapterTests.swift
  • planning/epic-14-implementation.md — document the deliberate Preview-vs-Export budget distinction

Do not change ExportResourceBudget and do not make Preview import ExportService.

Final app-private budget type

Replace the free-floating maxMergedResults constant with an adapter-owned value type:

extension PreviewContributionAdapter {
    struct ResourceBudget: Sendable, Equatable {
        let maxPlacedContributionCount: Int
        let maxAggregateGeneratedMarkdownUTF8Bytes: Int

        static let standard = ResourceBudget(
            maxPlacedContributionCount: 64,
            maxAggregateGeneratedMarkdownUTF8Bytes:
                64 * PreviewBlock.oversizeByteThreshold
        )
    }
}

PreviewBlock.oversizeByteThreshold is already the repository's established 64 KiB boundary protecting Textual from pathological block inputs. Deriving the aggregate generated-Markdown ceiling from 64 placements × that existing threshold yields a 4 MiB cap without inventing another unrelated size constant.

Do not add a per-fragment rejection at 64 KiB. PreviewBlock already handles an individual oversized block safely by setting isOversize and causing Preview to degrade that block to plain text rather than feeding it to Textual. The aggregate budget is for composition/memory growth; the existing per-block policy remains responsible for renderer safety.

Tests may inject a deliberately tiny ResourceBudget; production callers use .standard only.

Exact admission ordering

Pass 6's validation pipeline remains the single trust boundary. Budget admission is its last stage:

  1. matching source-generation token;
  2. result has content and Markdown representation;
  3. strict non-empty UTF-16 document bounds;
  4. physical-line mapping;
  5. exactly one containing base block;
  6. placement capability (.block/.inline constraints from passes 2 and 5);
  7. deterministic overlap check against already accepted ranges;
  8. budget admission.

A result rejected at stages 1–7 consumes zero count and zero generated-byte budget.

Do not implement contributions.prefix(64) anywhere.

Exact budget state

Keep bounded state inside the composition plan:

private struct CompositionPlan {
    var accepted: [ValidatedPlacement] = []
    var diagnostics: [Diagnostic] = []
    var aggregateGeneratedBytes = 0
    var omittedForCount = 0
    var omittedForBytes = 0
}

For a candidate that passed all semantic/range/overlap validation:

mutating func admit(
    _ placement: ValidatedPlacement,
    budget: PreviewContributionAdapter.ResourceBudget
) {
    guard accepted.count < budget.maxPlacedContributionCount else {
        omittedForCount += 1
        return
    }

    let generatedBytes = placement.markdown.utf8.count
    let (total, overflowed) = aggregateGeneratedBytes.addingReportingOverflow(generatedBytes)
    guard !overflowed,
          total <= budget.maxAggregateGeneratedMarkdownUTF8Bytes
    else {
        omittedForBytes += 1
        return
    }

    accepted.append(placement)
    aggregateGeneratedBytes = total
}

Use overflow-safe arithmetic. A malicious/buggy contribution must not wrap the aggregate counter.

Diagnostic cardinality

Do not emit one diagnostic for every omitted contribution after the limit is reached; a pathological input could turn the diagnostic array into the new unbounded resource problem.

After the candidate pass completes, emit at most one summary diagnostic per violated budget dimension:

Preview contribution limit reached: 64 valid placements were admitted and N additional placements were preserved as authored source.

and, when applicable:

Preview generated-content limit reached: N valid placements were preserved as authored source because the aggregate generated Markdown exceeded 4 MiB.

The exact human-readable byte formatter may remain simple/app-private; do not import ExportService merely to reuse ExportResourceBudget.describe.

These diagnostics are appended to the Composition.diagnostics channel finalized in pass 9.

Important overlap behavior after budget rejection

A valid range rejected only by budget must remain authored. It must also not become the previousAcceptedUpperBound for overlap purposes, because no replacement was admitted there. A later valid non-overlapping candidate is still eligible for admission if budget permits.

Conversely, once the count cap is full there is no reason to allocate more ValidatedPlacement values; continue only the minimum validation needed to count budget omissions / collect pre-existing diagnostics. Keep accepted bounded at 64.

For the aggregate-byte budget, a single oversized candidate may be rejected while a later small non-overlapping candidate can still be admitted if it fits the remaining byte budget. Do not make one byte-budget rejection permanently close the gate.

Composition behavior

Only plan.accepted is passed to the block/inline composition algorithms from passes 2 and 5. Therefore:

  • a rejected standalone marker remains its original authored [TOC] source;
  • if an accepted marker and a budget-rejected marker share one paragraph, the paragraph split occurs only around the accepted range and the rejected marker remains inside the appropriate authored fragment;
  • no synthetic blank line/source line is invented for rejected work.

Required regression tests

Use injectable small budgets for most boundary tests, plus one production-limit test:

  1. 65 otherwise-valid standalone markers under .standard: exactly 64 contributions are placed; marker 65 remains authored; exactly one count-budget diagnostic is emitted.
  2. 64 diagnostic-only/stale/invalid results followed by one valid result: the valid result is admitted because ineligible results consume no budget.
  3. Tiny count budget 2: three valid ranges -> first two admitted deterministically, third preserved, one summary diagnostic.
  4. Tiny aggregate-byte budget: first small generated fragment admitted, oversized next fragment rejected, later small fragment still admitted if it fits.
  5. Integer-overflow path for aggregate bytes is rejected fail-closed (exercise via a test budget/state seam rather than allocating enormous strings if practical).
  6. Accepted + budget-rejected markers in the same CommonMark paragraph preserve the rejected marker and all neighboring authored text.
  7. A generated fragment larger than PreviewBlock.oversizeByteThreshold but below the aggregate budget is allowed to form a PreviewBlock whose isOversize == true; it is not rejected by this adapter merely for being >64 KiB.
  8. Invalid/overlapping ranges do not alter the admitted-count or aggregate-byte counters.
  9. Diagnostics are bounded to one count summary + one byte summary regardless of how many contributions are omitted.

Acceptance criteria

  • There is no prefix(maxMergedResults) or equivalent pre-validation truncation.
  • Preview intentionally keeps a 64-placement hot-path budget; Export remains at its existing 4096-fragment one-shot budget.
  • Every budget-rejected range remains authored source.
  • Invalid/stale/non-renderable work cannot starve valid later contributions.
  • Generated payload is bounded in aggregate with overflow-safe arithmetic.
  • Oversized individual Preview blocks continue using the repository's existing PreviewBlock.isOversize fallback.
  • Budget diagnostics are explicit and bounded in cardinality.

Immediate hostile compatibility review

Preview/Textual safety: PASS. The new aggregate budget complements rather than replaces PreviewBlock.oversizeByteThreshold; oversized individual blocks still take the existing plain-text fallback.

Export behavior: PASS. No ExportService code or budget changes. The surfaces intentionally have different numeric limits because their execution profiles differ.

Dependency graph: PASS. ResourceBudget stays app-private in the adapter and depends only on Preview's already-imported PreviewBlock; no Preview → ExportService coupling is added.

Authored-source preservation: PASS. Only admitted placements reach composition. Rejected placements have no splice/split side effect.

Diagnostics DoS: PASS with the summary-cardinality rule. Thousands of rejected markers cannot produce thousands of budget diagnostics.

Performance: PASS. Accepted placement storage remains bounded at 64. Byte accounting is linear in generated content already materialized by the contribution result; no extra parse or source copy is introduced.

Conclusion: approved as final architecture. Keep the tighter Preview count ceiling, apply it only after correctness validation, add a derived aggregate-byte ceiling, and make every omission explicit and fail-closed.

@Joncallim Joncallim left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architecture pass 8/10 — P2: preserve structured cancellation through Preview publication

Original finding: issue comment 5534708039.

Required invariant

Cancellation is control flow, not an empty successful contribution result.

Once a Preview contribution task is superseded, no code path from that task may publish blocks, diagnostics, or an empty replacement state, even when a contributor itself does not promptly cooperate with cancellation.

There are three defensive layers:

  1. contributors cooperate while doing long work;
  2. ContributionRegistry checks cancellation before and after every contributor and before converting an ordinary thrown error into a diagnostic;
  3. DocumentEditorSplitView checks cancellation + captured snapshot identity immediately before state publication.

All three are required. Do not rely on SwiftUI .task(id:) cancellation alone.

Files to change

  • MacDown2/Packages/MacDownKit/Sources/Contributions/ContributionRegistry.swift
  • MacDown2/Packages/MacDownKit/Sources/Contributions/TOCContribution.swift
  • MacDown2/Packages/MacDownKit/Sources/Contributions/DeterministicTestContribution.swift
  • MacDown2/Packages/MacDownKit/Tests/ContributionsTests/ContributionRegistryTests.swift
  • MacDown2/Packages/MacDownKit/Tests/ContributionsTests/TOCContributionTests.swift
  • MacDown2/MacDown2/PreviewContributionAdapter.swift
  • MacDown2/MacDown2/DocumentEditorSplitView.swift
  • MacDown2/MacDown2Tests/PreviewContributionAdapterTests.swift

No new actor, task manager, or generation subsystem is needed.

1. Registry: cancellation checks bracket each contributor

Change the loop in ContributionRegistry.run to this exact control-flow shape:

for contribution in contributions {
    try Task.checkCancellation()

    do {
        let contributed = try await contribution.run(
            document: document,
            sourceText: sourceText,
            sourceGeneration: sourceGeneration
        )

        // A contributor may have ignored cancellation and returned normally.
        // Never append its result once the parent task is cancelled.
        try Task.checkCancellation()
        results.append(contentsOf: contributed)
    } catch is CancellationError {
        throw CancellationError()
    } catch {
        // Cancellation may have happened while a non-cancellation error was
        // being produced. Cancellation wins over failure isolation.
        try Task.checkCancellation()

        let diagnostic = ContributionDiagnostic(
            severity: .error,
            message: "\(contribution.id) failed: \(error.localizedDescription)"
        )
        results.append(ContributionResult(
            contributionID: contribution.id,
            content: nil,
            sourceGeneration: sourceGeneration,
            diagnostics: [diagnostic]
        ))
    }
}

// Close the tiny window after the final contributor and before returning.
try Task.checkCancellation()
return results

This preserves the existing isolation contract for ordinary errors. The only changed rule is that cancellation always outranks isolation.

Do not catch CancellationError and translate it into a ContributionResult.

2. Preview adapter: make cancellation throwable, not representable as []

Change:

static func results(
    document: MarkdownDocument?,
    text: String?,
    generation: UInt
) async -> [ContributionResult]

to the stricter API:

static func results(
    document: MarkdownDocument,
    text: String,
    generation: UInt
) async throws -> [ContributionResult] {
    try await ContributionRegistry.standard.run(
        document: document,
        sourceText: text,
        sourceGeneration: generation
    )
}

Remove the broad do/catch { return [] } entirely.

The optional-input behavior belongs at the view/snapshot boundary, where pass 1 already guards parseSession.document + publishedText together. An absent source snapshot is not the same state as “registry ran successfully and produced no results.”

Update/remove the current resultsIsEmptyWithoutADocumentOrText test accordingly; replace it with a test that proves registry cancellation propagates through the adapter unchanged.

3. Current TOC contributor: cooperate during its only potentially long scan

Pass 3 changes marker discovery to consume MarkdownDocument. Make that helper throwing:

static func findMarkers(
    in text: String,
    document: MarkdownDocument
) throws -> [Range<Int>]

Inside the physical-line loop:

for line in 1 ... sourceMap.lineCount {
    try Task.checkCancellation()
    // existing lexical + semantic eligibility checks
}

Then run(...) becomes:

try Task.checkCancellation()
let markers = try Self.findMarkers(in: sourceText, document: document)
try Task.checkCancellation()

guard !markers.isEmpty else { return [] }

let list = Self.markdownList(for: document.headings)
try Task.checkCancellation()

let results = markers.map { ... }
try Task.checkCancellation()
return results

Keep markdownList(for:) pure/non-throwing. Its work is bounded by the already-parsed heading list, and the checks immediately before/after it are sufficient for this implementation. Do not infect unrelated pure formatting helpers with async/throws solely for cancellation.

All direct findMarkers tests become try calls.

4. Deterministic regression seam: simulate a contributor that ignores cancellation

The existing .hangs case proves cooperative cancellation because Task.sleep throws. It does not prove the new post-contributor guard.

Add one behavior:

case returnsAfterCancellation(ContributionContent)

Implementation:

case let .returnsAfterCancellation(content):
    while !Task.isCancelled {
        await Task.yield()
    }
    // Deliberately ignore the cancellation and return a value.
    return [ContributionResult(
        contributionID: id,
        content: content,
        sourceGeneration: sourceGeneration
    )]

This is intentionally bad contributor behavior used only as a deterministic test seam. It does not require timing sleeps or continuations: the contributor returns only after cancellation has definitely happened.

Update its documentation to make clear that production contributors must not emulate it.

5. View publication: captured snapshot + cancellation is the final authority

Use the refreshPreviewContributions() architecture from pass 1. Capture the exact identity and parse revision before awaiting:

private func refreshPreviewContributions() async {
    let taskIdentity = identity

    guard let parsed = parseSession.document,
          let sourceText = parseSession.publishedText,
          let generation = UInt(exactly: parsed.revision)
    else {
        contributedPreviewBlocks = nil
        contributionDiagnostics = []
        return
    }

    let revision = parsed.revision

    do {
        let results = try await PreviewContributionAdapter.results(
            document: parsed,
            text: sourceText,
            generation: generation
        )
        try Task.checkCancellation()

        let base = PreviewBlock.blocks(from: parsed, text: sourceText)
        let composition = PreviewContributionAdapter.compose(
            base: base,
            sourceText: sourceText,
            sourceMap: parsed.sourceMap,
            contributions: results,
            currentGeneration: generation
        )

        try Task.checkCancellation()
        guard identity == taskIdentity,
              parseSession.document?.revision == revision
        else { return }

        // Check immediately before the first mutation. This intentionally
        // sits after the stale-snapshot guard.
        try Task.checkCancellation()
        contributedPreviewBlocks = composition.blocks
        contributionDiagnostics = composition.diagnostics
    } catch is CancellationError {
        return
    } catch {
        // Ordinary infrastructure/adapter failure is observable but does not
        // replace authored Preview content.
        guard identity == taskIdentity,
              parseSession.document?.revision == revision,
              !Task.isCancelled
        else { return }

        contributedPreviewBlocks = nil
        contributionDiagnostics = [
            .init(
                contributionID: nil,
                severity: .error,
                message: "Preview contribution processing failed: \(error.localizedDescription)"
            ),
        ]
    }
}

Important: the catch is CancellationError branch performs no state mutation at all. Do not clear the current state there; the parse-change handler already invalidates old composed state synchronously when the task key changes.

6. Task identity stays {document identity, parse revision}

Retain pass 1's:

.task(id: contributionTaskID) {
    await refreshPreviewContributions()
}

A save/mark-clean does not alter that key. A real new parse or document identity does, causing SwiftUI to cancel the old task and start a new one.

Do not include FileDocument.mutationGeneration in this key.

Required regression tests

Contributions package

  1. Existing .hangs cancellation test remains and still throws CancellationError.
  2. New .returnsAfterCancellation(content) contributor: start registry task, cancel it, await value -> registry throws CancellationError; the deliberately returned content is never returned.
  3. Registry [returnsAfterCancellation, succeedingSecondContributor]: after cancellation, second contributor is never executed/returned.
  4. A normal contributor failure with a non-cancelled task still becomes one diagnostic-bearing result and later contributors still run — proves isolation semantics were not broken.
  5. Cancelled task + contributor throwing an ordinary error after cancellation -> cancellation wins; no diagnostic-only result is returned.
  6. TOCContribution.findMarkers observes a task cancelled before invocation and throws CancellationError rather than scanning/returning markers.

Preview adapter/view seam

  1. PreviewContributionAdapter.results propagates CancellationError; it never returns [] for cancellation.
  2. Superseded-snapshot race: old task is made to return only after cancellation, newer snapshot publishes; old snapshot performs zero subsequent publication.
  3. Cancellation during/after composition but before publication leaves the latest valid Preview state untouched.
  4. Ordinary non-cancellation adapter failure preserves base Preview and emits the pass-9 diagnostic rather than being mistaken for cancellation.

If direct DocumentEditorSplitView async-state testing is impractical in the current harness, extract only the snapshot publication predicate into an app-private pure helper:

static func mayPublish(
    taskIdentity: String,
    currentIdentity: String,
    taskRevision: Int,
    currentRevision: Int?
) -> Bool

and test that helper plus adapter cancellation. Do not introduce a new observable object solely to make this testable.

Acceptance criteria

  • PreviewContributionAdapter.results is async throws and contains no blanket catch.
  • ContributionRegistry checks cancellation after contributor return and before failure isolation.
  • The standard TOC scan cooperates with cancellation during long line scans.
  • A cancelled/superseded Preview task performs no state mutation.
  • A non-cooperative contributor cannot make cancelled results escape the registry.
  • Ordinary contributor failures remain isolated and diagnostic-bearing.
  • No new actor/task-manager/generation mechanism exists.

Immediate hostile compatibility review

Existing registry error isolation: PASS. Ordinary errors are still converted into per-contributor diagnostic results when the parent task is live.

Structured concurrency: PASS. Cancellation is restored to the normal Swift contract: it propagates through library and adapter layers and is consumed only at the UI task boundary.

Pass 1 snapshot architecture: PASS. Cancellation and {identity, revision} stale checking are complementary: cancellation handles supersession promptly; the snapshot guard handles any race/non-cooperative completion that survives until publication.

TOC semantics: PASS. Adding Task.checkCancellation() does not change marker eligibility/ranges; it only makes the existing O(lines) scan interruptible.

API surface: PASS with a deliberate narrowing. PreviewContributionAdapter is app-private, so changing optional inputs to required inputs and async to async throws has no external compatibility cost. The public Contributing protocol signature is unchanged.

Performance: PASS. Task.checkCancellation() is a cheap flag check. One check per TOC physical line is appropriate for a user-edit hot path and avoids coarse latency on large files.

Conclusion: approved as final architecture. Cancellation must stay throwable until the SwiftUI boundary; never encode supersession as an empty successful contribution result.

@Joncallim Joncallim left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architecture pass 9/10 — P2: preserve contribution diagnostics across Preview and Export boundaries

Original finding: issue comment 5534709836.

Required invariant

Every ContributionDiagnostic produced by the registry is observable at the surface that requested the contribution, including when ContributionResult.content == nil.

Diagnostics and placement are independent channels:

  • no content -> nothing is placed, diagnostics still surface;
  • warning + valid content -> content may place, warning surfaces;
  • error + content -> authored source is preserved, error surfaces;
  • adapter validation/budget failure -> authored source is preserved, adapter diagnostic surfaces.

Do not invent a fake source range merely to transport an anchorless failure.

Files to change

Preview:

  • MacDown2/MacDown2/PreviewContributionAdapter.swift
  • MacDown2/MacDown2/DocumentEditorSplitView.swift
  • MacDown2/MacDown2/PreviewBusyIndicator.swift — add the related contribution-status indicator here rather than adding another project file
  • MacDown2/MacDown2Tests/PreviewContributionAdapterTests.swift

Export:

  • MacDown2/MacDown2/ExportContributionAdapter.swift
  • MacDown2/MacDown2/ExportCoordinator.swift
  • MacDown2/MacDown2Tests/ExportContributionAdapterTests.swift

Do not change ExportRequest, PreparedExportDocument, ExportResult, ExportService, or the Contributions public protocol solely for this diagnostic plumbing.

1. Preview: finalize the composition result model

Add app-private nested values:

enum PreviewContributionAdapter {
    struct Diagnostic: Sendable, Equatable {
        let contributionID: String?
        let severity: ContributionDiagnostic.Severity
        let message: String
    }

    struct Composition: Sendable, Equatable {
        let blocks: [PreviewBlock]?
        let diagnostics: [Diagnostic]
    }
}

contributionID is optional because infrastructure/composition-level failures may not belong to one contributor. Contribution-originated diagnostics always populate it from ContributionResult.contributionID.

Do not create another severity enum. The adapter already imports Contributions, and ContributionDiagnostic.Severity is renderer-neutral.

2. Preview validation starts by forwarding diagnostics

At the start of considering every ContributionResult:

for diagnostic in result.diagnostics {
    plan.diagnostics.append(Diagnostic(
        contributionID: result.contributionID,
        severity: diagnostic.severity,
        message: diagnostic.message
    ))
}

This occurs before guard let content and before source-generation/range validation.

Then apply this rule:

if result.diagnostics.contains(where: { $0.severity == .error }) {
    // The contributor said its own result failed. Preserve authored source.
    return
}

Warnings do not block otherwise-valid placement. Errors do.

This matches Export's existing DerivedContentComposer safety posture without requiring Preview to depend on ExportService.

Do not add a second generic “renderer failed” diagnostic for this case unless the adapter itself detects an additional independent failure; the contribution's original error is already the reason the source was preserved.

3. Preview adapter-generated diagnostics

Passes 5–7 require diagnostics for unsupported placement/representation, malformed/stale/overlapping ranges, partial unsupported containers, and resource-budget rejection.

All such messages use the same nested Diagnostic type. Rules:

  • contributionID: the offending result's ID when one exists;
  • severity: .error for a rejected replacement; .warning only for a condition where content is still validly placed;
  • message ends with or clearly states authored source preserved for rejected placement;
  • no source range is required in the diagnostic type; the range may be included in message text when useful for tests/developer debugging.

Composition.diagnostics is therefore the single Preview diagnostic output. No diagnostics are stored separately in ad-hoc arrays inside DocumentEditorSplitView.

4. Preview UI: reuse the existing subtle status-overlay pattern

PreviewBusyIndicator.swift already owns the small top-right Preview status affordance. Add a sibling view there:

struct PreviewContributionDiagnosticIndicator: View {
    let diagnostics: [PreviewContributionAdapter.Diagnostic]

    private let shownAtMost = 6

    var body: some View {
        if !diagnostics.isEmpty {
            Image(systemName: diagnostics.contains(where: { $0.severity == .error })
                ? "exclamationmark.triangle.fill"
                : "exclamationmark.triangle")
                .padding(8)
                .help(helpText)
                .accessibilityLabel(
                    diagnostics.count == 1
                        ? "Preview contribution issue"
                        : "Preview contribution issues"
                )
                .accessibilityValue(helpText)
                .accessibilityIdentifier("previewContributionDiagnosticIndicator")
        }
    }

    private var helpText: String {
        let shown = diagnostics.prefix(shownAtMost).map(\.message)
        var text = shown.joined(separator: "\n")
        if diagnostics.count > shown.count {
            text += "\n…and \(diagnostics.count - shown.count) more."
        }
        return text
    }
}

Exact Swift syntax can be adjusted for compiler/type-checker needs; behavior is mandatory.

Do not add a new logging framework or modal alert for background Preview contribution errors. The warning indicator is visible, testable, non-blocking, and consistent with Preview's existing intentionally subtle busy indicator.

5. Preview overlay composition

Replace the Markdown Preview's single busy overlay with one overlay containing both status elements:

.overlay(alignment: .topTrailing) {
    HStack(spacing: 0) {
        PreviewContributionDiagnosticIndicator(
            diagnostics: contributionDiagnostics
        )
        PreviewBusyIndicator(isVisible: parseSession.isParsing)
    }
}

If the exact spacing needs to match existing layout, keep it minimal; do not create a second overlapping .overlay(alignment: .topTrailing) that can stack both views in the same coordinates.

contributionDiagnostics is the state established in pass 1 and published atomically with contributedPreviewBlocks in pass 8.

When a new parse publishes, clear both composed blocks and diagnostics together before the replacement contribution task runs. When a cancelled superseded task exits, it mutates neither.

6. Export: adapter returns placements and anchorless diagnostics

Do not extend ExportDerivedContribution with an optional range and do not modify E12 types.

Replace:

static func exportContributions(
    from results: [ContributionResult]
) -> [ExportDerivedContribution]

with:

struct Adapted: Sendable, Equatable {
    let contributions: [ExportDerivedContribution]
    let unanchoredDiagnostics: [ExportDiagnostic]
}

static func adapt(_ results: [ContributionResult]) -> Adapted

Implementation behavior per result:

var contributions: [ExportDerivedContribution] = []
var unanchoredDiagnostics: [ExportDiagnostic] = []

for result in results {
    let mappedDiagnostics = result.diagnostics.map(exportDiagnostic)

    guard let content = result.content else {
        unanchoredDiagnostics.append(contentsOf: mappedDiagnostics)
        continue
    }

    var anchoredDiagnostics = mappedDiagnostics
    let html: String

    switch content.representation {
    case let .markdown(markdown):
        html = ExportService.renderMarkdownFragment(markdown)
    case .html:
        html = ""
        anchoredDiagnostics.append(ExportDiagnostic(
            severity: .error,
            message: "\(result.contributionID) produced an HTML representation, "
                + "which export does not support yet; authored source preserved"
        ))
    }

    contributions.append(ExportDerivedContribution(
        sourceRange: content.sourceRange,
        placement: exportPlacement(for: content.placement),
        html: html,
        sourceGeneration: result.sourceGeneration,
        diagnostics: anchoredDiagnostics
    ))
}

return Adapted(
    contributions: contributions,
    unanchoredDiagnostics: unanchoredDiagnostics
)

Important: diagnostics on a content-bearing result stay attached to that ExportDerivedContribution; DerivedContentComposer already forwards them. Do not also copy them into unanchoredDiagnostics, or the final alert will duplicate them.

7. ExportCoordinator: merge the anchorless diagnostics at the existing final UI boundary

Change exportContributions(for:) to return ExportContributionAdapter.Adapted.

In performExport:

let adapted = try await exportContributions(for: document)
let request = ExportRequest(
    text: document.text,
    sourceGeneration: document.mutationGeneration,
    theme: themeController.current,
    documentURL: document.fileURL,
    contributions: adapted.contributions
)

For successful HTML:

return ExportOutcome(
    primaryFile: result.primaryFile,
    diagnostics: adapted.unanchoredDiagnostics + result.diagnostics
)

For successful PDF:

return ExportOutcome(
    primaryFile: selection.url,
    diagnostics: adapted.unanchoredDiagnostics + prepared.diagnostics
)

This deliberately uses the coordinator's already-existing final diagnostic stream and its existing “show at most 6 then summarize” alert behavior. No E12 public API changes are needed.

Anchorless contribution diagnostics should precede composed export diagnostics so registry execution order is preserved as far as possible before metadata/resource diagnostics follow.

8. Do not silently place content that carries an error

Preview explicitly rejects an error-bearing result as described above. Export already rejects it in DerivedContentComposer because it checks contribution.diagnostics.contains { severity == .error } before placement.

Retain that parity. A warning may accompany placed content; an error may not.

Required regression tests

Preview

  1. ContributionResult(content: nil, diagnostics: [.error("boom")]) -> Composition.blocks equals authored base; Composition.diagnostics contains boom with contribution ID.
  2. Same shape with .warning -> warning survives even though nothing is placed.
  3. Valid content + warning -> content is placed and warning survives.
  4. Valid content + error -> authored source preserved and original error survives.
  5. Stale/malformed/unsupported result with its own warning -> both the contribution warning and adapter rejection diagnostic survive.
  6. Budget summary diagnostics from pass 7 appear in Composition.diagnostics.
  7. New parse clears stale diagnostics before new composition publishes.
  8. Cancelled old task cannot clear or replace diagnostics from the newer snapshot.
  9. PreviewContributionDiagnosticIndicator is absent for [], present for non-empty diagnostics, has accessibility identifier, and its help/accessibility value summarizes >6 entries rather than silently discarding the existence of the remainder.

Export

  1. Replace current aContentlessResultIsDropped test with: contentless diagnostic result -> adapted.contributions.isEmpty, adapted.unanchoredDiagnostics == [mapped diagnostic].
  2. Contentless result with no diagnostics -> both arrays empty.
  3. Content-bearing warning -> warning exists only on adapted.contributions[0].diagnostics, not in unanchoredDiagnostics.
  4. Content-bearing error -> remains attached so DerivedContentComposer preserves authored source and final prepared diagnostics include the error.
  5. Throwing registry contribution + otherwise valid export -> export succeeds using authored Markdown fallback and final ExportOutcome diagnostics include the registry failure.
  6. Same integration case for PDF preparation path (unit-test the merged diagnostics seam; do not require a real PDF file render if the current harness does not support it cheaply).

Acceptance criteria

  • No content == nil result loses its diagnostics at either surface.
  • Preview presents a non-modal diagnostic indicator with accessible/hover detail.
  • Preview error-bearing contribution content is never placed.
  • Export adapter has a typed two-channel result: anchored contributions + unanchored diagnostics.
  • Export's existing user-facing diagnostics alert receives anchorless contribution failures for both HTML and PDF success paths.
  • No fake source range exists.
  • ExportRequest and ExportService public API remain unchanged.
  • Content-bearing diagnostics are not duplicated.

Immediate hostile compatibility review

Contributions contract: PASS. This implements the contract already documented by ContributionResult/ContributionDiagnostic; no public protocol change is necessary.

ExportService/E12: PASS and lower risk than extending ExportRequest. Anchorless diagnostics are merged at ExportCoordinator.ExportOutcome, which already owns user presentation for both HTML and PDF. The package's source-range invariants remain untouched.

Preview UX: PASS. The indicator follows the existing subtle top-right status pattern and is non-modal; normal editing is not interrupted by extension failures.

SwiftUI layout: PASS with the single-HStack overlay requirement. Busy and diagnostics indicators no longer compete for identical top-trailing coordinates.

Diagnostic duplication: PASS if content-bearing diagnostics remain exclusively attached to ExportDerivedContribution and only nil-content diagnostics use the unanchored channel.

Cancellation: PASS. The diagnostic state follows the same atomic snapshot/publication rules as composed blocks; cancelled tasks perform no mutation.

Future E19/E20: PASS. A future renderer can report warnings/errors with or without placeable content and both surfaces already have observable behavior; it does not need to invent its own error UI.

Conclusion: approved as final architecture. Keep diagnostics orthogonal to placement, surface them through existing app-level UI seams, and leave ExportService's public request/composition contracts unchanged.

@Joncallim Joncallim left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architecture pass 10/10 — P3: PR description and verification become executable hand-off state

Original finding: issue comment 5534712299.

Required invariant

The PR body describes what is true at the current head, not the implementation plan from an earlier slice.

At every hand-off point, a reviewer must be able to answer from the PR body alone:

  1. what has actually landed;
  2. what remains intentionally pending in this draft;
  3. what user-visible behavior exists now;
  4. what exact manual cases matter;
  5. which canonical verification commands actually passed at this head;
  6. which residual limitations are intentional rather than missed defects.

Do not mark checks from expected results. Evidence is recorded only after the command/CI run completed on the final remediation head.

Files/code to change

No production code is required for this item.

After passes 1–9 are implemented and tests are complete:

  • update PR #54 body;
  • reconcile planning/epic-14-implementation.md with the final behavior from passes 1–9;
  • update planning/epics/README.md only if its epic/slice status is now objectively different.

Do not close #15 or mark the whole epic complete merely because the contribution/TOC half is remediated if text filters are still pending.

Exact PR body structure

Replace the stale body wholesale rather than trying to patch individual sentences. Use these sections in this order.

## Status

The first paragraph must make the draft boundary explicit. Unless text filters have landed by the time this remediation is implemented, say materially:

**Draft / in progress.** This head implements the first-party contribution seam and the TOC contribution end-to-end through Preview and Export. The text-filter half of EPIC-14 is still pending and this PR is not the final epic closeout yet.

If text filters have landed by then, replace that sentence with the actual landed status; do not preserve a knowingly stale statement.

## What this changes

List only landed behavior, grouped by seam rather than commit chronology:

  • renderer-neutral Contributions target and registry;
  • first-party [TOC] contribution;
  • semantic marker eligibility (ordinary paragraph context only);
  • Preview integration using parse-snapshot identity;
  • exact block/inline source-range composition and authored-source fallback;
  • Preview resource/cancellation/diagnostic handling;
  • Export adapter using E12 ExportDerivedContribution + anchorless diagnostic hand-off;
  • tests added for the above.

If text-filter files are still absent, explicitly list Text filters — pending rather than describing them as implemented.

## Behavior / invariants

Record the contracts reviewers should protect:

- `[TOC]` is active only as a standalone physical line in ordinary paragraph context; fenced/indented code and other literal/raw contexts are preserved.
- Preview and Export replace only the contribution-owned source range; neighboring authored Markdown is never discarded.
- Preview contribution freshness is keyed to `{document identity, Markdown parse revision}`, not file save/recovery mutation state.
- `.inline` and `.block` retain distinct placement semantics.
- malformed/stale/unsupported/over-budget contributions fail closed to authored source with diagnostics.
- cancellation never publishes superseded Preview state.
- Export continues using the existing E12 composer/resource policy; EPIC-14 does not create a second export renderer.

## What reviewers should test manually

Use concrete cases, not “test TOC”. At minimum:

  1. normal document with H1/H2/H3 + [TOC] -> nested TOC appears in Preview;
  2. Alpha\n[TOC]\nOmega -> all three remain visible, with only marker replaced;
  3. two [TOC] lines in one CommonMark paragraph -> both render and middle authored text remains;
  4. fenced-code [TOC] remains literal;
  5. four-space-indented [TOC] remains literal;
  6. whitespace-tolerant ordinary marker ( [TOC] ) still renders when parser classifies it as paragraph;
  7. save/mark-clean without editing -> rendered TOC does not disappear;
  8. edit source -> new parse recomputes TOC and no old snapshot flashes/publishes afterward;
  9. HTML export -> TOC appears, authored marker does not;
  10. PDF export preparation/render -> same content behavior;
  11. ordinary Markdown file with no contributions -> Preview/Export unchanged.

Do not add synthetic inline-contributor cases to the manual list unless a real user-facing inline contribution exists by then; those belong in automated contract tests.

## Automated verification

Record the repository's canonical commands exactly. The final remediation head must execute the same gates as .github/workflows/ci.yml:

swiftformat --lint MacDown2
swiftlint lint --strict MacDown2

cd MacDown2/Packages/MacDownKit
swift build
swift test --no-parallel
cd ../../..

cd MacDown2
xcodegen generate
cd ..

xcodebuild -project MacDown2/MacDown2.xcodeproj -scheme MacDown2 \
  -destination 'platform=macOS' build
xcodebuild -project MacDown2/MacDown2.xcodeproj -scheme macdown2 \
  -destination 'platform=macOS' build
xcodebuild -project MacDown2/MacDown2.xcodeproj -scheme MacDown2 \
  -destination 'platform=macOS' -enableCodeCoverage NO build-for-testing

Then give a compact evidence table:

| Gate | Result | Evidence |
|---|---|---|
| SwiftFormat lint | PASS/FAIL | local command / CI run |
| SwiftLint strict | PASS/FAIL | local command / CI run |
| MacDownKit build | PASS/FAIL | ... |
| MacDownKit tests | PASS/FAIL (`N/N`) | ... |
| MacDown2 app build | PASS/FAIL | ... |
| CLI build | PASS/FAIL | ... |
| UI-test build-for-testing | PASS/FAIL | ... |
| GitHub `lint` | PASS/FAIL | run link/ID |
| GitHub `build-and-test` | PASS/FAIL | run link/ID |

Do not write PASS before the final-head command/run exists.

## Focused regression coverage

Name the tests by behavior after implementation, grouped as:

  • source snapshot/save stability;
  • exact block splitting + scroll-sync ownership;
  • literal-code marker exclusion;
  • inline placement semantics;
  • malformed/overlapping ranges;
  • Preview budget ordering/diagnostics;
  • cancellation/non-cooperative contributor;
  • Preview + Export diagnostic-only failures;
  • end-to-end TOC Preview/Export path.

This lets a reviewer distinguish “the suite passed” from “the defect got a regression test”.

## Risks / intentional limits

Retain only limits that are still true after remediation. At minimum, if still current:

  • TOC items are plain Markdown list text, not clickable in-document anchors;
  • Preview still has no arbitrary HTML-fragment contribution renderer;
  • Export performs a separate contribution parse per export;
  • partial splitting of non-paragraph Preview containers is intentionally fail-closed until a future contribution requires a richer container-aware representation.

Delete the old “whole paragraph replacement is an accepted limitation”; pass 2 specifically removes it.

Do not list the 64-placement Preview cap as a silent limitation. It is now an explicit safety policy with authored fallback + diagnostic.

## Traceability

Record:

  • Epic/issue: #15;
  • architecture document path;
  • actual slices/areas landed at this head;
  • remaining slices still pending;
  • remediation source: deep-review findings 1–10 / architecture passes 1–10.

Avoid a false Slice 1 of 8 statement once multiple slices are in the head.

Documentation synchronization

After code/tests are final, update planning/epic-14-implementation.md in the same commit as the PR-body reconciliation (or immediately adjacent documentation commit):

  1. replace FileDocument.mutationGeneration Preview-freshness language with parse-snapshot token semantics;
  2. remove the accepted whole-paragraph replacement limitation;
  3. state paragraph semantic whitelist for [TOC];
  4. document Preview .inline constraints;
  5. document strict fail-closed validation order;
  6. document 64-placement + aggregate generated-Markdown Preview budget and explicit fallback diagnostics;
  7. document cancellation propagation/publication guard;
  8. document Preview/Export anchorless diagnostic handling;
  9. preserve Export's existing resource policy unchanged.

Do not rewrite unrelated epic history while doing this remediation.

Required review checks

Before changing the PR from draft or requesting final review:

  1. compare git diff base...HEAD --name-only against the PR's “What this changes” section — every material subsystem in the diff is represented;
  2. search PR body for stale phrases: only Slice 1, standard is intentionally empty, no user-visible, TOC ... next push, Preview/Export wiring ... later — none may remain if false;
  3. compare every checked verification box/evidence row against a real final-head command or CI result;
  4. confirm residual-risk section contains no defect that passes 1–9 just fixed;
  5. keep PR draft if the broader EPIC-14 scope intentionally remains unfinished.

Acceptance criteria

  • PR body no longer claims only the protocol/registry exists.
  • TOC + Preview + Export behavior is described if present at head.
  • Pending text-filter scope is explicit rather than implied complete.
  • Manual tests exercise the actual P1 regressions.
  • Verification commands are identical to canonical CI and evidence is final-head evidence.
  • No fixed defect survives as an “accepted limitation”.
  • Draft/epic state matches actual remaining work.

Immediate hostile compatibility review

Production code: PASS. This item requires no runtime behavior change.

Review integrity: PASS. Separating landed state from pending epic scope prevents reviewers from either under-testing the current contribution code or accidentally treating the whole E14 epic as complete.

CI integrity: PASS. The body references the repository's existing commands verbatim; it does not invent a parallel verification path or weaken build-for-testing.

Documentation drift: PASS with the explicit same-head reconciliation checklist. The architecture document and PR body must be updated only after implementation so neither pre-claims the fixes.

Future slices: PASS. A later text-filter push can update the same status/traceability sections without rewriting the contribution architecture.

Conclusion: approved as final architecture. Do not treat PR prose as cleanup trivia: for a draft carrying multiple implementation slices, the body is part of the executable hand-off contract.

@Joncallim Joncallim left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cross-pass integration review — two corrections before implementation

This is the orthogonal review after architecture passes 1–10. The individual passes are sound, but two combined-path details should be tightened so the coding agent does not have to infer them.

Correction A — one atomic Preview composition state

Passes 1/8/9 described separate @State values for contributed blocks and diagnostics. Do not implement them as two independently published states.

Use one state value:

@State private var previewContributionComposition: PreviewContributionAdapter.Composition?

and make composition blocks non-optional:

enum PreviewContributionAdapter {
    struct Composition: Sendable, Equatable {
        let blocks: [PreviewBlock]
        let diagnostics: [Diagnostic]
    }

    static func compose(
        base: [PreviewBlock],
        sourceText: String,
        sourceMap: SourceMap,
        contributions: [ContributionResult],
        currentGeneration: UInt,
        budget: ResourceBudget = .standard
    ) -> Composition
}

There is no reason for base to be optional at this seam: pass 1 invokes composition only after it has captured a real MarkdownDocument + publishedText, and PreviewBlock.blocks(from:text:) always returns an array.

Exact state behavior:

  • new parse publishes -> refreshPreviewBlocks() then previewContributionComposition = nil;
  • contribution success -> one assignment: previewContributionComposition = composition;
  • ordinary adapter/infrastructure failure -> create Composition(blocks: base, diagnostics: [infrastructureError]) and assign it once;
  • cancellation/supersession -> no assignment;
  • no current parse -> state is nil.

Rendering:

blocks: previewContributionComposition?.blocks ?? previewBlocks

Indicator:

PreviewContributionDiagnosticIndicator(
    diagnostics: previewContributionComposition?.diagnostics ?? []
)

This makes blocks + diagnostics one snapshot and eliminates the theoretical intermediate body evaluation where new blocks could be paired with old diagnostics or vice versa.

Correction B — mixed .inline + .block placements in the same base paragraph

Pass 2 described block splitting and pass 5 described inline splicing. A future document can legally have both in one CommonMark paragraph, e.g. an inline-math contribution on line 1 and [TOC] block contribution on line 2. The final composer must support that without choosing one placement class and dropping the other.

Final per-base-block algorithm

After global validation/sorting/overlap/budget admission, group accepted placements by baseBlockIndex.

For each base block:

  1. no placements -> append the original PreviewBlock value unchanged;
  2. inline placements only -> call spliceInlineBlock(...) and emit one block with the original kind + line range;
  3. one or more block placements -> call splitBlock(...), passing both block and inline placements.

Because global overlap validation has already run, no accepted inline range can intersect an accepted block range.

Revised splitBlock shape

private static func splitBlock(
    _ block: PreviewBlock,
    blockPlacements: [ValidatedPlacement],
    inlinePlacements: [ValidatedPlacement],
    sourceText: String,
    sourceMap: SourceMap
) -> [PreviewBlock]

Walk the block placements by source line as in pass 2. Whenever an authored prefix/between/suffix fragment is emitted, pass the inline placements contained within that fragment to appendAuthoredFragment:

private static func appendAuthoredFragment(
    from block: PreviewBlock,
    lines: ClosedRange<Int>,
    inlinePlacements: [ValidatedPlacement],
    sourceText: String,
    sourceMap: SourceMap,
    to output: inout [PreviewBlock]
)

Inside:

  1. get the fragment's original absolute UTF-16 range with sourceMap.utf16Range(ofLines: lines);
  2. extract the fragment from sourceText via NSString;
  3. select inline placements whose absolute ranges are fully within that fragment range;
  4. convert each absolute range to fragment-local UTF-16 offsets by subtracting fragmentRange.location;
  5. splice them into the extracted fragment in descending local range order (or equivalent one-pass assembly);
  6. emit PreviewBlock(kind: block.kind, source: splicedSource, lineRange: lines).

Do not first splice inline replacements into the entire block and then use original SourceMap offsets to split the modified string; changed replacement lengths would invalidate those offsets.

A block placement itself still emits exactly one .custom(contributionID) block owning its original line range.

Required mixed-placement regression

Add at least one test with a real paragraph spanning three lines:

  • line 1 contains a valid synthetic inline contribution;
  • line 2 is a valid block contribution;
  • line 3 contains a second valid synthetic inline contribution.

Expected output:

  1. authored prefix block with line-1 inline replacement applied;
  2. custom block contribution on line 2;
  3. authored suffix block with line-3 inline replacement applied;

All three line ranges remain ordered/disjoint and ScrollSyncMap resolves them correctly.

Also test an inline range overlapping the block marker: deterministic overlap validation rejects the later candidate; there is never a double replacement.

Validation pipeline clarification

To keep the implementation deterministic and safe, use two phases rather than trying to sort raw ContributionResult values:

Phase 1 — preflight in registry order

  • always forward result diagnostics;
  • reject error-bearing content;
  • reject stale generation;
  • require content + Markdown representation;
  • validate 0 <= lower < upper <= sourceText.utf16.count;
  • record an app-private candidate containing originalResultIndex.

Only preflight-valid candidates enter the sortable array.

Phase 2 — deterministic source order
Sort candidates by:

sourceRange.lowerBound ASC,
sourceRange.upperBound ASC,
originalResultIndex ASC

Then perform:

  • source line mapping;
  • single-containing-base-block lookup;
  • placement-specific capability checks;
  • overlap check against accepted placements;
  • budget admission.

This means malformed/nil results never reach SourceMap, equal-range ordering is stable across runs, and resource admission is deterministic.

Immediate compatibility review

SwiftUI consistency: improved. One composition state is strictly safer than two correlated states and reduces state variables.

Existing Preview fallback: unchanged. While composition is nil, the existing previewBlocks render immediately.

Mixed future contributions: fixed. E19-style inline output can coexist with E14/E20-style block output in the same paragraph without being dropped or offset-corrupted.

UTF-16 correctness: preserved. Every local splice is derived from the original absolute SourceMap range before generated text changes string lengths.

Scroll sync: preserved. Block splitting still owns whole original lines exactly once; inline replacements do not alter line ownership.

Public API/module graph: unchanged. These are app-private implementation refinements only.

Conclusion: these two corrections supersede the corresponding optional-base/separate-state and block-vs-inline branching details in passes 1/2/5/8/9. All other architecture-pass requirements stand.

@Joncallim Joncallim left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FINAL IMPLEMENTATION HAND-OFF — architecture passes 1–10 + cross-pass review

This comment is the coding sequence. It consolidates the ten architecture passes and the cross-pass corrections into one implementation contract. Where an earlier pass differs from this comment, this comment wins.

The objective is to let the implementation agent write code mechanically, without redesigning the solution.


0. Non-negotiable boundaries

Do not change any of the following while implementing this remediation unless a compiler/test proves an unrelated pre-existing defect:

  • FileDocument.mutationGeneration semantics or save/recovery behavior;
  • MarkdownEngine.SourceMap clamping behavior or parser block taxonomy;
  • PreviewBlock.oversizeByteThreshold or Textual's existing oversize fallback;
  • ScrollSyncMap ordering/search algorithm;
  • the public Contributing protocol signature;
  • the package dependency direction (Contributions remains renderer-neutral and must not import Preview or ExportService);
  • ExportRequest, PreparedExportDocument, ExportResult, ExportResourceBudget, or DerivedContentComposer public contracts;
  • the 4096-fragment / 16 MiB Export resource policy;
  • .github/workflows/ci.yml gates or their strictness;
  • generated Xcode project settings as a workaround for source errors.

Do not create a ContributionsV2, a second source-map abstraction, a second export composer, a Preview-specific generation counter, a task-manager actor, or an extension-specific error UI framework.


1. Implementation sequence

Implement in these batches and keep each batch independently testable.

Batch A — restore the current branch's known red CI gate first

Before touching contribution behavior, reproduce the exact current failure:

cd MacDown2
xcodegen generate
cd ..

xcodebuild -project MacDown2/MacDown2.xcodeproj -scheme MacDown2 \
  -destination 'platform=macOS' -enableCodeCoverage NO build-for-testing

Capture the first non-cascade compiler/linker error. Fix only that proven root cause and rerun the command.

The current head's lint, package build/test, app build, and CLI build all reached green before the UI-test-bundle step; do not weaken or skip this final gate. The stored GitHub connector output does not expose the underlying compiler stdout, so the suspected missing-import explanation is not proven. If Xcode specifically reports URL/Foundation visibility in ExportContributionAdapterTests.swift, add the missing import Foundation and nothing broader. If it reports something else, fix what it actually reports.

Stop condition for Batch A: build-for-testing is green, or an external environment-only blocker is captured verbatim with evidence. Do not hide a source failure behind an environment label.


Batch B — Contributions package foundation

Implement passes 3 and 8 here, plus the source-token documentation correction from pass 1.

B1. ContributionResult.swift

Keep:

public let sourceGeneration: UInt

Do not rename it in this remediation. Update its documentation to call it an opaque source-snapshot token supplied by the caller, not specifically FileDocument.mutationGeneration.

Export may continue supplying file mutation generation. Preview will supply UInt(MarkdownDocument.revision).

B2. ContributionRegistry.swift

Use this exact control-flow shape:

for contribution in contributions {
    try Task.checkCancellation()

    do {
        let contributed = try await contribution.run(
            document: document,
            sourceText: sourceText,
            sourceGeneration: sourceGeneration
        )

        try Task.checkCancellation()
        results.append(contentsOf: contributed)
    } catch is CancellationError {
        throw CancellationError()
    } catch {
        try Task.checkCancellation()

        let diagnostic = ContributionDiagnostic(
            severity: .error,
            message: "\(contribution.id) failed: \(error.localizedDescription)"
        )
        results.append(ContributionResult(
            contributionID: contribution.id,
            content: nil,
            sourceGeneration: sourceGeneration,
            diagnostics: [diagnostic]
        ))
    }
}

try Task.checkCancellation()
return results

Cancellation always outranks contributor-failure isolation.

B3. TOCContribution.swift

Replace lexical-only discovery with parser-semantic eligibility.

Final helper signature:

static func findMarkers(
    in text: String,
    document: MarkdownDocument
) throws -> [Range<Int>]

For every physical source line:

  1. try Task.checkCancellation();
  2. obtain that physical line's source range from document.sourceMap;
  3. extract with UTF-16/NSString semantics already used by the repository;
  4. trim whitespace/newline characters and require exact [TOC];
  5. require:
document.block(atLine: line)?.kind == .paragraph
  1. append the exact original physical-line UTF-16 range, not a trimmed/token-only range.

This is a positive semantic whitelist. Fenced code, indented code, raw/literal containers, front matter, etc. remain authored because they are not paragraph blocks.

In run(...):

try Task.checkCancellation()
let markers = try Self.findMarkers(in: sourceText, document: document)
try Task.checkCancellation()

guard !markers.isEmpty else { return [] }

let list = Self.markdownList(for: document.headings)
try Task.checkCancellation()

let results = markers.map { ... }
try Task.checkCancellation()
return results

Keep markdownList(for:) pure.

B4. DeterministicTestContribution.swift

Add test-only behavior:

case returnsAfterCancellation(ContributionContent)

Implementation:

while !Task.isCancelled {
    await Task.yield()
}
return [ContributionResult(
    contributionID: id,
    content: content,
    sourceGeneration: sourceGeneration
)]

This deliberately simulates a broken contributor that ignores cancellation so the registry's post-return guard is testable without timing sleeps.

Batch B tests

Add/adjust package tests for:

  • ordinary paragraph [TOC] accepted;
  • whitespace-tolerant ordinary marker accepted;
  • CRLF marker gets correct absolute UTF-16 range;
  • fenced-code marker rejected;
  • four-space-indented marker rejected;
  • HTML/front-matter/literal block marker rejected where supported by parser fixtures;
  • 2–3 leading spaces remain governed by parser classification, not a hand-coded indentation rule;
  • task cancelled before marker scan -> CancellationError;
  • .hangs contributor still propagates cancellation;
  • .returnsAfterCancellation cannot escape the registry;
  • cancelled first contributor prevents a later contributor from running;
  • ordinary non-cancellation contributor failure still becomes one diagnostic result and later contributors still run;
  • cancellation wins if an ordinary error is produced after cancellation.

Run:

cd MacDown2/Packages/MacDownKit
swift build
swift test --no-parallel

Do not proceed with a red package suite.


2. Batch C — rewrite PreviewContributionAdapter once

Do not incrementally patch the current merged(...) implementation. Replace it with the final adapter architecture in one coherent change.

Add:

import Contributions
import Foundation
import MarkdownEngine
import Preview

Final type shape:

enum PreviewContributionAdapter {
    struct ResourceBudget: Sendable, Equatable {
        let maxPlacedContributionCount: Int
        let maxAggregateGeneratedMarkdownUTF8Bytes: Int

        static let standard = ResourceBudget(
            maxPlacedContributionCount: 64,
            maxAggregateGeneratedMarkdownUTF8Bytes:
                64 * PreviewBlock.oversizeByteThreshold
        )
    }

    struct Diagnostic: Sendable, Equatable {
        let contributionID: String?
        let severity: ContributionDiagnostic.Severity
        let message: String
    }

    struct Composition: Sendable, Equatable {
        let blocks: [PreviewBlock]
        let diagnostics: [Diagnostic]
    }

    private struct Candidate { ... }
    private struct ValidatedPlacement { ... }
    private struct CompositionPlan { ... }

    static func results(
        document: MarkdownDocument,
        text: String,
        generation: UInt
    ) async throws -> [ContributionResult]

    static func compose(
        base: [PreviewBlock],
        sourceText: String,
        sourceMap: SourceMap,
        contributions: [ContributionResult],
        currentGeneration: UInt,
        budget: ResourceBudget = .standard
    ) -> Composition

    private static func preflight(...)
    private static func validateAndAdmit(...)
    private static func composeBlocks(...)
    private static func spliceInlineBlock(...)
    private static func splitBlock(...)
    private static func appendAuthoredFragment(...)
}

Exact private type spelling may vary for SwiftLint, but the phases and stored data below are mandatory.

C1. results(...)

Implementation is only:

try await ContributionRegistry.standard.run(
    document: document,
    sourceText: text,
    sourceGeneration: generation
)

No optional inputs. No blanket catch. Cancellation remains throwable.

C2. Phase 1: preflight in original registry order

For each ContributionResult, first copy all contribution diagnostics into the plan's diagnostic output:

for diagnostic in result.diagnostics {
    plan.diagnostics.append(Diagnostic(
        contributionID: result.contributionID,
        severity: diagnostic.severity,
        message: diagnostic.message
    ))
}

Then:

  1. if any result diagnostic has severity .error, do not create a candidate; authored source will remain;
  2. require result.sourceGeneration == currentGeneration; otherwise add adapter error diagnostic and reject;
  3. require content != nil; contentless results are now finished after diagnostic forwarding;
  4. require .markdown(markdown) representation; unsupported representation -> error diagnostic + reject;
  5. validate the absolute half-open source range before any SourceMap call:
0 <= lower < upper <= sourceText.utf16.count
  1. record a candidate containing at least:
originalResultIndex
contributionID
placement
markdown
sourceRange // Range<Int>, absolute UTF-16

A malformed range never reaches SourceMap.line(...).

C3. Stable source ordering

Sort only preflight-valid candidates by:

sourceRange.lowerBound ASC
sourceRange.upperBound ASC
originalResultIndex ASC

Do not depend on sort stability alone for equal ranges; originalResultIndex is the explicit tiebreaker.

C4. Phase 2: line/block/capability validation

For each sorted candidate:

let startLine = sourceMap.line(atUTF16Offset: sourceRange.lowerBound)
let endLine = sourceMap.line(atUTF16Offset: sourceRange.upperBound - 1)
let lineRange = startLine ... endLine

Find exactly one base block whose lineRange contains the entire candidate lineRange.

If none exists, reject with error diagnostic and authored-source fallback.

The accepted private ValidatedPlacement must retain at least:

let originalResultIndex: Int
let contributionID: String
let placement: ContributionPlacement
let markdown: String
let sourceRange: Range<Int>       // original absolute UTF-16
let lineRange: ClosedRange<Int>   // original physical source lines
let baseBlockIndex: Int

.block capability rule

Compute:

let expected = sourceMap.utf16Range(ofLines: lineRange)

The contribution range must equal that complete line range:

sourceRange.lowerBound == expected.location
sourceRange.upperBound == NSMaxRange(expected)

Then:

  • if its line range equals the whole base block's line range: any base block kind may be replaced;
  • if it replaces only part of a base block: base block kind must be .paragraph;
  • otherwise reject and preserve authored source.

Do not partially dismantle list/table/code/HTML/raw containers.

.inline capability rule

Require all of:

  • startLine == endLine;
  • source range lies wholly in one existing base block;
  • generated Markdown contains neither \n nor \r.

Keep the original base block kind and original source-line ownership.

C5. Overlap

Check overlap only against already accepted placements, using absolute half-open UTF-16 ranges.

For source-sorted ranges, overlap exists when:

candidate.lowerBound < previousAccepted.upperBound

Touching boundaries are not overlap.

Reject the later overlapping candidate with an error diagnostic. Equal ranges resolve deterministically through originalResultIndex.

A range rejected for any earlier reason does not reserve its authored interval.

C6. Resource admission is last

Production budget:

max accepted placements = 64
max aggregate generated Markdown = 64 × PreviewBlock.oversizeByteThreshold = 4 MiB

Do not use contributions.prefix(64).

Only semantically/range-valid/non-overlapping candidates consume budget.

Use overflow-safe arithmetic:

let bytes = placement.markdown.utf8.count
let (total, overflowed) = aggregateGeneratedBytes.addingReportingOverflow(bytes)

Count rejection:

  • preserve authored range;
  • increment an omitted-count counter;
  • do not append placement.

Byte rejection:

  • preserve authored range;
  • increment an omitted-byte counter;
  • do not append placement;
  • later smaller candidates may still be admitted if they fit remaining bytes.

Do not reject an individual generated block merely because it exceeds 64 KiB. PreviewBlock.isOversize already gives the established Textual safety fallback.

After validation, add at most:

  • one count-budget summary diagnostic;
  • one byte-budget summary diagnostic.

Do not create one diagnostic per over-budget result.

C7. Final block composition algorithm

Group accepted placements by baseBlockIndex.

For each original base block, in original order:

Case 1 — no accepted placements

Append the original PreviewBlock value unchanged. This preserves its deterministic ID and current SwiftUI/scroll state.

Case 2 — inline placements only

Call:

spliceInlineBlock(
    block,
    inlinePlacements: ..., 
    sourceText: sourceText,
    sourceMap: sourceMap
)

Extract the original absolute block source using:

let blockNSRange = sourceMap.utf16Range(ofLines: block.lineRange)
let original = (sourceText as NSString).substring(with: blockNSRange)

Use an NSMutableString (or an equivalent demonstrably UTF-16-safe implementation) and apply replacements in descending local UTF-16 range order:

localLower = placement.sourceRange.lowerBound - blockNSRange.location
localLength = placement.sourceRange.count

Emit one new PreviewBlock with:

  • original block.kind;
  • spliced generated source;
  • original block.lineRange.

Never convert UTF-16 offsets through naïve Swift character indices.

Case 3 — one or more .block placements

Call:

splitBlock(
    block,
    blockPlacements: blockPlacements,
    inlinePlacements: inlinePlacements,
    sourceText: sourceText,
    sourceMap: sourceMap
)

Walk block placements in line order. Emit:

  1. authored prefix lines before the first block placement;
  2. custom block placement;
  3. authored lines between block placements;
  4. custom block placement;
  5. authored suffix lines after the last placement.

Skip empty line intervals.

A generated block emits:

PreviewBlock(
    kind: .custom(placement.contributionID),
    source: placement.markdown,
    lineRange: placement.lineRange
)

For every authored prefix/between/suffix interval call:

appendAuthoredFragment(
    from: block,
    lines: fragmentLines,
    inlinePlacements: inlinePlacements,
    sourceText: sourceText,
    sourceMap: sourceMap,
    to: &output
)

appendAuthoredFragment must:

  1. get the fragment's original absolute NSRange using sourceMap.utf16Range(ofLines:);
  2. extract the fragment from the original sourceText;
  3. select only inline placements fully contained by that fragment range;
  4. convert those absolute inline ranges to fragment-local UTF-16 offsets;
  5. splice them in descending local range order;
  6. emit a PreviewBlock with the original base block kind and the fragment's original line range.

Critical: never splice inline replacements into the entire block first and then split using original SourceMap offsets. Generated inline text can change string length and invalidate those offsets.

C8. Composition return

Always return:

Composition(
    blocks: composedOrBaseBlocks,
    diagnostics: plan.diagnostics
)

blocks is not optional. The adapter is called only after the view has an exact parsed source snapshot.

Batch C regression matrix

Rewrite/extend PreviewContributionAdapterTests.swift so it covers all of these behavioral families:

Exact block replacement

  • standalone marker replaces only its source line;
  • Alpha\n[TOC]\nOmega -> three ordered blocks, Alpha and Omega survive;
  • multiple markers within one multi-line CommonMark paragraph preserve text between them;
  • CRLF line ownership is exact;
  • untouched blocks preserve IDs;
  • partial non-paragraph block replacement rejects fail-closed;
  • whole non-paragraph block replacement remains permitted when source range exactly owns it;
  • resulting line ranges are ordered/disjoint and ScrollSyncMap resolves prefix/custom/suffix correctly.

Inline

  • middle/prefix/suffix inline replacement;
  • emoji/non-BMP source proves UTF-16 correctness;
  • CRLF source;
  • multiple inline replacements in one block;
  • inline range crossing physical lines rejects;
  • inline range crossing base blocks rejects;
  • generated Markdown containing CR/LF rejects;
  • block/inline overlap rejects later candidate deterministically.

Mixed inline + block

Use one real three-line paragraph:

  • line 1: synthetic inline contribution;
  • line 2: synthetic block contribution;
  • line 3: second synthetic inline contribution.

Expected: authored prefix block with line-1 inline replacement, custom line-2 block, authored suffix block with line-3 inline replacement. All original line ranges remain disjoint/ordered.

Range safety

  • negative lower bound;
  • empty range;
  • upper bound one past source UTF-16 length;
  • source range mapped outside every base block;
  • stale source token;
  • unsupported representation;
  • exact duplicate/overlap ordering;
  • invalid results do not alter source or consume budget.

Resource policy

  • production standard 65 valid markers -> 64 placed, 65th authored, one summary diagnostic;
  • many invalid/stale/contentless results before a valid result -> valid result still admitted;
  • injectable count budget 2 -> third authored + one count summary;
  • injectable small byte budget -> large candidate rejected, later small candidate admitted if it fits;
  • aggregate arithmetic cannot wrap;
  • 64 KiB generated fragment below aggregate cap is admitted and resulting PreviewBlock.isOversize == true;

  • budget diagnostics bounded to one per violated dimension.

Diagnostics

  • contentless error survives;
  • contentless warning survives;
  • valid content + warning places and warning survives;
  • valid content + error does not place;
  • malformed/stale result carrying a warning preserves both original warning and adapter rejection diagnostic.

Do not assert random generated PreviewBlock.id values. Assert source/kind/lineRange, and assert untouched input block IDs remain exactly unchanged.


3. Batch D — Preview view publication and status UI

This batch removes @State private var contributionResults and stops composing inside the view body.

Current DocumentEditorSplitView already notes its type-body-length pressure. Keep this change compact and do not pull the adapter algorithm into the View.

D1. State

Replace:

@State private var contributionResults: [ContributionResult] = []

with:

@State private var previewContributionComposition:
    PreviewContributionAdapter.Composition?

Blocks + diagnostics are one atomic state snapshot.

D2. Task identity

Add a small file-private Hashable key outside the View body if that avoids type-length pressure:

private struct PreviewContributionTaskID: Hashable {
    let documentIdentity: String
    let parseRevision: Int?
}

Inside the View:

private var previewContributionTaskID: PreviewContributionTaskID {
    PreviewContributionTaskID(
        documentIdentity: identity,
        parseRevision: parseSession.document?.revision
    )
}

Replace:

.task(id: parseSession.document) { ... mutationGeneration ... }

with:

.task(id: previewContributionTaskID) {
    await refreshPreviewContributions()
}

A save/mark-clean does not change this key. A real parse publication does.

D3. Parse publication invalidation

Change the existing parse-document observer to:

.onChange(of: parseSession.document) { _, _ in
    refreshPreviewBlocks()
    previewContributionComposition = nil
    refreshOutline()
}

Clearing composition here makes the already-valid base previewBlocks the immediate fallback while the new contribution task runs.

D4. refreshPreviewContributions()

Use the exact parse snapshot, not live values repeatedly across await:

private func refreshPreviewContributions() async {
    let taskIdentity = identity

    guard let parsed = parseSession.document,
          let sourceText = parseSession.publishedText,
          let generation = UInt(exactly: parsed.revision)
    else {
        previewContributionComposition = nil
        return
    }

    let revision = parsed.revision
    let base = PreviewBlock.blocks(from: parsed, text: sourceText)

    do {
        let results = try await PreviewContributionAdapter.results(
            document: parsed,
            text: sourceText,
            generation: generation
        )
        try Task.checkCancellation()

        let composition = PreviewContributionAdapter.compose(
            base: base,
            sourceText: sourceText,
            sourceMap: parsed.sourceMap,
            contributions: results,
            currentGeneration: generation
        )
        try Task.checkCancellation()

        guard identity == taskIdentity,
              parseSession.document?.revision == revision
        else { return }

        try Task.checkCancellation()
        previewContributionComposition = composition
    } catch is CancellationError {
        return
    } catch {
        guard identity == taskIdentity,
              parseSession.document?.revision == revision,
              !Task.isCancelled
        else { return }

        previewContributionComposition = .init(
            blocks: base,
            diagnostics: [
                .init(
                    contributionID: nil,
                    severity: .error,
                    message: "Preview contribution processing failed: \(error.localizedDescription)"
                ),
            ]
        )
    }
}

If lint/type-body-length requires extraction, extract only pure/helper detail; do not weaken the captured-snapshot + cancellation + stale-publication ordering.

Cancellation catch performs zero mutation.

D5. Markdown Preview rendering

Replace current PreviewContributionAdapter.merged(...) body-time call with:

blocks: previewContributionComposition?.blocks ?? previewBlocks

The view no longer reinterprets contribution ranges during body rendering.

D6. Diagnostic indicator

In existing PreviewBusyIndicator.swift, add sibling:

struct PreviewContributionDiagnosticIndicator: View {
    let diagnostics: [PreviewContributionAdapter.Diagnostic]
    ...
}

Behavior:

  • absent for empty diagnostics;
  • triangle symbol; filled if any .error, unfilled for warning-only;
  • subtle padding consistent with PreviewBusyIndicator;
  • .help(...) with at most first 6 messages + …and N more.;
  • accessibility label/value;
  • .accessibilityIdentifier("previewContributionDiagnosticIndicator").

Do not introduce OSLog, modal alerts, or a new diagnostics panel for Preview.

Replace Markdown Preview's current single top-right overlay with one HStack:

.overlay(alignment: .topTrailing) {
    HStack(spacing: 0) {
        PreviewContributionDiagnosticIndicator(
            diagnostics: previewContributionComposition?.diagnostics ?? []
        )
        PreviewBusyIndicator(isVisible: parseSession.isParsing)
    }
}

Leave JSON preview's existing busy overlay alone.

Batch D tests/build

At minimum verify:

  • saving/marking clean without text edit does not change the Preview contribution task token or remove the TOC;
  • a new parse revision invalidates old composition immediately to base Preview;
  • cancelled old result cannot publish after a newer revision;
  • ordinary infrastructure failure keeps base blocks and shows diagnostic state;
  • diagnostic indicator absent/present appropriately and summarizes >6;
  • accessibility identifier/value exists.

If direct async View-state testing is expensive in the current harness, extract only a pure publication predicate helper and test it. Do not add a coordinator/observable object just for testing.

Then run app build + build-for-testing before moving on.


4. Batch E — Export diagnostic preservation only

Do not redesign Export composition. E12's composer already has the correct strict range/overlap/resource behavior.

E1. ExportContributionAdapter.swift

Replace the array-only return with:

struct Adapted: Sendable, Equatable {
    let contributions: [ExportDerivedContribution]
    let unanchoredDiagnostics: [ExportDiagnostic]
}

static func adapt(_ results: [ContributionResult]) -> Adapted

For every result:

  1. map its diagnostics to ExportDiagnostic;
  2. if content == nil, append mapped diagnostics to unanchoredDiagnostics and continue;
  3. if content exists, keep those diagnostics attached to the ExportDerivedContribution only;
  4. .markdown -> render using the existing ExportService Markdown-fragment seam;
  5. unsupported .html -> empty generated HTML + appended anchored .error diagnostic stating authored source is preserved;
  6. preserve source range, placement and source generation exactly.

Never duplicate content-bearing diagnostics into the unanchored array.

E2. ExportCoordinator.swift

Have its contribution helper return ExportContributionAdapter.Adapted.

Pass only:

adapted.contributions

into ExportRequest.

After successful HTML export:

ExportOutcome(
    primaryFile: result.primaryFile,
    diagnostics: adapted.unanchoredDiagnostics + result.diagnostics
)

After successful PDF export:

ExportOutcome(
    primaryFile: selection.url,
    diagnostics: adapted.unanchoredDiagnostics + prepared.diagnostics
)

Reuse the coordinator's existing final diagnostics presentation. No fake source range, no E12 API extension.

Batch E tests

  • contentless result + error -> zero derived contributions, one unanchored diagnostic;
  • contentless result without diagnostics -> both arrays empty;
  • content-bearing warning -> only attached to contribution;
  • content-bearing error -> only attached to contribution, composer preserves authored source;
  • unsupported representation -> anchored error and authored fallback;
  • throwing registry contribution + valid authored document -> export succeeds, final diagnostics contain registry failure;
  • same merge seam exercised for HTML and PDF preparation outcomes;
  • no diagnostic duplication.

5. Batch F — complete regression + canonical gates

Run the full gates after all source/test changes:

swiftformat --lint MacDown2
swiftlint lint --strict MacDown2

cd MacDown2/Packages/MacDownKit
swift build
swift test --no-parallel
cd ../../..

cd MacDown2
xcodegen generate
cd ..

xcodebuild -project MacDown2/MacDown2.xcodeproj -scheme MacDown2 \
  -destination 'platform=macOS' build
xcodebuild -project MacDown2/MacDown2.xcodeproj -scheme macdown2 \
  -destination 'platform=macOS' build
xcodebuild -project MacDown2/MacDown2.xcodeproj -scheme MacDown2 \
  -destination 'platform=macOS' -enableCodeCoverage NO build-for-testing

Then push and require both GitHub lint and build-and-test jobs green on the final head.

A locally green package suite is not a substitute for the Xcode test-bundle compilation gate that is currently red.


6. Batch G — documentation / PR body only after final-head evidence exists

After Batch F is green:

  1. reconcile planning/epic-14-implementation.md with the actual implemented semantics;
  2. remove the old accepted whole-paragraph replacement limitation;
  3. document parse-revision Preview freshness, semantic TOC whitelist, inline capability, strict validation order, 64-placement + 4 MiB Preview budget, cancellation, and Preview/Export diagnostic transport;
  4. replace PR #54's stale body using architecture pass 10's exact section structure;
  5. record only verification results that actually ran on the final head;
  6. keep the PR draft if EPIC-14's text-filter half remains pending;
  7. do not close #15 merely because contribution/TOC remediation is green.

7. Final invariant checklist for the coding agent

Before declaring implementation complete, every answer below must be yes:

  • Does Preview use MarkdownDocument.revision as its source-snapshot token rather than FileDocument.mutationGeneration?
  • Can Save/mark-clean happen without making a rendered TOC disappear?
  • Does [TOC] inside fenced/indented/literal code stay authored?
  • Does Alpha\n[TOC]\nOmega preserve Alpha and Omega in Preview?
  • Are block contributions exact whole-source-line replacements?
  • Are only paragraph containers partially split?
  • Is .inline genuinely inline, constrained to one block/physical line and no generated line breaks?
  • Can inline + block contributions coexist in the same paragraph?
  • Does every malformed range fail before SourceMap.line(...)?
  • Are overlap decisions deterministic in source order?
  • Do invalid/stale/rejected candidates consume zero Preview placement budget?
  • Is Preview capped at 64 accepted placements and 4 MiB aggregate generated Markdown, with authored fallback + bounded diagnostics?
  • Can a >64 KiB generated Preview block still use the existing isOversize fallback instead of being silently discarded?
  • Does cancellation remain throwable until the SwiftUI task boundary?
  • Can a deliberately non-cooperative cancelled contributor fail to escape the registry?
  • Can a superseded Preview task perform zero state mutation after cancellation?
  • Are Preview blocks + diagnostics published as one composition value?
  • Does a contentless registry failure surface in Preview?
  • Does a contentless registry failure surface in final HTML/PDF export diagnostics?
  • Are error-bearing contributions never placed while warning-bearing valid contributions may be placed?
  • Is ExportService's existing public composition/resource architecture unchanged?
  • Does the exact CI build-for-testing gate pass at final head?
  • Does the PR body describe the actual final head rather than the earlier Slice-1 plan?

If any answer is no, implementation is not complete.


8. Cross-architecture review result

I re-checked this consolidated sequence against the current code seams:

  • DocumentEditorSplitView: compatible. The existing contributionResults state and body-time merged(...) call are replaced rather than layered on top. The existing previewBlocks remains the immediate fallback. The View's known type-length pressure is respected by keeping composition logic in the adapter.
  • PreviewBlock / Textual: compatible. Original blocks are preserved verbatim where untouched; generated fragments use existing block semantics; the established 64 KiB isOversize safety path remains authoritative for individual blocks.
  • ScrollSyncMap: compatible. Every split block owns ordered, non-overlapping original source line ranges; inline replacement never creates a second coordinate system.
  • MarkdownEngine: compatible. No parser or SourceMap behavior changes. Semantic TOC filtering reads existing block classification instead of reimplementing Markdown rules.
  • Contributions package: compatible. Renderer-neutral dependency direction remains intact; only cancellation discipline and TOC semantic consumption tighten.
  • ExportService: compatible. Existing E12 validation/resource/composition behavior is reused unchanged; anchorless diagnostics are joined at ExportCoordinator, where final user presentation already lives.
  • File lifecycle: compatible. Save/recovery mutationGeneration remains untouched; only Preview stops incorrectly treating that file-lifecycle counter as parse identity.
  • Future extensions: compatible. The adapter supports both block and constrained inline Markdown contributions without making arbitrary HTML or partial structured-container replacement silently legal.
  • Failure behavior: improved and consistently fail-closed. Stale/malformed/unsupported/over-budget/error-bearing work preserves authored source and becomes observable.
  • CI/review process: compatible. No gate is bypassed; documentation is deliberately deferred until evidence exists.

Architecture status: LOCKED for implementation. The only intentionally unresolved branch is the exact current UI-test-bundle compiler error; Batch A requires reproducing that error before editing because GitHub's available stored output proves the failing stage but not the first compiler diagnostic.

Copy link
Copy Markdown
Owner Author

Implementation hand-off — architecture locked for head a7aac6e44254817bb6679950cd24499fe92c669d

This is the implementation-order companion to the 10 validated findings and architecture passes 1–10. The later cross-pass correction is authoritative where it tightens earlier wording.

0. Restore the CI signal first

  • Reproduce the exact UI-test xcodebuild build-for-testing failure from workflow run 33585573413, job 100108958356.
  • GitHub confirms: package build/tests pass, project generation passes, app + CLI build passes, the UI-test bundle build fails, and lint passes.
  • The connector did not expose a retained first compiler diagnostic, so capture the first actionable compiler error from the exact reproduction instead of guessing at the fix.
  • Fix the first actionable build error. Do not waive, skip, or weaken the UI-test gate.
  • Re-run until package tests, app + CLI build, UI-test bundle build, and lint are all green.

1. Unify the source-snapshot and range contracts

  • Use MarkdownDocument.revision as Preview's contribution source-snapshot token.
  • Clarify ContributionResult.sourceGeneration as an opaque caller-supplied source-snapshot token; do not add another mutable generation counter.
  • Do not alter FileDocument save/mark-clean semantics to hide the invalidation bug.
  • Centralize/align Preview range acceptance with DerivedContentComposer: generation, bounds, non-empty, overlap, representation, and diagnostic-preserving rejection.
  • Validate before any SourceMap operation can clamp a malformed offset.

2. Make TOC discovery semantic and cancellation-cooperative

  • A physical [TOC] marker is eligible only when the line maps to a parsed ordinary .paragraph block.
  • Fenced and indented code remain literal.
  • The whitespace-trimmed marker remains valid when it is in a paragraph.
  • Add cooperative cancellation checks to potentially long marker scans.

3. Replace whole-block Preview substitution with exact UTF-16 composition

  • Do not replace an entire PreviewBlock merely because it contains a contribution range.
  • Admit only validated/eligible results.
  • Compose .inline and .block contributions in one deterministic algorithm/batch, ordered by descending UTF-16 source range so edits cannot invalidate later offsets.
  • Preserve adjacent authored source and support multiple markers in one paragraph.
  • If a mixed/unsafe case cannot be represented deterministically, preserve authored source and emit a diagnostic rather than mutating the wrong source.
  • Apply Preview resource limits after validation/eligibility: retain the 64 accepted-placement ceiling and add an aggregate generated-Markdown byte cap derived from the existing 64 KiB Preview oversize threshold. Budget rejection must emit a diagnostic. Do not raise Preview to Export's 4,096-fragment budget merely for symmetry.

4. Make Preview publication race-safe

  • Preview task identity must include document identity + parse revision.
  • Stamp contribution results with that revision/source-snapshot token.
  • ContributionRegistry checks cancellation before and after each contributor execution.
  • PreviewContributionAdapter rethrows CancellationError; never translate cancellation to [].
  • Check cancellation immediately before UI publication.
  • Publish rendered blocks and diagnostics atomically with one state value, e.g. PreviewContributionComposition; a cancelled/superseded task performs zero state mutations.

5. Surface diagnostics at both boundaries

  • Preview: reuse the existing top-right status surface for a subtle warning indicator with bounded hover/accessibility detail; do not add a parallel logging subsystem solely for this epic.
  • Export: retain diagnostic-only/anchorless contribution failures app-side in ExportCoordinator and merge them into the final ExportOutcome.diagnostics. Avoid expanding the E12 request/prepared-document API solely to transport anchorless diagnostics unless the coordinator seam proves insufficient.

6. Required regression/parity matrix

  • save/mark-clean does not clear a valid TOC Preview;
  • Alpha\n[TOC]\nOmega preserves Alpha and Omega;
  • two markers in one paragraph both render;
  • fenced and indented code markers remain literal;
  • .inline, .block, and mixed placement semantics are deterministic;
  • negative, empty, past-EOF, and overlapping ranges reject before mapping/splicing;
  • 64 eligible placements and aggregate-size exhaustion produce deterministic diagnostics, and invalid early results cannot starve later valid results;

  • cancellation propagates and superseded tasks do not publish;
  • diagnostic-only failures are visible in Preview and Export;
  • accepted TOC ranges demonstrate Preview/Export source-replacement parity.

7. Run every existing gate

  • MacDownKit/package tests;
  • Xcode project generation;
  • app + CLI build;
  • UI-test bundle build, plus execution where the runner supports it;
  • SwiftFormat + strict SwiftLint;
  • PR build-and-test and lint must both be green.

Do not mark the PR ready while a required gate is red.

8. Reconcile the PR body only after the implementation is green

  • Describe the scope that actually landed: first-party TOC, Preview path, Export path, diagnostics, limits, and cancellation behavior.
  • Replace the stale Slice-1-only test plan with the exact regression and CI evidence produced by remediation.
  • Do not pre-claim successful behavior or tests before that evidence exists.

9. Fresh independent adversarial re-review

  • Re-run the original correctness, CommonMark semantics, Preview/Export parity, extension-contract, range-boundary, cancellation, snapshot, budget, diagnostic, trust/safety, CI, and test-coverage axes after the fixes.
  • Resolve each original finding only when both implementation evidence and its regression test exist.
  • The already-documented clickable-TOC-anchor and unsupported-HTML-Preview limitations remain residual/out of scope unless this remediation deliberately changes them.

Guardrails

  • no new mutable source-generation subsystem;
  • no FileDocument save/clean semantic change to conceal invalidation;
  • no code-fence blacklist scanner — use parsed block semantics;
  • no CancellationError -> [] path;
  • no silent resource truncation or diagnostic loss;
  • no Preview limit of 4,096 merely for Export symmetry;
  • no weakening/disabling the UI-test gate;
  • no E12 API expansion solely for anchorless diagnostics unless genuinely necessary;
  • the cross-pass correction overrides any earlier wording that suggests separate inline/block composition passes or split Preview block/diagnostic state.

Completion criterion

All 10 findings are demonstrably closed, each has regression evidence, every required CI gate is green, the PR body matches the code that actually shipped, and a fresh post-fix adversarial review does not reopen a material issue.

Joncallim commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

Architecture takeover — implementation lock (authoritative)

Pinned review target: PR #54 head a7aac6e44254817bb6679950cd24499fe92c669d over base bbfb010074fbe44201ab6a8b1c4d6244c51ecf18.

I re-read the live implementation, all ten validated findings, the existing architecture comments, the exact failing CI log, the repository contribution rules, and the Epic 14 plan. The PR head remained unchanged throughout this architecture pass.

This comment is now the single authoritative coding hand-off. It supersedes earlier architecture comments wherever they differ. Those earlier comments remain useful review history, but the coding agent should implement only this version.

No production code was changed in this architecture pass.

Corrections made while taking over

  1. ContributionRegistry.standard already contains TOCContribution(). Do not re-register it.
  2. ContributionRegistry.contribute already checks cancellation before awaiting a contribution. Preserve that check; add a post-await/final check rather than rewriting the registry.
  3. TOC semantic admission must require a top-level parsed paragraph. Checking a recursively discovered paragraph is insufficient because paragraphs inside lists and block quotes are still literal/unsupported placement contexts for this first-party contribution.
  4. Content-bearing results that contain an .error diagnostic must be non-placeable in Preview, matching Export’s fail-closed contract.
  5. Composed Preview block IDs must be deterministic, but this does not justify widening the Preview package API. Generate IDs in the app adapter with system CryptoKit; add no package dependency.
  6. The CI failure is exact: ExportContributionAdapterTests.swift uses URL without importing Foundation. No production, project, package, or workflow change is justified for that failure.

Frozen compatibility invariants

These are implementation constraints, not suggestions.

  1. Authored Markdown remains durable source. Contributions may derive Preview/Export output; they may never mutate, normalize, or repair the editor source.
  2. Fail closed. Any stale, malformed, unsupported, over-budget, overlapping, or failed contribution leaves the corresponding authored source visible and exportable.
  3. No public SPI rename. Keep ContributionResult.sourceGeneration; revise its documentation so it is an opaque caller-selected snapshot token compared only for equality.
  4. Destination-specific tokens remain valid. Preview uses MarkdownDocument.revision; Export may continue using the captured FileDocument.mutationGeneration because its request is built from a captured export snapshot.
  5. UTF-16 is the sole source-coordinate system. Every range is half-open and measured against the exact captured source string used to parse the captured MarkdownDocument.
  6. No clamping during admission. SourceMap clamping helpers may not convert an invalid placement into an apparently valid one.
  7. Untouched Preview blocks pass through byte-for-byte/value-for-value, including their existing IDs, kinds, attributed content, and source line ranges.
  8. Affected Preview blocks have deterministic IDs. Re-rendering the same parsed snapshot and contributions must return equal IDs.
  9. Preview block order and source line ranges remain monotonic and non-overlapping, preserving TextualMarkdownPreview and ScrollSyncMap assumptions.
  10. Cancellation is control flow. It is never converted into an empty successful contribution set and never surfaced as a user diagnostic.
  11. No new dependency, target, package, or public E12 export API. New app/test files are discovered by the existing recursive XcodeGen source configuration.
  12. No gate weakening. Do not delete assertions, loosen resource limits, skip tests, or change CI to make the branch green.

Shared Preview composition seam

Findings 2, 5, 6, 7, and part of 9 converge on MacDown2/MacDown2/PreviewContributionAdapter.swift. Implement them once through a source-ordered, fail-closed composer rather than stacking special cases.

App-private result types

Replace the current catch-and-return-empty/merged(...) shape with app-private equivalents of:

struct PreviewContributionDiagnostic: Sendable, Equatable {
    let contributionID: String
    let severity: ContributionDiagnostic.Severity
    let message: String
}

struct PreviewContributionComposition: Sendable, Equatable {
    let sourceGeneration: UInt?
    let blocks: [PreviewBlock]?
    let diagnostics: [PreviewContributionDiagnostic]

    static let empty = PreviewContributionComposition(
        sourceGeneration: nil,
        blocks: nil,
        diagnostics: []
    )
}

struct PreviewContributionBudget: Sendable, Equatable {
    let maximumAcceptedPlacements: Int
    let maximumGeneratedMarkdownUTF8Bytes: Int

    static let standard = PreviewContributionBudget(
        maximumAcceptedPlacements: 64,
        maximumGeneratedMarkdownUTF8Bytes: 64 * 1024
    )
}

Names may follow repository style, but the ownership and fields must remain equivalent. results(...) becomes async throws; it must not catch cancellation or convert any thrown error to [].

Use one pure composition entry point with all dependencies explicit:

static func compose(
    base: [PreviewBlock],
    document: MarkdownDocument,
    sourceText: String,
    contributions: [ContributionResult],
    sourceGeneration: UInt,
    budget: PreviewContributionBudget = .standard
) -> PreviewContributionComposition

Do not read FileDocument, MarkdownParseSession, SwiftUI state, the filesystem, or globals from this function. That keeps range, placement, budget, and diagnostic behavior directly unit-testable.

Strict admission pipeline

Process each original ContributionResult in registry order. Preserve producer diagnostic order. A result’s diagnostics are copied into the composition before deciding whether its content is placeable.

For each content-bearing result, apply this exact order:

  1. Reject placement when any result diagnostic has severity .error.
  2. Require result.sourceGeneration == sourceGeneration.
  3. Require Markdown content. HTML is unsupported by textual Preview and must produce an adapter error/warning while preserving authored source.
  4. Require generated Markdown to be non-empty after whitespace trimming.
  5. Validate source bounds directly against (sourceText as NSString).length: lower >= 0, upper > lower, and upper <= sourceUTF16Length.
  6. Before any source-map lookup, require document.sourceMap.utf16Length == sourceUTF16Length.
  7. Convert every base block’s existing source line range into a source interval only after validating that the line range is inside the source map. Require base intervals to be ordered and non-overlapping.
  8. Require exactly one base block interval to contain the whole placement. Ranges in newline gaps, across blocks, or outside all blocks are invalid.
  9. Enforce placement capability:
    • .inline: the replaced source and generated Markdown must contain no \r or \n; the range must remain within one physical line. Splice in place and retain the containing block’s original kind.
    • .block: exact replacement of the entire containing block is allowed for any block kind. Partial replacement is allowed only inside a top-level paragraph and only when the range equals one or more complete physical-line source ranges returned by the captured SourceMap.
  10. Store the original registry/result index with the candidate.

After validation, sort candidates by (sourceLowerBound, originalResultIndex).

Resolve overlap before budgeting. Accepted candidates reserve their source interval; rejected candidates do not. The first valid candidate in source order wins. An overlapping later result receives an adapter diagnostic and leaves its source untouched.

Apply the Preview budget only to otherwise-valid, non-overlapping candidates. Count only accepted placements and use addingReportingOverflow for generated UTF-8 byte totals. An oversized candidate is rejected without preventing a later smaller candidate from fitting. Emit one aggregate warning stating how many valid placements were left as authored source. There must be no prefix(64) or other silent truncation.

Source-ordered block composition

Group accepted candidates by their containing base block. Walk base blocks in their original order.

  • If a base block has no accepted placements, append the exact original PreviewBlock instance.
  • For an affected block, walk accepted placements from left to right with a UTF-16 cursor starting at the base block interval’s lower bound.
  • Inline placements are spliced into the current authored buffer.
  • Before a block placement, flush any non-whitespace authored/inline buffer as a rendered .paragraph fragment, emit one .custom(contributionID) generated block, then move the cursor to the placement upper bound.
  • After a block placement, consume exactly one immediately following LF code unit when present. SourceMap line ranges already include the CR in CRLF input and exclude the LF, so this rule handles LF and CRLF without consuming authored content.
  • Flush the final authored/inline suffix after the last placement.
  • A whole-block replacement emits only the generated block.
  • Do not emit whitespace-only authored fragments.

Calculate each emitted fragment’s absolute source line range from its validated UTF-16 span. Never invent a line number and never clamp one. The generated block owns the placement’s source line range. Before returning, verify the complete output remains ordered and non-overlapping. A failed final invariant returns the original base array and an internal adapter error rather than crashing or publishing partial output.

Render fragments through the existing PreviewMarkupParser and existing typography path. Do not reparse the whole document and do not change PreviewMarkupParser, PreviewBlock, or ScrollSyncMap public API.

Deterministic IDs for affected blocks

Keep untouched IDs exactly. For each emitted fragment, derive a private app-side UUID from a stable serialization of:

  • containing base block UUID;
  • fragment role (authored, inline-composed, or generated);
  • absolute UTF-16 lower and upper bounds;
  • contribution ID for generated content; and
  • stable ordinal only when two fragments would otherwise have the same key.

Hash the UTF-8 key with CryptoKit.SHA256 and construct the UUID from the first 16 digest bytes. Do not use Swift Hasher, hashValue, or a random default UUID. Do not expose the package-private Preview ID helper and do not add CryptoKit as an SPM dependency; it is a system framework imported only by the app adapter.


Architecture pass 1/10 — Preview freshness uses the parsed snapshot

Finding: non-text FileDocument mutations can invalidate a valid TOC because work is triggered by a parsed-document snapshot but accepted against mutationGeneration.

Files and symbols

  • Packages/MacDownKit/Sources/Contributions/ContributionResult.swift
  • MacDown2/DocumentEditorSplitView.swift
  • app tests for task-key/freshness behavior

Exact implementation

  1. Change only the documentation of sourceGeneration: it is an opaque snapshot token supplied by the caller and must only be compared for equality. Do not mention FileDocument.mutationGeneration in the SPI contract.
  2. Add an app-private task key:
struct PreviewContributionTaskID: Hashable {
    let documentIdentity: ObjectIdentifier
    let parsedRevision: Int?
}
  1. Build .task(id:) from ObjectIdentifier(document) and parseSession.document?.revision. Do not include save state, dirty state, URL, display name, encoding, or mutationGeneration.
  2. At task entry, capture the currently published MarkdownDocument and its paired published source text. Guard that they still match the task ID before doing work.
  3. Convert revision with UInt(exactly:). A failed conversion is an internal diagnostic with unchanged base Preview; never use a wrapping or truncating conversion.
  4. Pass this token into ContributionRegistry and the Preview composer.
  5. Hold one atomic PreviewContributionComposition in state. Do not maintain independently publishable result/block/diagnostic arrays.
  6. Before publishing success or unexpected failure, compare the current document identity and parsed revision with the captured task ID. A stale task performs no state mutation.
  7. In body, use composed blocks and diagnostics only when their token equals UInt(exactly: currentParsedDocument.revision). Otherwise render current base blocks and hide old diagnostics.
  8. A non-text FileDocument transition therefore neither restarts the task nor invalidates the active composition. A text parse revision or document identity change does both.
  9. Leave Export generation capture unchanged.

Required tests

  • Same document + same parsed revision + changed dirty/save/URL metadata yields the same task ID.
  • Same document + next parsed revision yields a different task ID.
  • Different FileDocument object + same revision yields a different task ID.
  • A completion for revision N cannot publish after revision N+1 is current.
  • Non-text mutation does not clear a valid composed TOC.

Immediate architecture review

PASS (design-level). This separates Preview freshness from persistence state without changing FileDocument mutation semantics, parser publication semantics, or Export snapshot validity. The implementation is not considered verified until the tests above execute.


Architecture pass 2/10 — Preserve authored text around a marker in one CommonMark paragraph

Finding: replacing the containing Preview block deletes/hides adjacent lines when [TOC] is one physical line inside a multi-line paragraph.

Files and symbols

  • MacDown2/PreviewContributionAdapter.swift
  • MacDown2Tests/PreviewContributionAdapterTests.swift

Exact implementation

Use the shared source-ordered composer above. A block placement inside a paragraph is a source splice, not a request to discard the paragraph block.

For:

before
[TOC]
after

Preview must produce, in order:

  1. rendered authored before fragment;
  2. generated .custom("toc") fragment owning the marker line;
  3. rendered authored after fragment.

The authored prefix and suffix must come from the exact captured source UTF-16 slices. Never reconstruct them by joining logical lines or by slicing the already-rendered attributed string.

Required tests

  • Prefix/marker/suffix all survive and remain in source order.
  • Multiple markers in one paragraph compose deterministically.
  • Inline and block placements coexist in one paragraph.
  • LF and CRLF inputs produce equivalent visible content and correct source line ranges.
  • Emoji/non-BMP text before and after the marker proves UTF-16 offsets are respected.
  • Untouched neighboring blocks retain their original IDs and values.
  • Repeating composition with identical input returns identical affected IDs.

Immediate architecture review

PASS (design-level). The change is local to derived Preview composition, preserves the editor source, and retains scroll-map ordering. Splitting the authored paragraph around a declared .block placement is intentional and matches the contribution contract rather than CommonMark’s original single-paragraph grouping.


Architecture pass 3/10 — Discover TOC markers from parsed semantics

Finding: physical-line scanning interprets [TOC] inside fenced and indented code as executable contribution syntax.

Files and symbols

  • Packages/MacDownKit/Sources/Contributions/TOCContribution.swift
  • Packages/MacDownKit/Tests/ContributionsTests/TOCContributionTests.swift

Exact implementation

  1. Retain the current lexical rule: after the same leading/trailing whitespace handling already supported, the physical line must equal case-sensitive [TOC]. Do not add aliases or broaden syntax in this remediation.
  2. For each lexical candidate, obtain its exact UTF-16 line source range from the captured SourceMap.
  3. Search only document.blocks, the top-level parse blocks. Accept the candidate only when exactly one top-level block contains the whole line range and that block kind is .paragraph.
  4. Do not use recursive block(atLine:) as the admission decision; it can return nested paragraphs inside block quotes/lists.
  5. Emit the existing .block placement for accepted markers.
  6. Use the same semantic result for Preview and Export by keeping discovery inside TOCContribution, not in either destination adapter.

This intentionally treats a marker line inside a multi-line top-level paragraph as syntax, while markers inside fenced code, indented code, lists, block quotes, headings, front matter, HTML blocks, or any other non-paragraph top-level container remain literal.

Required tests

  • Standalone marker and marker in a multi-line top-level paragraph are accepted.
  • Fenced code (backticks and tildes), indented code, and tab-indented code are rejected.
  • Ordered/unordered list items and lazy continuations are rejected.
  • Block quotes, including lazy continuation paragraphs, are rejected.
  • Heading, front matter, and HTML-block occurrences are rejected.
  • LF and CRLF accepted-marker ranges are exact.
  • Preview and Export receive the same accepted marker set.

Immediate architecture review

PASS (design-level). This delegates Markdown semantics to the existing parser, avoids a second parser, and prevents Preview/Export divergence. Requiring a top-level paragraph also matches the Preview placement capabilities defined in pass 5.


Architecture pass 4/10 — Restore build-and-test without changing production

Finding: the pinned head is red while the exact base is green.

File

  • MacDown2/MacDown2Tests/ExportContributionAdapterTests.swift

Exact implementation

Add:

import Foundation

The failing symbols are the three URL(...) uses in that test file. Do not add imports elsewhere, change deployment targets, alter XcodeGen, edit CI, remove the tests, or replace URL with a weaker test value.

Required verification

Run the previously failing app test build first, then the complete repository sequence listed below. Preserve the failing log in the PR evidence until a newer run proves it green.

Immediate architecture review

PASS (design-level). The remedy is the narrow missing module import reported by the compiler. It has no production runtime effect and does not mask another known failure.


Architecture pass 5/10 — Enforce .inline and .block placement semantics

Finding: Preview currently treats both placement kinds as whole-block replacement.

Files and symbols

  • MacDown2/PreviewContributionAdapter.swift
  • MacDown2Tests/PreviewContributionAdapterTests.swift
  • documentation comments in the contribution placement/content SPI where currently ambiguous

Exact implementation

Use the admission and composition rules frozen above:

  • .inline replaces only its validated source span inside one physical line, may not introduce CR/LF, preserves authored neighbors, and retains the base block kind.
  • .block owns a Preview block boundary. Exact whole-base-block replacement is legal for any kind. Partial block replacement is legal only for complete physical line(s) inside a top-level paragraph.
  • Unsupported placement/content combinations produce a diagnostic and preserve source.
  • Do not reinterpret .inline as .block, silently promote partial ranges, or change the public enum.

Required tests

  • Inline replacement preserves prefix/suffix and base block kind.
  • Inline source spanning lines is rejected.
  • Inline generated Markdown containing LF or CRLF is rejected.
  • Whole-block .block replacement works for a non-paragraph base block.
  • Partial .block inside a paragraph works only on complete physical lines.
  • Partial .block inside a heading/code/list/quote is rejected.
  • Mixed non-overlapping inline/block placements preserve source order.

Immediate architecture review

PASS (design-level). The existing public contract gains enforcement rather than a new interpretation. Future math/diagram contributions can rely on a single deterministic placement model.


Architecture pass 6/10 — Reject malformed ranges; never clamp them onto content

Finding: malformed source ranges can be clamped to unrelated Preview blocks.

Files and symbols

  • MacDown2/PreviewContributionAdapter.swift
  • MacDown2Tests/PreviewContributionAdapterTests.swift

Exact implementation

  1. Validate source/document coherence and raw bounds before calling any SourceMap conversion.
  2. Reject negative, empty/reversed, past-end, newline-gap-only, cross-block, and no-containing-block ranges.
  3. Require exactly one containing base interval.
  4. Use source-map round trips only as equality checks after validation; a clamped result is never accepted merely because it points somewhere.
  5. Invalid candidates do not reserve overlap intervals and do not consume the budget.
  6. Preserve the exact authored base block and add a stable adapter diagnostic identifying the contribution and reason.

Required tests

  • Lower bound below zero.
  • Empty and reversed range.
  • Upper bound beyond UTF-16 length.
  • Range containing only LF between blocks.
  • Range crossing two blocks.
  • Range valid for a different source string/document pair.
  • Non-BMP source proving bounds use UTF-16 rather than grapheme or UTF-8 counts.
  • A malformed earlier candidate does not prevent a later valid candidate.

Immediate architecture review

PASS (design-level). This removes the dangerous use of clamping as validation while retaining SourceMap for already-valid coordinate conversion. The adapter fails locally and preserves all authored text.


Architecture pass 7/10 — Make the Preview budget explicit and observable

Finding: Preview silently truncates to 64 results while Export has an explicit 4096-fragment/32 MiB resource contract.

Files and symbols

  • MacDown2/PreviewContributionAdapter.swift
  • MacDown2Tests/PreviewContributionAdapterTests.swift

Exact implementation

  1. Delete the silent prefix(64) path.
  2. Keep Preview intentionally smaller than Export through the explicit PreviewContributionBudget.standard: 64 accepted placements and 64 KiB total generated Markdown UTF-8 bytes.
  3. Count only candidates that passed generation, content, diagnostic, range, placement, and overlap validation.
  4. Use overflow-safe byte accumulation.
  5. Reject an over-budget candidate while leaving its marker/source visible; continue evaluating later candidates so a later smaller result can still fit.
  6. Emit exactly one aggregate warning with the number of valid candidates omitted by the budget.
  7. Do not modify ExportResourceBudget, its 4096-fragment limit, or its 32 MiB limit.

Required tests

  • 64 accepted placements compose; the 65th remains authored with one warning.
  • Invalid/stale/overlapping candidates do not consume count budget.
  • Generated byte boundary at limit and one byte above it.
  • Overflow path is handled without trap.
  • An oversized candidate followed by a small valid candidate allows the small candidate when capacity remains.
  • Budget warning appears in the visible diagnostic model.

Immediate architecture review

PASS (design-level). Preview keeps bounded work without silent data loss or accidental coupling to Export limits. Every omission becomes visible and reversible.


Architecture pass 8/10 — Propagate cancellation end-to-end

Finding: Preview converts cancellation into an empty successful result, which can publish a false “no contributions” state.

Files and symbols

  • Packages/MacDownKit/Sources/Contributions/ContributionRegistry.swift
  • Packages/MacDownKit/Tests/ContributionsTests/ContributionRegistryTests.swift
  • MacDown2/PreviewContributionAdapter.swift
  • MacDown2/DocumentEditorSplitView.swift

Exact implementation

  1. Preserve the existing pre-await Task.checkCancellation() in ContributionRegistry.contribute.
  2. Immediately after each contribution returns, call try Task.checkCancellation() before appending its result. Add a final check before returning so an empty registry also honors cancellation.
  3. Preserve the existing behavior that rethrows CancellationError; ordinary contribution failures may continue to become contribution diagnostics under the registry’s current contract.
  4. Make PreviewContributionAdapter.results(...) async throws and remove its catch-all [] fallback.
  5. In DocumentEditorSplitView:
    • capture the parsed snapshot/task ID;
    • call the registry;
    • check cancellation before composition and again before publication;
    • catch CancellationError and return without any state mutation;
    • publish an unexpected-error diagnostic only when the captured task ID is still current.
  6. Never display cancellation as an error and never clear a valid previous composition merely because a replacement task was cancelled. Revision gating already prevents stale content from displaying on a newer parse.

Required tests

  • Task cancelled before registry execution throws CancellationError.
  • A cooperative contribution cancellation propagates.
  • A deliberately non-cooperative contribution catches its own cancellation and returns; the registry’s post-await check still throws.
  • Empty registry called by an already-cancelled task throws.
  • Cancelled Preview task does not publish empty blocks or diagnostics.
  • Revision N completion cannot overwrite revision N+1.

Immediate architecture review

PASS (design-level). The change strengthens Swift cooperative-cancellation semantics without changing ordinary contribution-error conversion. No cancelled work can masquerade as a successful empty result.


Architecture pass 9/10 — Surface diagnostic-only failures in Preview and Export

Finding: a ContributionResult with diagnostics but no content disappears, despite the SPI promising that callers surface diagnostics.

Preview files and implementation

  • MacDown2/PreviewContributionAdapter.swift
  • MacDown2/DocumentEditorSplitView.swift
  • new MacDown2/PreviewContributionDiagnosticsView.swift
  • Preview adapter/view tests
  1. Carry diagnostics in the same atomic composition as the derived blocks and generation token.
  2. Preserve diagnostic-only results even when no block is emitted.
  3. Treat any content-bearing result with an .error diagnostic as non-placeable; keep authored source visible.
  4. Add a compact non-modal diagnostics control at the Preview’s top-trailing edge. Replace the existing single busy-indicator overlay with one HStack/container that can show both PreviewBusyIndicator and the diagnostics badge without overlap.
  5. The badge exposes highest severity and count; activation opens a list in deterministic result/diagnostic order with contribution ID, severity, and message.
  6. Add stable accessibility identifiers/labels for the badge, count, and rows. Do not use a modal alert for continuously recomputed Preview diagnostics.
  7. Show only diagnostics whose composition generation matches the currently displayed parsed revision.

Export files and implementation

  • MacDown2/ExportContributionAdapter.swift
  • MacDown2/ExportCoordinator.swift
  • MacDown2Tests/ExportContributionAdapterTests.swift
  • a focused coordinator-diagnostic test/helper in the app test target

Change the app-private adapter return to:

struct Adaptation {
    let contributions: [ExportDerivedContribution]
    let standaloneDiagnostics: [ExportDiagnostic]
}

Mapping rules:

  • content == nil: convert all result diagnostics to standaloneDiagnostics.
  • content != nil: attach diagnostics only to that ExportDerivedContribution; do not also place them in the standalone array.
  • Preserve result and diagnostic order.

Both HTML and PDF paths in ExportCoordinator must pass adaptation.contributions to the existing Export service and surface:

adaptation.standaloneDiagnostics + serviceResult.diagnostics

exactly once through the existing export diagnostic presentation. Extract one small internal combination/presentation helper and make both paths call it; test the helper so the two paths cannot drift through copy-pasted merge logic.

Do not fabricate source ranges, create empty derived fragments, alter E12 public result types, or display the same content-bearing diagnostic twice.

Required tests

  • Preview retains and exposes diagnostic-only warning/error results.
  • Preview blocks content carrying an .error diagnostic while preserving source.
  • Preview warning + valid content remains placeable.
  • Diagnostics from a stale generation are not displayed.
  • Busy and diagnostic controls coexist and have accessibility identifiers.
  • Export diagnostic-only result appears in standaloneDiagnostics.
  • Content-bearing diagnostic appears only on the derived contribution.
  • Coordinator merge order is standalone first, service diagnostics second, with no duplicates, for both HTML and PDF call paths.

Immediate architecture review

PASS (design-level). Diagnostics become visible without polluting Markdown, inventing placement, widening E12, or blocking editing with repeated alerts. Error-bearing content fails closed consistently across Preview and Export.


Architecture pass 10/10 — Restore hand-off integrity

Finding: the PR description claims only the initial registry/protocol slice exists, that ContributionRegistry.standard is empty, and that no user-visible behavior has landed; the pinned head already contains TOC plus Preview and Export integration.

Files/surfaces

Exact implementation

After code and tests are complete—but before requesting review—rewrite the PR description from the actual final diff. It must include:

  1. the contribution SPI and registry actually present;
  2. the first-party TOC behavior and exact accepted context;
  3. Preview and HTML/PDF Export integration actually present;
  4. the explicit Preview and Export resource budgets;
  5. cancellation and diagnostic behavior;
  6. what remains out of scope, including text-filter implementation and clickable-anchor/HTML-Preview work;
  7. the exact commands run and their observed outcomes;
  8. manual evidence still unverified, if any;
  9. current risk/rollback notes; and
  10. links to the ten findings and this authoritative hand-off.

Remove stale statements, generated-session/footer links, and any claim that a check passed without current evidence. Reconcile the Epic 14 plan and source comments with the final .inline/.block, semantic marker, opaque token, diagnostic, and budget contracts. Do not rewrite unrelated planning history.

Required review

  • Compare every “implemented” PR-body bullet with the final diff.
  • Compare every “verified” claim with a current command/check result.
  • Keep the PR in Draft while any blocker, required test, Release build, or manual evidence remains unresolved.

Immediate architecture review

PASS (design-level). Documentation is updated last, from implementation truth, avoiding another stale hand-off while preserving useful planning history.


Coding order and per-slice review gates

Implement in this order. Commit boundaries are optional, but each slice must be independently reviewable and green before the next begins.

Slice 0 — Restore compilability

  • Add import Foundation to ExportContributionAdapterTests.swift.
  • Re-run the exact failing app test build.
  • Review the diff: one test import only.

Slice 1 — Contract and producer correctness

  • Generalize sourceGeneration documentation.
  • Add registry post-await/final cancellation checks.
  • Make TOC discovery top-level-paragraph semantic.
  • Add package tests.
  • Run swift build and serialized swift test --no-parallel in Packages/MacDownKit.
  • Review for public API drift and Preview/Export marker parity.

Slice 2 — Pure Preview adapter/composer

  • Add atomic composition/diagnostic/budget types.
  • Implement strict admission, placement, overlap, budget, deterministic IDs, and source-ordered composition.
  • Remove silent cap and catch-all empty fallback.
  • Add exhaustive app adapter tests.
  • Run focused app tests, then build-for-testing.
  • Review every reject path for authored-source preservation and every accept path for ordered line ranges.

Slice 3 — Preview task ownership and visible diagnostics

  • Re-key work to document identity + parsed revision.
  • Add stale/cancellation publication guards.
  • Replace split state with atomic composition.
  • Add diagnostics control beside the busy indicator.
  • Add task-ID/state/UI tests.
  • Run focused tests plus build-for-testing.
  • Review rapid-edit, save-state, accessibility, and overlay behavior.

Slice 4 — Export diagnostic side channel

  • Return Adaptation from the app-private Export adapter.
  • Merge standalone diagnostics once in both HTML and PDF coordinator paths.
  • Add adapter/coordinator tests.
  • Review for duplicate diagnostics, fake placements, and any E12 public API change.

Slice 5 — Full verification and hand-off repair

  • Run every repository gate below.
  • Perform the manual matrix.
  • Re-read the complete diff against all ten findings.
  • Update the PR description and Epic 14 documentation from observed truth.
  • Fetch the PR discussion after editing and verify the hand-off landed.

Do not combine Slice 2 and Slice 3 into a large untestable SwiftUI rewrite. The pure adapter must be green before view integration.


Exact verification matrix

Run from the repository root with Xcode 26 selected, matching CI:

swiftformat --lint MacDown2
swiftlint lint --strict MacDown2

cd MacDown2/Packages/MacDownKit
swift build
swift test --no-parallel
cd ../../..

cd MacDown2
xcodegen generate
cd ..

xcodebuild \
  -project MacDown2/MacDown2.xcodeproj \
  -scheme MacDown2 \
  -destination 'platform=macOS' \
  build

xcodebuild \
  -project MacDown2/MacDown2.xcodeproj \
  -scheme macdown2 \
  -destination 'platform=macOS' \
  build

xcodebuild \
  -project MacDown2/MacDown2.xcodeproj \
  -scheme MacDown2 \
  -destination 'platform=macOS' \
  -enableCodeCoverage NO \
  build-for-testing

Also execute the app test suite on a compatible macOS 26 host:

xcodebuild \
  -project MacDown2/MacDown2.xcodeproj \
  -scheme MacDown2 \
  -destination 'platform=macOS' \
  -enableCodeCoverage NO \
  test

The current CI workflow builds the app tests but does not prove they executed. Record the local/host test result separately; do not infer it from build-for-testing.

Build a Release configuration before merge:

xcodebuild \
  -project MacDown2/MacDown2.xcodeproj \
  -scheme MacDown2 \
  -configuration Release \
  -destination 'platform=macOS' \
  build

Manual matrix, with authored Markdown visibly checked after every case:

  • standalone TOC in Preview, HTML Export, and PDF Export;
  • marker between adjacent authored paragraph lines;
  • fenced, indented, list, quote, front-matter, heading, and HTML-block literals;
  • LF and CRLF documents;
  • emoji/non-BMP text around marker;
  • save, Save As, dirty-state transitions, rename/URL changes, and other non-text state changes;
  • rapid consecutive edits causing cancellation;
  • 65+ valid markers and generated-byte overflow warning;
  • malformed/overlapping injected test contributions;
  • diagnostic-only warning/error and content-bearing error;
  • busy indicator and diagnostics badge together;
  • reopen exported/original document and confirm authored source was never modified.

Stop conditions

Stop the slice and re-architect rather than improvising when any of these occurs:

  • the PR head changes from the pinned SHA before implementation is based on it;
  • a fix appears to require a new public SPI symbol, E12 API change, package dependency, deployment-target change, or CI relaxation;
  • Preview composition requires clamping an invalid range, reparsing the entire authored document, or fabricating source coordinates;
  • affected IDs cannot be deterministic without changing Preview’s public API;
  • output line ranges cannot remain ordered/non-overlapping;
  • a test passes only after deleting/weakening an assertion or increasing a resource limit without evidence;
  • HTML and PDF require different contribution admission rules;
  • cancellation must be represented as a normal diagnostic/result; or
  • Release/manual evidence cannot be obtained. Record it as unverified and keep the PR Draft rather than claiming completion.

Merge definition

PR #54 is merge-ready only when all four P1s and all five P2 contract defects are fixed, their focused regressions execute, package tests pass, app tests execute on macOS 26, lint/build/build-for-testing/Release gates pass, the manual matrix has recorded evidence, the PR description matches the final diff, and a final head-SHA/discussion fetch confirms the reviewed state is the state being merged.

@Joncallim Joncallim left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architecture pass 4/10 — validation addendum: exact CI repair

This resolves the conditional branch in review 5108936542 against the actual failed job log.

Confirmed root cause

The package tests, generated-project step, and app/CLI build all succeeded. The failure is confined to the UI-test bundle build. Xcode reports:

MacDown2Tests/ExportContributionAdapterTests.swift:86:13: error: cannot find 'URL' in scope
MacDown2Tests/ExportContributionAdapterTests.swift:92:13: error: cannot find 'URL' in scope

ExportContributionAdapterTests.swift uses URL(...) but does not import the module that defines it. This is a compile-time test-source regression, not an XcodeGen, target-membership, scheme, linker, or runner problem.

Exact implementation

Change only:

  • MacDown2/MacDown2Tests/ExportContributionAdapterTests.swift

Add the missing import beside the existing imports:

import Foundation

Do not:

  • change project.yml or any target dependency;
  • add Foundation to production files merely to make the test compile;
  • weaken or skip the UI-test build step;
  • alter the workflow runner/Xcode version;
  • move these tests to another target;
  • use a fully qualified workaround such as Foundation.URL at each call site.

Verification sequence

Run the smallest previously failing build first:

cd MacDown2
xcodegen generate
xcodebuild build-for-testing \
  -project build/MacDown2.xcodeproj \
  -scheme MacDown2UITests \
  -destination 'platform=macOS'

Then execute the repository's normal package tests, app/CLI build, strict lint/format checks, and the full CI workflow. The PR remains blocked until the same build-and-test check that failed on head is green.

Immediate hostile compatibility review

Production behavior: PASS. Test-only import; no runtime code changes.

Module graph: PASS. Foundation is an SDK module already available to the test target; no package or project dependency mutation is needed.

CI integrity: PASS. The failing gate is repaired rather than bypassed.

Scope control: PASS. The logs identify one deterministic compiler failure. Broader build-system changes would be unsupported and risky.

Conclusion: candidate A from pass 4 is confirmed. The implementation is exactly one import plus verification; no architectural change is authorized for this blocker.

@Joncallim Joncallim left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architecture pass 5/10 — P2: implement .inline Preview placement without changing block ownership

Original finding: issue comment 5534701855.

Required invariant

ContributionPlacement has one meaning on both surfaces:

  • .block replaces an exact, line-owned directive range and becomes one or more standalone Preview blocks, as specified in pass 2;
  • .inline replaces only its exact UTF-16 source span inside the authored block that owns it. It must not replace, split, or acquire the line ownership of the containing block.

Preview must never silently reinterpret .inline as .block.

Files to change

  • MacDown2/MacDown2/PreviewContributionAdapter.swift
  • MacDown2/Packages/MacDownKit/Sources/Contributions/ContributionContent.swift — tighten documentation only
  • MacDown2/MacDown2Tests/PreviewContributionAdapterTests.swift
  • planning/epic-14-implementation.md — record the final placement contract

No change is required to PreviewBlock, TextualMarkdownPreview, ScrollSyncMap, MarkdownEngine, or ExportService.

Contract refinement

Keep the existing enum and public signatures. Clarify the .inline documentation:

/// Replaces an exact source span inside one source line and one Preview block.
/// A Markdown representation used with `.inline` must be an inline fragment:
/// it must contain neither `\n` nor `\r`.
case inline

This is not a new feature restriction. The existing name already promises inline placement; allowing line breaks would make source-line ownership and Preview/Export parity undefined.

Final composition model

Pass 6 supplies a sorted array of validated placements. Each placement must carry:

private struct ValidatedPlacement {
    let ordinal: Int
    let contributionID: String
    let sourceRange: Range<Int>
    let placement: ContributionPlacement
    let markdown: String
    let blockIndex: Int
    let lineRange: ClosedRange<Int>
}

ordinal is the result's original registry order and is only the final deterministic tie-breaker. blockIndex is resolved once during validation; do not repeatedly scan blocks during composition.

For each base block, partition accepted placements into inline and block arrays.

  • no placements: append the original PreviewBlock value unchanged;
  • inline placements only: emit one rewritten PreviewBlock with the original kind and lineRange;
  • block placements present: use pass 2's line-oriented split, applying inline replacements only to authored fragments that remain around the block placements.

Inline eligibility rules

After the generic range checks in pass 6, accept an inline placement only when all of these hold:

  1. the range is wholly contained by exactly one base block's absolute UTF-16 range;
  2. sourceMap.line(atUTF16Offset: range.lowerBound) equals sourceMap.line(atUTF16Offset: range.upperBound - 1);
  3. the Markdown representation contains neither \n nor \r;
  4. the range does not overlap another accepted placement;
  5. if the same base block also has accepted block placements, the inline range lies wholly within one surviving authored fragment, not on a line owned by a block placement.

A failed rule preserves authored source and emits one adapter diagnostic. Do not truncate the range, promote it to a block, or drop it silently.

Exact UTF-16 splice algorithm

Add an app-private helper:

private static func spliceInline(
    source: String,
    absoluteSourceRange: Range<Int>,
    placements: [ValidatedPlacement]
) -> String

absoluteSourceRange is the UTF-16 range represented by source. For a whole base block it comes from sourceMap.utf16Range(ofLines: block.lineRange); for a pass-2 authored fragment it comes from that fragment's line range.

Behavior:

let units = Array(source.utf16)
var output = ""
var cursor = 0

for placement in placements { // already sorted and non-overlapping
    let lower = placement.sourceRange.lowerBound - absoluteSourceRange.lowerBound
    let upper = placement.sourceRange.upperBound - absoluteSourceRange.lowerBound

    output += String(decoding: units[cursor ..< lower], as: UTF16.self)
    output += placement.markdown
    cursor = upper
}

output += String(decoding: units[cursor...], as: UTF16.self)
return output

Use safe empty-tail handling rather than literally forming a closed range at units.count. The key requirements are one materialisation of UTF-16 units and one forward pass. Do not repeatedly convert absolute offsets with String.Index(utf16Offset:in:), and do not apply replacements in ascending order to a mutating String.

Construct the rewritten block as:

PreviewBlock(
    kind: original.kind,
    source: rewrittenSource,
    lineRange: original.lineRange
)

The original block's line ownership is unchanged. Its ID should change because its rendered source changed; the normal initializer is correct, and pass 1 guarantees this value is composed once per parse snapshot rather than recreated on every SwiftUI body evaluation.

Mixed inline/block placements

Pass 2's appendAuthoredFragment must accept the validated inline placements assigned to that fragment. Its sequence is:

  1. extract the authored fragment with SourceMap + NSString;
  2. call spliceInline using the fragment's absolute UTF-16 range;
  3. append one PreviewBlock with the original container kind and fragment line range.

Accepted block placements still produce custom blocks owning their directive line ranges. An inline placement that overlaps a block directive is rejected by pass 6 before this stage. This permits, for example, inline generated content in Alpha and Omega around a block [TOC] marker without either feature deleting the other.

Required regression tests

Add tests using real parsed blocks and absolute UTF-16 ranges:

  1. one inline replacement in the middle of a paragraph changes only that span and leaves one block with the same line range;
  2. two non-overlapping inline replacements are applied in source order without offset drift;
  3. an inline replacement containing Unicode before/inside/after the range is exact;
  4. CRLF source on surrounding lines remains unchanged;
  5. inline Markdown containing \n is rejected with authored source preserved;
  6. inline Markdown containing \r is rejected likewise;
  7. an inline range crossing a physical-line boundary is rejected;
  8. an inline range crossing two top-level blocks is rejected;
  9. inline placements in authored prefix/suffix fragments around a block placement both survive;
  10. inline/block overlap rejects the later conflicting placement and preserves that placement's authored source;
  11. Export adapter still maps .inline to .inline and .block to .block.

Acceptance criteria

  • There is no code path where .inline produces a standalone custom Preview block.
  • The containing block's lineRange is unchanged for inline-only composition.
  • All splicing uses the contribution contract's UTF-16 coordinate space.
  • Multiple replacements cannot drift due to earlier generated text length.
  • Invalid inline output leaves the original Markdown visible and reports why.
  • Mixed inline and block placements are deterministic and do not erase each other.

Immediate hostile compatibility review

Existing TOC behavior: PASS. TOC emits .block; this path does not alter it.

Scroll sync: PASS. Inline replacement retains the original block and line range. Block replacements retain pass 2's disjoint line ownership.

Textual rendering: PASS with the single-line contract. Preview continues to render ordinary Markdown source through the existing PreviewBlock/Textual path; no parallel attributed-string renderer is introduced.

Unicode/CRLF: PASS. Coordinates and extraction remain UTF-16/SourceMap based; generated inline Markdown is inserted as a Swift string only after source slices are decoded.

View identity: PASS. Untouched blocks retain IDs; rewritten blocks get one new ID per parse snapshot, not per body evaluation.

Module graph/API compatibility: PASS. No dependency changes and no enum/signature changes. Only the already-implied .inline precondition is documented explicitly.

Future contributions: PASS. Inline math or other first-party contributors receive a real exact-span path. Contributors needing multiline/block output must declare .block instead of relying on accidental promotion.

Conclusion: approved as final architecture. Implement exact in-block UTF-16 splicing; do not choose the superficially simpler option of rejecting all inline contributions, because the public extension seam already advertises and Export already supports them.

@Joncallim Joncallim left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architecture pass 6/10 — P2: fail-closed Preview range validation

Original finding: issue comment 5534703764.

Required invariant

No extension-provided range may influence Preview block selection until it has been proven to belong to the exact source snapshot and to be a non-empty, in-bounds UTF-16 range. SourceMap.line(atUTF16Offset:) is a convenience lookup that deliberately clamps; it is not a validator.

A rejected placement leaves its authored source untouched and emits one bounded diagnostic.

Files to change

  • MacDown2/MacDown2/PreviewContributionAdapter.swift
  • MacDown2/MacDown2Tests/PreviewContributionAdapterTests.swift
  • planning/epic-14-implementation.md

Keep this validation app-private. Do not move ExportService's internal composer into Contributions, add a package dependency, or create a second public validation framework. Export's existing DerivedContentComposer remains independent defense-in-depth at its own trust boundary.

One canonical Preview validation pipeline

compose(...) must execute these stages exactly once, in this order:

  1. collect every renderer diagnostic from every ContributionResult, including results whose content is nil;
  2. verify the source snapshot itself;
  3. turn content-bearing results into candidates;
  4. validate representation and raw range bounds;
  5. resolve one containing base block;
  6. apply placement-specific shape rules from passes 2 and 5;
  7. sort valid candidates deterministically;
  8. reject overlaps;
  9. apply the pass-7 placement budget;
  10. compose accepted placements.

Do not distribute these checks between placements, merged, splitBlock, and the SwiftUI view. There must be one validation entry point before composition.

Snapshot precondition

Before examining any range:

guard sourceMap.utf16Length == sourceText.utf16.count else {
    return Composition(
        blocks: base,
        diagnostics: [.systemError("Preview contribution source map does not match source text; authored source preserved")]
    )
}

Also require each result's sourceGeneration == currentGeneration. Stale results retain their diagnostics but produce no candidate.

Candidate construction

Enumerate results so registry order is retained:

for (ordinal, result) in contributions.enumerated() { ... }

A content-bearing result becomes a candidate only when:

  • its generation matches;
  • its representation is .markdown (Preview HTML remains unsupported and is diagnosed, not silently dropped);
  • its raw range passes every basic bound below.

Basic range validation must be performed using integers only:

let range = content.sourceRange
let sourceLength = sourceMap.utf16Length

guard range.lowerBound >= 0,
      range.lowerBound < range.upperBound,
      range.upperBound <= sourceLength
else { reject }

Only after this guard may code call:

let firstLine = sourceMap.line(atUTF16Offset: range.lowerBound)
let lastLine = sourceMap.line(atUTF16Offset: range.upperBound - 1)

Resolve exactly one containing base block

Find the base block whose lineRange contains firstLine. Use a small binary-search helper over the already source-ordered base array; do not perform an O(blocks × contributions) full scan.

Accept only if the same block also contains lastLine. A range crossing base-block boundaries, or lying on a source line owned by no parsed top-level block, is rejected.

Store the resolved blockIndex on the validated placement. Composition must not recalculate it.

Placement-specific shape checks

For .block, enforce pass 2:

let claimedLines = firstLine ... lastLine
let exact = sourceMap.utf16Range(ofLines: claimedLines)
let exactRange = exact.location ..< (exact.location + exact.length)
guard range == exactRange else { reject }

A partial replacement of a base block is accepted only when base[blockIndex].kind == .paragraph; an entire-block replacement may replace any block kind.

For .inline, enforce pass 5:

  • firstLine == lastLine;
  • Markdown contains no CR or LF;
  • range is wholly within the resolved base block.

Deterministic ordering and overlap policy

Sort structurally valid candidates by:

  1. sourceRange.lowerBound ascending;
  2. sourceRange.upperBound ascending;
  3. original ordinal ascending.

Then walk once with previousAcceptedUpperBound. Accept when range.lowerBound >= previousAcceptedUpperBound; otherwise reject the candidate as overlapping and preserve its source.

This makes exact duplicates deterministic: the first candidate under the comparator wins, every later duplicate is diagnosed. Do not merge, union, clip, or nest overlapping ranges.

An invalid/rejected candidate must not advance previousAcceptedUpperBound, consume the budget, or prevent a later independent range from being accepted.

Diagnostic messages

Use stable, testable reason categories rather than embedding arbitrary debug dumps:

  • stale source snapshot;
  • negative/out-of-bounds range;
  • empty range;
  • no containing Preview block;
  • crosses Preview blocks;
  • block range is not whole-line;
  • partial block replacement in unsupported container;
  • inline range crosses lines;
  • inline Markdown contains a line break;
  • overlaps another accepted contribution;
  • unsupported representation.

Include contributionID and the range where one exists. Do not expose source content in the diagnostic.

Required regression tests

  1. -1..<0, -1..<1, 0..<0, EOF zero-width, and past-EOF ranges all preserve base blocks.
  2. None of those malformed ranges map to line 1 or the last line through clamping.
  3. A valid range at source start and a valid range ending exactly at EOF are accepted.
  4. Unicode before the range does not change UTF-16 correctness.
  5. A range crossing two top-level blocks is rejected.
  6. A whole-line block range is accepted; a partial-line block range is rejected.
  7. A partial block range in a list/table/code/HTML container is rejected; an exact whole-block replacement remains accepted.
  8. Exact duplicates accept one and diagnose the rest.
  9. Partial overlap and containment overlap are rejected deterministically.
  10. A rejected early candidate does not starve a later valid candidate.
  11. Mismatched SourceMap/text returns the original base wholesale with one system diagnostic.
  12. Existing TOC ranges, including CRLF, pass validation unchanged.

Acceptance criteria

  • No call to line(atUTF16Offset:) occurs before integer bounds validation.
  • No malformed range can select or replace an unrelated first/last block.
  • Validation and composition use one immutable source snapshot.
  • Overlaps are fail-closed and deterministic.
  • Every rejected placement preserves authored source and is observable.
  • Export's existing validation is retained unchanged.

Immediate hostile compatibility review

Module graph: PASS. All new mechanics stay in the app adapter; no package dependency changes.

Export behavior: PASS. DerivedContentComposer keeps its own stale/bounds/overlap/budget checks. Duplicate validation at separate renderer trust boundaries is intentional defense-in-depth, not competing business logic.

SourceMap semantics: PASS. Clamping remains useful to existing editor/scroll callers; the fix avoids misusing it rather than changing its contract globally.

Performance: PASS. Source length is measured once, candidates are O(n), sort is O(n log n), block lookup is O(log b) per candidate, and composition remains linear in blocks plus accepted placements.

CRLF/Unicode: PASS. Whole-line normalization is derived from SourceMap, and all coordinates remain UTF-16.

Existing Preview: PASS. With zero accepted placements, the original block array is returned unchanged, including IDs.

Conclusion: approved as final architecture. Do not “fix” this by changing SourceMap to stop clamping; that would risk unrelated scroll/editor behavior and leave the extension boundary under-validated.

@Joncallim Joncallim left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architecture pass 7/10 — P2: apply an explicit Preview contribution budget after validation

Original finding: issue comment 5534706013.

Required invariant

A Preview safety budget may reject otherwise valid generated content, but it must:

  • count only placements that passed snapshot, representation, range, shape, and overlap validation;
  • make the rejection visible;
  • preserve source for every rejected placement;
  • behave deterministically in source order;
  • not diverge from Export for ordinary documents such as 65 valid markers.

Files to change

  • MacDown2/MacDown2/PreviewContributionAdapter.swift
  • MacDown2/MacDown2Tests/PreviewContributionAdapterTests.swift
  • planning/epic-14-implementation.md

Remove the current cap at the wrong layer

Delete:

private static let maxMergedResults = 64

and delete every use of:

contributions.prefix(maxMergedResults)

Raw registry-result order is not a resource-policy boundary. Diagnostic-only, stale, malformed, unsupported, and overlapping results must never consume render capacity.

App-private injectable budget

Add inside PreviewContributionAdapter:

struct Budget: Sendable, Equatable {
    let maxAcceptedPlacementCount: Int
    let maxAggregateGeneratedMarkdownUTF8Bytes: Int

    static let standard = Budget(
        maxAcceptedPlacementCount: 4096,
        maxAggregateGeneratedMarkdownUTF8Bytes: 32 << 20
    )
}

The 4096 count deliberately matches Export's existing ExportResourceBudget.standard.maxDerivedFragmentCount; 32 MiB provides an aggregate generated-payload ceiling analogous to Export's aggregate-derived limit. Keep this app-private rather than making ExportService public internals leak into Preview or adding a package dependency solely to share two constants.

Give compose an internal defaulted parameter:

budget: Budget = .standard

Production callers omit it. Tests inject tiny budgets.

Exact admission algorithm

Pass 6 produces structurally valid, sorted, non-overlapping candidates. Walk that array once:

var accepted: [ValidatedPlacement] = []
var aggregateBytes = 0
var rejectedCount = 0
var countLimitHit = false
var byteLimitHit = false

for candidate in candidates {
    let bytes = candidate.markdown.utf8.count
    let (newTotal, overflowed) = aggregateBytes.addingReportingOverflow(bytes)

    guard accepted.count < budget.maxAcceptedPlacementCount else {
        rejectedCount += 1
        countLimitHit = true
        continue
    }
    guard !overflowed, newTotal <= budget.maxAggregateGeneratedMarkdownUTF8Bytes else {
        rejectedCount += 1
        byteLimitHit = true
        continue
    }

    accepted.append(candidate)
    aggregateBytes = newTotal
}

Do not stop iterating at the first rejection. A single oversize candidate must not prevent a later small independent candidate from rendering when count capacity remains.

Emit one aggregate adapter diagnostic after the loop when rejectedCount > 0, describing:

  • how many placements were left as authored source;
  • which limit(s) were encountered;
  • the configured count/byte limits.

Do not emit one budget diagnostic per omitted placement: a pathological contributor must not turn the diagnostic array itself into an unbounded allocation. Renderer-supplied diagnostics from the original results are still retained under pass 9's bounded presentation rules.

Deterministic preservation behavior

Accepted placements are the earliest admissible placements in pass-6 source order, except that an individually byte-oversize candidate is skipped and later candidates may still fit. Rejected candidates simply do not participate in composition; their original source remains in the base blocks.

The budget must not mutate ranges, shorten Markdown, or collapse several generated blocks into one.

Required regression tests

  1. 65 valid one-line block placements all render under .standard.
  2. 64 diagnostic-only/stale/unsupported results before one valid result do not starve it.
  3. With an injected count limit of 2, the first two valid placements render, later placements remain authored, and one aggregate diagnostic reports the count rejection.
  4. Invalid and overlapping candidates do not consume the count budget.
  5. With a small byte limit, one oversize candidate is preserved while a later small candidate still renders.
  6. Aggregate-byte addition overflow fails closed.
  7. Count and byte limits hit together still produce one bounded summary diagnostic.
  8. A result set under both limits produces no budget diagnostic.
  9. Untouched blocks remain byte-for-byte and ID-for-ID equal.
  10. A 4096-placement standard-boundary test may use synthetic validated fixtures if constructing full parsed Markdown would make the test unnecessarily slow; separately retain at least one end-to-end parsed-source budget test.

Acceptance criteria

  • No prefix is applied to raw results.
  • Budget admission occurs after all semantic eligibility checks.
  • 65 valid TOC markers no longer diverge from Export solely because of Preview's former 64 cap.
  • Rejected generated content remains visibly authored.
  • Budget rejection is observable but diagnostic output is bounded.
  • Tests can exercise boundaries without allocating thousands of large Markdown fragments.

Immediate hostile compatibility review

Export parity: PASS. Ordinary and moderately large documents share the same 4096 placement count ceiling. Export keeps its own renderer-specific HTML byte budget and validation.

Module graph: PASS. No new dependency from Preview/app code to ExportService internals and no public API expansion.

Memory/CPU: PASS. Validation is already O(n log n); admission adds one O(n) pass. Aggregate byte counting is overflow-safe. Per-block PreviewBlock.oversizeByteThreshold remains active as a second, different guard against Textual crashes.

Failure semantics: PASS. Every rejected placement preserves authored Markdown, matching Export's established fallback.

Diagnostic denial-of-service: PASS. Budget rejection is summarized once rather than multiplied by pathological result count.

Future tuning: PASS. The budget is a named value with injectable tests, not scattered literals. Changing it later does not alter contribution semantics.

Conclusion: approved as final architecture. Raise and reposition the safety boundary; do not simply change 64 to 4096 while leaving prefix ahead of eligibility filtering.

@Joncallim Joncallim left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architecture pass 8/10 — P2: preserve structured cancellation through Preview publication

Original finding: issue comment 5534708039.

Required invariant

Cancellation is control flow, never an empty successful contribution result. A task canceled because a newer parse snapshot superseded it must perform no subsequent state publication, even when a contributor itself does not observe cancellation while suspended.

Ordinary contributor failure remains isolated by ContributionRegistry; cancellation aborts the whole superseded run.

Files to change

  • MacDown2/Packages/MacDownKit/Sources/Contributions/ContributionRegistry.swift
  • MacDown2/MacDown2/PreviewContributionAdapter.swift
  • MacDown2/MacDown2/DocumentEditorSplitView.swift
  • MacDown2/Packages/MacDownKit/Tests/ContributionsTests/ContributionRegistryTests.swift
  • MacDown2/MacDown2Tests/PreviewContributionAdapterTests.swift
  • planning/epic-14-implementation.md

Registry cancellation boundary

Keep sequential contributor execution and ordinary-error isolation. Add a second cancellation check immediately after each contributor returns and before its output is appended:

for contribution in contributions {
    try Task.checkCancellation()
    do {
        let produced = try await contribution.run(
            document: document,
            sourceText: sourceText,
            sourceGeneration: sourceGeneration
        )
        try Task.checkCancellation()
        results.append(contentsOf: produced)
    } catch is CancellationError {
        throw CancellationError()
    } catch {
        results.append(failureResult(for: contribution, error: error, sourceGeneration: sourceGeneration))
    }
}

The post-call check is mandatory. It closes the case where a contributor ignores cancellation, finishes normally, and returns stale output after the parent task was canceled.

Do not convert cancellation into a renderer diagnostic. The canceled snapshot is obsolete; showing its failure would itself be stale UI.

Adapter API

Change:

static func results(...) async -> [ContributionResult]

to:

static func results(
    document: MarkdownDocument?,
    text: String?,
    generation: UInt,
    registry: ContributionRegistry = .standard
) async throws -> [ContributionResult]

Implementation:

guard let document, let text else { return [] }
return try await registry.run(
    document: document,
    sourceText: text,
    sourceGeneration: generation
)

Delete the catch-all catch { return [] }. The defaulted registry parameter is an app-internal test seam; production behavior remains .standard.

View task sequence

Use pass 1's {identity, revision} task key and capture one immutable snapshot before the first suspension:

let capturedDocument = parsed
let capturedText = sourceText
let capturedRevision = parsed.revision
let capturedGeneration = generation

The task must follow this order:

  1. await PreviewContributionAdapter.results;
  2. try Task.checkCancellation();
  3. compose results using the captured document/text/generation;
  4. try Task.checkCancellation();
  5. verify the live session still has the captured revision and published text;
  6. publish contributedPreviewBlocks and contributionDiagnostics together.

Use one helper such as refreshPreviewContributions() so the sequence cannot drift between modifiers/call sites.

Error handling is exact:

do {
    ...
} catch is CancellationError {
    return                         // no state mutation
} catch {
    guard stillCurrent else { return }
    contributedPreviewBlocks = nil // ordinary base Preview remains visible
    contributionDiagnostics = [system diagnostic for this snapshot]
}

Do not clear or overwrite state in the cancellation branch. Parse-change handling already clears the old composed snapshot synchronously as specified in pass 1.

Publication guard

The final guard must test source identity, not FileDocument.mutationGeneration:

guard parseSession.document?.revision == capturedRevision,
      parseSession.publishedText == capturedText
else { return }

This guard is defense-in-depth after structured cancellation. It protects against a task whose cancellation arrives too late, session replacement under the same view identity, or a future executor that does not inherit SwiftUI cancellation as expected.

Contributor author contract

Add protocol documentation stating:

  • contributors performing loops or multiple awaits should call Task.checkCancellation() at bounded intervals;
  • the registry checks before and after each contributor, but cannot pre-empt synchronous CPU work inside a contributor;
  • cancellation must be rethrown, never wrapped as an ordinary contribution error.

No new protocol method or cancellation token is required; Swift structured concurrency is the existing mechanism.

Required regression tests

In ContributionRegistryTests:

  1. an already canceled task throws before the first contributor runs;
  2. a contributor throwing CancellationError propagates it and later contributors do not run;
  3. a controllably suspended contributor is allowed to return normally after its parent task is canceled; the registry's post-call check still throws and discards its output;
  4. ordinary thrown errors remain isolated as diagnostic-only results and later contributors still run.

In PreviewContributionAdapterTests:

  1. results propagates cancellation instead of returning [];
  2. results still returns [] for a genuinely absent document/text without error;
  3. deterministic supersession race: start snapshot A with a gated contributor, start/publish snapshot B, cancel A, release A, and assert A reaches no publication callback/state recorder;
  4. final snapshot mismatch guard rejects an otherwise successful old composition;
  5. cancellation does not replace a current diagnostic/composition with an empty state.

Use an actor/continuation-based gate, not sleeps or timing assumptions. Tests must signal “contributor started” and explicitly release it.

Acceptance criteria

  • No catch { return [] } remains on the contribution execution path.
  • Registry checks cancellation both before and after contributor execution.
  • A canceled task cannot assign either contribution blocks or diagnostics.
  • The only ordinary fallback is the unmodified base Preview plus a current-snapshot diagnostic.
  • Supersession tests are deterministic and contain no wall-clock sleeps.

Immediate hostile compatibility review

Ordinary fault isolation: PASS. Non-cancellation errors are still converted to diagnostic-only results exactly as today.

SwiftUI lifecycle: PASS. View disappearance or task-ID changes cancel naturally; canceled work becomes silent non-publication rather than UI churn.

Non-cooperative contributors: PASS within Swift's cooperative model. The registry cannot interrupt synchronous work, but the post-call check prevents returned stale output from escaping.

Export: PASS. Export already propagates cancellation through its async pipeline. The registry hardening benefits Export as well without changing successful semantics.

State consistency: PASS. Blocks and diagnostics are assigned from one captured composition after one final current-snapshot guard.

Performance: PASS. One additional cancellation check per contributor and one final text/revision guard; no new task, actor, or polling loop.

API compatibility: PASS. The adapter is app-internal. ContributionRegistry and Contributing signatures remain unchanged.

Conclusion: approved as final architecture. Do not introduce detached tasks, generation races, or bespoke cancellation tokens; preserve Swift structured cancellation end-to-end and gate the final state write.

@Joncallim Joncallim left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architecture pass 9/10 — P2: preserve and surface diagnostic-only contribution failures

Original finding: issue comment 5534709836.

Required invariant

Every diagnostic produced by the registry or either adapter survives to an observable boundary, regardless of whether the result has placeable content. Authored source remains the fallback, but “source preserved” is not permission to make the failure silent.

Diagnostics must be snapshot-scoped in Preview and export-scoped in Export. They must not trigger modal alerts while the user types.

Files to change

Preview:

  • MacDown2/MacDown2/PreviewContributionAdapter.swift
  • MacDown2/MacDown2/DocumentEditorSplitView.swift
  • add MacDown2/MacDown2/PreviewContributionIssueIndicator.swift
  • MacDown2/MacDown2Tests/PreviewContributionAdapterTests.swift
  • add/update focused view tests where the existing SwiftUI test harness permits

Export:

  • MacDown2/MacDown2/ExportContributionAdapter.swift
  • MacDown2/MacDown2/ExportCoordinator.swift
  • MacDown2/Packages/MacDownKit/Sources/ExportService/ExportRequest.swift
  • MacDown2/Packages/MacDownKit/Sources/ExportService/ExportComposer.swift
  • MacDown2/MacDown2Tests/ExportContributionAdapterTests.swift
  • MacDown2/Packages/MacDownKit/Tests/ExportServiceTests/ExportComposerTests.swift

Documentation:

  • planning/epic-14-implementation.md

Preview diagnostic model

Define app-private types inside PreviewContributionAdapter:

struct Diagnostic: Sendable, Equatable, Identifiable {
    let id: String
    let contributionID: String?
    let severity: ContributionDiagnostic.Severity
    let message: String
    let sourceRange: Range<Int>?
}

struct Composition: Sendable, Equatable {
    let blocks: [PreviewBlock]?
    let diagnostics: [Diagnostic]
}

Construct id deterministically from contribution ID, severity, message, source range, and occurrence ordinal. Do not use a fresh UUID in SwiftUI body evaluation.

For every ContributionResult, append converted result.diagnostics before inspecting result.content. Use the result's contributionID; use the content range when content exists, otherwise nil.

Adapter-generated diagnostics from passes 5–7 use the same type. A result with content == nil and no diagnostics is a legitimate no-op and adds nothing.

compose always returns both blocks and diagnostics. It must never offer a blocks-only convenience that callers can use to accidentally discard the second channel.

Preview state and lifecycle

Use pass 1's state:

@State private var contributedPreviewBlocks: [PreviewBlock]?
@State private var contributionDiagnostics: [PreviewContributionAdapter.Diagnostic] = []

On parse snapshot change, clear both before starting replacement work. On successful current-snapshot composition, assign both from the same local Composition. On cancellation, assign neither. On an unexpected adapter/system error, leave ordinary Preview blocks active and publish one current-snapshot system diagnostic.

This prevents diagnostics from one document revision remaining visible beside another.

Passive Preview presentation

Add PreviewContributionIssueIndicator, following the small top-trailing visual language of PreviewBusyIndicator.

Required behavior:

  • render nothing for an empty array;
  • render a borderless warning-triangle button plus total count otherwise;
  • .help text: Generated preview content has N issues;
  • button opens a popover, never a modal alert;
  • popover lists severity, contribution ID when present, and message;
  • display at most the first 20 rows, followed by …and N more;
  • expose previewContributionIssueIndicator and per-row accessibility identifiers;
  • no raw source text is displayed;
  • errors sort before warnings, then retain source/production order within severity.

In the Markdown preview overlay, replace the single busy indicator with:

HStack(spacing: 4) {
    PreviewContributionIssueIndicator(diagnostics: contributionDiagnostics)
    PreviewBusyIndicator(isVisible: parseSession.isParsing)
}
.padding(8)

Remove the inner padding from the busy indicator or avoid double padding so its visual position does not shift materially. Other preview kinds remain unchanged.

Do not emit an alert on each keystroke, automatically open the popover, or write every diagnostic repeatedly to unified logging.

Export adapter batch

Replace the array-only adapter result with:

struct Batch: Sendable, Equatable {
    let contributions: [ExportDerivedContribution]
    let diagnostics: [ExportDiagnostic]
}

static func adapt(_ results: [ContributionResult]) -> Batch

For each result:

  • content == nil: map result.diagnostics into Batch.diagnostics;
  • content-bearing Markdown: place mapped diagnostics on that ExportDerivedContribution as today;
  • content-bearing unsupported HTML: place the result diagnostics plus the adapter's unsupported-HTML error on that derived contribution, so DerivedContentComposer preserves source and forwards them;
  • content-bearing supported output: do not also copy its diagnostics into the batch, or alerts will duplicate them.

This keeps anchorless diagnostics separate while retaining the existing per-placement diagnostic path.

Export request channel

Extend ExportRequest with a defaulted public field:

public let contributionDiagnostics: [ExportDiagnostic]

and initializer parameter:

contributionDiagnostics: [ExportDiagnostic] = []

The default preserves every existing caller and test fixture.

ExportCoordinator.exportContributions(for:) returns ExportContributionAdapter.Batch. performExport supplies both:

let batch = try await exportContributions(for: document)
let request = ExportRequest(
    ...,
    contributions: batch.contributions,
    contributionDiagnostics: batch.diagnostics
)

Export composer integration

In ExportComposer.prepare, merge upstream anchorless diagnostics exactly once:

let diagnostics = try resolve(
    derived: parsed.metadata.diagnostics
        + request.contributionDiagnostics
        + derived.diagnostics,
    resources: resolver.diagnostics,
    rendered: rendered,
    policy: policy
)

Do not synthesize fake source ranges, empty HTML contributions, or zero-width sentinels merely to transport diagnostics. PreparedExportDocument.diagnostics already reaches ExportCoordinator.presentDiagnosticsIfNeeded, which lists at most six messages and summarizes the remainder.

Contribution diagnostics remain non-fatal by themselves, matching the existing ExportComposer policy: failed generated content preserves authored Markdown and completes export with an issue report.

Required regression tests

Preview adapter:

  1. throwing contributor -> content == nil diagnostic appears in Composition.diagnostics while base blocks remain;
  2. diagnostic-only warning survives;
  3. content-bearing warning survives once;
  4. adapter rejection diagnostic includes contribution ID/range and preserves source;
  5. diagnostics clear between snapshot revisions;
  6. cancellation publishes no stale diagnostic;
  7. deterministic IDs remain stable for the same composition;
  8. issue indicator is absent for zero diagnostics, exposes count for nonzero diagnostics, and summarizes >20 rows.

Export adapter/composer:

  1. diagnostic-only result yields zero contributions and one batch diagnostic;
  2. content-bearing diagnostic stays attached to the derived contribution and is not duplicated in batch;
  3. unsupported HTML carries original + adapter error exactly once;
  4. ExportRequest default diagnostics leave existing prepared output unchanged;
  5. supplied anchorless contribution diagnostics appear in PreparedExportDocument.diagnostics;
  6. ordinary standalone/self-contained/PDF diagnostic policies remain unchanged;
  7. coordinator's existing successful-export issue alert receives the diagnostic-only failure.

Acceptance criteria

  • No compactMap discards a diagnostic-only result at either integration boundary.
  • Preview displays a passive, accessible indication for current-snapshot issues.
  • Export reports anchorless failures through its existing post-success issue alert.
  • No diagnostic is duplicated merely because it crossed adapter and composer layers.
  • No fake placement is created to carry an unanchored diagnostic.
  • Existing callers compile because all new public initializer input is defaulted.

Immediate hostile compatibility review

Typing experience: PASS. Preview feedback is passive and snapshot-scoped; no alert storm or focus theft.

Export behavior: PASS. Existing prepared-document diagnostics and alert plumbing are reused. Contribution errors remain recoverable, not fatal.

Public API: PASS. ExportRequest gains one defaulted value-semantic field; source compatibility is preserved, and Equatable/Sendable remain synthesized.

Module graph: PASS. ExportService still knows only ExportDiagnostic, not the Contributions module. Conversion remains in the app adapter.

Failure isolation: PASS. A broken contributor cannot blank Preview or abort an otherwise valid export solely through its diagnostic.

Privacy/security: PASS. UI shows IDs/messages/ranges, never authored source or filesystem content.

Diagnostic duplication: PASS if the batch/content-bearing split above is followed exactly.

Conclusion: approved as final architecture. Preserve diagnostics as first-class side-channel data; do not encode them as malformed content and do not weaken the documented ContributionResult contract.

@Joncallim Joncallim left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architecture pass 10/10 — P3: make the PR body and implementation document an accurate hand-off contract

Original finding: issue comment 5534712299.

Required invariant

At every hand-off point, the PR description must describe the code at its current head—not the first commit, intended future scope, or an earlier CI state. A reviewer must be able to determine, without reconstructing commit history:

  • what is implemented now;
  • what remains intentionally unimplemented;
  • what behavior to test;
  • which automated gates passed or failed on this head;
  • which limitations are accepted rather than accidental defects.

Files/metadata to change

  • PR #54 description
  • planning/epic-14-implementation.md
  • planning/epics/README.md
  • issue #15 status/checkboxes only when supported by completed evidence

Do not mark the PR ready, close issue #15, or claim the entire epic complete merely because the TOC remediation passes. The current changed-file set contains the contribution/TOC/Preview/Export foundation, but no text-filter runner or its menu/palette UI.

Two-stage reconciliation

Stage A — immediately after implementing passes 1–9

Update the draft PR body to state accurately:

  • first-party contribution protocol/registry is implemented;
  • TOCContribution is registered and user-visible in Markdown Preview and Export;
  • Preview and Export adapters are implemented;
  • source-snapshot, precise range composition, semantic eligibility, placement, range, budget, cancellation, and diagnostic behavior are covered by tests;
  • text filters remain pending if they are still absent;
  • CI status is recorded for the exact remediation head.

Keep the PR in draft while intended slices remain.

Stage B — before ready-for-review/merge

Either:

  1. land and verify the remaining text-filter scope promised by the PR title/body; or
  2. deliberately split/defer it into a separately linked issue/PR and retitle/reframe PR #54 around the contribution/TOC scope.

Do not leave a mixed state where the title promises text filters while the body silently treats them as shipped.

Required PR-body structure

Use these sections, in this order:

## What this changes
## Scope status
## Why
## Architecture
## What I should test
## Automated verification
## Risks and accepted limitations
## Files / slices
## Traceability

What this changes

State only current behavior. Name Contributions, TOCContribution, PreviewContributionAdapter, ExportContributionAdapter, and the Preview/Export integration points.

Scope status

Use an explicit table or checklist:

  • contribution protocol/registry — landed;
  • TOC contributor — landed;
  • Preview integration — landed;
  • Export integration — landed;
  • remediation passes 1–9 — landed only after code/tests exist;
  • text-filter runner — pending/landed;
  • text-filter UI — pending/landed;
  • documentation closeout — current status.

Architecture

Summarize the final invariants, not obsolete implementation details:

  • one parsed source snapshot token per Preview contribution run;
  • fail-closed UTF-16 range validation;
  • exact inline/block placement semantics;
  • line-owned block splitting preserving adjacent authored source;
  • semantic [TOC] eligibility from parsed paragraph context;
  • bounded contribution composition;
  • structured cancellation and current-snapshot publication guard;
  • diagnostics survive independently of placeable content;
  • Export retains independent defense-in-depth validation.

Link the implementation document, but keep this summary self-sufficient.

What I should test

Include concrete manual cases:

  1. headings plus one standalone [TOC] render in Preview and HTML/PDF export;
  2. Alpha\n[TOC]\nOmega preserves both authored lines and shows the TOC between them;
  3. two [TOC] lines in one CommonMark paragraph both render and preserve middle text;
  4. fenced and four-space-indented [TOC] remain literal;
  5. two-space-indented paragraph [TOC] remains active;
  6. save/mark-clean without editing does not make the TOC disappear;
  7. CRLF and Unicode documents preserve exact surrounding text;
  8. 65 valid markers do not stop at 64;
  9. no-heading document shows the documented placeholder;
  10. a controlled failing test contribution produces a passive Preview issue indicator and a successful-export issue report while authored source remains.

Only include test-only contributor steps when a debug/test build exposes them safely; otherwise identify those as automated coverage, not end-user dogfood.

Automated verification

Record exact commands and outcomes for the final head. At minimum:

cd MacDown2/Packages/MacDownKit && swift test
cd MacDown2 && xcodegen generate
xcodebuild build -project build/MacDown2.xcodeproj -scheme MacDown2 -destination 'platform=macOS'
xcodebuild build-for-testing -project build/MacDown2.xcodeproj -scheme MacDown2UITests -destination 'platform=macOS'
# repository SwiftFormat/SwiftLint commands exactly as CI runs them

Then record the GitHub Actions run/check names for that same SHA. Check a box only when evidence exists. If manual dogfood was not performed, leave it unchecked and state so.

Risks and accepted limitations

Remove defects that passes 1–9 have fixed. Retain only true residuals, such as:

  • clickable TOC anchors are not implemented if still true;
  • Preview HTML-fragment rendering is unsupported if still true;
  • Export performs an extra contribution parse if still true;
  • text filters are out of this PR only if explicitly split/deferred.

Do not relabel unresolved correctness defects as accepted limitations.

Files / slices

List logical slices rather than dumping every filename:

  1. contribution contracts and registry;
  2. TOC contributor and semantic marker detection;
  3. Preview composition and diagnostics UI;
  4. Export adaptation and anchorless diagnostic channel;
  5. tests;
  6. project/Package/XcodeGen wiring;
  7. text-filter slices, accurately pending or landed;
  8. documentation closeout.

Traceability

Reference issue #15, the implementation document, the remediation review IDs, and the exact final head SHA. Remove duplicate generated footers and stale session boilerplate that does not help a maintainer.

Implementation-document reconciliation

After tests pass, update planning/epic-14-implementation.md so its normative sections match the final code:

  • source generation means immutable renderer snapshot identity, not universally FileDocument.mutationGeneration;
  • Preview block replacement is precise and line-owned, not whole-containing-block;
  • [TOC] detection uses parsed paragraph eligibility;
  • inline and block placement preconditions are explicit;
  • invalid/overlapping ranges fail closed;
  • Preview budget is applied after eligibility;
  • cancellation is propagated through adapters and publication;
  • diagnostic-only results have Preview and Export channels;
  • only actual residual risks remain in §18.

planning/epics/README.md must describe the epic as in progress until its declared scope is genuinely closed.

Required verification of the documentation itself

Before hand-off, compare the final PR changed-file list and head diff against every “landed” claim. Search the PR body and implementation document for these stale phrases and remove/update them where no longer true:

  • “only the architecture document and first implementation slice”;
  • “ContributionRegistry.standard is intentionally empty”;
  • “neither is user-visible yet”;
  • “Preview/Export wiring will arrive in later pushes”;
  • “not required yet — no user-visible behaviour”.

Acceptance criteria

  • A reviewer can execute the manual test plan directly.
  • Scope status distinguishes landed contribution work from pending text filters.
  • Verification evidence names the exact final SHA/run.
  • No checked box lacks evidence.
  • No fixed bug remains documented as an accepted limitation.
  • PR body, implementation plan, epic index, issue status, and repository head agree.

Immediate hostile compatibility review

Review integrity: PASS. The structure exposes incomplete scope and red/untested gates instead of hiding them in prose.

Future hand-off: PASS. A coding/review agent receives exact behavior, commands, residuals, and traceability without replaying this discussion.

Scope control: PASS. This does not force text-filter implementation into the remediation patch; it forces an explicit land-versus-defer decision before merge.

Evidence quality: PASS. Claims are tied to a head SHA and actual commands/checks, preventing green evidence from an older commit being reused accidentally.

Maintenance: PASS. The implementation document remains normative architecture; the PR body remains concise operational state.

Conclusion: approved as final architecture. Documentation reconciliation is a merge deliverable, not optional cleanup, but it must occur after the code/test state is known so it cannot immediately become stale again.

Joncallim and others added 7 commits September 5, 2026 10:41
Generalizes ContributionResult.sourceGeneration's documentation to an
opaque caller-selected snapshot token (no longer named as
FileDocument.mutationGeneration specifically), adds a post-await and a
final cancellation check to ContributionRegistry.run alongside the
existing pre-await one, and makes TOCContribution.findMarkers require
the marker line to be the entire content of exactly one top-level,
parsed .paragraph block rather than a lexical [TOC] line anywhere —
so a marker inside fenced/indented code, a list item, a block quote,
a heading, front matter, or an HTML block is literal text in both
Preview and Export.

Per the architecture takeover hand-off on PR #54
(issuecomment-5538790625), findings/passes 1 (partial) and 3, plus
half of pass 8.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replaces PreviewContributionAdapter.merged (whole-block substitution)
with compose(...): a pure, fully-parameterized admission ->
overlap/budget resolution -> source-ordered composition pipeline
(PreviewContributionAdmission.swift, PreviewContributionComposer.swift).
.inline splices in place; .block replaces a whole base block or
complete physical line(s) inside a top-level paragraph. Malformed
ranges are rejected before any SourceMap lookup, never clamped.
Overlap resolution runs before the new explicit
PreviewContributionBudget (64 placements / 64 KiB, replacing a silent
prefix(64)). Affected/generated blocks get a deterministic ID from
CryptoKit.SHA256 over their role/span/contribution-id; untouched
blocks pass through with their original IDs.

PreviewContributionSession (mirrors MarkdownParseSession's shape)
owns one atomic PreviewContributionComposition and a
PreviewContributionTaskID (document/tab identity + parsed revision)
publish guard, wired into DocumentEditorSplitView in place of the
former separate previewBlocks/contributionResults state and the
task keyed on parseSession.document. A cancelled/superseded refresh
never publishes; a non-text FileDocument mutation can no longer
invalidate a valid composed TOC. PreviewContributionDiagnosticsBadge
surfaces producer- and adapter-raised diagnostics beside the existing
busy indicator, gated to the currently displayed parsed revision.

Per the architecture takeover hand-off on PR #54
(issuecomment-5538790625): the shared Preview composition seam,
findings/passes 1 (remainder), 2, 5, 6, 7, and the Preview half of 8
and 9.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds import Foundation to ExportContributionAdapterTests.swift,
fixing the exact build-for-testing failure at this PR's pinned head
(cannot find 'URL' in scope) without any production, project,
package, or workflow change.

Replaces ExportContributionAdapter.exportContributions(from:) with
adapt(_:) -> Adaptation { contributions, standaloneDiagnostics }: a
content == nil result has no sourceRange to anchor an
ExportDerivedContribution to, so its diagnostics now become
standaloneDiagnostics instead of being dropped; a content-bearing
result's diagnostics stay attached only to its own contribution.
ExportCoordinator.combinedDiagnostics(_:_:) merges
standaloneDiagnostics before the export service's own diagnostics,
identically for the HTML and PDF paths, so the two cannot drift
through separately copy-pasted merge logic.

Per the architecture takeover hand-off on PR #54
(issuecomment-5538790625), findings/passes 4 and the Export half of 9.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…state

Adds epic-14-implementation.md §19, reconciling the document with what
actually shipped in the PR #54 remediation rather than restating the
architecture takeover comment: the compose(...) pipeline shape,
opaque sourceGeneration semantics, semantic TOC discovery,
PreviewContributionSession's task ownership, the Export Adaptation
side channel, and the explicit Preview budget. The two residual risks
already named in §18 (unhandled .html, non-clickable TOC entries) are
unchanged.

Per the architecture takeover hand-off on PR #54
(issuecomment-5538790625), finding/pass 10.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Simplifies the force-unwrap-avoidance construct SwiftFormat generated
locally into a plain boolean comparison, which both the local and CI
SwiftFormat installs agree is already formatted.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
NSEvent.mouseEvent(with:...) asserts its type is an actual mouse
event; .appKitDefined is a system-defined type and must be
constructed via otherEvent(with:...) instead. macOS 26's AppKit
enforces this assertion strictly (crashing the whole test process),
where a prior macOS silently tolerated the mismatch.

Discovered while running the full app test suite to verify the
EPIC-14 remediation on this branch; unrelated to that work but fixed
here per user request rather than split into a separate PR.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Production fix: RecoveryBuffer.migrateLegacyFenceLedger unconditionally
swept every legacy (non-generation-encoded) recovery epoch into the
durable `retired` set on ledger load, even when that epoch was still
the recorded current owner for its document. Since isRetired's legacy
branch checks only `retired` membership (never `currentByDocument`),
this made a document's still-current crash-recovery snapshot
permanently unrecoverable the next time the ledger loaded (e.g. app
relaunch) — silent data loss for any document using a legacy,
plain-UUID recovery epoch. Now skips retiring a legacy lifetime that
is still its document's current owner.

Test fixes (all pre-existing bugs, not product regressions):
- ExternalFileControllerRecoveryTests: `ExternalFileController.model`
  is `weak`; the move-retry test constructed `WorkspaceModel` inline
  so it was deallocated before any assertion ran, making every
  isCurrentRecoveryAction check silently see `model == nil`.
- ExternalFileControllerCloseRecoveryTests: pendingCleanupFixture()
  relied on TabStore's 300ms-debounced session-save timer firing
  before the assertion ran; now saves synchronously.
- ScriptedRecoveryExecutor: persist/retire now perform real
  recovery-buffer IO (matching sibling fakes already in the suite) so
  tests asserting against the real on-disk buffer can actually pass;
  remove/migrate deliberately stay fully scripted since one shared
  test replays the same document identity/epoch across all four
  actions to exercise the controller's own retry bookkeeping in
  isolation.

Discovered while running the full app test suite to verify the
EPIC-14 remediation on this branch; unrelated to that work but fixed
here per user request rather than split into a separate PR. Verified:
full `MacDown2Tests` target (71/71), full MacDownKit package suite
(1054/1054), swiftformat --lint and swiftlint --strict clean, app
build green.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@Joncallim
Joncallim marked this pull request as ready for review September 5, 2026 23:53
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@Joncallim
Joncallim merged commit 86a2431 into master Sep 6, 2026
2 checks passed
Joncallim added a commit that referenced this pull request Sep 6, 2026
PR #54 shipped Slices 1-4 (contribution SPI, TOCContribution, Preview/
Export integration) with a 10-finding remediation not anticipated when
epic-14-implementation.md was first drafted. Adds an as-built note
pointing §6.6/§7.1/§16's now-stale merged(...)/exportContributions(
from:) references to §19's authoritative final shape, and marks
Slices 1-4 done / Slices 5-8 (text filters) not started in the slice
headers. Slices 5-8 themselves are unchanged — they already specify
command discovery, a typed command model, structured (no-shell)
process execution, explicit working directory/environment, timeout +
cancellation, bounded stdout/stderr, fail-closed error handling that
preserves the original selection, one undoable editor mutation,
Commands-menu + palette integration with built-in/user-installed
visual distinction, and the post-1.0 extension-API design doc — this
is the exact, still-current scope for the next PR ("E14B").

Also updates planning/epics/README.md's E14 row from "open" to
reflect the partial shipment (E13's row was already corrected on the
epic/14 branch and is unaffected here).

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants