Skip to content

feat(messaging): scope the plugin-facing inbox writes to the authenticated caller - #11450

Merged
os-sam merged 2 commits into
mainfrom
claude/issue-10753-messaging-markread-authenticated-scope
Aug 23, 2026
Merged

feat(messaging): scope the plugin-facing inbox writes to the authenticated caller#11450
os-sam merged 2 commits into
mainfrom
claude/issue-10753-messaging-markread-authenticated-scope

Conversation

@os-sam

Copy link
Copy Markdown
Collaborator

Part of #10753

⚠️Deliberately Part of, not a closing keyword. The dispatch asked for the closing form; this PR uses Part of instead — flagged here rather than silently switched. The ruling covers two verbs and this PR lands one of them: recall() hits the ruling's own fork clause (last section), so merging this must not close the card while that half is undecided. #10753 stays open. (The keyword is kept away from every issue number on purpose — GitHub's parser ignores any negation in front of it.)

What this is

MessagingService gains a plugin-facing inbox write door scoped to the authenticated caller:

markReadAsCaller(caller: ExecutionContext|undefined,ids: readonlystring[])markAllReadAsCaller(caller: ExecutionContext|undefined)

Ruled 2026-08-23 (5386675655), maintainer, verbatim and untranslated: 「10950 不考虑存量,其他接受你的建议」 — recorded as Q1 = A1 · Q2 = (a) hard recall · Q3 = implementation-first, with two constraints the same ruling attaches:

the implementation must scope by AUTHENTICATED usermarkRead's userId is a plain parameter, not auth context.
⛔ adding a provenance column to an engine-owned platform object is NOT approved by this ruling.

Everything here lands in service-messaging (Q3, implementation-first). No packages/spec edit, no new column, no object declaration moved.

The measured BEFORE — where this card's severity actually lives

markRead(userId, ids) is the REST door's contract method (INotificationService.markRead?), and on that path the parameter is trustworthy: runtime/src/domains/notifications.ts:198 binds it to context.executionContext.userId after answering 401 for a request that has none.

The same service is also a kernel service — ctx.registerService('messaging', service), messaging-service-plugin.ts:162 — and the kernel hands every plugin one shared PluginContext (core/src/kernel.ts:65,:91) whose getService carries no caller identity. For an in-process caller that first parameter is therefore a free string:

Today any plugin can mark any user's inbox messages read — unconstrained and undeclared. And the receipt write lands context-lessly on sys_notification_receipt, which is managedBy: 'engine-owned' with apiMethods: ['get','list'] (ADR-0103), so no engine permission check sees it either.

So this PR is both an API widening (new plugin-facing methods) and the first tightening of in-process power on this path. That fact was not in front of the maintainer when Option A was first framed.

Why the new door is a tightening and not just a convenience

The pair takes no target user at all. The recipient is derived from caller.userId, so "mark someone else's inbox read" has no spelling on this surface — unrepresentable rather than discouraged. It fits the case the card was filed for exactly: the approver who clears a request is the recipient whose badge is stuck, and the close-out hook runs inside that approver's own request.

userId is read, and nothing that merely resembles one:

FieldVerdictWhy
userIdthe recipientthe subject the engine authorizes AS
attributedUserIdrefusedattribution only; its own contract states "nothing in the authorization path reads this", and a context carrying only it authorizes as anonymous (ADR-0118 D2). Promoting it opens the second adjudication track ADR-0095 D3 closed
actorrefuseda service-principal label (svc:<name>), not a sys_user id — svc:flow has no inbox
isSystem: true, no userrefusedthe system has no inbox to be the recipient of. A sweep that must name a target still has markRead(userId, ids)

A tolerant userId ?? attributedUserId ?? actor chain is exactly the consumer-side widening contract-first exists to refuse: it reads as working and silently clears the wrong person's badge.

Refusals throw InboxCallerError carrying the ADR-0112 envelope pair a boundary reads — status: 401 + the registered code: 'UNAUTHENTICATED' — so an HTTP surface answers 401 rather than the 500 a bare Error demotes to. UNAUTHENTICATED and not PERMISSION_DENIEDis the axis: there is no second identity to disagree with, so there is no forbidden-target case.

Order is pinned, not left to reading: the refusal is evaluated before the empty-ids and no-data-engine short-circuits, both of which return { success: true, readCount: 0 }. Reaching one of those with no authenticated caller would report success for a write that was never authorized — the silent-success shape this door exists to replace.

Honest about what it is NOT

A discipline boundary, not a security boundary, and that is stated in the module header rather than left to be discovered. An in-process plugin already holds the data engine and can write sys_notification_receipt directly; nothing at this layer stops trusted code that means to. And markRead(userId, ids) stays reachable — it is the published INotificationService contract the REST door needs, and this lane has zero ownership of packages/spec. What changes is that the correct pattern is the only one the plugin-facing surface expresses, and the wrong one now fails loudly at the call site. The INotificationService declaration of the new pair is the follow-up inside the contract-review chain.

Verification

Everything below was run against the final commit 1ce71df0e5 — the same tree the gate union was derived from, with a clean working tree. Union derived by node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack with no hand-supplied paths (the script takes its own change set from the merge base, three-dot; the run confirmed 5 committed paths / 0 working-tree / 0 untracked).

  • 19 of the 19 path-matched + convention-triggered gates green, each read from its own verdict line with the exit code captured before any pipe. check:i18n first answered PREREQUISITE NOT MET — the workspace CLI is not built, which is not measured, never a pass; the CLI was built and it re-ran to its own verdict: check-i18n-bundles: OK (9 package(s) — all bundles in sync, no undeclared authoring keys).
  • pnpm --filter @objectstack/service-messaging typecheck (tsc --noEmit) — clean.
  • pnpm --filter @objectstack/service-messaging test267 passed, 26 files, including 8 new cases pinning the door.

One declared narrowing:check:type-check-debt --re-measure was not run — it refuses without the whole workspace closure built, and a repo-wide scan is CI's run. It is named here only by the kind "adds or edits a test file", and the three things that make this a measurement rather than a gap: (1) its population is its own ledger, whose single service-messaging entry is packages/services/service-messaging/scripts (the i18n-extract.config.ts); (2) none of this PR's 5 changed paths is under it; (3) the appended test code is inside the package's own tsc program — tsconfig.json is include: ["src"] with no test exclusion — and that program is green above. Its structural half, check:type-check-coverage, ran and passed.

Ablation — the prediction was written down before the run

Predicted, in writing, before mutating anything: replacing the strict derivation with the forbidden fallback chain (caller?.userId ?? caller?.attributedUserId) turns the suite RED on exactly one test, named in advance — "refuses a context carrying only attributedUserId — attribution never becomes authorization" — with the absent / {} / blank / actor / isSystem refusals staying green, because the ablated chain never reaches those fields.

Observed:Tests 1 failed | 266 passed (267), and the one failure is that test, by name. Direction RED as predicted.

  • On-disk confirmation in both directions with grep -F, plus a reverse control taken before the mutation: injected literal 0 → 1, deleted literal 1 → 0.
  • Restore verified byte-identical by git hash-object: 59883355018fe7b5fcbad947a3edaa0efb78786c before and after. trap … EXIT INT TERM carried the restore.
  • No rebuild needed, and proven rather than assumed: the suite imports ./inbox-caller.js relatively, so vitest resolves src. A dist-resolved subject would have stayed green — and a green under ablation would have been not measured, never a pass.

Documentation

Checked by reading the prose, not by trusting docs-drift's green (which cannot see prose or table claims). Every page mentioning markRead / sys_inbox_message / sys_notification_receipt / service-messaging was read: content/docs/api/client-sdk.mdx (client SDK, REST path), content/docs/kernel/services-checklist.mdx:395 (the three REST routes — all three keep their signatures), content/docs/concepts/north-star.mdx:124, and the package roster in content/docs/plugins/packages.mdx:240. None enumerates who may mark read, and none is falsified by this change, so there is nothing to repair in-PR. The package ships no README.

⚠️recall() — the ruling's fork clause is triggered. NOT implemented here.

Q2 ruled (a) hard recall: delete the notification's sys_inbox_message rows and cancel its pending/unclaimed inbox deliveries. Writing that contract before its implementation — as instructed — is what surfaced that it cannot be built under this ruling's constraints. Two independent blockers, both measured:

1. Authorization has no expressible predicate.recall is an emitter-side verb — "retract what I sent, from everyone's inbox" — and emitter-scoped and authenticated-scoped are different axes. No emitter provenance exists on any row (sys_notification carries topic/payload/severity/dedup_key/source_object/source_id/actor_id/created_at; actor_id is the user who caused the event, not the emitting plugin; sys_inbox_message has none; EmitInput has no such field; the shared PluginContext mints no caller identity). On the authenticated axis the only expressible predicate is "the caller is a recipient" — which yields a recipient-scoped retract, not a hard recall across all recipients. Shipping that under the name recall would be the weaker approximation the dispatch forbids, so it is not shipped.

2. The mechanism has no safe expression either — and this one is independent of the column.INotificationOutbox declares exactly enqueue | claim | ack | list | claimDigest (outbox.ts:115-129). There is no cancel. Worse than absent: ack(id, { success: false, suppressed: true })would appear to work — neither MemoryNotificationOutbox.ack nor SqlNotificationOutbox.ack checks that the row is claimed, so both would flip a pending row to suppressed. It races the dispatcher (the row can be claimed between the list and the ack, sent, and acked back to success) and it increments attempts on a delivery that was never attempted. A real cancellation needs INotificationOutbox widened plus both implementations — a separate surface decision.

Reported, not invented, and not approximated. This is the PM seat's call, not a re-ruling request.


Generated by Claude Code

…ng inbox writes
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4
@github-actionsgithub-actionsBot added size/m documentation Improvements or additions to documentation tests tooling labels Aug 23, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-messaging, touching 11 documentable anchor(s).

30 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: node scripts/docs-audit/affected-docs.mjs --json 14cfc007a72f334892d7a68968dfa40cd1ed0bfe.

5 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/services/service-messaging/src/index.ts) — pages documenting those are invisible to this run
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 45 of 222 client-bound route-ledger rows — the other 177 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: node scripts/docs-audit/affected-docs.mjs --bridge-coverage

Coarse fallback — 4 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 14cfc007a72f334892d7a68968dfa40cd1ed0bfepackageMentionDocs.

Which tree this was computed on

This run read content/docs from aba6e35027edd1ab459e853045a14e28f4d7b228 — the merge of head 1ce71df0e5d7783bb4d7b34ff9eb4e904c22c9f6 into base 14cfc007a72f334892d7a68968dfa40cd1ed0bfe, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin aba6e35027edd1ab459e853045a14e28f4d7b228 && git checkout aba6e35027edd1ab459e853045a14e28f4d7b228
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 14cfc007a72f334892d7a68968dfa40cd1ed0bfe 1ce71df0e5d7783bb4d7b34ff9eb4e904c22c9f6 && git checkout -B drift-repro 14cfc007a72f334892d7a68968dfa40cd1ed0bfe && git merge --no-ff 1ce71df0e5d7783bb4d7b34ff9eb4e904c22c9f6
node scripts/docs-audit/affected-docs.mjs --json 14cfc007a72f334892d7a68968dfa40cd1ed0bfe

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 14cfc007a72f334892d7a68968dfa40cd1ed0bfe → pass the list as
args.docs, on the commit named under Which tree this was computed on.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@os-sam@claude