Skip to content

Call a card's operations, one at a time or as one atomic batch - #6198

Open
habdelra wants to merge 11 commits into
mainfrom
cs-12799-card-ops-operations-entry-point-host-operations-service
Open

habdelra wants to merge 11 commits into
mainfrom
cs-12799-card-ops-operations-entry-point-host-operations-service

Conversation

@habdelra

@habdelra habdelra commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Calling an operation

A card declares its operations as plain data. This adds the other half: calling them, without hand-writing an HTTP envelope.

import { operations } from '@cardstack/base/operations';

// one operation on a card — the lean result: what it wrote, and the version it now holds
let { id, version } = await operations(report).addComment({ body: 'Reviewed.' });

// a class-scoped create, in the realm you name or the one the session writes to
let activity = await operations(Activity).create({ headline: 'Lab safety' }, { realm });

// a file's writes work on its bytes
await operations(notes).appendLine({ line: 'deployed' });

operations(x) answers a bucket of callables — nothing hangs off the instance, because a card's property namespace belongs to its author's fields, the same reason isSaved(instance) is a function. A bucket carries exactly what can be invoked on what it was built for:

read update delete appendContainsMany appendLine create atomic
a card instance
a file instance ✓ (content)
a card class

plus every operation the def's author declared, in the scope it belongs to. Two names are absent on purpose, and the absence is the API: readSource has no member at all, because the card source and byte routes serve stored bytes; and a bare transform has none either, because a transform runs a program over the card's document and an envelope entry has no member to carry one — a transform is reached under the name its declaration gives it. A declared query is the one member that answers with where to go instead of a request: a query runs on the search engine, not in a batch.

Several operations as one batch

let [activity] = await operations(classroom).atomic((builder) => {
  let created = builder.create(ClassroomActivity, { headline: 'Lab safety' });
  builder.addActivity({ activity: created });      // travels as { "lid": "l1" }
  return [created];
});

The builder registers entries in the order they are called, and a handle is what a call answers with: it stands for the position the entry's result will sit in, and for a create it is also the local id a later entry links the new card by — so a link to a card that has no URL yet is a value you hold rather than a token to keep consistent by hand. Handles are substituted wherever they sit in a payload, since a link can be a member of an item appended to a collection as readily as a member of the payload itself.

A builder that returns nothing is answered positionally — every entry's result, in the order the entries were registered. A builder that returns handles is answered with those handles' results, in the order it listed them, which is also what types them.

builder.on(other) targets another card in the same realm; another realm is refused before anything is sent, because a batch commits under one realm's write lock and there is no lock that would make the write atomic with the rest. builder.parallel(…) and builder.serial(…) emit group entries and nest to any depth, and the results mirror the nesting. builder.find(filter, { field?, expect? }) names a target by search: the filter is the card-rooted spelling authors write everywhere else in this API and it is translated to the search grammar here, so there is one spelling to learn.

No realm reads boxel:target yet. The envelope's parse reads an entry's href and its data, so an entry carrying a target is refused as one that names nothing to run against — the form here is the one the parse will read, and a batch that uses it fails until it does. find, on(target) and expect ship as the emitted contract, checked in the client core's own suite; everything else in this description works against a realm today.

Two filters are refused rather than sent, both because the realm would answer them with something indistinguishable from a search that found nothing. One that names nothing would target every card in the realm. And one containing an invocation marker — actor(), params('x') — is a well-formed search operand that would be compared against stored values as a literal object: markers stand for what an invocation supplies, and a target written at the call site is the invocation, so nothing resolves one. The refusal names what the author wrote.

The transport

OperationsService is the one place that knows how a batch reaches a realm: the authenticated fetch, the method the realm derives its permission check from (POST for a batch that writes, QUERY for one that only reads), and the instance:-prefixed client request id a write is tracked by — registered with the same service the store's own writes register with, so an operation's write is echo-suppressed the way a save is. A write is refused in a render: the render holds the worker a write would wait on, so the two would wait on each other. A read from the same render is carried out, since it takes no lock.

A card module has no service to inject, so the service registers itself on a global bridge the runtime declares, the way the realm subscription and the choosers do, and an instance initializer arms it at boot rather than leaving it to whatever happens to inject the service first. In node there is nothing to register and operations() says so outright rather than no-opping — a silent no-op would turn a write nobody performed into a call that appeared to succeed.

Where the pieces live

The core that turns a call into a request and an answer into a result is isomorphic (runtime-common/card-operations/client.ts): it reaches no network, no loader and no card module, takes everything it needs to know about a def as data, and is driven in tests by a transport that only records what it was handed. It is a wire producer — the envelope's parse is the consumer and reads a different shape, so the two share no types.

isWrite moves from the envelope to the base-operation types, with its table intact. Both the parse and the client need to know which behaviors write, and the classification belongs to a base operation rather than to one front door. The Record<BaseOperation, boolean> is load-bearing: it is exhaustive by type, so a tenth base operation has to state whether it writes instead of defaulting to "read" and reaching a commit on a request authorized only to read.

Typing, and one limit worth knowing

A bucket's base operations, atomic, and a batch's results are typed exactly. Declared operations are typed from the class's declarations — which are statics, and TypeScript cannot read a class's statics through an instance type. So an instance call types its declared operations when it names the class (operations<typeof Report>(report)), and a call that does not name one leaves a declared name callable with a payload nothing checks, rather than making the ordinary spelling carry a type argument for the compiler's sake. Naming the class also makes the absent names visible. The run-time bucket is the same either way.

Not in this change

Store reconciliation for batch-created cards, the optimistic path, and calling a query operation are separate; a queried target's result is handed back as the realm reported it, since which behavior the name resolves to is the matched card's own type to decide.

Testing

Two suites, split by what each can see.

packages/realm-server/tests/card-operations-client-test.ts drives the client core directly with a transport that records what it was handed and answers from a script: what each bucket carries, the envelope every behavior emits, which method carries it (one case per writing behavior, so a behavior that stopped counting as a write fails here rather than reaching a realm), the patch that names its own type, a create-and-link batch's local ids, group nesting in both directions, a query target's filter, and each refusal that has to happen before a request. Fifty-seven tests, no realm, no database and no lane, so it runs in about a second — which is what made negative controls affordable: reverting the patch-type fill reddens exactly its one test, and flipping transform in the write table reddens seven.

packages/host/tests/integration/operations-invocation-test.ts (nineteen tests) covers what only a realm and the host can answer: a named transform and what the card is left holding, a base update, an appendContainsMany, a read, a delete, a class-scoped create, a declared create minting its own type, a file's read/update/appendLine round trip, the instance:-prefixed request id registered with the store's own set, the render refusal (against the realm, since a stand-in transport would replace the very code under test), an environment with no transport, the members a card and a file each carry, a create-and-link batch and the link it produced, and a batch reaching a second realm. Its typed-surface tests declare their own cards, so they mount no realm at all — mounting one resets the loader, and a locally declared card would then extend a different copy of the card API than the module reading its declarations.

One limit of the host harness is worth naming: its verifyJWT (tests/helpers/adapter.ts) treats a token that has not expired as expired, so an integration test's request reaches the in-browser realm unauthenticated. An operation that reads actor() is refused outright on such a request, so this suite's fixture declares none and what the actor resolves to stays covered against a real realm in the realm server's endpoint suite.

Locally: 70/70 standalone, 21/21 host integration, against a stack rebuilt from this branch. Each fix from review carries a case confirmed to redden only itself when the fix is reverted — the foreign-batch handle, the shared-reference payload, the group projection, and the entry point's own typing, which fails to compile against the predicate it replaces.

🤖 Generated with Claude Code

habdelra and others added 8 commits September 17, 2026 22:06
`operations(x)` answers the callable form of everything a def carries: a
card instance's own operations, a class's creates, and `atomic`, which
sends several as one all-or-nothing batch. The client core that turns a
call into an envelope and an answer into a result is isomorphic; the host
service owns the transport — the authenticated fetch, the method the realm
derives its permission from, and the client request id a write is tracked
by — and registers itself on a bridge a card module can reach.

`isWrite` moves from the envelope to the base-operation types: both the
parse and the client need to know which behaviors write, and the table is
exhaustive by type so a tenth behavior has to say.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The suite's helpers asked whether a store result was an error by testing
for a `status` member, which the card under test declares as a field, and
imported the base module before the realm was up — so the module's
`BaseDef` came from a loader older than the instances the store handed
back. A def that reaches `operations()` from another copy of the card API
now says so, naming what arrived.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An update of a card is a patch of its document, which is a card resource
and so names the type it patches — filled from the class of the instance
being patched rather than left to a caller who is naming field values.

The suite's own two lessons: the render refusal belongs to the host
transport, so a test that stands one in proves nothing, and mounting a
realm resets the loader, so a card the store builds afterwards is not an
instance of the card API an earlier import closed over. The typed-surface
tests declare their own cards and so mount no realm at all.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The client core needs no realm, no database and no browser: it turns a
call into an envelope and an answer into a value, and both halves are
checked against a transport that records what it was handed. That makes
the suite fast enough for negative controls — reverting the type a patch
names reddens exactly one test, and flipping `transform` in the write
table reddens seven — and leaves the host's suite to what only a realm
and the host can answer.

A filter written at a call site resolves no invocation marker, so one
left in a `find()` filter is refused by name rather than compared against
stored values as a literal object and read as a search that matched
nothing. A builder's returned handles are read before the batch is sent,
so a batch that writes does not commit because the caller asked for its
answer in a shape nothing can read.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… field

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ions

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 18, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-18T04:46:58.061282Z 7c7cf65 Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@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: 039f65919c

ℹ️ 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/services/operations.ts
Comment thread packages/runtime-common/card-operations/client.ts Outdated
Comment thread packages/runtime-common/card-operations/client.ts
Comment thread packages/runtime-common/card-operations/client.ts
@github-actions

github-actions Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Preview deployments

Host Test Results

280 tests   277 ✅  9m 57s ⏱️
  1 suites    3 💤
  1 files      0 ❌

Results for commit ee1d37e.

Realm Server Test Results

    1 files    245 suites   1h 22m 11s ⏱️
3 647 tests 3 647 ✅ 0 💤 0 ❌
3 698 runs  3 698 ✅ 0 💤 0 ❌

Results for commit ee1d37e.

@habdelra habdelra left a comment

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 🤖] This review went after the wire contract between the new client core and the realm's envelope parse, the bucket's membership rules against the TypeScript surface, the result mapping against every shape atomic:results can hold, and whether each new assertion discriminates. It did not run the host integration suite or the realm-server lane (both contended on this machine), so those 19 tests are read rather than executed; the standalone client suite was run (57/57) and both lint:types are clean apart from the pre-existing boxel-ui noise.

Two things need a decision before this lands: the find() / on(target) surface emits a wire member no realm code reads, and the un-annotated operations(report).addComment(…) spelling does not type-check. Everything else is small.

  1. boxel:target has no reader — ship the search-target surface or say it is refused: thread on registerEntry in card-operations/client.ts.
  2. NamesNoClass never fires, so the unchecked members are dead and the ordinary call spelling is a type error: thread on NamesNoClass in packages/base/operations.ts.
  3. guardCycle refuses a repeated object reference, not a cycle: thread on guardCycle.
  4. Both suite headers say the realm cannot carry out a group; it can, and the host suite could drive one: thread on the client suite's header comment.
  5. A declared query is a run-time member the types hide: thread on carriesMember.
  6. read(payload?) promises a payload every executor drops: thread on CardInstanceBaseOperations.
  7. The render refusal covers one of the store's two arms: thread on assertMayWrite.
  8. Two host-suite assertions can pass while the mechanism they name is broken: thread on the defaultWritableRealm test.

Adjacent, not asked of this PR: the host harness's verifyJWT (packages/host/tests/helpers/adapter.ts) throws TokenExpiredError when exp > now, i.e. for tokens that have not expired, so every integration-test request to an in-browser realm arrives anonymous. This suite documents the consequence and routes actor coverage to the endpoint suite, which is the right call here, but the inversion is a one-character fix that would unblock actor coverage for every integration suite.

Comment on lines +1127 to +1129
...(entry.queryTarget === undefined
? {}
: { 'boxel:target': entry.queryTarget }),

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 🤖] boxel:target has no reader anywhere in the tree, so every batch a find() target reaches is refused by the realm — this needs a decision about whether the surface ships.

parseInvocation in card-operations/envelope.ts reads boxel:name, href and data and nothing else, and OperationTarget is { kind: 'instance' } | { kind: 'type' }. An entry carrying boxel:target therefore reaches targetFor with no href and no data.meta.adoptsFrom, and comes back as "entry 0 names no "href" and no type in "data.meta.adoptsFrom", so there is nothing for it to run against" — a refusal that names neither what the caller wrote nor why. Grepping packages/ for boxel:target returns exactly two files: this one and card-operations-client-test.ts.

expect: 'many' has the matching gap on the way back: resultsTree nests an array only under a group node, so the handle.many branch in memberResult has no producer either.

Either hold find / on(QueryTarget) / expect back until the parse reads them, or keep them and say in FindOptions and in the description that a find() target is refused today — the description currently presents it beside create and on(card) with no caveat, and the suite comments only say the form is "checked here" (see the comment on the client suite header). New surface that cannot work end to end; blocking as a decision rather than as a code change.

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 🤖] Decision, in 7c7cf65: the surface ships and the caveat is stated rather than implied. find, on(target) and expect stay, because they are named deliverables of this change, but FindOptions and the description now say plainly that no realm reads boxel:target yet and a batch using one is refused until the parse does. The client suite's header says the same, and says why that one form is checked there rather than against a realm.

Your grep matches mine: two files, and zero on main. I had taken a peer session's "the wire shape is settled" as meaning a reader existed; it meant their branch. Worth the correction independently of the code.

Comment thread packages/base/operations.ts Outdated
Comment on lines +1854 to +1857
// Whether the call named the class its operations are declared on. It did not
// when the type parameter is still the constraint, which is every call that
// passes an instance without a type argument.
type NamesNoClass<Type> = [BaseDefConstructor] extends [Type] ? true : false;

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 🤖] NamesNoClass never evaluates to true, so UncheckedOperations and UncheckedBatchOperations are never mixed in and operations(report).addComment({ body: 'Reviewed.' }) — the ordinary spelling the description leads with — does not compile.

InstanceType<Type> is not an inference site, so on operations<Type extends CardDefConstructor>(instance: InstanceType<Type>) an un-annotated call leaves Type at its constraint, typeof CardDef. [BaseDefConstructor] extends [typeof CardDef] is then false, because CardDef adds required instance members BaseDef has not (id, cardInfo, [localId], [isSavedInstance]). Reproduced against this repo's tsc: the call fails with Property 'addComment' does not exist on type 'Base & DeclaredMembers<typeof CardDef>' — the resolved type names the constraint, and carries no index signature.

b.on(other) inherits it. on<Other extends CardDefConstructor>(instance: InstanceType<Other>) is the same non-inferrable shape, and BaseDef declares ['constructor']: BaseDefConstructor, so nothing recovers a subclass from an instance type — a declared operation on an on() target is unreachable by name even when the top-level call names its class.

Parameterizing the predicate by the constraint gives the comment above the mechanism it describes. Verified in the same reproduction that the un-annotated call becomes callable while a named class stays exactly checked — a name the def does not carry still errors:

type NamesNoClass<Type, Constraint> = [Constraint] extends [Type] ? true : false;
type Bucket<Type, Constraint, Members> =
  NamesNoClass<Type, Constraint> extends true
    ? Members & UncheckedOperations
    : Members;

Nothing caught this because every run-time call site in the host suite is (operations(x) as any), and the typed-surface tests build CardInstanceOperations<typeof Report> by hand rather than through operations() — so a green typecheck says nothing about the entry point. Worth one test that calls operations(report).addComment(…) with no cast and no type argument, plus one b.on(nearby).escalate(). Regression; 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 🤖] Fixed in 62b7214, with your repair: the predicate takes the constraint it is compared against, so each overload answers for its own. Reproduced the diagnosis before changing anything — [typeof BaseDef] extends [typeof CardDef] is false, so the fallback was dead — and then the reverse: with the old predicate restored, the new test fails to compile at exactly the two lines you named, operations(report).addComment(…) and b.on(nearby).escalate().

The test is the part I should have had first. the entry point's own typing calls operations() with no cast and no type argument, uses the result's id, reaches a second card through on(), and creates through the class — so the entry point is now typechecked rather than a hand-built bucket type standing in for it.

Comment on lines +1210 to +1217
function guardCycle(value: object, name: string, seen: Set<object>): void {
if (seen.has(value)) {
throw new Error(
`operation "${name}" was handed a payload that contains itself`,
);
}
seen.add(value);
}

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 🤖] This refuses any repeated object reference, not a cycle, so two ordinary payloads are rejected with a message that misdescribes them. seen is added to on the way in and never cleared on the way out, so a value reached twice down two sibling paths is indistinguishable from a value that contains itself.

Driving the real buildOperations with a recording transport:

appendContainsMany({ field: 'comments', items: [shared, shared] })
  -> operation "appendContainsMany" was handed a payload that contains itself
update({ attributes: { one: shared, two: shared } })
  -> operation "update" was handed a payload that contains itself

Both are plain DAGs that JSON.stringify serializes fine, and the same shape inside atomic() is refused too. Deleting on the way out keeps the cycle check and drops the false refusal:

if (Array.isArray(value)) {
  guardCycle(value, name, seen);
  let members = value.map((member) =>
    substituteHandles(member, name, seen, inBatch),
  );
  seen.delete(value);
  return members;
}

Same in the record arm. 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 🤖] Fixed in 62b7214seen.delete(value) on the way out of both arms, so the set is the path rather than the history. Your two reproductions are the new case: items: [shared, shared] travels as two values, and a self-referential payload is still refused. Reverting the unwind reddens only that case.

Comment on lines +32 to +35
// `@cardstack/base/operations` and real `@operation` declarations. Two of the
// wire forms are deliberately only checked here for now: a `parallel`/`serial`
// group and a target found by search are read by realms still landing, and
// what the builder emits for them is a contract in its own right.

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 🤖] The realm does carry out groups, so half this reason for stubbing is wrong — and the stub costs coverage that is available. parseGroup is in card-operations/envelope.ts, and realm-endpoints/operations-test.ts pins the behavior end to end: "a parallel group commits its members together, under one job and one event", "groups nest, and the results mirror the shape of the request", "groups three deep commit together". The in-browser realm the host suite mounts serves the same handleOperations, so a parallel/serial batch there would be carried out rather than needing a recording transport — which means nothing currently checks that the group wire this builder emits is the wire the parse accepts.

The same claim sits in packages/host/tests/integration/operations-invocation-test.ts ("carried out by realms this endpoint does not have yet").

Please cut groups from both caveats and drive one group batch against the realm in the host suite; the search target is the one form that genuinely has no reader (see the comment on registerEntry). While editing these: "for now", "still landing" and "does not have yet" are the temporal wording the evergreen rule cuts — state what each suite covers, not what is on the way. 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 🤖] You are right on both counts, and the caveat was stale rather than wrong-headed: #6192 merged while this branch was open, so parseGroup and the end-to-end group tests you cite landed under me. Fixed in 7c7cf65 — both suite headers now say groups are carried out, the one form with no reader is named as the search target, and the temporal wording is gone from both.

The coverage came with it: the host suite drives a real parallel group against the in-browser realm and asserts both members committed — the group's create minted a card, its sibling appended to the anchor, and both are on disk afterwards. That is coverage the recording transport could not give.

Comment on lines +279 to +284
if (info.base === 'query') {
// Only a name an author wrote. A member that says where a query is
// reached is worth having for a name the author can see in their own
// card, and `operations(Type).query(…)` was never how one is run.
return info.declared && subject.scope === 'type';
}

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 🤖] A declared query is a run-time member the types hide, so the name an author is meant to see is reachable only through a cast — pick one of the two answers.

carriesMember returns it for declared && scope === 'type', but DeclaredNames<Type, 'type'> admits only Base extends 'create' (TypeScopedBase), and CardTypeOperations is not wrapped in Bucket, so it has no index signature either. operations<typeof Report>(Report).openReports is therefore a type error, and both suites reach openReports only as bucket.openReports as … / (operations(Report) as any).

So the stated purpose — a name the author can see in their own card — is not delivered to typed code, and the refusal this member exists to throw is unreachable without a cast. Either type it (a member whose call type states it is reached through the entry search API) or drop it from the bucket and let the absence say it, the way readSource does. 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 🤖] Took the second answer, in 62b7214: the member is gone, and the absence says it the way readSource's does. A member whose only behavior is to throw, that TypeScript then has to hide, is worse than nothing there — the author cannot reach it without a cast, which is how both suites were reaching it.

REACHED_ELSEWHERE now names both behaviors in one place with where each is served, and the client suite asserts exhaustively over them: for every subject, every operation whose base is readSource or query has no member. A tenth behavior reached elsewhere fails that list rather than shipping a member a caller can only discover by calling it.

Comment thread packages/base/operations.ts Outdated
// over the card's document and a batch entry has no member to carry one, so the
// behavior is reached under the name a declaration gives it.
export interface CardInstanceBaseOperations {
read(payload?: Record<string, unknown>): Promise<OperationDocument>;

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 🤖] read(payload?) promises a payload nothing reads. The read executor (card-operations/read.ts) never calls paramsFor, and a declaration that would give a payload meaning — input, output, program — is refused 501 by refuseUnservedStages there and by assertStagesAreServed in the envelope. So read({ … }) hands back a plain document with no signal that the payload was dropped.

Please drop the parameter from the three read signatures here and from CardBatchBaseOperations.read, or say what a payload is for. 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 🤖] Dropped in 62b7214 — all four signatures, including the batch one. The read executor reads no params and a declaration that would give one meaning is refused as a stage a batch does not run, so the parameter promised something nothing could honor. A parameterized read arrives as a declared operation, under its own name with its own payload type, which is where the promise can be kept. The types module asserts the new shape, so a payload cannot creep back in silently.

Comment on lines +106 to +122
// A store rendering for the indexer must never write: the render holds the
// only worker while a write takes the realm's write lock and waits for a
// reindex that needs that worker, so the write and the render wait on each
// other. The prerender app marks itself, and every write from it is refused
// — the same rule the store applies to its own persistence, applied here
// because an operation reaches the realm without going through the store at
// all.
private assertMayWrite(method: OperationsMethod) {
if (method !== 'POST') {
return;
}
if ((globalThis as any).__boxelPrerenderApp) {
throw new Error(
`an operation that writes cannot run in a render: the render holds the worker a write would wait on, so the two would wait on each other`,
);
}
}

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 🤖] This is one of the store's two arms, not "the same rule" — is the other deliberately out of scope? renderContextBlocksPersistence refuses on __boxelPrerenderApp or on isRenderStore && __boxelRenderContext, and app/routes/render.ts never sets __boxelPrerenderApp under isTesting() precisely because in-browser index renders run alongside an interactive app. assertMayWrite reads only the first flag, so a card invoking a write from a render in an app that is not the dedicated prerender app is let through, while the store would refuse its own save in the same render.

If that is fine because a render outside the prerender app only happens in the harness, say so in this comment — as written the paragraph reads as the whole guard, and the next person will build on it. If it is not, __boxelRenderContext is the term to add. Non-blocking; an answer rather than a change.

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 🤖] Deliberate, and the comment now says so instead of reading as the store's whole rule (62b7214).

The store can ask the second question because the caller is the store: isRenderStore names that instance. An operation reaches the realm without going through a store, so there is no instance to ask about, and the only signal left — __boxelRenderContext alone — is set around every in-browser index render in the ordinary app too. Reading it here would refuse a card's write for coinciding with someone else's indexing, which is the failure the store's own comment warns about when it explains why it does not gate the interactive store on that flag. So the deadlock this guards is the prerender app's, where the sole worker and the write lock meet, and the comment names the arm it does not read and why.

Comment on lines +433 to +437
assert.strictEqual(
getService('operations').defaultWritableRealm(),
getService('realm').defaultWritableRealm?.path,
'the realm a class-scoped create defaults to is the one the session writes to',
);

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 🤖] This holds when both sides are undefined, which is the state it needs to rule out: realm.defaultWritableRealm returns null whenever no known realm reports canWrite, and then the assertion passes for an implementation that returns nothing at all. Please assert the concrete realm the harness logs in to (testRealmURL), so the test says which realm a class-scoped create defaults to rather than that two expressions agree.

The client-request-id test above has the mirror-image hole: it asserts one instance:-prefixed id landed in cardService.clientRequestIds, which is the half the store reads, but nothing ties that id to the X-Boxel-Client-Request-Id header — and the header is what the realm reads (#callerOfcommitBatch) to stamp the index event the suppression depends on. A send that registered one id and sent another, or none, still passes. Asserting the sent header carries the registered value closes it. 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 🤖] Both fixed in 62b7214, and both were assertions that could not fail.

The realm one now asserts testRealmURL — the realm this harness logs into — so it says which realm a class-scoped create defaults to rather than that two expressions agree, and it fails for an implementation that answers nothing.

The request-id one now reads the clientRequestId off the event the realm broadcast, which is the only place the sent header surfaces. It also asserts the opposite of what it used to: nothing is registered with the store, because registering is what suppresses the reload and an operation has no document to apply in its place. So the test now pins both halves of the header's journey — that it reached the realm, and that it did not silence the reload that keeps the card fresh.

@habdelra

Copy link
Copy Markdown
Contributor Author

[Claude Code 🤖] Revising the bottom line on my review above: the Codex threads hold three real findings I had not covered, all verified against the branch, so there are four blocking items, not two.

  • Echo suppression without reconciliation (services/operations.ts) — a write registers an instance: id, which suppresses the reload, but the answer carries no document to apply, so a transformed or appended card stays stale in the UI. The premise the store's suppression rests on does not transfer to an operation.
  • A handle from an earlier batch is substituted by lid alone (client.ts) — it silently links the card the current batch minted under the same l1.
  • A group builder's projection is discarded (client.ts) — a group returning one of two handles is typed as one result and resolves to both.

Plus the two from my review: boxel:target has no reader, and NamesNoClass never fires so the un-annotated call spelling does not compile. Each Codex thread carries my verification; the cycle-guard one was a duplicate of my own finding.

habdelra and others added 2 commits September 18, 2026 00:29
…ecked

`operations(report).addComment(…)` did not compile: the predicate that asks
whether a call named its class compared every overload's type parameter
against `BaseDefConstructor`, which answers no for all of them — a card
constructor's instances carry members a base def's do not — so the
unchecked fallback was dead. It reads the constraint it was given now, and
a test calls the entry point with no cast and no type argument, which is
the one thing a hand-built bucket type cannot say anything about.

A handle is refused unless the batch substituting it is the batch that
made it. A local id is the batch's own sequence, so another batch's
handle was not merely unresolvable: its `l1` was this batch's `l1`, and
the link pointed at whatever card this batch minted first.

A group's builder projects its results the way the whole batch's does,
through one shared path rather than a second implementation that ignored
what it was handed. The payload walk tracks the path it is on, so a value
named twice travels twice and only a value containing itself is refused.

An operation's write no longer registers its request id with the store.
That registry makes the store skip a reload, which a save may do because
it already applied the document it sent; an operation's answer carries
identity and version, so suppressing the event left a transformed card
showing pre-write values with nothing to correct it.

A type-scoped subject carries its code ref by type, and a class no module
exports is refused where the caller can see which class they passed.
`read` takes no payload, because every executor drops one, and a query has
no member at all — its absence says where it is reached, the way
`readSource` does, rather than a member the types have to hide.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@habdelra

Copy link
Copy Markdown
Contributor Author

[Claude Code 🤖] All twelve findings across both reviews are addressed in 62b7214 and 7c7cf65; each thread has the detail. Where a fix was in code, a case that reddens only that fix came with it.

The four blocking ones. Echo suppression is gone — the write still carries its instance: id to the realm, but nothing registers it with the store, because that registry is what skips the reload and an operation has no document to apply in the reload's place. A handle is refused unless the batch substituting it is the batch that made it, which was linking the wrong card rather than failing. A group's builder projects its results through the same path the batch's does. And the predicate that decides whether a call named its class now reads the constraint it was given, so operations(report).addComment(…) compiles — with a test that calls the entry point with no cast and no type argument, which is the gap that let it ship.

The one decision. find / on(target) / expect stay, and the description and FindOptions now say that no realm reads boxel:target yet and a batch using one is refused until the parse does. They are named deliverables of this change; presenting them without the caveat was the actual defect.

Verification. 70/70 in the standalone client suite, 21/21 host integration against a stack rebuilt from this branch, both typechecks clean. Negative controls: reverting the batch-owner check, the cycle-guard unwind or the group projection reddens exactly its own case; restoring the old predicate fails the new typing test at the two lines the review named.

Two of these were mine to have caught. The entry point's own typing was never exercised without a cast, and I described groups as "carried out by realms this endpoint does not have yet" — stale the moment #6192 merged, and it cost coverage the realm could already give. The host suite now runs a real parallel group against the realm and asserts both members committed.

@habdelra

Copy link
Copy Markdown
Contributor Author

@codex review

@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: 7c7cf65b00

ℹ️ 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 on lines +338 to +339
if (subject.scope === 'instance' && subject.family === 'card') {
bucket.atomic = (build: unknown) => runAtomic(subject, env, build);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reserve atomic before overwriting declared operations

When a card legally declares an operation named atomic, the loop above installs its callable and this assignment immediately replaces it with the batch helper. The generated type exposes both call signatures, but invoking the declared operation with its payload reaches runAtomic and fails because the payload is not a builder function. Either reject atomic as a reserved declaration name or provide a non-colliding route to the declared operation; the same collision should also be addressed for the on, create, find, parallel, and serial helpers installed by builderFor.

Useful? React with 👍 / 👎.

@habdelra habdelra Sep 18, 2026

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 and fixed in ee1d37e — a declaration under one of those names was installed by the loop and then replaced, so the author's operation stayed in the type while a call on it reached the builder's member and died on the payload.

Split by whether the name can legitimately be declared:

  • atomic, on, find, parallel, serial are the invocation surface's own members and no base operation is named after them, so the decorator refuses a declaration under one — at the point it is written, where the author can see it, rather than at a call that lands somewhere else.
  • create cannot be reserved: it is a base operation an author may specialize, and a batch reads create as its own way to mint a card of any type. Both are legitimate, so neither shadows the other — building a batch on a card that declares create is refused with a message naming both spellings. The declared one is still invocable outside a batch, which the new case asserts alongside the refusal.

Control: stubbing the collision check out reddens only that case.

Comment on lines +1848 to +1851
type DeclaredOperationMembers<Type, Scope extends 'instance' | 'type'> = {
[Name in DeclaredNames<Type, Scope>]: (
...args: PayloadArgs<OperationsOf<Type>[Name]>
) => Promise<ResultOf<OperationsOf<Type>[Name]>>;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Expose realm options on declared class creates

For a declared operation based on create, the type-scoped call can run in an explicitly selected realm at runtime because invokeOne reads its second argument as InvokeOptions, but this mapped signature permits only PayloadArgs. Consequently, a valid call such as operations<typeof Report>(Report).createActivity(payload, { realm }) is rejected by TypeScript and typed callers can only use the session's default realm. Add the options argument for type-scoped declared creates, as is already done for the base create member.

Useful? React with 👍 / 👎.

@habdelra habdelra Sep 18, 2026

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 and fixed in ee1d37e. The run time already read a second argument as InvokeOptions for any type-scoped call, so the type was the half that was wrong — typed callers could only have the session default, which is not what a type-scoped call means.

A type-scoped declared operation now takes the same options the base create does, and InvokeOptions is re-exported from @cardstack/base/operations, since it is part of what an author writes rather than an internal shape. The types module pins the two-argument signature, so the run time and the type cannot drift apart again in this direction.

`operations(x)` answers a bucket keyed by operation name, and `atomic` sits
in that namespace beside them; a batch builder adds the members that say
what an entry runs against. A declaration under one of those names was
installed and then overwritten, so the author's operation was reachable
only by a call that landed on the builder's member and failed on the
payload. They are refused where they are written now.

`create` is the one that cannot be reserved — it is a base operation an
author may specialize — so the batch refuses the collision when it is
built, naming both spellings rather than picking one.

A declared create invoked on a class takes the realm option the base
create takes: a type has no instance to read a realm from, so the caller
names it, and the type said otherwise while the run time already allowed
it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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