Repository navigation
fix(service-messaging)!: mark-read writes a read receipt only for a notification delivered to that user - #22037
Conversation
…otification delivered to that user upsertReadReceipt inserts a read receipt only when the notification was delivered to the caller: a receipt keyed on them already exists (flipped in place, unchanged), or their inbox holds a message for it. Any other id writes no row, is not counted in readCount, and its event's organization is not read. The delivery check reads the caller's own inbox under INBOX_SYSTEM_CONTEXT, keyed on user_id. Claude-Session: https://claude.ai/code/session_01WMQprn46CND82KmY8sZWBu Co-authored-by: Claude <noreply@anthropic.com>
…ad-receipt-recipient-only
📓 Docs Drift CheckThis PR changes 1 package(s): ⛔ 1 release-owned page(s) name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 7000293be03d435b72c079849c2b6c0921414b89 && git checkout 7000293be03d435b72c079849c2b6c0921414b89
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 2301e17eaf470c2cdefe2eb1cd05b41ba350cc84 f2d085db2236498fa12419f85ed9103ba4a88002 && git checkout -B drift-repro 2301e17eaf470c2cdefe2eb1cd05b41ba350cc84 && git merge --no-ff f2d085db2236498fa12419f85ed9103ba4a88002
node scripts/docs-audit/affected-docs.mjs --json 2301e17eaf470c2cdefe2eb1cd05b41ba350cc84
|
…thods and the HTTP outbox's redeliver take the explicit system opt-in (objectstack-ai#22045) Part of objectstack-ai#21908 Clause-②: no Phase two, stage 2a of the principal-less hand-off closure (ADR-0096 E1 / D5), per the maintainer's ruling on the card (letter A). Seven more producers of a principal-less, non-system data context now take the explicit system opt-in (`isSystem: true`) inside their owning service. The deny itself lands last and is not in this PR, so objectstack-ai#21908 stays open. ⛔ No `plugin-security`, `packages/spec`, `metadata-protocol` or `docs/adr` file is in the diff, there is no new elevation API, and no door's authorization changed. ## What moved Positions are at the base `9a0401fdd3`. "Gate fired" means one of the six gates the security middleware still runs before its hand-off (package-managed, system-row, curated-capability, audience-anchor, ADR-0103 engine-owned, ADR-0090 D12 delegated-admin) fires on the producer's calls. Counts are principal-less calls at the hand-off, before and after the change; every "after" call arrived as an `isSystem` call. | Position | Function | Doors | Door authorizes (where) | Gate fired | Moved | Evidence (harness · dogfood subset) | | :-- | :-- | :-- | :-- | :-- | :-- | :-- | | `service-storage` `metadata-store.ts:373` | `StorageMetadataStore.getFile` | upload commit (`storage-routes.ts:453`), both download doors (`:799`, `:846`), and the chunked completion through `updateFile` | yes. Commit and chunked completion: session authentication (`requireUploadSession`, `:458`, `:684`) before the read. Downloads: `authorizeDownload` (`:808`, `:855`) after the read and before anything is disclosed, because its verdict reads the row | no | yes | 5 → 0 · 111 → 0 | | `metadata-store.ts:399` | `StorageMetadataStore.updateFile` | upload commit (`:472`), chunked completion (`:720`) | yes. Session authentication before the write; the session's organization scopes the write | no | yes | 2 → 0 · 46 → 0 | | `metadata-store.ts:434` | `StorageMetadataStore.deleteFile` | none. No production caller in this repository; the class is exported | not applicable, no door | no | yes | 1 → 0 (direct call) · not reached | | `metadata-store.ts:502` | `StorageMetadataStore.getSession` | chunk upload (`:593`), chunked completion (`:681`), progress (`:751`) | yes. Session authentication before the read; at the chunk door the resume-token check (`:617`) follows the read, because it reads the row's token, and precedes every write | no | yes | 8 → 0 · 2 → 0 | | `metadata-store.ts:529` | `StorageMetadataStore.updateSession` | chunk upload (`:658`, expiry via `:623`), chunked completion (`:705`, `:725`, expiry via `:694`, failure stamp via `:732`), progress (expiry via `:770`) | yes. Session authentication before every write; the chunk door also checks the resume token first | no | yes | 4 → 0 · 1 → 0 | | `metadata-store.ts:562` | `StorageMetadataStore.deleteSession` | none. No production caller in this repository | not applicable, no door | no | yes | 1 → 0 (direct call) · not reached | | `service-messaging` `sql-http-outbox.ts:468` | `SqlHttpOutbox.redeliver` (two reads, one reset write) | `POST /api/v1/webhooks/redeliver` (`plugin-webhooks` `webhook-outbox-plugin.ts:397`) through `MessagingService.redeliverHttp` | yes. An authenticated session or `401` before the call; the session's active organization is the tenant; the producer's veto runs before the write | no | yes | 3 → 0 · not reached | Read scope, asked for by the claim: `getFile` and `getSession` read by id with no organization scope before the move (no context at all, so no `tenantId` reached the driver, and the middleware handed the call through before any row or tenant filter) and after it (the middleware short-circuits before the same filters, and still no `tenantId` reaches the driver). ## What the opt-in would also have changed, and what holds it The opt-in skips more than the six gates. Measured on the real engine, per call: - **The update-side `readonly` strip.** `updateFile` and `updateSession` used to send the whole row read back, merged with the patch. Under the strip, the engine took `organization_id`, `created_at`, `updated_at`, `created_by` and `updated_by` out of that row on every call. Under the opt-in the strip does not run, so the full row, `organization_id` included and read without tenant scope, would have been written back. **Fix inside the store:** the two updates now send the caller's patch alone (`changedColumns`). Measured on SQLite through the real engine: on the base the strip took those five columns; on the change it takes none, and the stored rows are equal on every non-timestamp column, with `updated_at` still stamped by the platform. - **The tenant-audit fill-in.** For an `isSystem` write, `ObjectQL.buildDriverOptions` fills in `bypassTenantAudit: true` when the object is outside the platform tenancy inventory. `sys_file` and `sys_upload_session` are in it as tenant-scoped, so nothing changes for them (pinned). `sys_http_delivery` is not, so the opt-in would have silenced the audit for a redelivery from a caller with no organization, which is the one line `RedeliverOptions` keeps. **Fix inside the outbox:** the reset write states `bypassTenantAudit: false`. The engine never overwrites an explicit value, and the driver still audits. - **Unchanged, measured or read:** row and field security, the masker and the Layer 0 tenant wall are skipped by the hand-off and by the opt-in alike. Read auditing records neither (no user id). No lookup field exists on the three objects, so the referential-integrity skip does not apply. Redelivery's bulk data event publishes without an organization in both cases. ## Pins - **Store, `tenant-audit-update-delete-half-repairs.test.ts`.** - Every by-id read carries `{ context: { isSystem: true } }` and no tenant. - Every by-id write carries the opt-in beside the door's tenant, or the opt-in alone with no tenant invented. The route-level pins are updated to match. - ⛔ An update payload never carries the provisioned columns of the row read back. - The opt-in leaves these writes' tenant audit armed. - ⛔ Negative, through the real `ObjectQL` over a real `SqlDriver` on SQLite in the isolated posture: under the opt-in, a door tenant's `updateFile`, `updateSession`, `deleteFile` and `deleteSession` on a row stamped for another organization are refused (`StorageMetadataStoreError` wrapping `RECORD_NOT_FOUND`) and leave the row untouched. The still-works half shows the caller's own row and an organization-less row are reached. - **Outbox, `system-context.pin.test.ts`.** Both reads and the reset write carry the opt-in. The caller's `tenantId` stays on every bag. The reset states `bypassTenantAudit: false`, and a caller with no tenant gets none invented. - **Outbox, `delivery-update-tenant-audit.integration.test.ts`.** - ⛔ Negative, on the real driver: a foreign row stays `RESOURCE_NOT_FOUND` with zero writes under the opt-in, and the caller's own row resets. - The two existing audit assertions now read `false`, the value the driver receives, where they read "absent". - **The stage-1 pins stay green.** These are objectstack-ai#22025's insert pins and objectstack-ai#22037's inbox pins, inside the full package suites. **Ablations**, run at `0f6df0f306`. Each mutation went through `scripts/ablation-replace.mjs`: the anchor hit, the blob changed, and the restore was proved by blob equal to `HEAD` and an empty `git diff HEAD`. The pins import from `src`, so no `dist` leg applies. | # | Mutation | Result | | :-- | :-- | :-- | | A1 | by-id writes keep the opt-in but drop the door tenant | 13 of 28 red; the negative pins read "promise resolved … instead of rejecting" (the foreign row was reached) | | A3 | `updateFile` sends the merged row again | 1 of 28 red; the payload carried `organization_id` and `created_by` | | A4 | `redeliver`'s deciding read drops `tenantId` | 5 of 30 red; the negative pin got `DELIVERY_NOT_ELIGIBLE` where it expects `RESOURCE_NOT_FOUND` | | A5 | the reset write's `bypassTenantAudit: false` removed | 4 of 21 red; the driver received `true` ("expected true to be false"), and the tenant-less redelivery went silent | ## Measurement The measurement used a local instrument in `plugin-security`'s middleware, never committed. Its instrument file was reverted, and blob `5b4ab28045af` equals `HEAD`. `plugin-security` was rebuilt, and `ablation-dist-preflight --absent` passes; its positive control found the marker in 2 built files while it was live. For each call from the seven producers, the instrument recorded the producer frame, whether the call was principal-less, and a dry run of the six gates with the system flag cleared. Two runs used it: - A scratch harness, deleted and not in the diff. It drove every door on a booted stack: upload commit, both downloads, the chunk upload, progress, chunked completion, progress on an expired session, and `POST /api/v1/webhooks/redeliver`. It called `deleteFile` and `deleteSession` directly. - The 11-file dogfood subset named below. Every door answered the same on the base and on the change (200, or 302 for the redirect). There were 0 gate firings before and after. ## Acceptance notes - **Premise correction.** `updateFile` and `updateSession` already accepted the optional organization context (`metadata-store.ts:399`, `:529`), as `deleteFile` and `deleteSession` did. Only `getFile` and `getSession` take none, and they still take none: the reads carry the opt-in without a tenant. No public signature changed. - **Door ordering, reported for the seat.** At both download doors the read precedes `authorizeDownload`. At the chunk door the read precedes the resume-token check. Each check reads the row it needs, and each precedes any disclosure and any write. The read's reach is the same before and after (unscoped by principal and by organization). - **Doors without a door-level binding.** The chunked completion and progress doors authorize by session authentication and check no resume token. The commit door checks no ownership of the file id. The move leaves all three unchanged, and owner checks are the question the ruling sent to its own card (option C). Reported to the seat at class level. - **`deleteFile` and `deleteSession` have no caller in this repository.** They were moved as the ruling wires all seven. Their door check is vacuous. - **Files beyond the two named files, all in the same packages.** - `outbox-dispatcher-scope.ts` declares `REDELIVER_SYSTEM_CONTEXT` (module-internal) with its own warrant, and amends the two docblocks that kept the outbox opt-ins off `redeliver`. - `messaging-service.ts` gets one sentence made true. - `scripts/engine-double-contract.pinned.json` counts the storage test file's second double, which `--write` grew. - `plugin-audit`'s `audit-writers.test.ts` fixture prose still describes `updateSession` writing the merged full record. That is test residue in another package and is left alone. ## Verification (head `2665532dc9`) Each command ran in the foreground of this worktree, and its exit code was captured before any pipe. - **Gates.** Running `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` at `2665532dc9`, with no paths, derived 73 commands. The dispatch-time 51 are a subset. All 73 ran and all exited 0. The reconciliation (`--ran`) printed: "73 derived famil(ies) accounted for — 73 run, 0 NOT-MEASURED (a DERIVED zero — all 73 recorded an exit code and none of them is 3)". - On an earlier pass at `6f366adcb1`, `check:engine-double-contract` exited 1. It had counted the new recording wrapper as a fake engine. The wrapper now routes each verb through the engine's dispatch predicates, and `--write` grew the ledger. - `check:dual-build-cjs-loads` exited 3 (PREREQUISITE NOT MET: 8 packages outside the dogfood closure had no `dist/`). They were built, and it then exited 0. - **Suites at `2665532dc9`.** `@objectstack/service-storage`: 42 files, 659 tests passed. `@objectstack/service-messaging`: 48 files, 534 passed. `typecheck` passed for both, the storage test layer's `check:test-typecheck` included. `--listFiles` confirms both programs compile the edited test files. `@objectstack/plugin-webhooks`, which mounts the redeliver door, passed at `0f6df0f306`: 15 files, 165 tests. - **Dogfood.** The 11 files below passed, 100 passed and 1 skipped, on both the base and the change. They ran against the change's built `dist/`; the source is unchanged since `230ed9ea15`. The files: `attachments-permission-matrix`, `attachments-public-read-acl`, `attachments-unscoped-delete-gate`, `field-file-collection`, `file-field-constraint-refusal`, `sys-file-metadata-write-refusal`, `predicate-write-unreadable-not-matched`, `write-door-unreadable-is-not-found`, `storage-growth`, `temporal-storage-e2e`, `webhook-materialization`. None of these files reaches the redeliver door; the scratch harness covered it. - **Lint, a proven narrowing; CI runs the full `pnpm lint`.** The population was read from `eslint.config.mjs`: the `**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}` block plus the `packages/**` blocks. `eslint --no-inline-config --format json` over the 7 changed `.ts` files reported 7 files, 0 errors and 0 warnings at `c4f10218af`. The config enables no type-aware linting (no `parserOptions.project` and no typed rules), so no untouched file's verdict can move. ## Changeset `.changeset/21908-by-id-producers-opt-in.md` grades `patch` for `@objectstack/service-storage` and `@objectstack/service-messaging`. It names the opt-in, the patch-only update payload and the held tenant audit. No exported type or entry symbol changed: the new constants and the store helper are module-internal. --- _Generated by [Claude Code](https://claude.ai/code/session_01WMQprn46CND82KmY8sZWBu)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
Fixes #22026
Clause-②: no (narrowing)
A read receipt belongs to a recipient: ADR-0030 keys
sys_notification_receiptby recipient.MessagingService.markReadis the method behind the notifications mark-read door and behindmarkReadAsCaller. It used to insert areadreceipt for any notification id it was handed, stamped with that notification's organization, and never checked that the caller was a recipient. It now inserts one only for a notification that was delivered to that user. This follows triage's direction (6022270298) and the seat verdict on the measure-first round (6023979476, option A). Rows already written are not cleaned up; the verdict ruled that out.What changed
Positions are at base
b88c356413, inpackages/services/service-messaging/src/messaging-service.ts.upsertReadReceipt(near:888) keeps its first step: it flips a receipt keyed(notification_id, user_id, channel 'inbox')toreadin place. That read is also the receipt half of the delivery check. The receipt key is thedeliveredreceipt's key, so when the read finds nothing, there is no delivered receipt either.inboxMessageNameschecks for the inbox half. It reads the caller's ownsys_inbox_messagefor that notification:where: { user_id, notification_id },fields: ['id'], underINBOX_SYSTEM_CONTEXT. An inbox message is enough on its own, because the inbox channel'sdeliveredreceipt is best-effort:writeDeliveredReceiptlogs a failure and moves on, and the inbox row stays.upsertReadReceiptreturns 0. No row is written,readCountdoes not count the id, andnotificationOrganizationis not called. That organization read now happens only on the delivered path.markRead's existing per-id catch. It is logged at warn, the id counts 0, and no row is written. A check should fail in that direction.markAllReadis unchanged. Its ids come fromunreadNotificationIds, which reads the user's own inbox rows that carry anotification_id. So every id it passes already has an inbox message for that user. A pin below covers it.markReaddocblock now says an undelivered id writes nothing. Theinbox-system-context.tsdocblock lists the new read among the calls that context carries; that edit is comment-only, in the same package.{ success, readCount }. There is no spec, permission-set orplugin-securitychange, and nothing changes who may read what.Reachability, measured before the build
Triage asked for this measurement first. It was taken on base
b88c356413with a local engine-level probe: the realSecurityPlugin,ObjectQLandSqlDriver, the shipped permission sets, and theisolated,groupandsinglepostures. The probe was deleted and never committed. Classes and roles only:_selfpolicy binds every authenticated principal on this object, and the object grants no superuser bypass.Report:
6023938722. Seat verdict:6023979476.Pins and ablation
The pins are in
messaging-service.test.ts, block[#22026]. They use the file's existing statefulinboxEnginedouble, observed through spies, so no new double was added.{ success: true, readCount: 0 }. The receipt table stays empty, nothing is inserted, the event's organization is not read, and the recipient's state does not move.markReadAsCaller.user_id, run with{ isSystem: true }, and projectingidalone.notification_id: a call naming its row id writes nothing, and the row still lists as unread.markAllRead: only delivered ids are swept, by construction. The test counts 2 and finds receipts for that user only.Fixture triage: in
read-receipt-organization.test.ts(pins E and E2), the reader had no delivery evidence. The fixture now gives them their inbox message, which is exactly the case the insert path exists for. The assertions are unchanged.Ablation. Run on the committed state
7eb0b82008withscripts/ablation-replace.mjsin wrap mode: delete the delivery-check statement, run the block, restore.448159c99ceetoac84bbb73a43.expected { success: true, readCount: 1 } to deeply equal { success: true, readCount: +0 }.markAllRead, which do not depend on the check.448159c99ceeandgit diff HEADwas empty.#22025's pins (
system-context.pin.test.ts) are green and unchanged.Tests
Head
f2d085db22unless a line says otherwise. That commit mergesorigin/main2301e17eaf, a CI-script commit that touches none of these files.pnpm --filter '@objectstack/service-messaging^...' buildexited 0.pnpm --filter @objectstack/service-messaging buildexited 0, withcheck-dts-emitted: 2/2.pnpm --filter @objectstack/service-messaging test: 48 files and 531 tests passed.typecheckexited 0, andtsc --listFilesshows its program includes all 48 test files, the two edited ones among them.node scripts/pm/dispatch-gates.mjs --commands, run on this change with no paths, derives 65 commands: the dispatch's 51 plus 14 from the changeset and test-file families, none dropped. All 65 exit 0. The--ranreconciliation over the recorded exit codes reads "65 derived, 65 run, 0 NOT-MEASURED, 0 UNRUN".7eb0b82008,check:dual-build-cjs-loadsandcheck:i18nexited 3 (PREREQUISITE NOT MET: no build output). After a turbo build of the workspace (72 tasks, 71 cached), both exited 0, and they exit 0 at the merge head too.check-nul-bytes: OK, no raw ASCII control bytes.check-adr-0087-registrationprinted its no-migration-prescription exemption notice.check-empty-changeset: "No empty-frontmatter changeset introduced by this diff".check-system-context-census: OK.check-changeset-no-majorreads its level axis from the PR payload, so that half is answered in CI.7eb0b82008(the merge touched no package). These suites reach service-messaging throughdist/, which was checked to contain the change:approval-notification-body.dogfood.test.ts,schedule-acting-organization.dogfood.test.ts,schedule-sweep-organization-scope.dogfood.test.tsandplatform-app-object-entry-views.test.ts. 4 files and 84 tests passed.7eb0b82008.notifications.hono.integration.test.tsdelivers through the real pipeline, marks one notification read (readCount: 1), then marks all read. Together withnotification-schema-conformance.test.tsanddomains/notifications-query-validation.test.ts: 3 files and 54 tests passed.Acceptance notes
plugin-security, carrier: none. Indefault-permission-sets.ts, the comment abovesys_inbox_message_selfsays neither inbox object declaresorganization_id, so Layer 0 is inert on them. In fact the registry injectsorganization_idintosys_notification_receipt(it is engine-owned, not better-auth), and the tenant wall and the driver's native tenant scope do act on it, only ever narrowing. This is comment drift, not a behaviour defect. That file is not edited here.notification_id. notification: 没有notification_id的sys_inbox_message行永远无法被标记已读 —— 读态的键在事件 id 上 #6448, which asked about such rows, ended in the not-planned state.listInboxlooks up read state only through a row'snotification_id, so a row without one always lists as unread. Nothing regresses for those rows.minorwith a BREAKING banner (an accept-set narrowing, under the launch-window convention) and carries an ADR-0087not-required (no-migration-prescription)disposition.Generated by Claude Code