refactor(snd): fold GenesisCeremonyNeeded into GenesisCeremonyComplete; make spec.genesis immutable#333
Conversation
…e; make spec.genesis immutable GenesisCeremonyNeeded was a presence-signal (School 2) condition — its True value meant "ceremony is needed," with absence meaning "not applicable." The recently-codified conditions doctrine (CLAUDE.md `### Conditions`) defaults to always-present School 1 conditions whose state is carried by Reason. This PR removes the last sei `*Needed` condition by folding the two genesis conditions into one always-present condition with a reason enum: - True / Complete — ceremony finished (latched) - False / NotApplicable — spec.genesis is unset - False / NotStarted — spec.genesis set, ceremony not yet started - False / InProgress — ceremony executing under an active plan The True/Complete state is latched: once a ceremony completes, the condition stays True regardless of later spec edits. Clearing the spec doesn't unmake history. To prevent the latch from being defeated by post-bootstrap spec edits (and to forbid the semantically meaningless act of editing chainID or gentx amounts after on-chain bootstrap), spec.genesis is now CEL- immutable once set. The existing field-level `self == oldSelf` rule silently allowed clearing through (CEL on an optional pointer field doesn't fire when the field is removed); the new rule lives at the spec level with explicit `has()` checks. Creating without genesis and adding it later is still permitted. The planner now triggers on absence of GenesisCeremonyComplete=True instead of presence of GenesisCeremonyNeeded — same semantics, one fewer condition to maintain. Pre-existing SNDs in etcd that carry a stale GenesisCeremonyNeeded entry are harmless (no controller code reads it any more); the operator-side cleanup is one kubectl patch per SND. Tested: - All unit tests pass (including 5 cases for setGenesisCeremonyCondition, one of which asserts the latched-True survival when an operator clears spec.genesis) - envtest: 46.4s, no regressions - New envtest: TestGenesis_ImmutabilityGate (4 cases — clear rejected, chainId rejected, stakingAmount rejected, same-content allowed) + TestGenesis_CreationWithoutGenesisAllowsLaterAddition Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
PR SummaryMedium Risk Overview Strengthens API guarantees by making Reviewed by Cursor Bugbot for commit 9939697. Bugbot is set up for automated code reviews on this repo. Configure here. |
- Hoist setGenesisCeremonyCondition above any early-return path in Reconcile so the condition is visible on every reconcile, matching the always-present doctrine. - Add unit test case for the post-failPlan transition (False/NotStarted). - Extend immutability envtest with Accounts and Overrides mutation cases (both should be rejected by the deep-equality CEL rule). - New envtest TestGenesis_ConditionSeededOnEveryReconcile asserts the hoist guarantees first-reconcile visibility. - Comment sweep: present tense, no historical context, doctrine refs cite CLAUDE.md by section rather than narrating prior reviewer feedback. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The hoist (setGenesisCeremonyCondition runs before reconcileSeiNodes) makes the controller patch status fast enough on Linux CI that the test's Get→mutate→Update loses on resourceVersion before CEL validation can fire. The test then sees a 409 Conflict instead of the expected immutability rejection. updateSNDWithRetry re-fetches and re-applies on conflict, surfacing the first non-conflict error — which is the CEL rejection we're asserting on. Also regenerates CRD YAMLs to pick up the doc-comment text change that the prior commit's comment sweep left out of `make manifests`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Comments in this PR were leaning on "(the new signal)" and "now that ... is hoisted" framings — both implicitly past-relative and prone to rotting as the code moves on. Reword to direct present-tense statements of the invariant. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The doctrine fold makes ConditionGenesisCeremonyComplete always present, so the next planner refactor that reads "hasCondition(...)" would plausibly misread it as a presence check and break needsGenesisPlan's gate. Rename to make the True-check semantics explicit at every call site, matching the controller-side helper in nodedeployment/status.go. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Summary
Folds
GenesisCeremonyNeeded+GenesisCeremonyCompleteinto a single always-presentGenesisCeremonyCompletecondition whose state is carried by a stable reason enum. Removes the last sei*Needed(School 2) condition, completing the harmonization to the doctrine codified in #329.Also makes
spec.genesisCEL-immutable once set, so the latched-True semantics can't be defeated by post-bootstrap spec edits — and so operators can't silently mutate fields (chainID, staking amounts, gentx outputs) that are already baked into chain state.State machine
True / CompleteFalse / NotApplicablespec.genesisis unset (and ceremony never ran)False / NotStartedspec.genesisset, ceremony not yet startedFalse / InProgressLatch-check ordering is load-bearing — a completed ceremony's history must not be downgraded to
NotApplicableby a later spec clear (doc-commented at the call site).setGenesisCeremonyConditionis hoisted above any early-return path inReconcile, so the condition is visible on every reconcile andkubectl describe sndnever shows an absent value during initial-reconcile windows.Spec.genesis immutability
Replaces a field-level
+kubebuilder:validation:XValidation:rule="self == oldSelf"(which silently allowed set→nil through, because CEL on an optional pointer field doesn't fire when the field is removed) with a spec-level rule using explicithas()checks:spec.genesis: allowedspec.genesisto an SND that didn't have it (nil → set): allowedspec.genesis: rejectedspec.genesis: rejectedPlanner translation
needsGenesisPlanpreviously triggered on presence ofGenesisCeremonyNeeded; now triggers on absence ofGenesisCeremonyComplete=True. Same semantics across all four states (NotApplicable/NotStarted/InProgress/Complete), one fewer condition to maintain.Operator-visible cleanup
Pre-existing SNDs in etcd carry a stale
GenesisCeremonyNeededentry. The new controller never reads or writes that type, so the entry is harmless — same situation as #330's staleRouteReady. Manual cleanup is a one-shotkubectl patchper SND.Test plan
go build ./...cleango test ./...— all unit tests passmake test-integration— envtest passes (46.2s, no regression)TestSetGenesisCeremonyConditioncover NotApplicable, latched-True (twice, including operator-clears-spec-after-completion), InProgress, NotStarted, post-failPlan return-to-NotStartedgenesis_immutable_test.go:TestGenesis_ImmutabilityGate— 6 subtests: clear rejected; chainId / stakingAmount / Accounts / Overrides mutations rejected; same-content update allowedTestGenesis_CreationWithoutGenesisAllowsLaterAddition— confirms the!has(oldSelf.genesis)short-circuit permits nil → setTestGenesis_ConditionSeededOnEveryReconcile— confirms the controller-side hoist ensuresGenesisCeremonyCompleteis visible after first reconcileDeferred follow-ups
NotStarted → InProgress → Completereason transition driven by a real plan. The current envtest harness focuses on InPlace rollouts; adding a genesis ceremony plan path is its own investment and best landed alongside an explicit "Phase X envtest: genesis ceremony" effort.manifests/mirror — the platform repo's Kustomization consumesconfig/default, notmanifests/. Themanifests/mirror is redundant overhead and a candidate for deletion in a follow-up.🤖 Generated with Claude Code