fix: build update bodies from fetched actuals, desired-side hierarchy gate, plan/apply robustness

src/api/ctClient.ts

@@ -15,6 +15,7 @@
  */
 import { type CtConfig } from "../config.js";
 import { fetchWithRetry } from "./http.js";
+import { meetsMinVersion, MIN_CT_VERSION, type CtInfo } from "./version.js";
 
 export interface WhoAmI {
   id: number;
@@ -39,13 +40,42 @@ type Json = Record<string, unknown>;
 export class CtClient {
   private cookie: string | null = null;
   private csrfToken: string | null = null;
+  private ctVersion: string | null = null;
 
   constructor(private readonly config: CtConfig) {}
 
   get host(): string {
     return this.config.host;
   }
 
+  /**
+   * Hard-fail if the ChurchTools instance is below the minimum version the CLI
+   * requires (group hierarchy / metadata CRUD need v3.96+). One `/info` GET,
+   * cached so repeated calls in a session cost nothing. plan/apply/destroy call
+   * this via {@link authedSession} — a half-applied structure from a stale
+   * instance is exactly what the gate exists to prevent.
+   */
+  async assertMinVersion(min: string = MIN_CT_VERSION): Promise<void> {
+    if (this.ctVersion === null) {
+      const info = await this.get<CtInfo>("/info");
+      this.ctVersion = info?.version ?? "";
+    }
+    const version = this.ctVersion;
+    if (!version) {
+      throw new CtApiError(
+        `ChurchTools did not report a version (GET /info) — cannot verify the required minimum ${min}.`,
+        0,
+        null,
+      );
+    }
+    if (!meetsMinVersion(version, min)) {
+      throw new Error(
+        `ChurchTools ${version} is below the required minimum ${min}. ` +
+          `Upgrade ChurchTools before running plan/apply/destroy.`,
+      );
+    }
+  }
+
   /** Run the login-token handshake and cache the session cookie + CSRF token. */
   async authenticate(loginToken: string): Promise<WhoAmI> {
     const url = `${this.config.host}/api/whoami?login_token=${encodeURIComponent(loginToken)}`;
@@ -103,7 +133,19 @@ export class CtClient {
     if (res.status === 204) {
       return undefined as T;
     }
-    const parsed = (await res.json()) as { data?: T };
+    // Any 2xx may carry an empty or non-JSON body (DELETEs commonly do). A bare
+    // res.json() there throws a raw SyntaxError naming no request. Read the text
+    // first: empty → undefined; unparseable → a CtApiError that names method+path.
+    const text = await res.text();
+    if (text.trim() === "") {
+      return undefined as T;
+    }
+    let parsed: { data?: T };
+    try {
+      parsed = JSON.parse(text) as { data?: T };
+    } catch {
+      throw new CtApiError(`${method} ${path} returned a non-JSON body`, res.status, text);
+    }
     return (parsed.data ?? parsed) as T;
   }
 

src/api/session.ts

@@ -19,5 +19,8 @@ export async function authedSession(): Promise<AuthedSession> {
   }
   const client = new CtClient(config);
   const me = await client.authenticate(token);
+  // Hard-fail below the minimum CT version before any command reads or writes —
+  // a stale instance half-applies (tier-0 writes succeed, hierarchy endpoints 404).
+  await client.assertMinVersion();
   return { client, me };
 }

src/engine/build.ts

@@ -38,6 +38,8 @@ export async function buildPlan(
   // Keyed by logical key (globally unique), not CT id (unique only within a type — the Mainz campus is id 0).
   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) => {
@@ -57,7 +59,10 @@ export async function buildPlan(
         return; // vanished in CT — the plan will propose recreating (or pruning) it
       }
       // A read-only plan should not abort on one bad fetch: record it, keep going, flag the plan as partial.
+      // Track the key separately from a real 404 so computePlan renders it as a fetch failure, not a recreate.
       const message = err instanceof Error ? err.message : String(err);
+      const status = err instanceof CtApiError ? String(err.status) : "error";
+      fetchFailed.set(managed.key, status);
       fetchErrors.push(`${managed.type}.${managed.key} (#${managed.id}): ${message}`);
       warn(`Failed to fetch ${managed.type}.${managed.key} (#${managed.id}): ${message}`);
     }
@@ -66,6 +71,6 @@ export async function buildPlan(
   // 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);
-  const plan = computePlan(folded.desired, state, actual, { unresolved });
+  const plan = computePlan(folded.desired, state, actual, { unresolved, fetchFailed });
   return { plan, actual, fetchErrors };
 }

src/engine/execute.ts

@@ -96,13 +96,20 @@ export async function executePlan(plan: Plan, deps: ExecuteDeps): Promise<Execut
         if (id === null) {
           throw new Error("update item has no id");
         }
-        const base = state.resources[item.key]?.fields ?? {};
-        const snapshot = snapshotFromChanges(base, item.changes);
+        // Base the write body on the FETCHED ACTUAL, not the stale state snapshot: a field that
+        // drifted in the CT UI but isn't in `changes` must pass through, never be reverted (#27).
+        // The post-write snapshot (actual ∪ changes) is what CT holds afterward, so state records
+        // exactly that regardless of verb.
+        const actualFields = item.actual ?? state.resources[item.key]?.fields ?? {};
+        const snapshot = snapshotFromChanges(actualFields, item.changes);
         const hasFieldChange = item.changes.some((c) => !isSyntheticField(c.field));
         if (hasFieldChange) {
+          // PATCH resources take only the changed fields (unchanged/drifted siblings are left alone);
+          // PUT resources replace the whole object, so send actual ∪ changes to preserve those siblings.
+          const body = spec.updateMethod === "PATCH" ? snapshotFromChanges({}, item.changes) : snapshot;
           const path = spec.itemPath(id);
           assertNotPeople(path);
-          await client.request(spec.updateMethod, path, snapshot);
+          await client.request(spec.updateMethod, path, body);
         }
         upsert(state, { type: item.type, id, key: item.key, fields: snapshot }, now());
         await save(statePath, state);

src/engine/plan.ts

@@ -71,6 +71,13 @@ export function driftFields(
 export interface ComputePlanOptions {
   /** Logical keys whose managed type has no registry entry — cannot be fetched, so left untouched (not recreated/deleted). */
   unresolved?: ReadonlySet<string>;
+  /**
+   * Logical keys whose actual value could not be fetched (a non-404 error). Mapped
+   * to a short status descriptor (e.g. "500"). These are NOT vanished resources, so
+   * they must be excluded from create/recreate/stale classification and surfaced as
+   * a fetch failure — otherwise a transient 500 reads as "recreate — missing in CT".
+   */
+  fetchFailed?: ReadonlyMap<string, string>;
 }
 
 export function computePlan(
@@ -80,6 +87,7 @@ export function computePlan(
   opts: ComputePlanOptions = {},
 ): Plan {
   const unresolved = opts.unresolved ?? new Set<string>();
+  const fetchFailed = opts.fetchFailed ?? new Map<string, string>();
 
   for (const d of desired) {
     if (!isKnownType(d.type)) {
@@ -124,6 +132,19 @@ export function computePlan(
       });
       continue;
     }
+    if (fetchFailed.has(d.key)) {
+      // Fetch errored (non-404). We can't diff it, and it is NOT gone — do not propose a recreate.
+      updates.push({
+        type: d.type,
+        key: d.key,
+        id: managed.id,
+        action: "no-op",
+        changes: [],
+        note: "fetch-failed",
+        detail: fetchFailed.get(d.key),
+      });
+      continue;
+    }
     const a = actual.get(d.key);
     if (!a) {
       creates.push({
@@ -144,6 +165,8 @@ export function computePlan(
       id: managed.id,
       action: changes.length > 0 ? "update" : "no-op",
       changes,
+      // The fetched actual — the write body is built from this, not the stale state snapshot (#27).
+      actual: a,
       drift: drift.length > 0 ? drift : undefined,
     });
   }
@@ -163,6 +186,19 @@ export function computePlan(
       });
       continue;
     }
+    if (fetchFailed.has(managed.key)) {
+      // Fetch errored — we can't tell if it is gone, so do not propose a stale-prune.
+      deletes.push({
+        type: managed.type,
+        key: managed.key,
+        id: managed.id,
+        action: "no-op",
+        changes: [],
+        note: "fetch-failed",
+        detail: fetchFailed.get(managed.key),
+      });
+      continue;
+    }
     const a = actual.get(managed.key);
     if (!a) {
       // Already gone from ChurchTools but still in the state file — surface it so the user prunes state (not a silent no-op).

src/engine/render.ts

@@ -27,9 +27,16 @@ export function renderPlan(plan: Plan): string {
   const drifted = plan.items.filter((i) => i.drift && i.drift.length > 0);
   const stale = plan.items.filter((i) => i.note === "stale");
   const unresolved = plan.items.filter((i) => i.note === "unresolved-type");
+  const fetchFailed = plan.items.filter((i) => i.note === "fetch-failed");
   const lines: string[] = [];
 
-  if (changed.length === 0 && drifted.length === 0 && stale.length === 0 && unresolved.length === 0) {
+  if (
+    changed.length === 0 &&
+    drifted.length === 0 &&
+    stale.length === 0 &&
+    unresolved.length === 0 &&
+    fetchFailed.length === 0
+  ) {
     return pc.green("No changes. Desired state matches ChurchTools.");
   }
 
@@ -74,6 +81,14 @@ export function renderPlan(plan: Plan): string {
     }
   }
 
+  if (fetchFailed.length > 0) {
+    lines.push("");
+    lines.push(pc.yellow("Fetch failed (could not read from ChurchTools — diff unavailable, left untouched):"));
+    for (const item of fetchFailed) {
+      lines.push(`  ? ${item.type}.${item.key} (#${item.id}) — fetch failed (${item.detail ?? "error"})`);
+    }
+  }
+
   const s = summarize(plan);
   lines.push("");
   lines.push(pc.bold(`Plan: ${s.create} to create, ${s.update} to update, ${s.delete} to delete.`));

src/engine/synthetic.ts

@@ -45,8 +45,13 @@ function resolveId(state: State, key: string): number {
 const parentsField: SyntheticField = {
   field: "parents",
   async fold({ client, state, desired, actual }) {
-    const hasManagedGroups = Object.values(state.resources).some((m) => m.type === "group");
-    if (!hasManagedGroups) return { desired, errors: [] };
+    // Gate on the DESIRED side, not the pre-apply state: on a fresh state the
+    // groups don't exist yet, so a state-side gate returns early and the first
+    // apply drops every declared hierarchy edge (flat groups, exit 0). Gating on
+    // "some desired group opted into parents" makes the first apply create edges,
+    // and still skips the /groups/hierarchies fetch when nobody opts in.
+    const optedIn = desired.some((d) => d.type === "group" && d.parents !== undefined);
+    if (!optedIn) return { desired, errors: [] };
     try {
       const raw = await client.get<HierarchyEntry[]>("/groups/hierarchies");
       const parentIds = parentIdsByGroupId(Array.isArray(raw) ? raw : []);

src/engine/types.ts

@@ -35,8 +35,10 @@ export type PlanAction = "create" | "update" | "delete" | "no-op";
  *  - `recreate`         — desired + managed, but vanished from ChurchTools (a create that replaces a dead id).
  *  - `stale`            — managed + dropped from config + already gone from ChurchTools (nothing to delete; prune from state).
  *  - `unresolved-type`  — managed but its type has no registry entry, so it cannot be fetched/diffed (left untouched).
+ *  - `fetch-failed`     — managed but its actual value could not be fetched (non-404 error); NOT a vanish, so it is
+ *                         excluded from create/recreate/stale classification and rendered as a fetch failure.
  */
-export type PlanNote = "recreate" | "stale" | "unresolved-type";
+export type PlanNote = "recreate" | "stale" | "unresolved-type" | "fetch-failed";
 
 export interface FieldChange {
   field: string;
@@ -52,10 +54,18 @@ export interface PlanItem {
   action: PlanAction;
   /** For create: every desired field; update: only the differences; delete/no-op: empty. */
   changes: FieldChange[];
+  /**
+   * The fetched actual managed fields (updates only). The write body is built from
+   * THIS, not the stale state snapshot — so a field that drifted in the CT UI but
+   * isn't in `changes` passes through untouched instead of being reverted (#27).
+   */
+  actual?: Record<string, unknown>;
   /** Manual changes in ChurchTools since adoption (last-known snapshot vs actual). */
   drift?: FieldChange[];
   /** A non-standard state the plan must surface (see {@link PlanNote}). */
   note?: PlanNote;
+  /** Extra human-readable context for a note (e.g. the HTTP status behind `fetch-failed`). */
+  detail?: string;
 }
 
 export interface Plan {

tests/build.test.ts

@@ -37,5 +37,35 @@ describe("buildPlan", () => {
     const item = plan.items.find((i) => i.key === "mainz")!;
     expect(item.action).toBe("update");
     expect(item.changes).toEqual([{ field: "name", from: "Mainz", to: "Mainz City" }]);
+    // The fetched actual is threaded onto the update item so execute builds the body from it (#27).
+    expect(item.actual).toEqual({ name: "Mainz", shorty: "MZ" });
+  });
+
+  it("renders a transient (non-404) fetch error as fetch-failed, not a recreate (#33)", async () => {
+    const state = emptyState("https://x.church.tools");
+    state.resources.mainz = {
+      type: "campus",
+      id: 0,
+      key: "mainz",
+      fields: { name: "Mainz", shorty: "MZ" },
+      adoptedAt: "t",
+      updatedAt: "t",
+    };
+    const desired: DesiredResource[] = [
+      { type: "campus", key: "mainz", fields: { name: "Mainz", shorty: "MZ" }, dependsOn: [] },
+    ];
+    const client = {
+      get: async <T>(): Promise<T> => {
+        throw new CtApiError("boom", 500, null);
+      },
+    };
+    const { plan, fetchErrors } = await buildPlan(client, state, desired);
+    expect(fetchErrors.length).toBe(1);
+    const item = plan.items.find((i) => i.key === "mainz")!;
+    // NOT classified as a create/recreate — a healthy resource must never be advised for recreation.
+    expect(item.action).toBe("no-op");
+    expect(item.note).toBe("fetch-failed");
+    expect(item.detail).toBe("500");
+    expect(plan.items.some((i) => i.action === "create")).toBe(false);
   });
 });

tests/ctClient.test.ts

@@ -54,4 +54,49 @@ describe("CtClient", () => {
     expect(headers.get("CSRF-Token")).toBe("csrf-xyz");
     expect(headers.get("Cookie")).toContain("s=1");
   });
+
+  async function authedClient(): Promise<{ client: CtClient; fetchMock: ReturnType<typeof vi.fn<typeof fetch>> }> {
+    const fetchMock = vi
+      .fn<typeof fetch>()
+      .mockResolvedValueOnce(jsonResponse({ data: { id: 1 } }, { setCookie: "s=1; Path=/" }))
+      .mockResolvedValueOnce(jsonResponse({ data: "csrf" }));
+    vi.stubGlobal("fetch", fetchMock);
+    const client = new CtClient({ host: "https://eqrm.church.tools" });
+    await client.authenticate("tok");
+    return { client, fetchMock };
+  }
+
+  it("assertMinVersion passes on a supported instance and refuses an old one", async () => {
+    const { client, fetchMock } = await authedClient();
+    fetchMock.mockResolvedValueOnce(jsonResponse({ data: { version: "3.123.0" } }));
+    await expect(client.assertMinVersion()).resolves.toBeUndefined();
+
+    const { client: old, fetchMock: oldFetch } = await authedClient();
+    oldFetch.mockResolvedValueOnce(jsonResponse({ data: { version: "3.95.0" } }));
+    await expect(old.assertMinVersion()).rejects.toThrow(/3\.95\.0 is below the required minimum/);
+  });
+
+  it("assertMinVersion fetches /info only once (cached)", async () => {
+    const { client, fetchMock } = await authedClient();
+    fetchMock.mockResolvedValueOnce(jsonResponse({ data: { version: "3.123.0" } }));
+    await client.assertMinVersion();
+    await client.assertMinVersion();
+    const infoCalls = fetchMock.mock.calls.filter((c) => String(c[0]).includes("/api/info"));
+    expect(infoCalls.length).toBe(1);
+  });
+
+  it("returns undefined for an empty 2xx body instead of throwing a raw SyntaxError", async () => {
+    const { client, fetchMock } = await authedClient();
+    fetchMock.mockResolvedValueOnce(new Response("", { status: 200 }));
+    await expect(client.request("DELETE", "/groups/1/parents/2")).resolves.toBeUndefined();
+  });
+
+  it("wraps a non-JSON 2xx body in a CtApiError naming method + path", async () => {
+    const { client, fetchMock } = await authedClient();
+    fetchMock.mockResolvedValueOnce(new Response("<html>oops</html>", { status: 200 }));
+    await expect(client.request("PUT", "/campuses/0")).rejects.toMatchObject({
+      name: "CtApiError",
+      message: expect.stringContaining("PUT /campuses/0"),
+    });
+  });
 });