Repository navigation
feat: coverage, bulk grant adoption, per-instance catalog, honest pagination - #107
Merged
Merged
Conversation
…ination Closes #100, #101, #102, #103, #104, #105. Also closes #98, which shipped in PR #99 but never auto-closed. A theme runs through all six: the tool was quiet in places where being quiet is the same as being wrong. A paged read that returned 10 of 645 rows, a ruleset that froze one host's ids into a cross-host file, a warning that told consumer repos to run a script they do not have. #100 — `ct get raw` follows pagination It issued one request and printed CT's default first page, so raw and the typed commands disagreed by 635 rows on the same path, in a valid-looking JSON array. A probe request now runs first with no paging params (so a single-object path is never given paging params it might 400 on); an array that reports more pages is re-read through `getAll`. `--no-paginate` / `--page <n>` / a caller's own `page=`/`limit=` keep the single-request probe — and then dropped rows are a loud WARNING, since silence is the one thing this must not do again. #101 — un-portablized ruleset ids are reported, not swallowed CT does not validate the ids inside a ruleset, so one carrying prod's `ctgroup.id` applied to dev does not error: the auto-group collects the wrong people and `ct plan` stays green, because the ruleset round-trips byte-identically against the host it was written for. - every id left numeric is now named with its REASON (unmanaged target / role unknown to /group/roles / role's group type unmanaged / no logical form exists), at adoption AND at plan time; - `ReverseResolver` reads its catalogs with `getAll`. It used a plain `get`, so only the first 10 rows of `/group/roles` were ever indexed — on eqrm prod that silently left 36 of 46 roles unportablizable. The forward resolver was fixed for this; the reverse side had the same bug; - portablization is ON by default (`--no-portable-rulesets` opts out); - `--strict-rulesets` refuses to write a ruleset that still carries one. #102 — opt-in `preserveUnknown` One `cc_html_template` grant made a 41-grant role undeclarable, because a partial declaration turns a clean no-op into a destructive plan. `preserveUnknown: true | ["<dimension>"]` keeps undeclared live grants instead of revoking them. Opt-in per declaration, the strict default unchanged, a dimension list never widening to unscoped rights, a dimension no right scopes by rejected at eval time (a typo that preserves nothing reads exactly like "nothing to preserve"), and every preserved grant rendered explicitly and counted apart from the change totals — "I forgot one" and "I left the module grants alone" must not look alike. #103 — `ct coverage` Answers "what exists here that I am not managing, and could I manage it?" — joining `/groups?include[]=roles`, `/dynamicgroups` and `/permissions/group_role` against state. Declarability is per (group, role), not per group, since one group routinely has two declarable roles and one blocked one. Inherited rows are excluded from authored counts; forgetting that inflated one hand-rolled audit from 590 to 714 grants. `--json` for CI gates, `--type` / `--declarable` / `--blocked` for the detail. #104 — bulk `ct adopt grants` `--group` / `--all-declarable` / `--write`. Emits the portable `group` + `role` domain form with a derived `<group>_<role>` key whenever the group is managed — the two edits a human forgets on the 30th paste. A block that would revoke live grants is skipped and summarised rather than printed: in bulk the per-block WARNING header stops being a safeguard and becomes something to scroll past. Nothing is capped silently. #105 — per-instance catalog + `ct refresh` `ct permissions catalog --refresh` captures the catalog from the repo's own host into `.ct/permission-catalog.<host>.json`; plan/apply load it over the bundled one, and the version-skew warning goes quiet when it does — a capture from the target host is authoritative for it, and a warning on every plan is one nobody reads. `ct refresh --group <key>` / `--all` re-evaluates a managed auto-group that did NOT change (which `apply --refresh` cannot reach), and apply now says outright that CT materializes membership on its own schedule, so an empty auto-group after a green apply is expected. The legacy scheduler ping runs every due job on the instance, so it stays documented and unfired. Also: `/coverage/` in .gitignore is now root-anchored. The bare form matched `src/coverage/` and would have dropped the new report module from every commit. Claude-Session: https://claude.ai/code/session_016TacHkv3oVczwXGHBwckq2
`npm run format:check` existed but ran nowhere, so it had drifted to failing on 109 files on main — a check that fails by default is not a check. Added it to both CI and release (before lint, so the cheapest gate fails first) and ran `npm run format` across the repo to make it pass. The bulk of the diff is mechanical Prettier reflow with no behaviour change. Docs staleness signatures re-signed, since reformatting touched declared sources. Verified: format:check, lint, typecheck, 681 tests, build, docs staleness — all green. Claude-Session: https://claude.ai/code/session_016TacHkv3oVczwXGHBwckq2
Both introduced by the previous commit's handbuch edits: - dynamic-groups.md linked ../runbook-manual-surface.md. Only pages under handbuch/ publish, so a relative link out of the section resolves to nothing in the built site — strict mode treats that as fatal. Uses the absolute GitHub URL the other cross-section links already use. - permissions.md's in-page anchor had a doubled hyphen from the em dash in the heading; mkdocs collapses whitespace runs to one. Verified by running the strict build locally (mkdocs build --strict, clean). Claude-Session: https://claude.ai/code/session_016TacHkv3oVczwXGHBwckq2
…unchecked claims Review findings on #100–#105. Catalog consistency. `ct plan`/`ct apply` loaded the per-instance capture AFTER loadConfig, so `preserveUnknown` dimensions were validated against the BUNDLED catalog and then planned against the captured one — a dimension present only in the capture threw, and one present only in the bundle passed validation and preserved nothing, which is the exact silent failure that validation exists to prevent. `ct coverage` and `ct adopt grants` never loaded it at all, so they judged declarability off the bundled catalog: coverage reported "blocked by authId N" (failing --json CI gates) and `--all-declarable` SKIPPED role instances that `ct plan` manages without complaint. All four now load it first. Version skew. A per-instance capture is authoritative for its host at capture time, not forever, so suppressing the comparison entirely meant an instance upgraded past its committed capture got no staleness signal at all. The comparison runs either way now; only the remediation wording differs. The unknown-authId warning stops pointing consumer repos at `npm run regenerate:permission-catalog`, a script they do not have. Unchecked claims. `scanUnportablized` runs portablizeRuleset with no id maps and no role catalog, so its "unmanaged" / "role-unknown" reasons were never checked against anything — `ct plan` told anyone who hand-wrote the raw id of an ADOPTED group that it is not under management, and handed them a `ct adopt` that fails. Scan mode now degrades to a neutral `left-numeric`. Details are id-free so formatPortablizeWarnings can merge ids without stamping one id's remedy across the group; the detail joins the grouping key so a future id-bearing one splits. Also: `ct get raw <path?limit=50> --page 2` appended a second `limit`, making the window size depend on the server's parsing rule — rejected now, since both spellings are explicit. A per-instance catalog whose entries lack `authId` passed the "is it an object" gate and reached `ct apply` as a PUT with `authId: undefined` — entry shape is validated on load. Pagination guard. Every `/permissions/<domainType>` read was a plain `get` on the unchecked belief that the endpoint returns one instance-wide blob. If that is ever wrong the failure is silent, because `request()` drops the `meta` that is the only evidence rows are missing. `fetchPermissionRows` reads the envelope and pages properly when pagination says more rows exist — one request, as before, while the endpoint stays un-paged. Claude-Session: https://claude.ai/code/session_016TacHkv3oVczwXGHBwckq2
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 #100, closes #101, closes #102, closes #103, closes #104, closes #105.
Also closes #98 — it shipped in #99 but never auto-closed.
A theme runs through all six: the tool was quiet in places where being quiet is
the same as being wrong. A paged read that returned 10 of 645 rows. A ruleset
that froze one host's ids into a cross-host file. A warning that told consumer
repos to run a script they do not have.
#100 —
ct get rawfollows paginationIt issued one request and printed CT's default first page, so
rawand the typedcommands disagreed by 635 rows on the same path — in a valid-looking JSON array,
which gave the reader no reason to doubt it.
A probe request now runs first with no paging params added (so a single-object
path like
/groups/42is never handed params it might 400 on); an array whosemeta.paginationreports more pages is re-read throughgetAll.--no-paginate/--page <n>/ a caller's ownpage=/limit=keep thesingle-request probe — and then dropped rows are a loud
INCOMPLETE:warning.#101 — un-portablized ruleset ids are reported, not swallowed
ChurchTools does not validate the ids inside a ruleset, so one carrying prod's
ctgroup.idapplied to dev does not error: the auto-group simply collects thewrong people, and
ct planstays green because the ruleset round-tripsbyte-identically against the host it was written for.
unknown to
/group/roles/ role's group type unmanaged / no logical formexists — at adoption and at plan time;
ReverseResolvernow reads its catalogs withgetAll. It used a plainget, so only the first 10 rows of/group/roleswere ever indexed; on eqrmprod that silently left 36 of 46 roles unportablizable. This is most of why
role.idstayed raw in ct-structure. The forward resolver was fixed forexactly this; the reverse side had the same bug;
--no-portable-rulesetsopts out);--strict-rulesetsrefuses to write a ruleset that still carries one.#102 — opt-in
preserveUnknownOne
cc_html_templategrant made a 41-grant role undeclarable, because a partialdeclaration turns a clean no-op into a destructive plan.
Opt-in per declaration; the strict default does not move. A dimension list never
widens to unscoped rights. A dimension no right scopes by is an eval-time error —
a typo that preserves nothing reads exactly like "nothing to preserve", right up
until an apply revokes 41 grants. And preserved grants are rendered explicitly
and counted apart from the change totals, so "I forgot one" and "I deliberately
left the module grants alone" cannot look alike:
#103 —
ct coverageJoins
/groups?include[]=roles,/dynamicgroupsand/permissions/group_roleagainst state. Declarability is per (group, role) — one group routinely has
two declarable roles and one blocked one, which group granularity would hide.
Inherited rows are excluded from the authored counts (forgetting that inflated
one hand-rolled audit from 590 to 714 grants).
--jsonfor CI gates;--type/--declarable/--blockedfor the detail.#104 — bulk
ct adopt grants--group <keyOrId>,--all-declarable,--write <path>. Emits the portablegroup+roledomain form with a derived<group>_<role>key whenever thegroup is managed — the two edits a human forgets on the 30th paste. A block that
would revoke live grants is skipped and summarised rather than printed: in bulk
the per-block
WARNINGheader stops being a safeguard and becomes something toscroll past. Nothing is capped silently.
#105 — per-instance catalog +
ct refreshct permissions catalog --refreshcaptures the catalog from the repo's ownhost into
.ct/permission-catalog.<host>.json; plan/apply load it over thebundled one and say so. The version-skew warning goes quiet when they do — a
capture from the target host is authoritative for it, and a warning that fires on
every plan is one nobody reads, including on the plan where a moved authId
matters.
ct refresh --group <key>/--allre-evaluates a managed auto-group that didnot change (which
apply --refreshcannot reach, and which a no-op planleaves no lever for at all).
ct applynow states outright that CT materializesmembership on its own schedule, so an empty auto-group after a green apply is
expected rather than alarming — the surprise was the expensive part.
Per the decision on this PR's scope, the legacy scheduler ping
(
?q=cron&standby=true) runs every due job on the instance, so it staysdocumented in the runbook and
ctnever fires it.Note
.gitignore'scoverage/is now root-anchored: the bare form also matchedsrc/coverage/and would have silently dropped the new report module from everycommit.
Also:
format:checkis now a real gatenpm run format:checkexisted but ran nowhere, so it had drifted to failing on109 files on
main— a check that fails by default is not a check. It now runsin CI and release, ahead of lint so the cheapest gate fails first, and the
repo has been formatted to satisfy it (second commit; almost entirely mechanical
Prettier reflow, no behaviour change).
Verification
npm run format:check,npm run lint,npm run typecheck,npm test(681passing, +55 new),
npm run build, and the docs staleness gate — all green.Handbuch pages re-signed after re-reading each against its changed sources.
https://claude.ai/code/session_016TacHkv3oVczwXGHBwckq2