Uh oh!
There was an error while loading. Please reload this page.
fix(service-messaging): correct sys_notification_subscription index comment to name sys_member/sys_team_member - #9918
Merged
Conversation
…omment to name sys_member/sys_team_member The (topic, principal) unique-index comment claimed role:x/team:x resolve against sys_permission_set/sys_position (citing #8461/#8556, which scoped those objects' OWN uniqueness — an unrelated precedent, not evidence about what this selector resolves against). RecipientResolver.resolveRole/ .resolveTeam (the sole reader of this selector) actually read sys_member and sys_team_member — pinned by recipient-resolver.test.ts. The comment's own worked example (role:sales_manager) was not even a reachable sys_member.role value (closed vocabulary: owner/admin/delegated_admin/ member); sales_manager is sys_position's example name, a different object this selector does not read. Searched docs/adr/ and the cited #8461/#8556 threads for evidence the comment recorded RULED/intended semantics before touching it (per the card's stop-condition) — found none; both PRs are about the general per-organization index-scoping pattern for other objects, and ADR-0090 D3 independently confirms sys_member.role as the sole permitted "role" spelling platform-wide, consistent with the resolver. Comment-only: no schema field, index declaration, or behavior changed. The tenancy argument stays load-bearing (both sys_member and sys_team_member are tenant-scoped), and the example moved to the reachable role:admin. Fixes#9722 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx
Contributor
📓 Docs Drift CheckNothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 1 changed package(s)), so this run has no opinion about the docs. What this run could not see
Coarse fallback — 4 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): |
os-warren
marked this pull request as ready for review
August 19, 2026 08:16
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 19, 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 freeto 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.
Fixes#9722
What was wrong
packages/services/service-messaging/src/objects/notification-subscription.object.ts's(topic, principal)index comment justified the per-organization scope by claimingrole:x/team:xresolve againstsys_permission_set/sys_position(citing #8461and #8556). Those two PRs scope
sys_user_preference/sys_capability/sys_position'sown uniqueness per organization — the general index-scoping precedent this object's
index also follows — but say nothing about what a notification
role:/team:selectorresolves against.
RecipientResolver(recipient-resolver.ts, the sole reader of this selector)disagrees on both halves:
resolveRolereadssys_member(tenant-scoped viaorganization_id) andresolveTeamreadssys_team_member(tenant-scoped via itsteam_idlookup intosys_team) — pinned byrecipient-resolver.test.ts's"expands role: via sys_member (tenant-scoped) and de-dups"case. The comment's ownworked example (
role:sales_manager) is not even a reachablesys_member.rolevalue —that field's closed vocabulary is
owner/admin/delegated_admin/member(
BUILTIN_MEMBERSHIP_ROLE_OPTIONS);sales_managerissys_position's canonicalexample name, a different object this selector does not read at all.
Stop-condition check (before editing)
Per the card's hard stop-condition, searched for evidence the comment recorded a
RULED/intended semantics (role: → positions) rather than being simply wrong: grepped
docs/adr/forsys_permission_set/sys_position/notification-principal language andread #8461 and #8556 in full. Both are exclusively about scoping other objects' own
declared-index uniqueness per organization (the
unique: 'organization'pattern thisobject's own index already follows) — neither states or implies anything about what a
notification principal selector resolves against. ADR-0090 D3 independently confirms
sys_member.role(the org-administration tier) is the sole permitted "role" spellingplatform-wide — consistent with, not contradicting, what the resolver actually reads.
No ruling found → stop-condition cleared, proceeded with the comment fix.
What changed
Rewrote the flagged comment (
notification-subscription.object.ts:75-92) to namesys_member/sys_team_memberas the objectsRecipientResolveractually reads,kept the tenancy argument load-bearing (both objects are tenant-scoped, so
role:x/team:xstill denote different rows per organization — the same reason the(topic, principal)index has to beunique: 'organization'), and replaced theunreachable
role:sales_managerexample with the reachablerole:admin. Comment-only —no schema field, index declaration, or runtime behavior changed. Declared file surface
only;
recipient-resolver.tsand its test were read-only references, untouched.Gates (comment-only change; local scope per AGENTS.md; final HEAD
99521a51c)pnpm check:slot-lookup— PASS (107 unswept sites in 25 files, none new; baselinekey set unchanged vs
b030055)pnpm check:test-source-alias— PASS (72 packages with tests scanned)pnpm check:type-source-resolution(--self-test+ full) — PASS (76 packagesscanned)
node scripts/docs-audit/check-affected-docs.mjs— PASS (262 self-test cases)pnpm check:nul-bytes(--self-test+ full) — PASS (6298 text files scanned, no rawcontrol bytes)
node scripts/pm/dispatch-gates.mjs(no paths — self-derived from the actual diff, 1changed path) — same 4 named families, plus one convention-triggered addition:
pnpm check:i18n, because the package owns ani18n-extract.config.ts. Diff reviewconfirmed no
label/description/authorable string changed (comment-only), so theextractor had nothing to pick up — ran it to completion anyway (built the CLI per its
own PREREQUISITE-NOT-MET message): PASS, 9 packages, all bundles in sync, no
undeclared authoring keys.
pnpm --filter @objectstack/service-messaging test -- --maxWorkers=2— unchangedfrom
main, as expected for a comment-only diff: 242 passed / 22 files, 0failed.
All gates run against the committed tree at
99521a51c(working tree was clean beforeand after commit — no drift between the runs above and the final diff).
Changeset
skip-changesetapplied (this repo's real mechanism — confirmed against.github/workflows/pr-automation.yml'schangeset-checkjob and the repo-conditionalconvention
pm-dispatch/os-devPR #9100 settled: this repo uses the label, objectuiuses an empty-frontmatter changeset instead). Rationale: comment-only prose change, zero
user-visible or runtime behavior change, nothing to release.
Generated by Claude Code