Skip to content

fix(sharing): eligibility binds declared fields through the canonical materializeDeclaredFields (#8489) - #9278

Merged
os-project-manager merged 2 commits into
mainfrom
claude/issue-8489-declared-field-binder-converge
Aug 17, 2026
Merged

fix(sharing): eligibility binds declared fields through the canonical materializeDeclaredFields (#8489)#9278
os-project-manager merged 2 commits into
mainfrom
claude/issue-8489-declared-field-binder-converge

Conversation

@os-project-manager

Copy link
Copy Markdown
Collaborator

Fixes #8489

Implements the maintainer's 2026-08-16 ruling: Option A — retire the local mirror; assertEligible adopts the canonical materializeDeclaredFields, with a spread at the call site to preserve non-mutation.

What changed

packages/plugins/plugin-sharing/src/share-link-service.ts carried its own bindDeclaredFields — a hand-written mirror of @objectstack/objectql's materializeDeclaredFields, named as a copy in its own doc comment. It is deleted. assertEligible now imports the canonical helper from @objectstack/objectql/core (already a runtime dependencies entry of this package, not a devDependency), and calls it as:

record: materializeDeclaredFields({ ...record }, schema?.fields),

The spread is load-bearing and is the ruling's own term: the canonical helper materialises in place and returns the same reference, while record here is the row just read out of engine.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 the trigger-record-change mirror.

The divergence, re-measured rather than inherited

⚠️ The divergence the card and both triage comments name does not exist. They said the copies differ when schema.fields is absent or not an object. Measured over an 11-case table against the built canonical: all six malformed-schema shapes (schema undefined / null, fields missing / 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 — "undefined counts as absent (not just a missing key): CEL treats an own key holding undefined exactly as it treats no key at all." Measured against the real @objectstack/formula engine, that is exactly CEL's behaviour: has(record.status) is false and record.status == null faults No 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-undefined row shape only, with a declared status:

eligibility predicate before after
record.status == null 422 ELIGIBILITY_UNEVALUABLE link is minted
has(record.status) 422 RECORD_NOT_ELIGIBLE link is minted
!has(record.status) link was minted 422 RECORD_NOT_ELIGIBLE
record.status == 'published' 422 ELIGIBILITY_UNEVALUABLE 422 RECORD_NOT_ELIGIBLE

Two 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. assertEligible fails 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 == null against a row that carries owner_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_at while 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-undefined behaviour.

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.

case ablated result
record.archived_at == null expects a mint faults No such key: archived_at — 422
!has(record.archived_at) expects a refusal a link is minted
own-key-undefined record.status == null expects a mint faults No such key: status — 422
own-key-undefined !has(record.status) expects a refusal a link is minted

They 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 0
  • pnpm --filter '@objectstack/plugin-sharing' test — 24 files, 620 tests passed
  • Downstream consumer sweep (prefix filter — consumers, not dependencies): @objectstack/rest 122 files / 2011 tests, @objectstack/runtime 165 files / 2464 tests, both green
  • Gates derived with node scripts/pm/dispatch-gates.mjs against 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-docs

A 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-undefined as a storage-contract defect — is filed as #9276 with a backlink, per the ruling. It lands in packages/drivers/* and is not addressed here. Reachability was re-measured for that card rather than asserted: InMemoryDriver.create with an explicit undefined preserves the own key through find, while the same driver omits the key entirely when the field is simply never written, and SqlDriver cannot 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

claude added 2 commits August 17, 2026 08:59
… 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
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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): node scripts/docs-audit/affected-docs.mjs --json origin/mainpackageMentionDocs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

plugin-sharing re-derives materializeDeclaredFields instead of importing it — a second copy of the declared-field binding contract, already diverged

2 participants