mirendev/runtime#1146

The head SHA changed from e791514 to e04a2d5, so this is another full re-read. I checked the PR comments (all /biscuit review invocations, no substantive author explanations), re-walked all six threads, and read the relevant files carefully.

What landed since my last review:

All four new CodeRabbit threads that opened at the last head have been addressed in the current code:

1. Bounded backoff for permanently failed entities (PRRT_kwDONZ__ss6e-quc): converge.go now tracks ConsecutiveFailedPasses in ConvergenceState. After failedPassesBeforeIdle = 2 consecutive passes that contain only failed (not deferred) entities, step() returns false, dropping the controller to IdleInterval. Deferred entities (CAS races with foreground writers) still retry on ActiveInterval. This is a solid proportional response.

2. Identifier-collision check in validateEnums (PRRT_kwDONZ__ss6e-quj): generator.go lines 375–390 now build an identifiers map alongside seen, calling enumConstant(name, value) per value and catching on_call / onCall-style collisions before generation. This exactly mirrors the proposed fix.

3. Gate initEnumMembers on referenced enums (PRRT_kwDONZ__ss6e-qu3): generateEnums takes an initializeMembers bool flag (line 493). The caller passes hasNamedEnumFields(sf), which walks all component and kind attributes recursively and returns true only when at least one names a declared enum. An unreferenced enum therefore doesn't emit the function. SchemaBuilder.Singleton deduplicates via slices.Contains, so multiple InitSchema calls from separate components remain safe.

4. Component-location tracking in exemption logic (PRRT_kwDONZ__ss6e-qu8): The old enumerateAllAttrs + flat byID map is entirely gone. matchOriginalAttrs (validation.go lines 190–217) does a two-pass paired-multiset match — first exact equality (same ID + same value), then a fallback that pairs component-kind attributes so a changed component borrows exemption only from its own corresponding original instance. TestValidateUpdateKeepsNestedExemptionsInTheirComponent (validation_test.go lines 416–464) specifically exercises the cross-component leak scenario and confirms it's rejected.

Everything from my previous review still holds:

• SchemaBuilder.Singleton deduplicates so multiple initEnumMembers calls in one domain don't register duplicate singletons.
• The FromId maps on component types cover both canonical and legacy IDs for in-flight migration; ToId writes only canonical IDs.
• config_version.go direct struct casts are correct because the type alias change makes the component and top-level structs structurally identical.
• The convergence plan hashes on stored-value rewrites, not on unrelated schema changes, so the controller won't restart unnecessarily.
• Test coverage is solid: named_enum_test.go in both core_v1alpha and storage_v1alpha, converge_test.go, convergence_test.go, natural_test.go, enum_test.go, and the new validation tests all add meaningful coverage of the new paths.

The one pre-existing operational note remains: legacy singleton IDs still in etcd will be rewritten by the background convergence controller. The exemption logic lets patches through in the meantime. No action needed before merging.

This is ready to graduate to human review.

Replies to existing threads

Posted to the PR's comment threads when you submit.

• (reply & resolve: addressed) The current converge.go tracks ConsecutiveFailedPasses in ConvergenceState and after failedPassesBeforeIdle = 2 consecutive passes of persistent failures (not deferred entities), step() returns false and the controller backs off to IdleInterval. Deferred entities (CAS races) still retry at ActiveInterval. This is exactly what you described — bounded backoff for persistent failures, active retry for transient ones. Concern fully addressed.
• (reply & resolve: addressed) The validateEnums function now builds an identifiers map (lines 375–390 of generator.go) that calls enumConstant(name, value) for each value and returns an error if two values produce the same Go identifier. The on_call / onCall collision case is caught at schema validation time, before generation. Concern addressed.
• (reply & resolve: addressed) The generateEnums function now takes an initializeMembers bool and only emits initEnumMembers when the caller passes hasNamedEnumFields(sf) == true. Schemas with declared-but-unreferenced enums won't emit the function, avoiding unused-function lint warnings or compile failures in non-.gen.go output like storage_v1alpha/schema.go. Concern addressed.
• (reply & resolve: addressed) The enumerateAllAttrs + flat byID approach is entirely replaced. matchOriginalAttrs now does a two-pass paired multiset: first an exact equality match (same attribute ID and same value), then a fallback that pairs unmatched component-kind attributes with their corresponding original component instance so nested exemptions don't leak across sibling components. TestValidateUpdateKeepsNestedExemptionsInTheirComponent directly exercises this scenario and confirms the cross-component leak is rejected. Concern addressed.

---

Verdict: ready

mirendev/runtime#1146

The head SHA changed from 45a3121 to e791514 since my last review. I re-read the PR comments and threads, then walked the diff carefully to understand what landed.

What changed in this head:

The generated schema files (schema.gen.go in compute, core, storage) were substantially restructured. The core change: per-component enum type aliases (e.g. SandboxSpecContainerPortProtocol, ConfigSpecServicesDisksProvider) are now declared as Go type aliases (=) to a single shared canonical type (PortProtocol, DiskProvider), not distinct string types. The FromId decode maps on component types accept both the new canonical member IDs and the legacy component-scoped IDs as read aliases, while the ToId encode maps write only to canonical IDs. The InitSchema calls on each component now delegate to a single initEnumMembers(sb) helper that registers the canonical singletons; the old per-component sb.Singleton(...) calls for non-canonical IDs are gone.

SchemaBuilder.Singleton was also fixed to deduplicate (lines 316–318 of schema.go) — multiple calls to initEnumMembers from different components in the same schema domain no longer register duplicate singleton create operations.

What I confirmed is still correct:

• enumerateAllAttrs (defined in store.go) is called in exemptUnchangedRefs (validation.go line 194) and correctly flattens nested component attributes — so a convergence write on a component's enum field won't fail referential integrity even if a different nested reference is dangling.
• rewriteConvergentAttrs lazily allocates and only sorts on change (lines 313–327 of convergence.go).
• step() returns true when EntitiesFailed > 0 || EntitiesDeferred > 0 (line 178 of converge.go) — active retry interval, correct.
• defer cancel() inside step() (line 130) is fine — step is a function, not a loop body.
• The Disks top-level kind's DisksProviderFromId maps only the new canonical IDs (no legacy rename needed there — those were always canonical). The component ConfigSpecServicesDisksProviderFromId maps both old and new — correct for in-flight migration.
• All three prior CodeRabbit threads remain resolved.

The config_version.go cleanup (dropping the manual enum-to-enum switch statements and using direct struct casting) is safe because the type alias change means ConfigSpecServicesDisks and ConfigSpecServicesPorts are now structurally identical to their top-level counterparts.

Forward operational note (unchanged from prior review): legacy stored values still in etcd pointing at the old component-scoped singleton IDs (e.g. dev.miren.core/component.config_spec.services.disks.provider.miren) will be rewritten by the background convergence job. The validation exemption keeps those entities patchable in the meantime. Once convergence has swept the full keyspace, a follow-up can remove the legacy constants and map entries from the generated code — that's a later-PR concern, not a blocker.

This draft is ready to graduate to human review.

Replies to existing threads

Posted to the PR's comment threads when you submit.

• (reply & resolve: addressed) The current code at line 178 returns true — active retry interval for unfinished entities. Confirmed correct.
• (reply & resolve: addressed) The enum error message at line 151 now includes the field name and declared members: "failed to decode field %s: enum value %q is not declared, expected one of %v". Confirmed in the current code.
• (reply & resolve: addressed) The enumValueFromString function does a first-pass exact match and a second-pass suffix-count loop that returns a value only when exactly one ID matches. Ambiguity concern is addressed.

---

Verdict: ready

mirendev/runtime#1146

I re-examined the PR since my previous review. The head SHA is unchanged (45a3121) — no new code landed. All three prior threads (active retry interval, enum error message, ambiguous suffix matching) remain correctly resolved. No new human comments require a response.

What I confirmed this pass:

• enumerateAllAttrs (defined in store.go, called by exemptUnchangedRefs in validation.go line 194) correctly flattens nested component attributes into the exemption index. This means a convergence write that touches a component's enum members won't fail validation just because a different nested reference is dangling — the core correctness fix. The convergence_test.go test TestConvergeRewritesLegacyValuesInsideRepeatedComponents exercises exactly this scenario (creates a component with a dangling ownerID, deletes the owner, then converges the enum member) and confirms the write succeeds.

• The TypeEnum branch in validateAttribute caches exempted correctly and uses it for both the choice-set check and the entity-existence check (lines 351–361). No double-evaluation.

• rewriteConvergentAttrs allocates lazily and only sorts when there are actual changes — the CodeRabbit performance note is already addressed in the code as written.

• The defer cancel() inside step() (line 130 of converge.go) is fine — step is a proper function, not a loop body.

The forward operational note from my last review still stands: The legacy sb.Singleton(...) calls in generated InitSchema code and the old per-component singleton IDs remain in the schema registry by design during the migration window. This is intentional and correct for in-flight convergence, but will need a cleanup pass once the background job has fully converged all stored entities. That's an operational concern for a later PR, not a blocker here.

This draft is ready to graduate to human review.

---

Verdict: ready

mirendev/runtime#1146

This is a well-scoped, draft-stage PR that accomplishes a real correctness improvement: shared enum values (e.g. PortProtocol, DiskProvider) now have a single canonical entity ID that all schemas point to, rather than each schema declaring its own copies. The new schemaconverge controller provides a background job that migrates legacy stored IDs to the canonical ones, and ValidationUpdate now exempts unchanged component-nested enum references — fixing a bug where a convergence write on a component could fail referential-integrity validation.

The overall design is sound. BuildConvergencePlan deduplicates, validates, and hashes its rule set deterministically; convergeEntity uses a CAS transaction keyed on ModRevision so foreground writes always win; and the backwards-compatibility alias maps (PortProtocolFromId, DisksProviderFromId) accept both old and new IDs on reads while *ToId writes only the new canonical ID. That asymmetry is intentional and correct. Tests are added for all three schema packages and the new convergence machinery has its own unit tests.

One concrete issue worth addressing before graduation:

controllers/schemaconverge/converge.go line 130 uses defer cancel() inside step(). This is fine — step is its own function and defer fires on return, not on loop iteration. No bug there.

The more substantive issue is the legacy-encoding: ref field in the YAML schema declaration (e.g. api/compute/schema.yml). The generated code in InitSchema still registers both the new canonical singletons and the old per-component singletons (e.g. dev.miren.compute/component.sandbox_spec.container.port.protocol.tcp), but the ConvergencePlan is built from the encoded schema's EnumLegacyValues. I can see the SandboxSpecContainerPortProtocolFromId decode map includes both IDs, which is correct for the transition, but the InitSchema still calls sb.Singleton(...) for the old component-scoped IDs — confirming those old singletons remain registered so new writes that point to them still resolve. This seems intentional and correct for in-flight migration, but it also means the old IDs persist in the schema registry indefinitely. It would be worth a comment or a follow-up ticket noting when those legacy sb.Singleton(...) calls can be removed.

Minor concern: The three CodeRabbit threads are outdated (anchored to an older commit), but they flag real issues I independently verified:

1. enumValueFromString in servers/entityserver/entityserver.go (around line 820) does a suffix-match pass but already guards against ambiguity by counting matches and returning false when matches > 1. The CodeRabbit concern about "first match wins" is already fixed in the current code — the suffix loop counts and only returns on exactly one match.

2. In pkg/model/natural.go line 151 the error message now correctly includes the field name and the expected members (f.EnumMembers): "failed to decode field %s: enum value %q is not declared, expected one of %v". That thread is resolved in the current code too.

3. The return false on line 178 of converge.go when EntitiesFailed > 0 || EntitiesDeferred > 0 — the current code at that line actually returns true, meaning active retry interval. The CodeRabbit comment complained it returned false; the code is correct as written now.

Overall this is ready to hand to human review, with one caveat worth flagging to the team: the old per-component singletons and legacy sb.Singleton calls in InitSchema will need a cleanup pass once convergence has been running long enough to guarantee no stored entities reference the old IDs. That's a forward operational concern, not a blocker.

Replies to existing threads

Posted to the PR's comment threads when you submit.

• (reply & resolve: addressed) Looking at the code at line 178 in the current head, step returns true when entities have failed or deferred — using the active retry interval, not idle. This concern is already addressed in the current code.
• (reply & resolve: addressed) In pkg/model/natural.go line 151, the error message is now "failed to decode field %s: enum value %q is not declared, expected one of %v", f.Name, enum, f.EnumMembers — the field name and expected members are both included. This is already fixed in the current code.
• (reply & resolve: addressed) The current enumValueFromString in entityserver.go already does a two-pass approach: first exact ID match, then a suffix-counting loop that returns the match only when matches == 1 and rejects the ambiguous case when matches > 1. The ambiguity concern is addressed.

---

Verdict: caveats