fix: Phase 4 follow-ups — destroy ordering/protection from state, backup fidelity, state migration, shared fetchActual (#17)

src/commands/apply.ts

@@ -15,6 +15,7 @@ import { buildPermissionPlan } from "../permissions/plan.js";
 import { renderPermissionPlan } from "../permissions/render.js";
 import { applyPermissionPlan } from "../permissions/apply.js";
 import { confirm } from "../ui/prompt.js";
+import { resolveWithEnv } from "../util/resolve.js";
 import { info, warn, success, error } from "../ui.js";
 
 interface ApplyOptions {
@@ -74,7 +75,7 @@ export function resolveBackupDir(
   statePath: string,
   env: NodeJS.ProcessEnv = process.env,
 ): string {
-  return explicit?.trim() || env.CT_BACKUP_DIR?.trim() || join(dirname(statePath), "backups");
+  return resolveWithEnv(explicit, env.CT_BACKUP_DIR, join(dirname(statePath), "backups"));
 }
 
 export function applyCommand(): Command {

src/commands/destroy.ts

@@ -3,18 +3,19 @@ import { authedSession } from "../api/session.js";
 import { CtApiError, type CtClient } from "../api/ctClient.js";
 import { resolveConfig } from "../config.js";
 import { loadState, resolveStatePath, saveState, type State } from "../state/state.js";
-import { loadConfig, resolveConfigPath } from "../config/load.js";
 import { RESOURCES } from "../resources/registry.js";
 import { assertNotPeople } from "../engine/guard.js";
-import { tierOf } from "../engine/graph.js";
+import { orderKeys } from "../engine/graph.js";
+import { fetchActual } from "../engine/build.js";
+import { parentIdsByGroupId, managedParentKeys, type HierarchyEntry } from "../engine/hierarchy.js";
+import type { DesiredResource } from "../engine/types.js";
 import { writeBackup } from "../engine/backup.js";
 import { resolveBackupDir } from "./apply.js";
 import { confirmTyped } from "../ui/prompt.js";
 import { info, warn, success, error } from "../ui.js";
 
 interface DestroyOptions {
   target?: string[];
-  config?: string;
   state?: string;
   backupDir?: string;
   force?: boolean;
@@ -34,16 +35,69 @@ export function parseTargets(raw: string[]): string[] {
   return out;
 }
 
-/** Reverse dependency order: highest tier first (leaves before their base metadata). */
-export function orderDestroy(state: State, keys: string[]): string[] {
-  return [...keys].sort((a, b) => tierOf(state.resources[b]!.type) - tierOf(state.resources[a]!.type));
+/**
+ * Reverse dependency order for destroy: highest tier first (leaves before their
+ * base metadata) and, within the group tier, a child before its parent.
+ *
+ * The state file carries no hierarchy edges (the synthetic `parents` field is
+ * stripped from snapshots — see execute.ts), so the caller passes `parentKeysByKey`
+ * discovered live from `/groups/hierarchies` (managed groups only). We reuse
+ * `orderKeys` — the very topological apply order plan uses — with those edges,
+ * then reverse it, so destroy is the exact inverse of apply and honours intra-tier
+ * parent edges. Pass an empty map (or omit) to fall back to tier-only ordering.
+ */
+export function orderDestroy(
+  state: State,
+  keys: string[],
+  parentKeysByKey: Map<string, string[]> = new Map(),
+): string[] {
+  const entries: DesiredResource[] = keys.map((key) => ({
+    type: state.resources[key]!.type,
+    key,
+    fields: {},
+    dependsOn: parentKeysByKey.get(key) ?? [],
+  }));
+  return orderKeys(entries).reverse();
+}
+
+/**
+ * Discover managed group→parent edges from the live `/groups/hierarchies`, so
+ * `orderDestroy` can put a child before its parent. Only the group targets need
+ * edges; every managed group is mapped id→key so a parent edge to a not-targeted
+ * managed group is still resolvable (harmless — `orderKeys` ignores deps outside
+ * the target set). Best-effort: a fetch failure warns and returns no edges, so
+ * ordering degrades to tier-only rather than aborting the destroy.
+ */
+async function fetchParentEdges(
+  client: Pick<CtClient, "get">,
+  state: State,
+  keys: string[],
+): Promise<Map<string, string[]>> {
+  const groupKeys = keys.filter((k) => state.resources[k]?.type === "group");
+  if (groupKeys.length === 0) return new Map();
+  const groupIdToKey = new Map<number, string>();
+  for (const m of Object.values(state.resources)) {
+    if (m.type === "group") groupIdToKey.set(m.id, m.key);
+  }
+  try {
+    const raw = await client.get<HierarchyEntry[]>("/groups/hierarchies");
+    const parentIds = parentIdsByGroupId(Array.isArray(raw) ? raw : []);
+    const edges = new Map<string, string[]>();
+    for (const key of groupKeys) {
+      edges.set(key, managedParentKeys(parentIds.get(state.resources[key]!.id) ?? [], groupIdToKey));
+    }
+    return edges;
+  } catch (err) {
+    const message = err instanceof Error ? err.message : String(err);
+    warn(`Failed to fetch group hierarchies for destroy ordering: ${message}. Falling back to tier-only order.`);
+    return new Map();
+  }
 }
 
 export function destroyCommand(): Command {
   return new Command("destroy")
     .description("Explicitly delete managed resources (protected; never implicit)")
     .requiredOption("--target <keys...>", "logical key(s) to destroy (repeatable or comma-separated)")
-    .option("-c, --config <path>", "config file (or set CT_CONFIG)")
     .option("-s, --state <path>", "state file (or set CT_STATE)")
     .option("--backup-dir <path>", "directory for the pre-destroy backup (or set CT_BACKUP_DIR)")
     .option("--force", "skip the typed confirmation (preventDestroy is still enforced)")
@@ -63,35 +117,35 @@ export function destroyCommand(): Command {
         }
       }
 
-      // preventDestroy guard: a target still declared with the flag is blocked.
-      const { resources: desired } = await loadConfig(resolveConfigPath(opts.config));
-      const protectedKeys = new Set(desired.filter((d) => d.preventDestroy).map((d) => d.key));
-      const blocked = targets.filter((k) => protectedKeys.has(k));
+      // preventDestroy guard: read from STATE, never the config. A resource dropped from config
+      // (the real destroy scenario) has lost its config flag, but its state entry still carries the
+      // protection apply mirrored there — so it survives the drop. destroy loads no config file at
+      // all, so a config eval error (e.g. a sibling still referencing the dropped target) can't
+      // block a teardown either (items 2 + 3).
+      const blocked = targets.filter((k) => state.resources[k]!.preventDestroy);
       if (blocked.length > 0) {
         throw new Error(
-          `preventDestroy is set for: ${blocked.join(", ")}. Remove the flag in config first.`,
+          `preventDestroy is set (in state) for: ${blocked.join(", ")}. ` +
+            `Set preventDestroy:false in config and re-apply (or clear it in the state file) first.`,
         );
       }
 
-      const ordered = orderDestroy(state, targets);
       const { client } = await authedSession();
 
-      // Backup: fetch each target's current actual values (best-effort; 404 → skip).
-      const actual = new Map<string, Record<string, unknown>>();
-      for (const key of ordered) {
-        const managed = state.resources[key]!;
-        const spec = RESOURCES[managed.type];
-        if (!spec) {
-          continue;
-        }
-        try {
-          const raw = await client.get<Record<string, unknown>>(spec.itemPath(managed.id));
-          actual.set(key, spec.managedFields(raw));
-        } catch (err) {
-          if (!(err instanceof CtApiError && err.status === 404)) {
-            throw err;
-          }
-        }
+      const parentEdges = await fetchParentEdges(client, state, targets);
+      const ordered = orderDestroy(state, targets, parentEdges);
+
+      // Backup: fetch each target's current actual values via the same fetchActual as plan/apply
+      // (404 → skip: already gone in CT, nothing to back up). A non-404 failure must ABORT before
+      // any DELETE — proceeding would irreversibly delete a target with no backup of its state.
+      const { actual, fetchErrors } = await fetchActual(client, ordered.map((k) => state.resources[k]!));
+      if (fetchErrors.length > 0) {
+        error(
+          `Backup fetch failed for: ${fetchErrors.join("; ")}. ` +
+            `Nothing was deleted — resolve the error (or wait out the outage) and re-run.`,
+        );
+        process.exitCode = 1;
+        return;
       }
       const backupPath = await writeBackup(resolveBackupDir(opts.backupDir, statePath), config.host, actual);
       info(`Backup written: ${backupPath}`);

src/config/context.ts

@@ -32,7 +32,11 @@ export interface ResourceInput {
    */
   parents?: string[];
   dependsOn?: string[];
-  /** Block `ct destroy` for this resource until the flag is removed. */
+  /**
+   * Block `ct destroy` for this resource. Mirrored to the state entry on `apply`,
+   * so protection survives the resource being dropped from config; clear it by
+   * setting `false` (or removing it) and re-applying before you destroy.
+   */
   preventDestroy?: boolean;
   [field: string]: unknown;
 }
@@ -89,7 +93,9 @@ function toDesired(type: string, input: ResourceInput): DesiredResource {
   // `parent` is an ordering hint only — a dependency edge, never a diffed/managed field
   // (its pre-hierarchy meaning; a `parent` may point at a campus). Group hierarchy is
   // managed opt-in via `parents`: `undefined` → unmanaged, `[]` → managed with no parents.
-  const parentKey = typeof parent === "string" && parent !== "" ? parent : undefined;
+  // `parent` is already narrowed to `string | null | undefined` by the guard above; `|| undefined`
+  // collapses null and "" (an empty parent is "no parent", never opt-in) to undefined.
+  const parentKey = parent || undefined;
   const parentKeys = parents !== undefined ? [...new Set(parents)] : undefined;
   const edges = [...new Set([...dependsOn, ...(parentKey ? [parentKey] : []), ...(parentKeys ?? [])])];
   return {

src/config/load.ts

@@ -12,11 +12,12 @@ import { createJiti } from "jiti";
 import { evaluateConfig, type ConfigModule } from "./context.js";
 import type { DesiredResource } from "../engine/types.js";
 import type { DesiredPermission } from "../permissions/types.js";
+import { resolveWithEnv } from "../util/resolve.js";
 
 export const DEFAULT_CONFIG_PATH = "ct.config.ts";
 
 export function resolveConfigPath(explicit?: string, env: NodeJS.ProcessEnv = process.env): string {
-  return explicit?.trim() || env.CT_CONFIG?.trim() || DEFAULT_CONFIG_PATH;
+  return resolveWithEnv(explicit, env.CT_CONFIG, DEFAULT_CONFIG_PATH);
 }
 
 export async function loadConfig(

src/engine/backup.ts

@@ -5,6 +5,23 @@
  */
 import { mkdir, writeFile } from "node:fs/promises";
 import { join } from "node:path";
+import { isSyntheticField } from "./synthetic.js";
+
+/**
+ * Drop synthetic pseudo-fields (`parents`, `dynamic`, …) from an actual record.
+ *
+ * `buildPlan` folds these into its `actual` map IN PLACE for diffing, and apply
+ * reuses that same map for the backup. They are internal logical-key sets, not
+ * real CT columns, so they are non-restorable noise in a backup — strip them so
+ * the file holds only real, restorable values (and matches destroy's clean backup).
+ */
+function realFields(fields: Record<string, unknown>): Record<string, unknown> {
+  const out: Record<string, unknown> = {};
+  for (const [k, v] of Object.entries(fields)) {
+    if (!isSyntheticField(k)) out[k] = v;
+  }
+  return out;
+}
 
 export async function writeBackup(
   dir: string,
@@ -18,7 +35,7 @@ export async function writeBackup(
   const payload = {
     host,
     capturedAt: now.toISOString(),
-    resources: Object.fromEntries(actual),
+    resources: Object.fromEntries([...actual].map(([key, fields]) => [key, realFields(fields)])),
   };
   await writeFile(path, `${JSON.stringify(payload, null, 2)}\n`, "utf8");
   return path;

src/engine/build.ts

@@ -7,7 +7,7 @@
  */
 import type { CtClient } from "../api/ctClient.js";
 import { CtApiError } from "../api/ctClient.js";
-import type { State } from "../state/state.js";
+import type { ManagedResource, State } from "../state/state.js";
 import type { DesiredResource, Plan } from "./types.js";
 import { RESOURCES } from "../resources/registry.js";
 import { computePlan } from "./plan.js";
@@ -24,25 +24,36 @@ export interface BuildResult {
   fetchErrors: string[];
 }
 
-export interface BuildOptions {
-  /** Directory of the config file — `{ ref }` ruleset paths resolve relative to it (not the cwd). */
-  configDir?: string;
+export interface FetchActualResult {
+  /** Managed fields per logical key, for every resource that fetched cleanly (a 404 is omitted, not an error). */
+  actual: Map<string, Record<string, unknown>>;
+  /** Keys whose managed type has no registry entry — cannot be fetched/diffed, so left untouched. */
+  unresolved: Set<string>;
+  /** Keys whose fetch errored (non-404), mapped to a short status descriptor for the plan render. */
+  fetchFailed: Map<string, string>;
+  /** Human-readable fetch-error lines (non-404), one per failed key. */
+  fetchErrors: string[];
 }
 
-export async function buildPlan(
+/**
+ * Fetch the actual ChurchTools values of a set of managed resources, concurrently.
+ *
+ * Shared by `buildPlan` (plan/apply) and `destroy` (its pre-delete backup) so the
+ * two read actuals identically and cannot drift. A 404 means the resource vanished
+ * in CT — omitted from `actual` (the plan recreates/prunes; the backup skips it).
+ * A non-404 error is recorded (never thrown) so one bad fetch neither aborts a
+ * read-only plan nor blocks tearing down the other targets.
+ */
+export async function fetchActual(
   client: Pick<CtClient, "get">,
-  state: State,
-  desired: DesiredResource[],
-  opts: BuildOptions = {},
-): Promise<BuildResult> {
-  // Keyed by logical key (globally unique), not CT id (unique only within a type — the Mainz campus is id 0).
+  resources: readonly ManagedResource[],
+): Promise<FetchActualResult> {
   const actual = new Map<string, Record<string, unknown>>();
   const unresolved = new Set<string>();
-  // Keys whose fetch errored (non-404), mapped to a short status descriptor for the plan render.
   const fetchFailed = new Map<string, string>();
   const fetchErrors: string[] = [];
 
-  await mapConcurrent(Object.values(state.resources), FETCH_CONCURRENCY, async (managed) => {
+  await mapConcurrent(resources, FETCH_CONCURRENCY, async (managed) => {
     const spec = RESOURCES[managed.type];
     if (!spec) {
       unresolved.add(managed.key);
@@ -68,6 +79,26 @@ export async function buildPlan(
     }
   });
 
+  return { actual, unresolved, fetchFailed, fetchErrors };
+}
+
+export interface BuildOptions {
+  /** Directory of the config file — `{ ref }` ruleset paths resolve relative to it (not the cwd). */
+  configDir?: string;
+}
+
+export async function buildPlan(
+  client: Pick<CtClient, "get">,
+  state: State,
+  desired: DesiredResource[],
+  opts: BuildOptions = {},
+): Promise<BuildResult> {
+  // Keyed by logical key (globally unique), not CT id (unique only within a type — the Mainz campus is id 0).
+  const { actual, unresolved, fetchFailed, fetchErrors } = await fetchActual(
+    client,
+    Object.values(state.resources),
+  );
+
   // Synthetic sub-resource fields (parents, dynamic, …) fold into the diff on both sides.
   const folded = await foldSynthetic({ client, state, desired, actual, configDir: opts.configDir });
   fetchErrors.push(...folded.errors);

src/engine/execute.ts

@@ -9,7 +9,7 @@
  * their own dedicated endpoints, not the owning resource's body — see synthetic.ts.
  */
 import type { CtClient } from "../api/ctClient.js";
-import type { State } from "../state/state.js";
+import type { ManagedResource, State } from "../state/state.js";
 import { upsert, saveState } from "../state/state.js";
 import type { FieldChange, Plan } from "./types.js";
 import { RESOURCES } from "../resources/registry.js";
@@ -31,6 +31,12 @@ export interface ExecuteResult {
   failed?: { key: string; message: string };
 }
 
+/** Mirror the config's `preventDestroy` onto a state entry (kept absent, not `false`, when unset). */
+function mirrorPreventDestroy(entry: ManagedResource, flag: boolean | undefined): void {
+  if (flag) entry.preventDestroy = true;
+  else delete entry.preventDestroy;
+}
+
 /** The managed field snapshot after a write: base ∪ changed fields, minus any synthetic sub-resource fields. */
 function snapshotFromChanges(base: Record<string, unknown>, changes: FieldChange[]): Record<string, unknown> {
   const snap = { ...base };
@@ -62,6 +68,15 @@ export async function executePlan(plan: Plan, deps: ExecuteDeps): Promise<Execut
       continue;
     }
     if (item.action === "no-op") {
+      // A preventDestroy toggle alone yields a no-op plan (the flag is never a diffed field),
+      // so reconcile it here too — otherwise adding protection wouldn't reach state until some
+      // other field changed. Only clean (note-less) no-ops are desired-side and safe to touch;
+      // stale/unresolved/fetch-failed no-ops are delete-side or undiffable — leave them alone.
+      const entry = state.resources[item.key];
+      if (!item.note && entry && entry.preventDestroy !== (item.preventDestroy || undefined)) {
+        mirrorPreventDestroy(entry, item.preventDestroy);
+        await save(statePath, state);
+      }
       continue;
     }
     const spec = RESOURCES[item.type];
@@ -88,6 +103,7 @@ export async function executePlan(plan: Plan, deps: ExecuteDeps): Promise<Execut
         // re-run POSTs another duplicate. Drop the stale entry: a create owns its key outright.
         delete state.resources[item.key];
         upsert(state, { type: item.type, id: res.id, key: item.key, fields: body }, now());
+        mirrorPreventDestroy(state.resources[item.key]!, item.preventDestroy);
         await save(statePath, state);
         await applySyntheticFields(client, state, res.id, item.changes);
         created.push(item.key);
@@ -112,6 +128,7 @@ export async function executePlan(plan: Plan, deps: ExecuteDeps): Promise<Execut
           await client.request(spec.updateMethod, path, body);
         }
         upsert(state, { type: item.type, id, key: item.key, fields: snapshot }, now());
+        mirrorPreventDestroy(state.resources[item.key]!, item.preventDestroy);
         await save(statePath, state);
         await applySyntheticFields(client, state, id, item.changes);
         updated.push(item.key);