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..5372a2d 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: 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 `, 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. + +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 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..81dfc8c 100644 --- a/src/resolve/resolver.ts +++ b/src/resolve/resolver.ts @@ -187,6 +187,20 @@ 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(); + /** + * 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; @@ -199,6 +213,16 @@ export class Resolver { this.declaredByType.set(d.type, set); } set.add(d.key); + if (d.type === "group-role" && typeof d.fields.name === "string") { + 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]); + } + } } } @@ -292,9 +316,59 @@ 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 ((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 ` + + `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)); + } + + /** + * 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 @@ -452,6 +526,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 +548,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..32c5daf 100644 --- a/src/resources/registry.ts +++ b/src/resources/registry.ts @@ -510,12 +510,48 @@ 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`/`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." validation.boolean + // isHidden "Eingabe muss TRUE oder FALSE sein." validation.boolean + // + // 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. 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..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,7 +185,18 @@ describe("executePlan", () => { expect(calls[0]).toEqual({ method: "POST", path: "/group/roles", - body: { name: "Verantwortlicher", groupTypeId: 2, shorty: "Verantwort" }, + // `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, + shorty: "Verantwort", + type: "participant", + isDefault: false, + isHidden: false, + sortKey: 0, + }, }); 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..cd53bff --- /dev/null +++ b/tests/permission-pending-role.test.ts @@ -0,0 +1,196 @@ +/** + * 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"/, + ); + }); + + // 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" }]); + 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..8a07741 100644 --- a/tests/registry.test.ts +++ b/tests/registry.test.ts @@ -235,12 +235,27 @@ 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)", () => { + // 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).