Repository navigation
feat: pending group_role domains (#106), Bereiche + security levels as managed resources (#108/#110), commentViewer refs (#102), and an audit that can see past OpenAPI (#111) - #112
Merged
Conversation
…hat can see past OpenAPI Three issues that all trace back to one habit: writing down "the API cannot do this" when what was actually checked was narrower than that. #106 — a `ct.groupRole` whose group is declared in the same config could not plan on a host where that group did not exist yet, so one config planned clean on prod and exited 1 on dev. The information needed was already available at apply time: the resource tier runs first, so the group exists before permissions are applied. The domain now resolves to a PendingRef and is completed inside applyPermissionPlan by reading the freshly created group's own /groups/{id}/roles and matching the declared role name. Pending is opt-in per call site, because this is the one pending ref reresolvePendingValue cannot finish from state alone — everywhere else a same-run group keeps the old fail-fast message. A role the created group does not have is still a hard error listing the available ones. #110 — "security-level ids are universal, so a numeric literal is portable" was an assumption. cc_securitylevel is an admin-editable master-data table with an auto-increment id, so 1/2/3 line up by convention, not by construction. Adds `{ securityLevel: "<name-slug>" }`, resolved through GET /securitylevels (a new `ct get security-levels` lists them), keeps numerics working, and retires the "numeric-universal dimension" exemption in the coverage report. #111 — the manual-surface re-audit only grepped the OpenAPI spec, so it could never discover the legacy master-data endpoint the admin UI writes through — and concluded "permanently", "never", "not resources at all" on that basis. The procedure now requires a REST probe AND a getMasterData registry probe, plus a UI capture where neither settles it, and says to record which probes ran. The three buckets are relabelled so a capability fact, a not-yet-implemented item and a product decision stop reading alike, and the absolutes that were unsupported are down-scoped in the runbook, api-coverage, the handbook and the code comments. Bereiche (#108) and the generic master-data driver (#109) are not implemented here: both need live probing and a design decision about REST-vs-master-data authority per table. Claude-Session: https://claude.ai/code/session_016TacHkv3oVczwXGHBwckq2
…as POST/PATCH/DELETE
Live-probed against eqrm-dev (CT 3.135.2, 2026-08-14) with the two-probe
procedure this PR added, and it immediately caught an error in this PR's own
text: `/securitylevels` is GET-only, but `/securitylevels/{id}` exposes POST
(create, body `{name}`), PATCH (update, body `{name, newid, forcereorder}` —
reordering is a first-class supported operation) and DELETE. #110 asserted "no
REST write verb", inherited it from the issue, and I repeated it.
What actually changes: security levels stay a catalog `ct` reads and does not
manage, but for a different and much weaker reason — the id-bearing create path
does not fit the resource registry's "POST the collection path" contract, so
promotion is a design call, not an API limit. The reference form (#110's actual
ask) is unaffected and stays exactly as it was.
Also strengthens the case for the ref form rather than weakening it: a supported
`newid`/`forcereorder` reorder means "the ids are stable in practice" was never
going to hold.
Corrected in scope.ts, refs.ts, resolver.ts, api-coverage.md, the runbook, and
both handbook pages. Departments re-probed on dev and unchanged: GET only.
Claude-Session: https://claude.ai/code/session_016TacHkv3oVczwXGHBwckq2
…half of #102, via REST `/person/commentviewers` is conventional REST CRUD (`[{id, name, sortKey}]` plus POST/PUT/DELETE on the item path), live-probed on eqrm-dev CT 3.135.2 2026-08-14. So `{ commentViewer: "<name-slug>" }` is one more CATALOG_PATH entry and RefKind, exactly like the security-level ref — no legacy master-data endpoint needed, which is what #109 assumed this would take. Unblocks the three role instances on Bereich Pastoral Care that each carried exactly one comment_viewer-scoped grant, and so fell out of `ct coverage` as undeclarable in their entirety. `id: 0` is a real row on a live instance ("Alle"), so there is a test pinning that a falsy id resolves rather than reading as "not found". Two tests that used cc_securitylevel and then cdb_comment_viewer as their stand-in for "a dimension with no logical form" now use cc_calcategory, which is a more honest example: the dimensions still without a ref kind are overwhelmingly those of modules outside this tool's mandate (calendars, resources, services, wiki, finance) — a scope decision, not an inability to name them. Comments and docs say that now instead of "not resources at all". Claude-Session: https://claude.ai/code/session_016TacHkv3oVczwXGHBwckq2
…ed id (#110) Live-probed on eqrm-dev (CT 3.135.2, 2026-08-14; created id 99 next to levels 1–4, renamed it, deleted it, instance restored): POST /securitylevels/99 {name} → 200 {"id":99,"name":…,"sortkey":99} PATCH /securitylevels/99 {name} → 200 DELETE /securitylevels/99 → 200 Levels 1–4 were untouched throughout and sortkey mirrored the id, so an insert does not implicitly renumber — reordering is opt-in via newid + forcereorder. The interesting part is that the id is CALLER-ASSIGNED: you POST to the id you want and CT 409s if it is taken. Every other managed type lets CT mint the id. That is deliberate on CT's side — a security level id is referenced instance-wide (person fields' securityLevelId, cc_securitylevel grant scopes), so it has to be reproducible rather than whatever a fresh instance auto-increments to. It also resolves #110's original worry from the other end: a config that declares its levels makes the numeric `scope: [1, 2, 3]` form genuinely portable. Engine: two additions to the resource registry, both opt-in and both currently used by exactly one type (pinned by a test). - `createPath(body)` — the POST target for a create, defaulting to collectionPath. Security levels return `/securitylevels/{declared id}`. - `callerAssignedId` — declares that the id comes from the config. The planner refuses a CHANGED id for such a type: that is a renumber, which rewrites what every numeric scope on the dimension grants, and the executor would otherwise emit a plain `PATCH {id}` that CT either ignores or 409s — a plan that never converges. The error names the real operation instead. `id` is a managed field so drift is visible; `ct adopt security-level <id>` emits it, which is what lets an adopted config recreate the level at the same id on a fresh host. destroyWarning matches person-status: a delete reaches every person field and grant referencing the level. Also makes `security-level` a managed ref kind, so a level declared in the config resolves from state before the catalog and an adopted grant scoped to a managed level now emits `{ securityLevel: "stufe_2_mittel" }` instead of a bare number. Claude-Session: https://claude.ai/code/session_016TacHkv3oVczwXGHBwckq2
… legacy master-data endpoint (#108) Reads stay REST (`GET /departments`); only writes take the legacy `POST /index.php?q=churchdb/ajax` route the admin UI uses. Verified end to end against eqrm-dev (CT 3.135.2, 2026-08-14) with the real CLI — plan, apply, re-plan, apply an update, delete — and the instance was left as found: func=saveMasterData table=cdb_bereich id= → creates (server assigns the id) func=saveMasterData table=cdb_bereich id=N → updates that row func=deleteMasterData table=cdb_bereich id=N → deletes it Before this, a Bereich-scoped grant only planned on a host where somebody had already created the Bereich by hand, so `ct` could not stand up a fresh instance from config — the assumption the whole dev→prod promotion model rests on. Scope: ONE table. Classifying all 24 tables in the registry against dev's own OpenAPI spec found 15 with a REST write path — REST stays authoritative for every one of them, per the standing rule — and of the 9 without, 8 are person master data or field option lists outside this tool's mandate. Bereiche were the only table both in-mandate and REST-less, which is why #109's generic driver is not built: it would have had exactly one in-scope user. Engine, all opt-in and each used by exactly one type (pinned by tests): - `CtClient.ajax()` — the legacy channel, reusing the session cookie and CSRF token the client already holds. A failure there is a 200 with `{"status":"error"}`, so the envelope is checked rather than the status code. - `writer` — create/update/(remove) for a type whose writes are not REST. `create` returns the id itself because saveMasterData does not return one; it re-reads `/departments` and matches the name, refusing to guess when 0 or 2+ rows match rather than binding state to the wrong Bereich permanently. - `fetchOne` — read one row when `GET {itemPath}` does not exist. That last one is a bug the live run caught and no unit test could have: there is no `/departments/{id}` path at all, so the default read 404d, every caller read that as "vanished in ChurchTools", and the second plan proposed creating the same Bereich again — forever. A mock answers whatever path it is asked for. Fixed and pinned. Columns are validated against the instance's own DESCRIBE before posting, because the legacy endpoint does not 400 on an unknown colN — it ignores it, so an unvalidated write looks like a success while silently dropping the field. Bereiche also become a managed ref kind: a `{ department: "…" }` scope now resolves from state first, and can name a Bereich declared in the same run. Claude-Session: https://claude.ai/code/session_016TacHkv3oVczwXGHBwckq2
2000game
marked this pull request as ready for review
August 14, 2026 07:49
…k creates, and a missing item path
Eight findings from the review of this PR, fixed with a regression test each.
CRITICAL. `applyPermissionPlan` handed the role-list fetcher `{ get: client.get }`, which DETACHES
the prototype method from its instance — the real `CtClient.get` calls `this.requestEnvelope`, so
every pending group_role domain (#106's whole scenario) died with "this.requestEnvelope is not a
function" at apply time. Bind instead. The existing tests could not see it: their doubles are plain
objects that close over nothing, so the new test double is a CLASS, state on `this`, method on the
prototype — verified to fail without the fix.
HIGH. `departmentWriter.create` wrote first and checked for ambiguity second, so a Bereich created
by hand on the target host made the create succeed, the ambiguity check throw, nothing land in
state — and every re-run add another orphan. Snapshot the catalog ids BEFORE the write, identify
the new row by set difference, and refuse a pre-existing name up front (pointing at `ct adopt`).
`ct adopt department <id>` GET `/departments/{id}`, a path that does not exist — every invocation
404'd, while the docs advertised the command. Adopt now prefers the spec's own `fetchOne`.
The master-data registry cache never hit: it keys a WeakMap on the client, but `requireAjax`
returned a fresh literal per call, so each write refetched the ~3 MB `getMasterData` payload.
Memoize the wrapper per client. Likewise `department.fetchOne` re-read the whole `/departments`
collection once per managed Bereich under the `fetchActual` fan-out — share one read, dropped by
every write so it can never go stale.
A `securityLevel` declaration missing `id` POSTed to a literal `/securitylevels/undefined` (and a
string `"3"` diffed forever against the numeric actual). Nothing validated it — `id` is a plain
managed field, not one of `ID_FIELDS`. Require a non-negative integer at eval time for any
caller-assigned-id type.
`ct destroy` exited 0 on the "no delete path" branch, reporting success while the resource stayed
in ChurchTools AND in state; same for a missing write spec. Both now exit 1.
Finally, `assertNotPeople` guards REST paths only and cannot see the legacy `ajax` channel, while
the master-data registry includes person tables. The one-table scope is now enforced by an
allowlist in masterdata.ts rather than only documented.
Claude-Session: https://claude.ai/code/session_016TacHkv3oVczwXGHBwckq2
…the two pages whose sources moved The handbook already said `id` is required on `ct.securityLevel`; it is now actually rejected at config-evaluation time (a missing id used to POST to `/securitylevels/undefined`), so say when the rejection happens. blueprints.md re-read unchanged — its sources moved, its subject matter did not. Claude-Session: https://claude.ai/code/session_016TacHkv3oVczwXGHBwckq2
This was referenced Aug 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #106, #108, #110, #111, and the
cdb_comment_viewerhalf of #102. #109 is closed as overtaken — see the bottom.All five started as one defect:
docs/runbook-manual-surface.mddefined the manual surface by grepping the OpenAPI spec, which structurally cannot see the legacy endpoint ChurchTools' admin UI writes master data through — and then wrote the results down as "permanently", "never", "not resources at all". Fixing the procedure first, then re-probing, turned three "impossible" items into shipped features. It also caught a wrong claim in this PR's own first commit.Everything below was probed live against eqrm-dev, CT 3.135.2, 2026-08-14. Every write experiment restored the instance to its prior state.
#106 — a group and its own
groupRoleplan on a fresh hostThe domain half of #29's deadlock. A
ct.groupRolewhosegroupwas declared in the same config but absent from the target host was a hard error, so the identical config planned clean on prod and exited 1 on dev — non-portable by construction, and one logical change had to be split across two merges.The information was already available:
executePlanruns the resource tiers beforeapplyPermissionPlan. So the domain resolves to aPendingRef, renders as<group-role(group=…, role=…) (created this apply)>, and is completed at apply time by reading the freshly created group's own/groups/{id}/roles. Arolethe group turns out not to have is still a hard error listing the roles it does have — shared code path with plan time, so the message cannot drift.Pending group-roles are opt-in per call site. This is the one pending ref
reresolvePendingValuecannot finish from state alone — a pairing id is not the group's id — and only the permission-domain position gets the live fetch. Everywhere else keeps the old fail-fast error.#108 — Bereiche are a managed resource
GET /departmentsis a normal catalog; no REST write verb exists. The admin UI writes Bereiche throughPOST /index.php?q=churchdb/ajaxfunc=saveMasterData, andctdoes now too:Verified end to end on dev with the real CLI — plan → apply → re-plan no-op → update → delete.
deleteMasterDataexists: a made-updelMasterDataanswers"Function delMasterData was not defined as Function!", so the endpoint validates function names and the success is real, not a silent no-op.Reads stay REST, columns are validated against the instance's own
DESCRIBEbefore posting (the legacy endpoint does not 400 on an unknowncolN— it ignores it, so an unvalidated write looks like a success while dropping the field), and create refuses to guess when it cannot identify the row it just wrote by name.A bug only the live run could catch: there is no
/departments/{id}path at all. The default read 404d, every caller read that as "vanished in ChurchTools", and the second plan proposed creating the same Bereich again — forever. A mock answers whatever path it is asked for. Fixed with afetchOnehook and pinned.#110 — security levels are a managed resource with a declared id
The issue said "no REST write verb". Wrong, and I repeated it in this PR's first commit before re-probing.
/securitylevels/{id}has POST (create), PATCH (update, incl. reorder vianewid+forcereorder) and DELETE. Corrected in a follow-up commit.The id is caller-assigned — you POST to the id you want, CT 409s if taken:
That is the first such type here, and it resolves the issue's worry from the other end: a config that owns its levels makes numeric
scope: [1, 2, 3]genuinely portable. The name-based ref form ships too, for configs that don't own them.Two guard rails: changing a declared id is refused at plan time (that is a renumber, which rewrites what every numeric scope on the dimension grants, and the executor would otherwise emit a
PATCH {id}CT ignores or 409s — a plan that never converges), andct destroywarns that a delete reaches every person field and grant referencing the level.#102 (partial) —
commentViewerscope refs/person/commentviewersis plain REST CRUD, so{ commentViewer: "gemeindeleitung" }is one more catalog ref — no legacy endpoint, contrary to #109's assumption. Unblocks the three Pastoral Care role instances each blocked by exactly one comment_viewer-scoped grant.id: 0is a real row ("Alle"), pinned by a test so nothing treats a falsy id as missing. ThepreserveUnknownhalf of #102 is untouched.#111 — the audit that produced all of the above
A "no write API" verdict now requires both the REST/OpenAPI probe and a
getMasterData → masterDataTablesprobe, plus a UI capture where neither settles it, plus a record of which probes ran and when. The three buckets are relabelled ("No write APIctcan drive" / "Not yet implemented" / "Out of tool scope") so a capability fact, a roadmap item and a product decision stop reading alike, with an explicit rule for when an absolute is allowed.Down-scoped claims across the runbook,
api-coverage.md, both handbook pages, and the code comments inrefs.ts,resolver.ts,scope.ts,types.ts,adopt.tsandreport.ts. The two rows still carrying only a REST probe (member statuses, meeting points) are flagged as such rather than left looking verified.#109 — closed as overtaken
The epic assumed most of the 24 registry tables lack a REST write. Live, 15 have one (campuses, group types, person statuses, security levels, comment viewers, group categories, roles, age/target groups, meeting templates, dbfields, sexes, relationship types, growpaths, contact labels) — and per your rule, REST stays authoritative for all of them. Of the 9 without, 8 are person master data or field option lists outside this tool's mandate. Bereiche were the only table both in-mandate and REST-less, so the generic driver would have had exactly one in-scope user.
Full 24-table inventory posted on #109 before closing, so the finding survives if the mandate ever widens. #96 needs no change either —
/statusesis full REST CRUD and person statuses are already adoptable.Verification
npm run typecheck,npm test(729 passed, 5 skipped),npm run format:check,npx eslint src tests, andmkdocs build --strictall pass. Docs-staleness re-signed for every touched page. Pre-existing repo-wideeslint .failures come from a local.venv-docs/and are unrelated.Live on eqrm-dev, each restoring the instance: security-level create/rename/delete (levels 1–4 untouched throughout, confirming an insert does not implicitly renumber); Bereich create/update/delete via
saveMasterData; and a fullct plan/apply/re-plan cycle for a declared Bereich through the real CLI.New tests cover the pending group-role apply path end to end, the caller-assigned-id create and the renumber refusal, the department writer including the
fetchOneregression and the unidentifiable-create refusal, column validation against the instance's schema, and the two new scope refs.https://claude.ai/code/session_016TacHkv3oVczwXGHBwckq2