record-share-grant-revoke: package-seeded sharing rules are unaddressable by name (org filter excludes org_id-null rows)

[huangyiirene]

Symptom

Package-seeded sharing rules are invisible and unmanageable through the admin API. Stock boot, admin bearer:

1. GET /api/v1/sharing/rules → 200 {"data":[]} though sys_sharing_rule lists 4 active seeded rules.
2. GET /api/v1/sharing/rules/share_red_projects_with_execs → 404 RULE_NOT_FOUND.
3. POST …/<name>/evaluate → 404.
4. Control: POST /api/v1/sharing/rules/srule_<row id>/evaluate → 200 {matchedRecords:1, expandedUsers:2, grantsCreated:1}.

Confirming experiment: a rule created via the API (where defineRule stamps organization_id from the same context) is findable by name, listed, and evaluable by name.

Enforcement is unaffected — the boot reconcile uses SYSTEM_CTX, which carries no org — which is exactly why this stayed invisible.

Root cause

Located by the run. packages/plugins/plugin-sharing/src/sharing-rule-service.ts applies a strict-equality org filter that package-seeded rows cannot satisfy: listRules does if (orgId) where.organization_id = orgId (~L234), and getRule's name fallback is orgId ? {name, organization_id: orgId} : {name} (~L255). Package-seeded rows carry organization_id = null while an authenticated admin's context carries organizationId='org_…', so only the unfiltered by-id branch survives. Confirmed present on origin/main.

Reproduction

1. Stock showcase boot with 4 package-seeded sharing rules present in sys_sharing_rule.
2. As admin: GET /api/v1/sharing/rules → empty; GET /api/v1/sharing/rules/<seeded name> → 404; by-id evaluate → 200.

Suggested fix

Treat package-seeded (organization_id = null) rows as visible to an org-scoped admin — e.g. match organization_id = orgId OR organization_id IS NULL in listRules/getRule, mirroring how enforcement reads them under SYSTEM_CTX.

Source

Extracted from the QA run #7637 (framework 92f26f7, console 09987b680).

[os-help]

Claim: PM loop (domain:identity seat, #6022)
Session: session_01BVc1ekPpi6yaWywAUhfzfd
Branch: claude/issue-7676-sharing-rules-org-null
Worktree: objectstack-issue-7676
Mode & model: M card, mode:subagent (Claude_Code_Remote absent this shift, sanctioned fallback), model: opus.

Scope: packages/plugins/plugin-sharing/src/sharing-rule-service.ts (listRules ~L234, getRule name-fallback ~L255) + tests. Direction per the card: package-seeded (organization_id = null) rows become visible/addressable to an org-scoped admin, mirroring how enforcement already reads them under SYSTEM_CTX. One tenant-isolation question is pre-answered in the dispatch prompt: null-org rows are platform-global by construction (seeded before any org exists), so exposing them to org admins leaks nothing cross-tenant — but the dev must verify no OTHER caller of these two functions relies on the strict-equality filter for isolation before widening it.

No file overlap with any in-flight card (plugin-sharing is untouched by #7505/#7626/#7665/#7677).

---

Generated by Claude Code

[os-help]

{
  "issue": 7676,
  "status": "done",
  "premise_still_valid": true,
  "pr": "https://github.com/objectstack-ai/objectstack/pull/7760",
  "branch": "claude/issue-7676-sharing-rules-org-null",
  "commits": [
    "676433443 fix(plugin-sharing): make package-seeded (org-null) sharing rules visible and addressable by name",
    "5fc2003a3 Merge remote-tracking branch 'origin/main' into claude/issue-7676-sharing-rules-org-null"
  ],
  "files_changed": [
    "packages/plugins/plugin-sharing/src/sharing-rule-service.ts",
    "packages/plugins/plugin-sharing/src/sharing-rule.test.ts",
    ".changeset/spotty-pans-visit.md"
  ],
  "summary": "Premise confirmed at file:line on origin/main — listRules applied `if (orgId) where.organization_id = orgId` and getRule's name fallback `orgId ? {name, organization_id: orgId} : {name}`, neither of which a package-seeded row (organization_id = null, stamped because bootstrapDeclaredSharingRules defines under SYSTEM_CTX) can satisfy. Both ADMIN reads now match `organization_id = orgId OR organization_id IS NULL` via a new private `adminOrgScope`, the same predicate #3807 settled on for sys_business_unit approver expansion. By-name resolution goes through a new `findRuleRowByName` that runs two SEQUENCED lookups (own org first, platform-global second) rather than one $or with limit:1, so when both rows exist the caller's own row wins deterministically instead of whichever row a dialect reached first. defineRule is deliberately NOT widened. Also fixed the test fake engine's filter matcher, which short-circuited on $or and dropped its sibling field keys — it would have matched the whole table for listRules's {object_name, active, $or:[...]} while driver-sql and driver-memory conjoin the two.",
  "callers_audit": {
    "method": "grep for `.listRules(` / `.getRule(` / `.deleteRule(` / `.defineRule(` / `.evaluateRule(` across packages, apps and examples; every production (non-test) call site classified by the context it passes.",
    "conclusion": "No caller relies on the strict equality for TENANT ISOLATION. Widening is safe because null-org rows are platform-global by construction; another org's rows remain excluded from both widened reads.",
    "sites": [
      "rest-server.ts GET /sharing/rules -> listRules — org-scoped admin. THE DEFECT. Widened.",
      "rest-server.ts GET /sharing/rules/:idOrName -> getRule — org-scoped admin. Widened (by-name branch only).",
      "rest-server.ts DELETE /sharing/rules/:idOrName -> deleteRule -> getRule — org-scoped admin. Write path; see write_path_finding.",
      "rest-server.ts POST /sharing/rules/:idOrName/evaluate -> evaluateRule -> getRule — org-scoped admin. Materialising path; same analysis as delete.",
      "rest-server.ts POST /sharing/rules -> defineRule — org-scoped admin. NOT fed by the two widened functions; left strict deliberately.",
      "sharing-plugin.ts x5 (boot reconcile, backfill, hooks) — {isSystem:true}, no org id: takes the unfiltered branch, unchanged.",
      "sharing-rule-service.ts evaluateAllForRecord -> listRules(context) — called from rule-hooks.ts with SYSTEM_CTX, no org id: unchanged.",
      "sharing-rule-service.ts evaluateAllRulesForObject -> listRules(SYSTEM_CTX) — unchanged.",
      "bootstrap-declared-sharing-rules.ts -> defineRule(SYSTEM_CTX) — the writer of the null-org rows; unchanged."
    ],
    "write_path_finding": "Read-widening DOES reach delete and evaluate, because both resolve through getRule. Kept, because the card's own test is met: it is already the established behaviour for by-id access. getRule's by-id branch (`where: { id: idOrName }`) carries NO org filter at all, so an org admin can already delete and evaluate a null-org seeded rule by row id on origin/main today — the issue's own control experiment (by-id evaluate -> 200) is that behaviour. The change therefore adds an address form and discoverability, not a new class of authority, and a deleted seeded rule is reseeded at the next boot. It is also consistent with the #2909 seed machinery, which explicitly supports admins deactivating/customizing seeded rules across redeploys.",
    "defineRule_held_strict": "This is where a genuine write widening WOULD have been created, and it was refused. defineRule's existence lookup decides upsert-vs-insert; widening it would let one org's admin rewrite the label, criteria, recipient and access level of a row every OTHER org reads — a cross-tenant WRITE, categorically different from a cross-tenant READ of a platform-global row. Left strict, so a same-named POST still creates a row stamped with the caller's own org, and findRuleRowByName prefers that row (pinned by a test).",
    "preexisting_gap_filed_not_fixed": "getRule's by-id branch is org-unfiltered, so an org admin holding ANOTHER organization's srule_ id can read, evaluate and DELETE that org's rule and purge its sys_record_share grants. Present on origin/main, predates this change, not made worse by it (the widening never admits another org's rows). Filed as #7761 rather than fixed here — narrowing it is a behaviour change deserving its own PR and its own tests."
  },
  "tests": {
    "new": "A [#7676] describe block in packages/plugins/plugin-sharing/src/sharing-rule.test.ts, 9 cases: a PREMISE guard (the seed really stores organization_id: null and managed_by 'package'; the other two rows really carry org stamps); three visibility cases (list / by-name get / by-name evaluate, the last cross-checked against the by-id control); two isolation pins (another org's rule absent from the list, unresolvable by name -> RULE_NOT_FOUND), each carrying a positive half so neither can pass by returning nothing; a conjunction pin (org scope AND-ed with object/activeOnly, not substituted for them); an unchanged-behaviour pin for org-stamped rules; an unchanged-behaviour pin for the no-org SYSTEM_CTX/boot path; and a precedence pin for the both-rows-share-a-name case.",
    "harness_note": "This file's pre-existing `SYS` is `{isSystem: true, organizationId: 'org1'}` — isSystem bypasses only the ADR-0111 D6 capability gate, never the org scope — so the new block defines its own org-less `BOOT` context to model the seeder faithfully. Using the file's SYS would have stamped the 'seeded' rows with org1 and the whole block would have proved nothing.",
    "commands": [
      "pnpm --workspace-concurrency=2 --filter @objectstack/plugin-sharing test -- --maxWorkers=2  ->  Test Files 16 passed (16) | Tests 428 passed (428)",
      "pnpm --workspace-concurrency=2 --filter @objectstack/plugin-sharing typecheck  ->  tsc --noEmit, clean",
      "pnpm --workspace-concurrency=2 --filter @objectstack/dogfood test -- --maxWorkers=2  ->  Test Files 90 passed | 1 skipped (91) | Tests 576 passed | 3 skipped (579), exit 0  [the HTTP-layer suite covering /api/v1/sharing/rules]",
      "node scripts/check-nul-bytes.mjs  ->  OK (scanned 7115 text files, no raw ASCII control bytes)",
      "pnpm check:engine-double-contract  ->  OK — 148 pinned, 133 in the DEBT ledger, 2 exempt  [implicated: a fake engine was edited]"
    ],
    "build_closure": "pnpm --filter '@objectstack/plugin-sharing^...' build run first in the fresh worktree, and again after the merge of origin/main (which moved packages/spec and packages/objectql), together with pnpm install --frozen-lockfile and rm -rf packages/runtime/.objectstack per AGENTS.md section 9."
  },
  "ablation": {
    "prediction_made_before_running": "The three visibility cases flip red; every isolation and unchanged-behaviour pin stays green.",
    "method": "git checkout <pre-fix commit> -- packages/plugins/plugin-sharing/src/sharing-rule-service.ts (never git stash), tests kept, run, then git checkout HEAD -- <path> to restore. Run twice: once pre-merge, once on the final post-merge tree.",
    "result": "Confirmed both times, identically: 3 failed | 425 passed (428). Red: 'listRules shows the seeded rule to an org-scoped admin', 'getRule resolves the seeded rule BY NAME for an org-scoped admin', 'evaluateRule works BY NAME for the seeded rule (no RULE_NOT_FOUND)'. Green throughout: both isolation pins, the conjunction pin, the org-stamped pin, the no-org pin, the both-rows-share-a-name pin, and the premise guard.",
    "note": "The block was deliberately restructured before the ablation so each case has ONE predicted direction — the exact-set assertion was moved onto the visibility test and the isolation pins were given own-org positive halves. Before that, three pins mixed a visibility assertion into an isolation test and would have flipped red for the wrong reason, making the ablation unreadable."
  },
  "neighbors": "No conflict. Final diff vs origin/main is 3 files: 2 in packages/plugins/plugin-sharing/src/ plus the changeset — verified with `git diff origin/main --name-only | grep -v '^packages/plugins/plugin-sharing/' | grep -v '^.changeset/'` returning nothing. #7505/#7697, #7626, #7665 and #7677 are all in plugin-security, which this PR does not touch. Confirmed separately that no upstream commit modified plugin-sharing between my base and origin/main (`git diff <base> origin/main --stat -- packages/plugins/plugin-sharing` empty), so the merge introduced no overlap either.",
  "escalations": [],
  "deviations": [
    "An HTTP-layer regression test was written for packages/qa/dogfood and then DELETED, because it could not discriminate. Measured: bootStack's admin carries NO organization — a rule created through the API in that harness returns organization_id: null — so the org-scoped filter path is never entered and the test was green on the UNFIXED code. An org-scoped admin requires multiTenant: true, which per packages/verify/src/harness.ts needs the cloud-private @objectstack/organizations package, absent from this repo. Shipping it would have been a phantom check that reads as coverage. The dogfood suite was still run in full (576 passed) as the card's HTTP-layer requirement; it proves nothing regressed, not that the fix is exercised there.",
    "The test fake engine's `matches` in sharing-rule.test.ts was fixed to conjoin $or with sibling field keys instead of short-circuiting. Strictly necessary — without it the conjunction pin is meaningless — and no existing test depended on the loose behaviour. The same one-line looseness exists in four sibling fakes in this package (boot-backfill, bulk-recompute, record-share-cascade, system-write-skip-notice test files); left untouched for scope discipline, since none of their tests passes a mixed $or filter today.",
    "A real-server probe (pnpm dev --fresh + curl) was attempted and abandoned: it requires a full `pnpm build` of the workspace, which is not worth the shared container's memory and time when the dogfood suite already boots real runtimes. Production-path safety was instead established by reading the drivers: driver-sql compiles `{field: null}` to IS NULL (sql-driver.ts, `nullGuardForFieldSpec`) and conjoins a top-level $or with sibling keys, driver-memory hands both to mingo as one document, and the identical `{...filter, $or: [{organization_id: orgId}, {organization_id: null}]}` shape already ships in plugin-approvals (approval-service.ts businessUnitOrgScope, #3807)."
  ],
  "changeset": ".changeset/spotty-pans-visit.md — patch bump on @objectstack/plugin-sharing, English. States the consequences honestly: seeded rules become listable/gettable/evaluable/deletable by name; other orgs' rules stay invisible; defineRule is deliberately not widened and why; no-org callers unaffected; and that delete-by-name is an address form for authority by-id already had rather than a new capability.",
  "out_of_scope_findings": [
    "filed as #7761: plugin-sharing getRule's by-id branch has no org filter — an org admin can read, evaluate and DELETE another organization's sharing rule and purge its materialised grants (pre-existing on main, unlabeled for PM triage)",
    "filed as #7762: [finding] @objectstack/verify bootStack mints admins with NO organization, so every org-scoped read is structurally untestable in the dogfood suite — the direct reason this defect escaped both the 428-test unit suite and the 579-test dogfood suite ('finding' label, no pm:queue)"
  ],
  "open_questions": []
}

---

Generated by Claude Code

[os-help]

Review: ACCEPT — PR #7760 (PM session session_01BVc1ekPpi6yaWywAUhfzfd; diff reviewed line-by-line, not the report).

The widening is correctly bounded. adminOrgScope matches org ∪ NULL and is applied to exactly two admin reads; defineRule's existence lookup is left strict, which is the one place a genuine cross-tenant WRITE could have been created — refusing it is the right call and the reasoning is recorded in the code, not just the PR. findRuleRowByName's two sequenced lookups instead of one $or+limit:1 is the correct fix for a real dialect-dependent ambiguity, and the preference (own row wins) is pinned.

The test-fake correction is the most valuable thing in this diff. The fake engine's matches short-circuited on $or and dropped sibling keys, so {object_name, active, $or:[…]} matched the whole table there while both real drivers conjoin. Fixing the fake before trusting the new tests is exactly the discipline that keeps a green suite from shipping a broken filter; the conjunction pin now means something. Noted that four sibling fakes in this package carry the same looseness — deliberately out of scope, and no current test passes a mixed $or, so that is a defensible line.

Ablation is the shape that makes the tests pins: 3 visibility cases flip red, all isolation/unchanged/precedence pins stay green, run twice (pre- and post-merge). The pre-ablation restructuring — moving the exact-set assertion onto the visibility test so each case has ONE predicted direction — is the reason the result is readable, and it is worth copying.

Two consequences recorded, neither blocking:

1. Delete-by-name on a platform-global row has cross-tenant effect: org A's admin deleting the seeded rule removes it for every tenant until the next boot reseeds it. The authority is genuinely pre-existing (the by-id branch has no org filter, which is the issue's own control), so this PR adds discoverability rather than power — but discoverability is precisely what makes an accidental cross-tenant delete reachable. Cross-noted on plugin-sharing: getRule's by-id branch has no org filter — an org admin can read, evaluate and DELETE another organization's sharing rule (and its grants) #7761, whose fix must decide the null-org case for by-id at the same time.
2. adminOrgScope spreads a $or key into the caller's where. No current caller supplies one, so it is safe today; a future filter that does would be silently clobbered. Not worth a change now — recorded so the next editor of that function knows.

Honest negative result kept out of the tree: the dogfood HTTP test was written, measured to be non-discriminating (bootStack's admin carries no organization, so the org-scoped path is never entered — it passed against the unfixed code), and removed rather than shipped as phantom coverage, with the harness gap filed as #7762. That is the correct call, and #7762 is the more valuable artifact of the two.

Suites green on merged main: plugin-sharing 428, dogfood 576 (+3 skipped), typecheck clean, check:nul-bytes and check:engine-double-contract OK (the latter run because a fake engine was edited). Docs-drift advisory is package-level fan-out; this changes an admin read's scope, not the documented sharing semantics — no doc edits needed.

Next: flipping #7760 ready and enabling auto-merge.

---

Generated by Claude Code