fix: destroy 404 wedge, adopt group-role DSL collision, dynamic normalizer + demote convergence

src/commands/destroy.ts

@@ -1,6 +1,6 @@
 import { Command } from "commander";
 import { authedSession } from "../api/session.js";
-import { CtApiError } from "../api/ctClient.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";
@@ -105,28 +105,57 @@ export function destroyCommand(): Command {
         return;
       }
 
-      for (const key of ordered) {
-        const managed = state.resources[key]!;
-        const spec = RESOURCES[managed.type];
-        if (!spec) {
-          error(`No write spec for type "${managed.type}" — skipping ${key}.`);
-          continue;
-        }
-        const path = spec.itemPath(managed.id);
-        assertNotPeople(path);
-        try {
-          await client.request("DELETE", path);
-        } catch (err) {
-          const message = err instanceof Error ? err.message : String(err);
-          error(
-            `Stopped at ${key}: ${message}. Already-deleted targets are removed from state — re-run with the remaining targets to resume.`,
-          );
-          process.exitCode = 1;
-          return;
-        }
+      await runDeleteLoop({ client, state, statePath, ordered });
+    });
+}
+
+export interface DeleteLoopCtx {
+  client: Pick<CtClient, "request">;
+  state: State;
+  statePath: string;
+  ordered: string[];
+  /** Injection seam for tests; defaults to the real state writer. */
+  save?: (path: string, state: State) => Promise<void>;
+}
+
+/**
+ * Delete each ordered target, removing it from state and saving after each success.
+ *
+ * A 404 means the target was already deleted in ChurchTools (e.g. by hand in the UI):
+ * treat it as success-with-note — drop the state entry, save, and continue to the next
+ * target. Any non-404 error stops the run with state saved up to that point, so a re-run
+ * can resume with the remaining targets. (Mirrors the backup loop's 404 tolerance.)
+ */
+export async function runDeleteLoop(ctx: DeleteLoopCtx): Promise<void> {
+  const { client, state, statePath, ordered } = ctx;
+  const save = ctx.save ?? saveState;
+  for (const key of ordered) {
+    const managed = state.resources[key]!;
+    const spec = RESOURCES[managed.type];
+    if (!spec) {
+      error(`No write spec for type "${managed.type}" — skipping ${key}.`);
+      continue;
+    }
+    const path = spec.itemPath(managed.id);
+    assertNotPeople(path);
+    try {
+      await client.request("DELETE", path);
+    } catch (err) {
+      if (err instanceof CtApiError && err.status === 404) {
         delete state.resources[key];
-        await saveState(statePath, state);
-        success(`Destroyed ${managed.type}.${key} (#${managed.id})`);
+        await save(statePath, state);
+        success(`${managed.type}.${key} (#${managed.id}) already deleted in ChurchTools — removed from state`);
+        continue;
       }
-    });
+      const message = err instanceof Error ? err.message : String(err);
+      error(
+        `Stopped at ${key}: ${message}. State saved up to this point — re-run with the remaining targets to resume.`,
+      );
+      process.exitCode = 1;
+      return;
+    }
+    delete state.resources[key];
+    await save(statePath, state);
+    success(`Destroyed ${managed.type}.${key} (#${managed.id})`);
+  }
 }

src/config/context.ts

@@ -50,6 +50,9 @@ export interface ConfigContext {
   ageGroup(input: ResourceInput): void;
   targetGroup(input: ResourceInput): void;
   relationshipType(input: ResourceInput): void;
+  /** Master-data group role (`/group/roles`). Named `roleDefinition` to avoid colliding
+   *  with the `groupRole` permission function below. */
+  roleDefinition(input: ResourceInput): void;
   groupRole(input: PermissionInput): void;
   groupTypeRole(input: PermissionInput): void;
 }
@@ -185,6 +188,7 @@ export function createContext(): {
     ageGroup: define("age-group"),
     targetGroup: define("target-group"),
     relationshipType: define("relationship-type"),
+    roleDefinition: define("group-role"),
     groupRole: definePermission("group_role"),
     groupTypeRole: definePermission("group_type_role"),
   };

src/engine/dynamic.ts

@@ -25,15 +25,27 @@ export function stripCosmeticLabels(node: unknown): unknown {
   return node;
 }
 
-/** Coerce numeric-string leaves to numbers (CT is int/string-inconsistent for `var` values). */
+/**
+ * Coerce numeric-string leaves to numbers (CT is int/string-inconsistent for `var` values).
+ *
+ * Only *canonical* integer strings are coerced: no leading zeros (`/^(-?[1-9]\d*|0)$/`) and
+ * within `Number.MAX_SAFE_INTEGER`. This leaves semantic strings that merely look numeric —
+ * leading-zero zip codes like `'01067'`, and >2^53 digit strings that would lose precision —
+ * untouched, so they round-trip byte-identical through normalize + write-back instead of being
+ * silently retyped (which broke their JSONLogic string comparisons). A canonical `"5"` still
+ * coerces to `5`, so a `5` vs `"5"` int/string pair still diffs equal.
+ */
 export function coerceScalars(node: unknown): unknown {
   if (Array.isArray(node)) return node.map(coerceScalars);
   if (node && typeof node === "object") {
     const out: Record<string, unknown> = {};
     for (const [k, v] of Object.entries(node as Record<string, unknown>)) out[k] = coerceScalars(v);
     return out;
   }
-  if (typeof node === "string" && /^-?\d+$/.test(node)) return Number.parseInt(node, 10);
+  if (typeof node === "string" && /^(-?[1-9]\d*|0)$/.test(node)) {
+    const n = Number.parseInt(node, 10);
+    if (Number.isSafeInteger(n)) return n;
+  }
   return node;
 }
 

src/engine/synthetic.ts

@@ -114,18 +114,31 @@ const dynamicField: SyntheticField = {
         errors.push(`dynamic ${managed.key} status (#${managed.id}): ${message}`);
       }
     }
-    const augmented = desired.map((d) =>
-      d.type === "group" && d.dynamic !== undefined
-        ? { ...d, fields: { ...d.fields, dynamic: normalizeDynamic({ status: d.dynamic.status, ruleset: resolveRulesetRef(d.dynamic.ruleset, configDir, d.key) }) } }
-        : d,
-    );
+    const augmented = desired.map((d) => {
+      if (d.type !== "group" || d.dynamic === undefined) return d;
+      // Demote-to-none: fold to the SAME sentinel the actual side uses for a non-dynamic group
+      // ({ status: "none", ruleset: {} }). The docs tell users to KEEP the dynamic block when
+      // demoting, so their authored ruleset is still present here — but folding it would diff
+      // forever against the sentinel actual. Collapsing both sides makes a demoted group converge.
+      const dynamic =
+        d.dynamic.status === "none"
+          ? { status: "none" as DynamicStatus, ruleset: {} }
+          : normalizeDynamic({ status: d.dynamic.status, ruleset: resolveRulesetRef(d.dynamic.ruleset, configDir, d.key) });
+      return { ...d, fields: { ...d.fields, dynamic } };
+    });
     return { desired: augmented, errors };
   },
   async apply({ client, id, change }) {
     const to = change.to as { status: DynamicStatus; ruleset: Record<string, unknown> } | undefined;
     if (!to || to.status === "none") {
       assertNotPeople(`/dynamicgroups/${id}/ruleset`);
-      await client.request("DELETE", `/dynamicgroups/${id}/ruleset`);
+      // A group that was never dynamic (or is already demoted) has no ruleset to delete — CT 404s.
+      // Tolerate that: the desired end-state (no ruleset) already holds, so treat it as done.
+      try {
+        await client.request("DELETE", `/dynamicgroups/${id}/ruleset`);
+      } catch (err) {
+        if (!(err instanceof CtApiError && err.status === 404)) throw err;
+      }
       assertNotPeople(`/dynamicgroups/${id}/status`);
       await client.request("PUT", `/dynamicgroups/${id}/status`, { dynamicGroupStatus: "none" });
       return;

src/resources/registry.ts

@@ -18,6 +18,12 @@ export interface AdoptableResource {
   deriveKey: (resource: Record<string, unknown>) => string;
   /** The subset of fields we manage — the desired-state baseline. */
   managedFields: (resource: Record<string, unknown>) => Record<string, unknown>;
+  /**
+   * DSL function name `configSnippet` emits for this type. Defaults to the camelCase of
+   * the type name. Set it when the natural camelCase collides with another DSL surface
+   * (e.g. `group-role` → `roleDefinition`, because `groupRole` is the permission function).
+   */
+  dslName?: string;
 }
 
 /** Build a full spec, deriving `itemPath` from the collection path so each entry names its path once. */
@@ -104,6 +110,9 @@ export const RESOURCES: Record<string, AdoptableResource> = {
     updateMethod: "PUT",
     deriveKey: (r) => slug(str(r, "name")),
     managedFields: (r) => ({ name: r.name, nameTranslated: r.nameTranslated, groupTypeId: r.groupTypeId }),
+    // `groupRole` is taken by the permissions DSL (`ct.groupRole` = definePermission("group_role")),
+    // so the master-data role resource declares under a distinct name.
+    dslName: "roleDefinition",
   }),
 };
 
@@ -116,9 +125,18 @@ export function resourceType(type: string): AdoptableResource {
   return entry;
 }
 
-/** Render a config entry as a TS-as-code call, e.g. `campus({ key: "mainz", name: "Mainz" })`. */
+/** Camel-case a hyphenated type name: `group-type` → `groupType`. */
+function camelCase(type: string): string {
+  return type.replace(/-([a-z])/g, (_, c: string) => c.toUpperCase());
+}
+
+/**
+ * Render a config entry as a TS-as-code call, e.g. `campus({ key: "mainz", name: "Mainz" })`.
+ * The function name comes from the registry entry's `dslName` (default: camelCase of the type),
+ * so the emitted snippet always names an actual `ConfigContext` function — never a colliding one.
+ */
 export function configSnippet(type: string, key: string, fields: Record<string, unknown>): string {
-  const fn = type.replace(/-([a-z])/g, (_, c: string) => c.toUpperCase());
+  const fn = RESOURCES[type]?.dslName ?? camelCase(type);
   return `${fn}(${tsObject({ key, ...fields })});`;
 }
 

tests/context.test.ts

@@ -107,11 +107,22 @@ describe("config context", () => {
     ct.ageGroup({ key: "d", name: "d" });
     ct.targetGroup({ key: "e", name: "e" });
     ct.relationshipType({ key: "f", name: "f" });
+    ct.roleDefinition({ key: "g", name: "g", groupTypeId: 2 });
     for (const r of resources) {
       expect(isKnownType(r.type), `type "${r.type}" is missing from TYPE_TIER`).toBe(true);
     }
   });
 
+  it("declares the master-data group role as a plannable group-role resource (not the permission surface)", () => {
+    const { ct, resources, permissions } = createContext();
+    ct.roleDefinition({ key: "leiter", name: "Leiter", groupTypeId: 2 });
+    expect(permissions).toEqual([]); // roleDefinition is a resource, never a permission grant
+    expect(resources).toEqual([
+      expect.objectContaining({ type: "group-role", key: "leiter", fields: { name: "Leiter", groupTypeId: 2 } }),
+    ]);
+    expect(isKnownType(resources[0]!.type)).toBe(true); // has an apply tier → plannable
+  });
+
   it("evaluateConfig runs a module against a fresh context (blueprints + loops)", async () => {
     const { resources } = await evaluateConfig((ct) => {
       for (const c of ["mainz", "berlin"]) {

tests/destroy.test.ts

@@ -1,6 +1,15 @@
-import { describe, it, expect } from "vitest";
-import { parseTargets, orderDestroy } from "../src/commands/destroy.js";
-import { emptyState } from "../src/state/state.js";
+import { describe, it, expect, vi } from "vitest";
+import { parseTargets, orderDestroy, runDeleteLoop } from "../src/commands/destroy.js";
+import { emptyState, type State } from "../src/state/state.js";
+import { CtApiError, type CtClient } from "../src/api/ctClient.js";
+
+function stateWith(...entries: Array<{ key: string; type: string; id: number }>): State {
+  const state = emptyState("h");
+  for (const e of entries) {
+    state.resources[e.key] = { type: e.type, id: e.id, key: e.key, fields: {}, adoptedAt: "t", updatedAt: "t" };
+  }
+  return state;
+}
 
 describe("parseTargets", () => {
   it("splits commas, trims, and dedupes", () => {
@@ -30,3 +39,42 @@ describe("orderDestroy", () => {
     expect(orderDestroy(state, ["mainz", "team"])).toEqual(["team", "mainz"]);
   });
 });
+
+describe("runDeleteLoop", () => {
+  const asClient = (request: unknown) => ({ request }) as unknown as Pick<CtClient, "request">;
+
+  it("treats a 404 (already deleted in CT) as success: removes from state and continues to the next target", async () => {
+    const state = stateWith({ key: "gone", type: "group", id: 1 }, { key: "other", type: "group", id: 2 });
+    const request = vi.fn(async (_m: string, path: string) => {
+      if (path === "/groups/1") throw new CtApiError("Not Found", 404, null);
+      return {};
+    });
+    const save = vi.fn(async () => {});
+
+    await runDeleteLoop({ client: asClient(request), state, statePath: "s.json", ordered: ["gone", "other"], save });
+
+    expect(state.resources.gone).toBeUndefined();  // already-deleted target removed from state
+    expect(state.resources.other).toBeUndefined();  // loop continued and deleted the rest
+    expect(request).toHaveBeenCalledTimes(2);
+    expect(save).toHaveBeenCalledTimes(2);
+  });
+
+  it("stops on a non-404 error with state saved up to that point and leaves later targets untouched", async () => {
+    const state = stateWith({ key: "boom", type: "group", id: 1 }, { key: "later", type: "group", id: 2 });
+    const request = vi.fn(async (_m: string, path: string) => {
+      if (path === "/groups/1") throw new CtApiError("Server Error", 500, null);
+      return {};
+    });
+    const save = vi.fn(async () => {});
+    const prevExit = process.exitCode;
+
+    await runDeleteLoop({ client: asClient(request), state, statePath: "s.json", ordered: ["boom", "later"], save });
+
+    expect(state.resources.boom).toBeDefined();   // not removed — the DELETE failed
+    expect(state.resources.later).toBeDefined();  // never reached
+    expect(request).toHaveBeenCalledTimes(1);     // stopped at the first target
+    expect(save).not.toHaveBeenCalled();
+    expect(process.exitCode).toBe(1);
+    process.exitCode = prevExit;
+  });
+});

tests/dynamic.test.ts

@@ -24,6 +24,20 @@ describe("coerceScalars", () => {
     expect(coerceScalars({ "==": [{ var: "groupmember.groupMemberStatus" }, "active"] }))
       .toEqual({ "==": [{ var: "groupmember.groupMemberStatus" }, "active"] });
   });
+  it("leaves leading-zero and >2^53 numeric strings as strings (no corruption, no precision loss)", () => {
+    // A leading-zero zip code is a semantic string; parseInt would drop the zero and break the compare.
+    expect(coerceScalars({ "==": [{ var: "person.zip" }, "01067"] }))
+      .toEqual({ "==": [{ var: "person.zip" }, "01067"] });
+    // A digit string beyond MAX_SAFE_INTEGER can't be represented exactly — must stay a string.
+    const big = "90071992547409910"; // > 2^53
+    expect(coerceScalars({ "==": [{ var: "x.id" }, big] }))
+      .toEqual({ "==": [{ var: "x.id" }, big] });
+    // Canonical ints still coerce, so a 5 vs "5" int/string pair keeps diffing equal.
+    expect(coerceScalars({ "==": [{ var: "x.n" }, "5"] }))
+      .toEqual({ "==": [{ var: "x.n" }, 5] });
+    expect(coerceScalars({ "==": [{ var: "x.n" }, "0"] }))
+      .toEqual({ "==": [{ var: "x.n" }, 0] });
+  });
 });
 
 describe("normalizeRuleset", () => {
@@ -49,6 +63,15 @@ describe("normalizeRuleset", () => {
     expect(out.query).toEqual({ "==": [{ var: "ctgroup.id" }, 112] }); // filter operand coerced
   });
 
+  it("round-trips a leading-zero query leaf byte-identical (no retype on write-back)", () => {
+    const authored = { description: "Zip filter", query: { "==": [{ var: "person.zip" }, "01067"] }, process: {} };
+    const once = normalizeRuleset(authored);
+    // The zip leaf survives normalization untouched — apply PUTs `to.ruleset`, so any retype here
+    // would be written back to CT and silently break the JSONLogic string comparison.
+    expect(once.query).toEqual({ "==": [{ var: "person.zip" }, "01067"] });
+    expect(JSON.stringify(normalizeRuleset(once))).toBe(JSON.stringify(once)); // byte-identical + idempotent
+  });
+
   it("read-then-normalize of every live fixture is stable and label-free", () => {
     for (const name of ["ruleset-683", "ruleset-2022", "ruleset-1092"]) {
       const raw = JSON.parse(readFileSync(`tests/fixtures/dynamic/${name}.get.json`, "utf8")); // array shape

tests/registry.test.ts

@@ -1,5 +1,9 @@
 import { describe, it, expect } from "vitest";
+import { mkdtempSync, writeFileSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
 import { slug, resourceType, configSnippet, RESOURCES } from "../src/resources/registry.js";
+import { loadConfig } from "../src/config/load.js";
 
 describe("slug", () => {
   it("normalises names to underscore keys", () => {
@@ -62,6 +66,29 @@ describe("configSnippet", () => {
       'group({ key: "team", name: "Team" });',
     );
   });
+
+  it("emits the master-data role under roleDefinition, not the colliding permission name groupRole", () => {
+    expect(configSnippet("group-role", "leiter", { name: "Leiter", groupTypeId: 2 })).toBe(
+      'roleDefinition({ key: "leiter", name: "Leiter", groupTypeId: 2 });',
+    );
+  });
+});
+
+// Round-trip guarantee: whatever `adopt` prints for any adoptable type must be declarable.
+// Wrapping every registry type's snippet in a config and loading it through the real loader
+// pins the adopt→config contract so a DSL-name collision (issue #31) can't regress silently.
+describe("configSnippet round-trips through the config loader for every adoptable type", () => {
+  it.each(Object.keys(RESOURCES))("%s", async (type) => {
+    const key = `adopted_${type.replace(/-/g, "_")}`;
+    const snippet = configSnippet(type, key, { name: "Round Trip" });
+    const dir = mkdtempSync(join(tmpdir(), "ct-adopt-"));
+    writeFileSync(join(dir, "ct.config.ts"), `export default (ct) => { ct.${snippet} };`);
+
+    const { resources } = await loadConfig(join(dir, "ct.config.ts"));
+    const loaded = resources.find((r) => r.key === key);
+    expect(loaded, `snippet for "${type}" did not load: ${snippet}`).toBeDefined();
+    expect(loaded?.type).toBe(type);
+  });
 });
 
 describe("write specs", () => {