fix(core): preserve GUID/instructions/options across markdown round-trip - #196
Open
oldrepublicwizard wants to merge 4 commits into
Open
fix(core): preserve GUID/instructions/options across markdown round-trip#196oldrepublicwizard wants to merge 4 commits into
oldrepublicwizard wants to merge 4 commits into
Conversation
added 3 commits
July 29, 2026 21:10
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.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
These were the origin docs for this branch's work but were never committed alongside it.
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
Before this fix, a mod guide's
<!--<<ModSync>>metadata block (which carries the GUID, install instructions, and Choose-option data alongside the human-readable markdown) silently failed to parse whenever it was regenerated from an installed instruction file and re-imported. The guide would come back with a brand-new random GUID and no instructions — install tracking and Choose trees quietly broke on round-trip. On guides with more than one instruction, the ones that did parse could have their file actions (Extract/Move/Delete/Patcher) shifted onto the wrong entry.Both bugs traced to the same class of assumption:
DeserializeComponentrequired aNamefield even for the embedded metadata block, which never carriesNameby design (it lives in the surrounding markdown header). Parsing always threw, and the code silently fell back to a lossy legacy parser that assigned a fresh GUID. Fixed by threading an optionalrequireNameparameter through the deserialize chain, defaulting totrueeverywhere except the two embedded-metadata parse paths.Action/Name) as the signal that one entry's fields ended and the next began. Real content putsGuidbeforeAction/Name, so multi-instruction blocks got fields shifted onto the wrong entries. Fixed by detecting entry boundaries from key repetition instead, which works regardless of field order.This also finishes re-enabling three round-trip test suites (
DocumentationRoundTripTests,MarkdownFileTests,MarkdownImportTests) that were excluded from compilation, fixing the compile/runtime issues that surfaced, and addsC2RoundtripInvariantTestsas a self-contained regression guard for the invariant (no external corpus dependency).Testing
MarkdownFileTests11/11,C2RoundtripInvariantTests4/4,MarkdownImportTests14/15 (the 1 failure is a pre-existing, unrelated category/tier regex bug),DocumentationRoundTripTestsclean skip/pass.MarkdownTomlParityTests.Kotor1Full_SourceFiles_CurrentlyContainKnownSemanticDivergencesneeds themod-buildscorpus, which CI provisions; not runnable in this review environment.Post-Deploy Monitoring & Validation
Failed to deserialize componentorMissing or invalid 'Name' field— an uptick would indicate a gap in therequireNamefix.caee366f..be15582c); no schema or data migration involved.