Skip to content

fix(plugin-sharing): per-grant catch in both reconcile loops so one refused grant no longer aborts the pass (#14754) - #14930

Draft
claude[bot] wants to merge 2 commits into
mainfrom
claude/issue-14754-reconcile-per-grant-catch
Draft

fix(plugin-sharing): per-grant catch in both reconcile loops so one refused grant no longer aborts the pass (#14754)#14930
claude[bot] wants to merge 2 commits into
mainfrom
claude/issue-14754-reconcile-per-grant-catch

Conversation

@claude

@claude claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Fixes #14754

What was wrong

After #14484 (landed as PR #14726) sys_record_share is tenant-scoped in the #13491 ledger, so on a walled install an organization-less system insert on it is refused loudly with ERR_SYSTEM_WRITE_ORGANIZATION_REQUIRED (#8844). SharingService.grant resolves 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 / reconcileForRecord had 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 remaining grants — recoverable, the next pass writes them;
  • the stale-row revocations of that pass — NOT recoverable by waiting, because every later pass meets the same organization-less record and dies in the same place. A stale over-grant of that rule persisted indefinitely, and the record kept aborting the pass until repaired by hand. That is the security half.

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):

what line
ENGINE_ORGANIZATION_REFUSAL_CODE 87
export interface SharingRuleReconcilePassResult 105
evaluateRule 695
evaluateAllForRecord return type 790
grantOrAbsorbOrganizationRefusal (the shared helper) 1316
reconcile 1352
reconcileForRecord 1416

One 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_REQUIRED is 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.ts deliberately pins the abort on the other error a reconcile pass can meet here — the scoped update half answering RECORD_NOT_FOUND for 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, real SqlDriver on better-sqlite3 :memory: behind a real ObjectQL, isolated posture, platform-global rule.

  • HALF 1 (pass continues)evaluateRule resolves; grantsRefused 2, grantsCreated 2, matchedRecords 4. A continuation witness asserts the attempt order and that the first refusal is not the last attempt.
  • HALF 2 (the security half) — a stale rule row on an unmatched record is gone from the table after the pass, with grantsRevoked 1 alongside grantsRefused 2. 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.
  • The same two halves on the reconcileForRecord path, plus the cross-rule case: one rule's refusal no longer aborts the whole evaluateAllForRecord sweep, so the next rule still reconciles.
  • The refusal log names rule, object, record, recipient and code — one line per refused grant.
  • Controls: the bare refusal really fires on this posture; an organization-carrying record is untouched; an unrelated engine error still propagates.

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 answers rec_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 grant sends unscoped, so it reaches the NULL row and is updated, not refused. Consequence: a refusal cannot reach the cur (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.

  • Mutation proven on disk before the run: guard-line count 1 → 0, injected marker count 1, blob dc843b130b433b30.
  • Mutant leg: Tests 7 failed | 22 passed (29) — reddened: both HALF 1 pins, HALF 2 (the security half), the log pin, the boundary pin, and both reconcileForRecord pins. Still green, correctly: the bare-refusal control, the narrow-catch pin, the organization-carrying control, and all 19 cases of record-share-organization-stamp.test.ts.
  • Restore proven: blob hash back to dc843b13 (equal to the HEAD blob), zero markers, git diff HEAD empty.
  • Restored leg: Tests 29 passed (29).

No rebuild was needed for either leg: the test imports ./sharing-rule-service.js relatively, so vitest reads the mutated source directly rather than a dist/ artifact.

Verification, on final head 7919e8036

  • pnpm --filter @objectstack/plugin-sharing testTest Files 33 passed (33) / Tests 779 passed (779).
  • pnpm --filter @objectstack/plugin-sharing typecheck — clean, and check:test-typecheck: OK (so the new test file is genuinely type-checked, not excluded).
  • 62 gate families derived from the real change set via scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (38 before the docs commit, re-derived to 62 after it added content/docs/**). 60 green. check-system-context-census was red — genuinely, at pure line rot: the 37 lines added above the two context?.isSystem reads moved them from :157/:382 to :194/:419. Repaired with the gate's own --fix (second commit) and re-run green. check-test-completeness exits 3 = NOT MEASURED (its own verdict text says so: no turbo run test log to hand it) — not a red.
  • eslint . --no-inline-config over the whole repo, not a narrowing: 5786 files, 0 errors, 0 warnings.
  • Downstream consumers: turbo run typecheck --filter='...@objectstack/plugin-sharing' (prefix filter = consumers OF the package) — 89 tasks successful.
  • Reverse verification that consumers read the REBUILT .d.ts: a probe reading grantsRefused off the package name compiles clean, and a deliberate typo is rejected with TS2551: 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:

before after
SharingRuleReconcilePassResult absent exported (index.d.ts:11790)
evaluateRule return Promise of SharingRuleEvaluationResult Promise of SharingRuleReconcilePassResult
evaluateAllForRecord return Promise of SharingRuleEvaluationResult array Promise of SharingRuleReconcilePassResult array
payload keys 6 7 (grantsRefused: number)

A new exported symbol and a new payload key on a published package. Purely additive and covariant — @objectstack/spec is untouched, its six declared fields are unchanged, and a consumer typed against ISharingRuleService keeps compiling (the 89-task sweep above). But additive-yes is still yes; the carriers are the seat's to hang.

grantsRefused > 0 does not mean the pass failed. It means the pass met a record it cannot grant on and carried on.

Changeset: minor, not the card's patch

.changeset/sharing-reconcile-per-grant-organization-refusal.md. The repair is a bug fix, which AGENTS.md would put at patch, but it reports through a newly exported type and a new payload key on a released package — additive public surface, which is minor. 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 shipped minor because the repair added a signal to a payload. Not breaking, so no ADR-0087 disposition marker is owed (check-adr-0087-registration green).

Scope held

SharingService.grant's organization resolution, the direct-grant read-failed ⇒ null rule, packages/spec and content/docs/releases/** are all untouched. The only file outside plugin-sharing is content/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

…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
@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation tests tooling labels Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-sharing, touching 11 documentable anchor(s).

2 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/kernel/runtime-services/sharing-service.mdx (via evaluateRule (symbol, a method of class SharingRuleService))
  • content/docs/permissions/sharing-rules.mdx (via /sharing/rules/:idOrName/evaluate (route, bridged from symbol evaluateRule — its registrar handler names it))
What this run could not see
  • 1 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 — 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 431979e67cc4d0edfb0b8743385628cc425662bcpackageMentionDocs.

Which tree this was computed on

This run read content/docs from 59292d31618731bd41426d1c42b15692ed46a776 — the merge of head 7919e803667f4a66ada8e204d83545f3ee976a39 into base 431979e67cc4d0edfb0b8743385628cc425662bc, 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 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

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

os-sales commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Docs Drift Check disposed — both rows correctly listed, neither page needs an edit, and one of them is worth reading before the contract review

Seat verification (domain:services execution seat, reviewer of record), read at the PR head 7919e8036, not from the bot's summary.

Row 2 — content/docs/permissions/sharing-rules.mdx: the page already states the guarantee this PR is what makes true

The bot anchored this page via POST …/:idOrName/evaluate. Read on the head, the page's closing claim in "Switching a rule off withdraws the access it granted" is:

The practical guarantee: an over-granting rule is always recoverable from the API surface. Switch it off or delete it, and the access it materialized is gone — not on the next time somebody happens to touch the record, and not only after a restart (objectstack#4433, #4434).

and its table's first row:

The write that deactivates or edits the rule — That rule's grants, immediately … revokes them before the call returns

That guarantee was false on main before this PR, for the shape #14754 describes, and this PR is what restores it. This PR's own body says so from the other side:

the stale-row revocations of that pass — NOT recoverable by waiting, because every later pass meets the same organization-less record and dies in the same place. A stale over-grant of that rule persisted indefinitely

So the drift is real but points the opposite way from the usual reading: the page is not made stale by this diff — it was already inaccurate against main, and this diff converges the code onto what the page promised. ⛔ No edit is owed, and an edit here would be wrong: there is nothing to soften, and adding a caveat would document a limitation this PR removes.

That is also, independently, the sharpest available statement of why this card is the security half rather than a robustness nicety: the published documentation sold a recoverability guarantee that the platform did not keep. Worth carrying into the contract-review packet.

One residual checked and cleared rather than assumed: grantsRefused > 0 does not weaken the page's claim, because the page's guarantee is about revocation (an over-granting rule being withdrawable), not about every desired grant being created. The revoke loop now runs to completion on this shape, which is exactly what the guarantee needs.

Row 1 — content/docs/kernel/runtime-services/sharing-service.mdx: named, but not on a surface this diff moves

The bot anchored it via evaluateRule (method of SharingRuleService). The page's only occurrence is at :59, and it names evaluateRule solely as one member of the ISharingRuleService list carrying the PERMISSION_DENIED (403) org-scope condition. This diff changes neither that gate nor that code. Correctly listed by a precision-first anchor; no re-verification finding.

Note on the bot's own caveat

Its "Which tree this was computed on" block ends with:

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

I did not rely on the bot's tree. Both readings above are taken from git show 7919e803667f4a66ada8e204d83545f3ee976a39:<path> — the PR head itself — so they are reproducible from a commit id that fully identifies what was read.

⛔ This PR stays draft and unarmed: needs:contract-review is hung on it and on #14754 for the declared Clause-② yes, and this seat does not clear that carrier on its own certification.


Generated by Claude Code

os-sales commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Patch round 1 closed with no push, and that is the correct outcome. What is blocking, and the fix I ordered that was wrong.

domain:services execution seat, session session_01AUF1NoViznQK32gqpK8wS8.

What is failing

Lint & Repo Gatespnpm --filter @objectstack/spec check:error-code-provenance, exit 1, reproduced by the dev at 7919e8036:

FAIL — 1 stamp site(s) of a registered code with no provenance row:
  @objectstack/plugin-sharing stamps 'ERR_SYSTEM_WRITE_ORGANIZATION_REQUIRED' (constdef)
  at packages/plugins/plugin-sharing/src/sharing-rule-service.ts:87 — not listed under its own owner key

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 fixes

I ordered: value-import SystemWriteOrganizationRequiredError and absorb by instanceof, deleting the literal. I had verified it is a named value export from objectql's entry point, and I told the dev to measure the cross-realm risk before committing and to stop rather than work around it if unsound.

It measured it. It is unsound, and the dev declined:

SAME CLASS IDENTITY (A === B):      false
instA instanceof A (same realm):    true
instA instanceof B (CROSS-REALM):   false
code compare survives the split:    true

@objectstack/objectql declares both realms in its own exports (importdist/index.mjs, requiredist/index.js), so the two builds are two class identities. instanceof would silently stop matching, the refusal would stop being absorbed, the pass would abort mid-loop again — losing the stale-row revocations, in the deployment shape this card exists for. It would have traded a lint red for the security defect coming back.

The class's own docblock says exactly this — identified by code rather than instanceof "so the check survives crossing a package boundary where two copies of this module can exist" — and the repo ships check:dual-build-cjs-loads because consumers really do load the CJS build. I checked that the symbol was importable and did not check why the codebase deliberately compares by code. That is my error, not the dev's; the order was wrong on the substance and the stop-and-report instruction is the only reason it cost a round instead of a regression. Recorded as correction 80 on the seat post.

The dev also declined a fix that WOULD have turned the gate green, and was right

Measured: replacing the named constant with a bare inline literal comparison makes the gate exit 0 — its patterns are objlit/assign/constdef and it declares itself blind to a bare binary comparison. ⛔ Refused as evasion: it exploits a published blind spot to convert a recorded decision into an unrecorded one, and it deletes the SystemWriteOrganizationRequiredError['code'] type annotation that is currently the only guard against the spelling drifting. Same objection to matching on err.name.

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 lane

The gate offers exactly two remedies — an owner-key row, or a PROVENANCE_WAIVERS entry — and both live in packages/spec/src/api/error-code-ledger.zod.ts, which is single-owner (domain:spec). There is no third mechanism; the dev read the script in full to confirm no in-file marker or stamp-site opt-out exists. So this is a genuine cross-lane block, not a missing effort.

Neither remedy is even mechanical, which is why it is an adjudication rather than a one-liner: an owner-key row would assert plugin-sharing emits this code, which is false — objectql emits it and its owner key already lists it at error-code-ledger.zod.ts:532; and the waiver vocabulary's documented case is a door naming the wire vocabulary, not a consumer matching a producer's code.

Filed, with this PR blocked on the first

Card #14754 moves to pm:blocked accordingly; the work here is complete and waiting on another lane, not in flight.

One more measured finding, handed on rather than inferred

I asked the dev to report whether dispatch-gates.mjs --commands derived this gate, explicitly preferring its measurement to my inference. Answer: not derived--commands never emits it, and --residue puts it in the SILENT bucket because it declares only the three files it reads (the comment masker and the ledger) and never its actual scan surface, which its own header states is every package src/ tree. The 62-family harvest was faithful to what the script emitted. That is a measured second instance of #14880, and the residue output names its own remedy. Recorded there.

Head is unchanged at 7919e8036, tree clean, PR still draft, needs:contract-review still hung for the declared Clause-② yes.


Generated by Claude Code

hotlong commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Contract review: PASS at head 7919e8036 — landing blocked cross-lane on #14937 (check:error-code-provenance), carriers stay hung

Director seat, summon #12, session session_01WXyGTWPbbreqXow7Z2pZCk, on the maintainer's instruction 「现在执行契约复审」. Fuse: served claude-fable-5-1 = CONTRACT_REVIEW_TIER.

① Derived judgments

  • Public surface, additive: SharingRuleReconcilePassResult (the spec's SharingRuleEvaluationResult plus grantsRefused: number) is newly exported from the package index; evaluateRule / evaluateAllForRecord (and the private reconcile / reconcileForRecord) return it. Covariant widening — @objectstack/spec is untouched, its six declared fields unchanged, and a consumer typed against ISharingRuleService keeps compiling (89-task prefix sweep). The dev's Clause-② yes against the card's no is the correct reading and the mechanical floor agrees.
  • Behaviour: both reconcile loops attempt each grant through one helper that absorbs exactly ERR_SYSTEM_WRITE_ORGANIZATION_REQUIRED (logged with rule / object / record / recipient / code, counted) and rethrows everything else — so the stale-row revoke loop is reached. The narrowness is load-bearing: the three record-share-organization-stamp pins deliberately keep the abort on RECORD_NOT_FOUND, and a wider catch would retire that reviewed decision silently. Verified unchanged.
  • The desired row leaves the stale set before the attempt, in both loops — the safe invariant under any absorbed error (a refused level update must never fall through to a revocation). Behaviour-neutral today (a refusal cannot reach the cur branch: the engine rule gates the INSERT half only, pinned as a boundary case), and correct by construction.
  • The recognition is by code, not instanceof — the class's own documented convention, and the dev measured why: two class identities across the CJS/ESM split, instanceof false cross-realm, the code compare survives. The seat's original instanceof order would have reintroduced this card's defect; declining it was right.

② Semver vs changeset

@objectstack/plugin-sharing minor — a bug fix that reports through a newly exported type and a new payload key; precedent #14143. Consistent.

③ Boundary flags — the provenance red

check:error-code-provenance is red because plugin-sharing now spells a registered code it does not emit. Both remedies live in packages/spec (single-owner), so the seat correctly stopped and filed #14937 (spec lane, fast path) and #14936 (objectql publishes a recognizer, the durable fix). This reviewer's reading, offered to the spec seat and not enacted here: a PROVENANCE_WAIVERS row naming @objectstack/objectql as the owner with the evidence "plugin-sharing matches, it does not emit" is the honest bookkeeping (option A); an owner-key row under plugin-sharing would assert a false emission; the bare-literal respelling that turns the gate green is evasion and stays refused. If #14936 lands first, #14937 closes and the stamp site disappears instead.

CI on 7919e8036: 37 of 38 green; the one red is the provenance gate above. Carriers (this PR and #14754) stay hung: the contract PASS is recorded here, and the landing seat strips both citing it once the ledger entry lands and Lint & Repo Gates is green on this head — a fix commit confined to the spec ledger row (or to removing the literal once #14936's recognizer exists) does not reopen this PASS.


Generated by Claude Code

os-sales commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Contract review ADOPTED — PASS WITH REQUIRED PATCHES

domain:services execution seat, session session_01AUF1NoViznQK32gqpK8wS8. In-seat tier review, run as an isolated reviewer against head 7919e8036 and adopted verbatim. Reviewing model claude-fable-5-1 at xhigh, confirmed from the transcript rather than from the review's self-description.

The verdict stands as written: the catch is exactly one code wide, the delete-before-attempt reorder is measured behaviour-neutral, the Clause-② yes reading is correct on both axes, minor matches in-tree precedent, and both acceptance halves are pinned separately. Two patches are required. Neither touches the absorption logic, and neither can be applied to this frozen head.

The checkable structural claims, re-read here before adopting

A tier verdict is adopted, not trusted. The three wire-surface citations in required patch 1 were re-read against origin/main by this seat:

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?: numberoptional, 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

os-sales commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

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

os-steve pushed a commit that referenced this pull request Sep 3, 2026
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

3 participants