From 4e7c064fc656f4e548284886980549e839965114 Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Fri, 28 Aug 2026 14:54:28 +0200 Subject: [PATCH 1/2] fix(member-fields): reconcile rows that carry no referenceName MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up to #159. Making `referenceName` the exact identity left the documented name fallback unreachable and pointed several diagnostics at a remedy that did not work. - A live row with no `referenceName` was matched by name and then rejected by every consumer, so it could be neither updated nor created: the plan went INCOMPLETE and apply aborted the whole run. Only a row carrying a DIFFERENT reference name is a contradiction now (`conflictingReferenceName`). - The actual side reports the declared reference name for such a row, since ct never PATCHes one onto it and diffing it would never converge. - A declared field whose live row has no reference name is no longer reported as a DELETE CANDIDATE; the pass now skips rows a declaration claimed. - Identity-mismatch messages offer the non-destructive fix (declare the live `referenceName`) beside `ct destroy`, and `ct destroy --member-field` resolves the STATE-BOUND row — it previously matched on the local key alone, reported "already absent", dropped the binding and let the next apply POST a duplicate. - A live field that only shares a declaration's display name now warns and creates instead of failing the run; a near-identity (same string up to punctuation/case) is still refused. - A ref into a group that is adopted but declares no `memberFields` states no exact identity, so it keeps matching on the normalised local key. - The duplicate-match error no longer claims no row carries the exact reference name when the matches are the rows that do. Claude-Session: https://claude.ai/code/session_01PNmsG3pmvxQNA9sNmnAvYJ --- docs/handbuch/dynamic-groups.md | 2 +- docs/handbuch/group-member-fields.md | 38 ++++-- docs/handbuch/permissions.md | 2 +- src/application/operations/destroy.ts | 26 +++- src/engine/member-fields.ts | 25 +++- src/engine/synthetic.ts | 147 +++++++++++++++----- src/resolve/resolver.ts | 27 ++-- tests/destroy-command.test.ts | 20 +++ tests/member-fields.test.ts | 188 ++++++++++++++++++++++++++ 9 files changed, 411 insertions(+), 64 deletions(-) diff --git a/docs/handbuch/dynamic-groups.md b/docs/handbuch/dynamic-groups.md index c30d134..7925d4a 100644 --- a/docs/handbuch/dynamic-groups.md +++ b/docs/handbuch/dynamic-groups.md @@ -6,7 +6,7 @@ sources: - src/engine/dynamic.ts - src/engine/synthetic.ts - src/application/operations/adopt-group.ts -sources_hash: e4e2d552d54802f9 +sources_hash: 8a4a6effe4a42c70 reviewed: 2026-08-28 --- diff --git a/docs/handbuch/group-member-fields.md b/docs/handbuch/group-member-fields.md index a12e272..f19a084 100644 --- a/docs/handbuch/group-member-fields.md +++ b/docs/handbuch/group-member-fields.md @@ -1,5 +1,5 @@ --- -sources_hash: 740acdc4e12e5d8e +sources_hash: 560ebcf34c58867c title: Group member fields sources: - src/engine/member-fields.ts @@ -96,11 +96,27 @@ Two readable/writable properties need special handling: - **`referenceName`** — exact ChurchTools identity, kept separate from the local key. It is sent unchanged on create and compared byte-for-byte on every later plan; punctuation and case are significant, so `foo-bar` and `foo_bar` are - different. It is never silently PATCHed. A mismatch makes the plan - **INCOMPLETE** and tells the operator to perform an explicit replacement with - `ct destroy --member-field ::` followed by plan/apply. A - name fallback is used only for legacy/UI rows that genuinely carry no - `referenceName`; an existing different value is never ignored. + different. It is never silently PATCHed. + + A live field whose `referenceName` **differs** from the declared one makes the + plan **INCOMPLETE** and offers two ways out: manage the existing field by + declaring its `referenceName` in config, or replace it — destructively, the + field and its member values go — with + `ct destroy --member-field ::` followed by plan/apply. The + same refusal covers a live identity that differs only in punctuation or case + (`stand_bewerbung` next to a declared `stand-bewerbung`): ct neither renames it + nor mints a near-duplicate beside it. + + A live field that carries **no** `referenceName` at all — a legacy row, or one + created in the ChurchTools UI on a version that mints none — is matched by its + slugged `name`, reconciled on its mutable properties, and left without a + reference name, exactly as before. There is no competing identity to refuse. + + A live field that merely shares a declaration's display **name** while carrying + its own ChurchTools identity (`eigenesfeld_3`) is a coincidence, not a + contradiction: `ct plan` warns, names the `referenceName` to declare if that + field was meant, and otherwise proposes the create. `name` is mutable display + text, and refusing here would abort the run for every other resource too. A property outside the managed list still passes through to ChurchTools unchanged — it only earns a warning, and it is never diffed. @@ -165,7 +181,9 @@ already adopted in that run. The actual side includes the exact `referenceName` and is otherwise narrowed to the mutable properties the declaration names, so a server default ChurchTools returns can never make the two sides differ forever: **a clean apply re-plans -as a no-op.** +as a no-op.** (A row that carries no reference name reports the declared one: +ct never PATCHes `referenceName`, so diffing it there would propose the same +update on every run.) The same projection applies inside `options`: ChurchTools assigns host-specific ids to select options, while a portable config can declare `{ name }`. Those @@ -227,8 +245,10 @@ Local ct-cli keys are compared in their **normalised** form throughout — `"Wahl"` and `"wahl"` are the same local key in declarations, typed references, state and `ct destroy --member-field`. Two declarations in one group that differ only in case are therefore rejected as duplicates. ChurchTools -`referenceName` is a separate value and is always compared exactly; local-key -normalisation never applies to it. +`referenceName` is a separate value and is always compared exactly wherever a +declaration states one; local-key normalisation never applies to it. A group +that is adopted but declares no `memberFields` states none, so a reference into +it keeps matching the live row on the normalised local key. Three things follow: diff --git a/docs/handbuch/permissions.md b/docs/handbuch/permissions.md index 9c63b46..4b2c46a 100644 --- a/docs/handbuch/permissions.md +++ b/docs/handbuch/permissions.md @@ -7,7 +7,7 @@ sources: - src/resolve/resolver.ts - src/resolve/refs.ts - src/config/context.ts -sources_hash: 469fab5b33c1e5c4 +sources_hash: c3a6740c4d4bc134 reviewed: 2026-08-28 --- diff --git a/src/application/operations/destroy.ts b/src/application/operations/destroy.ts index d1e3358..b980ccf 100644 --- a/src/application/operations/destroy.ts +++ b/src/application/operations/destroy.ts @@ -14,7 +14,8 @@ import { writeBackup } from "../../engine/backup.js"; import { groupScopedRows, memberFieldStateKey, - matchesLocalKey, + knownMemberFieldId, + matchingMemberFieldRows, memberFieldItemPath, memberFieldRowId, memberFieldsReadPath, @@ -294,8 +295,8 @@ export async function runMemberFieldDeleteLoop(ctx: { const forget = async (): Promise => { const managed = state.resources[target.groupKey]; // Slugged, exactly as the apply that wrote it keyed the entry (`memberFieldStateKey`) and as - // `matchesLocalKey` matched the live row — otherwise `--member-field g::Wahl` deletes the - // field in ChurchTools but leaves `memberFields.wahl` pointing at the id it just destroyed. + // the live row was resolved above — otherwise `--member-field g::Wahl` deletes the field in + // ChurchTools but leaves `memberFields.wahl` pointing at the id it just destroyed. const stateKey = memberFieldStateKey(target.fieldKey); if (managed?.memberFields && stateKey in managed.memberFields) { const rest = { ...managed.memberFields }; @@ -308,7 +309,16 @@ export async function runMemberFieldDeleteLoop(ctx: { let fieldId: number | undefined; try { const rows = groupScopedRows(await client.get(memberFieldsReadPath(target.groupId))); - const matches = rows.filter((row) => matchesLocalKey(row, target.fieldKey)); + // The state binding wins where there is one. `--member-field g::wahl` is exactly the command + // every identity-mismatch message hands the operator, and those are the cases where the live + // row's name or referenceName has drifted away from the local key: matching on the key alone + // would report "already absent", drop the binding, and let the next apply POST a duplicate. + const matches = matchingMemberFieldRows( + rows, + target.fieldKey, + undefined, + knownMemberFieldId(state, target.groupKey, target.fieldKey), + ); if (matches.length > 1) { record(outcomes, ctx.observer, { kind: "member-field", @@ -446,7 +456,13 @@ export async function prepareDestroy( for (const target of memberFieldTargets) { try { const rows = groupScopedRows(await client.get(memberFieldsReadPath(target.groupId))); - const match = rows.find((row) => matchesLocalKey(row, target.fieldKey)); + // Same resolution as the delete loop below, so the backup holds the row that is deleted. + const match = matchingMemberFieldRows( + rows, + target.fieldKey, + undefined, + knownMemberFieldId(state, target.groupKey, target.fieldKey), + )[0]; if (match) actual.set(target.identity, match); } catch (err) { throw new CtApplicationError( diff --git a/src/engine/member-fields.ts b/src/engine/member-fields.ts index 2563630..ad6b7a4 100644 --- a/src/engine/member-fields.ts +++ b/src/engine/member-fields.ts @@ -197,11 +197,17 @@ export function knownMemberFieldId(state: State, groupKey: string, localKey: str * Prefer a state-bound id; otherwise match ChurchTools' identity-bearing `referenceName` EXACTLY. * A name fallback is permitted only for old/UI rows that genuinely carry no referenceName. Once CT * supplies one, punctuation and case are data: `foo-bar` and `foo_bar` are different identities. + * + * `referenceName === undefined` means the caller has NO declared exact identity to compare against — + * a ref into a group that is adopted but does not declare `memberFields`, or a `ct destroy + * --member-field` target. Those callers keep the pre-#158 local-key affinity ({@link + * matchesLocalKey}), because there is no config to state the exact string and demanding one would + * turn a working reference into a hard error. */ export function matchingMemberFieldRows( rows: MemberFieldRow[], localKey: string, - referenceName: string, + referenceName: string | undefined, knownId?: number, ): MemberFieldRow[] { if (knownId !== undefined) { @@ -210,6 +216,7 @@ export function matchingMemberFieldRows( // when a response variant was parsed incompletely. return rows.filter((row) => memberFieldRowId(row) === knownId); } + if (referenceName === undefined) return rows.filter((row) => matchesLocalKey(row, localKey)); return rows.filter((row) => { const liveReference = memberFieldReferenceName(row); if (liveReference !== undefined) return liveReference === referenceName; @@ -218,6 +225,22 @@ export function matchingMemberFieldRows( }); } +/** + * The live exact identity that CONTRADICTS `referenceName`, or `undefined` when nothing does. + * + * A row carrying no `referenceName` at all contradicts nothing: it is the legacy/UI row the name + * fallback in {@link matchingMemberFieldRows} exists for, and since ct never PATCHes + * `referenceName` there is no rename to refuse. Treating "missing" as "different" would make every + * such row unreconcilable — no update, no create, just a permanent error. + */ +export function conflictingReferenceName( + row: MemberFieldRow, + referenceName: string, +): string | undefined { + const live = memberFieldReferenceName(row); + return live !== undefined && live !== referenceName ? live : undefined; +} + /** * The exact identity-bearing ChurchTools reference name, when the row carries one. */ diff --git a/src/engine/synthetic.ts b/src/engine/synthetic.ts index d5e09c2..d13121b 100644 --- a/src/engine/synthetic.ts +++ b/src/engine/synthetic.ts @@ -14,10 +14,12 @@ import { assertNotPeople } from "./guard.js"; import { deepEqual } from "./plan.js"; import { mapConcurrent } from "../util/concurrency.js"; import { info, warn, formatError } from "../ui.js"; +import { slug } from "../resources/registry.js"; import { normalizeDynamic, normalizeRuleset, putRulesetBody, resolveRulesetRef } from "./dynamic.js"; import { formatPortablizeWarnings, scanUnportablized } from "../config/query-refs.js"; import { actualMemberFieldProps, + conflictingReferenceName, groupScopedRows, knownMemberFieldId, memberFieldStateKey, @@ -266,42 +268,104 @@ const memberFieldsField: SyntheticField = { ); }); const mismatched = new Set(); + // Every row a declaration accounts for — matched by identity, or merely a look-alike that + // was reported. Collected here so the DELETE CANDIDATE pass below can skip them by row + // rather than by reference name: a legacy row carries none, and comparing `"" ` against the + // declared names would flag a field the config demonstrably declares. + const claimed = new Set(); + const notes: string[] = []; for (const spec of d.memberFields!) { if (stale.has(spec.key)) continue; + const identity = memberFieldIdentity(d.key, spec.key); const knownId = knownMemberFieldId(state, d.key, spec.key); const matches = matchingMemberFieldRows(rows, spec.key, spec.referenceName, knownId); - const affinity = - matches.length > 0 ? matches : rows.filter((row) => matchesLocalKey(row, spec.key)); - if (affinity.length === 0) continue; - if (affinity.length > 1) { + for (const row of matches) claimed.add(row); + if (matches.length > 1) { mismatched.add(spec.key); - const identity = memberFieldIdentity(d.key, spec.key); errors.push( - `member field ${identity}: no row has the exact ChurchTools referenceName ` + - `${JSON.stringify(spec.referenceName)}, but ${affinity.length} rows collapse onto local ` + - `ct-cli key ${JSON.stringify(spec.key)} (${affinity - .map((row) => JSON.stringify(memberFieldReferenceName(row) ?? "")) - .join(", ")}). "-" and "_" are not equivalent API identities. Refusing to plan a ` + - `duplicate; choose a distinct local key or replace the intended field explicitly.`, + `member field ${identity}: ${matches.length} live fields on group #${managed.id} carry ` + + `this identity (${matches + .map( + (row) => + `#${String(memberFieldRowId(row) ?? "unknown")} ` + + JSON.stringify(memberFieldReferenceName(row) ?? ""), + ) + .join(", ")}). ct will not guess which one the declaration means; remove or rename ` + + `one in ChurchTools, then re-run plan/apply.`, ); continue; } - const row = affinity[0]!; - const liveReference = memberFieldReferenceName(row); - if (liveReference === spec.referenceName) continue; - mismatched.add(spec.key); - const identity = memberFieldIdentity(d.key, spec.key); - errors.push( - `member field ${identity} (#${String(memberFieldRowId(row) ?? "unknown")}): exact ` + - `ChurchTools referenceName is ${liveReference === undefined ? "missing" : JSON.stringify(liveReference)}, ` + - `but config requires ${JSON.stringify(spec.referenceName)}. Local ct-cli key ` + - `${JSON.stringify(spec.key)} does not change that API identity; "-" and "_" are not ` + - `equivalent. ct will not rename an identity-bearing field silently. Replace it ` + - `explicitly with \`ct destroy --member-field ${identity}\`, then re-run plan/apply.`, - ); + if (matches.length === 1) { + const row = matches[0]!; + // A row with NO referenceName is the legacy/UI row the name fallback exists for: there + // is no competing identity to refuse, and ct never PATCHes referenceName, so it is + // reconciled on its mutable properties exactly as before #158. + const conflicting = conflictingReferenceName(row, spec.referenceName); + if (conflicting === undefined) continue; + mismatched.add(spec.key); + errors.push( + `member field ${identity} (#${String(memberFieldRowId(row) ?? "unknown")}): exact ` + + `ChurchTools referenceName is ${JSON.stringify(conflicting)}, but config requires ` + + `${JSON.stringify(spec.referenceName)}. Local ct-cli key ${JSON.stringify(spec.key)} ` + + `does not change that API identity; "-" and "_" are not equivalent. ct will not ` + + `rename an identity-bearing field silently. Either MANAGE the existing field by ` + + `declaring \`referenceName: ${JSON.stringify(conflicting)}\` on it, or REPLACE it ` + + `(destructive — the field and its member values are deleted) with ` + + `\`ct destroy --member-field ${identity}\`, then re-run plan/apply.`, + ); + continue; + } + // No row carries this identity, so the field will be CREATED. Two very different kinds of + // live row can still look related, and they are treated differently on purpose. + // + // A NEAR-IDENTITY — a live referenceName that differs from the declared one (or from the + // local key) only in punctuation or case — is refused: "-" and "_" are not equivalent API + // identities, so creating would mint a second, near-indistinguishable field, and the + // declaration is far more likely to be a typo or a half-finished rename. + const nearIdentities = rows.filter((row) => { + const live = memberFieldReferenceName(row); + return ( + live !== undefined && + (slug(live) === slug(spec.referenceName) || slug(live) === slug(spec.key)) + ); + }); + for (const row of nearIdentities) claimed.add(row); + if (nearIdentities.length > 0) { + mismatched.add(spec.key); + errors.push( + `member field ${identity}: live field ` + + `#${String(memberFieldRowId(nearIdentities[0]!) ?? "unknown")} on group ` + + `#${managed.id} carries referenceName ` + + `${JSON.stringify(memberFieldReferenceName(nearIdentities[0]!))}, which differs from ` + + `the declared ${JSON.stringify(spec.referenceName)} only in punctuation or case. ` + + `"-" and "_" are not equivalent API identities, so ct will neither rename that field ` + + `nor plan a near-duplicate create. Either declare ` + + `\`referenceName: ${JSON.stringify(memberFieldReferenceName(nearIdentities[0]!))}\` to ` + + `manage the existing field, or replace it (destructive) with ` + + `\`ct destroy --member-field ${identity}\`, then re-run plan/apply.`, + ); + continue; + } + // A row that merely shares the declaration's slugged NAME is only a warning. `name` is a + // mutable display property, its ChurchTools-minted referenceName says the field was never + // ct-managed, and a fold error marks the WHOLE run incomplete — aborting apply for every + // other resource over a coincidence of naming. + const lookalikes = rows.filter((row) => matchesLocalKey(row, spec.key)); + for (const row of lookalikes) claimed.add(row); + for (const row of lookalikes) { + notes.push( + `member field ${identity}: will be CREATED, but live field ` + + `#${String(memberFieldRowId(row) ?? "unknown")} on group #${managed.id} ` + + `(name ${JSON.stringify(typeof row.name === "string" ? row.name : "")}, ` + + `referenceName ${JSON.stringify(memberFieldReferenceName(row) ?? "")}) looks ` + + `like it. referenceName is the exact identity and does not match, so ct plans a ` + + `second field. To manage the existing one instead, declare ` + + `\`referenceName: ${JSON.stringify(memberFieldReferenceName(row) ?? spec.referenceName)}\`.`, + ); + } } - for (const message of errors) warn(message); - return { key: d.key, rows, stale, mismatched, errors }; + for (const message of [...errors, ...notes]) warn(message); + return { key: d.key, rows, stale, mismatched, claimed, errors }; } catch (err) { // Same honesty rule as the dynamic fold (#126): an unread actual is NOT a known-absent one, // so the desired side stays unfolded and the group is reported unreadable rather than having @@ -311,6 +375,7 @@ const memberFieldsField: SyntheticField = { rows: undefined, stale: new Set(), mismatched: new Set(), + claimed: new Set(), errors: [`member fields ${d.key} (#${managed.id}): ${formatError(err)}`], }; } @@ -321,6 +386,7 @@ const memberFieldsField: SyntheticField = { const rowsByKey = new Map(outcomes.filter((o) => o.rows !== undefined).map((o) => [o.key, o.rows!])); const staleByKey = new Map(outcomes.map((o) => [o.key, o.stale])); const mismatchedByKey = new Map(outcomes.map((o) => [o.key, o.mismatched])); + const claimedByKey = new Map(outcomes.map((o) => [o.key, o.claimed])); const augmented = desired.map((d) => { if (d.type !== "group" || d.memberFields === undefined) return d; @@ -347,16 +413,19 @@ const memberFieldsField: SyntheticField = { // can be acted on (the apply path refuses it by name). if (matches.length === 1) { a[pseudo] = { - referenceName: memberFieldReferenceName(matches[0]!), + // A legacy row carries no referenceName and ct never PATCHes one onto it, so reporting + // the live `undefined` here would diff against the declared string on every run — an + // update that can never converge. Absent means "not knowable on this row", not drift. + referenceName: memberFieldReferenceName(matches[0]!) ?? spec.referenceName, ...actualMemberFieldProps(matches[0]!, spec.props), }; } } if (rows) { - const declared = new Set(d.memberFields.map((f) => f.referenceName)); + const claimed = claimedByKey.get(d.key) ?? new Set(); for (const row of rows) { const local = localKeyOf(row); - if (!local || declared.has(memberFieldReferenceName(row) ?? "")) continue; + if (!local || claimed.has(row)) continue; warn( `group "${d.key}": member field "${memberFieldIdentity(d.key, local)}" exists in ` + `ChurchTools but is not declared — DELETE CANDIDATE, left untouched. ct never removes a ` + @@ -389,8 +458,8 @@ const memberFieldsField: SyntheticField = { if (matches.length > 1) { throw new Error( `group member field "${memberFieldIdentity(key, local)}": ${matches.length} fields on group ` + - `#${id} answer to the local key "${local}". Rename one in ChurchTools so the key is unique ` + - `within the group, then re-apply.`, + `#${id} carry the identity ${JSON.stringify(referenceName)}. Remove or rename one in ` + + `ChurchTools so it is unique within the group, then re-apply.`, ); } @@ -403,15 +472,19 @@ const memberFieldsField: SyntheticField = { }; if (matches.length === 1) { - const liveReference = memberFieldReferenceName(matches[0]!); - if (liveReference !== referenceName) { + // Only a row carrying a DIFFERENT referenceName is a contradiction. One carrying none is the + // legacy/UI row matched by name; its mutable properties are updated and its (absent) identity + // is left exactly as it was, since referenceName is never PATCHed. + const conflicting = conflictingReferenceName(matches[0]!, referenceName); + if (conflicting !== undefined) { throw new Error( `group member field "${memberFieldIdentity(key, local)}": ChurchTools field ` + `#${String(memberFieldRowId(matches[0]!) ?? "unknown")} has exact referenceName ` + - `${liveReference === undefined ? "" : JSON.stringify(liveReference)}, but config ` + - `requires ${JSON.stringify(referenceName)}. Refusing to rename it silently; replace it ` + - `explicitly with \`ct destroy --member-field ${memberFieldIdentity(key, local)}\`, then ` + - `re-run plan/apply.`, + `${JSON.stringify(conflicting)}, but config requires ${JSON.stringify(referenceName)}. ` + + `Refusing to rename it silently; either declare ` + + `\`referenceName: ${JSON.stringify(conflicting)}\` to manage the existing field, or ` + + `replace it (destructive) with ` + + `\`ct destroy --member-field ${memberFieldIdentity(key, local)}\`, then re-run plan/apply.`, ); } const fieldId = memberFieldRowId(matches[0]!); diff --git a/src/resolve/resolver.ts b/src/resolve/resolver.ts index 79769bf..0c91814 100644 --- a/src/resolve/resolver.ts +++ b/src/resolve/resolver.ts @@ -30,6 +30,7 @@ import type { State } from "../state/state.js"; import type { DesiredResource } from "../engine/types.js"; import { slug } from "../resources/registry.js"; import { + conflictingReferenceName, groupScopedRows, knownMemberFieldId, matchingMemberFieldRows, @@ -519,19 +520,25 @@ export class Resolver { } const rows = await this.memberFieldList(managed.id); const knownId = knownMemberFieldId(this.state, r.group, r.field); - const desiredReferenceName = declaredReferenceName ?? r.field; - const matches = matchingMemberFieldRows(rows, r.field, desiredReferenceName, knownId); - if (matches.length === 1) { - const liveReference = matches[0]!.referenceName; - if (liveReference !== desiredReferenceName) { + // Only a DECLARED field has an exact identity to hold the live row to. For a group that is + // adopted but declares no `memberFields`, the ref names a local ct-cli key and nothing in + // config states the ChurchTools spelling, so the pre-#158 local-key affinity still applies — + // otherwise `ref.groupMemberField("g", "stand_bewerbung")` would stop resolving the live + // `stand-bewerbung` it has always resolved. + const matches = matchingMemberFieldRows(rows, r.field, declaredReferenceName, knownId); + if (matches.length === 1 && declaredReferenceName !== undefined) { + const conflicting = conflictingReferenceName(matches[0]!, declaredReferenceName); + if (conflicting !== undefined) { throw new Error( `Cannot resolve ${refLabel(r)} referenced at ${site} on ${this.host}: ChurchTools field ` + - `#${String(memberFieldRowId(matches[0]!))} is state-bound to ` + + `#${String(memberFieldRowId(matches[0]!))} is the live match for ` + `"${memberFieldIdentity(r.group, r.field)}", but its exact referenceName is ` + - `${liveReference === undefined ? "missing" : JSON.stringify(liveReference)} instead of ` + - `${JSON.stringify(desiredReferenceName)}. ` + - `ct will not rename an identity-bearing field silently. Replace it explicitly with ` + - `\`ct destroy --member-field ${memberFieldIdentity(r.group, r.field)}\`, then re-run plan/apply.`, + `${JSON.stringify(conflicting)} instead of the declared ` + + `${JSON.stringify(declaredReferenceName)}. ` + + `ct will not rename an identity-bearing field silently. Either declare ` + + `\`referenceName: ${JSON.stringify(conflicting)}\` on that field, or replace it ` + + `(destructive) with \`ct destroy --member-field ${memberFieldIdentity(r.group, r.field)}\`, ` + + `then re-run plan/apply.`, ); } } diff --git a/tests/destroy-command.test.ts b/tests/destroy-command.test.ts index cfc9eb6..6c88def 100644 --- a/tests/destroy-command.test.ts +++ b/tests/destroy-command.test.ts @@ -230,6 +230,26 @@ describe("ct destroy --member-field (#135)", () => { expect(after.resources.area!.memberFields).toBeUndefined(); }); + it("deletes the STATE-BOUND row even when its live name no longer matches the key", async () => { + // Every identity-mismatch message hands the operator exactly this command, and those are the + // cases where the live row drifted away from the local key. Matching on the key alone would + // report "already absent", drop the binding, and let the next apply POST a duplicate. + getMock.mockImplementation((async (path: string) => { + if (path === "/groups/hierarchies") return []; + if (path === "/groups/1/memberfields") return [{ id: 701, type: "group", name: "Birkman" }]; + return { name: path }; + }) as never); + const state = emptyState(HOST); + state.resources.area = group("area", 1, { memberFields: { birkmann: 701 } }); + await saveState(statePath, state); + + await runDestroy(["--member-field", "area::birkmann", "--state", statePath, "--force"]); + + expect(calls).toEqual([{ method: "DELETE", path: "/groups/1/memberfields/group/701" }]); + const after = await loadState(statePath, HOST); + expect(after.resources.area!.memberFields).toBeUndefined(); + }); + it("still refuses to delete anything with neither --target nor --member-field", async () => { await saveState(statePath, emptyState(HOST)); await expect(runDestroy(["--state", statePath, "--force"])).rejects.toThrow( diff --git a/tests/member-fields.test.ts b/tests/member-fields.test.ts index da4e8c5..33f2dd7 100644 --- a/tests/member-fields.test.ts +++ b/tests/member-fields.test.ts @@ -922,3 +922,191 @@ describe("groupScopedRows — { type, field } wrapper (#154 review)", () => { expect(groupScopedRows([row])).toEqual([row]); }); }); + +/** + * #159 review: the exact-identity rule of #158 must not swallow the rows it was never about. + * + * A live row that carries NO `referenceName` — the legacy/UI row the name fallback exists for — has + * no competing identity, so it is reconciled exactly as it was before #158. And a live row that + * merely shares a declaration's display NAME is a coincidence, not a contradiction: it is reported, + * never allowed to mark the run incomplete. + */ +describe("group member fields — rows without a referenceName (#158 follow-up)", () => { + const legacyRow = { id: 501, type: "group", name: "Birkman", fieldTypeCode: "text" }; + + function declaringPraktikum(memberFields: { key: string; [prop: string]: unknown }[]): { + resources: DesiredResource[]; + } { + const { ct: dsl, resources } = createContext(); + dsl.group({ + key: "praktikum_1", + name: "Praktikum 1", + groupTypeId: 5, + groupStatusId: 1, + memberFields, + }); + return { resources }; + } + + it("reconciles a state-bound row that carries no referenceName instead of blocking the plan", async () => { + const ct = makeCt(); + ct.memberFields[100] = [legacyRow]; + const state = stateWith({ + praktikum_1: { + type: "group", + id: 100, + fields: { name: "Praktikum 1", groupTypeId: 5, groupStatusId: 1 }, + }, + }); + state.resources.praktikum_1!.memberFields = { birkmann: 501 }; + const { resources } = declaringPraktikum([ + { key: "birkmann", name: "Birkmann", fieldTypeCode: "text" }, + ]); + + const { plan, fetchErrors } = await buildPlan(ct.client, state, resources); + expect(fetchErrors).toEqual([]); + expect(plan.items[0]!.changes.find((c) => c.field === memberFieldPseudo("birkmann"))).toMatchObject({ + to: { referenceName: "birkmann", name: "Birkmann" }, + }); + + await executePlan(plan, { client: ct.client, state, statePath: "s.json", save: async () => {} }); + // PATCHed in place — a row without a referenceName must never be re-created as a duplicate. + expect(ct.calls).toContain("PATCH /groups/100/memberfields/group/501"); + expect(ct.memberFields[100]).toHaveLength(1); + }); + + it("re-plans as a no-op once a referenceName-less row is converged", async () => { + // The actual side cannot report the live `undefined` here: ct never PATCHes referenceName, so + // diffing it against the declared string would propose the same update on every single run. + const ct = makeCt(); + ct.memberFields[100] = [{ id: 501, type: "group", name: "Birkmann", fieldTypeCode: "text" }]; + const state = stateWith({ + praktikum_1: { + type: "group", + id: 100, + fields: { name: "Praktikum 1", groupTypeId: 5, groupStatusId: 1 }, + }, + }); + state.resources.praktikum_1!.memberFields = { birkmann: 501 }; + const { resources } = declaringPraktikum([ + { key: "birkmann", name: "Birkmann", fieldTypeCode: "text" }, + ]); + + const { plan } = await buildPlan(ct.client, state, resources); + expect(plan.items[0]!.changes.some((c) => c.field.startsWith("memberField:"))).toBe(false); + }); + + it("matches an unbound referenceName-less row by name and never calls it a delete candidate", async () => { + const ct = makeCt(); + ct.memberFields[100] = [{ id: 501, type: "group", name: "Wahl", fieldTypeCode: "text" }]; + const state = stateWith({ + praktikum_1: { + type: "group", + id: 100, + fields: { name: "Praktikum 1", groupTypeId: 5, groupStatusId: 1 }, + }, + }); + const { resources } = declaringPraktikum([{ key: "wahl", name: "Wahl (neu)", fieldTypeCode: "text" }]); + + const { plan, fetchErrors } = await buildPlan(ct.client, state, resources); + expect(fetchErrors).toEqual([]); + expect(warnings.join("\n")).not.toContain("DELETE CANDIDATE"); + await executePlan(plan, { client: ct.client, state, statePath: "s.json", save: async () => {} }); + expect(ct.calls).toContain("PATCH /groups/100/memberfields/group/501"); + expect(ct.memberFields[100]).toHaveLength(1); + }); + + it("warns but still creates when only the display name of an unmanaged field collides", async () => { + // `name` is a mutable display property and `eigenesfeld_3` is a different API identity, so this + // is a coincidence — and a fold error here would abort apply for every other resource too. + const ct = makeCt(); + ct.memberFields[100] = [{ id: 501, type: "group", referenceName: "eigenesfeld_3", name: "Wahl" }]; + const state = stateWith({ + praktikum_1: { + type: "group", + id: 100, + fields: { name: "Praktikum 1", groupTypeId: 5, groupStatusId: 1 }, + }, + }); + const { resources } = declaringPraktikum([{ key: "wahl", name: "Wahl", fieldTypeCode: "text" }]); + + const { plan, fetchErrors } = await buildPlan(ct.client, state, resources); + expect(fetchErrors).toEqual([]); + expect(warnings.join("\n")).toMatch(/will be CREATED.*eigenesfeld_3.*declare/s); + expect(warnings.join("\n")).not.toContain("DELETE CANDIDATE"); + expect(plan.items[0]!.changes.some((c) => c.field === memberFieldPseudo("wahl"))).toBe(true); + }); + + it("offers the non-destructive remedy alongside the destructive one on a real identity mismatch", async () => { + const ct = makeCt(); + ct.memberFields[100] = [{ id: 501, type: "group", referenceName: "eigenesfeld_3", name: "Wahl" }]; + const state = stateWith({ + praktikum_1: { + type: "group", + id: 100, + fields: { name: "Praktikum 1", groupTypeId: 5, groupStatusId: 1 }, + }, + }); + state.resources.praktikum_1!.memberFields = { wahl: 501 }; + const { resources } = declaringPraktikum([{ key: "wahl", name: "Wahl", fieldTypeCode: "text" }]); + + const { fetchErrors } = await buildPlan(ct.client, state, resources); + expect(fetchErrors.join("\n")).toContain('declaring `referenceName: "eigenesfeld_3"`'); + expect(fetchErrors.join("\n")).toContain("ct destroy --member-field praktikum_1::wahl"); + }); + + it("does not claim the exact referenceName is absent when two rows genuinely carry it", async () => { + const ct = makeCt(); + ct.memberFields[100] = [ + { id: 501, type: "group", referenceName: "wahl", name: "Wahl" }, + { id: 502, type: "group", referenceName: "wahl", name: "Wahl (Kopie)" }, + ]; + const state = stateWith({ + praktikum_1: { + type: "group", + id: 100, + fields: { name: "Praktikum 1", groupTypeId: 5, groupStatusId: 1 }, + }, + }); + const { resources } = declaringPraktikum([{ key: "wahl", name: "Wahl", fieldTypeCode: "text" }]); + + const { fetchErrors } = await buildPlan(ct.client, state, resources); + expect(fetchErrors.join("\n")).toMatch(/2 live fields on group #100 carry this identity/); + expect(fetchErrors.join("\n")).not.toContain("no row has the exact"); + }); + + it("resolves a ref into an adopted group that declares no memberFields, as it always has", async () => { + // Nothing in config states the exact ChurchTools spelling for such a group, so the ref names + // the local ct-cli key and the pre-#158 normalised match still applies. + const ct = makeCt(); + ct.memberFields[100] = [{ id: 504, type: "group", referenceName: "stand-bewerbung", name: "Stand" }]; + const state = stateWith({ + praktikum_1: { + type: "group", + id: 100, + fields: { name: "Praktikum 1", groupTypeId: 5, groupStatusId: 1 }, + }, + }); + state.resources.praktikum_1!.memberFields = { stand_bewerbung: 504 }; + const { ct: dsl, resources } = createContext(); + dsl.group({ + key: "praktikum_1", + name: "Praktikum 1", + groupTypeId: 5, + groupStatusId: 1, + dynamic: { + status: "active", + ruleset: { + description: "stand", + query: { + "==": [{ var: "memberfield.id" }, ref.groupMemberField("praktikum_1", "stand_bewerbung")], + }, + process: {}, + }, + }, + }); + + const { plan } = await buildPlan(ct.client, state, resources); + expect(JSON.stringify(plan)).toContain("504"); + }); +}); From 1fd4e16235a64dd04872e66d46bd7ef943338d2f Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Fri, 28 Aug 2026 15:00:27 +0200 Subject: [PATCH 2/2] style: apply prettier to the member-field review fixes Claude-Session: https://claude.ai/code/session_01PNmsG3pmvxQNA9sNmnAvYJ --- docs/handbuch/dynamic-groups.md | 2 +- docs/handbuch/group-member-fields.md | 2 +- src/engine/member-fields.ts | 5 +---- src/engine/synthetic.ts | 3 +-- tests/member-fields.test.ts | 8 ++------ 5 files changed, 6 insertions(+), 14 deletions(-) diff --git a/docs/handbuch/dynamic-groups.md b/docs/handbuch/dynamic-groups.md index 7925d4a..edff036 100644 --- a/docs/handbuch/dynamic-groups.md +++ b/docs/handbuch/dynamic-groups.md @@ -6,7 +6,7 @@ sources: - src/engine/dynamic.ts - src/engine/synthetic.ts - src/application/operations/adopt-group.ts -sources_hash: 8a4a6effe4a42c70 +sources_hash: e38b8c0f6032d5cc reviewed: 2026-08-28 --- diff --git a/docs/handbuch/group-member-fields.md b/docs/handbuch/group-member-fields.md index f19a084..1da01b5 100644 --- a/docs/handbuch/group-member-fields.md +++ b/docs/handbuch/group-member-fields.md @@ -1,5 +1,5 @@ --- -sources_hash: 560ebcf34c58867c +sources_hash: c18b710bff24503d title: Group member fields sources: - src/engine/member-fields.ts diff --git a/src/engine/member-fields.ts b/src/engine/member-fields.ts index ad6b7a4..bc18fe0 100644 --- a/src/engine/member-fields.ts +++ b/src/engine/member-fields.ts @@ -233,10 +233,7 @@ export function matchingMemberFieldRows( * `referenceName` there is no rename to refuse. Treating "missing" as "different" would make every * such row unreconcilable — no update, no create, just a permanent error. */ -export function conflictingReferenceName( - row: MemberFieldRow, - referenceName: string, -): string | undefined { +export function conflictingReferenceName(row: MemberFieldRow, referenceName: string): string | undefined { const live = memberFieldReferenceName(row); return live !== undefined && live !== referenceName ? live : undefined; } diff --git a/src/engine/synthetic.ts b/src/engine/synthetic.ts index d13121b..9051201 100644 --- a/src/engine/synthetic.ts +++ b/src/engine/synthetic.ts @@ -325,8 +325,7 @@ const memberFieldsField: SyntheticField = { const nearIdentities = rows.filter((row) => { const live = memberFieldReferenceName(row); return ( - live !== undefined && - (slug(live) === slug(spec.referenceName) || slug(live) === slug(spec.key)) + live !== undefined && (slug(live) === slug(spec.referenceName) || slug(live) === slug(spec.key)) ); }); for (const row of nearIdentities) claimed.add(row); diff --git a/tests/member-fields.test.ts b/tests/member-fields.test.ts index 33f2dd7..81dde54 100644 --- a/tests/member-fields.test.ts +++ b/tests/member-fields.test.ts @@ -959,9 +959,7 @@ describe("group member fields — rows without a referenceName (#158 follow-up)" }, }); state.resources.praktikum_1!.memberFields = { birkmann: 501 }; - const { resources } = declaringPraktikum([ - { key: "birkmann", name: "Birkmann", fieldTypeCode: "text" }, - ]); + const { resources } = declaringPraktikum([{ key: "birkmann", name: "Birkmann", fieldTypeCode: "text" }]); const { plan, fetchErrors } = await buildPlan(ct.client, state, resources); expect(fetchErrors).toEqual([]); @@ -988,9 +986,7 @@ describe("group member fields — rows without a referenceName (#158 follow-up)" }, }); state.resources.praktikum_1!.memberFields = { birkmann: 501 }; - const { resources } = declaringPraktikum([ - { key: "birkmann", name: "Birkmann", fieldTypeCode: "text" }, - ]); + const { resources } = declaringPraktikum([{ key: "birkmann", name: "Birkmann", fieldTypeCode: "text" }]); const { plan } = await buildPlan(ct.client, state, resources); expect(plan.items[0]!.changes.some((c) => c.field.startsWith("memberField:"))).toBe(false);