Skip to content

fix(spec): refuse a whitespace-only reference on lookup / master_detail - #16920

Merged
os-zhuang merged 2 commits into
mainfrom
claude/issue-16126-whitespace-only-reference
Sep 8, 2026
Merged

fix(spec): refuse a whitespace-only reference on lookup / master_detail#16920
os-zhuang merged 2 commits into
mainfrom
claude/issue-16126-whitespace-only-reference

Conversation

@claude

@claude claude Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Fixes #16126

Clause-②: yes

What this does

FieldSchema's reference refinement (#13632, shipped in @objectstack/spec 17.3.0) spelled its emptiness test as an equality against '', so a whitespace-only target passed a door whose whole purpose is to name an object. This applies the test to the trimmed value, so a blank reference joins absent and '' under the same custom issue, on the same reference path, with the same message.

The notion of blank is .trim() — the same one EvaluatedExpressionSchema applies to source in shared/expression.zod.ts, not a third one.

The premise was re-derived from scratch, not taken from the card

The card cited field.zod.ts:1825-1828; the refinement was located by text instead, and it had moved. What it actually checks today, verbatim from packages/spec/src/data/field.zod.ts:

(field.type === 'lookup' || field.type === 'master_detail') &&
(field.reference === undefined || field.reference === '')

No .min(1), no trim(), no second refinement anywhere: reference itself is a bare z.string().optional(), and the only other reference rule in object.zod.ts is refuseForeignTreeReference, which returns early unless type === 'tree'.

Measured through the real built artifact, packages/spec/dist/data/index.mjs, with positive controls on both sides so the acceptance is a reading rather than a broken harness:

input before after
absent refused custom@reference refused custom@reference
'' refused custom@reference refused custom@reference
' ' ACCEPTED refused custom@reference
'\t\n' ACCEPTED refused custom@reference
'account' ACCEPTED ACCEPTED
42 / null refused invalid_type@reference refused invalid_type@reference

Identical at the document level through ObjectSchema (path fields.rel.reference) and for master_detail. Before the change, the blank case parsed through to output as {"type":"lookup","reference":" "}.

The before/after diff of that matrix is exactly four rows, all accept to refuse. Nothing moved refuse to accept.

Nothing widened — measured, not asserted

pnpm --filter @objectstack/spec check:generated reports all 15 generated artifacts up to date, check:api-surface, check:export-origins, check:authorable-surface and check:docs among them. No new accepted shape, no new export, no surface movement.

Population census before refusing

One repo-wide scan over all tracked files (no extension filter) for a reference whose value is whitespace-only found one site, with 688 non-blank matches of the identical shape on the identical corpus as the positive control:

  • packages/objectql/src/master-detail-reference-alias.test.ts:129 — a test fixture cast as never into resolveMasterDetailRelation, on the path its own neighbouring test documents as "a raw registerObject carries referenceTo verbatim into the registry… registerObject skips Zod by design". It never reaches this door.

Zero in examples/, zero in JSON, and no reference: key exists in any YAML in the repo (so that negative is an empty corpus, not a scan result). That one file was run after the change: 10 passed — the narrowing provably does not redden it.

Ablation — direction predicted before running: turn red

From the committed state, the .trim() was reverted in place under trap … EXIT INT TERM. Landing proven by blob hash, never an editor's exit code:

HEAD blob:      818caeaea8268d21026b2609294d96164bc3742d
on-disk after:  6584e41933be36405d64b7797598fe17ea1a5995   (differs -> mutation landed)
fixed anchor 0 / unfixed anchor 1

Result on the unfixed code: 6 failed | 240 passed — exactly the six new whitespace pins, while the invalid_type pin and the surrounding-whitespace boundary pin stayed green, as intended. Restored with git checkout HEAD -- ..., then verified: on-disk hash back to 818caeae…, git diff HEAD empty, git status --porcelain empty.

The test imports ./field.zod — a same-package relative specifier resolving to src/, not through dist/ — so no rebuild leg applies to this ablation.

What is deliberately NOT changed

  • The message is byte-identical. Per the triage ruling on the card: 「只有 undefined / '' / 全空白三者合并到同一个 custom issue 与同一句文案」. It already prescribes what to write (reference: 'account', snake_case).
  • invalid_type is preserved for non-strings, pinned — the card and the triage both call that distinction worth keeping.
  • Trimming is for the TEST only. ' company ' is still accepted and still stored as written, pinned, matching the expression.zod.ts precedent (「Surrounding whitespace is authored, not blank」). A normalizing trim would be a behaviour change nobody asked for.

Verification

All at ab4d4a284.

  • pnpm --filter @objectstack/spec test466 files, 13036 tests, all pass
  • pnpm --filter @objectstack/spec typecheck — pass, test layer compiles
  • pnpm --filter @objectstack/spec check:generated — 15/15 up to date
  • pnpm --filter @objectstack/objectql exec vitest run src/master-detail-reference-alias.test.ts — 10 pass (the census site)
  • pnpm lint — the full repo scan, eslint . --no-inline-config, exit 0, no narrowing
  • node scripts/pm/dispatch-gates.mjs --ran75 derived families, 75 run, 0 unrun

Two of those 75 exited 3 = PREREQUISITE NOT MET, which their own output states is "NOT a pass" and "nothing was measured" — both need a whole-workspace build closure that lint.yml builds before its step, so they are declared to CI rather than reported as green: check:dual-build-cjs-loads and check:type-check-debt.

Clause ② — a conflict I am flagging rather than resolving

The dispatch judged Clause-②: no (a narrowing pulling back to the declared contract). The card's own triage ruling says the opposite: 「尽管是 bug,Clause-② 的 conformance 肢是 yes:一个今天被接受的文档(reference: ' ')之后会被拒绝,输入类在两个已发布判决之间被重新选择」, and its carrier-discipline follow-up states the label 「会在认领同笔随 Clause-②: yes 重新挂上,或随草稿 PR 挂上」.

contract-review.md names that conformance limb verbatim — 「在两个已发布码之间重选输入类」 — as needing judgement, and sets the floor 「claim 拿不准 ⇒ 按 yes 派契约复审档」. So this PR declares yes and carries needs:contract-review. The declaration is 「按设计临时…⛔ 非终审」 and the real gate is the tier review at PR time, which is the dispatching seat's, not mine. Not mine to settle — routed, not absorbed.

验收备注

  • noted, not filedpackages/objectql/src/master-detail-reference-alias.test.ts:129 uses reference: ' ' as its stand-in for a "present-but-EMPTY" canonical key while asserting ?? fall-through semantics. It is correct and green as written, and it is deliberately off the Zod path, but the value it picks is now spec-invalid metadata. Not a defect and not filed. Carrier: the next PR touching that file.
  • Follow-up for whoever lands this, from the triage ruling on the card, not actioned here: objectui's two metadata writers already refuse this shape with reference.trim() !== '', declared as a deliberate divergence in docblocks and in the changeset on objectui#7685 (part of objectui#7122). Once this lands that divergence becomes contract-following and the note can be retired. Cross-repo and outside this PR's scope.

Generated by Claude Code

The #13632 refinement spelled its emptiness test as `reference === ''`, so a
whitespace-only target passed a door whose whole purpose is to name an object.
Measured on the built artifact: absent and `''` were refused while `'   '` and
`'\t\n'` were accepted, at both the field and the document level.

Apply the test to the trimmed value, so a blank target joins absent and `''`
under the same `custom` issue, the same `reference` path and the same message.
The notion of blank is `.trim()` — the same one `EvaluatedExpressionSchema`
applies to `source`, not a third one.

Trimming is for the TEST only: a target with surrounding whitespace is authored
and is still stored as written, and a non-string still answers `invalid_type`
from the base schema. Both boundaries are pinned.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016N6xmWt5hYm94ffVEwGH8x
@github-actions github-actions Bot added the size/m label Sep 8, 2026
@github-actions github-actions Bot added documentation Improvements or additions to documentation protocol:data tests tooling labels Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/spec, touching 1 documentable anchor(s).

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

  • content/docs/concepts/metadata-driven.mdx (via FieldSchema (symbol, a top-level const))
  • content/docs/data-modeling/external-datasources.mdx (via FieldSchema (symbol, a top-level const))
  • content/docs/data-modeling/field-types.mdx (via FieldSchema (symbol, a top-level const))
  • content/docs/data-modeling/validation-rules.mdx (via FieldSchema (symbol, a top-level const))
  • content/docs/deployment/troubleshooting.mdx (via FieldSchema (symbol, a top-level const))
  • content/docs/deployment/validating-metadata.mdx (via FieldSchema (symbol, a top-level const))
  • content/docs/getting-started/quick-reference.mdx (via FieldSchema (symbol, a top-level const))
  • content/docs/kernel/contracts/data-engine.mdx (via FieldSchema (symbol, a top-level const))
  • content/docs/protocol/backward-compatibility.mdx (via FieldSchema (symbol, a top-level const))
  • content/docs/protocol/objectql/types.mdx (via FieldSchema (symbol, a top-level const))
  • content/docs/protocol/objectui/concept.mdx (via FieldSchema (symbol, a top-level const))

1 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v17.mdx (via FieldSchema (symbol, a top-level const))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • the SDK route bridge reached 60 of 216 client-bound route-ledger rows — the other 156 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 156: 0 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; 100 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 — 131 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 70f7d6d735505c03a80bdb279262af5aa7c77ff1packageMentionDocs.

Which tree this was computed on

This run read content/docs from b6d5557a6fb4ba4c128732c7c7fda26fd6e2ea57 — the merge of head ab4d4a284c9cb9b8eb2abc9111e6fa26d60c6b72 into base 70f7d6d735505c03a80bdb279262af5aa7c77ff1, 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 b6d5557a6fb4ba4c128732c7c7fda26fd6e2ea57 && git checkout b6d5557a6fb4ba4c128732c7c7fda26fd6e2ea57
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 70f7d6d735505c03a80bdb279262af5aa7c77ff1 ab4d4a284c9cb9b8eb2abc9111e6fa26d60c6b72 && git checkout -B drift-repro 70f7d6d735505c03a80bdb279262af5aa7c77ff1 && git merge --no-ff ab4d4a284c9cb9b8eb2abc9111e6fa26d60c6b72

node scripts/docs-audit/affected-docs.mjs --json 70f7d6d735505c03a80bdb279262af5aa7c77ff1

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

@claude

claude Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Docs Drift Check — answered as a reading, on a named tree

Verdict: zero of the 12 listed pages teaches or exemplifies a reference value this diff refuses. No page is edited.

Tree read: ab4d4a284, git status --porcelain empty — so this checkout fully identifies what was read. That is deliberately not the advisory's tree: it reported its own checkout "carried uncommitted changes", and it merged head f917d11a3 into base 70f7d6d73 while this worktree is cut from 0939ba008. A different list from here would be a different tree, not a wrong row.

The scan

The question is narrow: does a page teach a value that is whitespace-only? That is the only shape whose verdict this diff changes.

grep -nE "reference[\"']?[[:space:]]*[:=][[:space:]]*(['\"`][[:space:]]+['\"`])" (all 12 files)
  -> HITS_EXIT=1 (zero hits)

Positive control on the same corpus, same grep shape, non-blank value — every reference assignment actually taught in those 12 files:

page taught values
data-modeling/field-types.mdx 'account', 'order', 'category'
deployment/troubleshooting.mdx 'user', 'project' (x2)
deployment/validating-metadata.mdx 'user'
protocol/objectql/types.mdx account, contact, project, order, category
releases/v17.mdx 'sys_user' (x3), 'account'

20 assignments, every one a real snake_case object name. None blank, none empty.

data-modeling/field-types.mdx — the row you flagged as the one worth real attention — is clean: { name: 'company', label: 'Company', type: 'lookup', reference: 'account' } at :320, reference: 'order' at :349, and a tree at :363. All still parse.

Every zero has its own control

Six of the twelve pages return zero for the blank scan because they contain no reference declaration at all — for those, the blank-scan zero is an empty corpus, not a result, so here is the control that makes it a reading:

page FieldSchema mentions reference declarations
concepts/metadata-driven.mdx 3 0
data-modeling/external-datasources.mdx 1 0
data-modeling/validation-rules.mdx 2 0
getting-started/quick-reference.mdx 3 0
kernel/contracts/data-engine.mdx 1 0
protocol/backward-compatibility.mdx 1 0

They are on the advisory's list because they name the anchor, which is not a hit. They teach no reference value, so this diff cannot falsify them.

One further apparent hit was a false positive of my own grep shape and is reported rather than quietly dropped: protocol/objectui/concept.mdx:388 is Sort preference: { field: 'name', dir: 'asc' } — the match is the substring inside the word preference. Not a reference key.

content/docs/releases/v17.mdx — read-only, and not wrong

Not edited, and no docs-only card is warranted: its four reference rows all teach valid targets ('sys_user', 'account'). I found nothing factually wrong with it in respect of this change, so there is nothing to route.

One row deliberately left alone

protocol/objectql/types.mdx:758 — "A lookup that omits reference: gets no join index" — describes the driver-layer behaviour for an omitted target. That shape has been refused at parse since #13632 shipped in 17.3.0, i.e. before this PR; this diff only adds the whitespace-only spelling to the refused set. So the sentence is not falsified by this change, and tidying it would be widening the PR into a page that this diff did not touch.


Generated by Claude Code

@claude

claude Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

⛔ PARKED — the work is sound; the gate this seat cannot clear is the blocker

domain:spec execution seat, 2026-09-08T16:4xZ. ⛔ Not ACCEPTed, not cleared, not enqueued, and deliberately left in draft. Reviewed against head ab4d4a284, base 0939ba008 (merge base — ⛔ not a two-dot diff).

⚠️ First: this seat's dispatch was wrong, and the dev was right to say so

The dispatch order declared Clause-②: no on card #16126. That contradicted a ruling already sitting on the card. Triage, 5556530693 (2026-09-06T03:06Z), verbatim:

⚠️ 尽管是 bug,Clause-② 的 conformance 肢是 yes:一个今天被接受的文档(reference: ' ')之后会被拒绝,输入类在两个已发布判决之间被重新选择。⇒ 挂 needs:contract-review

The no came from applying the mechanical boundary test (the fix narrows ⇒ 拉回已声明契约 ⇒ 常规档). That test does not reach this change — references/contract-review.md excludes exactly this case: 「conformance 类 ⛔ 不机械化:填充已声明字段、在两个已发布码之间重选输入类都需判断」. Re-deciding which inputs a published schema accepts is yes whichever direction it moves.

The declaration on the claim comment has been corrected to Clause-②: yes by the seat that got it wrong, additively, with the original quoted verbatim: #16126 (comment 5588069276).

⭐ The dev hung the dual carrier, declared yes, flipped nothing to ready, enqueued nothing, armed no auto-merge, and put the disagreement to this seat as an explicit open question with both options and a recommendation — instead of silently obeying an order that contradicted the card. That is the correct handling, and its recommendation was right.

Why it parks

node scripts/pm/check-clause2-carriers.mjs --pair 16920exit 0 (declaration legible in the fixed spelling, both carriers agree). Both limbs of the clause-② enqueue gate fire:

  • path limb — the diff touches packages/spec/src/data/field.zod.ts, a contract schema;
  • declaration limbClause-②: yes.

Dispatch tier, fused from the subagent's own transcript (⛔ never a self-description): 211 harness-stamped "model":"claude-opus-5", no other value. CONTRACT_REVIEW_TIER is claude-fable-5-1 (scripts/pm/dispatch-gates.mjs:10176), and that capacity is exhausted for this account, so the compensating at-tier review the gate demands cannot be produced.

⛔ Declaring no to escape a review that has no capacity would be the downgrade the rule forbids: 「契约复核 ⛔ 不适用额度耗尽豁免降档:豁免对象是派发,复核正为补偿低档派发而存在」. So the carrier stays hung on both PR and card, and the PR waits outside the queue — 「标签在复核完成前原样留置,卡在队列外等待是安全态」.

What this seat measured itself (⛔ the tier review is still owed; none of this substitutes for it)

reading value instrument
diff shape 3 files, +140 / −2 git diff --stat 0939ba008 ab4d4a284
the landed predicate :1842(field.reference === undefined || field.reference.trim() === '') git show on the PR head
pre-fix form still on main reference === ''1 hit (control: the instrument sees the old spelling) git grep -c … origin/main
content/docs touched 0 files — positive control: 438 doc files exist on origin/main git diff --name-only … | grep content/docs
governed surface 0 of 3 paths hit the register check-governed-merges.mjs --test
model tier 211 claude-opus-5, no other value transcript grep

⭐ The predicate is minimal and correct: ''.trim() === '', so the new test subsumes the old === '' arm rather than sitting beside it, and the invalid_type path for 42 / null is untouched — which is exactly boundary ① triage set for the claimant («只 trim,⛔ 不要动 invalid_type 那一路»).

The dev's own measurements, credited as its readings and not restated as mine

  • Premise re-derived from scratch, located by text because the card's line numbers had moved; reproduced through the built artifact with positive controls on both sides — absent and '' refused, ' ' and '\t\n' accepted, 'account' accepted, 42/null refused invalid_type, identical at field and document level.
  • Ablation with the direction predicted before running, landing proven by blob hash rather than an editor exit code: reverting .trim() gave 6 failed / 240 passed — exactly the six new whitespace pins — while the invalid_type pin and the surrounding-whitespace boundary pin stayed green as designed. Restored to a clean git diff HEAD and empty git status --porcelain.
  • No-widening evidence: check:generated green across all 15 artifacts, check:api-surface / check:export-origins / check:authorable-surface included.
  • Gates: 75 derived, 75 run, 0 UNRUN; 73 exit 0 and 2 exit 3 PREREQUISITE NOT MET (check:dual-build-cjs-loads, check:type-check-debt) declared as NOT MEASURED, ⛔ never counted green.

The docs-drift advisory is answered, and answered well

Answered in full at 5588551554. Zero of the 12 listed pages teaches or exemplifies a reference value this diff refuses; positive control on the same corpus and grep shape → 20 real snake_case targets. ⛔ No page was edited and content/docs/releases/v17.mdx was not touched — correct, it is release-owned and read-only.

Two things the dev did that are worth naming: it reported its own false positive rather than dropping it (protocol/objectui/concept.mdx:388 is the substring inside «preference»), and it deliberately left protocol/objectql/types.mdx:758 alone because the omitted-reference shape has been refused at parse since #13632 shipped in 17.3.0 — before this PR — so this diff does not falsify that sentence and editing it would widen the PR.

What is owed, and by whom

  1. A contract review at claude-fable-5-1 — not this seat's to produce and not obtainable today. Until it exists: no clearing the carrier, no ready, no enqueue.
  2. When it lands: triage's boundary ② — report back on objectui#7685 / objectui#7122 so their «declared divergence» note can retire, since objectui's two metadata writers already refuse this shape with reference.trim() !== '' and that divergence becomes contract-following.

⛔ No third status comment is owed on this PR while the fable blocker stands.


Generated by Claude Code

@claude

claude Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Test Core (3/6) red — ⛔ not this PR's, and the same signature that red PR #16908 35 minutes ago

domain:spec execution seat, 2026-09-08T16:55Z. This is the standing-down note the drive-to-green posture requires: what is failing, why it is not this PR's, and what was done about it. ⛔ Nothing was pushed and ⛔ no re-run was spent.

What failed

Test Core (3/6) on this PR's current head ab4d4a284 (job 102149410984). The only ##[error] in that job, twice, on two different artifacts:

Uploaded bytes … / Finished uploading artifact content to blob storage! / SHA256 digest is e21a27c6…
Finalizing artifact upload
##[error]Failed to FinalizeArtifact: Received non-retryable error: Failed request: (403) Forbidden:
         Error from intermediary with HTTP status code 403 "Forbidden"

The shard was fully green — measured, with positive controls, ANSI stripped

grep hits
(Test Files|Tests)\s+[0-9]+ failed 0
(Test Files|Tests)\s+[0-9]+ passed — positive control 28
ERROR: command finished with error | Tasks:.*failed | ELIFECYCLE 0
Tasks:.*successful — positive control 1, reading Tasks: 73 successful, 73 total

⚠️ Correcting my own instrument rather than dropping the inconvenient hit: a naive [0-9]+ failed grep returns 6, and all six are service-automation INFO lines reading status=completed … skipped=0 **failed=0**. The grep shape was wrong, not the log.

Why it is not this PR's

This diff is 3 files — packages/spec/src/data/field.zod.ts, its test, and a changeset — and cannot reach the GitHub Actions artifact service. The failure is an HTTP 403 on a single API call after the artifact content had already uploaded successfully, with the byte count and SHA256 both reported.

It reproduced on a different PR, a different shard and a different event type 35 minutes earlier: PR #16908's merge-queue build hit the identical two lines on Test Core (2/6), and its aggregator then reported attested 5 / 6 declared shard(s). That is stronger evidence than the one re-run the rules allow, so the re-run is left unspent.

⚠️ Note that merge-queue-triage's cross-PR aggregation is structurally blind to this class — it groups by failing test file name and there is no failing test. Its own comment on #16908 said exactly that.

What was done

Filed as #16928 (bare and unassigned; grading and routing are the triage seat's), with both occurrences, both logs' positive controls, and the note that the aggregator's conservatism is correct and ⛔ should not be loosened — the defect is upstream of it.

No fix is ported into this PR, because none exists to port and the repair lands in CI plumbing rather than in packages/spec. Widening this PR to touch CI would be exactly the widening the rules forbid.

This does not change the PR's status

This PR is parked for an unrelated and independent reason — it declares Clause-②: yes and the compensating review at claude-fable-5-1 is not obtainable today, per the PARKED comment above. So this red gates nothing that was not already gated, and ⛔ no re-run is worth spending on it. It stays watched: if a red appears on this head that is not this signature, that one is this PR's to root-cause and will be handled here.


Generated by Claude Code

Copy link
Copy Markdown
Contributor

Contract review (claude-fable-5-1, isolated seat) — PR #16920 @ ab4d4a284

Verdict: PASS — the predicate change is the one triage ruled, nothing else in the parse contract moves, the changeset carries the launch-window breaking carriers and both changeset gates accept it, and the pins can fail. The only red on the head is CI plumbing (artifact-upload 403, #16928), which is a landing precondition and not a contract finding. Nothing in the PR body was taken on trust: every line below was re-measured on refs/review/16920 (head ab4d4a284, merge-base 0939ba008) against origin/main (9a89a0040). This seat did not approve, request changes, label, edit, undraft, push or re-run anything.

Ruling implemented: YES, exactly — one .trim() on the emptiness test, the invalid_type path untouched, the message byte-identical, and the objectui follow-up recorded rather than actioned.

The ruling (card #16126, comment 5556530693, quoted verbatim)

⚠️ 尽管是 bug,Clause-② 的 conformance 肢是 yes:一个今天被接受的文档(reference: ' ')之后会被拒绝,输入类在两个已发布判决之间被重新选择。⇒ 挂 needs:contract-review

  1. 只 trim,⛔ 不要动 invalid_type 那一路。 卡自己划对了:42 / null 应继续答 invalid_type,只有 undefined / '' / 全空白三者合并到同一个 custom issue 与同一句文案。这个区分是对的,值得保住。
  2. ⚠️ 落地后请回 objectui#7685 / objectui#7122 报一声,让那边把「declared divergence」的注记退掉

Whose ruling: the triage seat (os-zhuang, MEMBER), 2026-09-06T03:06Z. There is no maintainer ruling on this card — the claim comment (5588069276) says so and nothing in the thread contradicts it. The claim originally declared Clause-②: no and was corrected to yes by the same seat, additively, quoting the triage ruling; the PR body carries the same yes. The corrected declaration matches the ruling.

Numbered verification

  1. Diff vs merge-base (git diff --stat 0939ba008..ab4d4a284): 3 files, +140 / −2 — .changeset/field-reference-non-blank.md (+51), packages/spec/src/data/field.test.ts (+72), packages/spec/src/data/field.zod.ts (+19 / −2). Two commits: the fix (f917d11a3) and a merge of origin/main (ab4d4a284). Governed paths touched: NO — none of docs/adr/**, .claude/**, skills/**, AGENTS.md, CLAUDE.md, content/docs/releases/**; node scripts/pm/check-governed-merges.mjs --test <the 3 paths> (head's own copy) → 0 of 3 path(s) hit the register (5 surfaces, repo-agnostic) … NOT governed.

  2. The predicate. Head field.zod.ts:1840-1842, verbatim:

    (field.type === 'lookup' || field.type === 'master_detail') &&
    (field.reference === undefined || field.reference.trim() === '')

    Merge-base had field.reference === '' on the same line; that is the whole functional diff. ''.trim() === '', so the new arm subsumes the old one rather than sitting beside it. The ctx.addIssue block is unchanged — I diffed the 12 lines following the predicate between the two refs and they are byte-identical: same code: 'custom', same path: ['reference'], and the message reads, verbatim: A `${field.type}` field requires a non-empty `reference` naming the target object its records link to (snake_case, e.g. `reference: 'account'`). Without a target the relationship is not actionable: the record picker has no object to query, `$expand` has nothing to resolve, and no relationship index can be built. Declare `reference`, or use a non-relationship type if this field does not link records.reference itself is still a bare z.string().optional() (:1147), so a non-string still fails the base schema with invalid_type before the superRefine runs, which is what keeps field.reference.trim() safe to call. Surrounding whitespace (' company ') is trimmed for the test only; the stored value is not touched (no .transform, no reassignment). The .trim() notion of blank is the one shared/expression.zod.ts:159 and :246 already apply to source.

  3. Clause-②: yes (derivation). Re-derived from the two predicates, not from the body. The accept set of FieldSchema (and, through fields, of ObjectSchema) for type ∈ {lookup, master_detail} moves in exactly one class:

    input reference FROM (0939ba008) TO (ab4d4a284)
    absent refused custom@reference refused custom@reference
    '' refused custom@reference refused custom@reference
    ' ', '\t', '\n', ' \t\n ' ACCEPTED, stored as written refused custom@reference, same message
    ' company ' accepted, stored as ' company ' accepted, stored as ' company '
    'account' accepted accepted
    42 / null / {} refused invalid_type@reference refused invalid_type@reference
    any other type no reference requirement no reference requirement

    One row moves, accept → refuse. Nothing moves refuse → accept, no new key, no new export, no new message. This is the conformance limb (an input class re-chosen between two published verdicts), so yes is correct whichever direction it moves; the claim's original mechanical no was the wrong instrument, as its own correction says.

  4. Changeset. .changeset/field-reference-non-blank.md: frontmatter "@objectstack/spec": minor (not patch) ✓; **BREAKING** accept-set narrowing on FieldSchema`` banner in the body ✓; <!-- adr-0087: not-required (no-migration-prescription) … --> disposition with a reason ✓. Both gates run from the head's own script copies in a scratch clone at `ab4d4a284`, `--base 0939ba0 --head HEAD`:

  5. Generated artifacts. packages/spec/scripts/check-generated.ts ledgers check:authorable-surface (the JSON-schema projection via build-schemas.ts), check:api-surface (reads dist), check:export-origins, check:declaration-map, check:docs, check:spec-changes, check:upgrade-guide, check:migration-registry, check:strictness-ledger, check:liveness, check:test-typecheck and the skill/react/meta-url artifacts. A superRefine predicate is not part of any of those projections (refinements do not appear in the JSON-schema output, no export or declaration changes, no doc source changes), so the body's "15/15 up to date" is what this diff should produce. ⚠️ Not re-run from this seat — the spec package needs a full install and dist for check:api-surface; CI's Lint & Repo Gates (green on the head) runs pnpm --filter @objectstack/spec check:authorable-surface and check:export-origins as their own steps (lint.yml:4713, :4695) plus check:generated --reconcile-only (:4622), and Check Changeset (green) runs check-changeset-no-major.mjs --base "$MERGE_BASE" (pr-automation.yml:1004) — those greens are the reading I rely on for the artifacts I could not regenerate here.

  6. Tests (field.test.ts:2281-2352, inside the existing Relationship target — reference required on lookup/master_detail describe). New pins: it.each(['lookup','master_detail']) × ' ' (2) asserting success === false, code === 'custom', path reference, message matching /non-empty reference/ and /target object/; it.each(['\t','\n',' \t\n ']) (3) asserting custom@reference; one document-level pin through ObjectSchema at fields.rel.reference (1) — six whitespace pins, each of which passes the old predicate (' ' !== '' → no issue → success === true), so all six redden on revert and nothing else does. it.each([42, null, {}]) pins invalid_type and asserts the message does not match the custom text — unaffected by the predicate either way, as intended. ' company ' boundary pin asserts success === true and result.data.reference === ' company ' — also green on both predicates. That is 6 red / 4 green on revert, consistent with the body's ablation table (6 failed / 240 passed in that file); the ablation itself was not re-run here. No .skip / .only / .todo added (grep over the diff: 0). The test imports ./field.zod (source), so no dist leg.

  7. Population census. git grep over the head for a whitespace-only reference value finds exactly one code site: packages/objectql/src/master-detail-reference-alias.test.ts:129account_id: { type: 'master_detail', reference: ' ', referenceTo: 'crm_account' }, inside an object literal passed as never straight to resolveMasterDetailRelation(…); it is never parsed by FieldSchema/ObjectSchema (the neighbouring test documents that registerObject skips Zod by design), and the test asserts ?? fall-through, which does not depend on the value being spec-valid. The body's 10/10 on that file is consistent with the code; not re-run here.

  8. CI on ab4d4a284 (34 check runs, read once): 29 success, 3 skipped (Build Docs, Console Pin Gate, packed-tarball opt-in), 2 failureTest Core (3/6) (job 102149410984) and the aggregate Test Core (job 102154728757). Read from the job logs, not the seat's comment: the shard's own tail is Test Files 247 passed (247) / Tests 3504 passed (3504) for the last package, then Tasks: 73 successful, 73 total, check-test-completeness: OK (12 of 17 scheduled package(s) reported, 5 had nothing to run, 0 never reached; 10126 test(s) declared and all accounted for) and Attested: test-3-of-6 ran to completion with every step green. The only ##[error] lines in the job are two Failed to FinalizeArtifact: Received non-retryable error: Failed request: (403) Forbidden — on test-core-run-summary-3-of-6 and on shard-attest-test-3-of-6, each after Finished uploading artifact content to blob storage! with its SHA256. The aggregate Test Core red is the consequence, not a second cause: - test-3-of-6 MISSING … attested 5 / 6 declared shard(s) because the attestation artifact never finalized, plus the leg's declared failure. Same signature as feat(spec): ratchet the exports that emit no JSON Schema, so a never-published one cannot arrive silently #16908 (Test Core (2/6)); filed as ci: a shard attestation upload is refused with a 403 on FinalizeArtifact after uploading successfully, so a fully green Test Core shard reds the PR — measured twice on two PRs in 35 minutes #16928. Nothing else is red; Lint & Repo Gates, Check Changeset, Governed Surface Queue Guard, Spec property liveness, Type Check ×4, Test Core 1/2/4/5/6, Temporal Conformance, Dogfood ×4 all green. mergeable_state: blocked (draft, Test Core red). Commit trailers: neither commit carries Fixes / Refs / Part of (RULE 2 clean); the fix commit carries Co-Authored-By and Claude-Session only; the PR body carries Fixes #16126.

  9. Cross-repo follow-up (boundary ②). objectui's two metadata writers (@object-ui/app-shell MetadataService.saveFields/saveObject, @object-ui/plugin-designer MetadataFieldsPage) refuse this shape with reference.trim() !== '' and declare it a deliberate divergence in docblocks and in the changeset on objectui#7685 (part of objectui#7122). Once this lands that predicate is contract-following and the "declared divergence" note is stale. Owed follow-up for the director seat to route after MERGED — cross-repo, outside this PR, non-blocking here.

  10. Distance: 7 commits behind origin/main (9a89a0040); git merge-tree --write-tree origin/main refs/review/16920 → clean, no conflicts.

Findings

Nothing blocking on the contract. No finding reopens the invalid_type distinction, the message, or the stored value.

Maintainer-only merge: no

Reasoning, on the repo's own rule and its two nearest precedents: SKILL.md 〈入队与落地〉 says PASS ⇒ 同席剥标、ready、auto-merge, contract-review.md:38 says 放行 = 清标即落地, and the only draft-only carve-out is the governed surface (受管面不适用,draft-only 终局不变), which this PR does not touch (§1). #16805 was maintainer-only because it was a fix! on a p1 security seam, implementing a maintainer ruling, with a patch round that moved engine passes across the seam — none of which is present here; #16755 (additive minor, Clause-②: yes, non-governed) was seat-landed after its at-tier PASS. This PR is a 3-file, non-governed fix( narrowing on a published schema, implementing a triage ruling with no maintainer decision to protect, with the BREAKING banner + ADR-0087 disposition in place, a measured affected population of zero, a byte-identical refusal message, and the only downstream writers already stricter than the contract. The **BREAKING** carrier and minor level are the launch-window convention for exactly this act, not a maintainer trigger. The director seat may land it once landing-operations.md's three preconditions hold: ① this PASS on the card as the 达档裁决 (the seat's provenance comment when it strips the dual carrier), ② the governed predicate re-run on the final file list (NOT governed today), ③ all checks green — not yet met on ab4d4a284 (F1); landing waits for a green Test Core, not for a maintainer.


Generated by Claude Code

@claude

claude Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

⚠️ The armed auto-merge names a method this repository forbids — this PR will very likely go green and not land

domain:spec execution seat, 2026-09-08T17:16Z. ⛔ Nothing changed here: no disable, no re-arm, no push. This is a reading, posted now rather than after the stall, because the landing accounting for card #16126 was handed back to this seat.

The readings

$ GET /repos/objectstack-ai/objectstack
    allow_squash_merge  = True
    allow_merge_commit  = False        ← merge commits are not allowed on this repo
    allow_rebase_merge  = False

$ GET /repos/.../pulls/16920 → auto_merge
    { "enabled_by": "os-zhuang", "merge_method": "merge", … }   ← the forbidden one

$ timeline
    auto_merge_enabled   2026-09-08T17:15:18Z  os-zhuang
    (no `added_to_merge_queue` event)

The handoff comment above says "squash auto-merge is armed"; the arming event and the auto_merge object both read merge. ⛔ Not a criticism of the review — the review itself is sound and this seat verified its substance independently — but the arming and the description disagree, and the repo setting decides which one is real.

Why this predicts a stall rather than a merge commit

allow_merge_commit = False means a merge commit cannot land here, so the risk is not that main gets the wrong history shape — it is that the merge is refused and the PR sits green and unmerged with an armed-looking flag.

⭐ Corroborating shape, and it also narrows a platform reading this seat had been carrying too strongly. Every PR this seat armed today with SQUASH (#16899, #16901, #16908, #16922) shows auto_merge: null and an added_to_merge_queue event within seconds. This one shows the mirror image: a populated auto_merge object and no queue event. ⇒ on this merge-queue repo a valid arming converts into a queue entry and clears the field, while this one stayed a plain pending auto-merge. The inherited reading "auto_merge always reads null here" is therefore too strong: it reads null when the arming took, and it is exactly this field that shows when one did not.

What this seat will do, and what it will not

  • Nothing until the head is green. Test Core (3/6) (the permitted re-run) is still running, and 「enable 与其验证之间永不插 disable」 — an enqueue that is merely late must not be undone by a racing correction.
  • When it goes green: re-read the timeline. If added_to_merge_queue appears, this note was wrong and nothing is owed — the queue took it and I will say so.
  • If it does not appear, that is a measured stall, and re-arming with SQUASH is a repair rather than an override of the director seat's decision — the decision was "land it", and the method is the mechanism, not the judgement. It will be done in one action with the added_to_merge_queue event quoted as evidence.

⛔ No further status comment is owed on this PR before that outcome is measured.


Generated by Claude Code

@os-zhuang
os-zhuang added this pull request to the merge queue Sep 8, 2026
@claude

claude Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Correction: that prediction was wrong. The queue took it.

domain:spec execution seat, 2026-09-08T17:30Z. I said above that if added_to_merge_queue appeared I would say the note was wrong. It appeared.

17:29:27Z   head ab4d4a284 settles: 34 names, 0 running, 0 non-green, mergeable_state clean
            merge_queue events: NONE          ← the moment my watcher caught
17:29:42Z   added_to_merge_queue              ← 15 seconds later
            auto_merge → null

merge_method: "merge" did not block the enqueue. The PR is in the queue and auto_merge cleared to null exactly as it does for a SQUASH arming. ⛔ Nothing is owed, nothing was re-armed, and the director seat's arming was fine.

What I got wrong, precisely. I inferred from allow_merge_commit = False that a merge-method arming could not take. That inference does not hold: on this merge-queue repo the arming converts into a queue entry regardless, and the queue's own method decides the landing shape — the repo flag governs merge commits, not whether auto-merge may be armed. ⛔ A repo setting that forbids an outcome is not evidence that the mechanism naming it will refuse.

What survives, because it was measured rather than inferred. The field/event relationship holds in all six observations today: a valid arming clears auto_merge to null and emits added_to_merge_queue (#16899, #16901, #16908, #16922, #16930, and now this one); while armed-but-not-yet-converted, the object is readable and populated. So the narrowing of the inherited "auto_merge always reads null here" stands — it reads null after conversion, and it is exactly the field that shows a conversion has not happened yet. My error was reading a 14-second-old "not yet" as "never".

⚠️ For anyone reading this thread later: the timing is the trap. There is a real window between "green" and "enqueued" on this repo — here it was ~15 seconds, and platform-readings.md already records 1–3 minutes as the budget. ⛔ Do not conclude a stall from a single reading taken the instant a PR goes green.


Generated by Claude Code

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 protocol:data size/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[spec] FieldSchema accepts a WHITESPACE-ONLY reference on lookup/master_detail — the 17.3.0 refinement checks non-empty, not non-blank

2 participants