operator: plan-pointer and crash-window gaps around supersede

[hardbyte]

Found during a review pass over the Phase 0 work. These are all real but distinct from the empty-plan class fixed in #183, so they are filed rather than folded in.

1. Supersede-before-create inverts the crash-safety invariant the code itself documents

create_or_update_plan deliberately supersedes old Pending plans only after the new plan is fully visible — plan.rs:462-464: "This avoids losing the current actionable plan if SQL persistence fails before the replacement is materialised."

Both reconciler replace paths do the opposite:

• Pending arm: mark_plan_superseded (reconciler.rs:1562) then create_or_update_plan (reconciler.rs:1612)
• Approved arm: mark_plan_superseded (reconciler.rs:1330) then create_or_update_plan (reconciler.rs:1334)

If the supersede patch lands and creation then fails (ConfigMap quota/403, API flake, operator death), the reconcile errors out and the error path keeps current_plan_ref — it is cleared only for UnsatisfiableWildcardGrant (reconciler.rs:634-635, 704-705). Until the next successful reconcile, current_plan_ref points at a Superseded plan, no actionable plan exists, and any reviewer decision against the referenced plan is silently void.

For the Pending arm the early supersede is redundant — the old plan's digest differs, which is why we are replacing it, so step 12 (plan.rs:465-489) would supersede it after the replacement exists. Dropping the call at reconciler.rs:1562 restores the invariant for free. The Approved-arm call is not redundant (step 12 skips Approved-phase plans), so that one needs step 12 extended instead.

2. current_plan_ref can be stranded permanently

• Clear crash window: in the Clear paths, mark_plan_superseded precedes the status write that sets current_plan_ref = None. If the process dies between them, the next reconcile finds no actionable plan and empty changes, landing in the "no changes needed (manual approval mode)" branch (reconciler.rs:1726-1753) — which sets Ready/InSync but never touches current_plan_ref. The stale reference persists; once cleanup_old_plans (plan.rs:939-963) deletes the Superseded plan, the policy references a nonexistent object indefinitely. Cheap fix: clear current_plan_ref in that branch too.
• Plan mode never revalidates at all. The whole mechanism lives only in the Manual-Apply arm. Plan mode (reconciler.rs:1051-1177) calls create_or_update_plan only when drift is detected; when drift vanishes out of band, the pending plan is never superseded and current_plan_ref is never updated — status says Drifted=False, InSync while pointing at a Pending plan describing changes that no longer exist. Harmless in that Plan mode never executes, but it is a reviewable artifact that lies, which is the exact shape Phase 0.2: revalidate pending plans instead of freezing them #178 set out to eliminate.

3. Failed-plan dedup routes the pointer at a Failed plan while status says "awaiting approval"

create_or_update_plan returns Deduplicated(failed_plan_name) for a recently failed identical digest (plan.rs:265-287). Callers treat the returned name as the current plan and write "Plan {name} created; N change(s) awaiting approval" (reconciler.rs:1786-1812, and the Pending-Replace path). The referenced plan is Failed, not awaiting anything.

4. Model coverage gaps behind these

• PlanRevalidation.tla models only the Pending arm; PlanApproval.tla fixes dbEffects \in Effects so effects can never vanish. Neither spec can reach the approved-arm state that feat(operator): revalidate pending plans instead of freezing them #183 just fixed in code.
• PlanRevalidation.tla has no database-drift action — dbEffects moves only via PolicyEditEffective, which always bumps the generation. Adding a drift action (effects move, generation does not) makes generational fail SummaryMatchesPlan too (probe: 67 states), so the current model understates the case for digest-identity. Semantic still passes with drift modeled (probe: 254 states generated, 99 distinct). Worth adding, since justifying digest-over-generation is the spec's whole purpose.
• All three PlanApproval configs fix HasPasswordChange = TRUE, which double-enforces approval and makes the safety invariants undetectably weak: removing the approved conjunct from OperatorExecutes passes the shipped config (19 states, no error), while the same mutant with HasPasswordChange = FALSE violates NoUnreviewedExecution in 7 states. A HasPasswordChange = FALSE config would close this.

5. The digest's target binds the Secret reference, not the server

approval.rs:21-22 claims the encoding binds "the target database identity, so an approval cannot be reused against a different server". For URL-mode connections the bound value is just {secret_ref.name}/{secret_key} (crd.rs:311-313); the connection Secret's resourceVersion is bound nowhere, and the doc at crd.rs:307-310 claiming host+port+dbname is true only for params mode.

So: approve a plan against staging, someone edits the Secret in place to point at prod, and if prod yields the same diff the digest still matches and the approved DDL executes there. This is largely what #180 (tiered target identity) is for — noting it here with the concrete mechanism, and flagging that the current doc comment overstates today's guarantee.

Refs #173.

[hardbyte]

Adding a sixth item that belongs with finding 1 — same fix site, found by CodeRabbit on #183 and confirmed with a narrower reproduction path than reported.

6. A same-second replacement can reuse the superseded plan's name

generate_plan_name (plan.rs:1207-1219) is {policy}-plan-{timestamp}-{sql_hash[..12]} at second resolution. When a replacement is created in the same second as the plan it replaces and renders identical SQL, the name collides. should_patch_existing_plan_status (plan.rs) only permits patching a Pending or statusless plan, so the 409 against a now-Superseded plan returns it as Deduplicated, and the caller writes that non-actionable name into current_plan_ref alongside a status message saying "awaiting approval".

The route matters, because the obvious one is closed: both replacement arms are reached only on digest mismatch, and mismatched digests normally mean different SQL and therefore a different name. What actually reaches it is a legacy plan — no changeDigest, or an older changeDigestEncoding. plan_matches_digest fails closed on those regardless of content, so the effects can be genuinely identical, the SQL renders the same hash, and the name repeats.

Bounded and self-healing — the next reconcile finds no actionable plan and creates one, and no approval can be recorded against a Superseded plan meanwhile — so the cost is a misleading reference and a wrong status message, not a lost or wrongly-executed approval.

Why it belongs with finding 1: dropping the pending arm's early mark_plan_superseded (reconciler.rs) closes this window outright, because the old plan is still Pending and therefore patchable when create_or_update_plan reaches it. The approved arm needs step 12 extended to Approved-phase plans, or an explicit error on collision with a non-patchable plan. Same three call sites as findings 1 and 3.

---

Generated by Claude Code