Phase 4 follow-ups: destroy ordering/protection, field-rename migration, backup fidelity, cleanups #17

Follow-up from the code review of #15 (Phase 4 — Apply + destroy). The two blocking correctness bugs were fixed in that PR (commit c152ea6: recreate no longer leaks duplicates; destroy fails gracefully). This issue collects the non-blocking findings so they don't get lost — roughly correctness/robustness first, then fidelity, then cleanup/altitude.

Correctness / robustness

1. destroy delete ordering can delete a parent group before its child

orderDestroy (src/commands/destroy.ts:38) sorts by tierOf only. All groups are tier 1, so ties preserve --target input order — destroy --target parent,child issues DELETE /groups/{parent} while child still references it, and destroy never tears down hierarchy edges (/groups/{id}/parents/{parentId}) before deleting. Combined with CT referential integrity this can 4xx mid-run or orphan children.

Note: this is currently latent — the state file carries no dependsOn/parents edges, so plan's reversed-topological delete ordering (plan.ts:187) also degrades to tier-only today. A real fix needs hierarchy-aware ordering in destroy (fetch /groups/hierarchies and order children before parents, or persist edges in state and reuse a shared orderDeletes() helper for both plan and destroy).

2. preventDestroy is read from live config, so it can't protect the resource you're actually destroying

src/commands/destroy.ts:68 builds protectedKeys from desired.filter(d => d.preventDestroy). apply tells the user to destroy a resource after dropping it from config — at which point its preventDestroy: true is gone too, so the guard silently passes. The flag protects the still-in-config case (not even a delete candidate) and is inert for the removed-from-config case (the real destroy scenario). Terraform-style, protection should live on the state entry, not the ephemeral config, so it survives config deletion.

3. destroy aborts on whole-config validation when a sibling still references the dropped target

destroy loads the full config only to read preventDestroy flags, but loadConfig → evaluateConfig → validateReferences (src/config/context.ts) throws if any other group still declares parents: ["<dropped-target>"]. So to destroy area you must first edit every child's parents list. destroy should read the flags leniently (catch/skip reference validation, or read flags without full-config validation).

4. Pre-rename campus state (shortName) drifts forever and breaks the update PUT

managedFields switched shortName → shorty (src/resources/registry.ts:60) with no migration for existing state. A campus adopted before the rename has state {name, shortName}, so driftFields reports a permanent shortName → undefined drift on every plan, and a real update builds a PUT body carrying the vestigial shortName while omitting the create-required shorty (execute.ts snapshotFromChanges). Needs a state migration (or re-adopt guidance) for the field rename. Low urgency while unreleased, but it bites any state adopted pre-rename.

5. Hierarchy fetch is gated on any managed group, not on hierarchy opt-in

src/engine/build.ts:62 fetches /groups/hierarchies whenever any managed group exists, even if none opted into parents. A failure of that endpoint pushes to fetchErrors, making plan print INCOMPLETE (exit 1) and apply abort — for a user whose hierarchy is entirely unmanaged. Gate on desired.some(d => d.parents !== undefined) instead.

Backup fidelity

6. Pre-apply backup is polluted with a synthetic parents pseudo-field

apply reuses buildPlan's actual map for the backup (src/commands/apply.ts:80), but applyHierarchy mutates that same map in place, injecting parents: ["<logical_key>", …] — internal logical keys, not a real CT column. The backup (documented as actual values for inspect/restore) then embeds non-restorable data for every opted-in group, and diverges from destroy's clean backup. Snapshot before the hierarchy fold, or back up an unmutated copy.

Cleanup / altitude

7. destroy re-implements buildPlan's fetch-actual loop with divergent semantics

The backup loop (src/commands/destroy.ts:81-95) duplicates build.ts's spec-lookup + itemPath + managedFields + 404-skip, but serially (vs mapConcurrent), re-throwing non-404 errors (vs record + continue) and silently continue-ing on a missing spec (vs warn). Extract a shared fetchActual(client, managed) used by both build.ts and destroy.ts so backup and plan read actuals identically and can't drift.

8. The parents pseudo-field is special-cased by string literal in the "field-agnostic" executor

src/engine/execute.ts advertises "adding a type never touches this file," yet hardcodes "parents" in four places (snapshotFromChanges guard + a redundant delete snap.parents, hasFieldChange, applyParentEdges). The next edge-style relation (group-role, permission — already tiered in graph.ts) forces copying every guard with a new literal + endpoint branch; a missed spot silently PATCHes a synthetic field onto a real object. Model "edge-set" as a first-class registry field kind (a descriptor with a PUT/DELETE path template) so the engine routes relation fields generically.

9. Smaller cleanups

• resolveBackupDir (apply.ts:97) repeats the explicit?.trim() || env?.trim() || default idiom already in resolveConfigPath and resolveStatePath — factor a shared resolveWithEnv(explicit, envVar, fallback).
• snapshotFromChanges (execute.ts:43) has both an in-loop c.field !== "parents" filter and a trailing delete snap.parents — the second is dead (base never carries parents). Keep one.
• context.ts parentKey narrowing is redundant after the type guard three lines above — const parentKey = parent || undefined; suffices.
• applyHierarchy (hierarchy.ts) does three passes + a derived optedIn Set to encode "for each opted-in group set both sides"; one loop over the opted-in desired groups does it.
• parentEdges (execute.ts:47) types its params unknown with as string[] casts for a single internal caller that always passes a sorted string array — type them string[].
• Efficiency (marginal, small N): /groups/hierarchies is fetched serially after the concurrent resource batch though it's independent (build.ts:65); destroy's backup fetch and applyParentEdges are serial. Only worth it if managed-set / fan-out grows.

Generated from a /code-review high pass over #15.