From 118b4c0a414f1730ab2dcdd37bd14d29e6fcfbb7 Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Sun, 3 May 2026 21:40:10 +0200 Subject: [PATCH] test(v2): extract DeathRattlePrelude so PR3 gate is directly testable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review on PR3: the AgentRuntimeHost integration (gate + path build + pre-Start read) is the fragile part of the plan and was only tested indirectly through the helper + emitter paths. A regress like "TryReadRaw moved to after Start", "WhiteGlove resume not skipped" or "clean exit fires" would have slipped through. Pull the gate / path / read sequence into a dedicated DeathRattlePrelude in V2.Core so it can be exercised in isolation: - DeathRattlePrelude.TryCapture(stateDirectory, previousExitType, isWhiteGloveResume, logger) returns the prior snapshot or null; IsUncleanExit exposed for callers that want to log the decision. - AgentRuntimeHost reduced to a single TryCapture call; private IsUncleanExit + System.IO using removed. - DeathRattlePreludeTests (24 cases) cover: - Clean-exit table (clean / first_run / unknown / null) — all skip, even with snapshot present. - Unclean-exit table (reboot_kill / hard_kill / exception_crash, mixed casing) — all read. - WhiteGlove Part-2 resume gate wins over unclean exit. - Snapshot missing / corrupt → null, never throws. - Path-layout pin: prelude reads the same hardcoded "snapshot.json" filename the orchestrator writes to. - Non-mutation contract: byte-level equality before/after, file still loadable via SnapshotPersistence.Load(), no .quarantine directory created (orchestrator owns the quarantine decision). - Argument validation (null logger / null+empty stateDirectory). Test stand: V2.Core 983 (+24). --- .../Persistence/DeathRattlePreludeTests.cs | 331 ++++++++++++++++++ .../Persistence/DeathRattlePrelude.cs | 108 ++++++ .../Runtime/AgentRuntimeHost.cs | 63 +--- .../buildcounter.txt | 2 +- 4 files changed, 452 insertions(+), 52 deletions(-) create mode 100644 src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Persistence/DeathRattlePreludeTests.cs create mode 100644 src/Agent/AutopilotMonitor.Agent.V2.Core/Persistence/DeathRattlePrelude.cs diff --git a/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Persistence/DeathRattlePreludeTests.cs b/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Persistence/DeathRattlePreludeTests.cs new file mode 100644 index 00000000..37d88620 --- /dev/null +++ b/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Persistence/DeathRattlePreludeTests.cs @@ -0,0 +1,331 @@ +#nullable enable +using System; +using System.IO; +using AutopilotMonitor.Agent.V2.Core.Logging; +using AutopilotMonitor.Agent.V2.Core.Persistence; +using AutopilotMonitor.Agent.V2.Core.Tests.Harness; +using AutopilotMonitor.DecisionCore.Signals; +using AutopilotMonitor.DecisionCore.State; +using AutopilotMonitor.Shared.Models; +using Xunit; + +namespace AutopilotMonitor.Agent.V2.Core.Tests.Persistence +{ + /// + /// Tests for — the extracted gate + + /// path-build + snapshot-read that lives in front of orchestrator.Start() in + /// AgentRuntimeHost. These tests directly cover the three regress classes that the + /// host wiring is fragile against: + /// + /// TryReadRaw moved to AFTER Start — caught by the no-mutation contract test + /// (file must be readable bit-identically by a fresh SnapshotPersistence after the + /// prelude returns) and by the path-layout test (prelude reads the same hardcoded + /// "snapshot.json" the orchestrator writes to). + /// WhiteGlove Part-2 resume not skipped — caught by the WG-skip-table test. + /// clean / first_run exits firing — caught by the clean-exit-table test. + /// + /// + public sealed class DeathRattlePreludeTests + { + private static AgentLogger NewLogger(TempDirectory tmp) + => new AgentLogger(Path.Combine(tmp.Path, "logs"), AgentLogLevel.Debug); + + private static DecisionState BuildState(SessionStage stage = SessionStage.AwaitingHello, int stepIndex = 7) + { + var b = DecisionState.CreateInitial("S1", "T1").ToBuilder() + .WithStage(stage) + .WithStepIndex(stepIndex) + .WithLastAppliedSignalOrdinal(stepIndex); + b.DesktopArrivedUtc = new SignalFact( + new DateTime(2026, 5, 1, 13, 45, 37, DateTimeKind.Utc), + sourceSignalOrdinal: 28); + return b.Build(); + } + + private static string SaveSnapshot(string dir, DecisionState? state = null) + { + var path = Path.Combine(dir, DeathRattlePrelude.SnapshotFileName); + new SnapshotPersistence(path).Save(state ?? BuildState()); + return path; + } + + // ============================================================================ + // Clean-exit table — none of the planned exit classifications triggers + // ============================================================================ + + [Theory] + [InlineData("clean")] + [InlineData("first_run")] + [InlineData("CLEAN")] + [InlineData("First_Run")] + public void Returns_null_for_planned_exit_types_even_when_snapshot_present(string exitType) + { + using var tmp = new TempDirectory(); + SaveSnapshot(tmp.Path); + + var result = DeathRattlePrelude.TryCapture( + stateDirectory: tmp.Path, + previousExitType: exitType, + isWhiteGloveResume: false, + logger: NewLogger(tmp)); + + Assert.Null(result); + } + + [Fact] + public void Returns_null_for_null_previousExitType_with_snapshot_present() + { + using var tmp = new TempDirectory(); + SaveSnapshot(tmp.Path); + + var result = DeathRattlePrelude.TryCapture( + stateDirectory: tmp.Path, + previousExitType: null, + isWhiteGloveResume: false, + logger: NewLogger(tmp)); + + Assert.Null(result); + } + + [Fact] + public void Returns_null_for_unknown_exit_type_with_snapshot_present() + { + // Defensive: if Program.DetectPreviousExit ever produces an unrecognised + // value, the prelude must default to "no death-rattle" rather than + // misclassify it as unclean. Telemetry-noise-by-default is a worse failure + // mode than missed-attestation-on-edge-case. + using var tmp = new TempDirectory(); + SaveSnapshot(tmp.Path); + + var result = DeathRattlePrelude.TryCapture( + stateDirectory: tmp.Path, + previousExitType: "totally_made_up_type", + isWhiteGloveResume: false, + logger: NewLogger(tmp)); + + Assert.Null(result); + } + + // ============================================================================ + // Unclean-exit table — every classification on the allowlist triggers + // ============================================================================ + + [Theory] + [InlineData("reboot_kill")] + [InlineData("hard_kill")] + [InlineData("exception_crash")] + [InlineData("REBOOT_KILL")] // case-insensitive + [InlineData("Hard_Kill")] + [InlineData("Exception_Crash")] + public void Returns_state_for_unclean_exit_types_when_snapshot_present(string exitType) + { + using var tmp = new TempDirectory(); + SaveSnapshot(tmp.Path); + + var result = DeathRattlePrelude.TryCapture( + stateDirectory: tmp.Path, + previousExitType: exitType, + isWhiteGloveResume: false, + logger: NewLogger(tmp)); + + Assert.NotNull(result); + Assert.Equal(SessionStage.AwaitingHello, result!.Stage); + Assert.Equal(7, result.StepIndex); + } + + [Fact] + public void IsUncleanExit_pins_the_allowlist_explicitly() + { + // Public gate is also exposed for callers that want to log the decision + // independently. Kept covered so a future "let me also include X" change + // forces the test to be updated alongside the policy. + Assert.True(DeathRattlePrelude.IsUncleanExit("reboot_kill")); + Assert.True(DeathRattlePrelude.IsUncleanExit("hard_kill")); + Assert.True(DeathRattlePrelude.IsUncleanExit("exception_crash")); + + Assert.False(DeathRattlePrelude.IsUncleanExit("clean")); + Assert.False(DeathRattlePrelude.IsUncleanExit("first_run")); + Assert.False(DeathRattlePrelude.IsUncleanExit(null)); + Assert.False(DeathRattlePrelude.IsUncleanExit("")); + Assert.False(DeathRattlePrelude.IsUncleanExit("totally_made_up")); + } + + // ============================================================================ + // WhiteGlove-Resume gate — wins even on unclean exit + // ============================================================================ + + [Theory] + [InlineData("reboot_kill")] + [InlineData("hard_kill")] + [InlineData("exception_crash")] + public void Returns_null_when_isWhiteGloveResume_even_on_unclean_exit(string exitType) + { + // Plan §B: WhiteGlove Part-2 resume's "death" is the planned Part-1 sealing + // exit, which kicks the box through Sysprep + reboot. That's not an unclean + // death — emitting a death-rattle there would be telemetry noise. + using var tmp = new TempDirectory(); + SaveSnapshot(tmp.Path); + + var result = DeathRattlePrelude.TryCapture( + stateDirectory: tmp.Path, + previousExitType: exitType, + isWhiteGloveResume: true, + logger: NewLogger(tmp)); + + Assert.Null(result); + } + + // ============================================================================ + // Snapshot-availability cases — missing / corrupt → null, never throws + // ============================================================================ + + [Fact] + public void Returns_null_when_snapshot_file_missing() + { + using var tmp = new TempDirectory(); + // No SaveSnapshot call. + + var result = DeathRattlePrelude.TryCapture( + stateDirectory: tmp.Path, + previousExitType: "reboot_kill", + isWhiteGloveResume: false, + logger: NewLogger(tmp)); + + Assert.Null(result); + } + + [Fact] + public void Returns_null_when_snapshot_corrupt_and_does_not_throw() + { + using var tmp = new TempDirectory(); + File.WriteAllText( + Path.Combine(tmp.Path, DeathRattlePrelude.SnapshotFileName), + ""); + + var result = DeathRattlePrelude.TryCapture( + stateDirectory: tmp.Path, + previousExitType: "reboot_kill", + isWhiteGloveResume: false, + logger: NewLogger(tmp)); + + Assert.Null(result); + } + + // ============================================================================ + // Path-layout contract — must match orchestrator's hardcoded path + // ============================================================================ + + [Fact] + public void Reads_from_path_layout_matching_orchestrator() + { + // EnrollmentOrchestrator hardcodes Path.Combine(_stateDirectory, + // "snapshot.json") at line 309. If anyone renames the file or moves it under + // a subfolder on either side without updating the other, the death-rattle + // would silently miss every snapshot. This test pins the exact filename + // contract — a change of the literal "snapshot.json" requires the test to + // be updated alongside both producers. + using var tmp = new TempDirectory(); + var expectedPath = Path.Combine(tmp.Path, "snapshot.json"); + new SnapshotPersistence(expectedPath).Save(BuildState(stepIndex: 99)); + + var result = DeathRattlePrelude.TryCapture( + stateDirectory: tmp.Path, + previousExitType: "reboot_kill", + isWhiteGloveResume: false, + logger: NewLogger(tmp)); + + Assert.NotNull(result); + Assert.Equal(99, result!.StepIndex); + + // Belt + suspenders: the file at the literal path is what we read. + Assert.True(File.Exists(expectedPath)); + } + + // ============================================================================ + // Non-mutation contract — orchestrator.Start() must see the snapshot intact + // ============================================================================ + + [Fact] + public void Does_NOT_modify_or_quarantine_the_snapshot_file_on_success() + { + // Critical for ordering: orchestrator.Start runs Snapshot.Load() right after + // the prelude. If the prelude moves / overwrites / quarantines the file, + // Start would see no snapshot and start from scratch — losing all recovery + // state. Pin via byte-level equality + post-prelude Load() round-trip. + using var tmp = new TempDirectory(); + var path = SaveSnapshot(tmp.Path); + var bytesBefore = File.ReadAllBytes(path); + + var result = DeathRattlePrelude.TryCapture( + stateDirectory: tmp.Path, + previousExitType: "reboot_kill", + isWhiteGloveResume: false, + logger: NewLogger(tmp)); + + Assert.NotNull(result); + var bytesAfter = File.ReadAllBytes(path); + Assert.Equal(bytesBefore, bytesAfter); + + // The file is also still loadable through the orchestrator-style instance API. + var viaInstance = new SnapshotPersistence(path).Load(); + Assert.NotNull(viaInstance); + Assert.Equal(result!.Stage, viaInstance!.Stage); + Assert.Equal(result.StepIndex, viaInstance.StepIndex); + + // No quarantine directory has been created either. + Assert.False(Directory.Exists(Path.Combine(tmp.Path, ".quarantine"))); + } + + [Fact] + public void Does_NOT_modify_or_quarantine_the_snapshot_file_on_corruption() + { + // Even when TryReadRaw fails the prelude must NEVER touch the file — + // orchestrator.Start owns the quarantine decision. If the prelude + // quarantined corrupt snapshots, Start would see a missing file and + // skip its own quarantine path → loss of forensic evidence. + using var tmp = new TempDirectory(); + var path = Path.Combine(tmp.Path, DeathRattlePrelude.SnapshotFileName); + File.WriteAllText(path, ""); + var bytesBefore = File.ReadAllBytes(path); + + var result = DeathRattlePrelude.TryCapture( + stateDirectory: tmp.Path, + previousExitType: "reboot_kill", + isWhiteGloveResume: false, + logger: NewLogger(tmp)); + + Assert.Null(result); + Assert.True(File.Exists(path)); + Assert.Equal(bytesBefore, File.ReadAllBytes(path)); + Assert.False(Directory.Exists(Path.Combine(tmp.Path, ".quarantine"))); + } + + // ============================================================================ + // Argument validation + // ============================================================================ + + [Fact] + public void Throws_for_null_logger() + { + using var tmp = new TempDirectory(); + Assert.Throws(() => DeathRattlePrelude.TryCapture( + stateDirectory: tmp.Path, + previousExitType: "reboot_kill", + isWhiteGloveResume: false, + logger: null!)); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + public void Throws_for_null_or_empty_stateDirectory(string? stateDirectory) + { + using var tmp = new TempDirectory(); + Assert.Throws(() => DeathRattlePrelude.TryCapture( + stateDirectory: stateDirectory!, + previousExitType: "reboot_kill", + isWhiteGloveResume: false, + logger: NewLogger(tmp))); + } + } +} diff --git a/src/Agent/AutopilotMonitor.Agent.V2.Core/Persistence/DeathRattlePrelude.cs b/src/Agent/AutopilotMonitor.Agent.V2.Core/Persistence/DeathRattlePrelude.cs new file mode 100644 index 00000000..13efcac8 --- /dev/null +++ b/src/Agent/AutopilotMonitor.Agent.V2.Core/Persistence/DeathRattlePrelude.cs @@ -0,0 +1,108 @@ +#nullable enable +using System; +using System.IO; +using AutopilotMonitor.Agent.V2.Core.Logging; +using AutopilotMonitor.DecisionCore.State; + +namespace AutopilotMonitor.Agent.V2.Core.Persistence +{ + /// + /// Death-Rattle prelude (Plan §B — Edge-Triggered State Snapshots, 2026-05-03). + /// Captures the prior agent run's last persisted BEFORE the + /// new run's EnrollmentOrchestrator.Start() runs the recovery pipeline. Returns + /// null when the previous exit was clean, when this is a planned WhiteGlove + /// Part-2 resume, when the snapshot is missing or corrupt, or when any I/O error + /// occurs — the caller emits the prior_run_died_with_state event only when + /// this returns non-null. + /// + /// Extracted from AgentRuntimeHost so the gate logic + ordering contract are + /// directly testable. Three regress classes the host wiring is fragile against: + /// + /// + /// TryReadRaw moves to after orchestrator.Start() → snapshot + /// either overwritten by the first reducer save or quarantined → we read the wrong + /// state. Pin via the "no-mutation" contract test. + /// WhiteGlove Part-2 resume no longer skipped → false-positive death-rattle + /// for a planned exit. Pin via the WG-skip test. + /// clean / first_run exits start emitting → telemetry noise on + /// every successful previous run. Pin via the clean-exit-table test. + /// + /// + public static class DeathRattlePrelude + { + // The orchestrator hardcodes this exact filename inside _stateDirectory + // (EnrollmentOrchestrator.cs:309 — `Path.Combine(_stateDirectory, "snapshot.json")`). + // Any rename on either side without updating both is a bit-level path-layout drift + // that DeathRattlePreludeTests.Reads_from_path_layout_matching_orchestrator catches. + internal const string SnapshotFileName = "snapshot.json"; + + /// + /// Returns the prior run's persisted snapshot if all gates pass, else null. + /// + /// + /// Same directory the orchestrator's SnapshotPersistence reads from / writes + /// to. Must be non-empty. + /// + /// + /// One of first_run / clean / exception_crash / hard_kill + /// / reboot_kill as classified by Program.DetectPreviousExit. + /// Case-insensitive on principle. + /// + /// + /// true when this run is the Part-2 continuation of a sealed WhiteGlove flow. + /// The "death" of the Part-1 run is a planned reboot, not an unclean exit, so no + /// attestation is warranted. + /// + /// Mandatory — used for telemetry-friendly Info / Debug / Warning lines. + public static DecisionState? TryCapture( + string stateDirectory, + string? previousExitType, + bool isWhiteGloveResume, + AgentLogger logger) + { + if (logger == null) throw new ArgumentNullException(nameof(logger)); + if (string.IsNullOrEmpty(stateDirectory)) + throw new ArgumentException("stateDirectory is mandatory.", nameof(stateDirectory)); + + if (isWhiteGloveResume) return null; + if (!IsUncleanExit(previousExitType)) return null; + + try + { + var snapshotPath = Path.Combine(stateDirectory, SnapshotFileName); + var prior = SnapshotPersistence.TryReadRaw(snapshotPath); + if (prior != null) + { + logger.Info( + $"Death-rattle: prior snapshot loaded (Stage={prior.Stage}, " + + $"StepIndex={prior.StepIndex}, exitType={previousExitType})."); + } + else + { + logger.Debug( + $"Death-rattle: no prior snapshot to attest (path missing or unreadable, exitType={previousExitType})."); + } + return prior; + } + catch (Exception ex) + { + logger.Warning($"Death-rattle: prior snapshot read failed: {ex.Message}"); + return null; + } + } + + /// + /// Gate: which previous-exit classifications warrant a death-rattle. The set is + /// intentionally small — first_run / clean are planned exits with no + /// state worth re-attesting. Case-insensitive on principle to be robust against any + /// future capitalization drift in the producer. + /// + public static bool IsUncleanExit(string? exitType) + { + return exitType != null + && (StringComparer.OrdinalIgnoreCase.Equals(exitType, "reboot_kill") + || StringComparer.OrdinalIgnoreCase.Equals(exitType, "hard_kill") + || StringComparer.OrdinalIgnoreCase.Equals(exitType, "exception_crash")); + } + } +} diff --git a/src/Agent/AutopilotMonitor.Agent.V2/Runtime/AgentRuntimeHost.cs b/src/Agent/AutopilotMonitor.Agent.V2/Runtime/AgentRuntimeHost.cs index 9b16fa39..9185bbd4 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2/Runtime/AgentRuntimeHost.cs +++ b/src/Agent/AutopilotMonitor.Agent.V2/Runtime/AgentRuntimeHost.cs @@ -1,5 +1,4 @@ using System; -using System.IO; using System.Threading; using System.Threading.Tasks; using AutopilotMonitor.Agent.V2.Core.Configuration; @@ -201,41 +200,18 @@ namespace AutopilotMonitor.Agent.V2.Runtime logger: logger); auth.AuthFailureTracker.ThresholdExceeded += authThresholdHandler; - // Death-Rattle (Plan §B, 2026-05-03): if the previous run died unclean - // AND its persisted snapshot is still readable, capture it as an - // in-memory draft BEFORE orchestrator.Start runs. Start triggers the - // recovery pipeline which may quarantine the snapshot or have the first - // reducer save overwrite it; reading "what the dead run last knew" - // through the orchestrator-owned SnapshotPersistence after Start would - // either return the post-Start state (wrong) or null. The static - // TryReadRaw is quarantine-free and shares the parse path with Load(), - // so failure modes match. Skip on WhiteGlove Part-2 resume — the - // "dying" run there is the Part-1 sealing run, which is a planned - // exit, not a death. - DecisionState priorStateForDeathRattle = null; - if (!isWhiteGloveResume && IsUncleanExit(previousExit?.ExitType)) - { - try - { - var snapshotPath = Path.Combine(stateSubdir, "snapshot.json"); - priorStateForDeathRattle = SnapshotPersistence.TryReadRaw(snapshotPath); - if (priorStateForDeathRattle != null) - { - logger.Info( - $"Death-rattle: prior snapshot loaded (Stage={priorStateForDeathRattle.Stage}, " + - $"StepIndex={priorStateForDeathRattle.StepIndex}, exitType={previousExit?.ExitType})."); - } - else - { - logger.Debug( - $"Death-rattle: no prior snapshot to attest (path missing or unreadable, exitType={previousExit?.ExitType})."); - } - } - catch (Exception ex) - { - logger.Warning($"Death-rattle: prior snapshot read failed: {ex.Message}"); - } - } + // Death-Rattle (Plan §B, 2026-05-03): capture the prior run's last + // persisted snapshot BEFORE orchestrator.Start runs. Start triggers + // the recovery pipeline which may quarantine the snapshot or have the + // first reducer save overwrite it; reading after Start would either + // return the post-Start state (wrong) or null. The gate + path + read + // live in DeathRattlePrelude so they are directly testable in + // isolation — see DeathRattlePreludeTests. + DecisionState priorStateForDeathRattle = DeathRattlePrelude.TryCapture( + stateDirectory: stateSubdir, + previousExitType: previousExit?.ExitType, + isWhiteGloveResume: isWhiteGloveResume, + logger: logger); try { @@ -525,20 +501,5 @@ namespace AutopilotMonitor.Agent.V2.Runtime return requested; } - /// - /// Death-Rattle gate (Plan §B, 2026-05-03). The classifications produced by - /// are: first_run / clean - /// (planned exits — no death-rattle), exception_crash / hard_kill - /// / reboot_kill (unclean — the prior run probably had un-reported state - /// worth attesting). Case-insensitive on principle to be robust against any - /// future capitalization drift in the producer. - /// - private static bool IsUncleanExit(string exitType) - { - return exitType != null - && (StringComparer.OrdinalIgnoreCase.Equals(exitType, "reboot_kill") - || StringComparer.OrdinalIgnoreCase.Equals(exitType, "hard_kill") - || StringComparer.OrdinalIgnoreCase.Equals(exitType, "exception_crash")); - } } } diff --git a/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt b/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt index f5e47edc..3749383d 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt +++ b/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt @@ -1 +1 @@ -552 +555