Skip to content

fix(service-storage): stamp sys_file with the acting organization, and backfill the rows that were never stamped (#12745) - #12929

Merged
os-litant merged 6 commits into
mainfrom
claude/issue-12745-sys-file-org-stamping
Aug 28, 2026
Merged

fix(service-storage): stamp sys_file with the acting organization, and backfill the rows that were never stamped (#12745)#12929
os-litant merged 6 commits into
mainfrom
claude/issue-12745-sys-file-org-stamping

Conversation

@os-litant

Copy link
Copy Markdown
Collaborator

Fixes #12745

Implements the maintainer ruling of 2026-08-28 — A with backfill (「12745 A回,其他同意。」): stamp sys_file forward and backfill the rows that were never stamped. Both halves are here; the fork the card deliberately left open is not re-litigated.

Draft: this seat does not land it.


MEASURED — re-derived on this branch's base, not inherited from the card

Every claim below was re-run on origin/main at aef1b7e64 (the branch's merge base), file and line named.

# Claim Evidence
1 The insert carried no context at all grep -c context packages/services/service-storage/src/metadata-store.ts0
2 sys_file is tenancy-ENABLED grep -n "tenancy" packages/services/service-storage/src/objects/system-file.object.tsexit 1, no matchisTenancyDisabled() reads false
3 The column exists on every install packages/objectql/src/registry.ts:450 applySystemFields: "The organization_id COLUMN is provisioned unconditionally (subject only to the explicit opt-outs above) — its existence no longer depends on the global multi-tenant flag."
4 Both callers already hold the session packages/services/service-storage/src/storage-routes.ts:330 and :435owner_id: session?.userId, ten lines below each createFile
5 The driver's arm is NULL-tolerant packages/drivers/driver-sql/src/sql-driver.ts applyTenantScope: builder.where(field, String(tenantId)).orWhereNull(field)
6 Layer 0 AND-composes a STRICT predicate above it packages/plugins/plugin-security/src/tenant-layer.ts:101 — "{ organization_id: ORG_ID } → the isolated wall, AND-composed unconditionally"; computeTenantLayer0Filter returns { organization_id: input.organizationId }
7 …and the conjunction annihilates the NULL arm packages/plugins/plugin-security/src/bootstrap-declared-permissions.ts:40-44: "Layer 0's strict organization_id = :tenant AND-composes over the driver's compatibility arm and the conjunction is the strict equality alone."
8 single posture is inert computeTenantLayer0Filter opens with if (!postureEnforcesWall(input.tenancyPosture)) return null;

⇒ The ruling's rationale still holds on the current ref: forward-only stamping would have made new files org-walled and existing NULL-org files invisible to every principal. That split is what the backfill prevents.

Half 1 — forward stamping

The channel that was missing is a context, not a column. createFile(rec, context?) takes a new optional StorageWriteContext ({ organizationId }) and hands the engine { context: { tenantId } }. The platform's own insert-side chokepoint does the rest:

context.tenantIdObjectQLEngine.buildDriverOptions (packages/objectql/src/engine.ts:3577) → DriverOptions.tenantIdSqlDriver.injectTenantOnInsert (packages/drivers/driver-sql/src/sql-driver.ts:11771) → the object's tenant column.

The column is deliberately NOT written onto the payload. Two questions a metadata store must not answer for itself are already answered there: whether this object has a tenant column (resolveTenantFieldnull ⇒ nothing is stamped) and whether an explicit value on the row wins (it does, "explicit values are never overwritten"). Re-deciding either one package away from the schema is how a stamp starts failing on an install that opted the object out. It also silences the [tenant-audit] warning this door has been raising on every walled deployment — the warning naming exactly this defect.

Where the organization comes from. buildAuthSessionResolver now reports session.session.activeOrganizationId — the platform's existing spelling, the same one HttpDispatcher (packages/runtime/src/http-dispatcher.ts:1116) and dispatcher-plugin.ts:1808 read — with the flattened shape as a fallback.

No membership fallback. marketplace-install-local-plugin's resolveActiveOrgId falls back to the caller's first sys_organization_member row; its own doc says that is a SCOPING read for a seed. Here the value becomes a wall, and a file stamped from a guessed membership is a file its uploader can no longer see from the organization they were actually acting in. No active organization ⇒ no stamp — the pre-#12745 behaviour, reported by the backfill rather than invented at the door.

Files: metadata-store.ts (createFile + StorageWriteContext), storage-routes.ts (both upload doors, StorageUploadSession), storage-service-plugin.ts (the session bridge), index.ts (the two new types).

Half 2 — the backfill

packages/services/service-storage/src/backfill-sys-file-organizations.ts, following the tree's precedent (plugin-approvals/src/backfill-platform-row-organizations.ts): one-off, idempotent, dry-run first, not exported from the package index and not shipped in dist.

Subject derivation — the two holder channels, and "exactly one". A sys_file's organization is its HOLDER's. There are exactly two holder channels in the tree and the sweep reads both: the exclusive field reference (ref_object / ref_id, ADR-0104 D3 wave 2) and every sys_attachment join row (file_idparent_object / parent_id, deliberately many-to-one). A row is stamped only when every named holder answered and they all answered the same organization.

⛔ Anything else stays NULL and is REPORTED, never guessed — including the case that reads like a majority vote: one holder in an organization beside another holder with none is AMBIGUOUS, because stamping the answer would aim the invisibility at the other holder instead. The uploader (owner_id) is not a subject: a user may belong to many organizations, so deriving from them would invent an answer the data does not carry.

The residual-NULL count is the deliverable, not the leftovers. report.totals.residualNull plus a per-reason breakdown (noSubject, subjectNotFound, subjectNotOrganizationScoped, subjectHasNoOrganization, ambiguousSubjects, unaddressable) is on the report object AND printed by formatSysFileOrganizationBackfillReport — for the dry run as well as the applied run, which is what makes the dry run a decision document rather than a preview. Those rows stay invisible under a wall; the ruling makes that residue the maintainer's to see.

Dry run and rollback. runSysFileOrganizationBackfill(engine) defaults to dryRun: true and writes nothing. applySysFileOrganizationBackfill(engine, plan) takes the plan a human read rather than a fresh scan, and each write carries only { id, organization_id } — nothing else on the row is touched, so the undo is "write NULL back to exactly these ids", which the dry run names.

Idempotency is by construction, not by a guard: the scan is WHERE organization_column IS NULL and every write fills that column. Pinned by running it twice, not by prose.

Scope: sys_file only. The precedent requires a maintainer order per table; the ruling is that order for this one table. SYS_FILE_BACKFILL_OBJECT is a single constant, not a target list, and sys_upload_session — same package, same NULL column — is deliberately not swept (filed separately, see below).

INFERRED — one deliberate divergence from the precedent, stated as such

The precedent resolves its organization column through @objectstack/metadata-core's createRecordOrganizationResolver. This sweep does not, and instead resolves the column locally in createWallOrganizationResolver, mirroring SqlDriver.computeTenantField limb for limb (ADR-0066 opt-out → declared tenancy.tenantField → injected organization_id). Two reasons, the second load-bearing:

  1. That resolver's limb 0 reads tenancy.organizationField, a key scope-pinned by the spec: audit stamping needs a read-neutral organization declaration — tenancy.tenantField cannot serve sys_api_key without walling the credential table (#8707 remainder) #8778 ruling (widened by name on cloud#1395) to exactly three platform-row writers — audit, approvals, automation runs. A fourth consumer needs its own maintainer ruling (packages/spec/src/data/object.zod.ts:598-638).
  2. It is the wrong question here. That key answers "which column says who this row is ABOUT"; the sweep needs "which column is this holder WALLED by", because the whole point is to put the file behind the same wall as its holder. sys_api_key is the shipped object where the two diverge on purpose — a credential table that must stay unwalled (tenancy.enabled: false, [finding] API keys carry no organization — under the isolated posture a minted key reads no org data at all (no leak, but the key surface is inert) #8287) while recording an organization under active_organization_id. Stamping a file from that column would wall the file into an organization its holder is not walled into. Pinned in both directions by a test.

This divergence is INFERRED from the two sources above, not ruled. If a maintainer prefers the shared resolver, the change is one function.

Verification

All figures below were taken at the branch head 882a2fe04, working tree clean.

Suitepnpm --filter @objectstack/service-storage test:

 Test Files  29 passed (29)
      Tests  470 passed (470)

26 of those are new (11 forward, 15 backfill).

Gate union, re-derived live on the ACTUAL changed set with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (9 paths, three-dot against the merge base) and every named family run. ⚠️ It named no contract family — the dispatch's clause-② no grading holds. Each gate's own judgment line, not a bare exit code:

  • check:engine-double-contractOK — 695 pinned, 134 in the DEBT ledger, 3 exempt.
  • check:where-matcher✓ where-matcher conformance holds: 310 matcher(s) discovered, 310 answer the combinator battery correctly or refuse it loudly (194 refuse).
  • check:type-check-debtcheck-type-check-coverage --re-measure: OK — 31 ledger entr(ies) re-measured in 225.6s, 1570 raw tsc error(s) total, none above its recorded number.
  • check:type-check-coverageOK — 65/78 workspace packages type-checked (plus the root), 13 in the DEBT ledger
  • check:i18nOK (9 package(s) — all bundles in sync, no undeclared authoring keys).
  • check:cross-package-test-inputsOK: 20 package(s) read outside themselves, all declared
  • check:objectql-double-limitOK ... 288 double(s) graded ... none new.
  • check:slot-lookup✓ slot-lookup ratchet holds ... none new.
  • check:query-options-erasure✓ query-options-erasure ratchet holds ... none new.
  • check:published-files, check:route-envelope, check:test-source-alias, check:type-source-resolution, check:page-declaration-shape, check:i18n-stale-fill, check:changeset-gate-self-tests, check:objectui-changeset, check:nul-bytes, check:agent-test-spelling, check:bash32-floor, check:cli-command-ids, check:entry-guard, check:parse-guard, check:pnpm-filter-targets, check:watch-hint-literal — all exit 0
  • check-ci-filter-parity, check-comment-mask-adoption, check-cross-package-test-inputs, check-plugin-teardown-shape, check-adr-0087-registration, check-changeset-no-major, check-empty-changeset, docs-audit/check-affected-docs, docs-audit/check-drift-comment — all exit 0

Repo-wide lint, not narrowed: pnpm lint (eslint . --no-inline-config) exits 0 with no findings, 58s.

⚠️ NOT MEASURED, reported as that: node scripts/pm/check-half-states.mjs returns PREREQUISITE NOT MET — the token in the environment is not a valid GitHub credential (exit 3). Nothing was swept, so it is neither a pass nor a fail. Its self-test sibling pnpm check:pm-half-states passes (1515 cases).

⚠️ @objectstack/service-storage declares no typecheck scriptpnpm --filter ... run typecheck would match nothing and exit 0, which is not a measurement. It is covered instead by DEBT['@objectstack/service-storage'] in scripts/check-type-check-coverage.mjs, and the ratchet's --re-measure runs the real tsc. That ratchet caught +5 new errors from the first draft of the new test file (four TS2835 missing .js extensions, one TS7006). They were fixed, not ledgered: the entry sits at its recorded 51 and a direct tsc --noEmit -p tsconfig.json now reports 0 errors in either new file. The package's tsconfig.json has no *.test.ts exclusion, and the positive control is stronger than a file listing — tsc reported those five errors AT lines in the new test file, so it demonstrably compiled it.

Ablation — the pins can go red

All three legs mutate a module the test imports relatively, from source (no exports resolution, no dist/ involved), so no rebuild leg applies. Each mutation was confirmed on disk by git hash-object, never by an editor's exit code; each restore used git checkout HEAD -- ABSOLUTE_PATH (never bare), under trap ... EXIT INT TERM on a path rooted at git rev-parse --show-toplevel, and was verified by hash equality to the HEAD blob plus an empty git diff HEAD and an empty git status --porcelain.

Leg Mutation Predicted Observed
Forward stamping metadata-store.ts: drop the options bag — engine.insert('sys_file', full), the pre-#12745 shape. Blob 199c0331425060dc; anchor 1 → 0, injected 1 RED RED — 5 failed / 6 passed of 11. Restored to 199c0331
Backfill: the "exactly one" clause distinct.length === 1 && answered.length === subjects.lengthdistinct.length >= 1 (first answer wins). Blob becbe0a9ce0e072c RED on the ambiguity + idempotency pins RED — 3 failed / 12 passed of 15, naming the two ambiguity pins and the second-run pin. Restored to becbe0a9
Backfill: the dry-run default if (options.dryRun !== false)if (options.dryRun === true) (writes unless asked not to). Blob becbe0a97e203af9 RED on the dry-run-default pin RED — 1 failed / 14 passed of 15. Restored to becbe0a9

Compatibility

createFile's new parameter is optional, and StorageRoutesOptions.resolveSession only WIDENS its return type ({ userId? }{ userId?, organizationId? }), so existing resolvers and callers keep compiling and keep their current behaviour. A changeset is included (minor on @objectstack/service-storage).

Out of scope, filed not fixed

sys_upload_session carries the identical shape — no tenancy key, createSession inserts with no context at metadata-store.ts:349 — and is deliberately untouched: the ruling authorises sys_file alone, and the precedent requires a maintainer order per table. Recorded as #12928, unassigned and ungraded, with the lifetime difference (transient, TTL-reaped) that may well change the answer there.

When this lands, the #11611 thread wants pointing at the ruling so the customer's operations planning is updated — the ruling asks for that and it is a PM action, not a code change.

Generated by Claude Code


Generated by Claude Code

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-storage, touching 79 documentable anchor(s). ⚠️ 1 changed file(s) yielded no anchor (packages/services/service-storage/src/index.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

54 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: node scripts/docs-audit/affected-docs.mjs --json 6228c1f0ef3aad1946763b543e9fb207b8bf7de2.

7 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/services/service-storage/src/index.ts) — pages documenting those are invisible to this run
  • 1 cross-cutting symbol(s) contributed no route anchor: organizationId (4 routes)
  • 16 name(s) were too generic to anchor anything (single lowercase words)
  • 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 6228c1f0ef3aad1946763b543e9fb207b8bf7de2packageMentionDocs.

Which tree this was computed on

This run read content/docs from 497e3bf2c1ec179b68f756db8b46c33ffbc85f14 — the merge of head 4dd420968ada19e56c52f11e518f0fb6308eda8d into base 6228c1f0ef3aad1946763b543e9fb207b8bf7de2, 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 497e3bf2c1ec179b68f756db8b46c33ffbc85f14 && git checkout 497e3bf2c1ec179b68f756db8b46c33ffbc85f14
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 6228c1f0ef3aad1946763b543e9fb207b8bf7de2 4dd420968ada19e56c52f11e518f0fb6308eda8d && git checkout -B drift-repro 6228c1f0ef3aad1946763b543e9fb207b8bf7de2 && git merge --no-ff 4dd420968ada19e56c52f11e518f0fb6308eda8d

node scripts/docs-audit/affected-docs.mjs --json 6228c1f0ef3aad1946763b543e9fb207b8bf7de2

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

@os-litant
os-litant marked this pull request as ready for review August 28, 2026 10:20
@os-litant
os-litant enabled auto-merge August 28, 2026 10:20
@os-litant
os-litant added this pull request to the merge queue Aug 28, 2026
Merged via the queue into main with commit c94be62 Aug 28, 2026
34 checks passed
@os-litant
os-litant deleted the claude/issue-12745-sys-file-org-stamping branch August 28, 2026 10:40
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/xl tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

multi-org: sys_file is never org-stamped — StorageMetadataStore.createFile inserts without a context while both callers hold the session

2 participants