fix: apply correctness & safety — #113, #120, #121, #124, #126

docs/handbuch/blueprints.md

@@ -4,8 +4,8 @@ sources:
   - src/config/context.ts
   - src/engine/graph.ts
   - src/engine/hierarchy.ts
-sources_hash: bbadf1a728820f5c
-reviewed: 2026-08-13
+sources_hash: 37db6b8730470f73
+reviewed: 2026-08-17
 ---
 
 # Blueprints (parametrized, reusable config)

docs/handbuch/dynamic-groups.md

@@ -6,8 +6,8 @@ sources:
   - src/engine/dynamic.ts
   - src/engine/synthetic.ts
   - src/commands/adopt-group.ts
-sources_hash: 3fe32a405c9a8b4f
-reviewed: 2026-08-10
+sources_hash: 9a8c3091484e9258
+reviewed: 2026-08-17
 ---
 
 # Auto-groups (dynamic groups)
@@ -343,6 +343,28 @@ ruleset/status — any auto-group configuration it happens to have in
 ChurchTools stays completely invisible to `ct plan` / `ct apply`, exactly
 like any other resource this tool doesn't manage.
 
+## When the ruleset cannot be read (#126)
+
+A group's ruleset and status are read as a **sub-resource**, separately from the
+group's own `GET`. If that read fails for any reason other than a 404 — a 429
+under rate limiting is the common one — `ct` does **not** fold the declared
+ruleset into the diff on its own. Folding only the desired side would diff a
+declared ruleset against an absent actual and report the group as `to update`,
+manufacturing a change out of a transient failure.
+
+Instead the group is marked `fetch-failed`: it renders as a no-op under
+_"Fetch failed (could not read from ChurchTools — diff unavailable, left
+untouched)"_, the summary line carries
+`INCOMPLETE — N resource(s) could not be read.`, `ct plan` exits `1`, and
+`ct apply` refuses to run at all. A 404 on the ruleset keeps its old meaning —
+"this group is not a dynamic group" — and is not a failure.
+
+The distinction is the point: **"I could not read the current state of X" and
+"X differs from config" are different facts, and only the second belongs in a
+diff.** A plan is the artefact a human approves, so a fabricated `to update` in
+a PR comment is indistinguishable from a real drift and invites the same
+response.
+
 ## Applying and refreshing membership
 
 `ct apply` writes the ruleset (`PUT /dynamicgroups/{id}/ruleset`) and status
@@ -383,6 +405,16 @@ never the default. `ct refresh` only ever touches **managed** groups, refuses a
 group that has no ruleset on this host (rather than POSTing into a 404), and
 keeps going after a per-group failure (exiting non-zero).
 
+> **Fixed in #124.** "Which groups are dynamic on this host?" is answered by
+> `GET /dynamicgroups`, which returns a flat array of **bare group ids**
+> (`[159, 1698, …]`) — not objects. Both readers parsed it as
+> `Number(row.id ?? row.groupId)`, i.e. `NaN` for every element, so the id set
+> came out empty on every host: `ct refresh` answered "not a dynamic group" for
+> every group and could not succeed at all, and `ct coverage` reported
+> `dynamic: 0` on an instance with 70 auto-groups (#113 — the same bug read a
+> second time). There is now one parser for that endpoint
+> (`src/api/dynamicGroups.ts`) with a test pinning the scalar shape.
+
 > **The scheduler ping is NOT fired by `ct`.** ChurchTools' admin cron page hits
 > `GET https://<host>/?q=cron&standby=true`, which runs **every due scheduled
 > job on the instance** — far beyond auto-groups. It is documented in

docs/handbuch/permissions.md

@@ -5,8 +5,8 @@ sources:
   - src/resolve/resolver.ts
   - src/resolve/refs.ts
   - src/config/context.ts
-sources_hash: 5c881a7bede7e01a
-reviewed: 2026-08-13
+sources_hash: 5f9ed09f1e3793b0
+reviewed: 2026-08-17
 ---
 
 # Permissions (`ct.groupRole` / `ct.groupTypeRole` / `ct.status`)
@@ -267,11 +267,80 @@ resulting pairing id — one run, no second merge.
 - `ct plan` renders the domain as
   `<group-role(group=<key>, role=<name>) (created this apply)>`, the same
   pending marker every other deferred reference uses.
-- A `role` name the created group turns out not to have is still a **hard
-  error**, listing the roles it does have — the same message you would get at
-  plan time on a host where the group already exists.
-- Nothing changes on a host where the group already exists: the domain resolves
-  to a concrete pairing id at plan time, exactly as before.
+- A `role` name that is nowhere to be found — not on the group, and not
+  declared as a `roleDefinition` in this config — is still a **hard error**,
+  listing the roles the group does have.
+- Nothing changes on a host where the group already exists and already has the
+  role: the domain resolves to a concrete pairing id at plan time, as before.
+
+**The ROLE half goes pending too, since #120.** #106 covered the case where the
+_group_ is missing; the mirror case is a `ct.groupRole` naming a role that this
+host does not have yet but the config **declares as a `ct.roleDefinition`**.
+That used to hard-error even though the role was declared a few lines above,
+with a message ("Fix the role name, or pass a numeric id") whose two remedies
+are both wrong for a shared config: the name is right, and a numeric id is
+host-specific, which is the very thing the logical form exists to avoid.
+
+Now the domain resolves as pending and completes in the same run. Role
+definitions are **tier 3**, which executes before permission reconciliation, so
+by the time the pending domain is finished the role is already on the group's
+role list and the grant lands on its fresh pairing id.
+
+The declaration has to be for **this group's group type**, not merely for a role
+of the same name. Role names repeat across group types — live prod carries three
+`Leiter`, six `Organisator` and six `Mitglied`, each on a different type — so a
+name match alone would read a genuine mistake (a `ct.groupRole` on a group whose
+type has no such role, next to a `roleDefinition` for some other type) as
+"pending", and defer the failure past `executePlan` to the post-apply fetch. It
+fails there with the same message, but only after resources have been written;
+matching the group's own `groupTypeId` keeps it a **plan-time** error.
+
+Where the answer is not knowable offline the pending path is kept: a
+`roleDefinition` that states no group type, or a group whose state entry does
+not record `groupTypeId` (adopted before it was managed). The check only ever
+turns a would-be pending back into the hard error it used to be, and only on
+positive evidence.
+
+Creating a role definition also needs fields CT validates but the tool does not
+otherwise diff (#121). `ct` supplies them at **create only**, so they are never
+diffed or reverted afterwards:
+
+| Field       | Sent as                  | Notes                                          |
+| ----------- | ------------------------ | ---------------------------------------------- |
+| `shorty`    | the declared `name`, ≤10 | required, 1–10 chars                           |
+| `type`      | `"participant"`          | **declarable** — `"leader"` or `"participant"` |
+| `isDefault` | `false`                  | create default                                 |
+| `isHidden`  | `false`                  | create default                                 |
+| `sortKey`   | `0`                      | create default; every stock role uses `0`      |
+
+All four non-`shorty` fields are genuinely required: verified live on eqrm-dev
+(CT 3.135.2), where a POST without them is rejected with one validation error
+each. `isLeader` is deliberately **not** sent — ChurchTools derives it from
+`type`, and a role created with `type: "leader"` reads back `isLeader: true` on
+its own.
+
+`type` is the one that is a real semantic choice — does holding this role make
+you a _leader_ of the group? — so it is declarable and defaults to the
+conservative half:
+
+```js
+ct.roleDefinition({
+  key: "appmodule_admin",
+  name: "Admin",
+  nameTranslated: "Admin",
+  groupType: "appmodule",
+  type: "leader", // optional; defaults to "participant"
+});
+```
+
+An invalid value is rejected when the config loads, not as an HTTP 400 halfway
+through an unattended apply.
+
+This matters because role definitions are **per group type**, and that is
+exactly the master data that drifts between two hosts of one instance — one
+host has `Read`/`Admin`/`Write` on a type where the other only ever had CT's
+stock roles. Bringing the second host into line is precisely when a `groupRole`
+first names a role the host lacks.
 
 Until #106 this was a hard error, and that made a config **non-portable by
 construction**: declaring a group and a grant on its own role planned clean on

src/api/dynamicGroups.ts

@@ -0,0 +1,53 @@
+/**
+ * The ONE place that reads `GET /dynamicgroups` (#113/#124).
+ *
+ * The endpoint does not return objects. It returns a flat array of the GROUP ids that have a
+ * ruleset:
+ *
+ * ```json
+ * [159, 1698, 1704, 1707, 1710, 1713, 1740, 1748, 1753]
+ * ```
+ *
+ * Two call sites independently read those rows as `Number(row.id ?? row.groupId)`, which is
+ * `Number(undefined)` → `NaN` for every element, so the id set came out EMPTY on every host. That
+ * silently broke both consumers in the way that looks like good news rather than like a bug:
+ * `ct coverage` reported `dynamic: 0` on an instance with 70 auto-groups, and `ct refresh` answered
+ * "not a dynamic group" for every group, on every host, so the command could not succeed at all.
+ *
+ * Hence one parser, one test, and a shape that tolerates both spellings — an array of scalars is
+ * easy to re-break, and CT could plausibly grow an object form later.
+ */
+import type { CtClient } from "./ctClient.js";
+
+/**
+ * Coerce one `/dynamicgroups` row to a group id.
+ *
+ * Accepts the scalar form the endpoint actually returns (`1753`, and the numeric-string variant CT
+ * is prone to elsewhere) as well as an object carrying `id`/`groupId`. Returns `undefined` for
+ * anything that does not yield a finite id, so callers can simply skip it.
+ */
+export function dynamicGroupRowId(row: unknown): number | undefined {
+  const raw =
+    typeof row === "object" && row !== null
+      ? ((row as Record<string, unknown>).id ?? (row as Record<string, unknown>).groupId)
+      : row;
+  if (typeof raw !== "number" && typeof raw !== "string") return undefined;
+  const id = Number(raw);
+  return Number.isFinite(id) ? id : undefined;
+}
+
+/** Parse a whole `/dynamicgroups` payload into the set of group ids that have a ruleset. */
+export function parseDynamicGroupIds(rows: readonly unknown[]): Set<number> {
+  const ids = new Set<number>();
+  for (const row of rows) {
+    const id = dynamicGroupRowId(row);
+    if (id !== undefined) ids.add(id);
+  }
+  return ids;
+}
+
+/** Fetch the set of group ids that are auto-groups on this host. One cheap, un-paged request. */
+export async function fetchDynamicGroupIds(client: Pick<CtClient, "getAll">): Promise<Set<number>> {
+  const { data } = await client.getAll<unknown>("/dynamicgroups");
+  return parseDynamicGroupIds(data);
+}

src/commands/coverage.ts

@@ -10,6 +10,7 @@
 import { Command } from "commander";
 import { authedSession } from "../api/session.js";
 import type { CtClient } from "../api/ctClient.js";
+import { parseDynamicGroupIds } from "../api/dynamicGroups.js";
 import { resolveConfig } from "../config.js";
 import { prepareEnv } from "../env/context.js";
 import {
@@ -106,7 +107,7 @@ export async function collectCoverage(
   const [groupRows, groupTypeRows, dynamicRows, roleDefRows] = await Promise.all([
     client.getAll<Record<string, unknown>>(GROUPS_WITH_ROLES),
     client.getAll<Record<string, unknown>>("/group/grouptypes"),
-    client.getAll<Record<string, unknown>>("/dynamicgroups"),
+    client.getAll<unknown>("/dynamicgroups"),
     client.getAll<Record<string, unknown>>("/group/roles"),
   ]);
   // Same guarded read the planner performs: one request while the endpoint stays un-paged, and a
@@ -123,12 +124,8 @@ export async function collectCoverage(
     const id = Number(row.id);
     if (Number.isFinite(id) && typeof row.name === "string") roleNamesById.set(id, row.name);
   }
-  const dynamicGroupIds = new Set<number>();
-  for (const row of dynamicRows.data) {
-    // `/dynamicgroups` rows are keyed by the GROUP id they belong to; tolerate either spelling.
-    const id = Number(row.id ?? row.groupId);
-    if (Number.isFinite(id)) dynamicGroupIds.add(id);
-  }
+  // `/dynamicgroups` returns BARE group ids, not objects — see api/dynamicGroups.ts (#113/#124).
+  const dynamicGroupIds = parseDynamicGroupIds(dynamicRows.data);
 
   return buildCoverageReport({
     host,

src/commands/plan.ts

@@ -79,6 +79,10 @@ export function planCommand(): Command {
           summary: {
             resources: summarize(plan),
             drifted: plan.items.filter((i) => i.drift && i.drift.length > 0).length,
+            // Non-zero means the plan is PARTIAL: these resources could not be read, so their diff
+            // is missing rather than empty. A machine consumer must not treat the plan as complete
+            // while this is > 0 (#126) — `ct plan` also exits 1 in that case.
+            unreadable: plan.items.filter((i) => i.note === "fetch-failed").length,
             permissions: {
               toPut: permItems.reduce((n, i) => n + i.diff.toPut.length, 0),
               toDelete: permItems.reduce((n, i) => n + i.diff.toDelete.length, 0),

src/commands/refresh.ts

@@ -16,6 +16,7 @@ import { Command } from "commander";
 import { authedSession } from "../api/session.js";
 import type { CtClient } from "../api/ctClient.js";
 import { CtApiError } from "../api/ctClient.js";
+import { fetchDynamicGroupIds } from "../api/dynamicGroups.js";
 import { resolveConfig } from "../config.js";
 import { prepareEnv } from "../env/context.js";
 import { assertNotPeople } from "../engine/guard.js";
@@ -101,12 +102,7 @@ async function selectTargets(
   state: State,
   groupKey: string | undefined,
 ): Promise<ManagedResource[]> {
-  const dynamicIds = new Set<number>();
-  const { data } = await client.getAll<Record<string, unknown>>("/dynamicgroups");
-  for (const row of data) {
-    const id = Number(row.id ?? row.groupId);
-    if (Number.isFinite(id)) dynamicIds.add(id);
-  }
+  const dynamicIds = await fetchDynamicGroupIds(client);
 
   if (groupKey !== undefined) {
     const managed = state.resources[groupKey];

src/config/context.ts

@@ -439,6 +439,19 @@ function toDesired(type: string, input: ResourceInput, location?: string): Desir
       );
     }
   }
+  // `roleDefinition.type` is a closed set on CT's side ("Die Eingabe sollte eine der folgenden Werte
+  // sein: leader, participant"), and getting it wrong is only discovered as an HTTP 400 halfway
+  // through an unattended apply (#121). Reject it at config load instead, where it costs nothing.
+  if (type === "group-role" && fields.type !== undefined) {
+    if (fields.type !== "leader" && fields.type !== "participant") {
+      throw new Error(
+        located(
+          location,
+          `roleDefinition "${key}": "type" must be "leader" or "participant" (got ${JSON.stringify(fields.type)}).`,
+        ),
+      );
+    }
+  }
   // Warn (never throw) on a declared field the registry does not manage for this type — e.g. a
   // seeded config using campus's vestigial `shortName` instead of the real `shorty` (#51). The
   // field still passes through into `fields` unchanged (unrecognised fields have always been sent

src/engine/build.ts

@@ -118,6 +118,12 @@ 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);
+  // A synthetic field whose ACTUAL side could not be read makes its resource undiffable, exactly
+  // like a failed top-level GET. Marking it fetch-failed is what stops a transient 429 on
+  // `/dynamicgroups/{id}/ruleset` from surfacing as a fabricated `to update` (#126).
+  for (const key of folded.unreadable) {
+    if (!fetchFailed.has(key)) fetchFailed.set(key, "sub-resource read failed");
+  }
 
   // Resolution pass (#20): rewrite Ref-valued fields (and the dynamic ruleset, walked deeply) to
   // numbers / pending markers AFTER folding, BEFORE computePlan — so the diff stays number↔number.

src/engine/execute.ts

@@ -157,7 +157,11 @@ export async function executePlan(plan: Plan, deps: ExecuteDeps): Promise<Execut
         // win) for the POST ONLY. State still records `body` (the managed fields), so the defaults
         // stay unmanaged — a later plan neither diffs nor reverts them. A field still missing after
         // this surfaces as CT's HTTP 400 (#71), not a silent omission.
-        const defaultedBody = spec.createDefaults ? { ...spec.createDefaults(body), ...body } : body;
+        // `{ ...body }` last so a DECLARED value always wins — but an explicitly-`undefined` declared
+        // field must not knock out a default CT requires (that turns into an HTTP 400 at create for
+        // no reason), so undefined entries are dropped from the winning side.
+        const declared = Object.fromEntries(Object.entries(body).filter(([, v]) => v !== undefined));
+        const defaultedBody = spec.createDefaults ? { ...spec.createDefaults(body), ...declared } : body;
         // CT's same-name group-creation guard (#75): opt-in only, via `force: true` on the POST body.
         // Never a managed field — not in `body`/state, so it never diffs and never touches update.
         const createBody = item.allowDuplicateName ? { ...defaultedBody, force: true } : defaultedBody;

src/engine/render.ts

@@ -98,6 +98,14 @@ export function renderPlan(plan: Plan): string {
 
   const s = summarize(plan);
   lines.push("");
-  lines.push(pc.bold(`Plan: ${s.create} to create, ${s.update} to update, ${s.delete} to delete.`));
+  // The summary line is the part that gets pasted into a PR comment and approved, so it must never
+  // read as a complete plan when it is not (#126): a partial plan carries its own caveat inline.
+  const incomplete =
+    fetchFailed.length > 0
+      ? pc.yellow(` INCOMPLETE — ${fetchFailed.length} resource(s) could not be read.`)
+      : "";
+  lines.push(
+    pc.bold(`Plan: ${s.create} to create, ${s.update} to update, ${s.delete} to delete.`) + incomplete,
+  );
   return lines.join("\n");
 }