EPIC-14: Extension points — first-party contributions + text filters - #54
Conversation
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
[P1] Preview contributions are invalidated by non-text document mutations
Deterministic failure path:
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 |
[P1] A physical
|
[P1]
|
[P1] Restore the existing
|
[P2] Preview ignores
|
[P2] Validate extension-provided source ranges before mapping them into Preview
The export path already has the correct defensive behavior: 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. |
[P2] Preview silently truncates valid contributions at 64 and diverges from Export
Two concrete consequences:
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 |
[P2] Cancellation is converted into a successful empty result, allowing superseded tasks to publish
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() |
[P2] Registry failure diagnostics are silently lost at both UI boundaries
Neither integration boundary currently honors it:
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. |
[P3] PR description/test plan is materially stale relative to the actual headThe current PR body still says this push contains only Slice 1 / the protocol-registry architecture, that 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. |
Deep-review hand-off index — head
|
Joncallim
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.swiftMacDown2/Packages/MacDownKit/Sources/Contributions/ContributionResult.swift— documentation semantics onlyMacDown2/MacDown2Tests/PreviewContributionAdapterTests.swift- add an app-level integration/state test only if the existing test harness has a practical
DocumentEditorSplitViewseam; 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:
- Run the existing
refreshPreviewBlocks()andrefreshOutline(). - Immediately set
contributedPreviewBlocks = nilandcontributionDiagnostics = []. This prevents old derived blocks from being displayed against a newly published parse while the replacement task is running. - 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 ?? previewBlocksThere must be no live FileDocument.mutationGeneration check anywhere in the Preview contribution path after this change.
Required regression tests
sourceGenerationmatching the parse-revision token is accepted.- A stale parse-revision token is rejected and authored Preview blocks are preserved.
- Model-level sequence: parse
[TOC]-> compose derived Preview -> performFileDocument.markClean()or a save-success transition that advancesmutationGenerationwithout changing text -> the same composed Preview remains valid because its task/source revision did not change. - New parse revision for changed text invalidates old derived blocks before publishing the new result.
- Switching
identitywith 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.mutationGenerationsemantics 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
left a comment
There was a problem hiding this comment.
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]
OmegaPreview must become three source-mapped Preview blocks:
- authored paragraph, line 1,
Alpha - custom
tocblock, line 2, generated TOC Markdown - 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.swiftMacDown2/MacDown2Tests/PreviewContributionAdapterTests.swiftplanning/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
) -> CompositionComposition 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
PreviewBlockvalue 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:
Alpha\n[TOC]\nOmegaparses as one paragraph but composes to three Preview blocks and preservesAlpha/Omega.[TOC]\nMiddle\n[TOC]in one paragraph produces two TOCs plus theMiddleauthored fragment.- Prefix only and suffix only variants.
- CRLF input preserves the correct authored text and disjoint line ranges.
- A marker that already occupies its own base block still yields one custom replacement with no empty fragments.
- Untouched surrounding blocks compare equal to the originals, including IDs.
- Resulting line ranges are sorted/non-overlapping and
ScrollSyncMap.previewIndex(forSourceLine:)maps prefix/TOC/suffix to the expected block indices. - 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
PreviewBlockinitializer recomputesisOversizefrom 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
left a comment
There was a problem hiding this comment.
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:
- lexical eligibility — after trimming surrounding whitespace/newline characters, the physical line is exactly
[TOC]; - semantic eligibility —
MarkdownDocument.block(atLine:)reports.paragraphfor that line.
Both conditions are required.
Files to change
MacDown2/Packages/MacDownKit/Sources/Contributions/TOCContribution.swiftMacDown2/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]
Omegablock(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:
- fenced code:
-> zero results.
```text [TOC] ```
- indented code:
[TOC]-> zero results. - HTML block containing an exact
[TOC]line -> zero results. - front matter containing
[TOC]-> zero results if front matter parsing is enabled by the standard options. - normal paragraph marker -> one result.
- two normal markers -> two results.
Alpha\n[TOC]\nOmega-> marker remains detected even though it shares the paragraph block.- two/three spaces of indentation -> detected when parser classifies as paragraph; four spaces -> not detected.
- CRLF marker -> still detected and range includes the same CR behavior as the current implementation.
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.
TOCContributionuses the already parsedMarkdownDocument; 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
left a comment
There was a problem hiding this comment.
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-testgreen; - head
a7aac6e44254817bb6679950cd24499fe92c669d: lint green,build-and-testred; - 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
.github/workflows/ci.yml— exact CI command sequence and environment contract.MacDown2/project.yml— XcodeGen source of truth for targets/dependencies/settings.- Generated
.xcodeproj— an output; never make a durable project fix only here. - 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
- Read
.github/workflows/ci.ymland copy the exact XcodeGen/bootstrap andxcodebuild ... build-for-testing/test command used bybuild-and-test. - Remove any locally generated project/build products that the workflow recreates.
- Regenerate the Xcode project exactly as CI does from
project.yml. - Run the exact first failing CI command, without adding flags that suppress diagnostics.
- 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 usesURL(fileURLWithPath:)and, at this head, does not explicitly importFoundation. - This is a candidate, not a pre-proven root cause. Add
import Foundationonly 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.ymltargetMacDown2Testsdependencies. - The PR already adds
ContributionsandPreview; correct omissions or target placement inproject.ymlonly. - Regenerate the project.
- Do not hand-edit
project.pbxprojas the durable fix.
3. Duplicate product/framework/link symbol error
- Compare direct dependencies of
MacDown2andMacDown2Testswith 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.ymlsyntax/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.ymldependency 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:
- regenerate project if
project.ymlchanged; - rerun the exact formerly failing build-for-testing command;
- stop and inspect the new first error if one remains;
- do not batch speculative fixes.
When the targeted command is green, run the complete gate set in this order:
- all SwiftPM package tests used by CI;
- formatter check;
- strict SwiftLint check;
- app build in the exact CI configuration;
- CLI build if the workflow builds it separately;
- app unit tests / build-for-testing;
- UI tests or test-without-building stage exactly as CI specifies;
- 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-testis green on the PR head after the repair.- lint remains green.
- no CI stage has been skipped/weakened.
project.ymland 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
left a comment
There was a problem hiding this comment.
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.swiftMacDown2/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:
- the range is non-empty and within the source;
- lower and upper-1 map to the same physical source line;
- the whole range belongs to exactly one existing top-level
PreviewBlockline range; - the generated representation is
.markdown; - the generated Markdown contains no
\nor\r; - 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.upperBoundThen 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:
- sort inline placements by
sourceRange.lowerBound, then upper bound, then original result index; - convert them to block-relative
InlineSplices; - 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
\ror\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:
- inline range in
Hello **marker** worldreplaces only the marker and preserves the rest. - two ordered inline ranges in one paragraph both apply.
- Unicode before the range proves UTF-16 coordinates are honored (
😀is two UTF-16 code units). - CRLF document with an inline placement still produces correct source.
- generated inline Markdown containing
\nis rejected and original block is unchanged. - source range spanning two physical lines is rejected.
- range crossing two top-level blocks is rejected.
.inlinenever produces.customkind.- output lineRange equals the original containing block's lineRange.
- inline + block contributions in the same multi-line paragraph compose correctly around the block split.
- overlapping inline-inline and inline-block cases preserve the rejected authored source according to pass 6's deterministic overlap policy.
- 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
left a comment
There was a problem hiding this comment.
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.swiftMacDown2/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:
result.sourceGeneration == currentGeneration; otherwise reject with an error diagnostic and preserve source.- representation must be
.markdown;.htmlis unsupported by current Preview and must be rejected with an explicit error diagnostic, not silently ignored. - capture a
Candidatewith originalresultIndex.
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
sourceRangeto 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:
sourceRange.lowerBoundascending;sourceRange.upperBoundascending;- original
resultIndexascending.
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:
- negative lower bound, e.g.
-1 ..< 4; - zero-width
0 ..< 0; - upper bound beyond
sourceMap.utf16Length; - fully beyond EOF;
- valid bounds that land on a blank line between Preview blocks;
- range spanning two top-level blocks;
- block placement that covers only part of a physical line;
- partial block placement inside a non-paragraph container;
- unsupported HTML representation;
- stale generation;
- overlapping inline-inline;
- overlapping block-block;
- overlapping inline-block;
- adjacent non-overlapping placements both succeed;
- malformed first result followed by valid result: valid result still places and invalid one does not consume overlap/budget state;
- sourceText/sourceMap mismatch causes all placement to fail closed with base unchanged;
- 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
left a comment
There was a problem hiding this comment.
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:
- invalid, stale, diagnostic-only, unsupported, or overlapping results must not consume Preview budget;
- budget rejection must preserve the authored source range;
- budget rejection must be observable, never silent;
- the budget must bound generated payload as well as placement count;
- 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.swiftMacDown2/MacDown2Tests/PreviewContributionAdapterTests.swiftplanning/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:
- matching source-generation token;
- result has content and Markdown representation;
- strict non-empty UTF-16 document bounds;
- physical-line mapping;
- exactly one containing base block;
- placement capability (
.block/.inlineconstraints from passes 2 and 5); - deterministic overlap check against already accepted ranges;
- 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:
- 65 otherwise-valid standalone markers under
.standard: exactly 64 contributions are placed; marker 65 remains authored; exactly one count-budget diagnostic is emitted. - 64 diagnostic-only/stale/invalid results followed by one valid result: the valid result is admitted because ineligible results consume no budget.
- Tiny count budget
2: three valid ranges -> first two admitted deterministically, third preserved, one summary diagnostic. - Tiny aggregate-byte budget: first small generated fragment admitted, oversized next fragment rejected, later small fragment still admitted if it fits.
- Integer-overflow path for aggregate bytes is rejected fail-closed (exercise via a test budget/state seam rather than allocating enormous strings if practical).
- Accepted + budget-rejected markers in the same CommonMark paragraph preserve the rejected marker and all neighboring authored text.
- A generated fragment larger than
PreviewBlock.oversizeByteThresholdbut below the aggregate budget is allowed to form aPreviewBlockwhoseisOversize == true; it is not rejected by this adapter merely for being >64 KiB. - Invalid/overlapping ranges do not alter the admitted-count or aggregate-byte counters.
- 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.isOversizefallback. - 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
left a comment
There was a problem hiding this comment.
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:
- contributors cooperate while doing long work;
ContributionRegistrychecks cancellation before and after every contributor and before converting an ordinary thrown error into a diagnostic;DocumentEditorSplitViewchecks 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.swiftMacDown2/Packages/MacDownKit/Sources/Contributions/TOCContribution.swiftMacDown2/Packages/MacDownKit/Sources/Contributions/DeterministicTestContribution.swiftMacDown2/Packages/MacDownKit/Tests/ContributionsTests/ContributionRegistryTests.swiftMacDown2/Packages/MacDownKit/Tests/ContributionsTests/TOCContributionTests.swiftMacDown2/MacDown2/PreviewContributionAdapter.swiftMacDown2/MacDown2/DocumentEditorSplitView.swiftMacDown2/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 resultsThis 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 resultsKeep 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
- Existing
.hangscancellation test remains and still throwsCancellationError. - New
.returnsAfterCancellation(content)contributor: start registry task, cancel it, await value -> registry throwsCancellationError; the deliberately returned content is never returned. - Registry
[returnsAfterCancellation, succeedingSecondContributor]: after cancellation, second contributor is never executed/returned. - 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.
- Cancelled task + contributor throwing an ordinary error after cancellation -> cancellation wins; no diagnostic-only result is returned.
TOCContribution.findMarkersobserves a task cancelled before invocation and throwsCancellationErrorrather than scanning/returning markers.
Preview adapter/view seam
PreviewContributionAdapter.resultspropagatesCancellationError; it never returns[]for cancellation.- Superseded-snapshot race: old task is made to return only after cancellation, newer snapshot publishes; old snapshot performs zero subsequent publication.
- Cancellation during/after composition but before publication leaves the latest valid Preview state untouched.
- 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?
) -> Booland test that helper plus adapter cancellation. Do not introduce a new observable object solely to make this testable.
Acceptance criteria
PreviewContributionAdapter.resultsisasync throwsand contains no blanket catch.ContributionRegistrychecks 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
left a comment
There was a problem hiding this comment.
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.swiftMacDown2/MacDown2/DocumentEditorSplitView.swiftMacDown2/MacDown2/PreviewBusyIndicator.swift— add the related contribution-status indicator here rather than adding another project fileMacDown2/MacDown2Tests/PreviewContributionAdapterTests.swift
Export:
MacDown2/MacDown2/ExportContributionAdapter.swiftMacDown2/MacDown2/ExportCoordinator.swiftMacDown2/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:.errorfor a rejected replacement;.warningonly for a condition where content is still validly placed;- message ends with or clearly states
authored source preservedfor 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]) -> AdaptedImplementation 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
ContributionResult(content: nil, diagnostics: [.error("boom")])->Composition.blocksequals authored base;Composition.diagnosticscontainsboomwith contribution ID.- Same shape with
.warning-> warning survives even though nothing is placed. - Valid content + warning -> content is placed and warning survives.
- Valid content + error -> authored source preserved and original error survives.
- Stale/malformed/unsupported result with its own warning -> both the contribution warning and adapter rejection diagnostic survive.
- Budget summary diagnostics from pass 7 appear in
Composition.diagnostics. - New parse clears stale diagnostics before new composition publishes.
- Cancelled old task cannot clear or replace diagnostics from the newer snapshot.
PreviewContributionDiagnosticIndicatoris 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
- Replace current
aContentlessResultIsDroppedtest with: contentless diagnostic result ->adapted.contributions.isEmpty,adapted.unanchoredDiagnostics == [mapped diagnostic]. - Contentless result with no diagnostics -> both arrays empty.
- Content-bearing warning -> warning exists only on
adapted.contributions[0].diagnostics, not inunanchoredDiagnostics. - Content-bearing error -> remains attached so
DerivedContentComposerpreserves authored source and final prepared diagnostics include the error. - Throwing registry contribution + otherwise valid export -> export succeeds using authored Markdown fallback and final
ExportOutcomediagnostics include the registry failure. - 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 == nilresult 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.
ExportRequestand 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
left a comment
There was a problem hiding this comment.
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:
- what has actually landed;
- what remains intentionally pending in this draft;
- what user-visible behavior exists now;
- what exact manual cases matter;
- which canonical verification commands actually passed at this head;
- 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.mdwith the final behavior from passes 1–9; - update
planning/epics/README.mdonly 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
Contributionstarget 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:
- normal document with H1/H2/H3 +
[TOC]-> nested TOC appears in Preview; Alpha\n[TOC]\nOmega-> all three remain visible, with only marker replaced;- two
[TOC]lines in one CommonMark paragraph -> both render and middle authored text remains; - fenced-code
[TOC]remains literal; - four-space-indented
[TOC]remains literal; - whitespace-tolerant ordinary marker (
[TOC]) still renders when parser classifies it as paragraph; - save/mark-clean without editing -> rendered TOC does not disappear;
- edit source -> new parse recomputes TOC and no old snapshot flashes/publishes afterward;
- HTML export -> TOC appears, authored marker does not;
- PDF export preparation/render -> same content behavior;
- 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-testingThen 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):
- replace
FileDocument.mutationGenerationPreview-freshness language with parse-snapshot token semantics; - remove the accepted whole-paragraph replacement limitation;
- state paragraph semantic whitelist for
[TOC]; - document Preview
.inlineconstraints; - document strict fail-closed validation order;
- document 64-placement + aggregate generated-Markdown Preview budget and explicit fallback diagnostics;
- document cancellation propagation/publication guard;
- document Preview/Export anchorless diagnostic handling;
- 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:
- compare
git diff base...HEAD --name-onlyagainst the PR's “What this changes” section — every material subsystem in the diff is represented; - 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; - compare every checked verification box/evidence row against a real final-head command or CI result;
- confirm residual-risk section contains no defect that passes 1–9 just fixed;
- 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
left a comment
There was a problem hiding this comment.
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()thenpreviewContributionComposition = 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 ?? previewBlocksIndicator:
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:
- no placements -> append the original
PreviewBlockvalue unchanged; - inline placements only -> call
spliceInlineBlock(...)and emit one block with the original kind + line range; - 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:
- get the fragment's original absolute UTF-16 range with
sourceMap.utf16Range(ofLines: lines); - extract the fragment from
sourceTextviaNSString; - select inline placements whose absolute ranges are fully within that fragment range;
- convert each absolute range to fragment-local UTF-16 offsets by subtracting
fragmentRange.location; - splice them into the extracted fragment in descending local range order (or equivalent one-pass assembly);
- 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:
- authored prefix block with line-1 inline replacement applied;
- custom block contribution on line 2;
- 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
left a comment
There was a problem hiding this comment.
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.mutationGenerationsemantics or save/recovery behavior;MarkdownEngine.SourceMapclamping behavior or parser block taxonomy;PreviewBlock.oversizeByteThresholdor Textual's existing oversize fallback;ScrollSyncMapordering/search algorithm;- the public
Contributingprotocol signature; - the package dependency direction (
Contributionsremains renderer-neutral and must not import Preview or ExportService); ExportRequest,PreparedExportDocument,ExportResult,ExportResourceBudget, orDerivedContentComposerpublic contracts;- the 4096-fragment / 16 MiB Export resource policy;
.github/workflows/ci.ymlgates 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-testingCapture 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: UIntDo 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 resultsCancellation 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:
try Task.checkCancellation();- obtain that physical line's source range from
document.sourceMap; - extract with UTF-16/
NSStringsemantics already used by the repository; - trim whitespace/newline characters and require exact
[TOC]; - require:
document.block(atLine: line)?.kind == .paragraph- 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 resultsKeep 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; .hangscontributor still propagates cancellation;.returnsAfterCancellationcannot 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-parallelDo 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 PreviewFinal 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:
- if any result diagnostic has severity
.error, do not create a candidate; authored source will remain; - require
result.sourceGeneration == currentGeneration; otherwise add adapter error diagnostic and reject; - require
content != nil; contentless results are now finished after diagnostic forwarding; - require
.markdown(markdown)representation; unsupported representation -> error diagnostic + reject; - validate the absolute half-open source range before any SourceMap call:
0 <= lower < upper <= sourceText.utf16.count
- record a candidate containing at least:
originalResultIndex
contributionID
placement
markdown
sourceRange // Range<Int>, absolute UTF-16A 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 ... endLineFind 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
\nnor\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:
- authored prefix lines before the first block placement;
- custom block placement;
- authored lines between block placements;
- custom block placement;
- 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:
- get the fragment's original absolute
NSRangeusingsourceMap.utf16Range(ofLines:); - extract the fragment from the original
sourceText; - select only inline placements fully contained by that fragment range;
- convert those absolute inline ranges to fragment-local UTF-16 offsets;
- splice them in descending local range order;
- emit a
PreviewBlockwith 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
ScrollSyncMapresolves 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 ?? previewBlocksThe 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]) -> AdaptedFor every result:
- map its diagnostics to
ExportDiagnostic; - if
content == nil, append mapped diagnostics tounanchoredDiagnosticsand continue; - if content exists, keep those diagnostics attached to the
ExportDerivedContributiononly; .markdown-> render using the existing ExportService Markdown-fragment seam;- unsupported
.html-> empty generated HTML + appended anchored.errordiagnostic stating authored source is preserved; - 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.contributionsinto 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-testingThen 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:
- reconcile
planning/epic-14-implementation.mdwith the actual implemented semantics; - remove the old accepted whole-paragraph replacement limitation;
- 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;
- replace PR #54's stale body using architecture pass 10's exact section structure;
- record only verification results that actually ran on the final head;
- keep the PR draft if EPIC-14's text-filter half remains pending;
- 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.revisionas its source-snapshot token rather thanFileDocument.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]\nOmegapreserve Alpha and Omega in Preview? - Are block contributions exact whole-source-line replacements?
- Are only paragraph containers partially split?
- Is
.inlinegenuinely 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
isOversizefallback 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-testinggate 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
contributionResultsstate and body-timemerged(...)call are replaced rather than layered on top. The existingpreviewBlocksremains 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
isOversizesafety 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
mutationGenerationremains 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.
Implementation hand-off — architecture locked for head
|
Architecture takeover — implementation lock (authoritative)Pinned review target: PR #54 head 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
Frozen compatibility invariantsThese are implementation constraints, not suggestions.
Shared Preview composition seamFindings 2, 5, 6, 7, and part of 9 converge on App-private result typesReplace the current catch-and-return-empty/ 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. 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
) -> PreviewContributionCompositionDo not read Strict admission pipelineProcess each original For each content-bearing result, apply this exact order:
After validation, sort candidates by 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 Source-ordered block compositionGroup accepted candidates by their containing base block. Walk base blocks in their original order.
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 Render fragments through the existing Deterministic IDs for affected blocksKeep untouched IDs exactly. For each emitted fragment, derive a private app-side UUID from a stable serialization of:
Hash the UTF-8 key with Architecture pass 1/10 — Preview freshness uses the parsed snapshotFinding: non-text Files and symbols
Exact implementation
struct PreviewContributionTaskID: Hashable {
let documentIdentity: ObjectIdentifier
let parsedRevision: Int?
}
Required tests
Immediate architecture reviewPASS (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 paragraphFinding: replacing the containing Preview block deletes/hides adjacent lines when Files and symbols
Exact implementationUse 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]
afterPreview must produce, in order:
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
Immediate architecture reviewPASS (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 Architecture pass 3/10 — Discover TOC markers from parsed semanticsFinding: physical-line scanning interprets Files and symbols
Exact implementation
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
Immediate architecture reviewPASS (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 productionFinding: the pinned head is red while the exact base is green. File
Exact implementationAdd: import FoundationThe failing symbols are the three Required verificationRun 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 reviewPASS (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
|
Joncallim
left a comment
There was a problem hiding this comment.
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 FoundationDo not:
- change
project.ymlor any target dependency; - add
Foundationto 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.URLat 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
left a comment
There was a problem hiding this comment.
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:
.blockreplaces an exact, line-owned directive range and becomes one or more standalone Preview blocks, as specified in pass 2;.inlinereplaces 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.swiftMacDown2/Packages/MacDownKit/Sources/Contributions/ContributionContent.swift— tighten documentation onlyMacDown2/MacDown2Tests/PreviewContributionAdapterTests.swiftplanning/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 inlineThis 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
PreviewBlockvalue unchanged; - inline placements only: emit one rewritten
PreviewBlockwith the originalkindandlineRange; - 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:
- the range is wholly contained by exactly one base block's absolute UTF-16 range;
sourceMap.line(atUTF16Offset: range.lowerBound)equalssourceMap.line(atUTF16Offset: range.upperBound - 1);- the Markdown representation contains neither
\nnor\r; - the range does not overlap another accepted placement;
- 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]
) -> StringabsoluteSourceRange 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 outputUse 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:
- extract the authored fragment with
SourceMap+NSString; - call
spliceInlineusing the fragment's absolute UTF-16 range; - 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:
- one inline replacement in the middle of a paragraph changes only that span and leaves one block with the same line range;
- two non-overlapping inline replacements are applied in source order without offset drift;
- an inline replacement containing Unicode before/inside/after the range is exact;
- CRLF source on surrounding lines remains unchanged;
- inline Markdown containing
\nis rejected with authored source preserved; - inline Markdown containing
\ris rejected likewise; - an inline range crossing a physical-line boundary is rejected;
- an inline range crossing two top-level blocks is rejected;
- inline placements in authored prefix/suffix fragments around a block placement both survive;
- inline/block overlap rejects the later conflicting placement and preserves that placement's authored source;
- Export adapter still maps
.inlineto.inlineand.blockto.block.
Acceptance criteria
- There is no code path where
.inlineproduces a standalone custom Preview block. - The containing block's
lineRangeis 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
left a comment
There was a problem hiding this comment.
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.swiftMacDown2/MacDown2Tests/PreviewContributionAdapterTests.swiftplanning/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:
- collect every renderer diagnostic from every
ContributionResult, including results whosecontentisnil; - verify the source snapshot itself;
- turn content-bearing results into candidates;
- validate representation and raw range bounds;
- resolve one containing base block;
- apply placement-specific shape rules from passes 2 and 5;
- sort valid candidates deterministically;
- reject overlaps;
- apply the pass-7 placement budget;
- 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:
sourceRange.lowerBoundascending;sourceRange.upperBoundascending;- original
ordinalascending.
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..<0,-1..<1,0..<0, EOF zero-width, and past-EOF ranges all preserve base blocks.- None of those malformed ranges map to line 1 or the last line through clamping.
- A valid range at source start and a valid range ending exactly at EOF are accepted.
- Unicode before the range does not change UTF-16 correctness.
- A range crossing two top-level blocks is rejected.
- A whole-line block range is accepted; a partial-line block range is rejected.
- A partial block range in a list/table/code/HTML container is rejected; an exact whole-block replacement remains accepted.
- Exact duplicates accept one and diagnose the rest.
- Partial overlap and containment overlap are rejected deterministically.
- A rejected early candidate does not starve a later valid candidate.
- Mismatched
SourceMap/text returns the original base wholesale with one system diagnostic. - 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
left a comment
There was a problem hiding this comment.
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.swiftMacDown2/MacDown2Tests/PreviewContributionAdapterTests.swiftplanning/epic-14-implementation.md
Remove the current cap at the wrong layer
Delete:
private static let maxMergedResults = 64and 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 = .standardProduction 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
- 65 valid one-line block placements all render under
.standard. - 64 diagnostic-only/stale/unsupported results before one valid result do not starve it.
- 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.
- Invalid and overlapping candidates do not consume the count budget.
- With a small byte limit, one oversize candidate is preserved while a later small candidate still renders.
- Aggregate-byte addition overflow fails closed.
- Count and byte limits hit together still produce one bounded summary diagnostic.
- A result set under both limits produces no budget diagnostic.
- Untouched blocks remain byte-for-byte and ID-for-ID equal.
- 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
prefixis 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
left a comment
There was a problem hiding this comment.
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.swiftMacDown2/MacDown2/PreviewContributionAdapter.swiftMacDown2/MacDown2/DocumentEditorSplitView.swiftMacDown2/Packages/MacDownKit/Tests/ContributionsTests/ContributionRegistryTests.swiftMacDown2/MacDown2Tests/PreviewContributionAdapterTests.swiftplanning/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 = generationThe task must follow this order:
- await
PreviewContributionAdapter.results; try Task.checkCancellation();- compose results using the captured document/text/generation;
try Task.checkCancellation();- verify the live session still has the captured revision and published text;
- publish
contributedPreviewBlocksandcontributionDiagnosticstogether.
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:
- an already canceled task throws before the first contributor runs;
- a contributor throwing
CancellationErrorpropagates it and later contributors do not run; - 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;
- ordinary thrown errors remain isolated as diagnostic-only results and later contributors still run.
In PreviewContributionAdapterTests:
resultspropagates cancellation instead of returning[];resultsstill returns[]for a genuinely absent document/text without error;- 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;
- final snapshot mismatch guard rejects an otherwise successful old composition;
- 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
left a comment
There was a problem hiding this comment.
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.swiftMacDown2/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.swiftMacDown2/MacDown2/ExportCoordinator.swiftMacDown2/Packages/MacDownKit/Sources/ExportService/ExportRequest.swiftMacDown2/Packages/MacDownKit/Sources/ExportService/ExportComposer.swiftMacDown2/MacDown2Tests/ExportContributionAdapterTests.swiftMacDown2/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;
.helptext: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
previewContributionIssueIndicatorand 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]) -> BatchFor each result:
content == nil: mapresult.diagnosticsintoBatch.diagnostics;- content-bearing Markdown: place mapped diagnostics on that
ExportDerivedContributionas today; - content-bearing unsupported HTML: place the result diagnostics plus the adapter's unsupported-HTML error on that derived contribution, so
DerivedContentComposerpreserves 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:
- throwing contributor ->
content == nildiagnostic appears inComposition.diagnosticswhile base blocks remain; - diagnostic-only warning survives;
- content-bearing warning survives once;
- adapter rejection diagnostic includes contribution ID/range and preserves source;
- diagnostics clear between snapshot revisions;
- cancellation publishes no stale diagnostic;
- deterministic IDs remain stable for the same composition;
- issue indicator is absent for zero diagnostics, exposes count for nonzero diagnostics, and summarizes >20 rows.
Export adapter/composer:
- diagnostic-only result yields zero contributions and one batch diagnostic;
- content-bearing diagnostic stays attached to the derived contribution and is not duplicated in batch;
- unsupported HTML carries original + adapter error exactly once;
ExportRequestdefault diagnostics leave existing prepared output unchanged;- supplied anchorless contribution diagnostics appear in
PreparedExportDocument.diagnostics; - ordinary standalone/self-contained/PDF diagnostic policies remain unchanged;
- coordinator's existing successful-export issue alert receives the diagnostic-only failure.
Acceptance criteria
- No
compactMapdiscards 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
left a comment
There was a problem hiding this comment.
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.mdplanning/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;
TOCContributionis 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:
- land and verify the remaining text-filter scope promised by the PR title/body; or
- 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
## TraceabilityWhat 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:
- headings plus one standalone
[TOC]render in Preview and HTML/PDF export; Alpha\n[TOC]\nOmegapreserves both authored lines and shows the TOC between them;- two
[TOC]lines in one CommonMark paragraph both render and preserve middle text; - fenced and four-space-indented
[TOC]remain literal; - two-space-indented paragraph
[TOC]remains active; - save/mark-clean without editing does not make the TOC disappear;
- CRLF and Unicode documents preserve exact surrounding text;
- 65 valid markers do not stop at 64;
- no-heading document shows the documented placeholder;
- 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 themThen 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:
- contribution contracts and registry;
- TOC contributor and semantic marker detection;
- Preview composition and diagnostics UI;
- Export adaptation and anchorless diagnostic channel;
- tests;
- project/Package/XcodeGen wiring;
- text-filter slices, accurately pending or landed;
- 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.
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>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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>
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.mdplaces 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
Packages/MacDownKit/Sources/Contributions):Contributing,ContributionResult/ContributionContent/ContributionPlacement/ContributionRepresentation/ContributionDiagnostic, andContributionRegistry(fault-isolating per contribution; cancellation propagates viatry Task.checkCancellation()before each contribution, immediately after each one returns, and once more before the registry returns — never converted into an empty successful result).sourceGenerationis an opaque, caller-selected snapshot token compared only for equality — Preview keys it toMarkdownDocument.revision; Export continues using theFileDocument.mutationGenerationof 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.paragraphblock (non-recursivedocument.blockscheck). 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.PreviewContributionAdapter.compose(...), split acrossPreviewContributionAdmission.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 —.inlinesplices in place,.blockreplaces 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 fromCryptoKit.SHA256over their role/span/contribution-id (neverHasher/a randomUUID()).PreviewContributionSession, mirrorsMarkdownParseSession's shape): keyed by document/tab identity + parsed revision (PreviewContributionTaskID), holds one atomicPreviewContributionComposition(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-textFileDocumentchange (save, rename, encoding) can no longer make a valid composed TOC disappear.PreviewContributionDiagnosticsBadgebeside 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.ExportContributionAdapter.adapt(_:) -> Adaptation): renders each placeable.markdownresult viaExportService.renderMarkdownFragmentinto anExportDerivedContributionfor E12's existingDerivedContentComposer; a diagnostic-only result (nothing to anchor to) becomes astandaloneDiagnosticsentry instead of being dropped.ExportCoordinator.combinedDiagnostics(_:_:)mergesstandaloneDiagnosticsbefore the export service's own diagnostics, identically for the HTML and PDF paths (one helper, both call it, tested directly).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 existingExportResourceBudget(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:RecoveryBuffer+StorageSupport.swift,migrateLegacyFenceLedger): unconditionally swept every legacy (non-generation-encoded) recovery epoch into the durableretiredset on ledger load, even when that epoch was still the recorded current owner for its document.isRetired's legacy branch checks onlyretiredmembership, 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.NSEventconstruction crash (CommandStateRefreshTests):NSEvent.mouseEvent(with:...)asserts itstypeis 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 usingNSEvent.otherEvent(with:...)instead.ExternalFileControllerRecoveryTests,ExternalFileControllerCloseRecoveryTests,ScriptedRecoveryExecutor): aweak var modeldeallocated 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
.html(ContributionRepresentation)case remains real, typed, and unhandled — Preview has no HTML-fragment rendering path today (every block goes throughPreviewMarkupParser/Textual as Markdown-shaped text); a contribution producing.htmlgets an explicit diagnostic and its authored source is preserved.id, and Preview hands every link toNSWorkspace.shared.openrather 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
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:
[TOC]and a[TOC]written between two lines of one paragraph both render correctly in Preview, with surrounding text intact;[TOC]inside a fenced code block stays literal text;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
Contributions, the two app-target adapters, and one new session/view pair — no publicExportService/E12 API changed, no new SPM dependency or target, no CI/workflow change.ContributionRegistry.standard's membership andDerivedContentComposerare unchanged.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.git revertof this PR's commits.Links
planning/epic-14-implementation.md(§19 for this remediation)🤖 Generated with Claude Code