From 35518351295105122398da9dd2c7f5360bd864a1 Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Mon, 17 Aug 2026 14:36:12 +0200 Subject: [PATCH 1/4] fix(plan): stop plan/apply producing or committing a wrong result (#113, #120, #121, #124, #126) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "Apply correctness & safety" milestone: five bugs where plan/apply could produce or commit a wrong result against a live ChurchTools instance. #124 + #113 — `GET /dynamicgroups` returns a flat array of BARE GROUP IDS, not objects. Two call sites read it as `Number(row.id ?? row.groupId)` — NaN for every element — so the id set was empty on every host. `ct refresh` answered "not a dynamic group" for every group and could not succeed at all; `ct coverage` reported `dynamic: 0` on an instance with 70 auto-groups, which is the one wrong answer that looks like good news. Both now read the endpoint through a single parser (`src/api/dynamicGroups.ts`) that pins the scalar shape and tolerates an object form. #126 — a plan degraded into a WRONG plan under HTTP 429. A dynamic group whose ruleset read failed still had its DESIRED side folded, so it diffed a declared ruleset against an absent actual and surfaced as an ordinary `to update`. In a PR comment that is indistinguishable from real drift and invites the same response: apply it. A fold that cannot read the actual side now reports the resource as unreadable, `buildPlan` marks it fetch-failed (a no-op, rendered under "could not be read"), and the summary line carries `INCOMPLETE — N resource(s) could not be read.` so the count can never be missing from the artefact humans approve. `--json` gains `summary.unreadable`. #121 — `apply` could not create a role definition: POST /group/roles 400s without `type`/`isDefault`/`isHidden`, none of which were sent. `plan` was green, so the breakage landed in `apply`, on the host the pipeline writes to. `type` is a real semantic choice (leader vs participant), so it is declarable and defaults to `participant`; the two booleans are create defaults. An invalid `type` is now rejected at config load, not as an HTTP 400 mid-apply. #120 — a `groupRole` whose ROLE is created in the same run hard-errored. #106 made the missing GROUP resolve as pending; the role half did not, so a `groupRole` naming a `roleDefinition` declared three lines up failed with a message whose two remedies ("fix the name, or pass a numeric id") are both wrong for a shared multi-host config. The role now resolves as pending and completes in the same run — role definitions are tier 3, which executes before permission reconciliation. BREAKING CHANGE: `roleDefinition` creates now send `type: "participant"`, `isDefault: false` and `isHidden: false`. Declare `type: "leader"` to get the previous-intent leader semantics on a newly created role. Existing roles are unaffected — these are create-only defaults and are never diffed. Claude-Session: https://claude.ai/code/session_01V24GdD2F6LTGTF5xWoGq4i --- docs/handbuch/blueprints.md | 4 +- docs/handbuch/dynamic-groups.md | 36 +++++- docs/handbuch/permissions.md | 61 ++++++++-- src/api/dynamicGroups.ts | 53 +++++++++ src/commands/coverage.ts | 11 +- src/commands/plan.ts | 4 + src/commands/refresh.ts | 8 +- src/config/context.ts | 13 ++ src/engine/build.ts | 6 + src/engine/execute.ts | 6 +- src/engine/render.ts | 10 +- src/engine/synthetic.ts | 113 ++++++++++++------ src/resolve/resolver.ts | 47 +++++++- src/resources/registry.ts | 37 +++++- tests/dynamic-groups-endpoint.test.ts | 95 +++++++++++++++ tests/execute.test.ts | 11 +- tests/permission-pending-role.test.ts | 164 ++++++++++++++++++++++++++ tests/plan-partial-honesty.test.ts | 153 ++++++++++++++++++++++++ tests/registry.test.ts | 8 +- 19 files changed, 768 insertions(+), 72 deletions(-) create mode 100644 src/api/dynamicGroups.ts create mode 100644 tests/dynamic-groups-endpoint.test.ts create mode 100644 tests/permission-pending-role.test.ts create mode 100644 tests/plan-partial-honesty.test.ts diff --git a/docs/handbuch/blueprints.md b/docs/handbuch/blueprints.md index 0a61d93..2190cf4 100644 --- a/docs/handbuch/blueprints.md +++ b/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) diff --git a/docs/handbuch/dynamic-groups.md b/docs/handbuch/dynamic-groups.md index 63a1a12..f911d64 100644 --- a/docs/handbuch/dynamic-groups.md +++ b/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:///?q=cron&standby=true`, which runs **every due scheduled > job on the instance** — far beyond auto-groups. It is documented in diff --git a/docs/handbuch/permissions.md b/docs/handbuch/permissions.md index cb6fe3b..e8b85bb 100644 --- a/docs/handbuch/permissions.md +++ b/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: 614b981fa6011b17 +reviewed: 2026-08-17 --- # Permissions (`ct.groupRole` / `ct.groupTypeRole` / `ct.status`) @@ -267,11 +267,58 @@ resulting pairing id — one run, no second merge. - `ct plan` renders the domain as `, role=) (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. + +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 | + +`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 diff --git a/src/api/dynamicGroups.ts b/src/api/dynamicGroups.ts new file mode 100644 index 0000000..e27c347 --- /dev/null +++ b/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).id ?? (row as Record).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 { + const ids = new Set(); + 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): Promise> { + const { data } = await client.getAll("/dynamicgroups"); + return parseDynamicGroupIds(data); +} diff --git a/src/commands/coverage.ts b/src/commands/coverage.ts index 608dbdb..1fb6174 100644 --- a/src/commands/coverage.ts +++ b/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>(GROUPS_WITH_ROLES), client.getAll>("/group/grouptypes"), - client.getAll>("/dynamicgroups"), + client.getAll("/dynamicgroups"), client.getAll>("/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(); - 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, diff --git a/src/commands/plan.ts b/src/commands/plan.ts index 0f0ecb3..4f13434 100644 --- a/src/commands/plan.ts +++ b/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), diff --git a/src/commands/refresh.ts b/src/commands/refresh.ts index 7a81a49..b942f47 100644 --- a/src/commands/refresh.ts +++ b/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 { - const dynamicIds = new Set(); - const { data } = await client.getAll>("/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]; diff --git a/src/config/context.ts b/src/config/context.ts index cfa19ff..ae15873 100644 --- a/src/config/context.ts +++ b/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 diff --git a/src/engine/build.ts b/src/engine/build.ts index 381cc11..f53bc30 100644 --- a/src/engine/build.ts +++ b/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. diff --git a/src/engine/execute.ts b/src/engine/execute.ts index b0054c9..7cdecea 100644 --- a/src/engine/execute.ts +++ b/src/engine/execute.ts @@ -157,7 +157,11 @@ export async function executePlan(plan: Plan, deps: ExecuteDeps): Promise 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; diff --git a/src/engine/render.ts b/src/engine/render.ts index 848596d..6ebcd4a 100644 --- a/src/engine/render.ts +++ b/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"); } diff --git a/src/engine/synthetic.ts b/src/engine/synthetic.ts index c39b389..6919163 100644 --- a/src/engine/synthetic.ts +++ b/src/engine/synthetic.ts @@ -50,9 +50,24 @@ export interface SyntheticPostApplyCtx { item: PlanItem; change: FieldChange; } +/** + * What a fold pass produced. + * + * `unreadable` is the half that keeps a degraded plan HONEST (#126): the logical keys whose ACTUAL + * side could not be read. A fold must never fold only the desired side of a field it could not read + * the actual of — that manufactures an update out of a transient 429. Reporting the key here makes + * `buildPlan` mark the resource fetch-failed, so it renders as "could not be read" instead of + * silently joining the `to update` count. + */ +export interface SyntheticFoldResult { + desired: DesiredResource[]; + errors: string[]; + unreadable?: string[]; +} + export interface SyntheticField { field: string; - fold(ctx: SyntheticFoldCtx): Promise<{ desired: DesiredResource[]; errors: string[] }>; + fold(ctx: SyntheticFoldCtx): Promise; apply(ctx: SyntheticApplyCtx): Promise; /** * Optional opt-in side effect run AFTER the whole plan has applied (e.g. `ct apply --refresh` @@ -87,8 +102,14 @@ const parentsField: SyntheticField = { } catch (err) { const message = formatError(err); warn(`Failed to fetch group hierarchies: ${message}`); - // Leave `parents` undiffed rather than fabricate "add all parents" from an empty map. - return { desired, errors: [`group hierarchies: ${message}`] }; + // Leave `parents` undiffed rather than fabricate "add all parents" from an empty map — and + // report every opted-in group as unreadable so the plan says so instead of quietly + // under-reporting a real hierarchy change as a no-op (#126). + return { + desired, + errors: [`group hierarchies: ${message}`], + unreadable: desired.filter((d) => d.type === "group" && d.parents !== undefined).map((d) => d.key), + }; } }, async apply({ client, state, id, change }) { @@ -128,39 +149,57 @@ const dynamicField: SyntheticField = { // sequential: the status GET must run only after the ruleset GET succeeds (a 404 there means // "not a dynamic group" and short-circuits). Per-group error strings are collected in input // order so the plan-degradation output is deterministic regardless of completion order. - const perGroupErrors = await mapConcurrent(targets, DYNAMIC_FETCH_CONCURRENCY, async ({ managed, a }) => { - // The ruleset GET and the status GET have distinct failure meanings, so they get distinct - // try/catch blocks: only a ruleset 404 means "not a dynamic group". A status GET that fails - // AFTER a successful ruleset GET must NOT fabricate the "none" sentinel (that would discard a - // real ruleset and propose a spurious re-PUT) — it degrades the plan via `errors` instead. - let ruleset: Record; - try { - ruleset = await client.get>(`/dynamicgroups/${managed.id}/ruleset`); - } catch (err) { - if (err instanceof CtApiError && err.status === 404) { - // Group exists but is not (yet) a dynamic group — its ruleset 404s. Sentinel so a promote - // (desired active vs actual none) diffs as a real change and demote-to-none is a clean no-op. - a.dynamic = { status: "none", ruleset: {} }; - return []; + const perGroupOutcome = await mapConcurrent( + targets, + DYNAMIC_FETCH_CONCURRENCY, + async ({ managed, a }) => { + // The ruleset GET and the status GET have distinct failure meanings, so they get distinct + // try/catch blocks: only a ruleset 404 means "not a dynamic group". A status GET that fails + // AFTER a successful ruleset GET must NOT fabricate the "none" sentinel (that would discard a + // real ruleset and propose a spurious re-PUT) — it degrades the plan via `errors` instead. + let ruleset: Record; + try { + ruleset = await client.get>(`/dynamicgroups/${managed.id}/ruleset`); + } catch (err) { + if (err instanceof CtApiError && err.status === 404) { + // Group exists but is not (yet) a dynamic group — its ruleset 404s. Sentinel so a promote + // (desired active vs actual none) diffs as a real change and demote-to-none is a clean no-op. + a.dynamic = { status: "none", ruleset: {} }; + return { errors: [], unreadable: [] }; + } + // Non-404: we do NOT know this group's ruleset. Folding the desired side now would diff a + // declared ruleset against an absent actual and manufacture an update out of a 429 (#126). + return { + errors: [`dynamic ${managed.key} (#${managed.id}): ${formatError(err)}`], + unreadable: [managed.key], + }; } - return [`dynamic ${managed.key} (#${managed.id}): ${formatError(err)}`]; - } - try { - const statusRes = await client.get<{ dynamicGroupStatus?: string }>( - `/dynamicgroups/${managed.id}/status`, - ); - a.dynamic = { - status: (statusRes?.dynamicGroupStatus ?? "none") as DynamicStatus, - ruleset: normalizeRuleset(ruleset), - }; - return []; - } catch (err) { - return [`dynamic ${managed.key} status (#${managed.id}): ${formatError(err)}`]; - } - }); - const errors = perGroupErrors.flat(); + try { + const statusRes = await client.get<{ dynamicGroupStatus?: string }>( + `/dynamicgroups/${managed.id}/status`, + ); + a.dynamic = { + status: (statusRes?.dynamicGroupStatus ?? "none") as DynamicStatus, + ruleset: normalizeRuleset(ruleset), + }; + return { errors: [], unreadable: [] }; + } catch (err) { + // Same reasoning as the ruleset case: an unknown status is not a known-different status. + return { + errors: [`dynamic ${managed.key} status (#${managed.id}): ${formatError(err)}`], + unreadable: [managed.key], + }; + } + }, + ); + const errors = perGroupOutcome.flatMap((o) => o.errors); + const unreadable = perGroupOutcome.flatMap((o) => o.unreadable); + const unreadableKeys = new Set(unreadable); const augmented = desired.map((d) => { if (d.type !== "group" || d.dynamic === undefined) return d; + // Actual side unknown → leave the desired side unfolded so nothing diffs. `buildPlan` turns + // the key into a fetch-failed no-op, which is the honest rendering of "I could not read this". + if (unreadableKeys.has(d.key)) 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 @@ -184,7 +223,7 @@ const dynamicField: SyntheticField = { const dynamic = normalizeDynamic({ status: d.dynamic.status, ruleset: resolvedRuleset }); return { ...d, fields: { ...d.fields, dynamic } }; }); - return { desired: augmented, errors }; + return { desired: augmented, errors, unreadable }; }, async apply({ client, id, change }) { const to = change.to as { status: DynamicStatus; ruleset: Record } | undefined; @@ -268,13 +307,15 @@ export async function runPostApplyHooks( /** Run every registered fold in order, threading the (immutably) augmented desired through each. */ export async function foldSynthetic( ctx: SyntheticFoldCtx, -): Promise<{ desired: DesiredResource[]; errors: string[] }> { +): Promise<{ desired: DesiredResource[]; errors: string[]; unreadable: Set }> { let desired = ctx.desired; const errors: string[] = []; + const unreadable = new Set(); for (const f of SYNTHETIC_FIELDS) { const res = await f.fold({ ...ctx, desired }); desired = res.desired; errors.push(...res.errors); + for (const key of res.unreadable ?? []) unreadable.add(key); } - return { desired, errors }; + return { desired, errors, unreadable }; } diff --git a/src/resolve/resolver.ts b/src/resolve/resolver.ts index 232080e..516ce54 100644 --- a/src/resolve/resolver.ts +++ b/src/resolve/resolver.ts @@ -187,6 +187,12 @@ export class Resolver { private readonly groupRoleLists = new Map>(); /** Declared logical keys indexed by resource type — a same-run target that resolves to pending. */ private readonly declaredByType = new Map>(); + /** + * Slugged NAMES of the `roleDefinition`s this config declares (#120). A `ct.groupRole` names its + * role by name, not by logical key, so the pending check has to match on the name — which is why + * this is a separate index from {@link declaredByType}. + */ + private readonly declaredRoleDefNames = new Set(); constructor(deps: ResolverDeps) { this.client = deps.client; @@ -199,6 +205,9 @@ export class Resolver { this.declaredByType.set(d.type, set); } set.add(d.key); + if (d.type === "group-role" && typeof d.fields.name === "string") { + this.declaredRoleDefNames.add(slug(d.fields.name)); + } } } @@ -292,9 +301,29 @@ export class Resolver { const groupId = this.groupIdForRole(r, site, opts); if (typeof groupId !== "number") return groupId; const rows = await this.groupRoleList(groupId); + // #106 made a same-run GROUP resolve as pending. #120: the ROLE half needs the same treatment. + // Role definitions are per group type and drift easily between two hosts of one instance, so a + // `groupRole` naming a custom role hard-errored on the host that lacks it — with a message + // ("Fix the role name, or pass a numeric id") whose two remedies are both wrong when the role is + // declared three lines up. Roles created this run land on the group's role list once the + // role-def create (tier 3) has run, which is before permissions — so the same post-apply + // completion that finishes a pending group finishes this too. + if (this.declaresRoleNamed(r.role) && !hasRoleNamed(rows, r.role)) { + if (opts.pendingGroupRole) return pendingRef(r); + throw new Error( + `Cannot resolve ${refLabel(r)} referenced at ${site} on ${this.host}: "${r.role}" is declared ` + + `as a roleDefinition in this config but does not exist on this host yet. Apply the master ` + + `data first, then declare the grants — or pass a numeric id.`, + ); + } return pickGroupRolePairingId(rows, r, groupId, site, this.host); } + /** Does this config declare a `roleDefinition` whose name matches `role` (slug-insensitive)? */ + private declaresRoleNamed(role: string): boolean { + return this.declaredRoleDefNames.has(slug(role)); + } + /** * Resolve a group-type-scoped role (`groupTypeRoleId`) by its (group-type, role) pair to this host's * numeric id (#76). Role names are NOT globally unique across group types (live prod, 2026-07-11: 3 @@ -452,6 +481,21 @@ function fetchGroupRoleRows(client: RoleListReader, groupId: number): Promise typeof row.name === "string" && slug(row.name) === slug(role)); + return bySlug.length >= 1 ? bySlug : rows.filter((row) => row.name === role); +} + +/** Whether the group's live role list already holds a role by this name. */ +function hasRoleNamed(rows: CatalogRecord[], role: string): boolean { + return matchRoleRows(rows, role).length > 0; +} + function pickGroupRolePairingId( rows: CatalogRecord[], r: GroupRoleRef, @@ -459,8 +503,7 @@ function pickGroupRolePairingId( site: string, host: string, ): number { - const bySlug = rows.filter((row) => typeof row.name === "string" && slug(row.name) === slug(r.role)); - const matches = bySlug.length >= 1 ? bySlug : rows.filter((row) => row.name === r.role); + const matches = matchRoleRows(rows, r.role); if (matches.length === 0) { const available = rows .map((row) => (typeof row.name === "string" ? JSON.stringify(row.name) : `#${row.id}`)) diff --git a/src/resources/registry.ts b/src/resources/registry.ts index 108b1d6..1024baf 100644 --- a/src/resources/registry.ts +++ b/src/resources/registry.ts @@ -510,12 +510,37 @@ export const RESOURCES: Record = { updateMethod: "PUT", tier: 3, deriveKey: (r) => slug(str(r, "name")), - managedFields: (r) => ({ name: r.name, nameTranslated: r.nameTranslated, groupTypeId: r.groupTypeId }), - // POST /group/roles requires `shorty` (1–10 chars, non-nullable in CT's OpenAPI POST schema) which - // the tool does not manage (#73 audit). Sent at CREATE only, derived from the declared `name`; not - // diffed afterward. (`type`/`isLeader`/`sortKey` are all optional/nullable — no default needed.) - // Padded up to the 1-char floor for the same empty-name edge case as group-type's shorty above. - createDefaults: (r) => ({ shorty: truncatePadded(str(r, "name"), 10, 1) }), + managedFields: (r) => ({ + name: r.name, + nameTranslated: r.nameTranslated, + groupTypeId: r.groupTypeId, + // `type` is `leader` | `participant` — a real semantic choice (does holding this role make you + // a leader of the group?), so it is declarable rather than defaulted-and-forgotten (#121). It + // only diffs when the config declares it: `diffFields` walks the DESIRED keys. + type: r.type, + }), + // Fields CT REQUIRES at create but the tool does not otherwise manage (#73/#121). The old comment + // here claimed `type` was "optional/nullable — no default needed"; live CT 3.135.2 disagrees and + // rejects the POST with three validation errors: + // + // type "Die Eingabe sollte eine der folgenden Werte sein: leader, participant" + // isDefault "Eingabe muss TRUE oder FALSE sein." + // isHidden "Eingabe muss TRUE oder FALSE sein." + // + // so a declared `roleDefinition` that did not exist on the target host could not be created at + // all — and it failed in `apply`, on the host the pipeline writes to, after `plan` had been green. + // + // `shorty` is 1–10 chars, non-nullable, derived from the declared `name`; padded up to the 1-char + // floor for the same empty-name edge case as group-type's shorty above. + // `type` defaults to `participant`: the conservative half of the choice, since `leader` confers + // group leadership. A config that wants the other one declares `type: "leader"` and wins here + // (createDefaults merges UNDER the declared body). + createDefaults: (r) => ({ + shorty: truncatePadded(str(r, "name"), 10, 1), + type: "participant", + isDefault: false, + isHidden: false, + }), // `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", diff --git a/tests/dynamic-groups-endpoint.test.ts b/tests/dynamic-groups-endpoint.test.ts new file mode 100644 index 0000000..0da6bb3 --- /dev/null +++ b/tests/dynamic-groups-endpoint.test.ts @@ -0,0 +1,95 @@ +/** + * `GET /dynamicgroups` returns a flat array of GROUP IDS, not objects (#113/#124). + * + * Both consumers used to read it as `Number(row.id ?? row.groupId)` — `NaN` for every element — so + * the id set was empty on every host: `ct coverage` reported `dynamic: 0` on an instance with 70 + * auto-groups, and `ct refresh` refused every group as "not a dynamic group". These tests pin the + * scalar shape so it cannot be re-broken silently. + */ +import { describe, it, expect } from "vitest"; +import { dynamicGroupRowId, parseDynamicGroupIds, fetchDynamicGroupIds } from "../src/api/dynamicGroups.js"; +import { buildCoverageReport, type GroupRow } from "../src/coverage/report.js"; +import type { State } from "../src/state/state.js"; + +describe("parseDynamicGroupIds (#124)", () => { + it("reads the bare-integer array the endpoint actually returns", () => { + // Verbatim from a live host, CT 3.135.2. + const ids = parseDynamicGroupIds([159, 1698, 1704, 1707, 1710, 1713, 1740, 1748, 1753]); + expect(ids.size).toBe(9); + expect(ids.has(1753)).toBe(true); + }); + + it("still reads an object form, in either spelling", () => { + expect([...parseDynamicGroupIds([{ id: 12 }, { groupId: 34 }])]).toEqual([12, 34]); + }); + + it("accepts numeric strings, which CT is prone to elsewhere", () => { + expect([...parseDynamicGroupIds(["1753"])]).toEqual([1753]); + }); + + it("skips rows that yield no finite id instead of poisoning the set with NaN", () => { + const ids = parseDynamicGroupIds([null, undefined, {}, "nope", 7]); + expect([...ids]).toEqual([7]); + expect([...ids].every(Number.isFinite)).toBe(true); + }); + + it("dynamicGroupRowId reports the miss rather than returning NaN", () => { + expect(dynamicGroupRowId({})).toBeUndefined(); + expect(dynamicGroupRowId(1753)).toBe(1753); + }); + + it("fetchDynamicGroupIds asks the endpoint once and un-paged", async () => { + const calls: string[] = []; + const client = { + getAll: async (path: string) => { + calls.push(path); + return { data: [1, 2, 3], meta: {} }; + }, + } as unknown as Parameters[0]; + expect([...(await fetchDynamicGroupIds(client))]).toEqual([1, 2, 3]); + expect(calls).toEqual(["/dynamicgroups"]); + }); +}); + +describe("coverage dynamic counts (#113)", () => { + const group = (id: number, groupTypeId: number, name = `g${id}`): GroupRow => ({ + id, + name, + groupTypeId, + roles: [], + }); + + it("counts dynamic and managedDynamic where the two sets only partially overlap", () => { + // 5 groups; 3 are dynamic (2 Merkmal, 1 Struktur); 2 are managed, only ONE of which is dynamic. + const groups = [group(1, 10), group(2, 10), group(3, 10), group(4, 20), group(5, 20)]; + const state: State = { + version: 1, + resources: { + // #2 is dynamic AND managed — the managedDynamic case. + merkmal_managed_dynamic: { type: "group", id: 2, fields: {} }, + // #5 is managed but NOT dynamic — must not inflate managedDynamic. + struktur_managed_static: { type: "group", id: 5, fields: {} }, + }, + } as unknown as State; + + const report = buildCoverageReport({ + host: "https://example.church.tools", + state, + groups, + groupTypeNames: new Map([ + [10, "Merkmal"], + [20, "Struktur"], + ]), + dynamicGroupIds: new Set([2, 3, 4]), + groupRolePermissions: [], + }); + + expect(report.groups).toMatchObject({ total: 5, dynamic: 3, managedDynamic: 1 }); + + // Acceptance: the per-type `dynamic` column sums to the /dynamicgroups count. + const perType = report.byType.reduce((sum, t) => sum + t.dynamic, 0); + expect(perType).toBe(3); + expect(report.byType.find((t) => t.name === "Merkmal")?.dynamic).toBe(2); + expect(report.byType.find((t) => t.name === "Struktur")?.dynamic).toBe(1); + }); +}); diff --git a/tests/execute.test.ts b/tests/execute.test.ts index 8c5099e..c6c29f0 100644 --- a/tests/execute.test.ts +++ b/tests/execute.test.ts @@ -185,7 +185,16 @@ describe("executePlan", () => { expect(calls[0]).toEqual({ method: "POST", path: "/group/roles", - body: { name: "Verantwortlicher", groupTypeId: 2, shorty: "Verantwort" }, + // `type`/`isDefault`/`isHidden` are create-only defaults CT validates (#121) — sent on the + // POST, absent from state, so they are never diffed or reverted afterwards. + body: { + name: "Verantwortlicher", + groupTypeId: 2, + shorty: "Verantwort", + type: "participant", + isDefault: false, + isHidden: false, + }, }); expect(state.resources.leiter?.fields).toEqual({ name: "Verantwortlicher", groupTypeId: 2 }); }); diff --git a/tests/permission-pending-role.test.ts b/tests/permission-pending-role.test.ts new file mode 100644 index 0000000..029092a --- /dev/null +++ b/tests/permission-pending-role.test.ts @@ -0,0 +1,164 @@ +/** + * Pending ROLE definitions in a `group_role` domain (#120). + * + * #106 made "declare a group + a `ct.groupRole` on it in one config" work against a host that lacks + * the group. The same was NOT true when the missing piece was the ROLE: the resolver only ever + * considered pending groups, so a `groupRole` naming a `roleDefinition` declared three lines up + * hard-errored — with a message ("Fix the role name, or pass a numeric id") whose two remedies are + * both wrong for a shared, multi-host config. Role definitions are per group type and drift easily + * between two hosts of one instance, which is exactly when this bites. + */ +import { describe, it, expect, vi } from "vitest"; +import { buildPermissionPlan } from "../src/permissions/plan.js"; +import { applyPermissionPlan } from "../src/permissions/apply.js"; +import { executePlan } from "../src/engine/execute.js"; +import { Resolver } from "../src/resolve/resolver.js"; +import { emptyState } from "../src/state/state.js"; +import { ref } from "../src/resolve/refs.js"; +import type { DesiredResource, Plan } from "../src/engine/types.js"; +import type { DesiredPermission } from "../src/permissions/types.js"; +import type { CtClient } from "../src/api/ctClient.js"; + +const HOST = "https://mychurch.church.tools"; +const GROUP_ID = 864; + +/** The group exists on this host; the custom "Admin" role does not (only CT's stock roles do). */ +const STOCK_ROLES = [ + { id: 11, name: "Systemdesign" }, + { id: 12, name: "leader" }, + { id: 13, name: "participant" }, +]; + +/** Config declares the role definition AND a grant block that names it — the #120 shape. */ +const desired: DesiredResource[] = [ + { + type: "group-role", + key: "appmodule_admin", + fields: { name: "Admin", nameTranslated: "Admin", groupTypeId: 30 }, + dependsOn: [], + }, +]; + +const grantBlock: DesiredPermission = { + key: "amflowsequip_admin", + domainType: "group_role", + domainId: ref.groupRole("amflowsequip", "Admin"), + grants: ["churchgroup:administer groups"], +}; + +function mockClient(roles: unknown[]) { + const get = vi.fn(async (path: string) => { + if (path === `/groups/${GROUP_ID}/roles`) return roles; + return []; + }); + return { get, request: vi.fn(async () => ({})) } as unknown as CtClient; +} + +function stateWithGroup() { + const state = emptyState(HOST); + state.resources.amflowsequip = { + type: "group", + id: GROUP_ID, + key: "amflowsequip", + fields: { name: "AM Flows Equip" }, + adoptedAt: "t", + updatedAt: "t", + }; + return state; +} + +describe("pending role definition in a group_role domain (#120)", () => { + it("plans as pending instead of hard-erroring when the role is declared in the same config", async () => { + const state = stateWithGroup(); + const client = mockClient(STOCK_ROLES); + const { items, fetchErrors } = await buildPermissionPlan(client, state, [grantBlock], desired); + + expect(fetchErrors).toEqual([]); + expect(items).toHaveLength(1); + // The domain is deferred to apply time, exactly like a pending GROUP (#106). + expect(items[0]?.pendingDomain).toEqual(ref.groupRole("amflowsequip", "Admin")); + }); + + it("still hard-errors when the role is NOT declared anywhere — that is a real config mistake", async () => { + const state = stateWithGroup(); + const client = mockClient(STOCK_ROLES); + const orphan: DesiredPermission = { + ...grantBlock, + domainId: ref.groupRole("amflowsequip", "Nonexistent"), + }; + + await expect(buildPermissionPlan(client, state, [orphan], [])).rejects.toThrow( + /has no role named "Nonexistent"/, + ); + }); + + it("resolves normally once the role exists on the host — no spurious pending", async () => { + const state = stateWithGroup(); + const client = mockClient([...STOCK_ROLES, { id: 114, name: "Admin" }]); + const { items } = await buildPermissionPlan(client, state, [grantBlock], desired); + expect(items[0]?.pendingDomain).toBeUndefined(); + expect(items[0]?.domainId).toBe(114); + }); + + it("applies in ONE run — create the role definition, re-read the roles, grant on the pairing id", async () => { + // The whole point of #120: two merges become one. Role definitions are tier 3, which executes + // before permissions, so by the time the domain is completed the role is on the group's list. + const NEW_PAIRING_ID = 114; + let roleCreated = false; + const calls: { method: string; path: string; body?: unknown }[] = []; + const client = { + request: vi.fn(async (method: string, path: string, body?: unknown) => { + calls.push({ method, path, body }); + if (method === "POST" && path === "/group/roles") { + roleCreated = true; + return { id: 77 }; + } + return {}; + }), + get: vi.fn(async (path: string) => + path === `/groups/${GROUP_ID}/roles` + ? roleCreated + ? [...STOCK_ROLES, { id: NEW_PAIRING_ID, name: "Admin" }] + : STOCK_ROLES + : [], + ), + } as unknown as CtClient; + + const state = stateWithGroup(); + const { items } = await buildPermissionPlan(client, state, [grantBlock], desired); + expect(items[0]?.pendingDomain).toBeTruthy(); // planned before the role exists + + await executePlan( + { + items: [ + { + type: "group-role", + key: "appmodule_admin", + id: null, + action: "create", + changes: [ + { field: "name", from: undefined, to: "Admin" }, + { field: "groupTypeId", from: undefined, to: 30 }, + ], + }, + ], + } as unknown as Plan, + { client, state, statePath: "unused", save: async () => {} }, + ); + + const res = await applyPermissionPlan(items, client, state); + expect(res.failed).toEqual([]); + expect(res.granted).toBe(1); + // The grant lands on the pairing id of the role created moments earlier — in the SAME run. + expect(calls.find((c) => c.method === "PUT")?.path).toBe(`/permissions/group_role/${NEW_PAIRING_ID}`); + }); + + it("the direct resolver error names the real remedy, not 'fix the role name'", async () => { + // Outside the permission-domain position there is nothing to finish a pending ref, so this + // stays an error — but the message must stop recommending two things that cannot work. + const resolver = new Resolver({ client: mockClient(STOCK_ROLES), state: stateWithGroup(), desired }); + await expect( + resolver.resolve(ref.groupRole("amflowsequip", "Admin"), 'group_role "x".domainId'), + ).rejects.toThrow(/declared as a roleDefinition in this config but does not exist on this host yet/); + }); +}); diff --git a/tests/plan-partial-honesty.test.ts b/tests/plan-partial-honesty.test.ts new file mode 100644 index 0000000..8bc943f --- /dev/null +++ b/tests/plan-partial-honesty.test.ts @@ -0,0 +1,153 @@ +/** + * A partial plan must never be a WRONG plan (#126). + * + * Under HTTP 429 the tool used to log the failed reads as warnings and then diff config against the + * partial actuals it managed to fetch, so a resource whose sub-resource GET was rate-limited showed + * up as an ordinary `update`. In a PR comment that is indistinguishable from a real drift, and it + * invites the same response: apply it. These tests pin the two properties that make the degraded + * plan honest instead — an unread resource is a no-op flagged `fetch-failed`, and the summary line + * that humans approve says so on its face. + */ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { buildPlan } from "../src/engine/build.js"; +import { renderPlan } from "../src/engine/render.js"; +import { emptyState } from "../src/state/state.js"; +import { CtApiError } from "../src/api/ctClient.js"; +import type { DesiredResource } from "../src/engine/types.js"; + +/** The state + config used by every case: one managed group that declares a dynamic ruleset. */ +function fixture() { + const state = emptyState("https://x.church.tools"); + state.resources.sll_koblenz = { + type: "group", + id: 3213, + key: "sll_koblenz", + fields: { name: "SLL Koblenz" }, + adoptedAt: "t", + updatedAt: "t", + }; + const desired: DesiredResource[] = [ + { + type: "group", + key: "sll_koblenz", + fields: { name: "SLL Koblenz" }, + dependsOn: [], + dynamic: { status: "active", ruleset: { "==": [{ var: "person.id" }, 1] } }, + }, + ]; + return { state, desired }; +} + +beforeEach(() => { + // The fold path warns on the failed read; keep the test output readable. + vi.spyOn(console, "error").mockImplementation(() => {}); +}); +afterEach(() => vi.restoreAllMocks()); + +describe("plan degradation under 429 (#126)", () => { + it("does NOT fabricate an update when the ruleset read is rate-limited", async () => { + const { state, desired } = fixture(); + const client = { + get: async (path: string): Promise => { + if (path === "/groups/3213") return { name: "SLL Koblenz" } as T; + // The sub-resource read is the one that 429s — the top-level GET succeeded, which is + // exactly the "mild" regime the issue describes as the dangerous one. + throw new CtApiError("GET /dynamicgroups/3213/ruleset failed (HTTP 429)", 429, null); + }, + }; + + const { plan, fetchErrors } = await buildPlan(client, state, desired); + const item = plan.items.find((i) => i.key === "sll_koblenz")!; + + expect(item.action).toBe("no-op"); // NOT "update" — the old behaviour + expect(item.note).toBe("fetch-failed"); + expect(item.changes).toEqual([]); + // The failure is still reported, so `plan` exits 1 and `apply` refuses to run. + expect(fetchErrors).toHaveLength(1); + expect(fetchErrors[0]).toContain("429"); + }); + + it("still diffs the ruleset normally when the read succeeds", async () => { + const { state, desired } = fixture(); + const client = { + get: async (path: string): Promise => { + if (path === "/groups/3213") return { name: "SLL Koblenz" } as T; + if (path === "/dynamicgroups/3213/ruleset") return { "==": [{ var: "person.id" }, 2] } as T; + if (path === "/dynamicgroups/3213/status") return { dynamicGroupStatus: "active" } as T; + throw new CtApiError(`not found: ${path}`, 404, null); + }, + }; + + const { plan, fetchErrors } = await buildPlan(client, state, desired); + expect(fetchErrors).toEqual([]); + // A genuine difference (person.id 2 live vs 1 declared) must still surface as an update — + // the fix must not have bought its honesty by going blind. + expect(plan.items.find((i) => i.key === "sll_koblenz")?.action).toBe("update"); + }); + + it("a hierarchy read failure reports the opted-in groups rather than silently dropping parents", async () => { + const state = emptyState("https://x.church.tools"); + state.resources.child = { + type: "group", + id: 10, + key: "child", + fields: { name: "Child" }, + adoptedAt: "t", + updatedAt: "t", + }; + const desired: DesiredResource[] = [ + { type: "group", key: "child", fields: { name: "Child" }, dependsOn: [], parents: ["parent"] }, + ]; + const client = { + get: async (path: string): Promise => { + if (path === "/groups/10") return { name: "Child" } as T; + throw new CtApiError("GET /groups/hierarchies failed (HTTP 429)", 429, null); + }, + }; + + const { plan, fetchErrors } = await buildPlan(client, state, desired); + const item = plan.items.find((i) => i.key === "child")!; + expect(item.note).toBe("fetch-failed"); + expect(item.action).toBe("no-op"); + expect(fetchErrors.some((e) => e.includes("group hierarchies"))).toBe(true); + }); + + it("the summary line humans approve carries the unread count", () => { + const rendered = renderPlan({ + items: [ + { + type: "group", + key: "a", + id: 1, + action: "update", + changes: [{ field: "name", from: "x", to: "y", source: "config" }], + }, + { type: "group", key: "b", id: 2, action: "no-op", changes: [], note: "fetch-failed", detail: "429" }, + ], + } as unknown as Parameters[0]); + + // eslint-disable-next-line no-control-regex -- strip the colour codes renderPlan emits + const plain = rendered.replace(/\[[0-9;]*m/g, ""); + expect(plain).toContain("Plan: 0 to create, 1 to update, 0 to delete."); + // "1 to update" must never stand alone when a resource could not be read. + expect(plain).toMatch(/to delete\..*INCOMPLETE — 1 resource\(s\) could not be read\./); + }); + + it("a complete plan's summary line stays exactly as it was", () => { + const rendered = renderPlan({ + items: [ + { + type: "group", + key: "a", + id: 1, + action: "update", + changes: [{ field: "name", from: "x", to: "y", source: "config" }], + }, + ], + } as unknown as Parameters[0]); + // eslint-disable-next-line no-control-regex + const plain = rendered.replace(/\[[0-9;]*m/g, ""); + expect(plain).toContain("Plan: 0 to create, 1 to update, 0 to delete."); + expect(plain).not.toContain("INCOMPLETE"); + }); +}); diff --git a/tests/registry.test.ts b/tests/registry.test.ts index 6e9d5cb..12b8b5f 100644 --- a/tests/registry.test.ts +++ b/tests/registry.test.ts @@ -235,9 +235,15 @@ describe("createDefaults — required-but-unmanaged create fields (#73)", () => expect(defaults?.shorty).toBe("Team"); }); - it("group-role fills the required `shorty` from the declared name (truncated to 10)", () => { + it("group-role sends every field CT requires at create (#121)", () => { + // Live CT 3.135.2 rejects the POST without `type`/`isDefault`/`isHidden`, so a declared + // roleDefinition missing on the target host could not be created at all — and it failed in + // `apply`, after a green `plan`. `shorty` is still derived from the name, truncated to 10. expect(RESOURCES["group-role"]?.createDefaults?.({ name: "Verantwortlicher", groupTypeId: 2 })).toEqual({ shorty: "Verantwort", + type: "participant", // the conservative half of leader|participant + isDefault: false, + isHidden: false, }); }); From d591fb465c1957e697ffb0dad2a6d42fc4e005cd Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Mon, 17 Aug 2026 15:44:20 +0200 Subject: [PATCH 2/4] =?UTF-8?q?fix(resources):=20role-definition=20creates?= =?UTF-8?q?=20also=20need=20`sortKey`=20=E2=80=94=20verified=20live=20(#12?= =?UTF-8?q?1)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The live probe against eqrm-dev (CT 3.135.2) that #121 asked for, run at last. POSTing the pre-fix body to /group/roles is rejected with FOUR validation errors, not the three the issue could name: sortKey "Bitte eine ganze Zahl eingeben (ohne Punkt und Komma)." validation.integer type "Die Eingabe sollte eine der folgenden Werte sein: leader, participant" isDefault "Eingabe muss TRUE oder FALSE sein." validation.boolean isHidden "Eingabe muss TRUE oder FALSE sein." validation.boolean `sortKey` is the field the issue recorded only as "one more field with an empty args". Without it the previous commit's fix still failed with HTTP 400 — the same unattended-apply breakage, one field further along. It defaults to 0, which is what all 87 stock roles on the probed instance carry. Also confirmed and recorded: `isLeader` must NOT be sent. CT derives it from `type` — a role created with `type: "leader"` reads back `isLeader: true` without it ever appearing in the body — so sending it would be a second source of truth for one fact. Verified end-to-end against dev: `ct apply` creates the role definition, and the re-plan is a clean no-op (the round-trip invariant CONTRIBUTING requires). Every probe artefact was deleted and the instance left as found — 87 roles before, 87 after. Claude-Session: https://claude.ai/code/session_01V24GdD2F6LTGTF5xWoGq4i --- docs/handbuch/permissions.md | 7 +++++++ src/resources/registry.ts | 23 +++++++++++++++++------ tests/execute.test.ts | 8 +++++--- tests/registry.test.ts | 15 ++++++++++++--- 4 files changed, 41 insertions(+), 12 deletions(-) diff --git a/docs/handbuch/permissions.md b/docs/handbuch/permissions.md index e8b85bb..26eb9f8 100644 --- a/docs/handbuch/permissions.md +++ b/docs/handbuch/permissions.md @@ -296,6 +296,13 @@ diffed or reverted afterwards: | `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 diff --git a/src/resources/registry.ts b/src/resources/registry.ts index 1024baf..32c5daf 100644 --- a/src/resources/registry.ts +++ b/src/resources/registry.ts @@ -520,26 +520,37 @@ export const RESOURCES: Record = { type: r.type, }), // Fields CT REQUIRES at create but the tool does not otherwise manage (#73/#121). The old comment - // here claimed `type` was "optional/nullable — no default needed"; live CT 3.135.2 disagrees and - // rejects the POST with three validation errors: + // here claimed `type`/`isLeader`/`sortKey` were "all optional/nullable — no default needed", which + // is what made this look supported. VERIFIED LIVE on eqrm-dev, CT 3.135.2 (2026-08-17): POSTing + // the old body (`{name, nameTranslated, groupTypeId, shorty}`) is rejected with FOUR validation + // errors, not three — // + // sortKey "Bitte eine ganze Zahl eingeben (ohne Punkt und Komma)." validation.integer // type "Die Eingabe sollte eine der folgenden Werte sein: leader, participant" - // isDefault "Eingabe muss TRUE oder FALSE sein." - // isHidden "Eingabe muss TRUE oder FALSE sein." + // isDefault "Eingabe muss TRUE oder FALSE sein." validation.boolean + // isHidden "Eingabe muss TRUE oder FALSE sein." validation.boolean // - // so a declared `roleDefinition` that did not exist on the target host could not be created at - // all — and it failed in `apply`, on the host the pipeline writes to, after `plan` had been green. + // so a declared `roleDefinition` missing from the target host could not be created at all — and it + // failed in `apply`, on the host the pipeline writes to, after `plan` had been green. Adding all + // four succeeds (probe created and deleted role #288, instance left as found). // // `shorty` is 1–10 chars, non-nullable, derived from the declared `name`; padded up to the 1-char // floor for the same empty-name edge case as group-type's shorty above. // `type` defaults to `participant`: the conservative half of the choice, since `leader` confers // group leadership. A config that wants the other one declares `type: "leader"` and wins here // (createDefaults merges UNDER the declared body). + // `sortKey: 0` matches every stock role on the probed instance (all 87 carry `sortKey: 0`), so it + // is a neutral default rather than a position claim. + // + // `isLeader` is deliberately NOT sent: CT DERIVES it from `type`. The probe confirmed a role + // created with `type: "leader"` reads back `isLeader: true` without it ever being in the body, so + // sending it would be a second, redundant source of truth for the same fact. createDefaults: (r) => ({ shorty: truncatePadded(str(r, "name"), 10, 1), type: "participant", isDefault: false, isHidden: false, + sortKey: 0, }), // `groupRole` is taken by the permissions DSL (`ct.groupRole` = definePermission("group_role")), // so the master-data role resource declares under a distinct name. diff --git a/tests/execute.test.ts b/tests/execute.test.ts index c6c29f0..4a4378b 100644 --- a/tests/execute.test.ts +++ b/tests/execute.test.ts @@ -164,7 +164,7 @@ describe("executePlan", () => { }); }); - it("group-role create POST carries the required `shorty` default (#73 audit)", async () => { + it("group-role create POST carries every create-required default (#73 audit, #121)", async () => { const state = emptyState("h"); const { client, calls } = recorder({ "POST /group/roles": { id: 4 } }); const plan: Plan = { @@ -185,8 +185,9 @@ describe("executePlan", () => { expect(calls[0]).toEqual({ method: "POST", path: "/group/roles", - // `type`/`isDefault`/`isHidden` are create-only defaults CT validates (#121) — sent on the - // POST, absent from state, so they are never diffed or reverted afterwards. + // `sortKey`/`type`/`isDefault`/`isHidden` are create-only defaults CT validates (#121, all four + // confirmed live on eqrm-dev) — sent on the POST, absent from state, so they are never diffed + // or reverted afterwards. body: { name: "Verantwortlicher", groupTypeId: 2, @@ -194,6 +195,7 @@ describe("executePlan", () => { type: "participant", isDefault: false, isHidden: false, + sortKey: 0, }, }); expect(state.resources.leiter?.fields).toEqual({ name: "Verantwortlicher", groupTypeId: 2 }); diff --git a/tests/registry.test.ts b/tests/registry.test.ts index 12b8b5f..8a07741 100644 --- a/tests/registry.test.ts +++ b/tests/registry.test.ts @@ -236,17 +236,26 @@ describe("createDefaults — required-but-unmanaged create fields (#73)", () => }); it("group-role sends every field CT requires at create (#121)", () => { - // Live CT 3.135.2 rejects the POST without `type`/`isDefault`/`isHidden`, so a declared - // roleDefinition missing on the target host could not be created at all — and it failed in - // `apply`, after a green `plan`. `shorty` is still derived from the name, truncated to 10. + // Verified live on eqrm-dev, CT 3.135.2: POSTing without these four is rejected with one + // validation error each (sortKey/type/isDefault/isHidden), so a declared roleDefinition missing + // on the target host could not be created at all — and it failed in `apply`, after a green + // `plan`. `shorty` is still derived from the name, truncated to 10. expect(RESOURCES["group-role"]?.createDefaults?.({ name: "Verantwortlicher", groupTypeId: 2 })).toEqual({ shorty: "Verantwort", type: "participant", // the conservative half of leader|participant isDefault: false, isHidden: false, + sortKey: 0, }); }); + it("group-role never sends isLeader — CT derives it from `type`", () => { + // The probe created a role with `type: "leader"` and it read back `isLeader: true` without + // `isLeader` ever being in the body. Sending it would be a second source of truth for one fact. + const defaults = RESOURCES["group-role"]?.createDefaults?.({ name: "Leiter", groupTypeId: 2 }); + expect(defaults).not.toHaveProperty("isLeader"); + }); + it("pads a 1-char group-type name's namePlural up to CT's 2-char minimum", () => { // truncate(name, 30) alone has no lower bound, so a 1-char name would otherwise produce a // 1-char namePlural — invalid against CT's 2–30 char requirement (reviewer finding, #73). From 13c2f00afe76358ae11fb75de95d4d5250a4ee3e Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Mon, 17 Aug 2026 16:27:12 +0200 Subject: [PATCH 3/4] fix(resolve): qualify the pending-role check by group type (#128 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #120 made a `ct.groupRole` naming a same-run `roleDefinition` resolve as pending instead of hard-erroring. The check matched on the role NAME alone — but role names are not unique across group types, as this very file documents at length (live prod: 3 "Leiter", 6 "Organisator", 6 "Mitglied", each on a different type). So a config declaring `roleDefinition({ name: "Mitglied", groupType: "x" })` alongside a `ct.groupRole` on a group of a DIFFERENT type — one that genuinely has no such role — read as pending rather than as the config error it is. It still fails, with the same message, but from `applyPermissionPlan`'s post-apply fetch instead of at plan time: after `executePlan` has already written resources. Fail-fast is the whole reason the plan-time check exists. The check now also matches the group's own `groupTypeId`. It stays lenient wherever the answer is not knowable offline — a declaration that states no group type, or a group whose state entry predates `groupTypeId` being recorded — so it only ever turns a would-be pending back into the hard error it used to be, and only on positive evidence. #120's behaviour is unchanged everywhere else. --- src/resolve/resolver.ts | 49 +++++++++++++++++++++++++-- tests/permission-pending-role.test.ts | 32 +++++++++++++++++ 2 files changed, 79 insertions(+), 2 deletions(-) diff --git a/src/resolve/resolver.ts b/src/resolve/resolver.ts index 516ce54..81dfc8c 100644 --- a/src/resolve/resolver.ts +++ b/src/resolve/resolver.ts @@ -193,6 +193,14 @@ export class Resolver { * this is a separate index from {@link declaredByType}. */ private readonly declaredRoleDefNames = new Set(); + /** + * Slugged roleDefinition name → the group-type id(s) this config declares it on (#120, review). + * A role NAME is not unique across group types — see {@link resolveGroupTypeRole}'s note (3 "Leiter", + * 6 "Organisator", 6 "Mitglied" on live prod) — so the name alone cannot answer "will this role + * appear on THIS group?". The value is whatever the declaration carries: a raw id, or a `group-type` + * Ref resolved lazily at check time. + */ + private readonly declaredRoleDefTypes = new Map(); constructor(deps: ResolverDeps) { this.client = deps.client; @@ -206,7 +214,14 @@ export class Resolver { } set.add(d.key); if (d.type === "group-role" && typeof d.fields.name === "string") { - this.declaredRoleDefNames.add(slug(d.fields.name)); + const name = slug(d.fields.name); + this.declaredRoleDefNames.add(name); + const gt = d.fields.groupTypeId; + if (typeof gt === "number" || isRef(gt)) { + const list = this.declaredRoleDefTypes.get(name); + if (list) list.push(gt); + else this.declaredRoleDefTypes.set(name, [gt]); + } } } } @@ -308,7 +323,7 @@ export class Resolver { // declared three lines up. Roles created this run land on the group's role list once the // role-def create (tier 3) has run, which is before permissions — so the same post-apply // completion that finishes a pending group finishes this too. - if (this.declaresRoleNamed(r.role) && !hasRoleNamed(rows, r.role)) { + if ((await this.declaresRoleForGroup(r, site)) && !hasRoleNamed(rows, r.role)) { if (opts.pendingGroupRole) return pendingRef(r); throw new Error( `Cannot resolve ${refLabel(r)} referenced at ${site} on ${this.host}: "${r.role}" is declared ` + @@ -324,6 +339,36 @@ export class Resolver { return this.declaredRoleDefNames.has(slug(role)); } + /** + * Will a `roleDefinition` this config declares actually land on THIS group — i.e. is the missing + * role plausibly a same-run creation rather than a config error? + * + * The name alone must not decide it. Role names repeat across group types, so a config declaring + * `roleDefinition({ name: "Mitglied", groupType: "kleingruppe" })` next to a `ct.groupRole` on a + * group of a DIFFERENT type — one that genuinely has no such role — would otherwise be read as + * "pending", deferring a plain config error past `executePlan` to the post-apply live fetch. It + * fails there with the same message, but only after the resource tier has already written. Matching + * the group's own type restores the plan-time failure. + * + * Lenient wherever the answer is not knowable offline — an unqualified declaration, or a group whose + * state entry predates `groupTypeId` being recorded. Those keep #120's behaviour exactly; the check + * only ever turns a would-be pending back into the hard error it used to be, and only on evidence. + */ + private async declaresRoleForGroup(r: GroupRoleRef, site: string): Promise { + if (!this.declaresRoleNamed(r.role)) return false; + const declaredTypes = this.declaredRoleDefTypes.get(slug(r.role)); + if (declaredTypes === undefined) return true; // declared, but on no stated group type + const groupTypeId = this.state.resources[r.group]?.fields.groupTypeId; + if (typeof groupTypeId !== "number") return true; // this host's state cannot say — stay lenient + for (const t of declaredTypes) { + // A group-type Ref that is itself pending cannot be this existing group's type, so a non-number + // simply does not match — no need to distinguish it from a mismatch. + const id = typeof t === "number" ? t : await this.resolve(t, site); + if (id === groupTypeId) return true; + } + return false; + } + /** * Resolve a group-type-scoped role (`groupTypeRoleId`) by its (group-type, role) pair to this host's * numeric id (#76). Role names are NOT globally unique across group types (live prod, 2026-07-11: 3 diff --git a/tests/permission-pending-role.test.ts b/tests/permission-pending-role.test.ts index 029092a..cd53bff 100644 --- a/tests/permission-pending-role.test.ts +++ b/tests/permission-pending-role.test.ts @@ -92,6 +92,38 @@ describe("pending role definition in a group_role domain (#120)", () => { ); }); + // Role NAMES repeat across group types (live prod: 3 "Leiter", 6 "Organisator", 6 "Mitglied"), so + // "a roleDefinition by this name is declared" cannot on its own mean "it will land on THIS group". + // Reading it that way turns a plain config error into a pending domain, deferring the failure past + // executePlan to the post-apply fetch — same message, but only after the resource tier has written. + it("hard-errors at PLAN time when the declared role is for a different group type", async () => { + const state = stateWithGroup(); + // The group is of type 99; the declared "Admin" roleDefinition is for type 30. + state.resources.amflowsequip!.fields.groupTypeId = 99; + const client = mockClient(STOCK_ROLES); + + await expect(buildPermissionPlan(client, state, [grantBlock], desired)).rejects.toThrow( + /has no role named "Admin"/, + ); + }); + + it("still plans as pending when the declared role IS for this group's type", async () => { + const state = stateWithGroup(); + state.resources.amflowsequip!.fields.groupTypeId = 30; // matches the declaration + const client = mockClient(STOCK_ROLES); + const { items } = await buildPermissionPlan(client, state, [grantBlock], desired); + expect(items[0]?.pendingDomain).toEqual(ref.groupRole("amflowsequip", "Admin")); + }); + + it("stays lenient when state does not record the group's type — #120 behaviour, unchanged", async () => { + // `stateWithGroup` records no groupTypeId, which is the shape of an entry adopted before it was + // managed. Nothing offline can contradict the declaration, so the pending path is kept. + const state = stateWithGroup(); + expect(state.resources.amflowsequip?.fields.groupTypeId).toBeUndefined(); + const { items } = await buildPermissionPlan(mockClient(STOCK_ROLES), state, [grantBlock], desired); + expect(items[0]?.pendingDomain).toBeTruthy(); + }); + it("resolves normally once the role exists on the host — no spurious pending", async () => { const state = stateWithGroup(); const client = mockClient([...STOCK_ROLES, { id: 114, name: "Admin" }]); From 9260705aba46ce6f963c024d77551526ce2118ed Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Mon, 17 Aug 2026 16:29:05 +0200 Subject: [PATCH 4/4] docs(permissions): the pending-role path is qualified by group type Re-read against src/resolve/resolver.ts and re-signed: the #120 pending path now requires the declared roleDefinition to be for this group's group type, so a name-only match no longer defers a config error past executePlan. --- docs/handbuch/permissions.md | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/docs/handbuch/permissions.md b/docs/handbuch/permissions.md index 26eb9f8..5372a2d 100644 --- a/docs/handbuch/permissions.md +++ b/docs/handbuch/permissions.md @@ -5,7 +5,7 @@ sources: - src/resolve/resolver.ts - src/resolve/refs.ts - src/config/context.ts -sources_hash: 614b981fa6011b17 +sources_hash: 5f9ed09f1e3793b0 reviewed: 2026-08-17 --- @@ -286,6 +286,21 @@ 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: