Uh oh!
There was an error while loading. Please reload this page.
fix(plugin-security,spec): scope sys_position.name uniqueness per organization (#8468) - #8556
Conversation
…anization (#8468) The declared index carried bare `unique: true` — the positional spelling of `'global'` on a DECLARED index — so `name` was an installation-wide key on a tenant-scoped object. Measured live before the fix (two organizations, same name): 201 / 409 UNIQUE_VIOLATION / 201 control, with the caller's own GET on the colliding name returning zero rows. A per-value refusal on an unreadable row is a cross-tenant existence oracle, and a plain dead end for an admin who simply wanted to name a position `sales_manager`. Third instance of the class ruled on 2026-08-13, after sys_user_preference and sys_capability (#8461). The hierarchy counter-argument does not arise: positions are deliberately flat (ADR-0090 D3) and this object has no parent_id. Also corrects the published text — the spec `describe()` said "Unique position name" and the reference page is generated from it, so the accident had reached authors as contract — and regenerates the page from source. Migration reuses #8461's `replace_unique_index` arm unchanged: one finding, categorised safe, CREATE before DROP. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
📓 Docs Drift CheckThis PR changes 2 package(s): 109 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 7 release-owned page(s) also reference the affected code. These are read-only:
|
… guarantees (#8468) PM review Q2: the "matches the fixture the driver suite copies" pin claimed the two copies "cannot drift apart silently". It guards ONE direction — shipped declaration moves, driver fixture does not. A driver-side edit is unguarded, because nothing compares the pin's literal to FIXED_APP. Both comments now say which direction is covered and which is not, rather than implying the loop is closed. Comment-only; no assertion changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH
os-zhuang
commented
Aug 13, 2026
Both addressed. Q1 turned out to be worth measuring rather than answering — two of my three predictions were wrong, and the reason is the interesting part. Q1 — the negative guards: coverage exists, but not where I assumedYou asked whether an ablation exists that makes the arm over-claim. I ran them rather than reasoning about it. Each removes one guard from
So C1 is genuinely single-guard-discriminating: that negative test is a positive control, and it goes red the moment the guard it describes is deleted. Coverage confirmed here, and #8461's equivalent test has the identical shape, so it holds upstream too. C2 and C3 staying green was not a coverage hole in the test — it is defence in depth, which I then confirmed by double- and triple-ablating:
Mechanism, now measured rather than argued:
The answer to your actual question: covered, no action needed. The failure mode that threatens That also sharpens my Ablation B caveat in the PR body, which was correct but for an incomplete reason. I said the negative guards "assert emptiness, and an ablated arm returns empty for everything" — true, but the deeper reason those tests are hard to move is that two of the three inputs are redundantly guarded, so even a targeted single-guard ablation leaves them green.
Q2 — the comment now says what the pin deliversYou are right, and the overclaim was on both copies, not just the pin. Fixed in The pin now names the two directions explicitly — caught: "the shipped declaration changes and the driver fixture does not… measured: reverting the declaration to bare The driver-side comment had made the same claim from the other end ("so this fixture and the real metadata cannot drift apart silently"), which was the more misleading of the two since it sits directly above the unguarded copy. It now carries the warning instead, telling the next reader to treat I did not close the loop by importing across the package boundary, per your instruction and #8461's precedent. Generated by Claude Code |
Uh oh!
There was an error while loading. Please reload this page.
Fixes#8468
sys_positiondeclared{ fields: ['name'], unique: true }. On a declared index that is the positional spelling of'global'— the listed columns verbatim — sonamewas an installation-wide key on a tenant-scoped object. It is the third instance of the class ruled on 2026-08-13, aftersys_user_preferenceandsys_capability(#8461).1. The probe came first, and the premise holds
The ruling was premise-bearing: the card was filed from a static read and no live probe had ever been run against this object. Prediction recorded before running: the oracle would reproduce as 201 / 409 / 201.
Real engine (
SqlDriver, better-sqlite3),OS_TENANCY_POSTURE=isolated, driving the real shipped declaration loaded fromsys-position.object.tsrather than a hand-copied fixture:Measured exactly as predicted. The refusal is per-value and the colliding row is invisible to the caller, so it is an existence oracle; the 201 control is what makes it one rather than a blanket refusal. No fork to report.
One correction to the record, in the ruling's favour. Both the card and the triage comment reason about a position hierarchy (
parent_id) as the candidate argument for an installation-wide namespace.sys_positionhas noparent_id. Measured off the built value:has parent_id? false. The object's own header says positions "carry no capability of their own and no hierarchy", and the generated reference page says they are "deliberately flat — noparent, no hierarchy… the mistake ADR-0057 D5 retired and ADR-0090 D3 finalizes." The counter-argument is not outweighed here, it is absent.2. What changed
sys-position.object.ts:unique: 'organization', spelled explicitly (R3 — no positional default in either direction).packages/spec/src/identity/position.zod.ts: thedescribe()said "Unique position name". That is the source of the generated reference page, so the accident had reached authors as published contract. Now "Position name, unique per organization", andcontent/docs/references/identity/position.mdxregenerated from it viapnpm gen:docs— the generated file was not hand-edited.descriptionand theclone_positiondialog'shelpTextcarried the same bare "Unique" claim; both corrected. The clone dialog is the exact moment an admin types a new name, so the scope has to be right there.schema-drift.ts— see A1 below.3. Ablations: predicted colour, then measured
Both predicted before running, and both committed-then-reverted (never stashed).
Ablation A — revert only the declaration to bare
true.plugin-securitypindriver-sqlsuiteThe driver suite staying green is the point, not a miss: it carries its own fixture and does not import
SysPosition. That is precisely why the pin asserts "matches the fixture the driver suite copies, entry for entry" — without it, reverting the shipped declaration would leave the whole driver suite green. Also as predicted, the test named "declares exactly one unique index, on (name)" stayed green under the ablation:trueis truthy and the filter still matches. A pin's name tells you what someone intended to test, not what it reaches.Ablation B — revert only #8461's declared-index arm in
schema-drift.ts(the #8323 measurement, re-run for this object).Predicted: the fresh-database oracle tests stay green, the deployed-installation block goes red, and the decisive assertion answers 409 where 201 is expected. Measured — 5 red, 12 green:
expected 409 to be 201is the #8323 finding reproduced for this object: with the arm ablated, a fully applied non-destructive migration still answers 409 cross-organization. The declaration change alone would have left every deployed installation enumerable while the plan read as applied. Every section-2 test (the fresh-database oracle) stayed green throughout — a fresh-schema suite cannot see this defect at all.One honest limit: the three negative guards in section 4 also stayed green under Ablation B, for the wrong reason — they assert emptiness, and an ablated arm returns empty for everything.
After both ablations
schema-drift.tswas restored and verified byte-identical toorigin/mainby object hash, not by eye:git hash-object=3a16c6f5e297d81eb63b07781664c6dbddf2a1fe=git rev-parse origin/main:packages/drivers/driver-sql/src/schema-drift.ts.4. The deployed-installation half is the load-bearing part
sql-driver-sys-position-organization-unique.test.tsbuilds an installation that already hasuniq_sys_position_nameplus real rows (two admin positions and one platform seed), then migrates it. A named harness guard asserts the seeded database really carries the pre-fix index and that the defect is live on it — without that, the whole block could be exercising a fresh schema and every assertion in it would still pass.It then pins: one
replace_unique_indexfinding categorisedsafe, zero orphandrop_indexfindings, applies without--allow-destructive, keeps all 3 rows, converges to no drift, and both halves hold on the migrated database. Also pinned: deploying the new code is not by itself the fix —initObjectsis additive, so until the retirement is applied the old index keeps enforcing and the cross-organization create still 409s.Anti-vacuity twins throughout: every "409 flips to 201" has a same-organization duplicate that must still be refused. A fix that removed uniqueness instead of scoping it would be a worse defect and is indistinguishable from the real fix by the 201 assertion alone.
5. PM assumptions
legacyColumnsarm covers this unchanged: CONFIRMED.legacyUniqueReplacementsemits exactly one entry,legacyNames: ['uniq_sys_position_name'], replacementuniq_sys_position_organization_id_nameover['organization_id', 'name']withnullSafeColumns: ['organization_id'].schema-drift.tsis untouched; R5 really was free.organization_idinjected: CONFIRMED, derived from the built value viaresolveInjectedSystemColumns(tenancydeclnull,plan.tenanttrue, injected setid, organization_id, created_at, created_by, updated_at, updated_by), not a regex over source. Pinned as a test, because if it ever goes false the'organization'spelling has no column to key on and the fix is silently inert.legacyName === replacement.name(the ADR-0120 S6 composite that protectssys_team/sys_business_unit/sys_member) claims nothing.sys_positionwas an instance ofunique/unscoped-declared-indexbefore this change and is not after. As on tenant-scoped objects get GLOBAL unique indexes: 409-vs-201 enumerates other tenants' values, and a user's preferences silently stop persisting in their second org #8323, the rule had accurately flagged a release blocker as a warning. No lint baseline or ledger names the object, so nothing needed updating.6. The sweep found the class is not closed
The #8468 triage recorded that the earlier sweep had bounded the rest of the platform's objects, so this card would close the class. Running
lintUnscopedDeclaredIndexesover all 76 loadable object definitions, cross-referenced againstresolveInjectedSystemColumns, that does not hold. Filed unassigned, not fixed here:sys_permission_set,sys_sharing_rule,sys_webhook,sys_email_template,sys_notification_preference), queued per the ruling's standing consequence.sys_permission_setis the third leg of the same ADR-0090 RBAC triad and sits in this very directory.sys_setting's unique key is installation-wide on a tenant-scoped object — but unlike the #8323 class it has a real argument for staying that way #8555 —sys_setting, filed separately and not queued because it carries a real semantic argument for staying installation-wide, which the ruling requires be stated rather than assumed.Neither is addressed in this PR.
Verification
@objectstack/spec10461 tests / 394 files,@objectstack/plugin-security1067 / 55,@objectstack/driver-sql1474 passed + 52 skipped / 95 — all green; typecheck green on all three.check:cross-package-test-inputs,check:test-source-alias,check:type-source-resolution,check:spec-parsed-alias,check:docs-audit-scope,check:quick-reference-counts,check:role-word,check:merge-driver,check:changeset-gate-self-tests,check:objectui-changeset,check:nul-bytes,check:query-options-erasure,check:adr-0087-registration,check:changeset-no-major,check:empty-changeset,check:authorable-surface,check:docs,check:generated,check:api-surface,check:export-origins.check:i18n— first run reported PREREQUISITE NOT MET (the built CLI was missing) and explicitly says that result means nothing. Built the CLI and re-ran: 9 packages, all bundles in sync. Worth running, since this change edits field text in a package that owns ani18n-extract.config.ts.check:type-check-debt— the--re-measurehalf first refused to run (missing built closure for@objectstack/service-knowledge), which is thedispatch-gates.mjsnamescheck:type-check-coveragefor a rationale onlycheck:type-check-debtsatisfies — the ratchet half never runs locally #8545 trap. Built the full workspace closure and re-ran: 33 ledger entries re-measured, 1969 raw errors, none above its recorded number, surplus none. No ledger was grown.Breaking: no — this is a pure relaxation. Shipped as
patch.Generated by Claude Code