Repository navigation
fix: apply correctness & safety — #113, #120, #121, #124, #126 - #128
Conversation
…, #120, #121, #124, #126) The "Apply correctness & safety" milestone: five bugs where plan/apply could produce or commit a wrong result against a live ChurchTools instance. #124 + #113 — `GET /dynamicgroups` returns a flat array of BARE GROUP IDS, not objects. Two call sites read it as `Number(row.id ?? row.groupId)` — NaN for every element — so the id set was empty on every host. `ct refresh` answered "not a dynamic group" for every group and could not succeed at all; `ct coverage` reported `dynamic: 0` on an instance with 70 auto-groups, which is the one wrong answer that looks like good news. Both now read the endpoint through a single parser (`src/api/dynamicGroups.ts`) that pins the scalar shape and tolerates an object form. #126 — a plan degraded into a WRONG plan under HTTP 429. A dynamic group whose ruleset read failed still had its DESIRED side folded, so it diffed a declared ruleset against an absent actual and surfaced as an ordinary `to update`. In a PR comment that is indistinguishable from real drift and invites the same response: apply it. A fold that cannot read the actual side now reports the resource as unreadable, `buildPlan` marks it fetch-failed (a no-op, rendered under "could not be read"), and the summary line carries `INCOMPLETE — N resource(s) could not be read.` so the count can never be missing from the artefact humans approve. `--json` gains `summary.unreadable`. #121 — `apply` could not create a role definition: POST /group/roles 400s without `type`/`isDefault`/`isHidden`, none of which were sent. `plan` was green, so the breakage landed in `apply`, on the host the pipeline writes to. `type` is a real semantic choice (leader vs participant), so it is declarable and defaults to `participant`; the two booleans are create defaults. An invalid `type` is now rejected at config load, not as an HTTP 400 mid-apply. #120 — a `groupRole` whose ROLE is created in the same run hard-errored. #106 made the missing GROUP resolve as pending; the role half did not, so a `groupRole` naming a `roleDefinition` declared three lines up failed with a message whose two remedies ("fix the name, or pass a numeric id") are both wrong for a shared multi-host config. The role now resolves as pending and completes in the same run — role definitions are tier 3, which executes before permission reconciliation. BREAKING CHANGE: `roleDefinition` creates now send `type: "participant"`, `isDefault: false` and `isHidden: false`. Declare `type: "leader"` to get the previous-intent leader semantics on a newly created role. Existing roles are unaffected — these are create-only defaults and are never diffed. Claude-Session: https://claude.ai/code/session_01V24GdD2F6LTGTF5xWoGq4i
…d live (#121) The live probe against eqrm-dev (CT 3.135.2) that #121 asked for, run at last. POSTing the pre-fix body to /group/roles is rejected with FOUR validation errors, not the three the issue could name: sortKey "Bitte eine ganze Zahl eingeben (ohne Punkt und Komma)." validation.integer type "Die Eingabe sollte eine der folgenden Werte sein: leader, participant" isDefault "Eingabe muss TRUE oder FALSE sein." validation.boolean isHidden "Eingabe muss TRUE oder FALSE sein." validation.boolean `sortKey` is the field the issue recorded only as "one more field with an empty args". Without it the previous commit's fix still failed with HTTP 400 — the same unattended-apply breakage, one field further along. It defaults to 0, which is what all 87 stock roles on the probed instance carry. Also confirmed and recorded: `isLeader` must NOT be sent. CT derives it from `type` — a role created with `type: "leader"` reads back `isLeader: true` without it ever appearing in the body — so sending it would be a second source of truth for one fact. Verified end-to-end against dev: `ct apply` creates the role definition, and the re-plan is a clean no-op (the round-trip invariant CONTRIBUTING requires). Every probe artefact was deleted and the instance left as found — 87 roles before, 87 after. Claude-Session: https://claude.ai/code/session_01V24GdD2F6LTGTF5xWoGq4i
Live probe done — #121 needed a fourth fieldRan the probe against eqrm-dev (CT 3.135.2), now that I'm cleared to connect there. The fix as originally committed was still broken: POSTing it returned The pre-fix body is rejected with four validation errors, not the three #121 could name:
Second finding: {"id":291,"name":"ZZ ct-cli probe leader","type":"leader","isDefault":false,"isHidden":false,"isLeader":true,"sortKey":0}Sending it would be a second source of truth for one fact, so the registry comment now says so explicitly. End-to-end verificationNot just the raw POST — the real command path against dev: That last line is the invariant CONTRIBUTING requires ("a clean apply must round-trip to a no-op"), which also confirms the create-only defaults are correctly excluded from the diff. Instance left as foundEvery artefact deleted and verified gone: probe roles #288 and #291 via Tests now 759 passing (+1 — a new case pinning that |
#120 made a `ct.groupRole` naming a same-run `roleDefinition` resolve as pending instead of hard-erroring. The check matched on the role NAME alone — but role names are not unique across group types, as this very file documents at length (live prod: 3 "Leiter", 6 "Organisator", 6 "Mitglied", each on a different type). So a config declaring `roleDefinition({ name: "Mitglied", groupType: "x" })` alongside a `ct.groupRole` on a group of a DIFFERENT type — one that genuinely has no such role — read as pending rather than as the config error it is. It still fails, with the same message, but from `applyPermissionPlan`'s post-apply fetch instead of at plan time: after `executePlan` has already written resources. Fail-fast is the whole reason the plan-time check exists. The check now also matches the group's own `groupTypeId`. It stays lenient wherever the answer is not knowable offline — a declaration that states no group type, or a group whose state entry predates `groupTypeId` being recorded — so it only ever turns a would-be pending back into the hard error it used to be, and only on positive evidence. #120's behaviour is unchanged everywhere else.
Re-read against src/resolve/resolver.ts and re-signed: the #120 pending path now requires the declared roleDefinition to be for this group's group type, so a name-only match no longer defers a config error past executePlan.
Closes #113, closes #120, closes #121, closes #124, closes #126.
The whole Apply correctness & safety milestone: five bugs where plan/apply could produce or commit a wrong result against a live instance.
#124 + #113 —
/dynamicgroupsreturns bare ids, not objectsThe endpoint returns
[159, 1698, 1704, …]. Two call sites read it asNumber(row.id ?? row.groupId)→NaNfor every element, so the id set was empty on every host.ct refreshfailed its guard for every group — the command could not succeed at all, and its "not a dynamic group" message sent you debugging the apply instead.ct coveragereporteddynamic: 0on an instance with 70 auto-groups. A zero is the one wrong answer that looks like good news.Fixed at one place —
src/api/dynamicGroups.ts— with a test pinning the scalar shape (an array of scalars is easy to re-break), plus tolerance for an object form and numeric strings.#126 — plan degraded into a wrong plan under 429
Top-level resource fetch failures were already handled correctly. The fabrication came from the synthetic fold: when
/dynamicgroups/{id}/ruleset429'd, the actual side was left unset but the desired side was still folded — so the group diffed against an absent actual and appeared as an ordinaryto update.buildPlanmarks it fetch-failed, so it renders under "could not be read" as a no-op instead of joining the update count./groups/hierarchiesfailure, which previously droppedparentssilently — an under-report rather than an over-report, but equally dishonest.INCOMPLETE — N resource(s) could not be read., soPlan: 10 to create, 24 to updatecan never appear bare in a PR comment.--jsongainssummary.unreadable.Retry-with-backoff honouring
Retry-After(suggestion 2 in the issue) already existed insrc/api/http.ts;planalready exited 1 andapplyalready refused to run on any fetch error. The gap was purely the content of the degraded plan.Verified the three new assertions fail on the pre-fix tree and pass after.
#121 —
applycould not create a role definitionPOST /group/roles400s withouttype/isDefault/isHidden.planwas green, so this landed inapply, on the host the pipeline writes to automatically, and the re-run failed identically.typeis a genuine semantic choice (leadervsparticipant), so it is declarable onct.roleDefinitionand defaults toparticipant; the two booleans are create defaults. An invalidtypeis rejected at config load rather than as an HTTP 400 mid-apply. The registry comment claimingtypewas optional — the thing that made this look supported — is replaced with the live validation errors.createDefaultsmerging also no longer lets an explicitly-undefineddeclared field knock out a default CT requires.Verified live on eqrm-dev (CT 3.135.2), and the probe found a fourth required field. The POST is rejected with four validation errors, not the three #121 could name —
sortKeyis the one the issue recorded only as "one more field with an emptyargs":fieldIdmessageKeysortKeyvalidation.integertypevalidation.inisDefaultvalidation.booleanisHiddenvalidation.booleanAlso confirmed:
isLeadermust not be sent — CT derives it fromtype, and a role created withtype: "leader"reads backisLeader: truewithout it ever being in the body.End-to-end through the real command path, not just the raw POST:
That last line is the round-trip invariant CONTRIBUTING requires, and it confirms the create-only defaults stay out of the diff. Every probe artefact was deleted and verified gone — 87 roles before, 87 after. Full detail in the probe comment.
#120 — a
groupRolewhose ROLE is created in the same run#106 made the missing group resolve as pending; the role half did not. A
groupRolenaming aroleDefinitiondeclared three lines up hard-errored with "Fix the role name, or pass a numeric id" — both wrong for a shared multi-host config.The role now resolves as pending and completes in the same run: role definitions are tier 3, which executes before permission reconciliation, so by completion time the role is on the group's list. A test drives the real build → execute → apply sequence to prove one-run convergence. A role that is nowhere — not on the host, not declared — is still a hard error, and the direct error now names the real remedy.
Breaking change
roleDefinitioncreates now sendtype: "participant",isDefault: false,isHidden: false. Declaretype: "leader"for leader semantics on a newly created role. Existing roles are unaffected: these are create-only defaults, never diffed.npm run typecheck,npx eslint src tests,npx prettier --check— cleannpx vitest run— 759 passed, 5 skipped (was 748; +11 new)docs-staleness.mjs— all 5 pages current;permissions.mdanddynamic-groups.mdre-read and updated for the new behaviourhttps://claude.ai/code/session_01V24GdD2F6LTGTF5xWoGq4i