Recs rebuild D2: persist the built RemediationAction (P2 reorder)#1059
Merged
Conversation
Dashboard-only. Lets the (future) Recommendations surface drive Apply + the two-sided consent gate from findings read back from storage, by persisting the BUILT RemediationAction (the artifact the builders need), mirroring the alert path's ContextJson. Lite is unchanged (advise/copy- paste only). P2 reorder (AnalysisService.AnalyzeAsync): mute-filter survivors -> enrich survivors -> build + attach each finding's RemediationAction -> batched insert with remediation_action_json. Muted/absolution findings are no longer enriched-then-discarded. The returned/enriched list still flows to AnalysisCompleted + NotifyAsync unchanged. D2: - AlertContextSerializer: add public SerializeAction/DeserializeAction wrapping the existing private ToDto/FromDto (identical round-trip to the alert path; null/try-catch on bad json). - AnalysisFinding: add ephemeral Remediation (built on write, deserialized on read; not a scored field). - config.analysis_findings: add remediation_action_json nvarchar(max) NULL to the canonical CREATE TABLE + an idempotent COL_LENGTH-guarded ALTER in EnsureTablesExistAsync (app-managed schema; not install/ or upgrades/). - SqlServerFindingStore: split SaveFindingsAsync into FilterMutedFindingsAsync (no insert) + InsertFindingsAsync (batched, persists the action json); GetRecentFindingsAsync selects + deserializes the column (field-count guarded so GetLatestFindingsAsync is unaffected). Tests (Dashboard.Tests/AnalysisNotificationTests.cs): RCSI + clear-plan action round-trips through the new seam (figures survive); null/garbage degrade to null; and the C1 gating test — a persisted RCSI action renders a non-null two-sided RiskDisclosure via GetForAction(action, finding:null) with the original inaction figures, proving the consent gate works from the stored action alone. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scope
Implements D2 (persist the built RemediationAction) + P2 (pipeline reorder) of the recommendations engine rebuild. Dashboard-only. Lite is unchanged (advise/copy-paste only; it renders advice from the fact-key and needs no persisted action). Does NOT build the Recommendations UI (WS1) or new config facts (WS3).
The (later) Recommendations surface must show Apply + the two-sided consent gate for findings read back from storage. The remediation builders REQUIRE
finding.DrillDown, butGetRecentFindingsAsyncreturns findings withDrillDown == null. So — exactly as the existing Alert path does withContextJson— we persist the BUILTRemediationActionon the finding row and deserialize it on read. (D7, already merged, ensures config/RCSI findings carry theirconfig_issuesdrill-down below severity 0.5 so their actions can be built.)File:line changelog
PerformanceMonitor.Notifications/AlertContext.cs(+36): added publicAlertContextSerializer.SerializeAction(RemediationAction?)(:169) andDeserializeAction(string?)(:184) that wrap the EXISTING privateToDto/FromDto(the proven round-trip forRcsiInactionFigures/ClearPlanFigures/all target lists).DeserializeActionis null/try-catch on bad json, mirroringTryDeserialize.PerformanceMonitor.Analysis/AnalysisModels.cs(+14):AnalysisFinding.Remediation(:126) — ephemeral likeDrillDown(built post-enrich on write, deserialized on read; not a scored field).RemediationActionis in the same assembly.Dashboard/Analysis/SqlServerFindingStore.cs(net +63):remediation_action_json nvarchar(max) NULLadded to the canonicalCREATE TABLE(:59) AND an idempotentIF COL_LENGTH(...) IS NULL ALTER TABLE ... ADDmigration inEnsureTablesExistAsync(:88-92). App-managed schema — nothing added toinstall/orupgrades/.SaveFindingsAsync→FilterMutedFindingsAsync(:106, mute-filter survivors, NO insert) +InsertFindingsAsync(:171, batched insert persisting the action json). Mute/dedup/ordering +_nextIdsequencing identical; both keep PR-1's single-connection + single-EnsureTablesExistAsyncdiscipline.InsertFindingAsync(:404-438): added the column/param; persistsAlertContextSerializer.SerializeAction(finding.Remediation).GetRecentFindingsAsyncSELECT (:216) +ReadFinding(:451): selects/deserializes ordinal 18, field-count-guarded (reader.FieldCount > 18) soGetLatestFindingsAsync(which does not select it) is unaffected.Dashboard/Analysis/AnalysisService.cs(net +35):AnalyzeAsyncreorder (:145-175) — filter survivors → enrich survivors → build+attachRemediationAction(tryBuildAction??BuildRcsiAction??BuildClearPlanAction) → batched insert. The returned/enriched (now action-bearing) list still flows toAnalysisCompleted+ (via the scheduler)NotifyAsyncunchanged.Dashboard/Services/AnalysisScheduler.cs(+1/-1): staleSaveFindingsAsynccomment reference updated (no behavior change; notify path verified intact at:193-198).Dashboard.Tests/AnalysisNotificationTests.cs(+93): 4 new tests (below).Tests (real results)
dotnet build PerformanceMonitor.sln -c Debug→ 0 errors (1 pre-existing unrelated CS0649 inRemediationTests.cs).Dashboard.Tests: 238 passed / 0 failed / 0 skipped.Lite.Tests: 360 passed / 0 failed / 0 skipped.--list-tests):SerializeAction_RcsiAction_RoundTripsFiguresAndTargets— RCSI action throughSerializeAction/DeserializeAction; assertsRcsiInactionFigures(BlockingEvents/Deadlocks/ReaderWriterPct) survive.SerializeAction_ClearPlanAction_RoundTripsFiguresAndTargets— clear-plan action; assertsClearPlanFigures(CpuPercent/AnomalyRatio) survive.SerializeAction_NullAction_SerializesAndDeserializesToNull— null/empty/garbage json → null, no throw.PersistedRcsiAction_RendersTwoSidedConsentGate_WithFindingNull(the C1 gating test) — synth RCSI finding (rcsi:false+ inaction enrichment) → serialize → deserialize →FactRiskDisclosure.GetForAction(action, finding: null)returns a non-null two-sidedRiskDisclosurewith BOTHRisksOfChangingandRisksOfNotChangingnon-empty AND the original inaction figures present ("12 blocked-process events", "3 deadlocks", "80%", "RCSI eliminates"). Proves the consent gate renders from the persisted action alone — exactly how the real Apply surface calls it.Needs integration verification (not unit-testable)
config.analysis_findingsis SQL Server; there is no in-memory harness for the store, so the following require a real DB and are NOT claimed as covered by unit tests:AnalyzeAsyncagainst a server with an RCSI-off / config finding; confirm a row is written with non-nullremediation_action_json, thenGetRecentFindingsAsyncreads it back withfinding.Remediation != nulland the consent gate renders.config.analysis_findingscreated before this change, confirmEnsureTablesExistAsyncadds the column once and is a no-op on subsequent runs (and that fresh-create DBs already have it, so the ALTER is skipped).AnalysisNotificationsEnabledon: confirmNotifyAsyncstill receives the enriched findings after the reorder.🤖 Generated with Claude Code