Skip to content

fix(reference-carrier): four C2 readers answer absence instead of propagating an unreadable reference - #19197

Merged
huangyiirene merged 3 commits into
mainfrom
claude/issue-19081-reference-carrier-narrowing
Sep 19, 2026
Merged

huangyiirene merged 3 commits into
mainfrom
claude/issue-19081-reference-carrier-narrowing

Conversation

@huangyiirene

Copy link
Copy Markdown
Collaborator

Fixes #19081

Clause-②: no

Four readers of FieldSchema.reference gated the carrier with a truthiness test, which a non-string object passes, and then propagated the value onward. The declared contract is an optional string, so the answer a reader owes for a carrier it cannot read is absence. The approvals site did worse than lose the information — out.push({ key, reference: String(f.reference) }) manufactures the literal target name [object Object], and hands it on as an object name to engine.find(), where the failure disappears into the caller's own catch.

These four are the class PR #18503 labelled C2 and deliberately left unchanged. This PR acts on that class; it is not a claim that #18503 or #19080 was wrong to leave it, and it establishes no reachability from an authored document — the engine's write path refuses this shape, so a row carrying it had to be written straight through the driver. Cheap insurance, not an incident.

The design question the card leaves open, and what decided it

Whether these sites should route through the arbiter referenceCarrierOf or narrow locally. Measured first, because the arbiter does not return the same narrowing a local typeof test would: it throws a TypeError. That is extra knowledge, and at these four sites it is hostile knowledge:

site what an escaping throw would do
ApprovalService.resolveLookupFields the whole body is inside try { … } catch { return []; }, so one unreadable field would drop every lookup field of the object
analytics relationship resolver a TypeError inside dataset compilation, replacing the compiler's own "cannot resolve this relationship" refusal with a crash
os doctor (both sites) aborts the diagnostic run on exactly the broken metadata doctor exists to report

So the repair is both: the carrier is read through the one arbiter — no fourth hand-copy of the narrowing, and '' / null / undefined keep their absence semantics — and the refusal is caught at the site, which yields absence plus one report. That is the deliberate line between these readers and the @objectstack/objectql cascade seams #19080 landed, which let the same refusal propagate: those assert something positive about the schema on a write path, where the silence cost an orphaned master_detail row and a reported success.

Per site

  • @objectstack/plugin-approvals — the unreadable field is left out of the inbox display enrichment and logged through the service's existing logger.warn. It is dropped rather than pushed with the target absent because the sole consumer destructures { key, reference } and uses reference as the object-name argument to engine.find; an entry carrying none has nothing for that consumer to do, and keeping it would widen the declared return type for no reader. The effect there is the one this best-effort resolver already produces for every other unresolvable case — the entry stays unresolved.
  • @objectstack/service-analytics — the ADR-0021 relationship resolver answers undefined, which its existing fallback turns into the dataset compiler's refusal, plus one ctx.logger.warn naming the field.
  • @objectstack/clios doctor's circular-dependency and unused-object checks report the unreadable carrier as a finding rather than skipping it. Both publish a positive verdict — "No circular references detected", "defined but not referenced" — that an edge nobody could read cannot support, and both already return diagnostic strings that doctor prints as warnings, so being loud here needed no new channel. The same file's collectViewObjectRefs already narrowed its own carrier.

Tests, and the ablation behind them

Each of the four sites has a case that fails before the change and a readable-target control beside it, so "narrowed" and "this path is now closed" stay distinguishable. The [object Object] string is pinned directly, since it is the card's whole evidence.

Ablation: each narrowing was reverted to its truthiness gate through scripts/ablation-replace.mjs (anchor must hit; on-disk counts and blob hashes are the tool's own verdict), the pin re-run, then restored. Every site is source-imported by its suite, so no dist round trip is involved. Restores verified by the tool: blob == HEAD and git diff HEAD empty.

site ablated run
approvals 3 failed / 1 passed — expected [ 'crm_account', …(2) ] to not include '[object Object]'
analytics 2 failed / 1 passed — expected { name: 'shop_invoice', fields: {} } to be undefined
doctor (both) 2 failed / 3 passed — the two carrier findings vanish

Verification

All commands run at 1130dc81b.

  • pnpm --filter @objectstack/plugin-approvals --filter @objectstack/service-analytics test — exit 0; 48 files / 775 tests and 113 files / 2411 tests.
  • pnpm --filter @objectstack/cli exec vitest run --project unit — exit 0; 218 files / 3069 tests. The integration tier is declared to CI: the diff touches no integration-tier test file, no bin/ entry and no driver or kernel boot path.
  • pnpm --filter … typecheck for the three packages — exit 0, test layer included.
  • Gate family re-derived for the actual diff with scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack: 64 families, each run with its exit code landed to a file, then reconciled with --ran: 60 run, 4 NOT MEASURED, 0 UNRUN. The four are check:dual-build-cjs-loads, check:i18n, check:i18n-coverage and check:i18n-walk-parity, each exiting 3 = PREREQUISITE NOT MET in this container because they read whole-tree built output. Not a pass — declared to CI.
  • eslint . --no-inline-config — exit 0 over the 6900 files eslint's own config reads, 0 errors / 0 warnings. Type-aware linting is not enabled in eslint.config.mjs, so no untouched file's verdict can move with this diff; this is the full population rather than a narrowing.

Acceptance notes

  • packages/cli/src/commands/doctor.ts's detectCircularDependencies gained an export so its carrier reading is assertable; it has no other caller, and doctor.ts is not a declared entry in this package's exports map, so nothing is added to the published API surface. That is the only change in the file beyond the two gates the card names.
  • Census re-run. The card's reading — one referenceCarrierOf hit across packages/objectql, plugin-approvals, service-analytics and packages/cli, and it is a test — no longer holds for that four-package corpus: packages/objectql/src/engine.ts now carries four production hits, landed by Refuse an unreadable reference carrier at the ten residual readers (ruling E item 2 residue) #19080 after the card's ref. Inside this PR's three packages the zero stood (only packages/cli/test/data-model-rules.test.ts), with the token resolving in 20 files repo-wide as the live control.
  • Noted, not filed: collectViewObjectRefs (same file) narrows with a bare typeof test, so it admits '' as a target name and reports nothing when a carrier is unreadable. Out of scope here — it is not one of the four propagating sites, and it already answers absence rather than propagating.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AhQASwqJr2Z7XfGWUdvnbF


Generated by Claude Code

…ating an unreadable `reference`

`FieldSchema.reference` is declared an optional STRING. Four readers gated it
with a truthiness test, which a non-string object passes, and then propagated
the value onward — the approvals one through `String()`, which manufactures the
literal target name `[object Object]`.

Each site now reads the carrier through the one arbiter, `referenceCarrierOf`,
and catches its refusal AT THE SITE so the reader yields absence and reports,
rather than aborting: these are best-effort display and diagnostic readers, not
the cascade seams that let the throw propagate on a write path.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AhQASwqJr2Z7XfGWUdvnbF
…adable control

Each site gets a case that fails before the narrowing — the approvals one pins
the literal `[object Object]` directly, since that string is the whole evidence
— and a control that keeps a readable string target flowing, so "narrowed" and
"this path is closed" stay distinguishable.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AhQASwqJr2Z7XfGWUdvnbF
@github-actions github-actions Bot added size/m documentation Improvements or additions to documentation tests tooling labels Sep 19, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 3 package(s): @objectstack/cli, @objectstack/plugin-approvals, @objectstack/service-analytics, touching 8 documentable anchor(s).

30 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: node scripts/docs-audit/affected-docs.mjs --json d402e328552622e3b421d567e793d4b77c324c44.

7 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails.

What this run could not see
  • the SDK route bridge reached 60 of 215 client-bound route-ledger rows — the other 155 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 155: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 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.
  • a key NAME is not a key, so the hand re-read the line above prescribes can land on the wrong schema. The same spelling is authorable on one governed type and a [REMOVED] tombstone on another for each of active, aria, joins, objects, template, tools and version (censused on [finding] tools is a key on BOTH AgentSchema (tombstoned, dead) and SkillSchema (live, cloud-attested), so a name-based search attributes skill examples to the agent key — it produced a false stop-the-line alarm on PR #19059 #19093 over the liveness ledger's governed types, top-level keys); nothing in a search result distinguishes the two, so a grep hit on a LIVE example reads as evidence about the DEAD key. Measured on fix(spec): the agent.tools liveness row says dead — it claimed live on a key the schema tombstoned #19059: content/docs/ai/agents.mdx was reported as contradicting the agent.tools tombstone over its tools: example at :161, which is inside the defineSkill({ block opened at :155 — the page was already correct. Settle ownership by PARSING the value against both schemas, never by the name: that literal PASSES SkillSchema, and as an AgentSchema it FAILS at tools with the tombstone prescription. ⛔ These names are not the whole class — a key retired through a .strict() guidance map leaves no tombstone in the walked shape and none of them here (tool.category, live as AIToolDefinition.category).

Coarse fallback — 33 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 d402e328552622e3b421d567e793d4b77c324c44packageMentionDocs.

Which tree this was computed on

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

node scripts/docs-audit/affected-docs.mjs --json d402e328552622e3b421d567e793d4b77c324c44

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

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

2 participants