fix(rest): resolve the public-form lookup picker target from reference only - #17449
Merged
Merged
Conversation
…ce` only The lookup-picker route read a four-spelling tolerant chain (`reference ?? referenceTo ?? target ?? options.objectName`). Retire the tolerant tail: the spec spelling is the contract, and a stored row spelling the target the old way is a producer defect, not a dialect this route accommodates. The three cases that pinned "a legacy spelling still resolves" now pin the refusal in both halves -- `500 LOOKUP_TARGET_MISSING` and the data engine never called -- and the file's two "open production census" notes are replaced with the ruling that closed it. Claude-Session: https://claude.ai/code/session_01DapQyvYrFb1MxSYe7BL2nt Co-authored-by: Claude <noreply@anthropic.com>
The `publicPicker.object` row described the tolerant chain this branch just retired, so it now states the opposite of what the route does. Say what the route does, and name the status an alias-spelled stored row now gets. Claude-Session: https://claude.ai/code/session_01DapQyvYrFb1MxSYe7BL2nt Co-authored-by: Claude <noreply@anthropic.com>
Contributor
📓 Docs Drift Check
What this run could not see
Coarse fallback — 14 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): |
This was referenced Sep 10, 2026
os-justin
marked this pull request as ready for review
September 10, 2026 15:28
os-justin
enabled auto-merge
September 10, 2026 15:29
os-justin
deleted the
claude/issue-12920-lookup-picker-reference-only
branch
September 10, 2026 15:53
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 #12920
The public-form lookup-picker route
GET /forms/:slug/lookup/:fieldresolved its targetobject through a four-spelling tolerant chain —
reference ?? referenceTo ?? target ?? options.objectName. It now readsreference, and nothing else.Clause-②: yes
Re-declared from the delivered diff, not from the claim: this is a wire-visible behaviour
change on a published route. A picker on a stored field def that spells the target
referenceTo/target/options.objectNameand carries noreferenceused to answer200with rows from the aliased object; it now answers500 LOOKUP_TARGET_MISSINGand thedata engine is never called.
needs:contract-reviewis on the card and on this PR.What changed
packages/rest/src/rest-server.tsdef?.referencealone; the three tolerant arms are deleted. The block comment is rewritten around the ruling and the reason not to re-widen. The handler-localreferenceTois renamedreferenceObject(5 lines, same handler) — a variable named after a spelling this route now refuses is exactly the trap the comment above it warns about.packages/rest/src/public-form-lookup-picker.test.ts500+ nestedLOOKUP_TARGET_MISSING, andfindDatanever called. Both of the file's "open production census" notes are replaced with the ruling that closed it.content/docs/ui/forms.mdxpublicPicker.objectrow described the retired chain, so this change made it false. It now states the one key the route reads and the status an alias-spelled row gets..changeset/lookup-picker-reference-only.md@objectstack/rest: minor, BREAKING banner, ADR-0087 disposition.The pin is red before and green after — proven by ablation
The valuable half of the ruling is a pin that an alias-spelled target is not resolved,
and a pin that passes in both states is not evidence. Both legs ran from the committed
state, on-disk mutation proved by anchor counts and by
git hash-object, restored withgit checkout HEAD -- ...under atrap:Exactly the three new pins move; the 16 that do not include the
GUARDcase (a def with notarget at all still answers
500) and the canonical-key case — so the narrowing did notmake a canonical or partially-migrated def collateral damage. The test resolves
./rest-server.jsrelative inside its own package, so vitest reads the mutated sourcedirectly; no
dist/sits between the mutation and the assertion.Verification
pnpm --filter @objectstack/rest testTest Files 188 passed (188)·Tests 3141 passed, 1 skipped, 3142 totalpnpm --filter @objectstack/rest typechecktsconfig.test.json, 0 files / 0 errors held intest-typecheck-debt.jsonpnpm --filter '@objectstack/rest^...' buildnode scripts/pm/dispatch-gates.mjs --ran0pnpm linteslint . --no-inline-config), re-run on the final commit88e4227fTwo gates exit
3(PREREQUISITE NOT MET) until their prerequisite is met, and neither is afinding:
check:dual-build-cjs-loadsneeds every package'sdist/, andcheck:type-check-debt --re-measureneeds the built closure and its own 6144 MB tscceiling — under a caller's tighter
NODE_OPTIONSit OOMs and refuses to record. Both aregreen once run as CI runs them, and only those green runs are recorded above.
Acceptance notes
Governing text, as the ruling names it:
referenceis the contract, and a stored row spelling the target the old way is aproducer defect;
fieldReferenceToAlias(idfield-reference-to-alias) — the declared,removable conversion seam, which converts the
reference_tospelling on stored-rowrehydration and deliberately converts no other, so the tolerance this PR deletes was
never the conversion layer's;
ObjectSchema/FieldSchemarefusingrelatedTo/referenceTo/target/targetObject/lookupObjectby name, with a rename hint rather than a fold —pinned three ways at the bottom of the same test file.
The ruling: director seat summon #20, decision batch #107 item 5, 2026-09-09T05:22:52Z,
maintainer verbatim 「其他同意」 = option A. Option B (a migration card) and option C
(per-consumer status quo) were refused; the production-census prerequisite was answered by
the maintainer — none to preserve.
Deviations and judgement calls, all six:
CONTRACT_REVIEW_TIERreview before enqueue is not performed. The ruling asks forit; the tier notice on the card (2026-09-10T03:12Z, rule text merged as pm-dispatch: reserve the contract-review tier for the skills seat and the spec seat’s clause-② review; default tier elsewhere; the director’s tier is the maintainer’s per-summon choice #17294) lifted it
for this lane, reserving that tier for the skills seat, the spec seat's clause-② review
and the summoned director. Build and review both ran at the default tier.
rest-server.ts:10502-10545is now:10661-10723(the comment block plus the chain); the test file's:378-379census noteis now
:386-397. Located by symbol, as instructed.#13137suite's ownSCOPEdocblock also called the census open. Leaving one sentence in the file saying thequestion is unanswered, next to three cases that answer it, would rot immediately — it
now records who closed it and that a schema pin was never the instrument for it.
referenceTotoreferenceObject, 5 lines, no wireor type surface.
content/docs/ui/forms.mdxis in the diff. Not scope creep: the row documented thechain being retired, so this change falsifies a published statement. Fixing it here is
cheaper than a card, and this PR is the only place the two halves are consistent.
no-migration-prescription, and that choiceremoved the FROM/TO block a breaking changeset usually owes. The ruling says
not-required; the closed vocabulary then offers exactly one honest category.already-registered field-reference-to-aliaswould be false — that entry convertsreference_to, a spelling this route never read, disjoint from all three it drops.runtime-interface-onlyandtype-surface-onlydo not fit a route's runtime read, andunpublishedis untrue.no-migration-prescriptionis mechanically refused by a bodycarrying a rewrite prescription, so the body carries none — which is also the honest
position: with zero producers, a write door that refuses the spellings by name and a
maintainer ruling that no deployment holds such rows, there is no population to migrate.
not-requiredhalf of the ruling, not with this changeset — say so and it becomes a newledger entry.
Out of scope, noted and not filed. The card body frames this as a two-way choice over
roughly 30 files across 6 packages; triage narrowed it to this route alone and this PR keeps
that boundary. The other tolerant consumers are untouched and no finding is filed for them —
they are that measurement's subject, already recorded on the card, and #13137 covers the
comment defect on this route (⛔ not addressed here; it landed already). The two files this
PR edits in
packages/rest/had 0 holders across the 15 open PRs at claim time, and PR#17420's
packages/rest/rows stay untouched.Line budget: not applicable — no
skills/**path in the diff.Generated by Claude Code