Feat/aio consolidation - #199
Open
oldrepublicwizard wants to merge 37 commits into
Open
Conversation
ProfileService persists per-profile paths, instruction file, and component selections; ProfileManagerDialog and menu entry support capture, activate, clone, rename, and delete without refactoring MainConfig consumers.
Parse FOMOD ModuleConfig.xml into typed models, detect FOMOD archives, and map installer steps to ModSync Option/Choose instructions. GUI installer dialog deferred to a follow-up slice.
Expose ProfileService through a headless profile CLI (list/show/create/delete/activate/clone/rename) so agents can manage loadouts without the GUI, closing a Tier-1 agent-native audit gap.
Emit structured validation pipeline results on stdout with console log suppression so agents can parse exit status, stage summaries, and dry-run issues without desktop ValidatePage UX.
Expose headless settings CRUD against the shared settings.json store with merge-safe writes, secret redaction, and tests so agents can configure paths and managed deploy without the GUI.
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.
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).
HEAD already carries an evolved ProfileService implementing IProfileStore with GetProfileArtifactDirectory; the branch content is the older ancestor. Recorded as merged so the branch no longer reads as unlanded work.
…/aio-consolidation
Resolves the ModBuildConverter dispatcher conflict by keeping both verbs: ProfileOptions/RunProfileAsync and SettingsOptions/RunSettingsAsync are now registered side by side rather than one replacing the other. Docs sections in core-cli-reference.md were de-interleaved into separate profile and settings sections. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9bxfbA3ekSx3sAeSxXrCX
continual-learning.json holds machine-local session ids and timestamps that arrived via the clutter clone's 'sync local changes' commit. Ignore it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9bxfbA3ekSx3sAeSxXrCX
Keeps the machine-readable console wrapper (try/finally around the parser plus ShouldUseValidateJsonOutput) from feat/validate-json-output while retaining the ProfileOptions and SettingsOptions verbs. Parity docs updated: profile CRUD and structured JSON validation are both shipped, so they no longer read as open gaps. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9bxfbA3ekSx3sAeSxXrCX
…D work) HEAD already carries the shipped FOMOD support: ModuleConfig parser, GUI/CLI post-download configure, --fomod-choices and the configured-only gate. The branch predates that and still documents the GUI wizard as a deferred slice. Recorded as merged so it no longer reads as unlanded work.
Recovered from an orphaned worktree in clutter/ whose git metadata was gone, so this work existed only as uncommitted files. - Detect Cloudflare interstitials and retry up to 3 times with backoff, rather than parsing the challenge page as if it were the mod page. - Share one CookieContainer between HttpClientHandler and the DeadlyStream handler so the session cookie survives the challenge round-trip. - Read csrfKey from the hidden form input as well as the inline script. - Fail with a message pointing at the manual download URL instead of an opaque parse error. The companion DownloadCacheService hunk from the same worktree is not included: it calls TryGetComponentRegistryMetadata/GetKnownFilenamesFromRegistry, which live in the unmerged feat/k2-prestige-nlp-folder-move branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9bxfbA3ekSx3sAeSxXrCX
ResolveSettingsFilePath redirected to the legacy AppData settings.json whenever the target file was missing, including when the caller passed --settings-dir. So 'settings --action set --settings-dir /tmp/scratch' wrote to the user's real settings file. An explicit directory is now authoritative; the legacy fallback applies only when no directory was requested. The existing tests pre-created settings.json in their temp directory, so the fallback never triggered. Added coverage for the missing-file case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9bxfbA3ekSx3sAeSxXrCX
dotnet format style flagged IMPORTS errors in five files whose using blocks were reordered by the merges. Using-statement ordering only; no behavior change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9bxfbA3ekSx3sAeSxXrCX
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.
No description provided.