Skip to content

fix(plugin-security,service-messaging): scope two more tenant-scoped declared unique indexes per organization (#8577) - #8626

Merged
os-zhuang merged 6 commits into
mainfrom
claude/issue-8577-two-more-unscoped-uniques
Aug 14, 2026
Merged

fix(plugin-security,service-messaging): scope two more tenant-scoped declared unique indexes per organization (#8577)#8626
os-zhuang merged 6 commits into
mainfrom
claude/issue-8577-two-more-unscoped-uniques

Conversation

@claude

@claudeclaudeBot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Fixes#8577

Two platform objects declared unique: true on a table-level index. On a declared index that is the positional spelling of 'global' — the listed columns verbatim — so each was an installation-wide key on a tenant-scoped object. Fifth act of the class ruled on 2026-08-13, after sys_user_preference / sys_capability (#8461), sys_position (#8556) and the five of #8554.

1. The probe came first, per object — predictions recorded before each run

R2 makes the live probe the required first step. Real engine (SqlDriver, better-sqlite3), OS_TENANCY_POSTURE=isolated, driving the real shipped declarations imported from their source files — not hand-copied fixtures — with organization_id derived from resolveInjectedSystemColumns rather than typed in.

objectmaterialized indexpredictionmeasured
sys_notification_subscriptionuniq_sys_notification_subscription_topic_principal (topic, principal)201 / 409 / 201 / GET 0exactly that
sys_audience_binding_suggestionuniq_sys_audience_binding_suggestion_79a05fef (package_id, permission_set_name, anchor)201 / 409 / 201 / GET 0exactly that
org_jia POST the key → 201
org_yi POST the SAME → 409 UNIQUE_VIOLATION
org_yi POST an unused → 201 ← the control that makes it an ORACLE
org_yi GET the key → total 0 ← refused by a row it cannot see

Neither object contradicted the static read, so there is no fork to report. Both got extra controls varying the leading column and a non-leading one — three for the three-column key — and all were accepted before and after, which is what proves the installation-wide key was the whole composite rather than its leading column. Both are pinned.

2. sys_audience_binding_suggestion: the install path, not just the 201

The card's severity note is right, and the 409/201 oracle is the lesser half. This object's key is the owning package's id, the package's own permission-set name and the anchor — the same triple for every tenant that installs the same package — while the row is per-tenant by construction.

So the pin that matters drives the realsyncAudienceBindingSuggestions on a real ObjectQL + SqlDriver engine, with the real sys_position / sys_permission_set / sys_position_permission_set declarations registered so the reconciler's anchor and binding lookups resolve to the genuine "anchor present, binding absent" state:

PRE-fix org_jia sync → { created: 1 } org_yi sync → { created: 0 }, org_yi sees 0 rows
POST-fix org_jia sync → { created: 1 } org_yi sync → { created: 1 }, each sees exactly its own

The silence is asserted too, because it is why nobody noticed: the reconciler emits no warning and no throw, and only the first tenant's reconciliation is logged. The one trace anywhere is the engine's own driver-level Insert operation failed line — the reconciler cannot tell that UNIQUE violation from the benign concurrent-sync race its catch was written for.

Anti-vacuity twin: re-running one organization's sync must still add nothing (created: 0, one row). A fix that removed uniqueness instead of scoping it would duplicate a row on every boot and is indistinguishable from the real fix by the "each org gets a row" assertion alone.

3. ⚠️ The install path is NOT whole after this PR, and that is measured, not assumed

Threading a tenant context onto the reconciler is what section 2 does. The shipped call sites do not.suggested-audience-bindings.ts reads and writes through a module-level SYSTEM_CTX = { isSystem: true } carrying no tenant, and security-plugin.ts invokes it with the bare engine. Measured on a real engine:

measurement
insert under { isSystem: true }stores organization_idNULL
insert under { isSystem: true, tenantId: X }stores organization_id = X
find under { isSystem: true }sees every organization's rows
find under { isSystem: true, tenantId: X }sees X's rows and the NULL-organization rows

Running it exactly as shipped, twice, on the post-fix schema gives one organization-less row — identical before and after this PR. So the same dead end is reachable by a second, independent road: the first tenant admin to confirm flips the single shared row and every other tenant's console stops prompting, while the binding was created only in the confirming tenant's organization.

Filed as #8617 with the measurements and the tenancy question it raises. This PR pins today's behaviour in a section named the shipped SYSTEM_CTX call path is tenant-blind (recorded, not endorsed) — a record, not an endorsement: #8617's fix must delete those two assertions, and if they still pass afterwards that fix did not work. Respelling the index is necessary (without it even a correctly tenant-scoped write is refused) and is what this card was ruled to deliver; I did not widen scope to fix the reconciler.

4. What changed

  • Two declarations respelled to unique: 'organization', spelled explicitly (R1), each with the measured probe recorded at the declaration.
  • No change to schema-drift.ts — see A1. Verified byte-identical to origin/main by object hash 3a16c6f5e297d81eb63b07781664c6dbddf2a1fe, not by eye.
  • Published text (A3): exactly one uniqueness claim exists about either object, in the hand-written content/docs/permissions/permission-sets.mdx — "one per package × set × anchor" — now "one per organization × package × set × anchor". Neither object's field text makes a uniqueness claim, so no translation bundle changed and plugin-security/src/translations/* is untouched (it is contended with an in-flight card).
  • New: one driver-level suite (39 tests) covering both objects, one install-path suite (9 tests) on a real engine, and two per-package declaration pins (5 each).
  • plugin-security gains @objectstack/objectql + @objectstack/driver-sql as devDependencies — the install-path suite needs a real engine, and the behaviour under test lives in this package. No cycle: neither depends on a plugin.

5. Ablations: colour predicted, then measured

Both predicted before running, both against the committed fix (restored with git checkout claude/issue-8577-two-more-unscoped-uniques -- the/path, never stashed).

Ablation A — revert only the two declarations to bare true.

predictedmeasured
each package pinRED 3 of 5RED 3 of 5 (×2)
driver suiteGREEN 39/39GREEN 39/39
install-path suiteGREEN 9/9GREEN 9/9

Two green results that are worth stating rather than hiding. The driver suite carries its own copied fixtures and imports no declaration — the shape #8461/#8556/#8599 all use to keep the package boundary. The install-path suite clones the real declaration and sets unique explicitly per case, so the shipped spelling is not one of its inputs either: it can prove the mechanism and cannot notice the mechanism being unshipped. The package pins are the only guard in that direction, which is exactly why each asserts "matches the fixture the driver suite copies, entry for entry". And, as on #8556 and #8554, the test named "declares exactly one unique index" stays green under the ablation — true is truthy and the filter still matches it. A pin's name is not its reach; the scope assertion is toBe('organization') and never a truthiness check.

Ablation B — disable #8461's declared-index arm in schema-drift.ts.

Predicted: fresh-database sections stay green; the deployed-installation block goes red; the decisive assertion answers 409 where 201 is expected; the three arm guards stay green for the wrong reason (they assert emptiness, and an ablated arm returns empty for everything). Predicted 11 red / 28 green.

Measured: 11 red / 28 green, decisive line:

× after applying, BOTH halves hold on the MIGRATED database
→ AssertionError: expected 409 to be 201

With that arm disabled, a fully applied non-destructive migration still answers 409 cross-organization, on both objects. Every fresh-database test stayed green throughout — a fresh-schema suite cannot see this defect at all. The red set is exactly the predicted one: per object the three deployed-installation assertions plus DROP happens only once the replacement is confirmed present (with no entry emitted there is nothing to skip) and the arm's own proposes exactly one retirement, plus section 6's convergence test.

Both ablations landed exactly on prediction, which I flag rather than claim credit for: as on #8599, two clean sweeps mean these ablations confirmed a mechanism already understood. The information on this card came from the two measurements nobody asked for — section 3 (#8617) and A1's correction that BOTH objects hit the truncation path.

6. The deployed-installation half is the load-bearing part

For each object the suite builds an installation that already has the old global index plus real rows (two tenant rows and one organization-less platform row), then migrates it. A named harness guard asserts the seeded database really carries the pre-fix index, that it holds 3 rows, and that the defect is live on it — without it the whole block could be exercising a fresh schema with every assertion still passing. The install-path suite carries its own two harness guards, asserting which index the engine actually materialized in each direction.

It then pins, per object: exactly one replace_unique_index finding categorised safe, zero orphan drop_index findings, applies without --allow-destructive, keeps every row, converges to no drift, and both halves hold on the migrated database. Also pinned: deploying the new code is not by itself the fixinitObjects is additive, so until the retirement is applied the old index keeps enforcing and the cross-organization create still 409s. The changeset says this to operators in those terms.

7. PM assumptions

8. Verification

  • Suites, all green on the merged tree (origin/main @ 60cbf9d45d merged in): driver-sql 1612 passed + 54 skipped / 99 files · plugin-security 1098 / 58 · service-messaging 229 / 22. Typecheck green on all three.
  • New: 39 driver-level tests, 9 install-path tests, 10 declaration pins.
  • Gates green: check:nul-bytes (plus a targeted control-byte self-scan of every file this PR touches), check:changeset-gate-self-tests, check:cross-package-test-inputs, check:docs-audit-scope, check:objectui-changeset, check:role-word, check:test-source-alias, check:type-source-resolution, check:query-options-erasure, check:empty-changeset, check:adr-0087-registration, check-changeset-no-major, check:i18n, check:type-check-debt.
  • ⚠️check:test-source-alias went RED and is the gate the brief did not name. The new install-path suite resolved @objectstack/objectql and @objectstack/driver-sql through their dist/, which would have made its verdict a function of build state — and for THIS suite that is not a theoretical hazard: a stale driver-sql would report the pre-fix installation-wide index as per-organization, i.e. the Two MORE clean inheritors of the #8323 class the #8554 sweep missed: sys_notification_subscription and sys_audience_binding_suggestion — both measured live #8577 defect passing. Fixed at the source with two anchored aliases in a new plugin-security/vitest.config.ts, never by widening the shrink-only registry. ⛔ No ledger was grown anywhere.
  • check:i18n first refused to run at all (@objectstack/cli not built) — note that it exits 1 while saying nothing was checked, so a piped invocation reads green either way. Built the CLI closure and re-ran: 9 packages, all bundles in sync.
  • check:type-check-debt — the --re-measure half first refused, naming @objectstack/service-knowledge as having no built type entry point ([finding][devx] check:type-check-debt 的 ledger 余量会让新写的 pin 变哑:mongodb 曾有 33 条余量吞掉一次真实回退,另有 5 条目前带 4–19 余量 #6376). It refused rather than measuring through a stale .d.ts, which is the gate working. Built it and re-ran: 33 ledger entries re-measured in 394s, 1969 raw errors, none above its recorded number, surplus none.
  • The gate list was re-derived from the actual changed paths with scripts/pm/dispatch-gates.mjs, then re-derived again for the vitest config the first pass caused me to add. It surfaced check:cross-package-test-inputs, check:docs-audit-scope, check:role-word, check:test-source-alias, check:type-source-resolution, check:adr-0087-registration, check-changeset-no-major, check:empty-changeset and the convention-triggered check:query-options-erasure / check:i18n / check:type-check-coverage — and the one that actually failed was among them.
  • origin/main was merged and the entire union plus every suite re-run after the final commit (A green gate union proves nothing about the head it is reported against — nothing ties the run to a commit, so a mid-round union goes stale silently #8550).

Breaking: no — a pure relaxation. Shipped as patch, the same call #8556 and #8554 made for the same shape. ⚠️ But it is not a no-op for operators: the changeset states that os migrate apply is required, why deploying alone is insufficient, and the caveat in section 3.

9. Patch round — check:engine-double-contract (CI red, now green)

CI failed under the job named ESLint, on the step check:engine-double-contract: the two ql handles in the install-path suite are engine doubles by the shape that gate reads, and their delete() / update() did not route through the producer's dispatch predicates. Both now open with assertEngineDeleteDispatch(options) / assertEngineUpdateDispatch(data, options). ⛔ The shrink-only engine-double-contract.baseline.json was not touched — the gate names that path maintainer-only, and it is a ratchet.

Import source, measured rather than assumed. The gate prefers @objectstack/metadata-core when @objectstack/objectql depends on the package being pinned, because that reverse edge is a cycle turbo refuses. Walking the runtime closure of @objectstack/objectql gives 12 packages and @objectstack/plugin-security is not among them, so the plugin-security → objectql devDependency edge this PR added is not a cycle and either source is legal. I took @objectstack/objectql because it is already a devDependency here, already imported by this file, and already aliased to SOURCE by the vitest.config.ts in section 8 — so it adds no new artifact-resolved specifier for check:test-source-alias to catch. metadata-core would have been a new one.

Both verbs are seams the reconciler really uses (update on the pending→confirmed-observed branch, delete on the prune branch), though this suite's fixtures reach neither; the predicates pin that whatever these seams forward is a call ObjectQL would accept.

Re-verified on the fix commit with origin/main merged in (now at 458025f266): driver-sql 1612 passed + 54 skipped / 99 files, plugin-security1132 passed / 59 files, service-messaging 229 / 22; typecheck green on all three; the whole re-derived union green, check:engine-double-contract included (213 pinned, 133 in the DEBT ledger, 2 exempt); check:type-check-debt --re-measure 33 entries, 1969 raw errors, none above its recorded number. CI: 27 success + 1 skipped, 0 failing.

One process note, since the union in section 8 did not name this gate: scripts/pm/dispatch-gates.mjs derives exactly two change-kind conventions (adds or edits a test file, edits a file in a package that owns an i18n-extract.config.ts). check:engine-double-contract appears in the script only in its closing prose, which hands it to human judgement — "the rest stay the PM judgment call — new fake engine ⇒ check:engine-double-contract". So the derivation did not miss an entry it has; it has none, and my own judgement did not classify these delegating pass-through seams as "fake engines". Both halves of that are worth recording, and the script is untouched here.


Generated by Claude Code


Generated by Claude Code

os-zhuangand others added 4 commits August 14, 2026 02:09
…organization
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH
…and changeset
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH
…ty's vitest config
check:test-source-alias measured the new install-path suite resolving both
workspace deps through dist/ — which would make its verdict a function of build
state. A stale driver-sql would report the pre-fix installation-wide index as
per-organization: the #8577 defect itself, passing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH
@vercel

vercelBot commented Aug 14, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 14, 2026 3:46am

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/plugin-security, @objectstack/service-messaging.

13 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/automation/webhooks.mdx(via @objectstack/service-messaging)
  • content/docs/deployment/cli.mdx(via @objectstack/plugin-security)
  • content/docs/kernel/runtime-services/sharing-service.mdx(via @objectstack/plugin-security)
  • content/docs/kernel/services-checklist.mdx(via @objectstack/plugin-security, @objectstack/service-messaging)
  • content/docs/permissions/access-recipes.mdx(via packages/plugins/plugin-security)
  • content/docs/permissions/authorization.mdx(via @objectstack/plugin-security)
  • content/docs/permissions/explain.mdx(via @objectstack/plugin-security)
  • content/docs/permissions/permissions-matrix.mdx(via packages/plugins/plugin-security)
  • content/docs/permissions/sharing-rules.mdx(via @objectstack/plugin-security)
  • content/docs/plugins/index.mdx(via @objectstack/plugin-security)
  • content/docs/plugins/packages.mdx(via @objectstack/plugin-security, @objectstack/service-messaging)
  • content/docs/ui/audience-based-interfaces.mdx(via packages/plugins/plugin-security)
  • content/docs/ui/dashboards.mdx(via @objectstack/plugin-security)

1 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx(via @objectstack/plugin-security, @objectstack/service-messaging)

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.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation dependencies Pull requests that update a dependency file tests tooling labels Aug 14, 2026
os-zhuangand others added 2 commits August 14, 2026 03:22
…QL's dispatch predicates
check:engine-double-contract counted the two ql handles as engine doubles whose
delete()/update() did not route through the producer's predicates. Fixed at the
doubles, never by growing the shrink-only baseline.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependenciesPull requests that update a dependency filedocumentationImprovements or additions to documentationsize/xlteststooling

Projects

None yet

1 participant

@os-zhuang