From 511ed70361dc8441da9dc3b3737ec3cb3aa2c117 Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Fri, 24 Apr 2026 15:37:26 +0200 Subject: [PATCH] v2 codex-post-#51: partial-skipUser guard regression + enum serialization parity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Codex second-pass on commit 51d57153: Fix #A (High, behavioral regression) — ShouldTransitionToAwaitingHello in DecisionEngine.Shared.cs was gated on Profile.EspConfig ∈ {DeviceEspOnly, NoEsp}, which requires BOTH skipUser + skipDevice observations before the derived enum leaves Unknown. A realistic bootstrap where only the skipUser half lands (skipDevice missing) therefore blocked the AwaitingHello promotion, while the pre-refactor `state.SkipUserEsp?.Value == true` guard would have unlocked it. The guard now reads the raw half-fact `ScenarioObservations.SkipUserEsp?.Value` directly, restoring exact legacy semantics. Profile.EspConfig keeps its "needs both halves" shape — it is a derived classification for other consumers, not a substitute for the raw observation here. Two new regression tests pin the fix: - EspConfigDetectedSkipUserTrue_onlyHalfPayload_stillUnblocksAwaitingHello - EspExiting_partialSkipUserTrue_armsHelloSafety Fix #B (Medium, serialization parity) — DecisionCoreJsonSettings did not register UnknownFallbackEnumConverter for the five new profile enums (EnrollmentMode, EnrollmentJoinMode, EspConfig, PreProvisioningSide, ProfileConfidence), so they round-tripped as numeric JSON with no cross-version fallback. Now registered alongside the other DecisionCore enums; fallbacks follow each enum's Unknown-equivalent value (None for PreProvisioningSide, Low for ProfileConfidence, Unknown for the rest). Six new SerializationTests verify string-form serialization, known-value round-trip, and unknown-value fallback for each of the five enums plus an end-to-end DecisionState round-trip through StateSerializer that asserts the profile dimensions persist as string literals, not integers. Suites: DecisionCore 211/211 (+8 new tests), Agent.V2.Core 650/650, Functions 735/735; 0 warnings. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../buildcounter.txt | 2 +- .../AutopilotMonitor.Agent/buildcounter.txt | 2 +- .../ClassicAwaitingHelloGuardTests.cs | 77 +++++++++++ .../SerializationTests.cs | 123 ++++++++++++++++++ .../Engine/DecisionEngine.Shared.cs | 22 ++-- .../Serialization/DecisionCoreJsonSettings.cs | 9 ++ 6 files changed, 222 insertions(+), 13 deletions(-) diff --git a/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt b/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt index cd307095..33a21f83 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt +++ b/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt @@ -1 +1 @@ -306 +308 diff --git a/src/Agent/AutopilotMonitor.Agent/buildcounter.txt b/src/Agent/AutopilotMonitor.Agent/buildcounter.txt index e29bf83e..fdab865f 100644 --- a/src/Agent/AutopilotMonitor.Agent/buildcounter.txt +++ b/src/Agent/AutopilotMonitor.Agent/buildcounter.txt @@ -1 +1 @@ -1143 +1144 diff --git a/src/Agent/AutopilotMonitor.DecisionCore.Tests/ClassicAwaitingHelloGuardTests.cs b/src/Agent/AutopilotMonitor.DecisionCore.Tests/ClassicAwaitingHelloGuardTests.cs index 7305ee72..5ccc0492 100644 --- a/src/Agent/AutopilotMonitor.DecisionCore.Tests/ClassicAwaitingHelloGuardTests.cs +++ b/src/Agent/AutopilotMonitor.DecisionCore.Tests/ClassicAwaitingHelloGuardTests.cs @@ -198,6 +198,83 @@ namespace AutopilotMonitor.DecisionCore.Tests Assert.Equal(SessionStage.AwaitingHello, afterFinalizing.NewState.Stage); } + [Fact] + public void EspConfigDetectedSkipUserTrue_onlyHalfPayload_stillUnblocksAwaitingHello() + { + // Codex second-pass Hoch (post-#51) — partial-payload regression guard. + // EspConfigDetected arrives with ONLY skipUser=true (skipDevice missing, e.g. + // registry key not yet present). Under the original legacy code this was sufficient + // to unlock AwaitingHello because the guard read SkipUserEsp?.Value directly. The + // derived Profile.EspConfig enum would stay Unknown (it needs BOTH halves), so the + // guard must NOT be gated on EspConfig. This test pins the behaviour: one half + + // FinalizingSetup → AwaitingHello. + var engine = new DecisionEngine(); + var seed = DecisionState.CreateInitial("s", "t") + .ToBuilder() + .WithStage(SessionStage.EspDeviceSetup) + .WithStepIndex(1) + .WithLastAppliedSignalOrdinal(0) + .Build(); + + var partial = new DecisionSignal( + sessionSignalOrdinal: 1, + sessionTraceOrdinal: 1, + kind: DecisionSignalKind.EspConfigDetected, + kindSchemaVersion: 1, + occurredAtUtc: Fixed, + sourceOrigin: "EnrollmentOrchestrator", + evidence: new Evidence(EvidenceKind.Raw, "esp_config_detected_bootstrap_partial", "SkipUser=true only"), + payload: new Dictionary + { + [SignalPayloadKeys.SkipUserEsp] = "true", + // NOTE: skipDevice key intentionally missing. + }); + var afterConfig = engine.Reduce(seed, partial).NewState; + + // Only the raw half-fact is set; derived EspConfig enum stays Unknown. + Assert.True(afterConfig.ScenarioObservations.SkipUserEsp!.Value); + Assert.Null(afterConfig.ScenarioObservations.SkipDeviceEsp); + Assert.Equal(EspConfig.Unknown, afterConfig.ScenarioProfile.EspConfig); + + // The guard MUST still unlock AwaitingHello — the semantic "Account-ESP is skipped" + // is fully known from SkipUser alone, regardless of skipDevice. + var finalizing = MakePhaseSignal(EnrollmentPhase.FinalizingSetup, ordinal: 2); + var afterFinalizing = engine.Reduce(afterConfig, finalizing); + Assert.Equal(SessionStage.AwaitingHello, afterFinalizing.NewState.Stage); + } + + [Fact] + public void EspExiting_partialSkipUserTrue_armsHelloSafety() + { + // Symmetric with EspExiting (the path that actually arms HelloSafety) — partial + // bootstrap payload must not block this path either. + var engine = new DecisionEngine(); + var seed = DecisionState.CreateInitial("s", "t") + .ToBuilder() + .WithStage(SessionStage.EspDeviceSetup) + .WithStepIndex(1) + .WithLastAppliedSignalOrdinal(0) + .Build(); + + var partial = new DecisionSignal( + sessionSignalOrdinal: 1, + sessionTraceOrdinal: 1, + kind: DecisionSignalKind.EspConfigDetected, + kindSchemaVersion: 1, + occurredAtUtc: Fixed, + sourceOrigin: "EnrollmentOrchestrator", + evidence: new Evidence(EvidenceKind.Raw, "esp_config_detected_bootstrap_partial", "SkipUser=true only"), + payload: new Dictionary + { + [SignalPayloadKeys.SkipUserEsp] = "true", + }); + var afterConfig = engine.Reduce(seed, partial).NewState; + var afterExit = engine.Reduce(afterConfig, MakeExitingSignal(ordinal: 2)); + + Assert.Equal(SessionStage.AwaitingHello, afterExit.NewState.Stage); + Assert.Contains(afterExit.NewState.Deadlines, d => d.Name == DeadlineNames.HelloSafety); + } + [Fact] public void Finalizing_beforeEspConfigDetected_staysInStage_evenIfRegistryWouldSaySkipUser() { diff --git a/src/Agent/AutopilotMonitor.DecisionCore.Tests/SerializationTests.cs b/src/Agent/AutopilotMonitor.DecisionCore.Tests/SerializationTests.cs index 142b9293..ba6e679d 100644 --- a/src/Agent/AutopilotMonitor.DecisionCore.Tests/SerializationTests.cs +++ b/src/Agent/AutopilotMonitor.DecisionCore.Tests/SerializationTests.cs @@ -245,5 +245,128 @@ namespace AutopilotMonitor.DecisionCore.Tests Assert.Equal(HypothesisLevel.Unknown, result); } + + // ================================================================ Codex follow-up #5 (post-#51) + // EnrollmentScenarioProfile enums — the five dimensions of the new profile aggregate + // must be registered in DecisionCoreJsonSettings just like every other DecisionCore + // enum, otherwise snapshots round-trip as numeric JSON and unknown cross-version + // values crash instead of landing on the Unknown-fallback. These tests pin the + // registration contract. + + [Fact] + public void UnknownFallbackEnumConverter_EnrollmentMode_roundtripsByName_andFallsBackOnUnknown() + { + var settings = DecisionCoreJsonSettings.Create(); + + var json = JsonConvert.SerializeObject(EnrollmentMode.WhiteGlove, settings); + Assert.Contains("WhiteGlove", json); + Assert.Equal(EnrollmentMode.WhiteGlove, JsonConvert.DeserializeObject(json, settings)); + + Assert.Equal(EnrollmentMode.Unknown, + JsonConvert.DeserializeObject("\"FutureMode\"", settings)); + } + + [Fact] + public void UnknownFallbackEnumConverter_EnrollmentJoinMode_roundtripsByName_andFallsBackOnUnknown() + { + var settings = DecisionCoreJsonSettings.Create(); + + var json = JsonConvert.SerializeObject(EnrollmentJoinMode.HybridAzureAdJoin, settings); + Assert.Contains("HybridAzureAdJoin", json); + Assert.Equal(EnrollmentJoinMode.HybridAzureAdJoin, + JsonConvert.DeserializeObject(json, settings)); + + Assert.Equal(EnrollmentJoinMode.Unknown, + JsonConvert.DeserializeObject("\"FutureJoinMode\"", settings)); + } + + [Fact] + public void UnknownFallbackEnumConverter_EspConfig_roundtripsByName_andFallsBackOnUnknown() + { + var settings = DecisionCoreJsonSettings.Create(); + + var json = JsonConvert.SerializeObject(EspConfig.DeviceEspOnly, settings); + Assert.Contains("DeviceEspOnly", json); + Assert.Equal(EspConfig.DeviceEspOnly, JsonConvert.DeserializeObject(json, settings)); + + Assert.Equal(EspConfig.Unknown, + JsonConvert.DeserializeObject("\"NewEspShape\"", settings)); + } + + [Fact] + public void UnknownFallbackEnumConverter_PreProvisioningSide_roundtripsByName_andFallsBackToNone() + { + var settings = DecisionCoreJsonSettings.Create(); + + var json = JsonConvert.SerializeObject(PreProvisioningSide.Technician, settings); + Assert.Contains("Technician", json); + Assert.Equal(PreProvisioningSide.Technician, + JsonConvert.DeserializeObject(json, settings)); + + // The fallback for PreProvisioningSide is None (there is no Unknown member). + Assert.Equal(PreProvisioningSide.None, + JsonConvert.DeserializeObject("\"FutureSide\"", settings)); + } + + [Fact] + public void UnknownFallbackEnumConverter_ProfileConfidence_roundtripsByName_andFallsBackToLow() + { + var settings = DecisionCoreJsonSettings.Create(); + + var json = JsonConvert.SerializeObject(ProfileConfidence.High, settings); + Assert.Contains("High", json); + Assert.Equal(ProfileConfidence.High, + JsonConvert.DeserializeObject(json, settings)); + + Assert.Equal(ProfileConfidence.Low, + JsonConvert.DeserializeObject("\"Unverified\"", settings)); + } + + [Fact] + public void ScenarioProfile_stateRoundtripsThroughStateSerializer_enumsAsStrings() + { + // End-to-end regression: the full DecisionState must serialize the new Profile + // dimensions as string literals (not integers) so cross-version reads stay robust. + var initial = DecisionState.CreateInitial("s", "t"); + var state = initial + .ToBuilder() + .Apply(b => b.ScenarioProfile = b.ScenarioProfile.With( + mode: EnrollmentMode.WhiteGlove, + joinMode: EnrollmentJoinMode.HybridAzureAdJoin, + espConfig: EspConfig.DeviceEspOnly, + preProvisioningSide: PreProvisioningSide.Technician, + confidence: ProfileConfidence.High, + evidenceOrdinal: 7, + reason: "test_round_trip")) + .Build(); + + var json = StateSerializer.Serialize(state); + + // Enums persist as their string names, not their underlying integer values. + Assert.Contains("\"Mode\":\"WhiteGlove\"", json); + Assert.Contains("\"JoinMode\":\"HybridAzureAdJoin\"", json); + Assert.Contains("\"EspConfig\":\"DeviceEspOnly\"", json); + Assert.Contains("\"PreProvisioningSide\":\"Technician\"", json); + Assert.Contains("\"Confidence\":\"High\"", json); + + var roundtripped = StateSerializer.Deserialize(json); + var p = roundtripped.ScenarioProfile; + Assert.Equal(EnrollmentMode.WhiteGlove, p.Mode); + Assert.Equal(EnrollmentJoinMode.HybridAzureAdJoin, p.JoinMode); + Assert.Equal(EspConfig.DeviceEspOnly, p.EspConfig); + Assert.Equal(PreProvisioningSide.Technician, p.PreProvisioningSide); + Assert.Equal(ProfileConfidence.High, p.Confidence); + Assert.Equal(7, p.EvidenceOrdinal); + Assert.Equal("test_round_trip", p.Reason); + } + } + + internal static class DecisionStateBuilderTestExtensions + { + public static DecisionStateBuilder Apply(this DecisionStateBuilder b, Action mutate) + { + mutate(b); + return b; + } } } diff --git a/src/Shared/AutopilotMonitor.DecisionCore/Engine/DecisionEngine.Shared.cs b/src/Shared/AutopilotMonitor.DecisionCore/Engine/DecisionEngine.Shared.cs index 25f74661..08eddec0 100644 --- a/src/Shared/AutopilotMonitor.DecisionCore/Engine/DecisionEngine.Shared.cs +++ b/src/Shared/AutopilotMonitor.DecisionCore/Engine/DecisionEngine.Shared.cs @@ -591,10 +591,9 @@ namespace AutopilotMonitor.DecisionCore.Engine /// on Classic V1 paths. Returns true only when the promotion is legitimate: /// /// AccountSetup has already been entered (the post-Account-ESP final exit case), or - /// The profile's resolves to - /// a shape where Account-ESP never runs ( - /// = skipUser=true, or = skipUser=true AND - /// skipDevice=true). In those shapes the first esp_exiting IS the final exit. + /// is observed as + /// true (Account-ESP phase is skipped; first esp_exiting IS the final + /// exit on device-only / full-skip flows). /// /// Otherwise returns false — a FinalizingSetup / EspExiting signal arriving /// before AccountSetup on a non-SkipUser enrollment is either a collector bug or the @@ -602,18 +601,19 @@ namespace AutopilotMonitor.DecisionCore.Engine /// Keeping this helper in the reducer means even a regression in Fix 7 cannot drive a /// premature AwaitingHello + HelloSafety arm. /// - /// Codex follow-up #5: the legacy SkipUserEsp?.Value == true check now reads - /// (derived enum) instead of the - /// half-fact. Semantically equivalent: DeviceEspOnly ≡ skipUser=true && skipDevice=false, - /// NoEsp ≡ skipUser=true && skipDevice=true. An profile - /// (EspConfigDetected not yet seen, or only one half parsed) blocks the promotion. + /// Codex follow-up #5 + post-#51 fix: the legacy state.SkipUserEsp?.Value == true + /// check now reads . + /// directly — NOT the derived enum. + /// The derived enum requires BOTH halves (skipUser + skipDevice) to be observed before + /// it leaves , so a partial bootstrap payload carrying + /// only skipUser=true would block the promotion indefinitely under the previous + /// pass. The raw half-fact mirrors the old SkipUserEsp?.Value behaviour exactly. /// /// private static bool ShouldTransitionToAwaitingHello(DecisionState state) { if (state.AccountSetupEnteredUtc != null) return true; - var esp = state.ScenarioProfile.EspConfig; - if (esp == EspConfig.DeviceEspOnly || esp == EspConfig.NoEsp) return true; + if (state.ScenarioObservations.SkipUserEsp?.Value == true) return true; return false; } diff --git a/src/Shared/AutopilotMonitor.DecisionCore/Serialization/DecisionCoreJsonSettings.cs b/src/Shared/AutopilotMonitor.DecisionCore/Serialization/DecisionCoreJsonSettings.cs index de7e539d..5dafb1f0 100644 --- a/src/Shared/AutopilotMonitor.DecisionCore/Serialization/DecisionCoreJsonSettings.cs +++ b/src/Shared/AutopilotMonitor.DecisionCore/Serialization/DecisionCoreJsonSettings.cs @@ -51,6 +51,15 @@ namespace AutopilotMonitor.DecisionCore.Serialization settings.Converters.Add(new UnknownFallbackEnumConverter(DecisionEffectKind.PersistSnapshot)); settings.Converters.Add(new UnknownFallbackEnumConverter(EnrollmentPhase.Unknown)); + // Codex follow-up #5 (post-#51) — EnrollmentScenarioProfile dimensions. Without + // these registrations the new enums would round-trip as numeric JSON with no + // unknown-value fallback, breaking drift-tolerated cross-version reads. + settings.Converters.Add(new UnknownFallbackEnumConverter(EnrollmentMode.Unknown)); + settings.Converters.Add(new UnknownFallbackEnumConverter(EnrollmentJoinMode.Unknown)); + settings.Converters.Add(new UnknownFallbackEnumConverter(EspConfig.Unknown)); + settings.Converters.Add(new UnknownFallbackEnumConverter(PreProvisioningSide.None)); + settings.Converters.Add(new UnknownFallbackEnumConverter(ProfileConfidence.Low)); + return settings; } }