From 981e168fb9daf9b08d588a74fb5d5f31f62b7bf5 Mon Sep 17 00:00:00 2001 From: Erik Date: Sat, 15 Aug 2026 08:19:23 +0200 Subject: [PATCH] fix(launcher): Campaign LA gate-round-1 review findings F1-F6 + hardening F1: the crash reporter comment claimed the launcher never holds a password in any field - false (ProfileEditorDialogViewModel, AccountProfile.Password, StartRequest.Password). Reworded to the true, narrower invariant (no throw site interpolates a credential VALUE into an exception message) and pinned it with CrashReportNeverContainsAStoredPassword: a real STJ failure over a profiles document containing a known password, corrupted after the credential, must yield a crash file with the stack and without the value. F2: the co-deploy Inputs covered only Bake own sources; a Content edit never refreshed the 83 MB exe. Now the full reference closure. Fixing it surfaced two more incrementality traps, both fixed and comment-documented: SkipUnchangedFiles left the output older than the triggering input (target re-ran forever - added an explicit Touch), and %(Item.Metadata) in a plain Include does not batch (the literal percent-text became a permanently out-of-date phantom input - globs are now spelled per project). Verified: Core edit retriggers, then two consecutive clean incremental builds. F3: RID publishes ran BOTH co-deploy paths (two self-contained bake publishes). Build-time target now guarded on _IsPublishing; verified a real win-x64 publish runs zero build-target co-deploys and still ships both exes. F4: comment misattributed PublishBakeTool=false to CI lanes; it is target-local recursion guarding. F5: the x:Name reflection sweep now walks the markup as XML and tolerates template-scoped names (no generated field exists for those). F6: dead using removed. Hardening: the crash reporter positional --data-dir fallback requires a fully-qualified path so a relative or flag-shaped value cannot create ./crash-reports at an arbitrary CWD. Launcher 67/67, Launcher.Core 317/317. Co-Authored-By: Claude Fable 5 --- docs/ISSUES.md | 16 ++-- docs/plans/2026-08-14-launcher-campaign.md | 19 +++- src/AcDream.Launcher/AcDream.Launcher.csproj | 45 ++++++++- src/AcDream.Launcher/MainWindow.axaml.cs | 1 - src/AcDream.Launcher/Program.cs | 38 +++++--- .../MainWindowViewTests.cs | 91 ++++++++++++++++++- 6 files changed, 178 insertions(+), 32 deletions(-) diff --git a/docs/ISSUES.md b/docs/ISSUES.md index 3a092547..2559c242 100644 --- a/docs/ISSUES.md +++ b/docs/ISSUES.md @@ -112,12 +112,16 @@ isolated root with the full stack; the real root stays empty. **Redaction, stated exactly (deliberate narrowing of the filed acceptance):** the report never serializes the command line, environment, or process state, but exception TEXT may quote an option name or path. -That is safe by construction — credentials never enter launcher state -(child-stdin only; `LauncherProcessSpec` has no credential member, guarded -by its reflection test) — so no scanning pass over this sink exists today. -The code comment carries the standing condition: if a credential ever -becomes reachable from launcher state, this sink needs the status-stream's -credential scanning. +The gate-round-1 review (F1) corrected the original by-construction claim: +the launcher DOES hold credentials (`ProfileEditorDialogViewModel`, +`AccountProfile.Password`, `StartRequest.Password`); the true invariant is +narrower — no code path interpolates a credential VALUE into an exception +message. That invariant is now PINNED by +`MainWindowViewTests.CrashReportNeverContainsAStoredPassword`: a real +STJ parse failure over a profiles document containing a known password, +corrupted after the credential so the parser consumed the value, must +produce a crash file with the stack and without the password. If that test +ever fails, this sink needs the status-stream's credential scanning. ## #397 — Windows: LauncherProcessSupervisor.Stop has no reliable graceful-stop signal for a no-window console host diff --git a/docs/plans/2026-08-14-launcher-campaign.md b/docs/plans/2026-08-14-launcher-campaign.md index 9d4dc94f..327effdb 100644 --- a/docs/plans/2026-08-14-launcher-campaign.md +++ b/docs/plans/2026-08-14-launcher-campaign.md @@ -738,8 +738,23 @@ launcher, incremental, `--help` verified). One transient 65/66 on the first post-merge test run did not reproduce across a clean rebuild + six repeats — consistent with stale-artifact mixing, but if it EVER recurs, capture the failing test name before anything else. Merged slice worktrees/branches -(la2/la3/la7a/la-uitest) removed. The §A–I connected script remains the -open user gate. +(la2/la3/la7a/la-uitest) removed. Opus batch review: PASS (HIGH +confidence) with 6 findings, all landed same-day: F1 the crash reporter's +by-construction claim was FALSE (the launcher holds passwords in three +fields; the true invariant — no throw site interpolates a credential +value — is now pinned by a forced-failure test), F2 the co-deploy's +Inputs covered only Bake's own sources, not its Content/Platform/Core/ +Plugin.Abstractions closure (the stale-artifact class again; fixing it +exposed and fixed two more incrementality traps: SkipUnchangedFiles +leaving outputs older than inputs, and %(Item.Metadata) in a plain +Include not batching — a literal '%(...)' input is permanently +out-of-date), F3 dual bake publish on RID publishes (guarded by +_IsPublishing; verified 0 build-target co-deploys during a real publish), +F4 misattributed comment, F5 template-scoped x:Name false-fail (sweep now +walks the XML with template-ancestor tolerance), F6 dead using, plus the +optional Path.IsPathFullyQualified hardening on the crash reporter's +--data-dir fallback. Launcher 67/67, Launcher.Core 317/317. The §A–I +connected script remains the open user gate. ## Ledger diff --git a/src/AcDream.Launcher/AcDream.Launcher.csproj b/src/AcDream.Launcher/AcDream.Launcher.csproj index 01a0dfbc..5ae49a22 100644 --- a/src/AcDream.Launcher/AcDream.Launcher.csproj +++ b/src/AcDream.Launcher/AcDream.Launcher.csproj @@ -17,19 +17,45 @@ (PublishCoDeployedBakeTool below), but an ordinary `dotnet build` did not, so a developer-built launcher reached the first-run wizard with no bake tool beside it. This does for Build what that target - does for Publish. Set to false to skip (CI publish lanes pass - PublishBakeTool=false into the inner build for the same reason). --> + does for Publish. Set to false to skip; the two co-deploy targets + themselves pass PublishBakeTool=false into their inner Bake builds so + the recursion can never nest (gate-round-1 review F4: no CI lane sets + it — the guard is target-local). --> true <_BakeExecutableName Condition="$([MSBuild]::IsOSPlatform('Windows'))">acdream-bake.exe <_BakeExecutableName Condition="'$(_BakeExecutableName)' == ''">acdream-bake + them: MSBuild evaluates a target's Inputs before the target body runs. + The globs must cover Bake's FULL source closure — Bake references + Content and Platform (and through them Core and Plugin.Abstractions), + so an edit to any of those must retrigger the republish or the + co-deployed exe silently serves stale code (gate-round-1 review F2: + the original Bake-only globs meant a Content edit never refreshed the + 83 MB exe, the exact stale-artifact class this project has been bitten + by before). Keep this list in step with AcDream.Bake's reference + closure. --> + <_BakeToolSource Include="$(MSBuildProjectDirectory)\..\AcDream.Bake\**\*.cs" Exclude="$(MSBuildProjectDirectory)\..\AcDream.Bake\bin\**\*.cs;$(MSBuildProjectDirectory)\..\AcDream.Bake\obj\**\*.cs" /> <_BakeToolSource Include="$(MSBuildProjectDirectory)\..\AcDream.Bake\AcDream.Bake.csproj" /> + <_BakeToolSource Include="$(MSBuildProjectDirectory)\..\AcDream.Content\**\*.cs" + Exclude="$(MSBuildProjectDirectory)\..\AcDream.Content\bin\**\*.cs;$(MSBuildProjectDirectory)\..\AcDream.Content\obj\**\*.cs" /> + <_BakeToolSource Include="$(MSBuildProjectDirectory)\..\AcDream.Content\AcDream.Content.csproj" /> + <_BakeToolSource Include="$(MSBuildProjectDirectory)\..\AcDream.Platform\**\*.cs" + Exclude="$(MSBuildProjectDirectory)\..\AcDream.Platform\bin\**\*.cs;$(MSBuildProjectDirectory)\..\AcDream.Platform\obj\**\*.cs" /> + <_BakeToolSource Include="$(MSBuildProjectDirectory)\..\AcDream.Platform\AcDream.Platform.csproj" /> + <_BakeToolSource Include="$(MSBuildProjectDirectory)\..\AcDream.Core\**\*.cs" + Exclude="$(MSBuildProjectDirectory)\..\AcDream.Core\bin\**\*.cs;$(MSBuildProjectDirectory)\..\AcDream.Core\obj\**\*.cs" /> + <_BakeToolSource Include="$(MSBuildProjectDirectory)\..\AcDream.Core\AcDream.Core.csproj" /> + <_BakeToolSource Include="$(MSBuildProjectDirectory)\..\AcDream.Plugin.Abstractions\**\*.cs" + Exclude="$(MSBuildProjectDirectory)\..\AcDream.Plugin.Abstractions\bin\**\*.cs;$(MSBuildProjectDirectory)\..\AcDream.Plugin.Abstractions\obj\**\*.cs" /> + <_BakeToolSource Include="$(MSBuildProjectDirectory)\..\AcDream.Plugin.Abstractions\AcDream.Plugin.Abstractions.csproj" /> @@ -53,9 +79,14 @@ scatter AcDream.Content/Chorizite assemblies into the launcher's output and risk colliding with its own. Inputs/Outputs keep incremental builds from re-publishing it every time. --> + @@ -74,6 +105,12 @@ + + diff --git a/src/AcDream.Launcher/MainWindow.axaml.cs b/src/AcDream.Launcher/MainWindow.axaml.cs index 90f6c3be..4a4e7083 100644 --- a/src/AcDream.Launcher/MainWindow.axaml.cs +++ b/src/AcDream.Launcher/MainWindow.axaml.cs @@ -3,7 +3,6 @@ using AcDream.Launcher.ViewModels; using Avalonia.Controls; using Avalonia.Input; using Avalonia.Interactivity; -using Avalonia.Markup.Xaml; using Avalonia.Platform.Storage; using Avalonia.Threading; diff --git a/src/AcDream.Launcher/Program.cs b/src/AcDream.Launcher/Program.cs index 982aad60..7a8895ad 100644 --- a/src/AcDream.Launcher/Program.cs +++ b/src/AcDream.Launcher/Program.cs @@ -58,22 +58,28 @@ internal static class Program /// exception goes to a file under the resolved data root instead of to /// stderr, and the path is printed. /// - /// Redaction contract, stated exactly. This method writes only the - /// exception chain plus non-identifying host facts; it never serializes - /// , the environment, or process state. It does NOT - /// claim the text is value-free: an exception message may quote whatever - /// the thrower put in it, including an offending option name or a path - /// (observed: "Launcher option '--x' requires a value"). That is acceptable - /// because a credential cannot reach this text by construction — the - /// launcher never holds a password in any field, credentials go straight to - /// a child process's stdin, and LauncherProcessSpec carries no - /// credential member (guarded by its own test). If that ever changes, this - /// sink needs the same credential scanning the status stream has. + /// Redaction contract, stated exactly (corrected by the gate-round-1 + /// review, F1). This method writes only the exception chain plus + /// non-identifying host facts; it never serializes , + /// the environment, or process state. Exception TEXT may quote whatever + /// the thrower put in it — an option name, a path (observed: "Launcher + /// option '--x' requires a value"). The launcher DOES hold credentials: + /// ProfileEditorDialogViewModel's password field, + /// AccountProfile.Password (plaintext by user decision), and + /// StartRequest.Password. The invariant this sink actually rests on + /// is narrower: NO code path interpolates a credential VALUE into an + /// exception message (System.Text.Json failures quote the JSON path, not + /// the value; option/path errors quote the flag, not file contents). + /// MainWindowViewTests pins it with a forced-failure test asserting + /// a stored password never appears in the crash file. Any new throw site + /// that puts profile or request state into a message breaks that test — + /// at which point this sink needs the status-stream's credential + /// scanning, not a bigger comment. /// /// Never throws: a crash reporter that can itself fail would replace /// the original failure with its own. /// - private static string? TryWriteCrashReport(string[] args, Exception failure) + internal static string? TryWriteCrashReport(string[] args, Exception failure) { try { @@ -124,14 +130,18 @@ internal static class Program /// reporter only, so an isolated run keeps its evidence inside its own /// roots even when option parsing is what failed. Never used for anything /// the launcher actually runs on — - /// remains the only validated path authority. + /// remains the only validated path authority. Fully-qualified paths only + /// (gate-round-1 review): a relative or flag-shaped value would create + /// ./<value>/crash-reports wherever the CWD happens to be, + /// which defeats the isolation this fallback exists to preserve. /// private static string? TryReadRequestedDataDirectory(string[] args) { for (int index = 0; index + 1 < args.Length; index++) { if (string.Equals(args[index], "--data-dir", StringComparison.Ordinal) - && !string.IsNullOrWhiteSpace(args[index + 1])) + && !string.IsNullOrWhiteSpace(args[index + 1]) + && Path.IsPathFullyQualified(args[index + 1])) { return args[index + 1]; } diff --git a/tests/AcDream.Launcher.Tests/MainWindowViewTests.cs b/tests/AcDream.Launcher.Tests/MainWindowViewTests.cs index c117563a..39713aac 100644 --- a/tests/AcDream.Launcher.Tests/MainWindowViewTests.cs +++ b/tests/AcDream.Launcher.Tests/MainWindowViewTests.cs @@ -1,5 +1,5 @@ using System.Reflection; -using System.Text.RegularExpressions; +using System.Xml.Linq; using AcDream.Launcher.Core.Installation; using AcDream.Launcher.Core.Launching; using AcDream.Launcher.Core.Orchestration; @@ -74,10 +74,23 @@ public sealed class MainWindowViewTests "src", "AcDream.Launcher", "MainWindow.axaml"); - string markup = File.ReadAllText(markupPath); - List names = Regex - .Matches(markup, "x:Name=\"([^\"]+)\"") - .Select(match => match.Groups[1].Value) + // Walk the markup as XML rather than regexing the raw text: + // template-scoped names (inside a DataTemplate/ControlTemplate/ + // ItemTemplate) get NO generated backing field, so demanding one + // would false-fail the first time a template gains an x:Name + // (gate-round-1 review F5 — latent today, MainWindow has two + // templates with none inside). + XDocument document = XDocument.Load(markupPath); + XNamespace x = "http://schemas.microsoft.com/winfx/2006/xaml"; + List names = document + .Descendants() + .Where(element => element.Attribute(x + "Name") is not null) + .Where(element => !element + .Ancestors() + .Any(ancestor => ancestor.Name.LocalName.EndsWith( + "Template", + StringComparison.Ordinal))) + .Select(element => element.Attribute(x + "Name")!.Value) .Distinct(StringComparer.Ordinal) .ToList(); @@ -260,6 +273,74 @@ public sealed class MainWindowViewTests throw new DirectoryNotFoundException("Could not find AcDream.slnx."); } + /// + /// Gate-round-1 review F1: the crash reporter's safety rests on the + /// invariant that no code path interpolates a credential VALUE into an + /// exception message — the launcher genuinely holds passwords + /// (ProfileEditorDialogViewModel, AccountProfile.Password, + /// StartRequest.Password), so "no password in any field" was never the + /// guarantee. This test pins the real one against the most + /// credential-adjacent realistic failure: a profiles-shaped document + /// that CONTAINS the password and is corrupted AFTER it, so the JSON + /// parser has consumed the credential value before throwing. + /// System.Text.Json quotes paths and positions, never values — if that + /// (or any future throw site) ever changes, this fails and the sink + /// needs the status-stream's credential scanning. + /// + [Fact] + public void CrashReportNeverContainsAStoredPassword() + { + string root = Path.Combine( + Path.GetTempPath(), + "acdream-tests", + Path.GetRandomFileName()); + string dataDirectory = Path.Combine(root, "data"); + const string password = "hunter2-gate-round-1-secret"; + string corruptProfiles = + "{ \"version\": 1, \"servers\": [ { \"name\": \"s\", \"host\": \"h\", " + + "\"port\": 9000, \"accounts\": [ { \"account\": \"a\", \"password\": \"" + + password + + "\", \"characters\": [ } ] } ] }"; + + Exception failure; + try + { + _ = System.Text.Json.JsonSerializer.Deserialize( + corruptProfiles); + throw new InvalidOperationException( + "The corrupt fixture unexpectedly parsed; the test premise is broken."); + } + catch (System.Text.Json.JsonException jsonFailure) + { + failure = new InvalidOperationException( + "Profile load failed during startup.", + jsonFailure); + } + + try + { + string? report = Program.TryWriteCrashReport( + ["--data-dir", dataDirectory], + failure); + + Assert.NotNull(report); + // Isolation re-pinned: the report must land under the caller's + // --data-dir, never the machine's real data root. + Assert.StartsWith(dataDirectory, report, StringComparison.OrdinalIgnoreCase); + string content = File.ReadAllText(report); + Assert.Contains("JsonException", content); + Assert.Contains(" at ", content); + Assert.DoesNotContain(password, content, StringComparison.OrdinalIgnoreCase); + } + finally + { + if (Directory.Exists(root)) + { + Directory.Delete(root, recursive: true); + } + } + } + /// /// Minimal no-op orchestrator. These tests exercise MainWindow's own /// dispatcher/focus wiring, not orchestrator behavior (already covered