wip: Campaign LA LA1 fix round — INCOMPLETE, stopped mid-task

Agent was stopped for token budget partway through the LA1 review fix
round. Landed here: F1 best-effort SessionStatusWriter, F2 App reader
tolerance (paths/mode), F5 argument-parsing hardening, plus new tests.
NOT DONE: F4 shared-fixture production shape (was the next step), F3
reconnect disconnected edge + recorded limitation, F6 exited
idempotency/reasons, F7 structural redaction test, F8 platform-guard
test + comment fix, optional RuntimeOptions PrintMembers redaction.

Build/test state UNVERIFIED at this commit. Next session: finish the
remaining findings, run the suites, then narrow re-review.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik 2026-08-14 16:32:52 +02:00
parent c9fc7f4a66
commit 75a6724d5b
9 changed files with 628 additions and 52 deletions

View file

@ -0,0 +1,73 @@
namespace AcDream.App.Configuration;
/// <summary>
/// Campaign LA slice LA1 review fix (F5): extracted from <c>Program.cs</c>'s
/// top-level-statement local functions so the trailing-flag edge case is
/// unit testable — a top-level program's local functions are compiler-
/// synthesized private members of the generated <c>Program</c> class with
/// no stable surface a test assembly can reach.
/// </summary>
internal static class SessionConfigArgumentParsing
{
/// <summary>
/// Finds <paramref name="flag"/> in <paramref name="arguments"/> and
/// returns its value. Three distinct outcomes, distinguished by
/// <paramref name="present"/> and the return value together:
/// <list type="bullet">
/// <item>flag absent: <paramref name="present"/> = <see langword="false"/>,
/// returns <see langword="null"/> — the caller's env-var/positional
/// fallback stays in effect, unchanged from before this flag
/// existed.</item>
/// <item>flag present with a following value: <paramref name="present"/>
/// = <see langword="true"/>, returns that value.</item>
/// <item>flag present but is the LAST argument, with nothing after it:
/// <paramref name="present"/> = <see langword="true"/>, returns
/// <see langword="null"/> — the caller MUST treat this as a hard error
/// (the flag was typed but its value was not), never silently fall
/// through to the flag-absent path.</item>
/// </list>
/// </summary>
internal static string? ExtractFlagValue(
string[] arguments,
string flag,
out bool present)
{
ArgumentNullException.ThrowIfNull(arguments);
ArgumentException.ThrowIfNullOrWhiteSpace(flag);
for (int i = 0; i < arguments.Length; i++)
{
if (!string.Equals(arguments[i], flag, StringComparison.Ordinal))
continue;
present = true;
return i == arguments.Length - 1 ? null : arguments[i + 1];
}
present = false;
return null;
}
/// <summary>Returns <paramref name="arguments"/> with <paramref name="flag"/>
/// and its following value (if any) removed. A trailing, valueless flag
/// is dropped on its own — this helper only strips arguments, it does
/// not decide whether a trailing flag is an error (see
/// <see cref="ExtractFlagValue"/>'s <c>present</c> output for that).</summary>
internal static string[] WithoutFlagAndValue(string[] arguments, string flag)
{
ArgumentNullException.ThrowIfNull(arguments);
ArgumentException.ThrowIfNullOrWhiteSpace(flag);
var result = new List<string>(arguments.Length);
for (int i = 0; i < arguments.Length; i++)
{
if (string.Equals(arguments[i], flag, StringComparison.Ordinal))
{
i++; // also skip the flag's value, if any
continue;
}
result.Add(arguments[i]);
}
return [.. result];
}
}

View file

@ -42,6 +42,26 @@ internal sealed class SessionConfiguration
internal sealed class SessionProcessSettings
{
public SessionContentDescriptor? Content { get; init; }
/// <summary>Campaign LA slice LA1 review fix (F2): accepted so the SAME
/// document also satisfies the Headless loader's own
/// <c>process.paths</c> member (<c>HeadlessPathOverrides</c>) — parsed
/// and ignored here, exactly like <see cref="SessionDescriptor.Policy"/>
/// and <see cref="SessionDescriptor.CharacterOptions"/> below. App has
/// no config/data/cache directory override concept of its own (those
/// come from <c>ApplicationPathSet</c>/env vars on this host); only the
/// Headless host consumes overrides composed under this key.</summary>
public SessionProcessPathOverrides? Paths { get; init; }
}
/// <summary>Accepted-but-ignored mirror of Headless's
/// <c>HeadlessPathOverrides</c> shape — see
/// <see cref="SessionProcessSettings.Paths"/>.</summary>
internal sealed class SessionProcessPathOverrides
{
public string? ConfigDirectory { get; init; }
public string? DataDirectory { get; init; }
public string? CacheDirectory { get; init; }
}
internal sealed class SessionContentDescriptor
@ -75,6 +95,19 @@ internal sealed record SessionDescriptor
/// App has no bot-policy concept.</summary>
public SessionPolicyDescriptor? Policy { get; init; }
/// <summary>Campaign LA slice LA1 review fix (F2): pinned-contract
/// mode discriminator. ABSENT means today's ONLY App behavior — an
/// ordinary play session — so every document written before this field
/// existed keeps parsing unchanged. <c>"probe"</c> (LA2's connect
/// ▸ characterList ▸ graceful-disconnect flow, no EnterWorld) is
/// HEADLESS-ONLY; the App loader rejects it with an explicit message
/// naming the field rather than the caller ever seeing a raw unmapped-
/// member <see cref="System.Text.Json.JsonException"/>. Any other value
/// is a configuration error — the pinned contract defines no other
/// mode literal, so a document is either silent about mode (play) or
/// says "probe" exactly.</summary>
public string? Mode { get; init; }
[JsonRequired]
public SessionCredentialDescriptor Credential { get; init; } = new();

View file

@ -154,5 +154,31 @@ internal static class SessionConfigurationLoader
throw new SessionConfigurationException(
$"Session '{session.Id}' statusFile must be a non-empty path when present.");
}
ValidateMode(session);
}
/// <summary>
/// Campaign LA slice LA1 review fix (F2): <c>mode</c> is Headless-only
/// on the App host — the graphical host has no probe concept (LA2
/// builds the probe in Headless only). An absent field is today's ONLY
/// App behavior (play); <c>"probe"</c> gets a specific, actionable
/// message instead of a cryptic unmapped-member JSON error; anything
/// else is a plain configuration error.
/// </summary>
private static void ValidateMode(SessionDescriptor session)
{
if (session.Mode is null)
return;
if (string.Equals(session.Mode, "probe", StringComparison.Ordinal))
{
throw new SessionConfigurationException(
$"Session '{session.Id}' has mode 'probe'; probe sessions "
+ "are headless-only and cannot run on the graphical host.");
}
throw new SessionConfigurationException(
$"Session '{session.Id}' has unsupported mode '{session.Mode}'.");
}
}

View file

@ -38,8 +38,22 @@ Log.Information(
// existing one positional dat-dir argument and every ACDREAM_* env var keep
// working exactly as before when the flag is absent. See
// docs/plans/2026-08-14-launcher-campaign.md LA1.
string? sessionConfigFlagPath = ExtractFlagValue(args, "--session-config");
string[] positionalArgs = WithoutFlagAndValue(args, "--session-config");
//
// Review fix F5 (LA1 review round): a trailing, valueless --session-config
// (the flag typed as the LAST argument, nothing after it) must be a hard
// error, never a silent fall-through to the env-var path — a launcher that
// mis-composed its argv would otherwise appear to work while quietly
// ignoring the session-config contract entirely.
string? sessionConfigFlagPath = SessionConfigArgumentParsing.ExtractFlagValue(
args, "--session-config", out bool sessionConfigFlagPresent);
if (sessionConfigFlagPath is null && sessionConfigFlagPresent)
{
Log.Error(
"--session-config requires a value (a path to the session-config document).");
return 2;
}
string[] positionalArgs =
SessionConfigArgumentParsing.WithoutFlagAndValue(args, "--session-config");
var datDirArg = positionalArgs.FirstOrDefault();
var envDatDir = Environment.GetEnvironmentVariable("ACDREAM_DAT_DIR");
@ -240,34 +254,9 @@ finally
return 0;
// Campaign LA slice LA1: --session-config <path> parsing helpers. Kept
// local/minimal rather than a general-purpose CLI parser — App has exactly
// one optional flag-with-value today; the positional dat-dir argument must
// stay untouched by its presence (see the comment above the flag parse).
static string? ExtractFlagValue(string[] arguments, string flag)
{
for (int i = 0; i < arguments.Length - 1; i++)
{
if (string.Equals(arguments[i], flag, StringComparison.Ordinal))
return arguments[i + 1];
}
return null;
}
static string[] WithoutFlagAndValue(string[] arguments, string flag)
{
var result = new List<string>(arguments.Length);
for (int i = 0; i < arguments.Length; i++)
{
if (string.Equals(arguments[i], flag, StringComparison.Ordinal))
{
i++; // also skip the flag's value
continue;
}
result.Add(arguments[i]);
}
return [.. result];
}
// Campaign LA slice LA1: --session-config value-presence helper. The
// flag/positional-argument extraction itself lives in
// AcDream.App.Configuration.SessionConfigArgumentParsing (review fix F5) so
// its trailing-flag edge case is unit testable.
static string? NullIfEmpty(string? value) =>
string.IsNullOrWhiteSpace(value) ? null : value;

View file

@ -33,6 +33,49 @@ namespace AcDream.Runtime.Session;
/// event method below takes only identifiers, names, and counts — there is no
/// parameter shape that could carry a password, by construction.
/// </para>
///
/// <para>
/// <strong>This writer can never fail or stall the session transaction it
/// observes</strong> (Campaign LA LA1 review fix F1). Every call site sits
/// inside a caller-owned try block that treats a throw as a real failure —
/// <c>LiveSessionController.StartCore</c>'s connect/roster/enter-world
/// sequence, <c>SessionStartCompositionPhase.Start</c> (which calls
/// <see cref="Started"/> BEFORE <c>Session.Start</c> even runs),
/// <c>GameWindow.CompleteShutdown</c> (which calls <see cref="Disconnected"/>
/// BEFORE <c>PublishShutdownRoots</c>, so a throw would skip graceful
/// teardown entirely), and <c>HeadlessSessionHost.Dispose</c>'s stage machine
/// (a throw from stage 8's <see cref="Exited"/> call leaves
/// <c>_disposeStage</c> unadvanced and <c>_disposed</c> unset forever — a
/// permanently un-disposable host). An observability sink that can fail the
/// transaction it is merely reporting on is a defect in the sink, not a
/// reason for every call site to defend itself — so every exception this
/// class's own I/O can raise (a missing parent directory on a fresh cache
/// dir, a path segment that collides with an existing file, a permissions
/// error, a network path some future caller supplies) is caught here, logged
/// once to stderr, and LATCHES the writer into a permanent no-op — the exact
/// same "cheap null-check forever after" shape a never-configured path
/// already gets. The parent directory is created lazily, once, on the first
/// write, inside the same protection, so a fresh
/// <c>.../launcher/sessions/&lt;id&gt;/status.jsonl</c> path (whose directory
/// does not exist yet) is the expected first-run case, not a failure.
/// </para>
///
/// <para>
/// <strong>Latency posture:</strong> every write is a synchronous local-disk
/// file open + line append + flush + close on the calling thread — there is
/// no batching, no background writer, no async path. This is fine for the
/// low-frequency lifecycle events this class carries (at most a handful per
/// second even under LA5/LA6 plugin/login-command load) against a local
/// disk. A <c>statusFile</c> path that resolves to a network location (a
/// UNC share, a mapped network drive, a FUSE mount with high per-syscall
/// latency) is UNSUPPORTED BY DESIGN — every event write would block the
/// session transaction's calling thread for the round-trip, and a slow or
/// wedged network path would eventually get caught by the same catch clause
/// that handles a missing directory and latch off, silently dropping the
/// rest of that session's status stream. Callers that need a status stream
/// over the network should tail the local file with a separate process,
/// never point <c>statusFile</c> at a network path directly.
/// </para>
/// </summary>
public sealed class SessionStatusWriter
{
@ -46,6 +89,8 @@ public sealed class SessionStatusWriter
private readonly string? _path;
private readonly TimeProvider _timeProvider;
private readonly object _gate = new();
private bool _directoryEnsured;
private bool _latchedOff;
public SessionStatusWriter(string? path, TimeProvider? timeProvider = null)
{
@ -54,12 +99,13 @@ public sealed class SessionStatusWriter
}
/// <summary>
/// True when this writer has a configured path and will actually append
/// events. Lets a caller with an expensive report to build (e.g. the
/// roster projection) skip that work entirely when nobody configured a
/// status file for this session.
/// True when this writer has a configured path and has not latched
/// itself off after a failed write. Lets a caller with an expensive
/// report to build (e.g. the roster projection) skip that work entirely
/// when nobody configured a status file for this session, or when this
/// writer already gave up after an I/O failure.
/// </summary>
public bool IsEnabled => _path is not null;
public bool IsEnabled => _path is not null && !_latchedOff;
public void Started(string sessionId) =>
Write(new
@ -143,20 +189,71 @@ public sealed class SessionStatusWriter
private void Write<T>(T value)
{
if (_path is not { } path)
if (_path is not { } path || _latchedOff)
return;
string line = JsonSerializer.Serialize(value, JsonOptions);
lock (_gate)
{
using FileStream stream = new(
path,
FileMode.Append,
FileAccess.Write,
FileShare.Read);
using var writer = new StreamWriter(stream);
writer.WriteLine(line);
writer.Flush();
// Re-check inside the lock: another thread may have latched the
// writer off (or already ensured the directory) between the
// fast check above and taking the gate.
if (_latchedOff)
return;
try
{
EnsureDirectory(path);
string line = JsonSerializer.Serialize(value, JsonOptions);
using FileStream stream = new(
path,
FileMode.Append,
FileAccess.Write,
FileShare.Read);
using var writer = new StreamWriter(stream);
writer.WriteLine(line);
writer.Flush();
}
catch (Exception error) when (IsRecoverableIoFailure(error))
{
LatchOff(path, error);
}
}
}
private void EnsureDirectory(string path)
{
if (_directoryEnsured)
return;
string? directory = Path.GetDirectoryName(path);
if (!string.IsNullOrEmpty(directory))
Directory.CreateDirectory(directory);
_directoryEnsured = true;
}
private void LatchOff(string path, Exception error)
{
_latchedOff = true;
Console.Error.WriteLine(
$"[status-writer] disabling status stream at '{path}' after a "
+ $"write failure ({error.GetType().Name}: {error.Message}); no "
+ "further events for this session will be written.");
}
/// <summary>
/// The set of exceptions this class's own file I/O can plausibly raise
/// — a missing parent directory, a path segment colliding with an
/// existing file, permission failures, an unsupported path shape, or a
/// platform security restriction. Anything outside this set (e.g. an
/// <see cref="OutOfMemoryException"/>) is deliberately NOT caught —
/// this class only promises to survive ITS OWN recoverable I/O
/// failures, never to become a blanket exception sink.
/// </summary>
private static bool IsRecoverableIoFailure(Exception error) =>
error is IOException
or UnauthorizedAccessException
or NotSupportedException
or ArgumentException
or System.Security.SecurityException
or DirectoryNotFoundException;
}