Skip to content

fix(service-messaging,plugin-webhooks): classify the delivery outboxes' update-op tenant-audit surface — ack is a dispatcher sweep, redeliver threads the caller's tenant - #11010

Merged
os-warren merged 4 commits into
mainfrom
claude/issue-10740-tenant-audit-update-half
Aug 22, 2026
Merged

fix(service-messaging,plugin-webhooks): classify the delivery outboxes' update-op tenant-audit surface — ack is a dispatcher sweep, redeliver threads the caller's tenant#11010
os-warren merged 4 commits into
mainfrom
claude/issue-10740-tenant-audit-update-half

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

Fixes#10740

Three single-record (multi: false) writes on sys_http_delivery / sys_notification_delivery are audited under the driver's update op — a different op, and a different throttle key, from the updateMany half. Their correct classifications are opposite, and treating them as one sweep is the dangerous reading this PR exists to avoid.

sitereachable fromclassificationhow it is discharged
SqlNotificationOutbox.ackdispatcher tick onlyglobal sweepdispatcherAckOptions(id)
SqlHttpOutbox.ackdispatcher tick onlyglobal sweepdispatcherAckOptions(id)
SqlHttpOutbox.redeliverPOST /api/v1/webhooks/redeliverrequest-contextualthreads the caller's tenant; no bypass anywhere on the path

The two ack sites — warrant re-derived, not inherited

Each limb was re-checked against this tree rather than carried over:

  1. No request context exists to thread.ack has exactly two callers — dispatcher.ts:210,240,249,286 and http-dispatcher.ts:183,193 — all inside runPartition(), which runs off a setInterval tick holding the notify.dispatcher.partition.N / http.dispatcher.partition.N cluster lock.
  2. The contract carries no tenant even in principle.ClaimOptions / HttpClaimOptions are {nodeId, limit, partition, claimTtlMs, now}.
  3. Partitioning is not an org key.hashPartition is 32-bit FNV-1a over refId | notificationId | digestKey, mod count — load-spreading, so a partition holds every organization's rows by construction.
  4. One outbox per environment drains the whole queue (messaging-service-plugin.ts:282,310), so a per-org predicate would strand every other organization's deliveries.

The helper is a new sibling, not a reuse: dispatcherSweepOptions returns & { multi: true }, so these sites cannot borrow it — deliberately, and the file now says so instead of asserting that ack and redeliver share a classification.

The helper's docstring also names the tempting wrong answer, because it is available here and it is worse than the flag: passing the claimed row's ownorganization_id as the tenant is a predicate read off the row it is about to write. It matches exactly that row, excludes nothing, adds no isolation — and silences the audit, leaving the next reader looking at a write that appears scoped. The audit asks whether the caller's tenant reached the write; on a dispatcher tick the honest answer is that there is no caller tenant.

redeliver — the site the audit was built for

The route in front of it authenticates and nothing more ("every authenticated user counts"), and sys_http_delivery is tenant-scoped. It now carries the caller's tenant, applied to the rows it reads as well as the row it writes, and the webhook route resolves the session's activeOrganizationId to supply it.

No bypassTenantAudit is anywhere on this path. A scoped write and a bypassed write produce the same silence in the log, so the flag would have converted a detectable hole into an undetectable one — which is why the tests below do not assert the audit line alone.

Scoping the reads is what makes the refusal fail-closed and quiet: a row in another organization is RESOURCE_NOT_FOUND (HTTP 404), so the endpoint neither replays it nor confirms it exists.

The contract change (why this stays draft, needs:contract-review)

redeliver(id, guard?)redeliver(id, { tenantId, guard? }), and redeliverHttp(id)redeliverHttp(id, { tenantId }). tenantId is a required property typed string | undefined: omitting it does not compile, and a genuinely tenant-less caller has to write tenantId: undefined and mean it. An optional property would have let the dangerous case — a request path that simply forgot — type-check in silence, which is the shape this change exists to remove. It worked as intended on landing: the first typecheck failed at 10 call sites, each of which had to state an answer.

Passing undefined leaves the write unscoped and the audit line still fires. That is deliberate reporting behaviour for a deployment that cannot resolve an organization for the caller, and it is pinned in both packages.

Zero packages/spec ownership. None was needed: tenantId is already a declared engine passthrough key (ENGINE_DRIVER_PASSTHROUGH_KEYS, legal for find/findOne/update/delete), so the tenant reaches DriverOptions through the existing contract.

Tests

packages/services/service-messaging/src/delivery-update-tenant-audit.integration.test.ts (6) — real SqlDriver on better-sqlite3, real syncSchemas(), OS_TENANCY_POSTURE=isolated read live, production outboxes and dispatchers.

  • A real delivery is driven through for both ack sites — a real HttpDispatcher / NotificationDispatcher tick, then a pin on status and attempts. ack only runs once a delivery is actually processed, so an audit line absent because nothing ran is NOT MEASURED, not a pass.
  • The scoped/bypassed distinction is drawn where it is real — a spy on SqlDriver.update (the method that both calls auditMissingTenant and applies applyTenantScope) records the options that actually reached the driver: tenantId: 'org_a' present, bypassTenantAudit absent. An assertion that only checked for the missing audit line would pass on the forbidden implementation.
  • Positive control on every silence — an unscoped by-id update on the same object through the same driver, run last (the gate throttles one warning per ${object}:${op}), which must produce the line.
  • Still-works leg — an in-tenant redeliver succeeds while a foreign one is refused, in the same test, so an implementation that refused everything would go red.

packages/plugins/plugin-webhooks/src/webhook-redeliver-tenant-scope.test.ts (4) — the half the service test cannot reach: that the tenant comes from the request, and the ADR-0112 codeandstatus (RESOURCE_NOT_FOUND + 404) on a cross-tenant refusal. code alone would pass on a refusal surfacing as a 500.

Ablation — prediction written before mutating

Dropped the threaded tenant from redeliver's write leg only, leaving the reads scoped.

predictedobserved
tenantId assertion RED, undefined vs 'org_a'RED, AssertionError: expected undefined to be 'org_a'
audit line appears (a diagnostic is added, not removed)not observable in this run — vitest aborts the test at the first failing assertion, which is the one above. Confirmed instead by the always-green tenant-less caller is NOT silenced test, which pins the same direction
cross-tenant refusal stays GREEN (it comes from the read leg)GREEN
still-works stays GREENGREEN
both ack tests stay GREENGREEN

Restore proved byte-identical: git hash-object reads 41c5a0b96e6d06a39f6f3eb59f3f896039a2d903 before the mutation and after the restore; the restore leg re-runs 6/6 green.

src vs dist, argued from the files

The subjects (SqlHttpOutbox, SqlNotificationOutbox, both dispatchers) are imported by relative specifier (./sql-http-outbox.js) from inside the same src/ directory — a relative import cannot leave src/, and only bare package specifiers reach a dist/ through exports. packages/services/service-messaging/distdid not exist at all when the ablation ran. The package's only vitest alias is @objectstack/core → source, which touches none of these. Empirically: the mutation flipped the verdict with no rebuild. What does resolve through dist/ is @objectstack/objectql and @objectstack/driver-sql, which is why the dependency closure was built first.

Gates

Union derived on the final commit 1091f1e60, clean tree, node scripts/pm/dispatch-gates.mjs with no path arguments (16 paths, identical set on re-derivation). Exit codes captured before any pipe. Every gate below reported its own verdict line green.

check:changeset-gate-self-tests · check:objectui-changeset · check:route-envelope · check:slot-lookup · check:test-source-alias · check:type-source-resolution · check-adr-0087-registration · check-changeset-no-major · check-ci-filter-parity · check-empty-changeset · check-plugin-teardown-shape · check-affected-docs · check:query-options-erasure · check:type-check-coverage · check:type-check-debt · check:engine-double-contract · check:where-matcher · check:i18n

Plus two the derivation did not name, run on judgment (class #10309 is live): check:nul-bytes (any edit) and check:tenant-chokepoint (topic-adjacent — 20 getBuilder() bindings across 3 files, every read builder routing through applyTenantScope()).

Two gates needed repair before they measured anything, both recorded rather than smoothed over:

  • check-adr-0087-registration failed twice. First because the marker spelled the symbol half as a member path (IHttpOutbox.redeliver) rather than the bare exported identifier parseSymbolRef requires; then because it reads changesets from git, not the working tree, so the corrected marker was invisible until committed. It then refused MessagingService as unresolvablepackages/spec/src/api/protocol.zod.ts mentions the class name in a prose comment without declaring or importing it. The marker now names only the two symbols the gate can verify, and states the MessagingService.redeliverHttp claim in prose where a reviewer reads it rather than where a checker would appear to have verified it.
  • check:i18n first returned PREREQUISITE NOT MET — the workspace CLI is not built … Nothing was checked — NOT MEASURED, not a pass. Built the CLI as the gate's own text prescribes and re-ran it: 9 packages, all bundles in sync.

Package suites: service-messaging 25 files / 254 tests, plugin-webhooks 11 files / 128 tests, both fully green, both typechecks clean.

Found on the way, not fixed here

#11009 — a compare-and-set where on a by-id update is silently inert: SqlDriver.update never applies options.where, so the status: { $in: [...] } guard in redeliver's own write does nothing. Measured (a pending row's attempts went 7 → 0 under a predicate demanding a terminal status). It pre-dates this card, is a different defect class, and its fix is a contract decision on ObjectQL.update — so it is filed unassigned and left untouched here. #11009 is not addressed by this PR.

Not in scope, deliberately: cloud#1512's crm_contract half.


Generated by Claude Code

…-audit surface — ack is a dispatcher sweep, redeliver threads the caller's tenant
Three single-record (multi:false) writes on sys_http_delivery /
sys_notification_delivery are audited under the `update` op, and their
classifications are opposite:
- SqlNotificationOutbox.ack / SqlHttpOutbox.ack are reachable only from the
dispatchers' setInterval tick under a cluster lock. Declared global sweeps
via the new dispatcherAckOptions() helper, whose warrant is re-derived from
this tree rather than inherited from the updateMany half.
- SqlHttpOutbox.redeliver is served to any authenticated user through
POST /api/v1/webhooks/redeliver. It now carries the caller's tenant, to the
rows it reads as well as the row it writes, and never bypassTenantAudit.
IHttpOutbox.redeliver(id, options) takes a required-but-nullable tenantId so a
caller cannot omit the decision, and the webhook route threads the session's
active organization into it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx
…d the ADR-0112 code+status on a cross-tenant refusal
Adds webhook-redeliver-tenant-scope.test.ts, the half the service-level test
cannot reach: that the tenant comes FROM THE REQUEST (the session's
activeOrganizationId) and that the cross-tenant refusal surfaces as
RESOURCE_NOT_FOUND with HTTP 404 rather than a 500.
Also carries the changeset and the last redeliverHttp call site.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx
…path>#<Symbol>
The marker named `IHttpOutbox.redeliver` / `MessagingService.redeliverHttp`,
member paths the gate's parseSymbolRef refuses by design: the symbol half must
be a bare identifier it can find as an exported type declaration. Names the
three declarations instead and says which members moved.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx
check-adr-0087-registration refuses `MessagingService` as unresolvable — a
prose comment in packages/spec/src/api/protocol.zod.ts mentions the class name
without declaring or importing it, so the gate cannot rule it unrelated. The
claim moves into the marker's prose, where a reviewer reads it rather than a
checker appearing to have verified it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/plugin-webhooks, @objectstack/service-messaging, touching 24 documentable anchor(s).

10 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/api/client-sdk.mdx(via getAudit (sdk), meta.getAudit (sdk), meta.publishItem (sdk), meta.rollbackItem (sdk), publishItem (sdk), rollbackItem (sdk))
  • content/docs/automation/webhooks.mdx(via /api/v1/webhooks/redeliver (route))
  • content/docs/kernel/cluster.mdx(via tenantId (symbol))
  • content/docs/kernel/contracts/auth-service.mdx(via tenantId (symbol))
  • content/docs/kernel/contracts/metadata-service.mdx(via tenantId (symbol), /:type/:name/publish (route), /:type/:name/rollback (route))
  • content/docs/kernel/runtime-services/audit-service.mdx(via tenantId (symbol))
  • content/docs/kernel/runtime-services/sharing-service.mdx(via tenantId (symbol))
  • content/docs/permissions/index.mdx(via tenantId (symbol))
  • content/docs/protocol/kernel/index.mdx(via tenantId (symbol))
  • content/docs/protocol/objectql/query-syntax.mdx(via tenantId (symbol))

3 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/implementation-status.mdx(via tenantId (symbol))
  • content/docs/releases/v16.mdx(via tenantId (symbol))
  • content/docs/releases/v17.mdx(via /:type/:name/publish (route))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

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
  • 3 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 45 of 221 client-bound route-ledger rows — the other 176 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 490879ad0fe22c57d74799828a49feac6860757apackageMentionDocs.

Which tree this was computed on

This run read content/docs from 0dc14bb02e38e65741a6a7a1c03b3dd3009d9109 — the merge of head 1091f1e60a5ef59868aaa33341969c69aa6b6075 into base 490879ad0fe22c57d74799828a49feac6860757a, 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 0dc14bb02e38e65741a6a7a1c03b3dd3009d9109 && git checkout 0dc14bb02e38e65741a6a7a1c03b3dd3009d9109
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 490879ad0fe22c57d74799828a49feac6860757a 1091f1e60a5ef59868aaa33341969c69aa6b6075 && git checkout -B drift-repro 490879ad0fe22c57d74799828a49feac6860757a && git merge --no-ff 1091f1e60a5ef59868aaa33341969c69aa6b6075
node scripts/docs-audit/affected-docs.mjs --json 490879ad0fe22c57d74799828a49feac6860757a

⚠️ 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 490879ad0fe22c57d74799828a49feac6860757a → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 22, 2026
@os-warren
os-warren marked this pull request as ready for review August 22, 2026 07:42
@os-warren
os-warren added this pull request to the merge queueAug 22, 2026
Merged via the queue into main with commit cdaa72fAug 22, 2026
32 checks passed
@os-warren
os-warren deleted the claude/issue-10740-tenant-audit-update-half branch August 22, 2026 07:53
@github-actions

Copy link
Copy Markdown
Contributor

⛔ merge queue 构建失败 — 先分诊,再决定要不要重排

队列构建 32560345637 红了。队列跑的是全量套件(PR 侧 CI 只跑 affected 子集),
所以失败的测试可能在本 PR 没碰过的包里 —— 那不是重排能修的。每次盲目重排都会让排在后面的所有 PR 重建一轮。

失败的 job(日志抽取,best effort):

  • Console Pin Gate — 失败步骤: Build the Console SPA at the pinned objectui SHA

    ✗ Build failed in 5.80s
    

↳ 失败原因 是判读的关键:超时Test timed out in … / Hook timed out in …)多半是负载/时序,不是本 PR 的回归;
断言AssertionError: …)才指向真实的行为改变。两者的 FAIL 行长得一模一样,只有这一行能区分。

跨 PR 相同签名(24h,按失败测试文件聚合):

  • ⚠️本次没有可用的聚合签名(日志里没有能解析出测试文件名的 FAIL 行)—— 这不是「没有同签名的其他 PR」,是这一轮没测到。跨 PR 聚合本次不可用,请手工比对其他 PR 的同类评论。
  • ⚠️ 24h 评论账本没读完(超过 5 页仍未读到窗口尽头),所以上面的「不同 PR 数」是下界,不是全量。

历史信号:

  • 本 PR 过去 24h 无队列失败记录(首次)。
  • 过去 24h 队列共有 89 个失败构建(不含本次)。

分诊清单:

  1. 失败测试在本 PR 改动的包里 → 真回归,修 PR。
  2. 失败测试与本 PR 无关 → 看上面的「跨 PR 相同签名」;已有汇总 issue ⇒ flaky/环境问题实锤,去那张 issue 上谈,修好前重排只会再烧一轮全队列。
  3. 两者都不是 → 可能与同组 PR 语义冲突;等前面的 PR 落地或失败出队后再重排一次即可,不要连续重排。

Generated by Claude Code · merge-queue-triage workflow (#4859)

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

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tenant-audit, the update half: ack is a dispatcher sweep but redeliver is request-reachable — two sites on one object with OPPOSITE classifications

2 participants

@os-warren@claude