feat(resources): promote comment-viewer to a declarable resource (#151) - #152
Merged
Conversation
`cdb_comment_viewer` was the last scope dimension a config could not express portably. It had a reference form since #102 but nothing to declare the viewers themselves, so a config granting `churchdb:view comments` had to write a raw, host-specific `dataId` — and that id silently means something different, or nothing, on another host. Nothing detects it: a plan compares the declared id against the live id and they match; the id is just meaningless on the target. Switching to a name ref made it worse rather than better, because the second host lacks the NAMES too, so the ref hard-errors instead of resolving wrongly. Declaring the viewer is what makes the name exist on both hosts, which is what makes the reference portable. - registry: `comment-viewer` at `/person/commentviewers`, tier 0, PUT updates, managing `name` + `sortKey` (the whole editable surface of this 3-column master-data table). No new machinery: CT mints the id (unlike `security-level`) and the writes are REST (unlike `department`). Reads of a single viewer filter the collection — a GET on the item path was never probed, and guessing it has the #108 failure mode (a 404 reads as "vanished", so every plan re-creates it). - DSL: `ct.commentViewer({ key, name, sortKey })`; `ct adopt comment-viewer <id>` falls out of the registry. - resolution: `{ commentViewer: "…" }` now resolves managed-state-first, live catalog second, so a viewer declared in the same run resolves pending and is re-resolved at apply time — and a ref to a viewer the config does not own keeps working exactly as before. - grant adoption: an unmanaged viewer dataId now earns the `ct adopt comment-viewer <id>` hint instead of the weaker "portable form" note, and a managed one is emitted as `{ commentViewer: "<key>" }`. Docs: permissions.md's catalog-only section is replaced by the declarable one (no scope dimension with a logical form is catalog-only any more), plus rows in api-coverage.md and README. Claude-Session: https://claude.ai/code/session_011DU9pubAh7dGWyojwd1smn
…t-in "Alle" Review of #152 raised five findings. A live probe on eqrm-dev (CT 3.135.2, 2026-08-26 — one throwaway row created, read, updated, deleted, instance left as found) settled the two that needed evidence. - The create path was described three different ways: "conventional REST", "the item path takes POST/PUT/DELETE", and a plain collection POST in the code. The probe confirms the code: POST on the COLLECTION, CT mints the id. Replaced the vague prose in registry.ts, api-coverage.md and permissions.md with the actual verb table, and recorded why a viewer can never be callerAssignedId — the minted id skips deleted rows and moved 5 -> 8 -> 11 on a three-row instance. - Dropped `fetchOne`. It filtered the whole collection because `GET {itemPath}` was unprobed and guessing wrong has #108's failure mode. The probe retired the guess: a present id returns the row, an absent one a clean 404 error.notfound. That is what the default read needs, and it removes one full collection read per managed viewer per plan/apply/destroy — `fetchActual` fans out concurrently, so the hook cost N round-trips against a rate-limited API. - `dataId: 0` (the built-in "Alle" viewer) is no longer advertised as adoptable. Only `-1` was filtered, so adoption printed `ct adopt comment-viewer 0`; following that hint duplicates "Alle" on any second host, misgrants to the duplicate, and makes `{ commentViewer: "alle" }` permanently ambiguous. Added BUILTIN_SCOPE_IDS, keyed by dimension so `0` elsewhere stays an ordinary host-specific id, and emitted as a number (an admin can rename the row). - Corrected the stale ScopeEntry doc (departments and security levels have been declarable since #108/#110; comment viewers since #151) and a section heading that stated the opposite of its own section. The registry's "3-column table, so the managed set is complete" reasoning was wrong — rows carry a fourth column, `nameTranslated`. The conclusion holds for a different reason: the probe's PUT sent only {name, sortKey} and nameTranslated followed name, so it is derived, not a sibling a PUT could blank. Claude-Session: https://claude.ai/code/session_011DU9pubAh7dGWyojwd1smn
Follow-up review of e9b03ac found three minor issues, no correctness defects. - `builtinScopeIdName` hardcoded the comment-viewer case instead of deriving from BUILTIN_SCOPE_IDS, so the two could drift: adding a dimension to the table but not the function would suppress the adoption hint while emitting no comment explaining the suppression — a bare `scope: [0]` with nothing saying why it was not flagged. The name now lives in the table and `isBuiltinScopeId` derives from it, which makes the two impossible to desync. - The "verified live on two hosts, 2026-08-26" date conflated two probes: the two-host read was 2026-08-24, and 2026-08-26 was eqrm-dev only. - The duplicate-"Alle" story overstated the ambiguity error. Resolution is managed-state-first, so on the host that created the duplicate the ref quietly resolves to it and never reaches resolveFromCatalog's ambiguity check. The hard error only reaches a config reading that catalog without a state entry, so the failure surfaces somewhere other than where it was caused — which is worse than the original wording, not better. Claude-Session: https://claude.ai/code/session_011DU9pubAh7dGWyojwd1smn
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
1 participant
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 #151.
cdb_comment_viewerwas the last scope dimension a config could not express portably. It had a reference form since #102 but nothing to declare the viewers, so a config grantingchurchdb:view commentshad to write a raw, host-specificdataId— which silently means something different, or nothing, on another host, and no plan or row count can see it. The obvious workaround (a name ref) is worse, not better: the second host lacks the names too, so the ref hard-errors instead of resolving wrongly.Declaring the viewer is what makes the name exist on both hosts, which is what makes the reference portable.
What changed
comment-viewerat/person/commentviewers, tier 0,PUTupdates, managingname+sortKey(the whole editable surface of this 3-column master-data table, per epic(resources): generic master-data driver — 22 self-describing editable tables behind one endpoint #109's list). No new machinery was needed: CT mints the id (unlikesecurity-level) and the writes are REST (unlikedepartment).ct adopt comment-viewer <id>falls out of the registry.ct.commentViewer({ key, name, sortKey }).{ commentViewer: "…" }now resolves managed state first, live catalog second, so a viewer declared in the same run resolves pending and gets its real id at apply time, while a ref to a viewer the config does not own keeps working exactly as in feat(permissions): opt-in partial grant ownership — one module-scoped grant currently blocks 403 of 590 grants #102.dataIdnow earns thect adopt comment-viewer <id>hint instead of the weaker "portable form" note; a managed one is emitted as{ commentViewer: "<key>" }.api-coverage.mdand the README.Acceptance, per the issue
ct adopt comment-viewer <id>emits a config entry and a state rowtests/comment-viewer-resource.test.ts"adopt → config → plan round-trips to a no-op"ct.commentViewer({ key, name, sortKey })declares one; plan/apply create and update, never delete implicitlyct.commentViewer+ "declaring, creating and updating a comment viewer" (apply never deletes — unchanged engine rule;ct destroycarries a warning naming what a delete reaches)REF_KIND_TYPEentry; "the same config means the same thing on two hosts"dataId [4]on one host and[2]on the otherOne judgement call worth a look
Reads of a single viewer filter the collection rather than hitting
GET /person/commentviewers/{id}. The probe behind this dimension recorded the collection read plus POST/PUT/DELETE on the item path; a GET on the item path was never probed, and guessing it has the #108 failure mode — a 404 reads as "vanished in ChurchTools", so every plan proposes creating the viewer again and every apply duplicates it. If someone probes that path live, thefetchOnehook can just be dropped.Not live-probed in this PR by design (the stored token points at prod).
Verification
npm run typecheck,npm test(1001 passed, 5 skipped — 11 new),npm run format.npm run lintreports 7 errors, all pre-existing and all inside an untracked stray.claude/worktrees/.../dist/index.js— identical on a clean checkout ofmain.https://claude.ai/code/session_011DU9pubAh7dGWyojwd1smn