fix(sharing): eligibility binds declared fields through the canonical materializeDeclaredFields (#8489) - #9278
Merged
os-project-manager merged 2 commits intoAug 17, 2026
Conversation
… materializeDeclaredFields (#8489) Retire plugin-sharing's local bindDeclaredFields mirror; assertEligible now imports materializeDeclaredFields from @objectstack/objectql/core (already a runtime dependency), with a spread at the call site to preserve non-mutation. The mirror bound by key presence, the canonical binds by value, so verdicts change on one row shape: a declared field held as an own key with undefined. Two widenings and one ADR-0112 code change are accepted knowingly; the same change closes today's over-acceptance, where the mirror minted share links on !has(record.<declared field>) predicates every other server-side surface refuses. Also rewrites the eligibility pin discriminatingly (#9085): the previous declared-field case passed with the binder fully ablated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y26DJEHSBhhAQ6wwfsHNza
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y26DJEHSBhhAQ6wwfsHNza
Contributor
📓 Docs Drift Check2 anchor(s) derived from 1 changed package(s); no hand-written page names any of them. ✅ What this run could not seeCoarse fallback — 8 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): |
os-project-manager
marked this pull request as ready for review
August 17, 2026 11:03
os-project-manager
deleted the
claude/issue-8489-declared-field-binder-converge
branch
August 17, 2026 11:23
This was referenced Aug 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #8489
Implements the maintainer's 2026-08-16 ruling: Option A — retire the local mirror;
assertEligibleadopts the canonicalmaterializeDeclaredFields, with a spread at the call site to preserve non-mutation.What changed
packages/plugins/plugin-sharing/src/share-link-service.tscarried its ownbindDeclaredFields— a hand-written mirror of@objectstack/objectql'smaterializeDeclaredFields, named as a copy in its own doc comment. It is deleted.assertEligiblenow imports the canonical helper from@objectstack/objectql/core(already a runtimedependenciesentry of this package, not a devDependency), and calls it as:The spread is load-bearing and is the ruling's own term: the canonical helper materialises in place and returns the same reference, while
recordhere is the row just read out ofengine.find. Copying keeps the gate a pure read.The RULE is no longer restated at this seam —
declared-fields.ts's doc comment is the canonical statement and the call site defers to it, which is the convention the repo already applies to thetrigger-record-changemirror.The divergence, re-measured rather than inherited
schema.fieldsis absent or not an object. Measured over an 11-case table against the built canonical: all six malformed-schema shapes (schemaundefined / null,fieldsmissing / null / string /[]) return the record unchanged in both. The canonical carries a byte-identical bail. This confirms the round-1 finding on the card; nothing here needed a decision about missing schemas.Exactly one input class diverges: a declared field held as an own key whose value is
undefined. The retired mirror tested key PRESENCE (!(name in bound)); the canonical tests the VALUE (target[name] === undefined). The canonical is correct, and the canonical helper's own doc comment says so — "undefinedcounts as absent (not just a missing key): CEL treats an own key holdingundefinedexactly as it treats no key at all." Measured against the real@objectstack/formulaengine, that is exactly CEL's behaviour:has(record.status)isfalseandrecord.status == nullfaultsNo such key: status.A third divergence, also handled: the canonical mutates in place where the mirror returned a fresh object. That is what the spread covers.
The accepted verdict change
On the own-key-
undefinedrow shape only, with a declaredstatus:record.status == nullELIGIBILITY_UNEVALUABLEhas(record.status)RECORD_NOT_ELIGIBLE!has(record.status)RECORD_NOT_ELIGIBLErecord.status == 'published'ELIGIBILITY_UNEVALUABLERECORD_NOT_ELIGIBLETwo widenings and one ADR-0112 code change, accepted knowingly by the ruling. ⭐ The third row is the reason the ruling went this way, and it is the one that should be read first: the retired mirror was minting share links on
!has(...)over a declared field — a predicate every other server-side surface refuses, and the semantics PR #6454 pinned. The status quo was the unsafe side; this change closes that over-acceptance.⛔ For the record, since this touches a security-relevant gate: this was not a live security hole and is not described as one.
assertEligiblefails closed, so the drift's other direction cost a refused link, not a leaked one.The #9085 pin rewrite — in this PR, not a separate one
The previous eligibility pin "a DECLARED field the row left empty is judged, not faulted" evaluated
record.owner_id == nullagainst a row that carriesowner_id: 'u1', like every seeded row. Verified directly: it passes green with the binder fully ablated. It pinned nothing and does not survive this PR.Its replacements use a declared field the stored row genuinely does not carry — the engine reports a schema declaring
archived_atwhile the driver was initialised from a table without it, which is the real production shape (a driver that omits NULL columns, an unrun migration, a dropped projection). Two more cases pin the accepted own-key-undefinedbehaviour.Ablation evidence — direction predicted before the run, then observed: with the binder ablated, the 4 new cases fail and all 13 pre-existing cases stay green.
record.archived_at == nullexpects a mintNo such key: archived_at— 422!has(record.archived_at)expects a refusalrecord.status == nullexpects a mintNo such key: status— 422!has(record.status)expects a refusalThey fail in opposite directions, which is what makes them a tripwire rather than a pair that happens to be red together. #9085 is absorbed here and needs no separate PR.
Verification
All at final commit
d23d3ab2b, gate union re-run at that head.pnpm --filter '@objectstack/plugin-sharing' typecheck— exit 0pnpm --filter '@objectstack/plugin-sharing' test— 24 files, 620 tests passed@objectstack/rest122 files / 2011 tests,@objectstack/runtime165 files / 2464 tests, both greennode scripts/pm/dispatch-gates.mjsagainst the actual changed paths, all PASS:check:nul-bytes,check:test-source-alias,check:type-source-resolution,check:engine-double-contract,check:where-matcher,check:query-options-erasure,check:i18n,check:changeset-gate-self-tests,check:objectui-changeset,check:type-check-coverage,check:type-check-debt(33 ledger entries re-measured, none above its recorded number),check-adr-0087-registration,check-changeset-no-major,check-empty-changeset,docs-audit/check-affected-docsA changeset is included — this changes evaluation behaviour in a published package, and it states the accepted verdict changes plainly.
Out of scope
Option C — driver-side normalization of own-key-
undefinedas a storage-contract defect — is filed as #9276 with a backlink, per the ruling. It lands inpackages/drivers/*and is not addressed here. Reachability was re-measured for that card rather than asserted:InMemoryDriver.createwith an explicitundefinedpreserves the own key throughfind, while the same driver omits the key entirely when the field is simply never written, andSqlDrivercannot express the shape at all.⛔ The card's own suggested shape (move the helper into
@objectstack/core) was not implemented — superseded by PR #8483's merged resolution. #4953 was not re-opened.Generated by Claude Code