From fa1aca41c3c12b244da0e664c19be64cd24b56f5 Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Mon, 17 Aug 2026 14:52:12 +0200 Subject: [PATCH 1/3] fix(adoption): make adopted config portable between hosts (#114, #115, #118, #119, #122, #123, #125, #127) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "Adoption & portability" milestone: adopting existing CT structure, un-adopting, and making declarations portable between hosts that materialize inheritance differently. #119 + #114 — reconciliation decided whether a declared grant was SATISFIED from how the live row was RECORDED, and provenance is not stable across two hosts of one instance even when the effective permissions are identical. The same right is system-authored (`modifiedPid === -1`) on one host and user-authored on the other; the same 15 group-member rights read `isInherited: true` on one host and `false` on the other (measured: 18 of 63 `group_role` domains carried inherited rows on one host, zero anywhere on the other). Omitting the right revoked it there; declaring it planned `+1 grant` here — so the domain had to be left undeclared, though nothing about it is genuinely undeclarable. Satisfaction is now judged against the EFFECTIVE set (`normalizeEffective`: direct ∪ inherited ∪ system baseline) while revocation stays judged against the OWNED set, so ct still never authors or revokes a row it did not write — it only stops proposing to re-author what the platform already grants. `ct adopt grants` emits the effective set to match, and names in a footer how many of the emitted grants are inherited or baseline on this host. #115 — `ct adopt grants` emitted `run `ct adopt department -1`` for CT's "alle" sentinel. `-1` is not an id: it means every value of the dimension on whatever host reads it, so it is already portable and there is no resource to adopt. It now gets a one-line explanation instead of a hint that sends you looking for something that cannot exist. In a real adoption run this fires on most of the broadly-scoped grants. #127 — `person.id` was the one entity var the ruleset portability audit never mentioned, so a raw person id frozen into a cross-host ruleset could only be found by reading the captured JSON by hand. It is reported now, worded as unfixable rather than pending: ct does not manage people, so the remedy is a decision, not a command. #125 — a ruleset's `role.id` portablized to a (group-type, role NAME) pair, and a duplicate role name on one type was unfixable in config: renaming edits CT master data (here, CT's own stock `leader`), and a numeric id differs per host with no env to branch on. Capture now prefers a managed `role-def` key resolved from state — the same "adopt the target to portablize it" move groups and campuses already use — falling back to the pair for an unmanaged role. The ambiguity error names the remedy that actually works. #123 — re-adopting an already-managed resource silently re-keyed it, which the "snapshot was refreshed" message did not mention. That happens on the documented ruleset-refresh workflow, which passes a LIST of ids — the one mode where `-k` is rejected. The config then matched nothing in state and the next plan read as "one to create, one to destroy" for an untouched resource. An adopted key is now kept (and the derived one reported) unless `--rekey` is passed; the ruleset filename follows the kept key, so a refresh overwrites the file the config points at instead of writing a second one. #122 — `ct state rm ` un-adopts: removes the entry, makes no HTTP call, leaves the resource in ChurchTools. Backing out an adoption previously meant hand-editing the state file, and `ct destroy` is the opposite of what un-adopting means. Refuses a key the config still declares (that would plan a CREATE for a live resource) unless `--force`. #118 — campus was the only resource deriving its key from `shorty`, producing keys like `of`, `wu` and `swa` for Dietzenbach, Würzburg and Idstein. `shorty` is a display abbreviation an admin edits to make a UI column fit, so it is also less stable than the name. It now derives from `name` like everything else, falling back to `shorty` only when `name` is empty; `shorty` is still written as a managed field. BREAKING CHANGE: campus logical keys now derive from `name`, not `shorty`. A campus adopted without an explicit `-k` has a key that no longer matches what adoption would derive; re-adopt with `-k ` to keep it, or `--rekey` to move to the name-derived one. BREAKING CHANGE: `ct adopt grants` now emits inherited and system-baseline grants, and reconciliation treats them as satisfying a declaration. A config that relied on an inherited right being revoked must now declare that revoke explicitly. Claude-Session: https://claude.ai/code/session_01V24GdD2F6LTGTF5xWoGq4i --- README.md | 7 ++ docs/handbuch/dynamic-groups.md | 88 +++++++++++++++++- docs/handbuch/permissions.md | 92 +++++++++++++------ src/commands/adopt-group.ts | 35 +++++++- src/commands/adopt.ts | 22 ++++- src/commands/state.ts | 102 ++++++++++++++++++++- src/config/query-refs.ts | 51 +++++++++-- src/permissions/adopt.ts | 112 +++++++++++++++++++---- src/permissions/grants.ts | 83 +++++++++++++++-- src/permissions/plan.ts | 11 ++- src/resolve/resolver.ts | 10 ++- src/resources/registry.ts | 11 ++- src/state/state.ts | 39 ++++++++ tests/adopt-identity.test.ts | 84 ++++++++++++++++++ tests/adopt.test.ts | 6 +- tests/permission-adopt.test.ts | 122 +++++++++++++++++++++++-- tests/query-refs.test.ts | 123 ++++++++++++++++++++++++++ tests/registry.test.ts | 13 ++- tests/state-rm-command.test.ts | 152 ++++++++++++++++++++++++++++++++ 19 files changed, 1085 insertions(+), 78 deletions(-) create mode 100644 tests/adopt-identity.test.ts create mode 100644 tests/state-rm-command.test.ts diff --git a/README.md b/README.md index e8c2387..1c95403 100644 --- a/README.md +++ b/README.md @@ -105,11 +105,18 @@ ct get groups # JSON to stdout — pipe into jq (every page, not ct adopt campus 0 # bring ONE existing resource under management ct coverage # what the instance has that the config does not manage ct state list # what is managed +ct state rm campus mainz # un-adopt: drop it from state. Never touches ChurchTools. ct plan # diff the config against ChurchTools (read-only) ct apply # create + update in dependency order (confirm + backup first) ct refresh --group # make ChurchTools re-evaluate one auto-group now ``` +`state rm` is the inverse of `adopt`, and only of `adopt`: it removes the entry +from the state file, makes no HTTP call, and leaves the resource in place in +ChurchTools, now unmanaged. It refuses a key the config still declares — that +would make the next plan propose creating a resource that already exists — so +delete the declaration first, or pass `--force` to do both in one change. + `apply` reconciles **creates and updates** only, saving state after each action (crash-safe / resumable). It **never deletes**: a resource dropped from the config is surfaced as a notice pointing at `destroy`. `destroy --target ` diff --git a/docs/handbuch/dynamic-groups.md b/docs/handbuch/dynamic-groups.md index 63a1a12..45726f8 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/commands/adopt-group.ts -sources_hash: 3fe32a405c9a8b4f +sources_hash: 7388c181938fa4b9 reviewed: 2026-08-10 --- @@ -140,6 +140,25 @@ ct adopt group --with-dynamic # portablized (the d ct adopt group --with-dynamic --no-portable-rulesets # verbatim, this host's ids ``` +> **A re-adopt refreshes the snapshot, not the key (#123).** Re-running this over +> a list of ids is the documented way to refresh rulesets once their scope +> targets become managed — and it is the one mode where `-k` is rejected ("only +> valid when exactly one group is resolved"). Any resource whose adopted key +> differed from the derived key therefore used to be **silently re-keyed** by a +> routine refresh, so the config's declaration matched nothing in state and the +> next plan read as "one to create, one to destroy" for a resource that was fine +> and untouched. An already-managed resource now keeps its key, and says what it +> would have become: +> +> ```text +> ! merkmal_alle_2_5_mz: key would change to "alle_2_bis_5_mz" (derived from the live name). +> Keeping the adopted key. Pass --rekey to change it. +> ``` +> +> Because the ruleset filename follows the key, this also means the refresh +> overwrites `rulesets/merkmal_alle_2_5_mz.json` — the file the config already +> points at — instead of writing a second one beside it. + `ct adopt group --with-dynamic` runs the captured (normalized) ruleset through `portablizeRuleset` (`src/config/query-refs.ts`) before writing `rulesets/.json`: every numeric id sitting in a known ChurchQuery `var` @@ -153,6 +172,7 @@ marker; every other id is left numeric. The `var → RefKind` catalog it keys of | `ctgroup.campusId` | `campus` | `/campuses` | | `person.campusId` | `campus` | `/campuses` | | `ctgroup.groupTypeId` | `group-type` | `/group/grouptypes` | +| `role.id` | `role-def` | managed state, when the role is adopted | | `role.id` | `group-type-role` | `/group/roles` (by `groupTypeId` + name) | The same `group-type-role` rewrite also covers the **out-of-query** integer @@ -172,7 +192,44 @@ is a **groupTypeRoleId**: a role scoped to a group **type**. Role names are marker carries the group-type key + role name, and the resolver picks the one `/group/roles` row whose `groupTypeId` matches this host's group type and whose name slugs to the role. This corrects the earlier `role-def` mapping (#86), -which was unresolvable on the real instance. +which read the role catalog by bare name and was unresolvable on the real +instance. + +##### When the pair collides too — adopt the role (#125) + +"Unique in practice" is not "unique". Two rows **can** share a name on one group +type, and then the pair is a hard error: + +```text +✗ Ambiguous group-type-role(groupType=community, role=leader) referenced at group "4_teamactive": + 2 roles on group type #30 match — "leader" (#87), "leader" (#207). +``` + +Neither obvious remedy works for a config shared across hosts. _Renaming_ +means editing ChurchTools master data to work around a config limitation — and +in the observed case one of the two rows is CT's own stock `leader`. _Passing a +numeric id_ cannot work at all: the roles have **different ids per host** +(`#127` on one, `#207` on the other), and `ConfigContext` deliberately exposes +no env or host, so there is nowhere to branch. + +The fix is the same "adopt the target to portablize it" move that already works +for groups, campuses and departments — **adopt the role**, under the same +logical key on every host: + +```bash +ct adopt group-role 127 --env prod -k community_leader +ct adopt group-role 207 --env dev -k community_leader +``` + +Capture now prefers the managed form, which is unambiguous by construction +because it resolves from state rather than from a name lookup: + +```json +{ "__ctRef": true, "kind": "role-def", "key": "community_leader" } +``` + +The `(groupType, role)` pair remains the fallback for a role the config does not +own, so nothing changes for the common case. #### What could not be portablized is REPORTED, never swallowed (#101) @@ -193,11 +250,36 @@ At **capture** time (`ct adopt … --with-dynamic`) the state file and the `/group/roles` catalog are both in hand, so the reason is a checked fact: ```text -! rulesets/jugend.json keeps 3 host-specific id(s) — NOT portable to another host: +! rulesets/jugend.json keeps 5 host-specific id(s) — NOT portable to another host: ctgroup.id: 1246 left numeric — not under management — `ct adopt group ` for each (then re-adopt) makes them portable ctgroup.groupStatusId: 1, 2 left numeric — group statuses have no REST catalog (#67) — no logical form exists + person.id: 5703, 4389 left numeric — person ids are NEVER portable — ct does not manage people, so this ruleset names DIFFERENT people on another host. Remove the clause or accept the divergence ``` +##### `person.id` is reported, and it is not fixable (#127) + +A ruleset that includes or excludes specific people by id is common — four of +five auto-groups captured in one week did it: + +```json +{ "oneof": [{ "var": "person.id" }, [5703, 4389]] } +{ "!": [{ "oneof": [{ "var": "person.id" }, [12, 1]] }] } +``` + +Those are the source host's person ids written verbatim into the other host's +ruleset, where they name entirely different people — the same failure mode as a +raw `ctgroup.id`. `person.id 1` is the unluckiest case: it exists on every +ChurchTools instance and is almost always an administrator, so an exclusion +aimed at one person on the source host lands on someone real on the target host +rather than harmlessly matching nothing. + +Unlike every other entry in the report, this one is **not fixable in config**, +and the wording says so rather than offering a command. `ct` correctly does not +manage people, so there is no person catalog to resolve against and no +`__ctRef` kind that could express it. The ask is only that the tool say so: +before #127 it was silent, and the absence of a warning actively implied there +was nothing to find. + At **plan** time the same scan runs over every declared dynamic group, but with no state, no catalogs and no network — it can prove that an id sits in an entity position and nothing more. So it says exactly that, rather than asserting a diff --git a/docs/handbuch/permissions.md b/docs/handbuch/permissions.md index cb6fe3b..e0da088 100644 --- a/docs/handbuch/permissions.md +++ b/docs/handbuch/permissions.md @@ -5,7 +5,7 @@ sources: - src/resolve/resolver.ts - src/resolve/refs.ts - src/config/context.ts -sources_hash: 5c881a7bede7e01a +sources_hash: 590e52f65461036a reviewed: 2026-08-13 --- @@ -568,12 +568,14 @@ In bulk mode: ### The single-domain form It fetches `GET /permissions//`, runs the rows through the -**same** normalization the planner uses (`normalizeActual`), and prints a -`ct.groupRole` / `ct.groupTypeRole` block whose every emitted grant is guaranteed -to be accepted by `ct plan` (the round trip is locked by tests): - -- **Excluded, as reconciliation excludes them:** the system baseline - (`meta.modifiedPid === -1`) and inherited rows. +**same** normalization the planner reconciles against (`normalizeEffective`), and +prints a `ct.groupRole` / `ct.groupTypeRole` block whose every emitted grant is +guaranteed to be accepted by `ct plan` (the round trip is locked by tests): + +- **Everything the host grants is emitted** — direct, inherited and + system-baseline rows alike (#114/#119). See [Provenance and + portability](#provenance-and-portability-114119) for why this is broader than + what `ct` owns. - **Revoke/deny rows are preserved, not emitted.** The reconciler never deletes a deny it did not author; if any exist, the block ends with a `NOTE` comment saying so (authoring denies as config is a separate, unshipped feature). @@ -604,12 +606,14 @@ to be accepted by `ct plan` (the round trip is locked by tests): - A scoped right granted **globally** in CT (row with no `dataId`) is a `WARNING` comment either way — the DSL deliberately cannot declare a global grant of a scoped right. -- **System-baseline and inherited rows are dropped, not emitted.** Adoption runs - the live rows through `normalizeActual`, which excludes the self-re-adding - system baseline (`modifiedPid === -1`) and any `isInherited` row. Admin-authored - direct grants — **including** the writable `authId >= 10000` `churchdb:+…` - member rights CT lets you set on `group_type_role` — are emitted as active - grants (no authId cutoff; #65). +- **The `-1` "alle" sentinel is not an id** (#115). A grant scoped to "alle" + comes back as `dataId: -1`, meaning _every_ value of the dimension on whatever + host reads it. It is therefore already portable, and it is emitted with a + one-line comment saying what it is — never with an adoption hint, because + `ct adopt department -1` names a resource that cannot exist. +- **Admin-authored member rights are emitted as active grants** — **including** + the writable `authId >= 10000` `churchdb:+…` member rights CT lets you set on + `group_type_role`. There is no authId cutoff (#65). - **Only `group_role` / `group_type_role` / `status`** are valid; people domains are refused (the same hard boundary as everywhere else). A `status` block is emitted with a numeric `id:` — rename it to the portable @@ -634,11 +638,13 @@ and `ct plan`. - **No authId cutoff.** Admin-authored member rights (`authId >= 10000`, the `churchdb:+…` family) ARE writable on `group_type_role` and can be declared under `ct.groupTypeRole` — verified live (eqrm prod `group_type_role/9` carries - 24 such admin-set rows). What ct never reconciles is decided by the live row's + 24 such admin-set rows). What ct never _writes_ is decided by the live row's flags, not its authId: `normalizeActual` drops the system baseline - (`modifiedPid === -1`) and every `isInherited` row, so those are neither adopted - nor revoked (#65). Earlier versions blocked `authId >= 10000` outright — that was - too broad and is removed. + (`modifiedPid === -1`) and every `isInherited` row, so those are never authored + and never revoked (#65). They are still **emitted by adoption and honoured as + satisfying a declaration** — see [Provenance and + portability](#provenance-and-portability-114119). Earlier versions blocked + `authId >= 10000` outright — that was too broad and is removed. - **Revocation is a later extension, not exposed yet.** `GrantTuple.type` is typed as `"grant" | "revoke"`, but the DSL and `desiredTuples` currently only ever _emit_ `"grant"` tuples — there is no config-level way to declare @@ -650,17 +656,53 @@ and `ct plan`. ## The baseline-tolerance model -`ct plan` and `ct apply` reconcile only the grants **you author** on a -domainId — never the platform's own bookkeeping. `normalizeActual` -(`src/permissions/grants.ts`) filters two kinds of rows out of every actual -fetch before diffing, making both invisible to reconciliation: +`ct plan` and `ct apply` **write** only the grants you author on a domainId — +never the platform's own bookkeeping. Two kinds of row are never authored and +never revoked (`normalizeActual` in `src/permissions/grants.ts`): - **System baseline rows** — any row with `meta.modifiedPid === -1`. These - are ChurchTools' own self-re-adding defaults; they are never proposed for - deletion and never conflict with a desired grant. + are ChurchTools' own self-re-adding defaults. - **Inherited rows** — any row with `isInherited: true`. These come from - hierarchy/role inheritance, not this domainId's own grant table; they are - not owned here either. + hierarchy/role inheritance, not this domainId's own grant table. + +### Provenance and portability (#114/#119) + +Those two rules decide what `ct` **owns**. They do _not_ decide whether a +declared grant is already **satisfied** — that is judged against the +**effective** set: every right the host grants, by any route +(`normalizeEffective`). + +The distinction matters because provenance is not stable across hosts of the +same instance, even when the effective permissions are identical: + +| | one host | the other host | +| ------------------------------------------------------ | ------------------------------- | --------------------------- | +| **#114** — same right, same role | `meta.modifiedPid: -1` (system) | `modifiedPid: 1` (a person) | +| **#119** — 15 group-member rights the TYPE also grants | `isInherited: true` | `isInherited: false` | + +Neither divergence is hand-made: the second host is a copy, and the copy +stamped a person id onto rows that are system rows upstream. Measured across +two hosts on CT 3.135.2: 18 of 63 `group_role` domains carried inherited rows +on one, and **zero anywhere** on the other. + +Deciding satisfaction from ownership made such a config impossible to write. +Omit the right (as adoption used to emit it) and the other host **revokes** it; +declare it and this host plans `+1 grant` forever. So the domain had to be left +out of adoption entirely, even though nothing about it is genuinely +undeclarable. + +Reconciling on the effective set removes the dilemma, because the two hosts +agree on effective permissions even when they disagree on provenance: + +- A declared right the host already grants by any route needs **no PUT**. +- A revoke is still computed from the **owned** set only, so `ct` never revokes + a baseline or inherited row. +- `ct adopt grants` emits the effective set, and names in a footer how many of + the emitted grants are inherited or baseline on this host. + +The "never fight the platform" property is unchanged. The only thing that +changed is that `ct` stopped proposing to re-author what the platform already +grants. Combined with the **managed-guard** (`buildPermissionPlan` only ever surfaces the `domainId`s you've declared — a bulk `GET /permissions/{domainType}` diff --git a/src/commands/adopt-group.ts b/src/commands/adopt-group.ts index f268c75..1f18d72 100644 --- a/src/commands/adopt-group.ts +++ b/src/commands/adopt-group.ts @@ -21,7 +21,7 @@ import { RESOURCES, configSnippet, fromInformation, slug } from "../resources/re import { ReverseResolver, type RoleCatalogEntry } from "../resolve/reverse.js"; import type { RefKind } from "../resolve/refs.js"; import { formatPortablizeWarnings, portablizeRuleset, scanUnportablized } from "../config/query-refs.js"; -import { loadState, saveState, upsert, type State } from "../state/state.js"; +import { chooseAdoptKey, loadState, saveState, upsert, type State } from "../state/state.js"; import { success, info, warn, out } from "../ui.js"; interface AdoptGroupOptions { @@ -32,6 +32,8 @@ interface AdoptGroupOptions { type?: string; childrenOf?: string; withDynamic?: boolean; + /** Opt in to changing an already-managed group's logical key (#123). Never the default. */ + rekey?: boolean; /** Commander's negatable `--no-portable-rulesets`: true unless the flag was passed (#101). */ portableRulesets?: boolean; strictRulesets?: boolean; @@ -174,6 +176,10 @@ export function adoptGroupCommand(): Command { .option("-k, --key ", "logical key (only valid when exactly one group is resolved)") .option("-s, --state ", "state file path (or set CT_STATE)") .option("-e, --env ", "environment profile from ct.envs.json (host + state + token)") + .option( + "--rekey", + "let a re-adoption change an already-managed group's logical key to the derived one (#123)", + ) .option("--dry-run", "preview the config entries and state changes without writing") .option("--type ", "adopt every group of this group type (numeric id or logical key)") .option( @@ -281,11 +287,25 @@ export function adoptGroupCommand(): Command { for (const id of resolvedIds) { const resource = await client.get>(GROUP_SPEC.itemPath(id)); - const key = - (resolvedIds.length === 1 ? opts.key?.trim() : undefined) || GROUP_SPEC.deriveKey(resource); + // An already-managed group keeps its adopted key unless --rekey says otherwise (#123). This + // is the mode that made the bug bite: the documented ruleset-refresh workflow passes a LIST + // of ids, which is exactly when `-k` is rejected, so there was no way to prevent the re-key. + // Because `relPath` below is built from `key`, preserving it also makes the refresh overwrite + // the ruleset file the config already points at instead of writing a second one. + const choice = chooseAdoptKey(state, "group", id, GROUP_SPEC.deriveKey(resource), { + explicitKey: resolvedIds.length === 1 ? opts.key : undefined, + rekey: opts.rekey, + }); + const key = choice.key; if (!key) { throw new Error(`Could not derive a logical key for group #${id} — pass --key explicitly.`); } + if (choice.wouldBecome) { + warn( + `${key}: key would change to "${choice.wouldBecome}" (derived from the live name). ` + + `Keeping the adopted key. Pass --rekey to change it.`, + ); + } const fields = GROUP_SPEC.managedFields(resource); // Reverse-resolve the group's numeric ids to logical sugar for the emitted snippet; the @@ -301,11 +321,18 @@ export function adoptGroupCommand(): Command { // Managed group ids come from state (no catalog for `group`), including any group this // same run already adopted; the master-data kinds come from the catalog maps above. const groupMap = new Map(); + // Managed role definitions, likewise from STATE rather than the live catalog (#125): + // state gives a per-host id under a shared logical key, which is what makes a + // duplicate role name fixable by adopting the role instead of renaming master data. + // (`ReverseResolver.idToKeyByKind("role-def")` keys by slug(name) off the catalog and + // is exactly the ambiguous mapping this replaces.) + const roleDefMap = new Map(); for (const r of Object.values(state.resources)) { if (r.type === "group") groupMap.set(r.id, r.key); + if (r.type === "group-role") roleDefMap.set(r.id, r.key); } const { ruleset, warnings } = portablizeRuleset(captured.normalizedRuleset, { - idToKeyByKind: { ...portableCatalogMaps, group: groupMap }, + idToKeyByKind: { ...portableCatalogMaps, group: groupMap, "role-def": roleDefMap }, roleCatalog, groupTypeIdToKey, }); diff --git a/src/commands/adopt.ts b/src/commands/adopt.ts index c861136..82b89a0 100644 --- a/src/commands/adopt.ts +++ b/src/commands/adopt.ts @@ -4,7 +4,7 @@ import { resolveConfig } from "../config.js"; import { prepareEnv } from "../env/context.js"; import { resourceType, configSnippet } from "../resources/registry.js"; import { ReverseResolver } from "../resolve/reverse.js"; -import { loadState, saveState, upsert } from "../state/state.js"; +import { chooseAdoptKey, loadState, saveState, upsert } from "../state/state.js"; import { success, info, warn, out } from "../ui.js"; import { adoptGrantsCommand } from "./adopt-grants.js"; import { adoptGroupCommand } from "./adopt-group.js"; @@ -13,6 +13,8 @@ interface AdoptOptions { key?: string; state?: string; env?: string; + /** Opt in to changing an already-managed resource's logical key (#123). Never the default. */ + rekey?: boolean; dryRun?: boolean; } @@ -24,6 +26,10 @@ export function adoptCommand(): Command { .option("-k, --key ", "logical key (defaults to a slug of the resource name)") .option("-s, --state ", "state file path (or set CT_STATE)") .option("-e, --env ", "environment profile from ct.envs.json (host + state + token)") + .option( + "--rekey", + "let a re-adoption change an already-managed resource's logical key to the derived one (#123)", + ) .option("--dry-run", "preview the config entry and state change without writing") .action(async (type: string, rawId: string, opts: AdoptOptions) => { const spec = resourceType(type); @@ -50,10 +56,22 @@ export function adoptCommand(): Command { throw new Error(`No ${type} with id ${id} exists in ChurchTools.`); } - const key = opts.key?.trim() || spec.deriveKey(resource); + const derived = spec.deriveKey(resource); + // An already-managed resource keeps its adopted key unless --rekey says otherwise (#123). + const choice = chooseAdoptKey(state, type, id, derived, { + explicitKey: opts.key, + rekey: opts.rekey, + }); + const key = choice.key; if (!key) { throw new Error("Could not derive a logical key — pass --key explicitly."); } + if (choice.wouldBecome) { + warn( + `${key}: key would change to "${choice.wouldBecome}" (derived from the live name). ` + + `Keeping the adopted key. Pass --rekey to change it.`, + ); + } const fields = spec.managedFields(resource); // Reverse-resolve numeric ids (campusId/groupTypeId/groupStatusId) to logical sugar so the // emitted snippet is portable and human-readable; unresolved ids stay numeric + a TODO (#52). diff --git a/src/commands/state.ts b/src/commands/state.ts index 1365e06..8448587 100644 --- a/src/commands/state.ts +++ b/src/commands/state.ts @@ -1,14 +1,22 @@ import { Command } from "commander"; import { resolveConfig } from "../config.js"; +import { loadConfig, resolveConfigPath } from "../config/load.js"; import { prepareEnv } from "../env/context.js"; -import { loadState } from "../state/state.js"; -import { info, out } from "../ui.js"; +import { loadState, saveState } from "../state/state.js"; +import { resourceType } from "../resources/registry.js"; +import { info, out, success, warn } from "../ui.js"; interface StateOptions { state?: string; env?: string; } +interface StateRmOptions extends StateOptions { + config?: string; + force?: boolean; + dryRun?: boolean; +} + export function stateCommand(): Command { const cmd = new Command("state").description("Inspect the managed-resource state file"); @@ -26,5 +34,95 @@ export function stateCommand(): Command { out(resources); }); + // `ct state rm` — the missing inverse of `ct adopt` (#122). + // + // `ct adopt` writes an entry; nothing removed one. `ct destroy` is the opposite of what un-adopting + // means (it deletes the resource IN ChurchTools), so backing out an adoption meant hand-editing + // `ct-state..json` with a text editor or a `node -e` one-liner — the very file the tool + // insists is its own. Adopt-then-declare is the documented loop, so an adoption that turns out to + // be wrong is normal rather than exotic; leaving the entry in makes `plan` report a DESTROY for a + // resource where nothing is wrong, and the offline config-matches-state check cannot be satisfied + // without editing the file the check is checking. + cmd + .command("rm") + .description("Un-adopt: remove a resource from the state file. Never touches ChurchTools.") + .argument("", "resource type, e.g. campus | group | group-role") + .argument("", "logical key of the entry to remove") + .option("-s, --state ", "state file path (or set CT_STATE)") + .option("-e, --env ", "environment profile from ct.envs.json (host + state + token)") + .option("-c, --config ", "config file to check the key against (or set CT_CONFIG)") + .option("--force", "remove even though the key is still declared in the config") + .option("--dry-run", "report what would be removed without writing") + .action(async (type: string, key: string, opts: StateRmOptions) => { + // Validate the type against the registry first, so a typo is a clear error rather than a + // confusing "not in state" for an entry that could never have existed. + resourceType(type); + + const cmdEnv = await prepareEnv(opts); + const statePath = cmdEnv.statePath; + const state = await loadState(statePath, (await resolveConfig()).host); + + const entry = state.resources[key]; + if (!entry) { + throw new Error(`No entry "${key}" in ${statePath}. List them with \`ct state list\`.`); + } + if (entry.type !== type) { + throw new Error( + `"${key}" in ${statePath} is a ${entry.type} (#${entry.id}), not a ${type}. ` + + `Pass the right type, or list them with \`ct state list\`.`, + ); + } + + // Removing an entry the config still declares turns the next plan into a CREATE for a resource + // that already exists on the host — which then 400s on a duplicate name, or worse, succeeds and + // leaves a second copy. So it is refused by default; --force is there for the case where the + // declaration is being deleted in the same change. + if (!opts.force) { + const declared = await declaredKeys(opts.config); + if (declared?.has(key)) { + throw new Error( + `"${key}" is still declared in the config, so removing it from state would make the next ` + + `plan propose CREATING a resource that already exists on this host. Remove the ` + + `declaration first, or pass --force if you are deleting both in the same change.`, + ); + } + } + + if (opts.dryRun) { + info(`Would remove ${entry.type}.${key} (#${entry.id}) from ${statePath}.`); + return; + } + + delete state.resources[key]; + await saveState(statePath, state); + success(`Removed ${entry.type}.${key} (#${entry.id}) from ${statePath}.`); + // The single most important thing to say: nothing happened in ChurchTools. Someone reaching for + // this command has usually just been told that `ct destroy` is the wrong tool. + info( + `ChurchTools was not contacted — #${entry.id} still exists there, now unmanaged. ` + + `Re-adopt it with \`ct adopt ${type} ${entry.id}\`.`, + ); + }); + return cmd; } + +/** + * The logical keys the config declares, or `undefined` when the config cannot be read. + * + * A missing/broken config must not block an un-adopt: backing out an adoption is exactly what you do + * when the config is mid-edit. So a load failure downgrades the guard to a warning rather than + * failing the command. + */ +async function declaredKeys(configOpt: string | undefined): Promise | undefined> { + try { + const { resources } = await loadConfig(resolveConfigPath(configOpt)); + return new Set(resources.map((r) => r.key)); + } catch (err) { + warn( + `Could not read the config to check whether "${configOpt ?? "the default config"}" still ` + + `declares this key (${err instanceof Error ? err.message : String(err)}) — removing anyway.`, + ); + return undefined; + } +} diff --git a/src/config/query-refs.ts b/src/config/query-refs.ts index 28c9177..74b80d0 100644 --- a/src/config/query-refs.ts +++ b/src/config/query-refs.ts @@ -29,11 +29,17 @@ export type { RoleCatalogEntry }; * `role.id` is DELIBERATELY NOT here (fixed in #76, reverting #86's `role-def` mapping). A ruleset's * `role.id` is a **groupTypeRoleId** — a role scoped to a group TYPE — not a global role-catalog id. * Role NAMES are not globally unique across group types (live prod, 2026-07-11: 3 roles named "Leiter", - * 6 "Organisator", 6 "Mitglied", each on a different group type), so mapping it to `role-def` (which - * keys `/group/roles` by `slug(name)` alone) makes the resolver throw "ambiguous". Only the - * (groupTypeId, name) PAIR is unique (0 collisions across all 46 prod roles), so `role.id` needs the - * catalog-driven special case in {@link portablizeRuleset} that emits a `group-type-role` marker — a - * lone name-based table entry cannot express it. + * 6 "Organisator", 6 "Mitglied", each on a different group type), so a NAME-KEYED `role-def` mapping + * makes the resolver throw "ambiguous". So `role.id` needs the special case in + * {@link portablizeRuleset}, which a lone name-based table entry cannot express. That special case + * now emits, in order of preference: + * + * 1. `{ kind: "role-def", key }` when the role is under MANAGEMENT (#125) — a per-host id under a + * shared logical key, resolved from state, so it is unambiguous by construction. This is what + * #86 got wrong: the mapping is right, the name-keyed catalog it used to source it from was not. + * 2. `{ kind: "group-type-role", groupType, role }` otherwise. The (groupTypeId, name) PAIR is + * unique in the common case (0 collisions across all 46 prod roles) — but NOT always, and when + * it collides there is no fix available in a shared config, which is precisely why (1) exists. * * Deliberately absent (the escape hatch — unknown vars are left untouched by {@link portablizeRuleset}): * - `ctgroup.groupStatusId` — group statuses have NO REST catalog (#67; `/group/memberstatus` is a @@ -107,6 +113,23 @@ export interface PortablizeWarning { */ const UNPORTABLE_ENTITY_VARS: Readonly> = { "ctgroup.groupStatusId": "group statuses have no REST catalog (#67) — no logical form exists", + // #127. A ruleset that includes or excludes specific people by id is common — four of five + // auto-group rulesets captured in one week did it — and it was the ONE entity var the audit never + // mentioned, so the only way to find it was to read the captured JSON by hand. The absence of a + // warning actively implied there was nothing to find. + // + // Unlike every other entry here, this one is not "pending a catalog": ct correctly does not manage + // people, so there is no person catalog to resolve against and no `__ctRef` kind that could express + // it. The ask is only that the tool SAY so, and the wording has to make clear it is unfixable + // rather than not-yet-fixed — hence a remedy that is a decision ("remove the clause or accept the + // divergence"), not a command to run. + // + // `person.id` 1 is the unluckiest case: it exists on every ChurchTools instance and is almost + // always an administrator, so an exclusion clause aimed at one person on the source host lands on + // someone real on the target host rather than harmlessly matching nothing. + "person.id": + "person ids are NEVER portable — ct does not manage people, so this ruleset names DIFFERENT " + + "people on another host. Remove the clause or accept the divergence", }; /** @@ -210,6 +233,24 @@ export function portablizeRuleset( // group type is unmanaged — the escape hatch, identical in spirit to mapScalar's. const mapRoleScalar = (value: unknown, varName: string): unknown => { if (typeof value !== "number") return value; + // A MANAGED role-def wins over the (group-type, role-name) pair (#125). + // + // The name pair is resolved by filtering `/group/roles` on (groupTypeId, slug(name)), so two rows + // sharing a name on one group type are a hard error — and neither remedy the error suggests works + // for a config shared across hosts. "Rename to disambiguate" means editing ChurchTools master + // data to work around a config limitation, and in the observed case one of the two rows is CT's + // own stock `leader`. "Pass a numeric id" cannot work at all: the roles have different ids per + // host (#127 on one, #207 on the other) and `ConfigContext` deliberately exposes no env or host, + // so there is nowhere to branch. + // + // A `role-def` ref carries a per-host id in managed state — the same mechanism `group` and + // `campus` refs already use to mean different numbers on different hosts. Emitting it makes the + // ambiguous case fixable IN CONFIG, by adopting the role under a shared key on both hosts, which + // is the same "adopt the target to portablize it" move that works everywhere else. + const roleDefKey = idToKeyByKind["role-def"]?.get(value); + if (roleDefKey !== undefined) { + return marker("role-def", roleDefKey); + } const entry = roleCatalog?.get(value); const groupTypeKey = entry ? groupTypeIdToKey?.get(entry.groupTypeId) : undefined; if (entry && groupTypeKey !== undefined) { diff --git a/src/permissions/adopt.ts b/src/permissions/adopt.ts index 66f46c5..8fa8079 100644 --- a/src/permissions/adopt.ts +++ b/src/permissions/adopt.ts @@ -4,12 +4,22 @@ * instance's rights structure comes under management without hand-transcription * (issue #25). * - * The live rows are run through the SAME normalization the planner uses - * (`normalizeActual`) so what is emitted is exactly what a subsequent `ct plan` - * would consider managed: the system baseline (`modifiedPid === -1`) and - * inherited rows are dropped, and pre-existing revoke/deny rows are surfaced as - * a note rather than emitted (the reconciler preserves them; re-authoring them - * is out of scope here). + * The live rows are run through the SAME normalization the planner reconciles + * against — `normalizeEffective` (#114/#119) — so what is emitted is exactly + * what a subsequent `ct plan` would consider SATISFIED: every right the host + * grants, whether directly, by inheritance, or as system baseline. + * + * That is deliberately broader than what ct OWNS. Emitting only the owned rows + * encoded one host's provenance into a config meant to drive several, and the + * same right can be inherited on one host and direct on another — so a right + * dropped here would be undeclared there, land in `toDelete`, and be revoked. + * Declaring it keeps the block a clean no-op on both, and ct still never + * authors or revokes a row it does not own (that is `normalizeActual`'s job, + * not adoption's). A footer names how many of the emitted grants are in that + * category. + * + * Pre-existing revoke/deny rows are surfaced as a note rather than emitted (the + * reconciler preserves them; re-authoring them is out of scope here). * * Grants are NOT state-tracked resources, so adoption prints config only — it * never writes the state file. The caller makes that explicit in its output. @@ -17,8 +27,15 @@ import type { State } from "../state/state.js"; import { findByTypeId } from "../state/state.js"; import { CATALOG } from "./catalog.js"; -import { normalizeActual, type DomainType, type GrantTuple, type RawPermission } from "./grants.js"; -import { GROUP_SCOPE_FIELD, SCOPE_REF_KIND } from "./scope.js"; +import { + normalizeActual, + normalizeEffective, + unownedGrantCount, + type DomainType, + type GrantTuple, + type RawPermission, +} from "./grants.js"; +import { ALL_SCOPE_SENTINEL, GROUP_SCOPE_FIELD, SCOPE_REF_KIND } from "./scope.js"; /** DSL function name for each domain type — the call the emitted block should be pasted as. */ const DSL_FN: Record = { @@ -119,6 +136,12 @@ export interface AdoptedGrantsBlock { omitted: number; /** Pre-existing deny rows on this domain; preserved by the reconciler, never emitted. */ revokes: number; + /** + * How many emitted grants are INHERITED or system-baseline on this host (#114/#119) — declared so + * the block is a no-op on a host that materializes them as direct rows, but never authored or + * revoked by ct on this one. + */ + unowned: number; } export function emitAdoptedGrants(args: AdoptGrantsArgs): string { @@ -127,9 +150,23 @@ export function emitAdoptedGrants(args: AdoptGrantsArgs): string { export function buildAdoptedGrants(args: AdoptGrantsArgs): AdoptedGrantsBlock { const { domainType, domainId, rows, state, domain, key } = args; - const normalized = normalizeActual(rows); + // Emit the EFFECTIVE set — direct, inherited and system-baseline rows alike (#114/#119). + // + // Adoption used to emit only the rows ct owns, which encoded THIS host's provenance into a config + // meant to drive several. On a host that materializes the same rights differently — as direct rows + // rather than inherited ones — every dropped right is undeclared, so it lands in `toDelete` and + // apply strips it. Measured on two hosts of one instance: adopting one and planning the other + // proposed removing 15 group-member rights from the largest role instance in the estate. + // + // Emitting the effective set makes the block a no-op on BOTH: the reconciler now treats a right the + // host grants by any route as satisfied (see `normalizeEffective`), so a declared-but-inherited + // right causes no PUT here and no revoke there. ct still never authors or revokes a row it does not + // own — that is decided by `normalizeActual`, not by what adoption prints. + const normalized = normalizeEffective(rows); const grants = normalized.filter((t) => t.type === "grant"); - const revokes = normalized.filter((t) => t.type !== "grant"); + // Deny rows stay owned-only: an inherited deny is not this domain's to restate. + const revokes = normalizeActual(rows).filter((t) => t.type !== "grant"); + const unowned = unownedGrantCount(rows); const rev = reverseCatalog(); const body: string[] = []; @@ -151,7 +188,7 @@ export function buildAdoptedGrants(args: AdoptGrantsArgs): AdoptedGrantsBlock { } if (grants.length === 0) { - body.push(" grants: [], // no user-authored grants on this domain (baseline/inherited rows excluded)"); + body.push(" grants: [], // this domain grants nothing"); } else { body.push(" grants: ["); for (const g of collapse(grants)) { @@ -182,6 +219,21 @@ export function buildAdoptedGrants(args: AdoptGrantsArgs): AdoptedGrantsBlock { } lines.push(...body); + if (unowned > 0) { + // #114/#119 asked for exactly this: the asymmetry named UP FRONT, not discovered later as an + // unexplained revoke in the other host's plan after the config has already been written. + lines.push( + `// NOTE: ${unowned} of the grant(s) above are INHERITED or system-baseline on this host — ct never`, + ); + lines.push( + "// authors or revokes those rows. They are emitted anyway because another host of the same instance", + ); + lines.push( + "// may materialize the same rights as direct rows; declaring them is what keeps this block a clean", + ); + lines.push("// no-op on BOTH hosts instead of a 15-right revoke on one of them (#114/#119)."); + } + if (revokes.length > 0) { lines.push( `// NOTE: ${revokes.length} revoke/deny row(s) exist on this domain. The reconciler PRESERVES them (it never`, @@ -192,7 +244,7 @@ export function buildAdoptedGrants(args: AdoptGrantsArgs): AdoptedGrantsBlock { lines.push("// config is not supported yet (see issue #25 stretch goal)."); } - return { block: lines.join("\n"), omitted, revokes: revokes.length }; + return { block: lines.join("\n"), omitted, revokes: revokes.length, unowned }; } /** The lines emitted for one collapsed grant, plus whether a LIVE grant was left as a comment @@ -257,7 +309,18 @@ function grantLines(g: CollapsedGrant, rev: Map, state: St // emitter is deliberately pure — no client, no fetch — so it cannot turn the id into a name. // Emit the number and point at the portable form the author can write by hand. const sugar = dimension?.managed ? SCOPE_SUGAR_FIELD[dimension.type] : undefined; - if (dimension && !dimension.managed) { + // #115: `-1` is not an id at all — it is ChurchTools' "alle" sentinel, meaning every value of + // the dimension on whatever host reads it. So it is ALREADY portable, and none of the + // host-specific-id advice below applies to it. Emitting `ct adopt department -1` sent people + // looking for a resource that cannot exist, and in a real adoption run this fires on most of + // the broadly-scoped grants — the interesting ones. + const realIds = g.dataIds.filter((id) => id !== ALL_SCOPE_SENTINEL); + const hasSentinel = g.dataIds.length > realIds.length; + if (hasSentinel) { + const what = dimension ? `every ${dimension.type}` : `every value of "${entry.scopeField}"`; + out.push(` // scope -1 is ChurchTools' "alle" sentinel — ${what}; host-independent.`); + } + if (dimension && !dimension.managed && realIds.length > 0) { const field = SCOPE_SUGAR_FIELD[dimension.type] ?? dimension.type; out.push( ` // NOTE: "${entry.name}" scopes by "${entry.scopeField}" (${dimension.type}), a catalog ct reads but does not manage —`, @@ -269,6 +332,11 @@ function grantLines(g: CollapsedGrant, rev: Map, state: St const entries: string[] = []; const unmanaged: number[] = []; for (const id of [...g.dataIds].sort((a, b) => a - b)) { + if (id === ALL_SCOPE_SENTINEL) { + // Portable as-is (see above) — emitted, but never counted as an unmanaged host-specific id. + entries.push(String(id)); + continue; + } const managed = dimension && sugar ? findByTypeId(state, dimension.type, id) : undefined; if (managed && sugar) { entries.push(`{ ${sugar}: ${JSON.stringify(managed.key)} }`); @@ -313,7 +381,15 @@ function grantLines(g: CollapsedGrant, rev: Map, state: St // bring it under management (rather than silently declaring an opaque numeric scope for it). const resolvedKeys: string[] = []; const unmanaged: number[] = []; + // #115: `-1` is the "alle" sentinel — every group on whatever host reads it — not an id that + // could ever be adopted. It is already portable, so it is emitted verbatim through the numeric + // escape hatch and never counted as an unmanaged target. + let sentinel = false; for (const id of g.dataIds) { + if (id === ALL_SCOPE_SENTINEL) { + sentinel = true; + continue; + } const group = findByTypeId(state, "group", id); if (group) resolvedKeys.push(group.key); else unmanaged.push(id); @@ -341,8 +417,14 @@ function grantLines(g: CollapsedGrant, rev: Map, state: St out.push(` // then add its logical key to the scope array below.`); omitted = true; } - if (resolvedKeys.length > 0) { - const scope = resolvedKeys.map((k) => JSON.stringify(k)).join(", "); + if (sentinel) { + out.push(` // scope -1 is ChurchTools' "alle" sentinel — every group; host-independent.`); + } + if (resolvedKeys.length > 0 || sentinel) { + const scope = [ + ...(sentinel ? [String(ALL_SCOPE_SENTINEL)] : []), + ...resolvedKeys.map((k) => JSON.stringify(k)), + ].join(", "); out.push(` { right: ${JSON.stringify(entry.name)}, scope: [${scope}] },`); } else if (unmanaged.length > 0) { // Every scope target is unmanaged: there is no valid key to emit, so the grant itself is a diff --git a/src/permissions/grants.ts b/src/permissions/grants.ts index 005ecec..a2f676c 100644 --- a/src/permissions/grants.ts +++ b/src/permissions/grants.ts @@ -75,16 +75,72 @@ export function tupleKey(t: { return `${t.type}:${t.authId}:${scope}`; } +/** One raw row → its tuple. dataId is `[]` or a single element (CT reads scoped grants back one row + * per dataId), so there is nothing to sort — `tupleKey` sorts defensively anyway. */ +function toTuple(r: RawPermission): GrantTuple { + return { authId: r.authId, dataId: r.dataId == null ? [] : [r.dataId], type: r.type }; +} + +/** Is this row one ct may WRITE or REVOKE — i.e. a direct, user-authored row? */ +function isOwnable(r: RawPermission): boolean { + if (r.meta?.modifiedPid === -1) return false; // system baseline — ct never authors or revokes it + if (r.isInherited) return false; // granted via the type — ct never authors or revokes it + return true; +} + +/** + * The rows reconciliation OWNS: direct, user-authored grants. These are the only rows `ct` will ever + * write or revoke, and the only candidates for `toDelete`. + */ export function normalizeActual(rows: RawPermission[]): GrantTuple[] { - const out: GrantTuple[] = []; + return rows.filter(isOwnable).map(toTuple); +} + +/** + * The rows that describe what the host EFFECTIVELY grants — direct, inherited and system-baseline + * alike (#114/#119). + * + * `ct` used to decide "is this declared grant already satisfied?" from the owned set alone, which + * meant it decided from **how a row was recorded**. That differs per host for identical effective + * permissions, and it is what made a config unable to be a clean no-op on two hosts of one instance: + * + * - #114 — the same right is system-authored (`meta.modifiedPid === -1`) on one host and + * user-authored on the other, because the copy that produced the second host stamped a person id + * onto rows that are system rows upstream. Declaring the right planned `+1 grant` on the first + * host; omitting it planned a **revoke** on the second. + * - #119 — the same right is `isInherited: true` on one host and `false` on the other. Measured on + * two hosts of one instance, both CT 3.135.2: 18 of 63 `group_role` domains carried inherited + * rows on one, and **zero** anywhere on the other. Adoption dropped the inherited rows, so + * planning the result elsewhere proposed stripping 15 rights from the largest role instance in + * the estate. + * + * Reconciling against the effective set fixes both, because the two hosts agree on effective + * permissions even when they disagree on provenance. The "never fight the platform" property still + * holds: {@link normalizeActual} still decides what may be written or revoked, so ct never authors + * and never revokes a baseline or inherited row — it only stops proposing to re-author something the + * platform already grants. + */ +export function normalizeEffective(rows: RawPermission[]): GrantTuple[] { + // DEDUPED by identity: a right can appear both as a direct row and as an inherited one on the same + // domain, and "effective" is a set, not a tally. Without this, `adopt grants` would print the same + // grant twice. The OWNED row wins when both exist, so anything downstream that reads a tuple's + // provenance-adjacent fields sees the writable one. + const byKey = new Map(); for (const r of rows) { - if (r.meta?.modifiedPid === -1) continue; // system baseline — invisible to reconciliation - if (r.isInherited) continue; // inherited — not directly owned here - // dataId is [] or a single element (CT reads scoped grants back one row per dataId), so there is - // nothing to sort here — and tupleKey sorts defensively anyway when it builds the identity key. - out.push({ authId: r.authId, dataId: r.dataId == null ? [] : [r.dataId], type: r.type }); + const t = toTuple(r); + const k = tupleKey(t); + if (!byKey.has(k) || isOwnable(r)) byKey.set(k, t); } - return out; + return [...byKey.values()]; +} + +/** How many of a domain's effective grants ct does NOT own — inherited or system-baseline rows. */ +export function unownedGrantCount(rows: RawPermission[]): number { + const owned = new Set(rows.filter(isOwnable).map((r) => tupleKey(toTuple(r)))); + const unowned = new Set( + rows.filter((r) => !isOwnable(r) && r.type === "grant").map((r) => tupleKey(toTuple(r))), + ); + return [...unowned].filter((k) => !owned.has(k)).length; } export interface GrantDiff { @@ -111,6 +167,13 @@ export function diffGrants( desired: GrantTuple[], actual: GrantTuple[], preserveUnknown?: PreservePredicate, + /** + * What the host effectively grants, including rows ct does not own (inherited, system baseline) — + * see {@link normalizeEffective}. Used ONLY to decide whether a declared grant is already + * satisfied, never to decide what to revoke. Defaults to `actual`, which is the pre-#114/#119 + * behaviour and keeps every existing caller and test honest. + */ + effective: GrantTuple[] = actual, ): GrantDiff { // Reconciliation owns only user-authored GRANT rows. `desiredTuples` only ever emits // `type: "grant"`, so an explicit deny row (`type: "revoke"`) has no desired counterpart and @@ -120,7 +183,11 @@ export function diffGrants( const preserved = actual.filter((t) => t.type !== "grant"); const desiredKeys = new Map(desired.map((t) => [tupleKey(t), t])); const actualKeys = new Map(managedActual.map((t) => [tupleKey(t), t])); - const toPut = [...desiredKeys].filter(([k]) => !actualKeys.has(k)).map(([, t]) => t); + // SATISFACTION is judged against the EFFECTIVE set (#114/#119): a right the host already grants by + // ANY route — directly, by inheritance, or as system baseline — needs no PUT. REVOCATION below is + // still judged against the owned set only, so ct never revokes a row it did not author. + const effectiveKeys = new Set(effective.filter((t) => t.type === "grant").map(tupleKey)); + const toPut = [...desiredKeys].filter(([k]) => !effectiveKeys.has(k)).map(([, t]) => t); const undeclared = [...actualKeys].filter(([k]) => !desiredKeys.has(k)).map(([, t]) => t); // Opt-in partial ownership (#102). Without it, ONE live grant on a dimension this tool has no // business managing (a wiki category, an HTML template) makes the whole role instance undeclarable, diff --git a/src/permissions/plan.ts b/src/permissions/plan.ts index ff687b6..957a013 100644 --- a/src/permissions/plan.ts +++ b/src/permissions/plan.ts @@ -19,6 +19,7 @@ import { compareVersions } from "../api/version.js"; import { resolveScope, resolveScopeRefs, type ScopeRefMap } from "./scope.js"; import { normalizeActual, + normalizeEffective, diffGrants, type GrantTuple, type GrantDiff, @@ -287,7 +288,12 @@ export async function buildPermissionPlan( } const all = byType.get(p.domainType); if (all == null) continue; // fetch failed for this domainType — recorded above - const normalizedAll = normalizeActual(all.filter((r) => r.domainId === p.domainId)); + const domainRows = all.filter((r) => r.domainId === p.domainId); + const normalizedAll = normalizeActual(domainRows); + // Everything the host grants on this domain by ANY route, provenance included (#114/#119). Only + // ever consulted to answer "is this declared grant already satisfied?" — never to decide a + // revoke, which stays the owned set below. + const effectiveAll = normalizeEffective(domainRows); // Actuals are already filtered by `normalizeActual`: the self-re-adding system baseline // (`modifiedPid === -1`) and every `isInherited` row are dropped, so what remains is admin-authored // DIRECT grants — INCLUDING the writable `authId >= 10000` group-member rights Equippers curates on @@ -324,6 +330,9 @@ export async function buildPermissionPlan( desiredTuples(p, state, declaredGroupKeys, scopeRefs), knownActual, preservePredicateFor(p.preserveUnknown), + // The unknown-authId guard (#25) deliberately does NOT apply here: an unnameable right can + // still satisfy a declaration, and excluding it would only re-propose a PUT we cannot name. + effectiveAll, ), }); } diff --git a/src/resolve/resolver.ts b/src/resolve/resolver.ts index 232080e..61cc399 100644 --- a/src/resolve/resolver.ts +++ b/src/resolve/resolver.ts @@ -325,9 +325,17 @@ export class Resolver { } if (matches.length > 1) { const list = matches.map((c) => `${JSON.stringify(c.name)} (#${c.id})`).join(", "); + // #125: the old message offered "rename to disambiguate, or pass a numeric id", and BOTH are + // dead ends for a config shared across hosts — a rename edits ChurchTools master data (often + // CT's own stock role) to work around a config limitation, and the roles have different ids per + // host with no env to branch on. Adopting the role is the remedy that actually works, and it is + // the same "adopt the target to portablize it" move groups and campuses already use. + const adopts = matches.map((c) => `\`ct adopt group-role ${c.id} -k \``).join(" / "); throw new Error( `Ambiguous ${refLabel(r)} referenced at ${site} on ${this.host}: ${matches.length} roles on group ` + - `type #${groupTypeId} match — ${list}. Rename to disambiguate, or pass a numeric id.`, + `type #${groupTypeId} match — ${list}. Adopt the one you mean under a logical key, on EVERY ` + + `host, and reference it as { kind: "role-def", key: "" } — ${adopts}. ` + + `(Renaming in ChurchTools and passing a numeric id both work on one host only.)`, ); } return matches[0]!.id; diff --git a/src/resources/registry.ts b/src/resources/registry.ts index 108b1d6..e0518b5 100644 --- a/src/resources/registry.ts +++ b/src/resources/registry.ts @@ -309,7 +309,16 @@ export const RESOURCES: Record = { tier: 0, // CT's campus short name is `shorty` (1–10 chars, required on create) — verified // live. `shortName` is a vestigial, usually-null sibling; do not use it for writes. - deriveKey: (r) => slug(str(r, "shorty") || str(r, "name")), + // + // The KEY comes from `name`, like every other resource in this registry (#118). It used to + // prefer `shorty`, which made campus the sole exception and produced keys nobody could read: + // Idstein → `swa` (abbreviated after the region it used to be named for), Würzburg → `wu`, + // Dietzenbach → `of` — and `{ campus: "of" }` inside a grant scope reads as a typo in the tool + // rather than as a name. `shorty` is also less stable: it is free text an admin edits in the UI + // to make a column fit, and every such edit silently changes what the derived key WOULD be, + // splitting new adoptions from the ones already in state. It is still written as a managed field + // exactly as before — this is only about the key. + deriveKey: (r) => slug(str(r, "name") || str(r, "shorty")), managedFields: (r) => ({ name: r.name, shorty: r.shorty }), }), group: define({ diff --git a/src/state/state.ts b/src/state/state.ts index 10a2fe1..105c950 100644 --- a/src/state/state.ts +++ b/src/state/state.ts @@ -154,6 +154,45 @@ export function isManaged(state: State, type: string, id: number): boolean { return findByTypeId(state, type, id) !== undefined; } +/** + * The key a re-adoption should actually use, and whether it differs from the derived one (#123). + * + * `ct adopt group --with-dynamic` is the documented way to refresh a dynamic group's ruleset + * once its scope targets become managed, and it runs over a LIST of ids — the one mode where `-k` is + * rejected ("only valid when exactly one group is resolved"). Any resource whose adopted key differed + * from the derived key was therefore silently re-keyed by that refresh: the config's declaration + * matched nothing in state, a state entry existed that nothing declared, and the next plan read as + * "one to create, one to destroy" for a resource that was fine and untouched on the host. The message + * said only that the snapshot was "refreshed", which reads as *nothing else changed*. + * + * An adopted key was chosen deliberately — very likely with `-k`, to match the config's naming — so a + * refresh touches the snapshot, not the identity. + */ +export interface AdoptKeyChoice { + /** The key to store under. */ + key: string; + /** Set when the derived key differs from the kept one — the caller warns with this. */ + wouldBecome?: string; +} + +/** + * Choose the key for an adoption. A resource already in state keeps its key unless `rekey` is set; + * an explicit `--key` is an explicit intent and always wins. New resources just take `derived`. + */ +export function chooseAdoptKey( + state: State, + type: string, + id: number, + derived: string, + opts: { explicitKey?: string; rekey?: boolean } = {}, +): AdoptKeyChoice { + const explicit = opts.explicitKey?.trim(); + if (explicit) return { key: explicit }; + const existing = findByTypeId(state, type, id); + if (!existing || opts.rekey || existing.key === derived) return { key: derived }; + return { key: existing.key, wouldBecome: derived }; +} + export interface UpsertInput { type: string; id: number; diff --git a/tests/adopt-identity.test.ts b/tests/adopt-identity.test.ts new file mode 100644 index 0000000..2694f86 --- /dev/null +++ b/tests/adopt-identity.test.ts @@ -0,0 +1,84 @@ +/** + * Adoption identity: a re-adopt refreshes the SNAPSHOT, not the KEY (#123), and there is a supported + * way back out of an adoption (#122). + * + * `ct adopt group --with-dynamic` is the documented ruleset-refresh workflow and takes a LIST + * of ids — the one mode where `-k` is rejected — so any resource whose adopted key differed from the + * derived key was silently re-keyed by a routine refresh. The config's declaration then matched + * nothing in state, and the next plan read "one to create, one to destroy" for a resource that was + * fine and untouched on the host. Recovering meant hand-editing the state file, which had no + * supported command either. + */ +import { describe, it, expect } from "vitest"; +import { chooseAdoptKey, emptyState, upsert, type State } from "../src/state/state.js"; + +const HOST = "https://mychurch.church.tools"; + +/** State holding one group adopted long ago under a deliberately-chosen key. */ +function adopted(): State { + const state = emptyState(HOST); + state.resources.merkmal_alle_2_5_mz = { + type: "group", + id: 1990, + key: "merkmal_alle_2_5_mz", + fields: { name: "Alle 2 bis 5 MZ" }, + adoptedAt: "t", + updatedAt: "t", + }; + return state; +} + +describe("re-adopting keeps the adopted key (#123)", () => { + it("keeps the existing key when the derived one differs, and reports what it would have been", () => { + const choice = chooseAdoptKey(adopted(), "group", 1990, "alle_2_bis_5_mz"); + expect(choice.key).toBe("merkmal_alle_2_5_mz"); + expect(choice.wouldBecome).toBe("alle_2_bis_5_mz"); + }); + + it("re-keys only when --rekey is passed explicitly", () => { + const choice = chooseAdoptKey(adopted(), "group", 1990, "alle_2_bis_5_mz", { rekey: true }); + expect(choice.key).toBe("alle_2_bis_5_mz"); + expect(choice.wouldBecome).toBeUndefined(); + }); + + it("says nothing when the derived key already matches", () => { + const choice = chooseAdoptKey(adopted(), "group", 1990, "merkmal_alle_2_5_mz"); + expect(choice).toEqual({ key: "merkmal_alle_2_5_mz" }); + }); + + it("an explicit --key still wins — it is an explicit intent, not a derivation", () => { + const choice = chooseAdoptKey(adopted(), "group", 1990, "alle_2_bis_5_mz", { + explicitKey: "something_else", + }); + expect(choice.key).toBe("something_else"); + }); + + it("a NEW resource just takes the derived key", () => { + const choice = chooseAdoptKey(adopted(), "group", 4242, "brand_new"); + expect(choice).toEqual({ key: "brand_new" }); + }); + + it("does not confuse a same id of a different TYPE", () => { + // Ids are unique only within a type — the Mainz campus is id 0. A campus #1990 must not inherit + // the group's key. + expect(chooseAdoptKey(adopted(), "campus", 1990, "derived")).toEqual({ key: "derived" }); + }); + + it("the preserved key is what state ends up keyed by — no second entry, no orphan", () => { + const state = adopted(); + const choice = chooseAdoptKey(state, "group", 1990, "alle_2_bis_5_mz"); + upsert(state, { type: "group", id: 1990, key: choice.key, fields: { name: "Alle 2 bis 5 MZ" } }, "n"); + expect(Object.keys(state.resources)).toEqual(["merkmal_alle_2_5_mz"]); + expect(state.resources.merkmal_alle_2_5_mz?.id).toBe(1990); + // The stale-file half of the bug: the ruleset path is built from this key, so preserving it means + // the refresh overwrites rulesets/merkmal_alle_2_5_mz.json instead of writing a second file. + expect(`rulesets/${choice.key}.json`).toBe("rulesets/merkmal_alle_2_5_mz.json"); + }); + + it("still re-keys through upsert when --rekey was given (no orphan left behind)", () => { + const state = adopted(); + const choice = chooseAdoptKey(state, "group", 1990, "alle_2_bis_5_mz", { rekey: true }); + upsert(state, { type: "group", id: 1990, key: choice.key, fields: { name: "x" } }, "n"); + expect(Object.keys(state.resources)).toEqual(["alle_2_bis_5_mz"]); + }); +}); diff --git a/tests/adopt.test.ts b/tests/adopt.test.ts index daec5e8..082ebb7 100644 --- a/tests/adopt.test.ts +++ b/tests/adopt.test.ts @@ -49,12 +49,14 @@ describe("ct adopt", () => { it("adopts a campus (id 0) into the state file", async () => { await runAdopt(["campus", "0", "--state", statePath]); const state = await loadState(statePath, HOST); - expect(state.resources.mz).toMatchObject({ + // The key comes from `name`, like every other resource (#118) — not from `shorty` ("MZ"). + expect(state.resources.mainz).toMatchObject({ type: "campus", id: 0, - key: "mz", + key: "mainz", fields: { name: "Mainz", shorty: "MZ" }, }); + expect(state.resources.mz).toBeUndefined(); }); it("captures a campus-assigned group's campusId from information (#21)", async () => { diff --git a/tests/permission-adopt.test.ts b/tests/permission-adopt.test.ts index 3400e73..b613ed2 100644 --- a/tests/permission-adopt.test.ts +++ b/tests/permission-adopt.test.ts @@ -3,6 +3,7 @@ import { emitAdoptedGrants } from "../src/permissions/adopt.js"; import { diffGrants, normalizeActual, + normalizeEffective, type DomainType, type RawPermission, } from "../src/permissions/grants.js"; @@ -174,12 +175,27 @@ describe("emitAdoptedGrants", () => { expect(parseEmittedGrants(block)).toEqual([]); // comment only, no active grant line }); - it("emits an empty grants array when no user-authored grants remain", () => { + it("emits an empty grants array only when the domain grants NOTHING", () => { + const block = emitAdoptedGrants({ + domainType: "group_role", + domainId: 42, + rows: [], + state: emptyState(), + }); + expect(block).toContain("grants: [],"); + }); + + it("emits a system-baseline grant and says so, instead of silently dropping it (#114)", () => { + // The same right can be system-authored on one host and user-authored on another — the copy that + // produced the second host stamps a person id onto rows that are system rows upstream. Dropping + // it here produced a config that could not be a clean no-op on both: omitted, it REVOKED the + // right on the other host; declared by hand, it planned `+1 grant` on this one. const rows: RawPermission[] = [ - { authId: 2, dataId: null, type: "grant", domainId: 42, meta: { modifiedPid: -1 } }, // baseline only + { authId: 2, dataId: null, type: "grant", domainId: 42, meta: { modifiedPid: -1 } }, ]; const block = emitAdoptedGrants({ domainType: "group_role", domainId: 42, rows, state: emptyState() }); - expect(block).toContain("grants: [],"); + expect(block).not.toContain("grants: [],"); + expect(block).toContain("NOTE: 1 of the grant(s) above are INHERITED or system-baseline"); }); it("group_type_role admin-authored authId >= 10000 member right IS emitted as an active grant (#65)", () => { @@ -352,15 +368,58 @@ describe("emitAdoptedGrants", () => { expect(block).toContain("churchdb:+add person"); const grants = parseEmittedGrants(block); - // Paste-and-plan: diff the emitted declaration against the FULL live row set. normalizeActual - // drops the system-baseline + inherited rows, so they never appear as revokes. + // Paste-and-plan: diff the emitted declaration against the FULL live row set. const desired = grants.flatMap((g) => desiredTuples({ key: "adopted", domainType: "group_type_role", domainId: 9, grants: [g] }, state), ); const actual = normalizeActual(rows); - const diff = diffGrants(desired, actual); + const diff = diffGrants(desired, actual, undefined, normalizeEffective(rows)); + // The system-baseline and inherited rows ARE now declared (#114/#119) and are satisfied by the + // live rows, so they need no PUT... expect(diff.toPut).toEqual([]); - expect(diff.toDelete).toEqual([]); // critically: the system/inherited rows are NOT revoked + // ...and they are still never revoked: revocation is judged against the OWNED set only. + expect(diff.toDelete).toEqual([]); + }); + + it("declaring an inherited right is a no-op HERE and stops the revoke THERE (#119)", () => { + // The measured case: one role instance holds 15 group-member rights that the group's TYPE also + // grants. On prod they read `isInherited: true`; on dev, byte-identical effective permissions + // read `isInherited: false`. Adoption used to drop them on prod, so planning that config on dev + // proposed stripping all 15. + const AUTH = 10107; // churchdb:+add person — unscoped, so the block round-trips cleanly + const prodRows: RawPermission[] = [ + { authId: 1113, dataId: null, type: "grant", domainId: 9, meta: { modifiedPid: 5 } }, + { authId: AUTH, dataId: null, type: "grant", domainId: 9, isInherited: true }, + ]; + const devRows: RawPermission[] = [ + { authId: 1113, dataId: null, type: "grant", domainId: 9, meta: { modifiedPid: 5 } }, + { authId: AUTH, dataId: null, type: "grant", domainId: 9, meta: { modifiedPid: 1 } }, + ]; + + // Adopt from prod — the inherited right is emitted, not dropped. + const block = emitAdoptedGrants({ + domainType: "group_type_role", + domainId: 9, + rows: prodRows, + state: emptyState(), + }); + const grants = parseEmittedGrants(block); + const desired = grants.flatMap((g) => + desiredTuples( + { key: "adopted", domainType: "group_type_role", domainId: 9, grants: [g] }, + emptyState(), + ), + ); + + // ...and the SAME config is a clean no-op on both hosts. + for (const [host, rows] of [ + ["prod", prodRows], + ["dev", devRows], + ] as const) { + const diff = diffGrants(desired, normalizeActual(rows), undefined, normalizeEffective(rows)); + expect(diff.toPut, `${host} toPut`).toEqual([]); + expect(diff.toDelete, `${host} toDelete`).toEqual([]); + } }); it("unmanaged group-dimension scope still gets the 'ct adopt group' hint (unchanged behavior)", () => { @@ -559,3 +618,52 @@ describe("emitAdoptedGrants", () => { } }); }); + +describe('the `-1` "alle" sentinel is not a host-specific id (#115)', () => { + // `churchdb:view alldata` (authId 102) scopes by `cdb_bereich`; `churchdb:view station` (124) by + // `cdb_station`. A grant scoped to "alle" comes back as `dataId: -1`. + const sentinelRows: RawPermission[] = [ + { authId: 102, dataId: -1, type: "grant", domainId: 42, meta: { modifiedPid: 5 } }, + { authId: 124, dataId: -1, type: "grant", domainId: 42, meta: { modifiedPid: 5 } }, + ]; + + const block = () => + emitAdoptedGrants({ domainType: "group_role", domainId: 42, rows: sentinelRows, state: emptyState() }); + + it("never tells you to adopt it — `ct adopt department -1` cannot work", () => { + expect(block()).not.toMatch(/ct adopt \S+ -1/); + }); + + it("does not call it host-specific, because it is not", () => { + expect(block()).not.toMatch(/-1 is not managed/); + expect(block()).not.toMatch(/host-specific number/); + }); + + it("says what it actually is, once per grant", () => { + const lines = block() + .split("\n") + .filter((l) => l.includes('"alle" sentinel')); + expect(lines).toHaveLength(2); + expect(lines[0]).toContain("host-independent"); + }); + + it("still emits the scope value itself, so the declaration keeps its meaning", () => { + expect(block()).toContain("scope: [-1]"); + }); + + it("still flags a REAL unmanaged id in the same grant", () => { + // Mixed scope: -1 is fine, 77 is a genuine host-specific id that does need adopting. + const mixed: RawPermission[] = [ + { authId: 1104, dataId: -1, type: "grant", domainId: 42, meta: { modifiedPid: 5 } }, + { authId: 1104, dataId: 77, type: "grant", domainId: 42, meta: { modifiedPid: 5 } }, + ]; + const out = emitAdoptedGrants({ + domainType: "group_role", + domainId: 42, + rows: mixed, + state: emptyState(), + }); + expect(out).toContain("ct adopt group 77"); + expect(out).not.toContain("ct adopt group -1"); + }); +}); diff --git a/tests/query-refs.test.ts b/tests/query-refs.test.ts index 014fdd8..981780b 100644 --- a/tests/query-refs.test.ts +++ b/tests/query-refs.test.ts @@ -370,3 +370,126 @@ describe("portablizeRuleset (#76 Stage 2)", () => { }); }); }); + +describe("person.id is reported by the portability audit (#127)", () => { + // Four of five auto-group rulesets captured in one week included or excluded specific people by + // id. Those are the SOURCE host's person ids written verbatim into the other host's ruleset, where + // they name entirely different people — the same failure mode as a raw `ctgroup.id`, and the one + // the warning did not mention. The absence of a warning implied there was nothing to find. + + it("reports an `oneof` include-list of person ids", () => { + const w = scanUnportablized({ oneof: [{ var: "person.id" }, [5703, 4389]] }); + expect(w.map((x) => x.id).sort((a, b) => a - b)).toEqual([4389, 5703]); + expect(w.every((x) => x.var === "person.id")).toBe(true); + }); + + it("reports a NEGATED exclusion clause too — the dangerous one", () => { + // `person.id` 1 exists on every ChurchTools instance and is almost always an administrator, so + // an exclusion aimed at one person here lands on someone real there. + const w = scanUnportablized({ "!": [{ oneof: [{ var: "person.id" }, [12, 1]] }] }); + expect(w.map((x) => x.id).sort((a, b) => a - b)).toEqual([1, 12]); + }); + + it("reports an equality comparison against a single person id", () => { + expect(scanUnportablized({ "==": [{ var: "person.id" }, 5703] })).toHaveLength(1); + }); + + it("words it as unfixable — a decision, not a command that will fail", () => { + const lines = formatPortablizeWarnings(scanUnportablized({ oneof: [{ var: "person.id" }, [5703]] })); + const text = lines.join("\n"); + expect(text).toContain("person.id"); + expect(text).toContain("NEVER portable"); + expect(text).toContain("DIFFERENT people on another host"); + // There is no `ct adopt person` and there never will be — people are permanently out of scope. + expect(text).not.toMatch(/ct adopt person/); + }); + + it("does not fire on a non-id person literal", () => { + // Reporting `person.age > 18` would bury the real findings in noise. + expect(scanUnportablized({ ">": [{ var: "person.age" }, 18] })).toEqual([]); + }); + + it("reports person ids alongside the ctgroup ids it already found", () => { + const w = scanUnportablized({ + and: [{ oneof: [{ var: "ctgroup.id" }, [3090]] }, { "!": [{ oneof: [{ var: "person.id" }, [1]] }] }], + }); + expect(new Set(w.map((x) => x.var))).toEqual(new Set(["ctgroup.id", "person.id"])); + }); +}); + +describe("ruleset role refs prefer a managed role-def over the ambiguous name pair (#125)", () => { + // `role.id` used to portablize to a (group type, role NAME) pair, resolved by filtering + // `/group/roles` on (groupTypeId, slug(name)). Two rows sharing a name on one group type made that + // a hard error — and neither remedy the error offered works for a config shared across hosts: + // "rename" edits ChurchTools master data (here, CT's own stock `leader`), and "pass a numeric id" + // cannot work because the roles have different ids per host with no env to branch on. + const ruleset = { oneof: [{ var: "role.id" }, [207]] }; + const roleCatalog = new Map([[207, { groupTypeId: 30, name: "leader" }]]); + const groupTypeIdToKey = new Map([[30, "community"]]); + + it("emits a role-def ref when the role is under management", () => { + const { ruleset: out, warnings } = portablizeRuleset(ruleset, { + idToKeyByKind: { "role-def": new Map([[207, "community_leader"]]) }, + roleCatalog, + groupTypeIdToKey, + }); + expect(warnings).toEqual([]); + expect(out).toEqual({ + oneof: [{ var: "role.id" }, [{ __ctRef: true, kind: "role-def", key: "community_leader" }]], + }); + }); + + it("still emits the (group-type, role) pair when the role is NOT managed", () => { + // The common case is unchanged: the pair is unique across all 46 prod roles. + const { ruleset: out } = portablizeRuleset(ruleset, { + idToKeyByKind: {}, + roleCatalog, + groupTypeIdToKey, + }); + expect(out).toEqual({ + oneof: [ + { var: "role.id" }, + [{ __ctRef: true, kind: "group-type-role", groupType: "community", role: "leader" }], + ], + }); + }); + + it("adopting the role under a shared key is what makes the ambiguous case resolvable", () => { + // Two hosts, same logical key, different numeric ids — the whole point. Each host's capture + // produces the SAME portable marker. + const prod = portablizeRuleset( + { oneof: [{ var: "role.id" }, [127]] }, + { + idToKeyByKind: { "role-def": new Map([[127, "community_leader"]]) }, + roleCatalog: new Map([[127, { groupTypeId: 30, name: "leader" }]]), + groupTypeIdToKey, + }, + ).ruleset; + const dev = portablizeRuleset(ruleset, { + idToKeyByKind: { "role-def": new Map([[207, "community_leader"]]) }, + roleCatalog, + groupTypeIdToKey, + }).ruleset; + expect(prod).toEqual(dev); + }); + + it("also portablizes the out-of-query handleMembership.groupTypeRoleId field", () => { + const { ruleset: out } = portablizeRuleset( + { process: { x: { handleMembership: { groupTypeRoleId: 207 } } } }, + { + idToKeyByKind: { "role-def": new Map([[207, "community_leader"]]) }, + roleCatalog, + groupTypeIdToKey, + }, + ); + expect(out).toEqual({ + process: { + x: { + handleMembership: { + groupTypeRoleId: { __ctRef: true, kind: "role-def", key: "community_leader" }, + }, + }, + }, + }); + }); +}); diff --git a/tests/registry.test.ts b/tests/registry.test.ts index 6e9d5cb..76731a8 100644 --- a/tests/registry.test.ts +++ b/tests/registry.test.ts @@ -32,8 +32,17 @@ describe("resourceType", () => { expect(() => resourceType("nope")).toThrow(/Adoptable types/); }); - it("derives a key from campus shorty", () => { - expect(RESOURCES.campus?.deriveKey({ name: "Mainz", shorty: "MZ" })).toBe("mz"); + it("derives a campus key from its NAME, not its shorty (#118)", () => { + // `shorty` is a display abbreviation an admin edits to make a UI column fit — often historical + // (Idstein is abbreviated "SWA" after the region it used to be named for) and sometimes actively + // misleading (Dietzenbach → "OF", which reads as a typo in a config file). + expect(RESOURCES.campus?.deriveKey({ name: "Mainz", shorty: "MZ" })).toBe("mainz"); + expect(RESOURCES.campus?.deriveKey({ name: "Dietzenbach", shorty: "OF" })).toBe("dietzenbach"); + expect(RESOURCES.campus?.deriveKey({ name: "Würzburg", shorty: "WÜ" })).toBe("wurzburg"); + }); + + it("falls back to shorty only when a campus has no name", () => { + expect(RESOURCES.campus?.deriveKey({ name: "", shorty: "MZ" })).toBe("mz"); }); it("snapshots group ids whether they are nested under information or top-level", () => { diff --git a/tests/state-rm-command.test.ts b/tests/state-rm-command.test.ts new file mode 100644 index 0000000..8214d05 --- /dev/null +++ b/tests/state-rm-command.test.ts @@ -0,0 +1,152 @@ +/** + * `ct state rm` — the missing inverse of `ct adopt` (#122). + * + * `ct adopt` wrote a state entry and nothing removed one. `ct destroy` is the opposite of what + * un-adopting means (it deletes the resource IN ChurchTools), so backing out an adoption meant + * hand-editing `ct-state..json` — the very file the tool insists is its own. Adopt-then-declare + * is the documented loop, so a wrong adoption is normal, not exotic: leaving the entry in makes + * `plan` report a DESTROY on a host where nothing is wrong, and the offline config-matches-state + * check cannot be satisfied without editing the file the check is checking. + * + * The load-bearing property is the one asserted first: NO ChurchTools call, ever. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { emptyState, loadState, saveState } from "../src/state/state.js"; + +const authedSession = vi.fn(async () => { + throw new Error("ct state rm must never contact ChurchTools"); +}); +vi.mock("../src/api/session.js", () => ({ authedSession })); + +const { stateCommand } = await import("../src/commands/state.js"); + +const HOST = "https://mychurch.church.tools"; +let dir: string; +let statePath: string; +let configPath: string; +const originalHost = process.env.CT_HOST; +const originalConfig = process.env.CT_CONFIG; + +async function run(args: string[]): Promise { + await stateCommand().parseAsync(["rm", ...args], { from: "user" }); +} + +/** A state file holding two adopted role definitions and one campus. */ +async function seedState(): Promise { + const state = emptyState(HOST); + for (const [key, id] of [ + ["appmodule_write", 51], + ["appmodule_read", 52], + ] as const) { + state.resources[key] = { + type: "group-role", + id, + key, + fields: { name: key }, + adoptedAt: "t", + updatedAt: "t", + }; + } + state.resources.mainz = { + type: "campus", + id: 0, + key: "mainz", + fields: { name: "Mainz" }, + adoptedAt: "t", + updatedAt: "t", + }; + await saveState(statePath, state); +} + +beforeEach(async () => { + dir = await mkdtemp(join(tmpdir(), "ct-state-rm-")); + statePath = join(dir, "ct-state.json"); + configPath = join(dir, "ct.config.ts"); + process.env.CT_HOST = HOST; + process.env.CT_CONFIG = configPath; + // A config that declares NOTHING — the normal state after deleting a wrong declaration. + await writeFile(configPath, "export default () => {};"); + await seedState(); + vi.spyOn(console, "log").mockImplementation(() => {}); + vi.spyOn(console, "error").mockImplementation(() => {}); +}); + +afterEach(async () => { + vi.restoreAllMocks(); + if (originalHost === undefined) delete process.env.CT_HOST; + else process.env.CT_HOST = originalHost; + if (originalConfig === undefined) delete process.env.CT_CONFIG; + else process.env.CT_CONFIG = originalConfig; + await rm(dir, { recursive: true, force: true }); +}); + +describe("ct state rm (#122)", () => { + it("removes the entry and contacts nothing", async () => { + await run(["group-role", "appmodule_write", "--state", statePath]); + const state = await loadState(statePath, HOST); + expect(state.resources.appmodule_write).toBeUndefined(); + expect(authedSession).not.toHaveBeenCalled(); + }); + + it("leaves every other entry untouched", async () => { + await run(["group-role", "appmodule_write", "--state", statePath]); + const state = await loadState(statePath, HOST); + expect(Object.keys(state.resources).sort()).toEqual(["appmodule_read", "mainz"]); + expect(state.resources.mainz?.id).toBe(0); + }); + + it("refuses a key the config still declares — that would plan a CREATE for a live resource", async () => { + await writeFile( + configPath, + `export default (ct) => { ct.roleDefinition({ key: "appmodule_write", name: "Write", groupTypeId: 2 }); };`, + ); + await expect(run(["group-role", "appmodule_write", "--state", statePath])).rejects.toThrow( + /still declared in the config/, + ); + const state = await loadState(statePath, HOST); + expect(state.resources.appmodule_write).toBeDefined(); + }); + + it("--force removes it anyway, for deleting both in one change", async () => { + await writeFile( + configPath, + `export default (ct) => { ct.roleDefinition({ key: "appmodule_write", name: "Write", groupTypeId: 2 }); };`, + ); + await run(["group-role", "appmodule_write", "--state", statePath, "--force"]); + const state = await loadState(statePath, HOST); + expect(state.resources.appmodule_write).toBeUndefined(); + }); + + it("--dry-run writes nothing", async () => { + await run(["group-role", "appmodule_write", "--state", statePath, "--dry-run"]); + const state = await loadState(statePath, HOST); + expect(state.resources.appmodule_write).toBeDefined(); + }); + + it("rejects a key that is not in state, pointing at `ct state list`", async () => { + await expect(run(["group-role", "nope", "--state", statePath])).rejects.toThrow(/ct state list/); + }); + + it("rejects a type/key mismatch instead of removing the wrong entry", async () => { + await expect(run(["campus", "appmodule_write", "--state", statePath])).rejects.toThrow( + /is a group-role \(#51\), not a campus/, + ); + const state = await loadState(statePath, HOST); + expect(state.resources.appmodule_write).toBeDefined(); + }); + + it("rejects an unknown resource type", async () => { + await expect(run(["widget", "appmodule_write", "--state", statePath])).rejects.toThrow(/Adoptable types/); + }); + + it("still works when the config cannot be read — that is when you need it most", async () => { + // Backing out an adoption is exactly what you do while the config is mid-edit. + await writeFile(configPath, "export default (ct) => { throw new Error('broken'); };"); + await run(["group-role", "appmodule_write", "--state", statePath]); + const state = await loadState(statePath, HOST); + expect(state.resources.appmodule_write).toBeUndefined(); + }); +}); From 5a232d0866540ca0314ec1becf16903c79235a18 Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Mon, 17 Aug 2026 16:23:32 +0200 Subject: [PATCH 2/3] fix(adopt): judge bulk adoption on the rows it actually emits (#129 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adoption emits the EFFECTIVE grant set since #114/#119, but the bulk gate in `ct adopt grants --group/--all-declarable` still judged the OWNED set, so gate and emitter disagreed in both directions: - a domain carrying its rights only as inherited rows counted as "no authored grants" and was skipped, leaving them undeclared for the other host's plan to put in toDelete and revoke — the exact regression #119 exists to fix, left in place on the documented way to adopt at scale; - an inherited grant on a dimension with no logical form was invisible to the declarability gate, so the domain passed --all-declarable and was emitted with a host-specific numeric dataId — the cross-environment misgrant that gate's own comment says it exists to prevent. `declarability` takes a `scope` option rather than switching wholesale: `ct coverage`'s headline number is authored grants, and inherited rows are somebody else's authorship. Default stays "authored", so coverage is untouched. Also in this review pass: - query-refs: a managed `role-def` now wins over the (group-type, role) pair only when that pair is genuinely ambiguous. `role-def` is the weaker ref off this host — the resolver falls back to a `/group/roles` lookup keyed on slug(name) alone, so on a host that never adopted the shared key it can resolve silently to a role on a different group type. The pair cannot fail that way. The #125 tests asserted the preference with a single-row catalog, i.e. never modelled the ambiguity they described; they now model both. - `ct state rm`: the still-declared guard also consults permission declarations. A key named only by a `groupRole` domain or a group-dimension scope passed a resources-only guard and then hard-errored on the next `ct plan` — after the state file had been written. --- src/commands/adopt-grants.ts | 12 +++-- src/commands/state.ts | 43 ++++++++++++++++- src/config/query-refs.ts | 52 ++++++++++++++++----- src/coverage/report.ts | 25 ++++++++-- tests/adopt-grants-bulk.test.ts | 16 +++++++ tests/coverage.test.ts | 37 +++++++++++++++ tests/query-refs.test.ts | 82 +++++++++++++++++++++++++++------ tests/state-rm-command.test.ts | 47 ++++++++++++++++++- 8 files changed, 276 insertions(+), 38 deletions(-) diff --git a/src/commands/adopt-grants.ts b/src/commands/adopt-grants.ts index 326a81c..48fc237 100644 --- a/src/commands/adopt-grants.ts +++ b/src/commands/adopt-grants.ts @@ -93,8 +93,9 @@ export function adoptGrantsCommand(): Command { const config = await resolveConfig(); const statePath = cmdEnv.statePath; const state = await loadState(statePath, config.host); - // Bulk selection runs the same declarability verdict as `ct coverage`, so it needs this - // host's catalog for the same reason (#105): under the bundled one, `--all-declarable` + // Bulk selection runs the same declarability verdict as `ct coverage` (over the effective + // rather than the authored rows — see the call below), so it needs this host's catalog for + // the same reason (#105): under the bundled one, `--all-declarable` // silently SKIPS role instances `ct plan` would manage, filed under an authId the active // catalog can name perfectly well. const hostCatalog = await loadHostCatalog(config.host); @@ -203,7 +204,12 @@ async function emitBulk( for (const role of candidates) { const rows = rowsByDomainId.get(role.domainId) ?? []; - const verdict = declarability(rows); + // Judged on the EFFECTIVE set, because that is what `buildAdoptedGrants` emits (#114/#119). An + // owned-rows verdict here would disagree with the emitter in both directions: it would skip a + // domain whose rights are all inherited on THIS host (18 of 63 group_role domains, measured) and + // leave them undeclared for the other one to revoke, and it would wave through an inherited grant + // on a dimension with no logical form, which the gate below exists to stop. + const verdict = declarability(rows, { scope: "effective" }); if (verdict.grantCount === 0) { skippedEmpty += 1; continue; // nothing authored on this domain — an empty block is not worth a paste diff --git a/src/commands/state.ts b/src/commands/state.ts index 8448587..6f1c7d5 100644 --- a/src/commands/state.ts +++ b/src/commands/state.ts @@ -4,6 +4,7 @@ import { loadConfig, resolveConfigPath } from "../config/load.js"; import { prepareEnv } from "../env/context.js"; import { loadState, saveState } from "../state/state.js"; import { resourceType } from "../resources/registry.js"; +import { collectRefs, isRef, type Ref } from "../resolve/refs.js"; import { info, out, success, warn } from "../ui.js"; interface StateOptions { @@ -114,10 +115,48 @@ export function stateCommand(): Command { * when the config is mid-edit. So a load failure downgrades the guard to a warning rather than * failing the command. */ +/** + * Every logical key the config still names — as a declared resource, AND as a reference from a + * permission declaration. + * + * The permission half matters because a key can be referenced without being declared as a resource in + * the same breath: a `ct.groupRole({ group: "" })` domain, or a group-dimension + * `scope: [""]`. Removing such a key from state passes a resources-only guard and then hard-errors + * on the next `ct plan` in `resolveScope` ("does not resolve to a managed group") — which is precisely + * the broken-config outcome this guard exists to catch before the write, not after it. + * + * Embedded `{ __ctRef }` markers are collected structurally, so a new referencing position is covered + * the day it is added; compound refs contribute the key they actually name (`group-role` a group, + * `group-type-role` a group type). The scope sugar (`{ group: "x" }`) and the bare-string group key + * are NOT refs, so they are picked up explicitly — leniently, since a malformed entry is `ct plan`'s + * to report, and this guard must not turn it into a failure to read the config at all. + */ async function declaredKeys(configOpt: string | undefined): Promise | undefined> { try { - const { resources } = await loadConfig(resolveConfigPath(configOpt)); - return new Set(resources.map((r) => r.key)); + const { resources, permissions } = await loadConfig(resolveConfigPath(configOpt)); + const keys = new Set(resources.map((r) => r.key)); + const addRef = (r: Ref): void => { + if (r.kind === "group-role") keys.add(r.group); + else if (r.kind === "group-type-role") keys.add(r.groupType); + else keys.add(r.key); + }; + for (const ref of collectRefs(permissions)) addRef(ref); + for (const p of permissions) { + for (const g of p.grants) { + if (typeof g === "string" || !Array.isArray(g.scope)) continue; + for (const entry of g.scope) { + // A bare string is a logical GROUP key; one-field sugar names a key on its dimension. + if (typeof entry === "string" && entry.length > 0) keys.add(entry); + else if (entry !== null && typeof entry === "object" && !isRef(entry)) { + const values = Object.values(entry as Record); + if (values.length === 1 && typeof values[0] === "string" && values[0].length > 0) { + keys.add(values[0]); + } + } + } + } + } + return keys; } catch (err) { warn( `Could not read the config to check whether "${configOpt ?? "the default config"}" still ` + diff --git a/src/config/query-refs.ts b/src/config/query-refs.ts index 74b80d0..0703fd8 100644 --- a/src/config/query-refs.ts +++ b/src/config/query-refs.ts @@ -12,6 +12,7 @@ */ import type { GroupTypeRoleRef, RefKind, SimpleRef } from "../resolve/refs.js"; import type { RoleCatalogEntry } from "../resolve/reverse.js"; +import { slug } from "../resources/registry.js"; export type { RoleCatalogEntry }; @@ -220,6 +221,21 @@ export function portablizeRuleset( const marker = (kind: RefKind, key: string): SimpleRef => ({ __ctRef: true, kind, key }) as SimpleRef; + /** + * Does this role's (groupTypeId, slug(name)) pair match more than one catalog row — i.e. is the + * `group-type-role` marker unable to name it? Counted once per call, over the whole catalog, using + * the SAME key the resolver filters on (`resolveGroupTypeRole`), so the two agree by construction. + */ + const pairCounts = new Map(); + const pairKey = (e: RoleCatalogEntry): string => `${e.groupTypeId}${slug(e.name)}`; + if (roleCatalog) { + for (const e of roleCatalog.values()) { + const k = pairKey(e); + pairCounts.set(k, (pairCounts.get(k) ?? 0) + 1); + } + } + const roleNameAmbiguous = (e: RoleCatalogEntry): boolean => (pairCounts.get(pairKey(e)) ?? 0) > 1; + const mapScalar = (value: unknown, kind: RefKind, varName: string): unknown => { if (typeof value !== "number") return value; // booleans/strings/nulls are literals, never entity ids const key = idToKeyByKind[kind]?.get(value); @@ -233,26 +249,38 @@ export function portablizeRuleset( // group type is unmanaged — the escape hatch, identical in spirit to mapScalar's. const mapRoleScalar = (value: unknown, varName: string): unknown => { if (typeof value !== "number") return value; - // A MANAGED role-def wins over the (group-type, role-name) pair (#125). + const entry = roleCatalog?.get(value); + const groupTypeKey = entry ? groupTypeIdToKey?.get(entry.groupTypeId) : undefined; + + // A MANAGED role-def wins over the (group-type, role-name) pair — but ONLY when that pair is + // genuinely ambiguous (#125, narrowed). // - // The name pair is resolved by filtering `/group/roles` on (groupTypeId, slug(name)), so two rows + // The pair is resolved by filtering `/group/roles` on (groupTypeId, slug(name)), so two rows // sharing a name on one group type are a hard error — and neither remedy the error suggests works // for a config shared across hosts. "Rename to disambiguate" means editing ChurchTools master // data to work around a config limitation, and in the observed case one of the two rows is CT's // own stock `leader`. "Pass a numeric id" cannot work at all: the roles have different ids per // host (#127 on one, #207 on the other) and `ConfigContext` deliberately exposes no env or host, - // so there is nowhere to branch. + // so there is nowhere to branch. A `role-def` ref carries a per-host id in managed state — the + // same mechanism `group` and `campus` refs already use — so it makes that case fixable IN CONFIG, + // by adopting the role under a shared key on both hosts. // - // A `role-def` ref carries a per-host id in managed state — the same mechanism `group` and - // `campus` refs already use to mean different numbers on different hosts. Emitting it makes the - // ambiguous case fixable IN CONFIG, by adopting the role under a shared key on both hosts, which - // is the same "adopt the target to portablize it" move that works everywhere else. - const roleDefKey = idToKeyByKind["role-def"]?.get(value); - if (roleDefKey !== undefined) { - return marker("role-def", roleDefKey); + // Why it must NOT win unconditionally: `role-def` is the WEAKER reference of the two off this + // host. `Resolver.resolveSimple` falls back to `resolveFromCatalog`, which keys `/group/roles` by + // slug(name) ALONE — the very mapping this module's header calls wrong for a `role.id`, and the + // reason #76 reverted #86. So on a host where the role was never adopted under the shared key, a + // name matching exactly one row resolves SILENTLY to a role on a different group type (only a + // multi-row match errors), and the ruleset then matches the wrong role. The `group-type-role` + // marker cannot fail that way: it resolves on the (groupTypeId, name) pair, which is unique by + // construction. So the safe pair stays the safe pair, and `role-def` is reserved for the case + // that has no safe pair to lose. + const ambiguous = entry !== undefined && roleNameAmbiguous(entry); + if (ambiguous || groupTypeKey === undefined) { + const roleDefKey = idToKeyByKind["role-def"]?.get(value); + if (roleDefKey !== undefined) { + return marker("role-def", roleDefKey); + } } - const entry = roleCatalog?.get(value); - const groupTypeKey = entry ? groupTypeIdToKey?.get(entry.groupTypeId) : undefined; if (entry && groupTypeKey !== undefined) { return { __ctRef: true, diff --git a/src/coverage/report.ts b/src/coverage/report.ts index 66a12ac..f57f38d 100644 --- a/src/coverage/report.ts +++ b/src/coverage/report.ts @@ -19,7 +19,7 @@ * the I/O, so the whole verdict is unit-testable without a network. */ import { KNOWN_AUTH_IDS, SCOPE_FIELD_BY_AUTH_ID } from "../permissions/catalog.js"; -import { normalizeActual, type RawPermission } from "../permissions/grants.js"; +import { normalizeActual, normalizeEffective, type RawPermission } from "../permissions/grants.js"; import { ALL_SCOPE_SENTINEL, SCOPE_REF_KIND } from "../permissions/scope.js"; import { fromInformation } from "../resources/registry.js"; import type { State } from "../state/state.js"; @@ -114,11 +114,26 @@ export interface CoverageInput { * ownership default. (With `preserveUnknown` (#102) those same instances become declarable while * leaving the module grants alone; the verdict below is the strict one, and the command surfaces the * blocking dimensions precisely so they can be passed to `preserveUnknown`.) + * + * `scope` picks WHICH rows the verdict is about, and the two callers genuinely need different ones: + * - `"authored"` (default) — `ct coverage`, whose headline number is "grants an admin authored here". + * Inherited rows are somebody else's authorship and would inflate it. + * - `"effective"` — `ct adopt grants`, which since #114/#119 EMITS the effective set. The verdict has + * to be about the same rows the emitter will print, or it decides on rows that never reach the + * block: a domain carrying rights only as inherited rows counts as empty and is skipped (so the + * rights stay undeclared and the next apply on the other host revokes them), and an inherited grant + * on an unnameable dimension is invisible to the `blockedBy` gate that exists to stop exactly that + * line from being emitted as a host-specific number. */ -export function declarability(rows: RawPermission[]): DeclarabilityVerdict { - // `normalizeActual` is what makes the count right: it drops the self-re-adding system baseline and - // every inherited row, so the verdict is about ADMIN-AUTHORED grants — the only ones ct ever owns. - const tuples = normalizeActual(rows).filter((t) => t.type === "grant"); +export function declarability( + rows: RawPermission[], + { scope = "authored" }: { scope?: "authored" | "effective" } = {}, +): DeclarabilityVerdict { + // Both normalizers drop the self-re-adding system baseline; they differ only on inherited rows, + // which `normalizeActual` also drops. Neither decides ownership — that stays `normalizeActual`'s + // job at plan time, so widening the verdict here never widens what ct writes or revokes. + const normalize = scope === "effective" ? normalizeEffective : normalizeActual; + const tuples = normalize(rows).filter((t) => t.type === "grant"); const blockedBy = new Set(); const unknownAuthIds = new Set(); for (const t of tuples) { diff --git a/tests/adopt-grants-bulk.test.ts b/tests/adopt-grants-bulk.test.ts index 1a9ae2c..bfaf6d5 100644 --- a/tests/adopt-grants-bulk.test.ts +++ b/tests/adopt-grants-bulk.test.ts @@ -42,6 +42,8 @@ const groupRows = [ ], }, { id: 11, name: "Ops", information: { groupTypeId: 2 }, roles: [{ id: 200, groupTypeRoleId: 16 }] }, + // `youth` (#12, managed) carries its rights ONLY as inherited rows — the #119 shape. + { id: 12, name: "Youth", information: { groupTypeId: 2 }, roles: [{ id: 300, groupTypeRoleId: 16 }] }, ]; const roleDefRows = [ { id: 16, name: "Leiter", groupTypeId: 2 }, @@ -53,6 +55,8 @@ const permissions: RawPermission[] = [ perm(UNSCOPED, 101), perm(HTML_TEMPLATE, 101, 3), // blocks role instance 101 perm(UNSCOPED, 200), + { ...perm(UNSCOPED, 300), isInherited: true }, + { ...perm(VIEW_GROUP, 300, 12), isInherited: true }, ]; const getMock = vi.fn(async (path: string): Promise => { @@ -83,6 +87,7 @@ const state: State = { host: HOST, resources: { kids: { type: "group", id: 10, key: "kids", fields: { name: "Kids" }, adoptedAt: "t", updatedAt: "t" }, + youth: { type: "group", id: 12, key: "youth", fields: { name: "Youth" }, adoptedAt: "t", updatedAt: "t" }, }, }; @@ -165,6 +170,17 @@ describe("ct adopt grants --all-declarable (#104)", () => { expect(stdout).not.toContain("kids_mitglied"); }); + // The bulk counterpart of #119. The single-domain path emits the effective set; the bulk path used + // to decide whether to emit at all from the OWNED set, so a domain whose rights are all inherited + // here counted as "no authored grants" and was skipped. Its rights then stay undeclared — and on the + // host that materialises the same rights as DIRECT rows, the next plan puts them in toDelete. + it("emits a domain whose grants are all inherited, rather than counting it empty", async () => { + const { stdout } = await run(["grants", "--all-declarable"]); + expect(stdout).toContain('key: "youth_leiter"'); + expect(stdout).toContain('group: "youth"'); + expect(stdout).toContain("churchgroup:view group"); + }); + it("--write appends the blocks to a file instead of printing them", async () => { const target = join(workDir, "grants.ts"); writeFileSync(target, "// existing\n", "utf8"); diff --git a/tests/coverage.test.ts b/tests/coverage.test.ts index a706c94..7de3c97 100644 --- a/tests/coverage.test.ts +++ b/tests/coverage.test.ts @@ -72,6 +72,43 @@ describe("declarability (#103)", () => { }); }); +describe("declarability scope (#114/#119)", () => { + // `ct adopt grants` emits the EFFECTIVE set, so its gate has to judge the effective set too. + // Judging owned rows while emitting effective ones desynchronises the two in both directions. + it("counts inherited grants under scope: effective, so an all-inherited domain is not 'empty'", () => { + // The measured shape: a domain carrying its rights ONLY as inherited rows on this host. Under the + // authored count it looks empty, bulk adoption skips it, the rights stay undeclared — and the next + // apply on the host that materialises them as DIRECT rows puts them in toDelete and revokes them. + const rows = [ + row({ authId: VIEW_GROUP, dataId: 42, domainId: 1, isInherited: true }), + row({ authId: UNSCOPED, domainId: 1, isInherited: true }), + ]; + expect(declarability(rows).grantCount).toBe(0); + expect(declarability(rows, { scope: "effective" }).grantCount).toBe(2); + expect(declarability(rows, { scope: "effective" }).declarable).toBe(true); + }); + + it("blocks on an inherited grant with no logical form under scope: effective", () => { + // The opposite desync: the emitter prints this row as a host-specific numeric dataId, so the gate + // must see it. Under the authored verdict it is invisible and the domain passes --all-declarable. + const rows = [ + row({ authId: VIEW_GROUP, dataId: 42, domainId: 1 }), + row({ authId: HTML_TEMPLATE, dataId: 3, domainId: 1, isInherited: true }), + ]; + expect(declarability(rows)).toMatchObject({ declarable: true, blockedBy: [] }); + expect(declarability(rows, { scope: "effective" })).toMatchObject({ + declarable: false, + blockedBy: ["cc_html_template"], + }); + }); + + it("defaults to the authored set, so `ct coverage` is untouched", () => { + const rows = [row({ authId: VIEW_GROUP, dataId: 42, domainId: 1, isInherited: true })]; + expect(declarability(rows)).toEqual(declarability(rows, { scope: "authored" })); + expect(declarability(rows).grantCount).toBe(0); + }); +}); + describe("buildCoverageReport (#103)", () => { const state: State = { version: 1, diff --git a/tests/query-refs.test.ts b/tests/query-refs.test.ts index 981780b..2b965a9 100644 --- a/tests/query-refs.test.ts +++ b/tests/query-refs.test.ts @@ -417,20 +417,33 @@ describe("person.id is reported by the portability audit (#127)", () => { }); }); -describe("ruleset role refs prefer a managed role-def over the ambiguous name pair (#125)", () => { - // `role.id` used to portablize to a (group type, role NAME) pair, resolved by filtering - // `/group/roles` on (groupTypeId, slug(name)). Two rows sharing a name on one group type made that - // a hard error — and neither remedy the error offered works for a config shared across hosts: - // "rename" edits ChurchTools master data (here, CT's own stock `leader`), and "pass a numeric id" - // cannot work because the roles have different ids per host with no env to branch on. - const ruleset = { oneof: [{ var: "role.id" }, [207]] }; - const roleCatalog = new Map([[207, { groupTypeId: 30, name: "leader" }]]); +describe("ruleset role refs fall back to a managed role-def only when the name pair is ambiguous (#125)", () => { + // `role.id` portablizes to a (group type, role NAME) pair, resolved by filtering `/group/roles` on + // (groupTypeId, slug(name)). Two rows sharing a name on ONE group type make that a hard error — and + // neither remedy the error offered works for a config shared across hosts: "rename" edits + // ChurchTools master data (here, CT's own stock `leader`), and "pass a numeric id" cannot work + // because the roles have different ids per host with no env to branch on. A managed `role-def` key + // is the way out of THAT case. + // + // It is only a way out of that case, though. Off this host a `role-def` ref is the WEAKER of the + // two: the resolver falls back to a `/group/roles` lookup keyed on slug(name) alone, so on a host + // where the role was never adopted under the shared key it can resolve silently to a role on a + // different group type. The pair cannot fail that way. So the pair stays the default and `role-def` + // is reserved for the ids the pair genuinely cannot name. const groupTypeIdToKey = new Map([[30, "community"]]); + // Two rows, same group type, same name — the collision #125 is actually about. + const ambiguousCatalog = new Map([ + [207, { groupTypeId: 30, name: "leader" }], + [208, { groupTypeId: 30, name: "Leader" }], + ]); + // One row — the common case: unique across all 46 prod roles. + const uniqueCatalog = new Map([[207, { groupTypeId: 30, name: "leader" }]]); + const ruleset = { oneof: [{ var: "role.id" }, [207]] }; - it("emits a role-def ref when the role is under management", () => { + it("emits a role-def ref when the name pair is ambiguous and the role is under management", () => { const { ruleset: out, warnings } = portablizeRuleset(ruleset, { idToKeyByKind: { "role-def": new Map([[207, "community_leader"]]) }, - roleCatalog, + roleCatalog: ambiguousCatalog, groupTypeIdToKey, }); expect(warnings).toEqual([]); @@ -439,11 +452,44 @@ describe("ruleset role refs prefer a managed role-def over the ambiguous name pa }); }); + it("keeps the (group-type, role) pair when it is unambiguous, EVEN IF the role is managed", () => { + // The safety property: a `role-def` ref can mis-resolve to another group type's role on a host + // that never adopted the key, and the pair cannot. Nothing is gained by trading down here, so a + // managed role-def must not win by default. + const { ruleset: out, warnings } = portablizeRuleset(ruleset, { + idToKeyByKind: { "role-def": new Map([[207, "community_leader"]]) }, + roleCatalog: uniqueCatalog, + groupTypeIdToKey, + }); + expect(warnings).toEqual([]); + expect(out).toEqual({ + oneof: [ + { var: "role.id" }, + [{ __ctRef: true, kind: "group-type-role", groupType: "community", role: "leader" }], + ], + }); + }); + it("still emits the (group-type, role) pair when the role is NOT managed", () => { - // The common case is unchanged: the pair is unique across all 46 prod roles. const { ruleset: out } = portablizeRuleset(ruleset, { idToKeyByKind: {}, - roleCatalog, + roleCatalog: uniqueCatalog, + groupTypeIdToKey, + }); + expect(out).toEqual({ + oneof: [ + { var: "role.id" }, + [{ __ctRef: true, kind: "group-type-role", groupType: "community", role: "leader" }], + ], + }); + }); + + it("leaves an ambiguous, unmanaged role to the pair — which hard-errors at plan time, honestly", () => { + // Nothing portable to emit: the pair cannot name it and there is no shared key yet. Emitting the + // pair anyway is right — the resolver's ambiguity error is what tells the author to adopt it. + const { ruleset: out } = portablizeRuleset(ruleset, { + idToKeyByKind: {}, + roleCatalog: ambiguousCatalog, groupTypeIdToKey, }); expect(out).toEqual({ @@ -461,16 +507,22 @@ describe("ruleset role refs prefer a managed role-def over the ambiguous name pa { oneof: [{ var: "role.id" }, [127]] }, { idToKeyByKind: { "role-def": new Map([[127, "community_leader"]]) }, - roleCatalog: new Map([[127, { groupTypeId: 30, name: "leader" }]]), + roleCatalog: new Map([ + [127, { groupTypeId: 30, name: "leader" }], + [128, { groupTypeId: 30, name: "Leader" }], + ]), groupTypeIdToKey, }, ).ruleset; const dev = portablizeRuleset(ruleset, { idToKeyByKind: { "role-def": new Map([[207, "community_leader"]]) }, - roleCatalog, + roleCatalog: ambiguousCatalog, groupTypeIdToKey, }).ruleset; expect(prod).toEqual(dev); + expect(prod).toEqual({ + oneof: [{ var: "role.id" }, [{ __ctRef: true, kind: "role-def", key: "community_leader" }]], + }); }); it("also portablizes the out-of-query handleMembership.groupTypeRoleId field", () => { @@ -478,7 +530,7 @@ describe("ruleset role refs prefer a managed role-def over the ambiguous name pa { process: { x: { handleMembership: { groupTypeRoleId: 207 } } } }, { idToKeyByKind: { "role-def": new Map([[207, "community_leader"]]) }, - roleCatalog, + roleCatalog: ambiguousCatalog, groupTypeIdToKey, }, ); diff --git a/tests/state-rm-command.test.ts b/tests/state-rm-command.test.ts index 8214d05..72e6f79 100644 --- a/tests/state-rm-command.test.ts +++ b/tests/state-rm-command.test.ts @@ -58,6 +58,14 @@ async function seedState(): Promise { adoptedAt: "t", updatedAt: "t", }; + state.resources.youth = { + type: "group", + id: 3090, + key: "youth", + fields: { name: "Youth" }, + adoptedAt: "t", + updatedAt: "t", + }; await saveState(statePath, state); } @@ -94,10 +102,47 @@ describe("ct state rm (#122)", () => { it("leaves every other entry untouched", async () => { await run(["group-role", "appmodule_write", "--state", statePath]); const state = await loadState(statePath, HOST); - expect(Object.keys(state.resources).sort()).toEqual(["appmodule_read", "mainz"]); + expect(Object.keys(state.resources).sort()).toEqual(["appmodule_read", "mainz", "youth"]); expect(state.resources.mainz?.id).toBe(0); }); + // A key can be named by a PERMISSION declaration without being declared as a resource. A + // resources-only guard waves those through, and the breakage surfaces one command later as a + // `ct plan` hard error ("does not resolve to a managed group") — after the state file was written. + it("refuses a key a permission DOMAIN still names, not just a declared resource", async () => { + await writeFile( + configPath, + `export default (ct) => { ct.groupRole({ key: "youth_leiter", group: "youth", role: "Leiter", grants: ["churchcore:administer settings"] }); };`, + ); + await expect(run(["group", "youth", "--state", statePath])).rejects.toThrow( + /still declared in the config/, + ); + const state = await loadState(statePath, HOST); + expect(state.resources.youth).toBeDefined(); + }); + + it("refuses a key a permission SCOPE still names", async () => { + await writeFile( + configPath, + `export default (ct) => { ct.groupRole({ key: "p", id: 77, grants: [{ right: "churchgroup:view group", scope: ["youth"] }] }); };`, + ); + await expect(run(["group", "youth", "--state", statePath])).rejects.toThrow( + /still declared in the config/, + ); + const state = await loadState(statePath, HOST); + expect(state.resources.youth).toBeDefined(); + }); + + it("still removes a key no declaration — resource or permission — mentions", async () => { + await writeFile( + configPath, + `export default (ct) => { ct.groupRole({ key: "p", id: 77, grants: [{ right: "churchgroup:view group", scope: ["someone_else"] }] }); };`, + ); + await run(["group", "youth", "--state", statePath]); + const state = await loadState(statePath, HOST); + expect(state.resources.youth).toBeUndefined(); + }); + it("refuses a key the config still declares — that would plan a CREATE for a live resource", async () => { await writeFile( configPath, From 2735be6ce09cbf4384e0bf4730cdaa3b93875607 Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Mon, 17 Aug 2026 16:31:59 +0200 Subject: [PATCH 3/3] docs: re-read and re-sign for the #129 review fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dynamic-groups: the managed `role-def` form is emitted only when the (group-type, role) pair actually collides — off-host it is the weaker reference of the two, since it resolves by bare name. permissions: bulk `ct adopt grants` judges declarability over the effective rows, the same set it emits. README: `state rm`'s refusal covers permission declarations, not only resources. --- README.md | 3 +++ docs/handbuch/dynamic-groups.md | 16 ++++++++-------- docs/handbuch/permissions.md | 2 +- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 1c95403..1b59c0d 100644 --- a/README.md +++ b/README.md @@ -116,6 +116,9 @@ from the state file, makes no HTTP call, and leaves the resource in place in ChurchTools, now unmanaged. It refuses a key the config still declares — that would make the next plan propose creating a resource that already exists — so delete the declaration first, or pass `--force` to do both in one change. +"Declares" covers permission declarations too, not only resources: a key named +by a `ct.groupRole` domain or a group scope is just as broken to remove, and the +refusal is what keeps that from surfacing one command later as a plan error. `apply` reconciles **creates and updates** only, saving state after each action (crash-safe / resumable). It **never deletes**: a resource dropped from the diff --git a/docs/handbuch/dynamic-groups.md b/docs/handbuch/dynamic-groups.md index 6726c2b..f8570ef 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/commands/adopt-group.ts -sources_hash: 9a8c3091484e9258 +sources_hash: d67f76530e841833 reviewed: 2026-08-17 --- @@ -166,13 +166,13 @@ position that maps to a **managed** logical key is rewritten to its `{ __ctRef } marker; every other id is left numeric. The `var → RefKind` catalog it keys off (`VAR_REF_KINDS`) is: -| ChurchQuery `var` | marker `kind` | source catalog / state | -| --------------------- | ----------------- | ---------------------------------------- | -| `ctgroup.id` | `group` | managed state (no REST catalog) | -| `ctgroup.campusId` | `campus` | `/campuses` | -| `person.campusId` | `campus` | `/campuses` | -| `ctgroup.groupTypeId` | `group-type` | `/group/grouptypes` | -| `role.id` | `group-type-role` | `/group/roles` (by `groupTypeId` + name) | +| ChurchQuery `var` | marker `kind` | source catalog / state | +| --------------------- | ----------------- | -------------------------------------------------- | +| `ctgroup.id` | `group` | managed state (no REST catalog) | +| `ctgroup.campusId` | `campus` | `/campuses` | +| `person.campusId` | `campus` | `/campuses` | +| `ctgroup.groupTypeId` | `group-type` | `/group/grouptypes` | +| `role.id` | `group-type-role` | `/group/roles` (by `groupTypeId` + name) | | `role.id` | `role-def` | managed state — only when the pair collides (#125) | The same `group-type-role` rewrite also covers the **out-of-query** integer diff --git a/docs/handbuch/permissions.md b/docs/handbuch/permissions.md index a7b13c4..1144be5 100644 --- a/docs/handbuch/permissions.md +++ b/docs/handbuch/permissions.md @@ -5,7 +5,7 @@ sources: - src/resolve/resolver.ts - src/resolve/refs.ts - src/config/context.ts -sources_hash: 5f9ed09f1e3793b0 +sources_hash: 87fe6c847d70d89c reviewed: 2026-08-17 ---