Skip to content

fix(plugin-sharing): scope an org-stamped sharing rule's criteria sweep to its own organization - #10422

Merged
os-warren merged 1 commit into
mainfrom
claude/issue-10119-sharing-rule-criteria-org-scope
Aug 20, 2026
Merged

fix(plugin-sharing): scope an org-stamped sharing rule's criteria sweep to its own organization#10422
os-warren merged 1 commit into
mainfrom
claude/issue-10119-sharing-rule-criteria-org-scope

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

Fixes#10119

What changes, and for whom

An org-stamped sharing rule's criteria sweep is now scoped to that rule's own organization. It previously swept every organization's records.

  • Org admins with org-stamped rules (organization_id non-null — what defineRule mints for any caller carrying a tenant): the rule now matches its own organization's records plus platform-owned null-org records, and no other tenant's. SharingRuleEvaluationResult.matchedRecords falls accordingly, and the next reconcile pass revokes the cross-org sys_record_share rows the rule previously created, through the existing revoke-the-remainder branch. No migration is needed.
  • Platform-global rules (organization_id = null): unchanged. They keep the full unscoped sweep, which is their declared behaviour, documented in-source at the deleteRule platform-authority guard (Decision: may an org-level sharing admin DELETE a platform-global (organization_id = null) sharing rule? #7795). This direction is pinned, not merely intended — see the ablation below.
  • No public contract changes. No schema, route, error code, or accept/reject set moves. The system elevation on the criteria read is retained: elevation and tenant are separate axes in ObjectQLEngine.buildDriverOptions, and the evaluator must still see rows no individual recipient could — it must just stop seeing rows the rule has no business in.

The defect

SharingRuleService.findMatchingRecords (whole-rule evaluation) and recordMatches (the per-record write-hook pass) both ran the criteria query under a bare SYSTEM_CTX, which carries no tenant, for every rule. The recipient half was already org-aware — expandRecipient threads rule.organization_id into TeamGraphService / BusinessUnitGraphService / PositionGraphService. So an org-stamped rule expanded recipients inside its own organization and then matched records belonging to all the others, and reconcile materialized the cross product.

The fix threads rule.organization_id as tenantId on the criteria read when it is non-null, via one new private helper criteriaContext(rule). Scoping is then done by the platform's existing chokepoint rather than by a second copy of it: the engine forwards tenantId to DriverOptions.tenantId, and SqlDriver.applyTenantScope emits (organization_id = ? OR organization_id IS NULL). An open-coded organization_id clause in filter would have collided with rules whose criteria already name that column, would not have known the object's declared tenant field, and would have dropped the NULL arm that keeps platform-seeded rows visible to every tenant (#2734).

Premise measurement — re-established before anything was changed

Measured on main @ be9dfe8e5, on one tree in one run, through a real ObjectQL on a real SqlDriver (better-sqlite3 :memory:) — not inferred from the source.

The card notes the resulting cross-org rows are inert under the Layer-0 tenant wall, which AND-composes over sharing's Layer-1 widening. So the measurement reads the materialized sys_record_share rows at rest, straight off the driver, unscoped. A "can this principal read it" probe would show nothing on either side of this change and would have wrongly read as "no defect".

Fixture: deal_a1 (org_a, won), deal_a2 (org_a, lost), deal_b1 / deal_b2 (org_b, won), deal_p1 (null org, won). Rule: criteria {stage: 'won'}, stamped org_a.

passbeforeafter
findMatchingRecords — records granted at restdeal_a1, deal_b1, deal_b2, deal_p1 (matchedRecords: 4)deal_a1, deal_p1 (matchedRecords: 2)
recordMatches on org_b's deal_b1grantsCreated: 1grantsCreated: 0

The sharpest form of the premise: the org_a-stamped rule matched exactly the same four records as a platform-global rule did — the org stamp made no difference to the sweep at all.

Ablation

Predicted signatures were written down before each leg ran. All four cases live in packages/plugins/plugin-sharing/src/rule-criteria-org-scope.test.ts.

Rebuild statement. The mutated file is src/sharing-rule-service.ts, imported by the test as the relative specifier ./sharing-rule-service.js, so Vite resolves it to the source and transpiles it in-process — no dist/ artifact of the package under test is consulted. Decisive corroboration: packages/plugins/plugin-sharing/distdoes not exist in this worktree, and the package's 624-test suite runs green regardless, so no build artifact of the subject can be what executed. scripts/ablation-dist-preflight.mjs is therefore not applicable here (it exists for subjects that resolve through a dependency's exports to dist/). The dependencies that do resolve that way were built before any measurement: @objectstack/driver-sql and @objectstack/metadata-core are aliased to source by this package's vitest.config.ts, but @objectstack/objectql is not, so pnpm --filter '@objectstack/plugin-sharing^...' build ran first.

legmutationpredictedobserved
1criteriaContext returns SYSTEM_CTX unconditionally (scope removed)the 2 org-stamped cases red with the two messages measured pre-fix; the 2 null-org cases greenexactly that. expected [ 'deal_a1', 'deal_b1', …(2) ] to not include 'deal_b1' and expected 1 to be +0 — the same two strings the pre-fix run produced. 2 failed, 2 passed
2criteriaContext scopes every rule (null org via a sentinel)the 2 null-org cases red; the 2 org-stamped cases greenexactly that. expected [ 'deal_p1' ] to deeply equal [ 'deal_a1', 'deal_b1', …(2) ] and expected +0 to be 1. 2 failed, 2 passed
3restore4 passed4 passed

Leg 2 is the one that makes the suite two-directional: it is the "scope every rule, silently retiring the platform-global sweep" implementation, and the null-org pins refuse it. A one-directional suite would have shipped that.

Restore is byte-identical, not merely equivalent: git hash-object src/sharing-rule-service.ts is fd7a27bee40b4e4e152c68c12a0a4f068495d910 both before leg 1 and after leg 3, with a clean git status.

Verification

Gate union derived by node scripts/pm/dispatch-gates.mjs (no paths passed — it takes the change set from the merge base itself), run after the final commit on a clean worktree, at 857acf74f. Exit codes captured before any pipe. 9 path-matched + 6 convention-triggered families, all green; each gate's own verdict line:

  • check:changeset-gate-self-tests, check:objectui-changeset, check:slot-lookup, check-adr-0087-registration, check-changeset-no-major, check-empty-changeset — exit 0
  • check-nul-bytes: OK (scanned 6119 text file(s) … no raw ASCII control bytes).
  • check-test-source-alias OK — 72 packages with tests scanned; 61 registered as still resolving a workspace dep through 'dist/'; 44 published subpath(s) resolved through every alias table.
  • check-engine-double-contract: OK — 338 pinned, 133 in the DEBT ledger, 2 exempt.
  • ✓ where-matcher conformance holds: 266 matcher(s) discovered … 0 silently-wrong and 0 unjudged … none new.
  • ✓ query-options-erasure ratchet holds: 67 unswept non-test site(s) … none new … baseline key set verified against be9dfe8: no files added.
  • check-type-check-coverage: OK — 64/77 workspace packages type-checked …
  • check-i18n-bundles: OK (9 package(s) — all bundles in sync, no undeclared authoring keys). — this one first refused on a precondition (PREREQUISITE NOT MET — the workspace CLI is not built, which checks nothing and is not a pass); re-run green after turbo run build --filter=@objectstack/cli.
  • check-affected-docs — exit 0, ✓ affected-docs self-test: 262 cases pass. Its body is a standing repo-wide route-reachability report, unchanged by this card.

Package suite and typecheck: pnpm --filter @objectstack/plugin-sharing testTest Files 25 passed (25) / Tests 624 passed (624); pnpm --filter @objectstack/plugin-sharing typechecktsc --noEmit, exit 0.

TEST_DEBT check.check:type-check-debt --re-measure needs the whole workspace closure built; instead of that whole-farm run I measured its subject directly for the one package this card touches — tsc --noEmit over plugin-sharing with the **/*.test.ts exclusion lifted returns exactly 3 errors (TS6133 x2, TS18048 x1), matching the ledger entry {errors: 3} exactly. The new test file contributes zero, so the ratchet is unmoved. @objectstack/plugin-auth's entry stands at 109 and is untouched — this diff is 3 files, none of them in that package.

Relationship to #10103

#10103 is not a blocker for this and this change did not wait on it: the remedy is correct under either doctrine, because it keys off the rule's own organization_id rather than off how rules are seeded. The cross-link matters in the other direction — if #10103 is ruled Option C (per-organization catalog materialization), the seeded org-less rules become N per-org copies and each copy would then sweep the whole table, multiplying this card's wrong-shaped row population by the organization count. Whoever implements that outcome should re-run this card's reasoning against the multiplied population. #10103 stays open and is not addressed here.

Grading

Clause-② graded no by triage and independently by the PM, and implementation did not falsify that: no public contract accept/reject set moves, no public surface widens. The behaviour that narrows is a system-context sweep narrowing to already-declared tenant semantics. The one externally observable delta is the sys_record_share population an org-stamped rule materializes — which is the defect, not a contract.


Generated by Claude Code

…s own organization
`SharingRuleService.findMatchingRecords` and `recordMatches` ran the rule's
criteria query under a bare `SYSTEM_CTX` carrying no tenant, for every rule.
The recipient half was already org-aware (`expandRecipient` threads
`rule.organization_id` into the team / business-unit / position graphs), so an
org-stamped rule expanded recipients inside its own organization and then swept
every other organization's records for matches — and `reconcile` materialized
the cross product as `sys_record_share` rows.
Thread the rule's own `organization_id` as `tenantId` on the criteria read when
it is non-null. The platform's existing chokepoint does the scoping:
`buildDriverOptions` forwards it to `DriverOptions.tenantId` and
`SqlDriver.applyTenantScope` emits `(organization_id = ? OR organization_id IS
NULL)`, so a scoped rule still sees platform-owned null-org rows. The system
elevation is retained — elevation and tenant are separate axes.
A null-org (platform-global) rule keeps the full unscoped sweep, which is its
declared behaviour. Both directions are pinned in
`rule-criteria-org-scope.test.ts` against a real ObjectQL on a real SqlDriver,
reading the materialized `sys_record_share` rows at rest (the cross-org rows are
inert under the Layer-0 wall, so a read probe would show nothing).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

4 anchor(s) derived from 1 changed package(s); no hand-written page names any of them. ✅

What this run could not see
  • the SDK route bridge reached 45 of 221 client-bound route-ledger rows — the other 176 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: node scripts/docs-audit/affected-docs.mjs --bridge-coverage

Coarse fallback — 8 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 e502a6a8ebafaee434a14481a45494a1dd4958c9packageMentionDocs.

Which tree this was computed on

This run read content/docs from b45dddf00eaf511706e3ee0b02c4496df5c72961 — the merge of head 857acf74f40e2d472b03cc952973c63d83d7c7b3 into base e502a6a8ebafaee434a14481a45494a1dd4958c9, 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 b45dddf00eaf511706e3ee0b02c4496df5c72961 && git checkout b45dddf00eaf511706e3ee0b02c4496df5c72961
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin e502a6a8ebafaee434a14481a45494a1dd4958c9 857acf74f40e2d472b03cc952973c63d83d7c7b3 && git checkout -B drift-repro e502a6a8ebafaee434a14481a45494a1dd4958c9 && git merge --no-ff 857acf74f40e2d472b03cc952973c63d83d7c7b3
node scripts/docs-audit/affected-docs.mjs --json e502a6a8ebafaee434a14481a45494a1dd4958c9

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

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 20, 2026
@os-warren
os-warren marked this pull request as ready for review August 20, 2026 18:43
@os-warren
os-warren enabled auto-merge August 20, 2026 18:43
@os-warren
os-warren added this pull request to the merge queueAug 20, 2026
Merged via the queue into main with commit bc400afAug 20, 2026
29 of 30 checks passed
@os-warren
os-warren deleted the claude/issue-10119-sharing-rule-criteria-org-scope branch August 20, 2026 19:58
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

2 participants

@os-warren@claude