fix(service-storage): stamp the acting organization on the last two sys_file insert doors - #13572
Conversation
…ys_file insert doors `copyOwnedFile` (the copy-on-claim lifecycle hook) and `materializeDataUri` (the operator backfill pass) insert `sys_file` rows without going through `StorageMetadataStore`, and carried `isSystem` with no tenant — so `buildDriverOptions` emitted no `DriverOptions.tenantId`, `injectTenantOnInsert` stamped nothing, and every row landed `organization_id = NULL` on a tenancy-enabled object. The driver's `(organization_id = :tenantId OR organization_id IS NULL)` term left those rows reachable from every organization. `isSystem` also sets `bypassTenantAudit = true`, which is the guard `auditMissingTenant` returns at, so the `[tenant-audit]` warning that names this defect never fired. Both doors now thread the organization as an execution context, mirroring the `StorageWriteContext` channel the four repaired doors already use — never as a column on the payload, so `resolveTenantField` / `injectTenantOnInsert` keep deciding whether the object has a tenant column and whether an explicit value wins. The copy takes the triggering write's organization from `HookContext.session.organizationId`; the backfill takes the organization of the record whose field held the bytes, resolved by the same `createWallOrganizationResolver` the `sys_file` organization sweep uses. Forward-stamping only: no existing row's organization is written. Where no organization is in scope the `tenantId` key is omitted entirely and the write proceeds exactly as before. Part of #13547 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs
…he engine ledgers The pins added for the two repaired insert doors introduced two fake engines. Route their `update()` through `assertEngineUpdateDispatch` so the doubles cannot accept a call shape the real `ObjectQL.update` refuses, make the lifecycle double refuse the WHERE combinators it does not implement instead of reading one as a field name, and apply the caller's `limit` after the filter. Register the new pinned coverage in the engine-double ledger (added rows only, no losses). Part of #13547 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs
📓 Docs Drift Check7 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to list — not a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run. What this run could not see
Coarse fallback — 6 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 09926fc4b3621e9db31ba2482aa4b6dbc5aa2722 && git checkout 09926fc4b3621e9db31ba2482aa4b6dbc5aa2722
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin b9972720f843033d24ec657f3d17b75435fca74a 38b13f12a399f5d1a5f84938db4e66e5054be26a && git checkout -B drift-repro b9972720f843033d24ec657f3d17b75435fca74a && git merge --no-ff 38b13f12a399f5d1a5f84938db4e66e5054be26a
node scripts/docs-audit/affected-docs.mjs --json b9972720f843033d24ec657f3d17b75435fca74a |
Fixes #13547
sys_filedeclares notenancykey, soisTenancyDisabled()readsfalseand the registry provisionsorganization_idon it. Four doors on the object had been given the acting organization one card at a time —createFile(#12745),createSession(#12928), and theupdate/deletehalves (#13178) — and all four run throughStorageMetadataStore, which threads aStorageWriteContextintocontext.tenantIdso the platform's insert-side chokepoint can stamp the column.Two doors bypassed that store entirely and carried no organization at all:
file-reference-lifecycle.tscopyOwnedFile, the copy-on-claim lifecycle hook{ context: { ...SYSTEM_CTX } }backfill-file-references.tsmaterializeDataUri, the operator backfill pass{ context: SYSTEM_CTX }SYSTEM_CTXis{ isSystem: true, [RAW_FILE_VALUES_CONTEXT_KEY]: true }—isSystem, and no tenant. SobuildDriverOptionsemitted noDriverOptions.tenantId,SqlDriver.injectTenantOnInserthad nothing to stamp from, and every row landedorganization_id = NULL. The driver's tenant term is(organization_id = :tenantId OR organization_id IS NULL), so those rows were reachable from every organization — including through the very update and delete doors #13178 had just scoped.Nothing warned, and the silence was explained rather than reassuring:
isSystemalso setsbypassTenantAudit = true, which is exactly the guardauditMissingTenantreturns at, so the[tenant-audit]line naming this defect never fired for either door.The three PM mechanism assumptions, measured
1. Can
copyOwnedFilereach the triggering write's organization? — YES.copyOwnedFileis reached only fromapplyCopyOnClaim, which is called directly by thebeforeInsert/beforeUpdatehandlers ininstallFileReferenceHooks. Those handlers receive theHookContext, andHookContext.session.organizationIdis set by ObjectQL'sbuildSession()verbatim fromExecutionContext.tenantId— the same value that would have reached the driver had the caller's own write been the one inserting. The value was already in scope; it was simply not passed down. No lookup was invented, and no default is taken.2. Does the backfill need an operator-supplied organization? — NO, and it must not take one.
materializeDataUriis reached only because a specific record's field held those bytes, and the file it creates is claimed moments later, by the rewrite below it, for that same record's slot. So the record being converted already names the answer. It was in the row being walked and merely absent from the projection.A single operator-supplied value would be wrong for most rows: one run spans every object and every organization in the deployment, so any one organization it was handed would be stamped onto other tenants' files. A backfill that stamps the wrong organization is worse than one that stamps NULL — a NULL row stays reachable, while a mis-stamped row is walled into somebody else's tenant. Hence: derive per record, or stamp nothing.
backfillFileReferences's exported signature is unchanged and takes no new parameter, so there is no operator-facing decision here.3. Is
copyOwnedFile's deliberate NULL only about ownership? — YES; the card is right.copyOwnedFiledocuments that it leaves ownership columns NULL because "the after-hook claims the copy for the slot that triggered it". Reading that after-hook:claimFilepatches exactlyref_object/ref_id/ref_field(plusstatus/deleted_atwhen reviving a tombstoned file) and never namesorganization_id. The tenant column is not an ownership column and no after-hook claims it, so the insert is the last point that can stamp it. That measurement is pinned as a test rather than left as a claim.The repair
Each door threads the organization as an execution context — never as a column on the payload, so
resolveTenantField/injectTenantOnInsertkeep deciding whether the object has a tenant column and whether an explicit value wins. Restating those two answers one package away from the schema is how they drift apart, andStorageMetadataStorealready says so in terms.HookContext.session.organizationId.createWallOrganizationResolverthesys_fileorganization sweep already uses — so an object declaringtenancy.tenantFieldis read by the column it is really walled by, and this pass cannot drift from that one. The column is added to the scan projection only when the subject really carries it; naming a column the object does not have would fail the scan for every row.Both stamp exactly what
backfill-sys-file-organizations.tswould independently derive from the new file's field-reference holder, so the forward-stamping and repair halves agree by construction rather than by coincidence.Where no organization is in scope — a caller with no active org, an unwalled object, a legacy row carrying none — the
tenantIdkey is omitted entirely and the write proceeds exactly as before.tenantId: undefinedwould not be the same thing:buildDriverOptionsreads presence.⛔ Forward-stamping only. No existing
sys_filerow's organization is written by either door; the legacy population's disposition is not this card's.The triage must-answer: are the write doors now exhaustively enumerated?
Answered by enumeration rather than by assertion, because the triage comment is right that finding these one card at a time guarantees the next door waits for the next incident. Every insert door on either object in non-test source — the whole repo,
packages/+apps/+examples/— is one of exactly four:metadata-store.ts:337StorageMetadataStore.createFilemetadata-store.ts:464StorageMetadataStore.createSessionfile-reference-lifecycle.ts:431copyOwnedFilebackfill-file-references.ts:196materializeDataUriDoors 1 and 2 are reached only from
storage-routes.ts:364,:480and:516, all three of which thread the session's organization. No other module inserts either object: the remaining storage-side modules that namesys_file(attachment-lifecycle.ts,files-to-references-migration.ts,verify-file-references.ts,stranded-orphan-inventory.ts, the CLI migrate command,plugin-email/attachment-storage.ts,service-knowledge) contain zeroinsert/createFile/createSessioncall sites — they read, update or delete only. The twocreateSession(hits outside this package are betterauth'sinternalAdapter.createSessionon the auth-session object, notsys_upload_session.⇒ With this PR, all four insert doors on both objects carry an organization.⚠️ Note the scope of that claim: it covers the INSERT side. Update/delete reach on
sys_filewas the subject of #13178, and the legacy NULL-org population remains deliberately reachable and out of scope here.Scope
⛔ No shared abstraction was generalised across this card and #13546 (the
sys_http_deliveryhalf of the same tenant-stamping family) — that sequencing is the PM's. No files underplugin-security/**,service-messaging/**,service-automation/src/builtin/http-nodes.tsorplugin-webhooks/**are touched.metadata-store.tsis unmodified: its exportedcreateWallOrganizationResolversibling is reused, and routing throughStorageMetadataStorewas not required (these two doors write a different row shape thancreateFiletakes).No new exported type and no signature change on any published entry —
installFileReferenceHooksandbackfillFileReferenceskeep their signatures, and every changed function is module-private. Clause-② stays no.Verification — union run at
38b13f1pnpm --filter @objectstack/service-storage test— 33 files, 518 tests, all passing, including 12 new pins.pnpm --filter @objectstack/service-storage build— green,check-dts-emitted: 2/2 declaration file(s) present.pnpm lint(eslint . --no-inline-config, whole repo, not narrowed) — exit 0, 102s.node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstackfrom the tree and the actual diff (re-derived after the ledger commit grew the change set, which pulled in 7 further families): 39 green.check-test-completenessgrades a saved CIturbo run testlog that only CI produces;check-half-statesneeds a real GitHub credential;check:dual-build-cjs-loadsneeds a whole-repopnpm build(54 packages have nodist/).check:i18nwas in this bucket and was cleared by building its declared closure — it now reports 9 packages, all bundles in sync.check:type-check-debtwas answered by measurement rather than skipped: this package's ledger entry records 51 errors with "THE MARGIN IS GONE", andtsc --noEmiton the package at this head returns exactly 51, with the per-code breakdown matching the ledger note limb for limb (TS2835 x23, TS7006 x15, TS2347 x4, TS2339 x4, TS2550 x3, TS6196, TS6133) and 0 attributable to the new test file. The ratchet cannot move.Reverse verification
Predicted before running: reverting both threading edits should redden exactly the stamping assertions while the "omits
tenantId" pins — which assert the pre-repair shape — stay green.Measured: 5 failed, 7 passed, precisely that partition. The mutation was confirmed on disk before the run (
grep -con both anchor texts fell to 0, and both files'git hash-objectchanged), the script carried atrap … EXIT INT TERMrestoring against absolute paths, and the restore leg was verified by state rather than exit code —git diff HEADempty and both files byte-identical to theirHEADblobs. The pins were then re-run on the restored tree: 12/12 green.The three doubles gates (
check:engine-double-contract,check:objectql-double-limit,check:where-matcher) reddened on the first run against the new test file and were fixed at the double rather than at any baseline:update()now routes throughassertEngineUpdateDispatch, the WHERE matcher refuses the combinators it does not implement instead of reading one as a field name, and thefinddouble applies the caller's bound after the filter. The new pinned coverage was registered with--write(added rows only, 0 lost).Generated by Claude Code
Generated by Claude Code