feat(parsing): lossless roundtrip via unified guide ingest/emit pipeline - #198
Merged
oldrepublicwizard merged 17 commits intoJul 30, 2026
Merged
Conversation
added 16 commits
July 30, 2026 04:50
Embedded <!--<<ModSync>> metadata blocks never carry a Name field (it lives in the markdown header), but DeserializeComponent required Name unconditionally, so parsing that block always threw and silently fell back to a lossy legacy parser that assigned a fresh random GUID. Thread an optional requireName parameter through DeserializeComponent, DeserializeYamlComponent, DeserializeModComponentFromTomlString, and ModComponent.DeserializeTomlComponent, defaulting to true everywhere except the two embedded-metadata-block parse paths in MarkdownParser. Also fix instruction/option grouping in ProcessInstructionsAndOptions and GroupKeyValuePairsIntoInstructions/GroupKeyValuePairsIntoOptions: boundary detection keyed on a hardcoded field name (Action/Name) broke when Guid legitimately preceded it in field order. Use duplicate-key detection instead, which is order-independent. Corrects MarkdownFileTests' own fixture, which was itself invalid YAML (brace-wrapped GUIDs parse as YAML flow-mappings; unindented fields de-indent out of their list item), and adds C2RoundtripInvariantTests as a self-contained regression guard for the invariant.
…params ProcessInstructionsAndOptions's inline Instructions/Options loops handled a KeyValuePair<string, object> case that can never execute there: that branch only runs when hasKeyValuePairs is false, and any KeyValuePair item's runtime type name always starts with "KeyValuePair", which would have made hasKeyValuePairs true. Removed the dead branch and the now- unused accumulator dictionaries. Also added XML doc comments for the requireName parameter on DeserializeYamlComponent, DeserializeModComponentFromTomlString, and ModComponent.DeserializeTomlComponent, which had gained it without documentation (DeserializeComponent already had one).
…API fixups) U1: remove the <Compile Remove> exclusions for DocumentationRoundTripTests.cs, MarkdownFileTests.cs, and MarkdownImportTests.cs in ModSync.Tests.csproj so they compile and run again. U2: fix the compile/runtime issues re-enabling them surfaced: - DocumentationRoundTripTests.cs: skip via Assert.Inconclusive when the test_modbuild_k1.md fixture is absent, instead of a hard Assert.Fail; fix a stale ModLinkFilenames reference to the current Dependencies property. - MarkdownImportTests.cs: skip via Assert.Inconclusive when the mod-builds corpus is absent; update to current ModComponent/Category APIs (no more CSharpKOTOR.ModComponent, Category is now a list); remove assertions on Instruction.Guid, which no longer exists on that type; drop assertions tied to corpus content that no longer matches the current fixture.
…s (U1) Docusaurus-style :::note/:::warning/:::tip fences were previously hardcoded to only Installation Instructions and Known Bugs, and only paired with one specific admonition type each. The live mod-builds guide format (used since Oct 2025) wraps any field in any of the five admonition types, so generalize BuildFencePattern across all 9 multiline fields and strip the shared `: ` definition-list prefix at the single ExtractValue chokepoint. Also fixes a real bug the new mixed-style test surfaced: fieldBoundary (the shared lookahead terminating a bold-inline field capture) didn't recognize a following ::: fence as a boundary, so a bold-inline field immediately followed by a fenced field swallowed the fence text into its own capture.
Directions prose that reads as actionable (contains a verb) but matches no known instruction pattern was previously logged at verbose level and silently dropped. NaturalLanguageInstructionParser.ParseInstructions now has an overload reporting these as unparsedGaps, distinct from prose recognized as pure commentary (which is correctly skipped, not a gap). DraftInstructionResult carries UnparsedGaps and GenerateDraftInstructions now emits one result per component with Directions prose - including components where nothing drafted - so CLI/GUI callers can render "N of M directions produced no draft" instead of the gap disappearing.
AE8: guide prose of the shape "delete X; if also using HD Visas, additionally delete Y" previously either merged Y into the same unconditional Delete instruction as X, or - when phrased as a single comma-joined sentence with no semicolon/period separator - silently dropped Y's clause entirely (confirmed via a minimal repro: only 1 of 2 described deletions drafted). SplitIntoProcessingUnits now recognizes "if (also) using X, additionally/ also Y" as an explicit clause boundary (both the inline form and the "bare" form already split off by an earlier semicolon/then), decomposing it into its own tagged processing unit. ParseInstructions threads that tag through as a new conditionalDrafts output; DraftInstructionService surfaces it via DraftInstructionResult.ConditionalDrafts and an InstallationWarning note, so the conditional deletion is captured as its own instruction (never merged or dropped) and flagged so a reviewer knows not to apply it unconditionally.
…ssing (U4) AE7: a Directions entry describing per-mod deletion driven by an externally-maintained cleanlist file (redrob's cleanlist_k1.txt convention) previously fell through to the generic Delete patterns, which mismatched fragments of the prose into a nonsensical fixed file path (e.g. "<<modDirectory>>\files listed in cleanlist_k1") - silently wrong, with zero gap signal. TryHandleCleanlistReference now runs before the generic pattern list: when the unit names a specific cleanlist file, it drafts a CleanList instruction referencing that file (resolution of which files it deletes stays deferred to install time, exactly like Instruction. ExecuteCleanListAsync already does - this never fabricates a fixed list). When only a bare "cleanlist" mention appears with no nameable file, it surfaces as an unparsed gap instead of guessing. Either way the unit no longer reaches the generic Delete patterns.
AE6 verification (HQ Blasters sequence: delete keblastore.utm to force
an intentional single TSLPatcher error, run the patcher, rename/delete
several more files afterward) surfaced a real, confirmed bug: 16
Delete/Move/Patcher/exception patterns share a boundary idiom that uses
a bare '.' as one alternative meaning "end of clause". Since a filename's
own extension separator is also a literal '.', this bare alternative
matched THERE instead, truncating "keblastore.utm" to "keblastore" (a
silently wrong deletion target) and, in multi-file lists, cutting the
capture short mid-list.
Changed the boundary's '.' alternative to '.(?=\s|$)' (period followed by
whitespace or end-of-string) across all 16 occurrences - a genuine
clause-ending period is always followed by whitespace/EOL by the time
SplitIntoProcessingUnits hands a unit to these patterns (trailing
periods are already stripped upstream), so this only prevents the
false-positive match on an extension dot; no legitimate clause-ending
match is affected.
Also verified the wildcard-prefix rename step in the same sequence
("rename w_ionrfl_04.* files to w_ionrfl_004.*") has no matching pattern
and correctly surfaces as an unparsed gap (U2) rather than drafting a
wrong rename or disappearing silently - satisfying AE6's "drafts
correctly or produces a clean, reviewable gap" bar without needing a new
pattern for that step.
… port (U6) GuideIngestService previously routed everything (including markdown) through ModComponentSerializationService.DeserializeModComponentFromString, which internally calls MarkdownParser but discards the resulting MarkdownParserResult's PreambleContent/EpilogueContent/WidescreenWarning Content/AspyrExclusiveWarningContent/Trace by writing them to a throwaway local MainConfig instance. GuideIngestResult only ever carried Components/DraftResults/DetectedFormat, so callers going through the port (rather than calling MarkdownParser directly) had no way to see those sections or the parse trace. GuideIngestService.IngestFromText now detects markdown format (via the same format-alias list DeserializeModComponentFromString uses, or DetectFormatFromContent when no format hint is given) and calls MarkdownParser directly for that case, populating the new GuideIngestResult fields. Non-markdown formats (TOML/YAML/JSON/XML) continue through the unchanged generic deserializer path - they have no equivalent content sections to carry. IGuideEmitService.EmitMarkdown/EmitMarkdownAsync also gained optional widescreenWarningContent/aspyrExclusiveWarningContent parameters, forwarding to GenerateModDocumentation's existing support for them (previously only reachable by bypassing the port).
…uide port (U7) CLI convert previously loaded components via the generic deserializer and, separately, called DraftInstructionService.GenerateDraftInstructions directly when --parse-directions was set. Both calls are now one call through GuideIngestService.Instance.IngestFromText, carrying the same --parse-directions value as before (off by default). Extracted FileLoadingService.ReadFileContentAndFormatHintAsync (Core) so CLI can read file content with the same UTF-8-fallback/case-insensitive-path handling LoadFromFileAsync already had, without a second deserialize pass. GUI FileLoadingService (paste and non-editor-mode file-open) previously constructed a MarkdownParser and separately called GenerateDraftInstructionsFromProseAsync (-> DraftInstructionService) directly. Both now route through the same port call, with each caller's existing draft-flag default preserved unchanged: file-open stays off, paste stays on. The interactive editor-mode path (RegexImportDialog, a live regex-configuration UI, not a plain parse-and-get-result flow) is intentionally left calling MarkdownParser directly - rerouting it through the generic port would lose its in-dialog preview/configuration capability for no benefit. GUI's "Generate Documentation" button now calls GuideEmitService. Instance.EmitMarkdown instead of ModComponentSerializationService. GenerateModDocumentation directly, with the same widescreen/Aspyr parameters. CLI/GUI drafting now share the exact same GuideIngestService. IngestFromText call, making parity a structural guarantee rather than something requiring a separate integration test - extended the existing GuiSmokeHeadlessTests paste-import test to assert the draft/review-flag behavior still holds through the new path.
C2RoundtripInvariantTests.cs already existed (cherry-picked from prior work) but its one "Choose"-named test only documented a known limitation (the natural-language #### Instructions/##### Option markdown format is not parsed) - it never actually built a Choose instruction, emitted it, and confirmed the tree survives re-ingestion. Adds two self-contained (no ./mod-builds dependency) tests: - C2_ChooseComponent_EmitAndReingest_PreservesChooseTreeAndBranchInstructions: builds a component with a Choose instruction referencing two Options, each with its own branch instruction, emits via the real GenerateModDocumentation path, and confirms the instruction, both option branches, and each branch's own instructions survive intact. - C2_MixedChooseAndPlainComponents_PreservesCountAndOrder: a plain component and a Choose component together preserve count and document order through the same round-trip.
C2_ChooseComponent_EmitAndReingest_PreservesChooseTreeAndBranchInstructions authored its expected Source paths with a literal backslash separator, but the real emit->re-parse round trip normalizes to forward slash. Match the fixture's expectation to what the round trip actually produces (caught by running the corpus-pinned NUnit suite for U9).
Corrected AGENTS.md's stale clone instruction (th3w1zard1/mod-builds ->
KOTOR-Community-Portal/mod-builds, dev branch) - the canonical repo, not
the frozen non-canonical TOML-only fork.
MarkdownTomlParityTests.cs hard-failed via Assert.That(File.Exists(...),
Is.True) when ./mod-builds was absent, instead of skipping cleanly like
the other three corpus-dependent test files already did. Now uses
Assert.Ignore, with a comment explaining the canonical repo has never had
a TOMLs/ directory - it only ever existed in the separate, frozen (no
commits since 2025-10-31) oldrepublicwizard/mod-builds snapshot, so this
C3 cross-check is historical/best-effort, not a release gate.
Measured against a live pinned read of KOTOR-Community-Portal/mod-builds
dev @ 336b36d911b879b0254da5d08a6cce72c28607e5: confirmed no TOMLs/
directory exists (both parity tests skip cleanly), and surfaced a
pre-existing, severe performance defect in MarkdownParser's per-field
regex scanning - time scales roughly cubically with document size
(confirmed on the pre-branch baseline too, not solely introduced by this
plan's changes, though the admonition-fence generalization measurably
worsens the constant factor for fence-heavy documents). A full real guide
(~150KB) takes on the order of tens of minutes to parse, impractical for
default CI/local runs. Tagged RealGuide_ModBuildsMarkdown_
DraftedInstructionsAreAllSandboxed [Category("Slow")] (excluded by the
existing default TestCategory!=Slow runsettings filter, matching this
project's established convention) until the underlying scan-scoping
performance issue gets its own dedicated fix - flagging this as a
release-blocking follow-up, not silently working around it.
Deduplicated three constants/lists that had drifted into copies across this branch's new code: the <<modDirectory>> placeholder (now shared from DraftInstructionService), the markdown format-alias list (now exposed once from ModComponentSerializationService instead of duplicated in GuideServices), and the newline-split array used to strip definition-list prefixes (now shared from MarkdownUtilities). Also unified ApplyReviewFlag/ApplyConditionalDraftNote's identical append-unless-present logic into one AppendWarningIfMissing helper. Added cheap substring pre-filters (checking for "using"/"cleanlist") before the new conditional-clause and redrob-cleanlist regexes in NaturalLanguageInstructionParser, since both require that literal text and were otherwise running unconditionally on every processing unit. GUI FileLoadingService.LoadMarkdownContentAsync: narrowed configuredProfile's scope to the editor-mode branch that actually uses it, and made explicit (via a log warning) that a custom MarkdownImportProfile has no effect through the non-editor-mode guide ingest port - previously silently dropped with no signal. No behavior change; verified via GuideIngestionTests, C2RoundtripInvariantTests, MarkdownAdmonitionFenceTests, and MarkdownFileTests (all green).
…arse Tier 2 code review (9 reviewers) confirmed three real bugs, one with cross-reviewer agreement across correctness/testing/api-contract: 1. Since U2 changed GenerateDraftInstructions to return one result per component with Directions prose (not just drafted ones), both the CLI (ModBuildConverter.cs's validation-issue loop) and the GUI (FileLoadingService.cs's GenerateDraftInstructionsFromProseAsync) were unconditionally applying the "DRAFT INSTRUCTIONS" review flag to every component in DraftResults, including ones that drafted nothing. Both now gate on DraftInstructionCount > 0, matching DraftInstructionService. ApplyReviewFlag's own internal gate. 2. The CLI's "Drafted instructions for N component(s)" log message counted the same inflated DraftResults.Count instead of the actually-drafted subset. 3. GuideIngestService.IngestFromText's auto-detect path (no format hint) called DetectFormatFromContent, which parses content as markdown just to detect the format, then parsed the same content a second time in IngestMarkdown - a genuine double full-parse for the common no-hint markdown case (e.g. CLI convert --stdin), compounding on top of MarkdownParser's already-documented cubic-scaling cost. Now tries TOML first (cheap, fails fast, preserves the original detection precedence) then attempts markdown directly and keeps that result instead of re-parsing. Added a regression test (CliConvert_StdinWithParseDirections_ DoesNotReviewFlagZeroDraftComponent) covering a two-component guide where one component drafts and one doesn't, asserting only the drafted one carries the review flag through the TOML round-trip. Simplify pass (dedup, pre-filters, GUI FileLoadingService scope narrowing) and remaining residual findings from review (chained-conditional-clause edge cases, hardcoded CleanList destination, file-size threshold, GUI generic file-open bypassing the port for non-.md-extension-checked callers) are tracked as known residuals, not fixed in this pass - see PR description.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
The .NET Format, Format and Fix, and lint_csharp CI checks failed on whitespace/indentation and file-encoding issues in test files touched this branch (C2RoundtripInvariantTests.cs, DocumentationRoundTripTests.cs, MarkdownAdmonitionFenceTests.cs, MarkdownFileTests.cs, MarkdownImportTests.cs). Ran `dotnet format whitespace` locally and committed the result - whitespace-only diff, no behavioral change. Verified via the affected test suites (all green except the one pre-existing unrelated RawRegexPattern_ExtractsCategoryTier failure).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The guide markdown that ModSync ingests has drifted from what the parser assumed: the canonical corpus (
KOTOR-Community-Portal/mod-builds) switched to Docusaurus-style:::note/:::warningfences in October 2025, never had a paired TOML file the way the parser's tests assumed, and its author has permanently declined to standardize free-text instruction wording. This PR closes the gaps that assumption left behind, grounded in reading the actual live corpus and the guide author's own correspondence rather than guessing.Parser generalization:
:::note/:::warning/:::tip), previously hardcoded to 2 of 9 multiline fields, now applies to all of themCleanListinstruction referencing the external file, instead of being silently resolved to a wrong fixed file list.as a clause-boundary marker, which also matched a filename's own extension separator, truncatingkeblastore.utmtokeblastoremid-parse. Fixed by requiring the period be followed by whitespace or end-of-string.Unified pipeline:
IGuideIngestService/IGuideEmitServicenow carry preamble/epilogue/widescreen/Aspyr/trace/warnings instead of dropping them at the port boundary (the generic deserializer discarded this by writing to a throwawayMainConfiginstance)convertand GUI file-open/paste now route through that one shared port instead of each callingMarkdownParser/DraftInstructionServiceindependently, so CLI/GUI drafting parity is now structural rather than something a separate integration test has to proveMeasurement:
KOTOR-Community-Portal/mod-buildsdev@336b36d9) and ran the full suite against itMarkdownParser's per-field regex scanning scales roughly cubically with document size (verified against the pre-branch baseline, not introduced by this PR — though the fence generalization does add a modest ~1.7x constant-factor cost on fence-heavy documents). A full real guide (~150KB) takes on the order of tens of minutes. Tagged the affected test[Category("Slow")]per the project's existing convention rather than silently working around it — this needs its own dedicated fix.MarkdownTomlParityTeststhrew instead of skipping when./mod-buildswas absent) and correctedAGENTS.md's stale clone instructions (the org was renamed/forked; canonical isKOTOR-Community-Portal, notth3w1zard1)Code Review
A Tier 2 review ran (9 personas, given the diff's size) and found 3 real bugs, confirmed by cross-reviewer agreement — all fixed and covered by a new regression test:
GenerateDraftInstructionschanged to return one result per component with Directions prose (not just drafted ones), but both CLI and GUI still applied the "DRAFT INSTRUCTIONS" flag unconditionally — now gated onDraftInstructionCount > 0.GuideIngestServicedouble-parsed markdown on the no-format-hint auto-detect path (DetectFormatFromContentparses to detect, thenIngestMarkdownparsed again) — now tries TOML first (cheap, preserves detection order) and reuses the markdown parse instead of repeating it.Known Residuals
Lower-priority findings tracked but not fixed in this pass (all
owner: human, none blocking):TryHandleCleanlistReferencehardcodes the CleanList destination toOverrideNaturalLanguageInstructionParser.cshas crossed the 1k-line threshold without decompositionFileLoadingService.cs's non-editor branch does a manual field-by-fieldGuideIngestResult→MarkdownParserResultmapping alongside editor-mode's separate dialog-based constructionmodsync://links) still bypasses the new port for.mdcontent — only the "Load File" button and paste flows get full preamble/epilogue/widescreen/Aspyr/trace parityTest Plan
RawRegexPattern_ExtractsCategoryTier) and two Avalonia headless font-rendering failures (sandbox environment limitation, not code)Post-Deploy Monitoring & Validation
Ships in the next ModSync release.
"Stored 0 characters in preamble"where content was expected); an uptick in "DRAFT INSTRUCTIONS" flags on components that shouldn't have any drafted instructions (would indicate the zero-draft-flag fix regressed); CLI convert taking much longer than before on stdin/no-format-hint input (would indicate the double-parse fix regressed).