Add the remediation credential seam, the journal's actor and the evict-then-observe state machine (#2138 phase 1) - #3170
Conversation
…ase 1) The monitoring credential stays read-only, so an action travels on a second, per-server, opt-in remediation credential or it does not travel. V113 adds remediation_username / remediation_encrypted_password to the registry, both nullable with no default and no fallback: a null credential is the shipped state of every server and means that server has no phase-1 surface at all. OperatorRemediationGate.SurfaceFor returns a NULLABLE surface rather than one carrying an enabled flag, so the absence is unrenderable instead of being a disabled control that explains itself. It reads Eligible AND Blockers off the existing structured_remediation verdict object and asks no evidence question of its own. V113 also adds collect.plan_force_actions.actor. GetPendingReviewsAsync' own- forces-only property was structural only while the bot was the table's only writer; an operator writing to it makes the bot's self-review able to find and unforce an operator's force. The read is now filtered on actor = 'bot', and the record member is required so the compiler enumerates construction sites.
…nvariant The gate's pins go at the two properties that are easy to lose: absence is expressed as a nullable return (read through NullabilityInfoContext, so a change to a non-nullable return carrying an enabled flag fails), and no capability shape can withhold evict-first without a named reason (exhaustive over all six). The flow's pins separate the two window limbs by the evidence each can support: the regressed plan returning is judged on identity and so is valid on the timeout limb, while a cost verdict is not. The executions floor is driven off ForcePlanBotSettings rather than a literal, and raising the setting has to move the boundary — so a restated 25 fails. The own-forces-only test journals an operator force shaped exactly like the bot force the read returns, alongside a bot-actored twin: without the twin, a filter matching nothing would satisfy the first assertion perfectly. The Lite divergence is asserted rather than inherited. Its first form scanned for "remediation" and found 12 correct hits, because rendering advice is a Lite feature; it now names the capability tokens instead, and checks the advise-only scope statements it rests on still exist.
V113 accepts a per-server remediation credential and nothing in this build consumes it, so an operator who entered one would believe the server was armed. A knob that silently does nothing is at its worst when what it claims to gate is a write to a production server, so the credential is accepted, stored, resolvable, and announced as inert at connect — the same discipline #2745 applied by journaling an all-gates-open force as WITHHELD rather than downgrading it. A one-sided credential gets its own warning: "you configured half of one" and "this build cannot use it yet" send an operator to different places. darling.sample.json documents both keys with the grants each lever needs, that ALTER SERVER STATE is genuinely optional, and that the service probes for it rather than guessing per platform.
Review summaryReviewed the diff against Checked in detail: the V113 migration ( One finding — stale doc comment (not inline-commentable; outside the diff hunk)
This line was left unchanged by the PR, but it's exactly the claim the PR invalidates: V113 makes the operator a second writer to Everything else looked solid — no other correctness, parity, security, or performance issues found. |
…V113 Five sites CI found, four of them the ratchets a new rung always trips and one of them the rung working as designed. The retention end-to-end's raw INSERT into plan_force_actions omitted actor and raised 23502 against a live store. That is the dropped DEFAULT doing its job: an INSERT that forgets the actor must fail rather than silently claim to be the bot, because a bot row is the kind the self-review may unforce. V112's two suites asserted they were the top rung, which V113 makes false. That claim belongs to whichever rung actually is top, or every new rung breaks every older rung's suite, so it moved here: the all-sentinels-true mapping, the last-ordinal equality, and the one-rung-behind check. V112 keeps its own ordinal and now switches off every LATER sentinel for its behind-check, which generalises. The Lite divergence pin's schema half was over-broad for the second time in one file: plan_correction carries a generated last_good_plan_force_failure_reason column, so a statement-substring scan reported the monitored server's own Query Store forcing-failure reason as a bot journal. It now parses table NAMES, with plan_correction as the control proving the parse discriminates them.
|
Reviewed the full diff (credential seam, Correctness: Traced every new positional-parameter INSERT/SELECT in Security: All new SQL is parameterized ( Lite/Darling parity: V113 is Darling-only by design (write path is deliberately unimplemented in this PR), and that's backed by Style: New T-SQL (migration DDL, the probe SELECT) follows CONTRIBUTING.md conventions (uppercase keywords, No correctness, security, or parity issues found. One purely-cosmetic observation, not a blocker: in |
…t the rule is The literal exclusion list is the mechanism, not an obstacle: an exclusion is only legitimate with an argument, and the argument sits in the comment beside the key, so growing the list has to appear in a diff. V113's two keys make it four. Bumping a literal by reflex is how a guard like this stops meaning anything, so the property behind it is now asserted too: only a credential-shaped key may be excluded. Excluding trustServerCertificate — the field #2552 actually reported — would fail that, where a bumped literal alone would have let it through.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Reviewed this against CONTRIBUTING.md's style rules and the Lite/Darling parity requirement. Summary: Scope. This is scaffolding only, matching the PR description: the remediation credential seam, the V113 migration, the journal's Correctness. Traced the V113 migration's parameter binding in One doc/comment inconsistency — left as an inline comment on Lite/Darling parity. This is Darling-only by design, and unusually for a cross-SKU change, the asymmetry is enforced rather than just described: Security. No injection surface — the new capability probe ( Style. The V113 Postgres migration schema-qualifies every object ( |
Review catch, and a correct one: the method's summary still said the property held because the read starts from rows the bot journaled. That was true while the bot was the table's only writer and V113 makes it false, so the summary contradicted the SQL comment a few lines below it. A stale WHY-comment is worse here than none — it invites removing the actor filter for looking redundant, which is exactly the guarantee it now carries alone.
Review: #2138 phase 1 (remediation credential seam + journal actor)Went through all 27 changed files (the V113 migration, the credential seam, Migration ladder (V113) — all four required parts are present and correct: rung registered at Positional parameter binding — Own-forces-only invariant — Gate/flow logic ( No-write-path guarantee — checked that none of the new doc comments ( ACL/provisioning parity — Lite divergence — the new pin test justifies the Lite/Darling asymmetry explicitly (advise-only scope statements in One trivial nit, not worth blocking on: |
ReviewWent through the full diff (all 6 commits, ~3000 lines) against CONTRIBUTING.md's T-SQL style rules, Lite/Darling parity requirements, and general correctness/security. Scope actually shipped, confirmed against the description: the credential seam (remediation_username/remediation_encrypted_password, V113), collect.plan_force_actions.actor plus the actor-filtered GetPendingReviewsAsync read, and the pure OperatorRemediationFlow/OperatorRemediationGate state machine. Confirmed there is genuinely no production caller wired to either new class yet (grep for OperatorRemediationFlow./OperatorRemediationGate. outside Tests turns up only XML-doc cross-references) — this really is seam-and-logic-only, matching the 'what is not in this branch' section. Correctness
Lite/Darling parity
Security
Style
No blocking findings. One open design question posted inline on OperatorRemediationFlow.cs. |
…cluded Review asked whether the identity arm waiting for a limb to close was deliberate. It was not: the window guard went first as the natural "don't decide yet" and the hash-already-matches case was never considered. Nothing pinned it either way, so it read as an open question. It is now a decision, and the behaviour is unchanged. The wait is right, but for a reason the class did not give. FREEPROCCACHE evicts the plan CACHE and Query Store keeps its plan row, so the regressed hash is present the instant after the eviction and stays present. ActivePlanHash only becomes evidence once post-eviction executions have been attributed to a plan, so an identity arm running while the window is open would not catch an early recompile — it would read the pre-eviction plan and offer a force on it, on the first call, every time. Query Store's default 900-second flush interval is also why the elapsed limb's 30 minutes is an order of magnitude rather than a round number. Second reason, independent of the instrument: OptimizerRecovered needs the executions floor, so firing identity earlier would make the force the quick answer and "nothing here needs pinning" the slow one. Wrong asymmetry for a lever whose premise is that the cheapest fix pins nothing. The class's own "one compile settles it" was the overstatement that made this look accidental — true of the optimizer, false of the instrument. Corrected here along with ActivePlanHash's contract, whose old wording invited deriving the value from plan-row presence, which after an eviction is always the regressed hash.
|
Reviewed the diff (production code + migration + tests) against CONTRIBUTING.md's T-SQL/C# style rules, Lite/Darling parity, and the credential-handling security surface. This is a Darling-only, no-write-path PR (correctly — Checked and correct:
One gap (left as an inline comment on No correctness bugs, no injection surface (all params bound, migration SQL has no interpolated identifiers), no missing-index-DMV suggestions here. The pure |
Per the ruling on #2138: remediation execution is Darling-only, permanently. The rule itself does not change — what changes is which of its two stated reasons carries it. The obsolete half named Dashboard as the owner of remediation execution, which points at nothing live. The half that decides it is Lite's store: a DuckDB file is per-workstation, so a remediation journal kept there cannot be the shared audit trail such a journal exists to be — two operators acting on one server would each hold half of it and neither could see the other's forces — and it cannot carry the bot's own-forces-only invariant, which is a predicate plus a self-reference within ONE shared store. No deprecation can invalidate that. All three sites that state the rule now state that reason; the two that carried the dead one no longer do. The divergence pin is strengthened rather than relaxed: it keeps every rule phrase it already asserted, adds the durable reason in all three files, and adds that the dead reason is absent. The #2138 wording fix is the case that shows why rule and reason need separate assertions — the rule never changed and one of its reasons died, so a pin checking only the rule would have gone on passing over a justification naming a deprecated SKU as the owner of a capability. The Dashboard mentions in LiteRecommendationsViewModel and RecommendationsTab stay. They are UI comparisons about an "Open in Active Queries" deep-link, not scope claims, and their reason is the surface's tab shape, which the deprecation does not touch. They now also serve as the positive control proving the dead-reason absence assertions are not passing against a failed read.
|
Reviewed the diff against CONTRIBUTING.md conventions, Lite/Darling parity, and the credential/security surface. No correctness, security, or parity bugs found. Notes from the pass: Credential seam ( Migration V113 ( Parameter/ordinal bookkeeping — hand-checked the own-forces-only invariant — Role grants ( Lite/Darling parity — no Lite production code gained a credential, executor, or journal table (correctly, per the PR's stated Lite-is-permanently-advise-only decision). The Lite doc-comment updates in State machine ( Nothing here rises to a blocking finding — this is a clean, thoroughly self-tested PR. |
Review catch. Darling/README.md's per-server table lists every other key in darling.json with its default and notes, and CONTRIBUTING names that README as the operator-facing configuration document — so two keys that are real, settable config today were undiscoverable from it. Both rows added, plus a paragraph for the part that does not fit a table cell: the grants each lever needs, that ALTER SERVER STATE is genuinely optional because evict-first degrades to force-only with a named reason, and that the service asks the server with a read-only has_perms_by_name check rather than guessing per platform. And a note that this release ships no consumer of the credential, so an operator provisioning ahead of the write path knows the arming is inert for now rather than discovering it silently does nothing.
Review: #2138 phase 1 (remediation credential seam + journal actor)Went through all 9 commits/diff hunks (credential seam,
No correctness, security, or Lite/Darling parity issues found. T-SQL style ( Nothing to flag as a blocking or non-blocking finding. |
|
Reviewed the diff (production code, migration, and tests) against CONTRIBUTING.md's Postgres/T-SQL conventions, Lite/Darling parity, security, and correctness. No blocking issues found. This is an unusually well-instrumented change:
Left one non-blocking inline nit on |
…Postgres ResolveConnectionString runs on both engine paths, so the inert-credential notice fired for PostgreSQL targets too and told the operator nothing would use it YET. On Postgres it is inert permanently: BuildRemediationConnectionString throws for that engine and plan-force remediation forces a Query Store plan. One message for both promised a future that engine does not have.
|
Reviewed the full diff (production code + tests) against the correctness / Lite-Darling-parity / security / performance / T-SQL-style checklist. Summary: no blocking issues found. This is an unusually disciplined change — worth calling out what makes it hold up rather than just saying "LGTM":
Didn't flag any inline comments — everything I traced through (parameter ordering in the |
1 participant
The credential seam, the journal's actor, and the shared evict-then-observe state machine for #2138 phase 1. This branch cannot write to a monitored server —
PlanForceNoWritePathTestsstays green, verified by scanning the built service assembly with a working positive control, not by reading the diff.What #2138's design asked for, and what
devalready hadThe design pass (#2138, 2026-08-18) specifies phase 1 as four pieces: the remediation credential seam, a
remediation_actionsjournal in both stores, the evict/observe/force flow, and a viewer surface. Two of its premises have moved since it was written.The journal already exists. #2745 (merged 2026-09-01) built it as
collect.plan_force_actionsat V107, with the design's exact column list — server, database, query/plan keys, evidence snapshot, outcome — and its doc comment calls it "the bot's audit trail and ledger". Adding a secondremediation_actionstable beside it would be the table-shaped version of the second policy path this design exists to avoid, so this branch extends that one instead. The design's own words are "built once, in Phase 1"; it was, under a different name.The bot already consumes it in dry run. #2745 also landed
ForcePlanBotPolicy,ForcePlanSelfReview, the cooldowns and the would-force ledger — design phase 2's brain — withIPlanForceExecutordeclared and unimplemented. #2731, which would have armed it, was closed on 2026-09-07 with the ruling that the write path is "genuinely outstanding and deliberately fenced" and phase 2 "should be rewritten againstdev's seam".The credential seam
V113addsremediation_username/remediation_encrypted_passwordtoconfig.config_monitored_servers, besideusername/encrypted_password: same DPAPI-LocalMachine blob, sameenv:/file:reference support, same--encrypt-password. Both nullable, no default, no fallback to the monitoring credential — that credential stays read-only forever, which is a promise both READMEs and the MCP instructions make and operators grant against.Presence of the username is the auth mode. There is deliberately no
remediationAuthsibling: an integrated remediation identity would be the service account, which is the monitoring identity, which is the thing this exists to keep read-only. And no plaintext slot:Password's dev-convenience arm exists because a wrong monitoring password fails a read, and a wrong remediation password fails a write against production.ResolveRemediationPasswordreturns null for an unarmed server rather than throwing, unlike its monitoring twin. A missing monitoring password is a misconfiguration; a missing remediation password is the shipped state of every server, and an exception in the normal case is one callers learn to swallow.BuildRemediationConnectionStringis a separate function rather than a flag on the existing builder — auseRemediationCredential: trueparameter would put the write identity one mistyped argument away from every collector. It presentsApplicationName = "PerformanceMonitorDarling-Remediation", which is the audit trail on the server's side: a DBA readingsys.dm_exec_sessionsduring an incident has to be able to tell the one connection that can change a plan from the forty that cannot.How a server without one has no surface, rather than a disabled one
OperatorRemediationGate.SurfaceForreturnsOperatorRemediationSurface?and returns null. The nullability is the mechanism, not a convention: a record carryingEnabled = falseis oneIsEnabledbinding away from a greyed-out button with a tooltip, whereas a view-model holding null has nothing to bind a command to and nothing to draw.TheSurfaceIsExpressedAsANullableReturn_NotAnEnabledFlagreads the return's nullability throughNullabilityInfoContextand rejects anyEnabled/Disabled/Visiblemember on the surface type, so the shape that makes a disabled control easy to write fails in CI.It executes the existing verdict object
The gate takes a
StructuredForcePlanTargetand readsEligibleandBlockers— the fieldsFactRemediation.BuildStructuredRemediationfills fromFactRemediation.ForcePlanBlockers, the same output an MCP consumer reads and the same functionPlanForceBotalready passes through. Nothing in the gate looks atParameterSensitivityCoFired,ReplicaRole, or any other evidence field.It reads both halves, and disagreement refuses. On any object the projection produced they agree (
Eligibleis defined asblockers.Count == 0) and the second read is free; on a hand-built or deserialized target it is not, and reading one alone would arm a blocked target on a flag.AVerdictWhoseHalvesDisagree_GetsNoSurfacepins both directions.The PSP never-auto-force contract arrives through the projection rather than being restated:
AParameterSensitiveTarget_GetsNoSurface_EvenFullyArmedbuilds a realForcePlanTarget, projects it, asserts the verdict really carriesparameter_sensitivity_cofired(so the case cannot pass for the wrong reason), and then asserts no surface.The journal's actor, and the own-forces-only invariant
V113addscollect.plan_force_actions.actor.GetPendingReviewsAsync' own-forces-only property was documented as structural because "the read starts from rows this bot journaled" — true only while the bot was the table's only writer. An operator writing to the same table makes the bot's self-review able to find an operator's force, judge it against evidence it never saw, and take it back, breaking the standing house rule in the direction nobody notices until a hand-pinned plan quietly stops being pinned. The read is nowAND pfa.actor = 'bot'.This is the shape phase 2's own-forces-only rule needs. Phase 2's unforce cannot reach a row it cannot join to a bot-actored
forcerow: the review read filters on the actor, and the terminal-row check joins onrelated_action_id, which is the table's own identity PK. So "only forces this bot placed" is two predicates on one table rather than a rule someone has to remember — the foreign-key-shaped invariant the design asked for, available before the bot can place anything.The DEFAULT is added and then dropped. Every existing row was written by the bot, so
DEFAULT 'bot'backfills them correctly; leaving it would make an INSERT that forgetsactorsilently claim to be the bot, which is the actor whose forces the review may unforce. Dropping it fails that INSERT loudly.PlanForceActionRecord.Actoris a required member for the same reason — the compiler enumerated both construction sites viaCS7036rather than a reviewer having to.GetQueryHistoryAsyncis deliberately not actor-filtered, and the asymmetry is documented so it does not get "fixed" for symmetry. That read restrains the bot, and every limb restrains it correctly by counting an operator's rows: a query a human touched two hours ago is a query the bot should stay off, an operator's force spends real blast radius, and an operator's force that would not stick is evidence the next one will not either. Filtering would make the bot more willing to act the more a human already had.The flow, and both click boundaries
OperatorRemediationFlow.Observeis pure and static, no clock and no I/O, matchingForcePlanBotPolicy. Nothing in it executes anything: the eviction happened before the caller could ask, andForceOfferedis permission to draw a control, never permission to act.MinReviewExecutionsexecutions orObservationWindowMinutes. The executions floor is not restated —Observetakes it as a parameter and callers passForcePlanBotSettings.MinReviewExecutions, the one named home for detection's 25.ObservationWindowMinutes = 30is new; there was no 30-minute detection floor to reuse, and it lands on the bot's settings so a human and the bot observe one eviction for one length of time.ForceOfferedis true for exactly one verdict,RegressedPlanReturned, and it is a property of the returned value so no caller can re-derive it differently.ExactlyOneVerdictOffersTheForcechecks it over every verdict by reproducing each one through the machine and readingForceOfferedback, rather than against a table retyped in the test.The two limbs are not interchangeable, and that is the design's substance. Plan identity is not a statistical quantity — one compile settles which plan the optimizer chose — so the regressed plan returning is a legitimate verdict on a window the timeout closed with three executions, and it is the arm that justifies the force. Cost is statistical and gets the executions floor. A window the timeout closed below the floor is
observation_inconclusive, journaled as its own outcome: folding it intoRegressedPlanReturnedwould offer a force on the strength of not having looked. Every outcome journals (optimizer_recovered,regressed_plan_returned,worse_after_evict,observation_inconclusive).Plan-handle
FREEPROCCACHEper platform, and the named degradeThe honest answer is that a per-platform table is the wrong instrument, so this branch does not ship one. Two facts have to hold and they fail for reasons an operator would fix differently:
DBCC FREEPROCCACHEdoes not exist on Azure SQL Database (EngineEdition5); Managed Instance has it. No grant changes that.ALTER SERVER STATE. A grant does change that.Collapsing them would tell an operator to ask a managed provider for a permission that would not help.
EvictCapabilitykeeps them separate andEvictUnavailableReasonresolves platform before permission, pinned byPlatformOutranksPermission_SoNoOneIsSentToAskForAGrantThatCannotHelp.The permission half is probed, not predicted:
AlterServerStateProbeSqlisSELECT has_alter_server_state = has_perms_by_name(NULL, NULL, 'ALTER SERVER STATE');, run read-only as the remediation credential. That answers for the effective permissions of the executing login, which is the only question that matters — the grant can arrive through a server role, throughCONTROL SERVER, or directly, and an operator on a managed platform generally cannot tell which they were given. A table of "platforms that allow plan-handle FREEPROCCACHE" is a claim about a managed service's grant set, which the vendor can change without telling us, and it would go stale in the direction that keeps passing.I did not execute a
DBCC FREEPROCCACHEagainst any server to settle this — that is a write to production and outside what this branch does at all. So the permission question is answered by the shipped probe rather than by an assertion in this description.TheCapabilityProbeIsReadOnlypins that the probe is a singleSELECTwith no DDL keyword, asks the server-scope form (both leading arguments NULL, or it answers about the current database and reports a grant an eviction cannot use), and aliases its output column.The degrade is pinned as never-silent, exhaustively.
NoCapabilityShapeCanDegradeWithoutANamedReasonwalks all sixEvictCapabilityshapes and asserts that a surface not offering evict-first always carries a reason and one offering it never carries a stale one, with a count check so a comprehension that produced nothing cannot pass by asserting about no cases.ExactlyOneCapabilityShapeOffersEvictFirststates the grant as a count, so quietly widening it — treating an unprobed capability as permitted, say — fails here rather than surfacing as an unexpected eviction attempt. An unprobed capability is its own reason (evict_capability_unknown), deliberately distinct from denied: refusing to guess rather than attempting a write to find out.Evidence
The pins run.
Darling.TestsandLite.Teststargetnet10.0-windowsand cannot execute on macOS, so the actual test sources — not copies — were compiled into anet10.0console harness over a minimal xUnit shim and executed against the real assemblies: 93/93 passing, covering the two new suites plusForcePlanBotPolicyTests,ForcePlanSelfReviewTests,MigrationDataMovingRungCensusPinsandMigrationLadderPins.Red-then-green on the census pin. The V113
CREATE INDEXon the populatedcollect.plan_force_actionsis a new data-moving rung; the real pin failed with exactly that finding before it was declared, and passes after. It also measured that the rung'sADD COLUMN ... DEFAULT 'bot'andALTER COLUMN ... DROP DEFAULTare not findings, rather than my reasoning that they would not be. DeclaredSetsTheFloor: falsewith the arithmetic: the table's size is capped by the bot's cooldowns and a 365-day horizon at roughly 46k rows across a 42-server fleet, and it is empty on every store today.Mutation: 11/11 killed, each with a before→after content hash proving it applied and each naming the test that caught it.
AServerWithNoRemediationCredential_GetsNoSurfaceBlockershalf of the verdict droppedAVerdictWhoseHalvesDisagree_GetsNoSurface(true, …)Eligiblehalf of the verdict droppedAVerdictWhoseHalvesDisagree_GetsNoSurface(false, …)PlatformOutranksPermission_…AnUnprobedCapability_…,ExactlyOneCapabilityShapeOffersEvictFirstWhenBothLimbsAreSatisfied_TheOneCarryingEvidenceWinsTheRegressedPlanComingBack_IsJudgedOnIdentity_EvenOnTheTimeoutLimbACheaperDifferentPlan_…,ExactlyOneVerdictOffersTheForceTheExecutionsLimbFiresAtTheSettingsFloor_NotAtALiteralAnAbsentRegressedHashOnTheTargetSide_AlsoNeverMatchesADifferentPlanIndistinguishableInCost_IsInconclusiveThe first pass reported 7 of the 11 as
NOT-APPLIEDrather than as kills, because the new source files are LF in the working copy while the anchors were CRLF. That is the failure mode worth naming: a mutation that never applied is indistinguishable from one that was caught, so the harness asserts the anchor count and the content hash before crediting anything.Two of my own tests were wrong and the harness found them.
TheCapabilityProbeIsReadOnlyfailed twice on the real statement for reasons that were the scan's fault: first the permission name'ALTER SERVER STATE'matched as a DDL keyword, then the output aliashas_alter_server_statedid, because a substring scan cannot tell an identifier from a statement. It now strips quoted literals and matches on word boundaries, with a positive control that it still fires on a real write. A scan with either flaw has to be silenced to ship, and a silenced scan guards nothing.The fence is green, measured. The built
PerformanceMonitor.Darling.Service.dllwas decoded at both UTF-16 alignments and searched for all three write statements: none present. The scan's positive control found the newPerformanceMonitorDarling-Remediationliteral in the same assembly, so it does read real literals.Not verified locally: the live-Postgres own-forces-only scenario in
PlanForceActionStoreTests(it needs theDarling PostgreSQL testsjob), and theLite.Testsdivergence pin's schema half (Schema.GetAllTableStatements()is in thenet10.0-windowsLite project). Its two source-scan halves were replicated in Python: 266 Lite.csfiles, zero hits on any of the 13 capability tokens.CI
All eight checks green on
162b615d0. Durations are the API'sstarted_at/completed_at, not wall-clock guesses.check-branchesdevdescription-driftDarling whole-tree guardsDarling Linux buildDarling PostgreSQL testsreviewclaude[bot]; findings belowverifybuildLite.Tests3494 / 0 failed / 0 not run,Darling.Tests8122 / 0 failed / 0 not run,Dashboard.Tests782 / 0buildat ~9 minutes is a real run, not the docs fast path, and its step list confirms it:Run Lite tests,Run Dashboard testsandRun Darling testsall executed rather than being skipped.The new tests ran, counted two ways.
Darling.Testswent 8115 → 8122 across the commit that addedRemediationCredentialRungTests, which is exactly its seven[Fact]s; and none of the new suites appears in the SKIP list. MTP prints no per-test pass lines, soFailed: 0+Not Run: 0+ absent-from-SKIP is the available evidence.What CI caught that local verification could not
Five failures on the first run, all in the class the Windows-only and live-Postgres suites exist to find. Recorded because four of them are the registration ratchets any new rung trips, and the fifth is the rung working.
DarlingRetentionTestsend-to-end:23502 null value in column "actor". Its raw INSERT intoplan_force_actionsomitted the column. That is the dropped DEFAULT doing exactly what it is for — the INSERT that forgets the actor fails instead of silently claiming to be the bot. The test now names it.CollectorStallProbeStoreTestsandCollectorStallProbeViewerGateTestsasserted V112 was the top rung, which V113 makes false. That claim belongs to whichever rung actually is top, or every new rung breaks every older rung's suite, so it moved toRemediationCredentialRungTests: the all-sentinels-true mapping, the last-ordinal equality, and the one-rung-behind check. V112 keeps its own fixed ordinal and now switches off every LATER sentinel for its behind-check, which generalises to the next rung rather than needing this edit again.DarlingManagedRolesTestspins the exact secret-column list;remediation_encrypted_passwordhad to be argued onto the secret side rather than waved through.RegisteredServerSettingDriftTestspins the drift-exclusion list literally, so growing it is a visible act — a good guard, and its own point is that a bumped literal should not be enough. The two new keys are excluded because a drift report is what triggers a disconnect-and-reconnect of the MONITORING connection, and tearing down collection because someone rotated a credential collection never uses would be a real outage. Since bumping a literal by reflex is how such a guard stops meaning anything, the property behind it is now asserted too: only a credential-shaped key may be excluded, so excludingtrustServerCertificate— the field Editing a registered server's settings in darling.json is silently ignored — only ADDING a server is warned about #2552 actually reported — fails rather than passing with a bigger number.plan_correctioncarries a generatedlast_good_plan_force_failure_reasoncolumn, so a statement-substring scan reported the monitored server's own Query Store forcing-failure reason as a bot journal. It now parses table NAMES, withplan_correctionas the control proving the parse discriminates a column from a table.Review finding, addressed.
claude[bot]caught thatGetPendingReviewsAsync' XML summary still said own-forces-only was "structural — the read starts from rows this bot journaled". True while the bot was the table's only writer, false as of V113, and directly contradicting the SQL comment a few lines below it. A stale WHY-comment here is worse than none: it invites removing theactorfilter for looking redundant, which is the guarantee it now carries alone. Fixed in162b615d0. The final review pass reports no blocking issues.Lite: Darling-only, permanently — and the divergence is asserted, not inherited
Settled on #2138: remediation execution is Darling-only, permanently. The design's "one PR-sized unit per SKU pair" does not apply, and this is a wording fix rather than a reversal.
The rule was already stated in Lite's own code, but with two different reasons, only one of which survived — which is the case that shows why this pin asserts rule and reason separately. The obsolete half named the deprecated Dashboard SKU as the owner of remediation execution, so it pointed at nothing live. The half that decides it is Lite's store: a DuckDB file is per-workstation, so a remediation journal kept there cannot be the shared audit trail such a journal exists to be — two operators acting on one server would each hold half of it and neither could see the other's forces — and it cannot carry phase 2's own-forces-only invariant, which is a predicate plus a
related_action_idself-reference within one shared store. No deprecation can invalidate that, so all three sites that state the rule now rest on it.The Dashboard mentions in
LiteRecommendationsViewModelandRecommendationsTabstay: they are UI comparisons about an "Open in Active Queries" deep-link, not scope claims, and their reason is the surface's tab shape. They now also serve as the positive control proving the dead-reason absence assertions are not passing against a failed read.OperatorRemediationLiteDivergencePinTestsasserts the divergence is complete and its stated reason still exists, failing in opposite directions: Lite gaining a credential, executor seam or journal fails the capability scan, and the advise-only statements being deleted or reworded away fails the last test, so this pin cannot outlive its own premise. It also asserts the shared seam is reachable from Lite and lives in the same assembly asFactRemediation, so whichever way the call goes no logic is written twice.A Lite journal table was deliberately not added. A
plan_force_actionsin DuckDB while Lite cannot act would be a permanently-empty table, which reads to every later consumer as "the feature is here and nothing has happened" rather than "the feature is not here" — the exact one-SKU-empty-value trap. V107's doc comment already keeps its column shape twin-ready.What is not in this branch
IPlanForceExecutor's implementation — the force/unforce/evict statements, and the orchestration that drivesObservefrom live Query Store deltas. This is the change that necessarily opensPlanForceNoWritePathTests, and per that file's own remarks the fence should be relaxed in the diff whose subject is making its claim stop being true. Opening it for a half-flow, while the SKU question above is unsettled, is the wrong order.DarlingCommandExecutor's switch is where a new operator-armed verb lands;RecommendationCardViewModelalready carries theRemediationActiona surface would gate on.execute_remediation— explicitly out of scope per the design's sequencing.plan_force_bot_enabledviewer surface V107 promised ("viewer surface to follow"), still absent.Open, and yours
Settled since this PR opened: Lite stays advise-only permanently, and the ship vehicle is whenever the next release is cut (it only bites once the write path exists). Both recorded on #2138.
CHANGELOG entry text (uncommitted)