Skip to content

One projection pipeline and the Direct execution adapter - #5919

Open
habdelra wants to merge 7 commits into
mainfrom
cs-12602-one-projection-pipeline-and-the-direct-adapter
Open

One projection pipeline and the Direct execution adapter#5919
habdelra wants to merge 7 commits into
mainfrom
cs-12602-one-projection-pipeline-and-the-direct-adapter

Conversation

@habdelra

@habdelra habdelra commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Every execution tier is supposed to answer with the same records for the same card. The way that stops being true is not a disagreement anyone can see — it is a second builder. Competing builders each agree with themselves, so nothing goes red until two of them are compared, and by then every consumer has grown a preference for one of the answers.

So this lands exactly one pipeline, in two halves.

The capture (boxel-projection.ts) is everything that touches a live object: the Card API, the Loader, an instance's own getters. It reads a type or an instance once and answers with values — a plain graph of strings, numbers, booleans, nulls, arrays and plain objects. One capture is what makes a record internally coherent: an instance's field list and its model come from a single read, so a card whose fields and values were read at different moments never describes a state the instance was never in. Each operation captures the one record it answers with, so none of them pays for the other two.

The assembler (boxel-render-record.ts) is pure. Its whole input is what the capture produced, so given one capture the records are a function of that capture alone. It is where two guarantees get established once instead of at every call site: the protocol envelope, and inertness — each record is rebuilt through the protocol's own normalizer, so an accessor, a class instance, a symbol-keyed member, or a value containing itself is refused where it was produced, with the offending member's origin still in the stack, rather than at the far side of a message port where the diagnostic names only a path.

The rules the pipeline holds are the ones the spec names. Linked cards cross as {$boxel:{id,type}} identity and nothing else — a card linking a card linking a card hands its recipient no graph to walk. Contains composites expand in place, because they are embedded data and not a separate resource. Only a present link slot yields a reference: the other four states have a reference string but no loaded value and therefore no actual class, and every trust decision follows an instance's actual class rather than a field's declared type. Configuration resolves against the owning instance, and a type reports none rather than an unresolved one, because resolution has no this to run with. A themed card's theme members are derived Host-side, since resolving the linked Theme is precisely the graph walk a projection forbids — by calling Base's own theme helpers, which this PR lifts out of getBoxComponent and exports rather than reimplementing, so there is one spelling of that derivation in the tree. A trusted Base value's plain prototype getters are evaluated and carried, because a plain getter is not a field and a tier holding only the field-derived view of a currency amount renders a different card.

DirectBoxelRuntime implements the runtime contract over today's trusted Loader and Card API. It is the reference implementation, not a bypass: everything it answers with goes through that one pipeline, so a semantic the operations cannot express is an incomplete interface — and here, in the tier that holds the live objects and could cheat, is where that has to be found out, before a tier that cannot cheat is built against the gap. It loads through the Host's own Loader, so a module reaches it already through transpileJS() with its scoped CSS delivered by that pipeline's side-effect import; there is no second stylesheet compiler here. It retains the canonical Store instance rather than re-deserializing one, which would produce an object with the same data and none of the identity, lazy relationships, or mutation context. Rendering sits beside the interface rather than in it, because a mountable component is process-local and not cloneable.

purpose reaches no decision in Direct, and that is the honest answer for this tier rather than an omission — main fails loudly on a definition it cannot identify whatever the caller wanted it for, and there is no purpose Direct could be lenient for. A test pins that: the same materialization rejects under host-display and under indexing.

A development-only diagnostic rounds it out. A member the pipeline failed to project is not an error anywhere — it reads as undefined, the binding renders empty, and the card is subtly wrong with nothing in any log. watchProjectionPaths hands back a view that reports the complete path, the type, the format, and the execution mode when a consumer reads a path the record does not carry. It never synthesizes a value, and in a production build it is the identity function, so no proxy exists and no read is intercepted. It is offered rather than applied to what projectInstance returns: a record that crossed a boundary as a wrapper would be neither cloneable nor comparable.

Two changes outside the pipeline. FieldDescription and ResolvedField gain isQueryBacked, and the spec is amended to match: render-time writability is (not computeVia) ∧ (not queryDefinition) ∧ permissions.canWrite, the permissions term is context the Host pushes per surface, and a record carrying only isComputed cannot state the rule at all — a query-backed relationship is never editable and is not computed. resolveFieldConfiguration becomes a public Card API export, which is what lets the capture resolve configuration through the module the Loader served rather than a copy of its merge semantics.

The tier registry now records the adapter's file against the direct mode, and both the unit and the integration suite hand Direct's records to checkRecordParity with direct registered — which with one tier still checks something real, since every record it is given is read as data.

Nothing renders through any of this. No call site changed.

Testing. All green locally against a dev stack: 21 integration tests over a real realm module, a real Store-resident instance, and real links (62 assertions) — covering the theme derivation, a trusted Base getter reached through an authored subclass, a query-backed field, and that a projection triggers no load of its own; 10 unit tests on the assembler; 6 on the diagnostic; and the 130 existing rendering-protocol unit tests, whose fixtures pick up the new field member. lint:tier-registry, lint:protocol-closure, lint:rp-bijection, host lint:types and eslint all pass. The bijection's uncovered-statement count drops from 104 to 88.

Three interface gaps this surfaced, each recorded rather than papered over, because finding them here is what building Direct first is for. Nested-composite field configuration is not addressable through the operations at all: getFields answers a flat list keyed by a root-level name, so a tier rendering venue.name cannot obtain its @configuration. A tier that renders from records has nothing to trigger a lazy load with, since it never reads a getter on the canonical instance — so the rule that the renderer is the trigger holds for Direct and not beyond it. And query-backed whole-field states have no spelling in the record: a search that has not started projects the same empty membership as one that matched nothing. All three want protocol changes rather than adapter workarounds, and are tracked for the tickets that own those records.

The pipeline has two halves: a capture that reads a live type or instance
into inert values, and a pure assembler that turns one capture into the
protocol's records. Splitting them is what makes the assembler checkable,
and one capture is what keeps describeBoxel, getFields and projectInstance
from being able to disagree about the same card.

Every record is rebuilt through the protocol's own normalizer at assembly, so
an accessor, a class instance, or a value containing itself is refused where
it was produced rather than at the far side of a boundary.

DirectBoxelRuntime implements the runtime contract over today's trusted
Loader and Card API. It loads through the Host's own Loader, so a module
reaches it already transpiled with its scoped CSS delivered; it retains the
canonical Store instance rather than re-deserializing one; and rendering
stays beside the interface, since a mountable component is not cloneable.

FieldDescription and ResolvedField gain isQueryBacked, because render-time
writability is (not computeVia) and (not queryDefinition) and canWrite, and
a record carrying only isComputed cannot state that rule.

Nothing renders through any of this yet.
… the file

A row's index does not make a gap distinct: a grid of a thousand rows reads
the same missing member a thousand times at a thousand paths, which is the
case the reporter exists to survive. The warning still names the concrete
path that reached it.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 15f4342380

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/host/app/lib/boxel-projection.ts
Comment thread packages/host/app/lib/boxel-projection.ts Outdated
Comment thread packages/host/app/lib/direct-boxel-runtime.ts Outdated
@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Preview deployments

Host Test Results

    1 files      1 suites   2h 38m 59s ⏱️
4 628 tests 4 614 ✅ 14 💤 0 ❌
4 647 runs  4 633 ✅ 14 💤 0 ❌

Results for commit bd585d6.

Realm Server Test Results

    1 files      1 suites   13m 33s ⏱️
2 348 tests 2 348 ✅ 0 💤 0 ❌
2 431 runs  2 431 ✅ 0 💤 0 ❌

Results for commit bd585d6.

A projection reads every field of an instance and of every composite inside
it, so using the field getter as its trigger fetched links no render asked
for. That is worse than wasteful: a failed fetch is terminal, so one
projection could permanently break a link nothing was rendering; a nested
composite is registered in no store, so its load ran against a throwaway
fallback and resolved nowhere; and a query-backed field's getter issued a
search per projection. Membership is now read purely and the renderer stays
the trigger, which is what RP-7.2 actually says.

Other corrections in the same pass:

- The root instance's own trusted getters reached no model at all, so a
  trusted Base card lost every member reachable only as @model.x.
- The trusted-getter walk stopped dead at an authored prototype rather than
  stepping over it, so an authored subclass of a Base value contributed
  none of the Base getters its own trusted templates read. Names the
  authored class defines stay the author's.
- A present link to an unsaved card projected as {id: null}: unresolvable,
  and identical for every unsaved sibling. Membership carries a resolvable
  reference and it is used.
- serializeCard omitted omitQueryFields, which every other call site sets.
  A frozen search snapshot in relationships becomes a declared link to
  whoever materializes the document.
- getFields built a whole render record to answer a question about
  declarations, so one field's configuration could not be read when an
  unrelated computeVia throws. Only projectInstance counts as a projection
  now, which is what makes revision comparable across tiers.
- dataValue fabricated nulls for non-data array elements and dropped
  non-data members, and read through Object.entries, which invokes an
  accessor the boundary exists to refuse. A container is data only if
  everything in it is.
- cssImports were dropped for a Theme card authored fonts-first, and were
  read only off the theme source where main asks every card first.
- Handle ids were per registry, so two runtimes of one tier both minted
  direct-instance:1 and a handle from either resolved in the other.
- The diagnostic proxy reported toJSON as a missing path on every node, so
  serializing a watched projection buried the real gaps, and it allocated a
  fresh wrapper per read.

Tests now cover the theme derivation, a trusted Base getter, a query-backed
field, and that a projection triggers no load of its own.
Its symptom is every field on every card reporting no configuration, which
reads as "these cards configure nothing" and sends a reader to the cards
instead of to which Base realm served them.
dataValue used undefined as its "not data" answer, so one unset member
deleted the whole container it appeared in. That is stricter than the
boundary it mirrors: normalizeJsonData deliberately carries an
undefined-valued member, and an unset primitive field's empty value IS
undefined — so a trusted getter that maps over its own fields produces one
for every field an author left blank, and a partly-filled typography field
projected nothing at all. A sentinel separates the two; refusing a container
that holds something which genuinely cannot cross stays.

Also in this pass:

- The claim that membership is read and nothing is triggered was false for a
  computed link: reporting its membership has to run the compute, and
  CardDef.cardTheme's reads cardInfo.theme. Main reads cardTheme on every
  card render, so that fan-out is main's; the comment now says which part is
  ruled out and which is not.
- A linked Theme fell back to no identity where a link's slot does, so an
  unsaved one projected as {id: null}.
- cssImports fell back to the theme's whenever the card's own were empty.
  Main branches on whether the card declares the field at all.
- describeBoxel reported an unknown instance handle as an unknown type one.

The test guarding the trigger removal could not fail: it wrapped
globalThis.fetch, which the VirtualNetwork captured at boot and which realm
traffic never reaches. It now reads membership before and after projecting —
verified to fail with the trigger restored. Added coverage for the
root-level trusted-getter path through an authored subclass, which nothing
exercised.
@habdelra
habdelra requested a review from a team August 27, 2026 16:13

@backspace backspace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] This review went after the capture half's fidelity to what main actually reads, the twin implementations the pipeline introduces, and the type boundaries the as casts cross. It did not run the adapter against a realm stack or re-derive the spec statements from the doc.

No blocking issues. Five things to settle, all non-blocking. The one worth settling before a consumer exists is the silent null for a value that could not cross — a tier holding the record cannot tell it from an empty field, and the diagnostic built for exactly that class structurally cannot see it.

From the earlier round: the link-identity fallback and the trusted-getter walk both hold under the paths I traced. The getFields fix removes the cost it named, and I've continued in that thread — the same cost still stands on the other side of it, and the module header now asserts an invariant the fix removed.

  1. bigint has no spelling in dataValue, so a BigIntegerField projects as null, and notData is collapsed to null at the one call site the sentinel exists for — thread on dataValue in boxel-projection.ts.
  2. A query-backed link's never-queried / in-flight state projects as [], identical to a resolved-empty search; a whole-field search failure projects as [null] — thread on linkSlots.
  3. captureTheme re-implements five module-private helpers from field-component.gts, and the two copies already differ — thread on captureTheme.
  4. serializeCard's as never crosses BaseDefCardDef, which retainInstance and boxelKindOf both make reachable — thread on serializeCard in direct-boxel-runtime.ts.
  5. projectInstance builds and discards a full description and field list — in the existing getFields thread.

Comment on lines +956 to +963
if (value instanceof URL) {
return value.href;
}
if (value instanceof Date) {
return value.toISOString();
}
if (typeof value !== 'object') {
return notData;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] bigint falls through this ladder to notData, and captureValue maps notData to null — so a BigIntegerField (static [primitive]: bigint, packages/base/big-integer.gts) projects as null on every tier, with nothing anywhere saying so. watchProjectionPaths cannot catch it either: it reports paths the record lacks, and model.<field> is present, holding null.

Two asks, separable.

bigint wants a spelling here, the way URL and Date two lines up have one — BigIntegerSerializer's decimal string is what every other view of that value already uses. (ResponseField's Response is the other non-JSON static [primitive] in Base; that one presumably should drop, which is an argument for making the drop visible rather than for widening the ladder.)

And notData reaching captureValue should be observable. The sentinel's own docblock says it exists so a caller "can tell 'this could not cross' from 'this is absent' and from 'this is null'", and captureValue is the one call site where that distinction has consequences — it discards it. captureTrustedGetters already does the alternative: omit the member. Omitting it here would route the read into the diagnostic's missing-path report instead of a silent null.

Regression, non-blocking — but the second half is worth settling before a consumer exists, because a tier holding the record has no way back to the difference.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Both, and the second was the more useful half. Fixed in d69d0b4.

bigint has a spelling now — String(value), which is what BigIntegerSerializer.serialize writes, so a projected value and a serialized one read the same.

And notData no longer collapses to null at a member: a field whose value cannot cross is absent from the model, so reading it routes through the missing-path diagnostic instead of looking like a field that genuinely is null. One deliberate exception — a containsMany element keeps null, because a position is an element's identity to anything iterating it and omitting one would renumber its siblings. That leaves ResponseField as the case that drops, visibly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Both halves land.

BigIntegerSerializer.serialize is val == null ? undefined : String(val), so the spelling matches the serializer exactly rather than approximately. And omission does reach the diagnostic — watch's !(key in target) is true for a member defineMember never created, so the read reports the path. It also composes with the parity harness, which already carries 'absent' as its own divergence reason, so a tier that omits a member and one that carries it part visibly.

One thing the change did that the reply does not mention. Dropping ?? null from captureValue means an unset primitive field now projects as a present member holding undefined rather than nullContains.emptyValue hands back the class's [emptyValue], which is unassigned on BaseDef and so undefined for everything but Boolean. That gives the model the three-way split the notData docblock always claimed and nothing implemented: member absent (could not cross), present and undefined (unset), present with a value. Better than what I asked for — and it is the model's spelling for "empty" changing under every future consumer, with nothing in the suite reading an unset contains field. Worth a line in captureModel's docblock or a pinned assertion, because the next reader will assume null.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] You are right that it was a semantic change I made without noticing, and pinning it turned up that my own account of it was too broad. Fixed in bd585d6.

A card served by a realm carries its unauthored primitives as null, so a store-loaded instance projects null, not undefined — the undefined case is narrower than "unset": it is a field the instance's data never carried at all. My first attempt at the test asserted on a realm-loaded card and failed for exactly that reason. It now materializes a document carrying one attribute and asserts the live instance and the model agree, and the docblock states which of the two cases yields which.

Comment on lines +498 to +503
let { membership } = api.getRelationshipMembershipState(
instance as CardDef,
fieldName,
);
return (membership ?? []).map((slot) =>
slot.kind === 'present'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] getRelationshipMembershipState answers membership: undefined for a query-backed field that is in flight or was never queried — the probe supplies queryMembership only once the search resource exists and has settled. ?? [] turns all of that into [], which is also what a search that resolved to nothing produces. A tier iterating @model.<queryField> renders "no results" for a query that has not run.

The same read has a second case: a search that failed as a unit surfaces one whole-field sentinel, so membership is a one-element array whose slot is not present — which maps to [null]. A template iterating that renders one empty row where main renders the broken-link presentation.

The docblock enumerates RP-7.1's five slot states carefully and reaches neither of these, because both are whole-field states. Worth deciding what a query-backed field projects across never-queried / in-flight / resolved-empty / failed and saying so; isLoading is already destructured off this call and dropped.

The integration fixture has a query-backed revivals, and it is only ever asserted through getFields — nothing asserts model.revivals.

Regression, non-blocking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Confirmed both, and the docblock did claim coverage it did not have. Addressed in d69d0b4 as documentation plus a pinning test rather than a shape change.

membership: undefined and the whole-field failure sentinel are exactly that — whole-field states — and RP-7.1's union describes slots, so neither has a place in it and isLoading has nowhere in the record to go. Carrying them is a protocol change, so the docblock now states both cases and what they currently project, and there is a test asserting model.revivals is [] whose message says that is also what a search matching nothing produces.

The real fix belongs with the relationship-state work, and I have written both this and the not-loaded reference loss onto CS-12622, which already specifies an explicit link-state union carrying a stable reference.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Accepted as documentation, and the pin is honest rather than cementing: the test's own message says the record has no spelling for the difference and that a tier reading it shows "no results" for a query that has not started — so it records the gap instead of blessing it. deepEqual(model.revivals, []) also fails if the shape later changes to omit query fields from the model, which is the direction a real fix would take, so it will not quietly outlive the decision it documents.

The docblock states both whole-field cases and why neither fits RP-7.1's slot union. Nothing further from me here.

Comment on lines +663 to +667
* The branch reproduces `field-component.gts`'s own `isThemeCard` / `themeCss`
* / `hasTheme` / `themeId` / `getCssImports`, which are module-private there. A
* card declaring its own `cssVariables` through a `CSSField` is a Theme card
* and scopes to its own identity; every other card scopes to the Theme its
* `cardTheme` mirror links.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Five closures re-implemented, with nothing to go red when the two copies drift — which is this file's own header argument one level down: competing builders each agree with themselves.

They already differ. Main's getCssImports hands card.cssImports back untouched; cssImportsOf filters non-strings and maps an empty array to null. Harmless today, and exactly the kind of difference that has no test on either side.

This PR already established the fix in the neighbouring case: resolveFieldConfiguration became a public Card API export so the capture could resolve configuration through the module the Loader served rather than a copy of its merge semantics. The theme helpers want the same treatment — they are closures inside getBoxComponent, so lifting them to module scope in field-component.gts (or a small shared module) and exporting them is the move, and captureTheme then derives from main's own answers.

Follow-up if the export is more than this PR should carry — but then say so in the docblock, because "reproduces" currently reads as a design choice rather than a debt.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Agreed, and the drift you spotted was real — filtering non-strings and mapping empty to null were both mine, not main's. Taken the export route in d69d0b4 rather than deferring it.

isThemeCard / themeCss / hasTheme / themeId / getCssImports are now module-scope exports of field-component.gts (they captured nothing from getBoxComponent, so lifting them was mechanical), re-exported from card-api the way resolveFieldConfiguration is. captureTheme calls them, and cssImportsOf and fieldTypedAs are gone — the capture no longer has a theme opinion of its own, only the record shape it puts main's answers into.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] The lift checks out: the five bodies moved byte for byte, and their only free identifiers — getField, CardDef, Theme — were already module-scope imports of field-component.gts, so nothing was captured. boxel-projection.ts is the only consumer and reaches all five through api.*, so there is one spelling in the tree.

Two leftovers, both small.

The reply says the capture no longer has a theme opinion of its own, but cssImports still normalizes: getCssImports hands card.cssImports back untouched, and the capture maps an empty array to null and filters non-strings. Mapping undefined to null is the record's shape and has to happen somewhere — the other two are the opinion the thread was about. They also disagree with each other: length > 0 is tested before the filter, so [] projects as null while [123] projects as [].

And the removal left a double blank line at the old definition site, which prettier flags. Nothing in CI will tell you — packages/base's lint script is lint:hbs plus the isUsed check, with no eslint or prettier pass, so this file's formatting is only ever enforced by the commit hook.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Both fixed in bd585d6.

The cssImports inconsistency was real and worse than it looked: emptiness was tested before the filter, so a list of nothing but unauthored entries projected as [] while a list of none projected as null. Filtering first makes the two agree. I kept the filter and the null-mapping but split them in the comment — the null is the record's shape, the filter is what keeps the member the string[] it is declared to be, since a containsMany entry that was never authored reads as the empty value rather than as a URL.

And the blank line is gone. Your point about it is the more useful half: packages/base's lint is lint:hbs plus the isUsed check, so nothing in CI formats that file and only the commit hook would have caught it.

Comment on lines +260 to +264
async serializeCard(
instance: BoxelInstanceHandle,
): Promise<LooseSingleCardDocument> {
let api = await this.getCardAPI();
return api.serializeCard(this.instances.get(instance) as never, {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] as never is crossing BaseDefCardDef, and the gap is reachable: retainInstance takes a BaseDef, the instance registry is RuntimeHandleRegistry<BaseDef>, and boxelKindOf explicitly answers 'field'. api.serializeCard reads model[localId], which CardDef declares and FieldDef does not — so a FieldDef handle reaching here builds {type: 'card', lid: undefined} and dies inside isSingleCardDocument with a shape dump, rather than saying the handle names a field.

Either narrow the instance registry to CardDef (and retainInstance with it), or check here and throw the named error. The cast is currently the only thing standing between the two types.

Regression, non-blocking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Right — and I went with the named error rather than narrowing the registry, because the pipeline genuinely does describe field instances (boxelKind answers 'field', and projectInstance on one is meaningful). It is only the document form that is card-shaped. Fixed in d69d0b4: serializeCard checks api.isCard and throws naming both the handle and the class, instead of dying inside isSingleCardDocument with a shape dump that names neither.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] The named error is the right call, and it sits ahead of everything that can throw.

The cast the thread was about is still there, though. isCard is exported from card-api as card is CardDef, so after the guard card is narrowed and serializeCard(model: CardDef, …) takes it directly — as never has no job left, and while it stands the next change to that signature still will not be caught here. Drop it, or say what it is standing for.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Dropped in bd585d6isCard narrows to CardDef, so serializeCard takes card directly and the cast had nothing left to stand for. Agreed on the reason it mattered more than the noise: while it stood, the next change to that signature would have gone unchecked at this call site.

… silently

The theme derivation was five closures copied out of field-component into the
capture, which is the header's own argument one level down: two builders of
one answer, nothing to go red when they drift. They had already drifted. The
closures are now module-scope exports of field-component, re-exported from
card-api the way resolveFieldConfiguration is, and the capture calls them.

Values that could not cross were being lost quietly:

- A bigint reached the not-data branch, so every BigIntegerField projected as
  null on every tier with nothing saying so. It has a spelling now — the same
  decimal string its serializer writes.
- A field whose value genuinely cannot cross is absent from the model rather
  than present holding null, so reading it routes through the missing-path
  diagnostic instead of looking like a field that is null. An array element
  keeps null: a position is an element's identity to anything iterating it.

serializeCard's `as never` was the only thing between BaseDef and CardDef.
retainInstance takes any BaseDef and the pipeline describes field instances,
so a field handle reaching it built {type:'card', lid: undefined} and died
inside the document check naming neither the handle nor the reason.

projectInstance built all three records and returned one, so every projection
walked the ancestry, resolved every format slot, and evaluated every field's
configuration function for records no caller could reach. It captures the
instance half alone now.

The header claimed one capture keeps getFields and projectInstance from
disagreeing. That stopped being true when getFields got its own read. What
one capture buys is a record that is internally coherent, and it says that.

Query-backed whole-field states have no spelling in the record: a search that
has not started projects the same empty membership as one that matched
nothing, and a failed search projects one absent slot. Both are stated in the
docblock and the first is pinned by a test.

@backspace backspace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Re-review of the response commit, scoped to the five open threads and to what each change implicates around it.

Convergence holds and the gate is met: no blocking issues. Every thread moved on the mechanism it named rather than its symptom, no fix landed on only one twin, and nothing new turned up above nit level. Severity is strictly decreasing: the two I called worth settling before a consumer exists — a value that could not cross being indistinguishable from an empty one, and a second read path for one instance — are both closed at the mechanism, and the second was closed without introducing the very duplication it was about.

Dispositions; detail stays in each thread:

  1. dataValue / captureValueresolves it. String(bigint) is exactly what the serializer writes, and omission does reach the diagnostic. One undocumented side effect: unset primitives now project as undefined rather than null.
  2. Query-backed whole-field states — resolves it as documentation. The pinning test names the gap in its own assertion message, so it records current behaviour rather than blessing it.
  3. captureThemeresolves it. The lift captured nothing and the tree has one spelling. Two nits: the cssImports normalization is still the capture's own opinion, and the removal left a prettier-flagged blank line that no CI step for that package will catch.
  4. serializeCardresolves the mechanism, not the cast. The guard narrows, so as never is now removable.
  5. projectInstanceresolves both halves, and orphans captureBoxelInstance (no caller anywhere) and buildBoxelRenderRecord (test-only).

Outside the threads: the description still says 13 integration tests and a bijection count dropping "from 104 to 91", where the suite now has 14 and the script records 88. It is the public record of the change, so worth a pass before merge.

…no stale cast

- cssImports tested emptiness before filtering, so a list of nothing but
  unauthored entries projected as [] while a list of none projected as null.
  Filtering first makes the two agree, and the comment separates the one
  normalization that is the record's shape from the one that keeps the member
  the string[] it is declared to be.
- serializeCard's `as never` had no job left once the isCard guard narrowed,
  and while it stood the next change to that signature would not be caught.
- captureBoxelInstance had no caller anywhere once each of the three captures
  was called directly; an exported function with no caller reads as an
  oversight. buildBoxelRenderRecord stays, and its docblock now says it is the
  all-three entry point and that nothing on a render path calls it yet.
- Prettier had flagged a blank line left where the theme closures used to sit.
  Nothing in CI would have caught it: packages/base runs ember-template-lint
  and the isUsed check, with no eslint or prettier pass.

The unset-field claim needed a fixture that actually exercises it. A card
served by a realm carries its unauthored primitives as null, so the test now
materializes a document that never carried the field, and the docblock says
which of the two cases yields undefined.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants