Skip to content

fix(service-storage): stamp the acting organization on the last two sys_file insert doors - #13572

Merged
os-steve merged 2 commits into
mainfrom
claude/issue-13547-sys-file-remaining-insert-doors
Aug 31, 2026
Merged

fix(service-storage): stamp the acting organization on the last two sys_file insert doors#13572
os-steve merged 2 commits into
mainfrom
claude/issue-13547-sys-file-remaining-insert-doors

Conversation

@os-steve

Copy link
Copy Markdown
Collaborator

Fixes #13547

sys_file declares no tenancy key, so isTenancyDisabled() reads false and the registry provisions organization_id on it. Four doors on the object had been given the acting organization one card at a time — createFile (#12745), createSession (#12928), and the update/delete halves (#13178) — and all four run through StorageMetadataStore, which threads a StorageWriteContext into context.tenantId so the platform's insert-side chokepoint can stamp the column.

Two doors bypassed that store entirely and carried no organization at all:

site function what it passed
file-reference-lifecycle.ts copyOwnedFile, the copy-on-claim lifecycle hook { context: { ...SYSTEM_CTX } }
backfill-file-references.ts materializeDataUri, the operator backfill pass { context: SYSTEM_CTX }

SYSTEM_CTX is { isSystem: true, [RAW_FILE_VALUES_CONTEXT_KEY]: true }isSystem, and no tenant. So buildDriverOptions emitted no DriverOptions.tenantId, SqlDriver.injectTenantOnInsert had nothing to stamp from, and every row landed organization_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: isSystem also sets bypassTenantAudit = true, which is exactly the guard auditMissingTenant returns at, so the [tenant-audit] line naming this defect never fired for either door.

The three PM mechanism assumptions, measured

1. Can copyOwnedFile reach the triggering write's organization? — YES. copyOwnedFile is reached only from applyCopyOnClaim, which is called directly by the beforeInsert / beforeUpdate handlers in installFileReferenceHooks. Those handlers receive the HookContext, and HookContext.session.organizationId is set by ObjectQL's buildSession() verbatim from ExecutionContext.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. materializeDataUri is 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. copyOwnedFile documents that it leaves ownership columns NULL because "the after-hook claims the copy for the slot that triggered it". Reading that after-hook: claimFile patches exactly ref_object / ref_id / ref_field (plus status / deleted_at when reviving a tombstoned file) and never names organization_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 / injectTenantOnInsert keep 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, and StorageMetadataStore already says so in terms.

  • The copy takes the organization of the write that triggered it, from HookContext.session.organizationId.
  • The backfill takes the organization of the record whose field held the bytes, resolved with the same createWallOrganizationResolver the sys_file organization sweep already uses — so an object declaring tenancy.tenantField is 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.ts would 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 tenantId key is omitted entirely and the write proceeds exactly as before. tenantId: undefined would not be the same thing: buildDriverOptions reads presence.

Forward-stamping only. No existing sys_file row'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:

# site door organization
1 metadata-store.ts:337 StorageMetadataStore.createFile threaded, #12745
2 metadata-store.ts:464 StorageMetadataStore.createSession threaded, #12928
3 file-reference-lifecycle.ts:431 copyOwnedFile threaded here
4 backfill-file-references.ts:196 materializeDataUri threaded here

Doors 1 and 2 are reached only from storage-routes.ts:364, :480 and :516, all three of which thread the session's organization. No other module inserts either object: the remaining storage-side modules that name sys_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 zero insert / createFile / createSession call sites — they read, update or delete only. The two createSession( hits outside this package are betterauth's internalAdapter.createSession on the auth-session object, not sys_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_file was 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_delivery half of the same tenant-stamping family) — that sequencing is the PM's. No files under plugin-security/**, service-messaging/**, service-automation/src/builtin/http-nodes.ts or plugin-webhooks/** are touched. metadata-store.ts is unmodified: its exported createWallOrganizationResolver sibling is reused, and routing through StorageMetadataStore was not required (these two doors write a different row shape than createFile takes).

No new exported type and no signature change on any published entry — installFileReferenceHooks and backfillFileReferences keep their signatures, and every changed function is module-private. Clause-② stays no.

Verification — union run at 38b13f1

  • pnpm --filter @objectstack/service-storage test33 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.
  • 42 gate families derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack from the tree and the actual diff (re-derived after the ledger commit grew the change set, which pulled in 7 further families): 39 green.
  • The 3 not green are PREREQUISITE NOT MET — NOT MEASURED, not red, and each is structurally unmeasurable in this container: check-test-completeness grades a saved CI turbo run test log that only CI produces; check-half-states needs a real GitHub credential; check:dual-build-cjs-loads needs a whole-repo pnpm build (54 packages have no dist/). check:i18n was in this bucket and was cleared by building its declared closure — it now reports 9 packages, all bundles in sync.
  • check:type-check-debt was answered by measurement rather than skipped: this package's ledger entry records 51 errors with "THE MARGIN IS GONE", and tsc --noEmit on 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 -c on both anchor texts fell to 0, and both files' git hash-object changed), the script carried a trap … EXIT INT TERM restoring against absolute paths, and the restore leg was verified by state rather than exit code — git diff HEAD empty and both files byte-identical to their HEAD blobs. 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 through assertEngineUpdateDispatch, the WHERE matcher refuses the combinators it does not implement instead of reading one as a field name, and the find double 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

claude added 2 commits August 31, 2026 01:16
…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
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

7 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to listnot 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
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 6 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 b9972720f843033d24ec657f3d17b75435fca74apackageMentionDocs.

Which tree this was computed on

This run read content/docs from 09926fc4b3621e9db31ba2482aa4b6dbc5aa2722 — the merge of head 38b13f12a399f5d1a5f84938db4e66e5054be26a into base b9972720f843033d24ec657f3d17b75435fca74a, 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 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

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

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Aug 31, 2026
@os-steve
os-steve marked this pull request as ready for review August 31, 2026 02:37
@os-steve
os-steve enabled auto-merge August 31, 2026 02:37
@os-steve
os-steve added this pull request to the merge queue Aug 31, 2026
Merged via the queue into main with commit d475838 Aug 31, 2026
34 checks passed
@os-steve
os-steve deleted the claude/issue-13547-sys-file-remaining-insert-doors branch August 31, 2026 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Two sys_file insert doors bypass StorageMetadataStore and land organization_id = NULL — outside all four doors repaired so far

2 participants