fix(plugin-sharing): per-grant catch in both reconcile loops so one refused grant no longer aborts the pass (#14754) - #14930
Conversation
…used grant no longer aborts the pass After #14484 `sys_record_share` is tenant-scoped in the #13491 ledger, so an organization-less system insert on it is refused loudly with `ERR_SYSTEM_WRITE_ORGANIZATION_REQUIRED` on a walled install. A platform-global sharing rule materialising a grant onto an organization-less record resolves no organization and meets that refusal; with no per-grant catch it propagated and that rule's reconcile pass aborted mid-loop, taking the pass's stale-row revocations with it. Those revocations are the security-relevant half: a stale over-grant persisted across every later pass, which met the same record and died in the same place. Both loops now attempt each grant individually. A refusal is logged with the rule, object, record, recipient and the engine's code, counted in the pass result, and the pass continues. The catch is narrow — only `ERR_SYSTEM_WRITE_ORGANIZATION_REQUIRED` is absorbed. `record-share-organization-stamp.test.ts` deliberately pins the abort on the other error a pass can meet here (the scoped update half answering `RECORD_NOT_FOUND`), a shape the 2026-09-02 contract review left standing; a catch-all would retire that decision as a side effect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
…fted elevation reads `check:check-system-context-census` failed with pure line rot: the per-grant catch added 37 lines above `sharing-rule-service.ts`'s two `context?.isSystem` elevation reads, so the census page's anchors pointed at :157/:382 while the reads now sit at :194/:419. Re-anchored with the gate's own `--fix`; no prose and no behaviour changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
📓 Docs Drift CheckThis PR changes 1 package(s): 2 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
What this run could not see
Coarse fallback — 8 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 59292d31618731bd41426d1c42b15692ed46a776 && git checkout 59292d31618731bd41426d1c42b15692ed46a776
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 431979e67cc4d0edfb0b8743385628cc425662bc 7919e803667f4a66ada8e204d83545f3ee976a39 && git checkout -B drift-repro 431979e67cc4d0edfb0b8743385628cc425662bc && git merge --no-ff 7919e803667f4a66ada8e204d83545f3ee976a39
node scripts/docs-audit/affected-docs.mjs --json 431979e67cc4d0edfb0b8743385628cc425662bc
|
Docs Drift Check disposed — both rows correctly listed, neither page needs an edit, and one of them is worth reading before the contract reviewSeat verification ( Row 2 —
|
Patch round 1 closed with no push, and that is the correct outcome. What is blocking, and the fix I ordered that was wrong.
What is failing
Genuinely this PR's. Not a flake, not a base-branch failure, not re-run. ⭐ The in-lane fix this seat ordered would have reintroduced the defect this PR fixesI ordered: value-import It measured it. It is unsound, and the dev declined:
The class's own docblock says exactly this — identified by The dev also declined a fix that WOULD have turned the gate green, and was rightMeasured: replacing the named constant with a bare inline literal comparison makes the gate exit 0 — its patterns are Declining a green light on principle is the right call and I am endorsing it explicitly so it is not re-litigated by the next reader. Why nothing can be pushed from this laneThe gate offers exactly two remedies — an owner-key row, or a Neither remedy is even mechanical, which is why it is an adjudication rather than a one-liner: an owner-key row would assert Filed, with this PR blocked on the first
Card #14754 moves to One more measured finding, handed on rather than inferredI asked the dev to report whether Head is unchanged at Generated by Claude Code |
Contract review: PASS at head
|
Contract review ADOPTED —
|
| claim | re-read | result |
|---|---|---|
ledger row disposition: 'sdk', client: 'shares.rules.evaluate' |
rest-route-ledger.ts:390 |
exact |
SDK method typed Promise<SharingRuleEvaluationResult> |
client/src/index.ts:4747 |
exact — and unwrapResponse<SharingRuleEvaluationResult> on the line below, so the lag is stated twice in one method |
res.json(await svc.evaluateRule(...)) at rest-server.ts:10983 |
rest-server.ts:10983-10984 |
substance exact, citation compressed by one line: :10983 is const result = await svc.evaluateRule(req.params.idOrName, context ?? {}) and :10984 is res.json(result). Unfiltered passthrough of the whole service return value, which is the load-bearing part |
So the finding holds: the seventh key reaches the wire of a ledgered SDK route whose declared client type cannot name it.
Required patch 1 — declare the wire-surface consequence
Declaration only, in-lane, no code change. (a) One paragraph in .changeset/sharing-reconcile-per-grant-organization-refusal.md and in the PR body naming the route POST /api/v1/sharing/rules/:idOrName/evaluate, the ledger row, and the client-type lag. (b) The domain:spec follow-up to lift grantsRefused?: number — optional, so other ISharingRuleService implementers are not broken — into SharingRuleEvaluationResult: filed by this seat as #14962, so this half is durable rather than owed to whoever next reads this thread.
Required patch 2 — census regeneration on rebase
Adopted with one amendment, and the amendment is about measurement expiry rather than substance. The review measured the merged tree against main at 4b4d5a331 and found check:system-context-census red (4 errors, exit 1) with the two context?.isSystem reads landing at :202/:427. main has since moved to 09cc6be43. Those line numbers are therefore already stale, and re-measuring them now would only produce a second set that expires the same way.
⛔ So this patch is adopted as an instruction with a re-measurement, not as a set of line numbers to apply: at the actual rebase, take either side's anchors, run pnpm gen:system-context-census, and hand-resolve prose — the os-regen driver's own printed recipe, because system-context.mdx is a MIXED file (generated anchors plus hand prose). --fix is the instrument for line rot, not for a conflict: it cannot restore prose it never generated. Commit the resolution before regenerating; never regenerate in MERGE state.
Correcting the record on the three-pin witness
Non-blocking note §5.6 is taken up here rather than left for the next reviewer. The director-seat comment on this PR records three pins as the regression witness against a widened catch. The review's Ablation C measured two of the three: the first (record-share-organization-stamp.test.ts:459) pins grant directly and never passes through this catch, so it stays green under a wide catch. The witness is real and the standing decision is still protected — by two pins, not three.
What is NOT adopted as a finding
The review is explicit about its own gaps in §4, and this seat does not launder them into green: the "62 gate families / 60 green" and eslint figures were not re-run, the TS2551 probe was not repeated, check-test-completeness's exit-3 reading was not verified, and the 89-task consumer sweep reproduced as 89/89 turbo cache hits — a cache-key match on identical inputs, not a local tsc run. CI's own Type Check · consumer gates being green on the head is the independent evidence there, and that is what this seat is relying on.
Landing state — unchanged by this adoption
Still blocked, and not by anything in this verdict. check:error-code-provenance is red on the one constdef stamp site at sharing-rule-service.ts:87, and both remedies live in packages/spec (single-owner, another lane). #14937 (the spec adjudication) and #14936 (have objectql publish a recognizer — the root-cause fix that removes the stamp site instead of recording it) are both still open and both still awaiting triage routing. Required patch 2 belongs to the same unblock commit as whichever remedy lands.
⛔ This PR is not undrafted or armed on this adoption. The two required patches and the provenance remedy are one round, taken when the blocker clears.
Generated by Claude Code
|
Correction to the adoption above (5528078734), one reference: required patch 1(b) is filed as #14969, not #14962. I wrote the number before the card existed and it did not come back sequential — #14962 is some other card and is not related to this PR. Nothing else in that comment changes. Generated by Claude Code |
…side the derivation A family whose declared literals all name tracked FILES declares a roster — a baseline, an allowlist of the members it already has — and never a population. A list of the files that already exist can never contain one added tomorrow, so this derivation scores those families `silent` for every card in the tree, and no path a caller passes can move them. Two measured CI reds were carried by exactly that shape: `check:optional-error-sink` on PR #14866 and `check:error-code-provenance` on PR #14930, both invisible to a `--commands` harvest by construction, for every card. `artifactOnlyNote` already said all of this — per family, but only inside the silent listing, which is behind a flag no dispatch brief tells anyone to pass. The block says it where the default run shows it, and where `--commands` puts every other accounting: on stderr, so the stream a consumer executes still carries commands and nothing else. Measured before building it, over the SILENT bucket for the diff of PR #14866: 32 of 120 silent families declare only tracked artifacts, 5 of them with the roster sitting in a directory one of that card's paths is in. A minority, so the block enumerates rather than only counting, and marks the correlated subset. The block is never counted among the derived families and never merged into the runnable list. That is structural, not a filter: rosters are `silent`, and `commandsFor` reads only the matched, convention and always-runs rows. It deliberately does NOT call these gates repo-wide scanners. Whether a roster is a baseline sitting in a directory or a census taken of that directory is intent, and intent is not in the tree — the two live side by side here. The block states what is true of every member instead, and points at the producer-side remedy the residue already carries: declare the scan surface beside the roster. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019RfFHiRCSs3JXLK4cwcfox
Fixes #14754
What was wrong
After #14484 (landed as PR #14726)
sys_record_shareistenant-scopedin the #13491 ledger, so on a walled install an organization-less system insert on it is refused loudly withERR_SYSTEM_WRITE_ORGANIZATION_REQUIRED(#8844).SharingService.grantresolves the organization on every path that can; a platform-global sharing rule (organization_id = null, its sweep unscoped) materialising a grant onto an organization-LESS record resolves none, and meets that refusal.SharingRuleService.reconcile/reconcileForRecordhad no per-grant catch, so the refusal propagated and that rule's pass aborted mid-loop. Two things were lost, and they are not equally serious:The two loops
Both now attempt each grant individually through one shared helper. A refusal is logged (rule, object, record, recipient, the engine's own code), counted, and the pass continues — the remaining grants and, above all, the revoke loop still run.
Measured positions on this branch (
packages/plugins/plugin-sharing/src/sharing-rule-service.ts):ENGINE_ORGANIZATION_REFUSAL_CODEexport interface SharingRuleReconcilePassResultevaluateRuleevaluateAllForRecordreturn typegrantOrAbsorbOrganizationRefusal(the shared helper)reconcilereconcileForRecordOne further change inside both loops that is not cosmetic: the desired row is taken out of the stale set before the grant is attempted. Deleting it only on success would drop a still-desired row into the revoke loop and retract a grant the rule wants — this card's defect inverted. It is behaviour-neutral today (see the measured boundary below) and is the safe invariant under any error the catch absorbs.
The catch is deliberately narrow
Only
ERR_SYSTEM_WRITE_ORGANIZATION_REQUIREDis absorbed; everything else rethrows unchanged. Beyond the obvious (a catch-all would swallow driver outages and report a pass that "completed" having written nothing), there is a measured reason:record-share-organization-stamp.test.tsdeliberately pins the abort on the other error a reconcile pass can meet here — the scoped update half answeringRECORD_NOT_FOUNDfor a row stamped with a different organization — a shape the 2026-09-02 contract review left standing on "loud beats a wrong count". A wider catch would retire that decision as a side effect. Those three pins are untouched and still green, and they are this PR's regression witness.Both acceptance halves, pinned separately
packages/plugins/plugin-sharing/src/reconcile-refused-grant-continues.test.ts— 10 cases, realSqlDriveron better-sqlite3:memory:behind a realObjectQL,isolatedposture, platform-global rule.evaluateRuleresolves;grantsRefused2,grantsCreated2,matchedRecords4. A continuation witness asserts the attempt order and that the first refusal is not the last attempt.grantsRevoked1 alongsidegrantsRefused2. Pinned on its own, because a catch that swallowed the refusal and then skipped the revocation would satisfy HALF 1 while leaving the defect where it was.reconcileForRecordpath, plus the cross-rule case: one rule's refusal no longer aborts the wholeevaluateAllForRecordsweep, so the next rule still reconciles.Measured, and it corrects an assumption in the card
The engine returns organization-less rows LAST in a rule's criteria sweep — the driver's NULL-org compatibility arm is appended to the scoped arm. Raw driver order is
rec_first, rec_orgless, rec_last, …; the engine's filtered read answersrec_first, rec_last, rec_orgless, ….So a refused grant is nearly always one of the final attempts of a pass, and what an abort destroyed was hardly ever "the remaining grants" — it was almost entirely the revoke loop that runs after the whole upsert loop. The security half is not merely the more serious half; it is very nearly the only half. The fixture therefore carries TWO organization-less records, so the second refusal is an order-independent witness that the loop survived the first.
Measured boundary
The engine's rule gates the INSERT half only ("Insert on '...' was REFUSED"). An organization-less row that already exists takes the update half, which
grantsends unscoped, so it reaches the NULL row and is updated, not refused. Consequence: a refusal cannot reach thecur(update) branch of either loop today. Pinned as its own case so the next reader does not have to re-derive it.Ablation
Mutation: the absorption line replaced by an unconditional rethrow (i.e. the catch removed). Predicted before running: the pins redden and the revocation pin is among them.
dc843b13→0b433b30.Tests 7 failed | 22 passed (29)— reddened: both HALF 1 pins, HALF 2 (the security half), the log pin, the boundary pin, and bothreconcileForRecordpins. Still green, correctly: the bare-refusal control, the narrow-catch pin, the organization-carrying control, and all 19 cases ofrecord-share-organization-stamp.test.ts.dc843b13(equal to theHEADblob), zero markers,git diff HEADempty.Tests 29 passed (29).No rebuild was needed for either leg: the test imports
./sharing-rule-service.jsrelatively, so vitest reads the mutated source directly rather than adist/artifact.Verification, on final head
7919e8036pnpm --filter @objectstack/plugin-sharing test—Test Files 33 passed (33)/Tests 779 passed (779).pnpm --filter @objectstack/plugin-sharing typecheck— clean, andcheck:test-typecheck: OK(so the new test file is genuinely type-checked, not excluded).scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack(38 before the docs commit, re-derived to 62 after it addedcontent/docs/**). 60 green.check-system-context-censuswas red — genuinely, at pure line rot: the 37 lines added above the twocontext?.isSystemreads moved them from :157/:382 to :194/:419. Repaired with the gate's own--fix(second commit) and re-run green.check-test-completenessexits 3 = NOT MEASURED (its own verdict text says so: noturbo run testlog to hand it) — not a red.eslint . --no-inline-configover the whole repo, not a narrowing: 5786 files, 0 errors, 0 warnings.turbo run typecheck --filter='...@objectstack/plugin-sharing'(prefix filter = consumers OF the package) — 89 tasks successful..d.ts: a probe readinggrantsRefusedoff the package name compiles clean, and a deliberate typo is rejected withTS2551: Property 'grantsRefusedTYPO' does not exist ... Did you mean 'grantsRefused'?— the error names the new key, so the declaration under test is the fresh one.Clause-② — I judge this yes, against the card's "no"
The card body declares
Clause-② no("no accept-set change; a loop keeps going instead of aborting"). That is right about the accept set and wrong about the public surface, because reporting the count requires somewhere to report it.Evidence — exported-symbol diff on the built
dist/index.d.ts:SharingRuleReconcilePassResultindex.d.ts:11790)evaluateRulereturnSharingRuleEvaluationResultSharingRuleReconcilePassResultevaluateAllForRecordreturnSharingRuleEvaluationResultarraySharingRuleReconcilePassResultarraygrantsRefused: number)A new exported symbol and a new payload key on a published package. Purely additive and covariant —
@objectstack/specis untouched, its six declared fields are unchanged, and a consumer typed againstISharingRuleServicekeeps compiling (the 89-task sweep above). But additive-yes is still yes; the carriers are the seat's to hang.grantsRefused > 0does not mean the pass failed. It means the pass met a record it cannot grant on and carried on.Changeset:
minor, not the card'spatch.changeset/sharing-reconcile-per-grant-organization-refusal.md. The repair is a bug fix, which AGENTS.md would put atpatch, but it reports through a newly exported type and a new payload key on a released package — additive public surface, which isminor. Precedent in-tree for exactly this shape:fix(runtime): tell an action handler when its caller-scope record load was refused(#14143), a bug fix that shippedminorbecause the repair added a signal to a payload. Not breaking, so no ADR-0087 disposition marker is owed (check-adr-0087-registrationgreen).Scope held
SharingService.grant's organization resolution, the direct-grantread-failed ⇒ nullrule,packages/specandcontent/docs/releases/**are all untouched. The only file outsideplugin-sharingiscontent/docs/permissions/system-context.mdx, and only as the census gate's own line-rot re-anchoring.🤖 Generated with Claude Code
https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
Generated by Claude Code