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 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
1f87acf1af
commit
981e168fb9
6 changed files with 178 additions and 32 deletions
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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). -->
|
||||
<CoDeployBakeToolOnBuild Condition="'$(CoDeployBakeToolOnBuild)' == ''">true</CoDeployBakeToolOnBuild>
|
||||
<_BakeExecutableName Condition="$([MSBuild]::IsOSPlatform('Windows'))">acdream-bake.exe</_BakeExecutableName>
|
||||
<_BakeExecutableName Condition="'$(_BakeExecutableName)' == ''">acdream-bake</_BakeExecutableName>
|
||||
</PropertyGroup>
|
||||
|
||||
<!-- Declared outside the target so the Inputs/Outputs check below can see
|
||||
them: MSBuild evaluates a target's Inputs before the target body runs. -->
|
||||
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. -->
|
||||
<!-- Spelled out per project rather than batched: %(Item.Metadata) inside a
|
||||
plain Include does NOT batch — the literal '%(...)' text survives into
|
||||
the input list as a nonexistent file, which is permanently out-of-date
|
||||
and silently defeats the incrementality this exists for (observed). -->
|
||||
<ItemGroup>
|
||||
<_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" />
|
||||
</ItemGroup>
|
||||
|
||||
<ItemGroup>
|
||||
|
|
@ -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. -->
|
||||
<!-- '_IsPublishing' != 'true': Publish depends on Build, so without the
|
||||
guard a RID publish ran BOTH this target and PublishCoDeployedBakeTool —
|
||||
two full self-contained Bake publishes per launcher publish (gate-round-1
|
||||
review F3). The publish target alone owns that lane; if _IsPublishing is
|
||||
ever absent the cost returns but the output stays correct. -->
|
||||
<Target Name="CoDeployBakeToolToBuildOutput"
|
||||
AfterTargets="Build"
|
||||
Condition="'$(CoDeployBakeToolOnBuild)' == 'true' and '$(PublishBakeTool)' == 'true' and '$(DesignTimeBuild)' != 'true'"
|
||||
Condition="'$(CoDeployBakeToolOnBuild)' == 'true' and '$(PublishBakeTool)' == 'true' and '$(DesignTimeBuild)' != 'true' and '$(_IsPublishing)' != 'true'"
|
||||
Inputs="@(_BakeToolSource)"
|
||||
Outputs="$(OutputPath)$(_BakeExecutableName)">
|
||||
<PropertyGroup>
|
||||
|
|
@ -74,6 +105,12 @@
|
|||
<Copy SourceFiles="$(_BakeBuildStagingDirectory)$(_BakeExecutableName)"
|
||||
DestinationFolder="$(_BakeBuildOutputDirectory)"
|
||||
SkipUnchangedFiles="true" />
|
||||
<!-- The Touch is load-bearing for incrementality: when the rebuilt exe is
|
||||
byte-identical, SkipUnchangedFiles leaves the output OLDER than the
|
||||
source edit that triggered this run, so the target would re-run on
|
||||
every subsequent build forever (observed). Outputs must end newer
|
||||
than Inputs whenever the target completes. -->
|
||||
<Touch Files="$(_BakeBuildOutputDirectory)$(_BakeExecutableName)" />
|
||||
<Message Importance="high"
|
||||
Text="Co-deployed $(_BakeExecutableName) ($(_BakeBuildRid)) next to the launcher." />
|
||||
</Target>
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
///
|
||||
/// <para>Redaction contract, stated exactly. This method writes only the
|
||||
/// exception chain plus non-identifying host facts; it never serializes
|
||||
/// <paramref name="args"/>, 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 <c>LauncherProcessSpec</c> carries no
|
||||
/// credential member (guarded by its own test). If that ever changes, this
|
||||
/// sink needs the same credential scanning the status stream has.</para>
|
||||
/// <para>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 <paramref name="args"/>,
|
||||
/// 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:
|
||||
/// <c>ProfileEditorDialogViewModel</c>'s password field,
|
||||
/// <c>AccountProfile.Password</c> (plaintext by user decision), and
|
||||
/// <c>StartRequest.Password</c>. 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).
|
||||
/// <c>MainWindowViewTests</c> 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.</para>
|
||||
///
|
||||
/// <para>Never throws: a crash reporter that can itself fail would replace
|
||||
/// the original failure with its own.</para>
|
||||
/// </summary>
|
||||
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 — <see cref="LauncherStartupOptions"/>
|
||||
/// 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
|
||||
/// <c>./<value>/crash-reports</c> wherever the CWD happens to be,
|
||||
/// which defeats the isolation this fallback exists to preserve.
|
||||
/// </summary>
|
||||
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];
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<string> 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<string> 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.");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
[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<System.Text.Json.JsonElement>(
|
||||
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);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Minimal no-op orchestrator. These tests exercise MainWindow's own
|
||||
/// dispatcher/focus wiring, not orchestrator behavior (already covered
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue