Conversation
`POST` and `QUERY {realm}/_operations` read a `boxel:operations` envelope,
resolve each entry's name against its target's definition, and answer
positionally in `atomic:results`: reads with the document, writes with the
identity and version they wrote, deletes with nothing. A read entry answers
from the state the batch started from, and the writes commit through the same
coordinator the card verbs do, all or nothing.
The envelope's parse, its validation and the shapes it answers with live in
`card-operations/envelope.ts`, which reads no file and resolves no identifier,
so a batch can be read and answered wherever the operation core runs.
A file write on an instance whose extension the realm does not register now
reaches its executor, which is where a card's stored JSON is told from plain
bytes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… bytes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a947904e6
ℹ️ 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".
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] I went after the transport's edges rather than the operation core behind it: how a body is routed and parsed, how an href becomes a target, where an entry's position is reported, whether the three isWrite decisions can disagree, what the single data member means per resolved base, the anonymous-actor gate, and what the dispatch change moves. The core's own behavior — what a transform program does to a document, how commitBatch composes two entries onto one file — I read for context but did not re-review. I did not run the realm-server suite (one shared lane on this machine), so nothing below rests on a suite run: each claim is traced through the tree or checked with a standalone script, and the threads say which.
Five things I'd fix before merge: the bxl-emit import (it is why Lint is red), media-type routing, the un-canonicalized href, registered-prefix hrefs, and a malformed adoptsFrom answering 500. The rest are small. The shape of the endpoint — one verb on the wire, the name resolved against the target's own definition, reads before the lock, commitBatch untouched — I have no argument with.
On the two questions the description leaves open for a reader: the three isWrite call sites cannot disagree (they read one definition object per entry), and every entry fills exactly one results slot — the hazard there is the guard, not the filling, which is recommendation 6.
envelope.tsimportingcallsActorputs@cardstack/bxlback inrealm.ts's static graph, whichexecutors.tsdocuments as the thing to keep out. Lint Postgres is green on the merge base and red here. → thread on the import.- Only the byte-exact spelling of the operations media type routes; every other legal one falls through, and
carriesOperationsExt's tolerance cannot be reached. → thread on the route registration. - The raw
hrefreachesresolveOperationand the coordinator: a declared operation 404s on a trailing slash or query string, a write reports the raw spelling as the card's id, and the gate resolves a different definition from the one that runs. Same thread covers prefix-form hrefs, which the endpoint canonicalizes outbound and never resolves inbound. → thread on#resolveEnvelopeEntry. - A nested
adoptsFromwith a null member answers 500 with a stack trace. → thread ontargetFor. labelEntryremapsmeta.entrybut not the batch positions in nine message strings or inmeta.conflictsWith. → thread onlabelEntry.answeredcannot fire —mapskips array holes. → thread on theatomic:resultsassembly.needsActormissesdefinition.items. → thread onneedsActor.paramsForpasses through four members the envelope reads for itself, not one. → thread onparamsFor.- No write base runs an
inputoroutputstage, and this endpoint is what makes a declaration that needs one reachable. → thread on theappendLinearm.
The red Lint check is recommendation 1 and is not a flake — the failing symbols (Object.hasOwn, Array.prototype.at, WeakRef) are the ones executors.ts predicts for this exact import edge.
Of the four findings Codex posted, three reproduce with the mechanism named in the threads above; the fourth (appendLine and input) is real but wider than appendLine. None was taken on trust.
Adjacent, out of scope: "which base does which def kind carry" now lives in three hand-maintained tables — CARRIED_BY in dispatch.ts, DECLARABLE_BY in lowering.ts, and the authoring table in base/operations.ts. The dispatch change makes them differ deliberately (appendLine is carried on any instance but declarable only on a file def), which is fine, but a pointer between the three would save the next person working out whether the difference is intended.
| @@ -0,0 +1,664 @@ | |||
| import { BOXEL_OPERATIONS_EXT } from '../supported-mime-type.ts'; | |||
| import { RealmPaths, type LocalPath } from '../paths.ts'; | |||
| import { callsActor } from './bxl-emit.ts'; | |||
There was a problem hiding this comment.
[Claude Code 🤖] This one import is what turned Lint red, and the invariant it breaks is written down twenty lines into executors.ts:
reaching for
@cardstack/bxlat all — for a value or for a type — pulls its sources into the typecheck program of every package that reaches an executor.packages/postgresgets here throughruntime-common/realm, and compiles those sources under an olderlibthan they are written for.
bxl-emit.ts imports @cardstack/bxl and @cardstack/bxl/mutation at the top level, realm.ts imports envelope.ts statically, so bxl is back in the static graph on exactly that path. Lint is green on the merge base (f59228eb) and red here, with the errors the comment predicts — Object.hasOwn, Array.prototype.at, WeakRef, two-argument Error, all in packages/bxl/src/** under the Lint Postgres group. This is the PR's to fix, not a flake.
The cheapest fix is also the better design: needsActor asks a question about a stored definition that never changes between invocations, and the definition already carries a lowering-computed boolean of exactly this shape — deterministic, set in lowering.ts from usesVolatileCall(program.source). Record readsActor there the same way and needsActor becomes a field read, bxl stays in the indexer where it already is, and the enumeration problem in the thread on needsActor goes away too, since lowering has the whole declaration in hand rather than the four members this function remembers to check.
If you'd rather keep the scan at request time, executors.ts shows the other pattern: import('@cardstack/bxl/mutation' as string) behind a specifier the compiler cannot follow, with the shapes stated locally and bxl-mirror-check.ts holding them honest.
Regression, blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 94d6158680, and by the route you recommended rather than by hiding the import.
readsActor is now recorded at lowering time beside deterministic, and needsActor is a field read. That removes the enumeration problem in the other thread as a side effect — lowering has the whole declaration, so items is covered without a list to keep in sync — and it is the better answer anyway: the question cannot change between invocations, so asking it per request was work as well as a bad edge.
Reproduced your diagnosis with the command that actually sees it, pnpm --filter @cardstack/postgres lint:types | grep bxl/src: errors before, nothing after. The two package-level checks I had been running are both false-green here, which is why I shipped it.
| .post( | ||
| '/_operations', | ||
| SupportedMimeType.BoxelOperations, | ||
| this.handleOperations.bind(this), | ||
| ) | ||
| .query( | ||
| '/_operations', | ||
| SupportedMimeType.BoxelOperations, | ||
| this.handleOperations.bind(this), | ||
| ) | ||
| .post( | ||
| '/_operations', | ||
| SupportedMimeType.JSONAPI, | ||
| this.handleOperations.bind(this), | ||
| ) | ||
| .query( | ||
| '/_operations', | ||
| SupportedMimeType.JSONAPI, | ||
| this.handleOperations.bind(this), | ||
| ) |
There was a problem hiding this comment.
[Claude Code 🤖] Registering a parameterized media type in a table the router matches by exact string means only one spelling of the header ever reaches handleOperations — so the fall-through this comment sets out to prevent still happens, to the requests that got the extension right.
extractSupportedMimeType splits the header on , and tests each candidate for membership in Object.values(SupportedMimeType). Running that function over the spellings carriesOperationsExt was written to accept:
application/vnd.api+json;ext="https://boxel.ai/ext/operations" -> routes
application/vnd.api+json; ext="https://boxel.ai/ext/operations" -> no route family
application/vnd.api+json;ext=https://boxel.ai/ext/operations -> no route family
application/vnd.api+json;ext="…/ext/atomic …/ext/operations" -> no route family
application/vnd.api+json;charset=utf-8;ext="…/ext/operations" -> no route family
Every one of those returns true from carriesOperationsExt — its trimming, case-folding, quote-stripping and whitespace-splitting exist for exactly them — and none can reach it. Whether that ends as a 404 turns on Accept: */* is itself a registered family with no POST route, so lookup falls through to the exact Content-Type compare and misses. A fetch that sets Content-Type and no Accept is the shape that gets bitten.
The suite can't see it: every request in operations-test.ts sets both headers to the byte-exact SupportedMimeType.BoxelOperations.
The fix has to be in the lookup, not the handler — compare the media type with its parameters stripped and let carriesOperationsExt be the whole of the extension check. Note extractSupportedMimeType also serves Accept, so whatever lands there has to keep a parameterized Accept selecting this family. Worth a test per spelling above, including one with no Accept at all.
Regression, blocking. Codex flagged the same gap; the per-spelling results above are from running the router's own function.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 285c4a9525, in the lookup as you said.
extractSupportedMimeType now compares a media type by its parts after the whole-value pass finds nothing: the type and subtype, and the URIs its ext parameter names. It prefers the registered type whose extensions the candidate carries and falls back to the unqualified one, so a body naming this extension reaches this endpoint and one naming …/ext/atomic reaches the plain JSON:API family. Additive by construction — the exact pass runs first, so every header that routed before routes to the same place.
Your five spellings plus a few more are pinned two ways: a test that sends each as the content type alone with no Accept, and the Accept side kept honest by the existing card+json and */* cases. application/vnd.card+json; charset=utf-8 now resolves to card+json where it used to 404 — a deliberate widening, called out in the description.
| let target = targetFor(entry, this.url); | ||
| let definition = await resolveOperation( | ||
| this.operationCore, | ||
| target, | ||
| entry.name, | ||
| scope, | ||
| ); | ||
| assertTravelsInEnvelope(entry, definition); |
There was a problem hiding this comment.
[Claude Code 🤖] targetFor hands over the href exactly as hrefIn produced it, and resolveOperation's definition lookup bottoms out in an exact-URL index read — getInstance compares i.url = $href / i.file_alias = $href — so every spelling canonicalizeTarget exists to fold resolves nothing.
The caller sees it as an asymmetry between base and declared names. {"boxel:name": "read", "href": "/thing?v=1"} works, because the built-in resolves without a row and runOperation canonicalizes before it reads. {"boxel:name": "escalate", "href": "/thing?v=1"} comes back 404 unknown-operation, "there is no operation named escalate on …", on a card whose type declares it. Same for a trailing slash and for a fragment — the three spellings canonicalizeTarget's own doc comment enumerates.
It also splits the gate from the executor on the read path: this resolves the raw target, then runOperation canonicalizes and resolves again, so isWrite, needsActor and assertTravelsInEnvelope are decided against a different definition from the one that runs. canonicalizeTarget names that outcome directly — "Canonicalizing in only one of those places is worse than canonicalizing in none, because the two then disagree about which card the request names".
On the write path the raw href becomes the reported identity: stageAppendLine, stageTransform, stageUpdate and stageDelete all set id: url.href from targetURL(entry.href), while localPathIn strips the query and trailing slash for the path. So an appendLine on /telemetry.log?v=1 writes telemetry.log and answers data.id: {realm}telemetry.log?v=1 — an id no other surface spells that way and one the caller cannot send back.
Second half of the same ask: the href is never resolved out of registered-prefix form. parseOperationsEnvelope builds its own new RealmPaths(new URL(realmURL)) with no VirtualNetwork — right for that module, which must not touch one — and RealmPaths then only answers same-form membership. An href of @cardstack/base/thing does not parse as a URL, falls into the relative branch, and silently becomes {realm}@cardstack/base/thing rather than being refused. That is the spelling writeResult's canonical hands back through unresolveURL on a prefix-served realm, so a result id cannot be used as the next entry's href. /_atomic closes this with #resolveAtomicHref; the envelope has the outbound half and not the inbound one.
Both belong here rather than in envelope.ts: resolve a registered prefix with the realm's VirtualNetwork, run the result through canonicalizeTarget(this.operationCore, target), then carry the canonical target into runOperation and the canonical href into batchEntryFor. rootNamesIndexCard can stay at its default — readSource is the only definition-free name and assertTravelsInEnvelope refuses it anyway.
Regression, blocking. Codex raised the canonicalization half independently.
There was a problem hiding this comment.
[Claude Code 🤖] Both halves fixed in 285c4a9525.
Inbound: parseOperationsEnvelope takes a bound resolveIdentifier, wired to the same #resolveAtomicHref /_atomic uses, so a registered prefix resolves before the URL parse instead of falling into the relative branch. The envelope still touches no VirtualNetwork.
Canonicalization: #resolveEnvelopeEntry runs canonicalizeTarget once and rewrites the entry's href to the canonical spelling, so the definition is resolved from it, the read runs against it, and the staged entry writes it — one spelling everywhere, which is what that comment asks for. Pinned by a test sending ?view=full and #section for a declared operation and asserting both the write and the id it answers with.
| return { | ||
| kind: 'type', | ||
| codeRef: adoptsFrom as unknown as CodeRef, | ||
| realm: realmURL, | ||
| }; |
There was a problem hiding this comment.
[Claude Code 🤖] The cast admits any object, and a nested ref with a null member turns malformed client input into a 500 carrying a stack trace.
{"data": {"meta": {"adoptsFrom": {"type": "fieldOf", "card": null, "field": "x"}}}} reaches resolveCodeRef → codeRefWithAbsoluteIdentifier, which takes the 'type' in ref branch and recurses into ref.card, where 'type' in null throws TypeError: Cannot use 'in' operator to search for 'type' in null (I ran those four lines). That is not an OperationFailure, so handleOperations rethrows and the router answers unexpected exception in realm <stack>.
The comment above says the ref is handed on unchecked because "a ref that names nothing comes back from the lookup as a type that cannot be resolved". That holds for a well-formed ref naming nothing; it does not hold for a ref that is not a CodeRef at all, which is what the wire can now send. Either validate the recursive shape here and refuse 400, or null-guard the recursion in codeRefWithAbsoluteIdentifier — the second helps every other caller too, and either way the comment should say which of the two it is relying on.
Regression (this endpoint is what makes the shape reachable from the wire), blocking — client input should not produce a 500. Codex flagged this one as well.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 285c4a9525, taking the first of your two options: targetFor validates with isCodeRef, which is already the recursive shape check, and refuses 400.
The comment now says which of the two it rests on — that the lookup answers for a well-formed ref naming nothing, and that a thing which is not a code ref at all is refused here instead. I left codeRefWithAbsoluteIdentifier alone deliberately: null-guarding it is the wider fix and helps every caller, but it changes a shared resolver's behavior for callers I have not looked at, and it belongs in a change that can say what each of them does with the new answer.
Your exact payload is the test case.
| export function labelEntry( | ||
| err: unknown, | ||
| at: (index: number) => number, | ||
| ): unknown { | ||
| if (!isOperationFailure(err)) { | ||
| return err; | ||
| } | ||
| let entry = err.error.meta?.entry; | ||
| if (typeof entry !== 'number') { | ||
| return err; | ||
| } | ||
| return new OperationFailure({ | ||
| ...err.error, | ||
| meta: { ...err.error.meta, entry: at(entry) }, | ||
| }); |
There was a problem hiding this comment.
[Claude Code 🤖] The remap covers meta.entry and nothing else, so a mixed batch's refusal comes back carrying two contradictory positions.
The coordinator also writes the batch position into the message text — nine sites in coordinator.ts: entry ${index} names no staged operation, entry ${index} is a ${entry.op}, which has no base version, entry ${index} could not be staged, local id "…" is claimed more than once (at entry ${index}), entry ${index}, included[${offset}], entry ${index} links to …, entry ${index} replaces the content of …, which entry ${appender} appends to, entries ${owner} and ${index} both replace …, entry ${index} appends to …, which an earlier entry removes — and three of those carry meta.conflictsWith, which is a batch position too.
Concretely, for [read, appendLine f, update f] the append-then-write conflict answers meta.entry: 2 with a detail reading "entry 1 replaces the content of f, which entry 0 appends to" and meta.conflictsWith: 0. The one entry-labelling test asserts meta.entry on a refusal whose detail carries no index, so it can't see the disagreement.
Two ways out. Stop interpolating indices into coordinator prose and name paths and local ids instead — most of those messages read fine without a number. Or give commitBatch the label for each entry up front, so the coordinator writes the envelope's position everywhere and labelEntry disappears; that one also fixes conflictsWith, which no post-hoc remap can reach without knowing which meta keys are positions.
Regression, non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 285c4a9525, taking your second option — the coordinator is told each entry's label rather than having one key corrected afterwards.
EntryCommon gains label, the envelope sets it to the entry's position, and positionsOf feeds every site that reports a position: meta.entry, meta.conflictsWith, and all nine prose interpolations. labelEntry is gone. An entry that carries no label reports its position in the batch, so nothing else that calls commitBatch changes.
Your [read, appendLine f, update f] case is the test, asserting meta.entry: 2, meta.conflictsWith: 1, and that the sentence names 2 and 1 — the assertion the old test could not make, since its refusal carried no index in the prose.
| 'atomic:results': results.map((result, index) => | ||
| answered(result, index), | ||
| ), |
There was a problem hiding this comment.
[Claude Code 🤖] answered cannot fire here. results is new Array(entries.length), so an unassigned position is a hole, and Array.prototype.map skips holes — the callback never runs for one, and JSON.stringify writes null there, which is the outcome answered's own comment says it prevents.
let results = new Array(3); results[0] = 'a'; results[2] = 'c';
results.map((r, i) => { if (r === undefined) throw new Error(i); return r; });
// callback runs 2 of 3 times; JSON.stringify -> ["a",null,"c"]Nothing produces a hole today — resolved is one element per entry and each one fills its own index — so this is the guard rather than a live bug. Array.from({ length: entries.length }, (_, index) => answered(results[index], index)) visits every position and makes it real.
Regression, non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 285c4a9525 — Array.from({ length: entries.length }, …), which visits every position.
Worth flagging that this is the second thing in this PR I wrote as a guard and never ran: the guard against a hole, and the hole it guards against. Thanks for running it rather than reading it.
| export function needsActor(definition: OperationDefinition): boolean { | ||
| for (let program of [ | ||
| definition.program, | ||
| definition.input, | ||
| definition.output, | ||
| ]) { | ||
| if (program && callsActor(program.source)) { | ||
| return true; | ||
| } | ||
| } | ||
| return definition.fill !== undefined && templateReadsActor(definition.fill); | ||
| } |
There was a problem hiding this comment.
[Claude Code 🤖] The enumeration misses definition.items, the other marker-carrying template a definition holds. read.ts's refuseUnservedStages already writes the full set down — program, input, output, fill, items, of, query — and lowerItem passes { $ref: 'actor' } straight through templateOf with no issue raised whenever the item's field is not a link, so a containsMany of a primitive can carry one today.
Nothing consumes definition.items yet, so this is latent rather than live. When a named appendContainsMany starts substituting its template, an anonymous caller gets resolveMarker's 400 "No actor in scope" instead of the 401 this gate exists to produce — which is the "refuse part-way through for a reason that reads as a payload problem" the comment above rules out.
Adding items beside fill is a one-liner. Better, if readsActor moves to lowering (see the thread on the bxl-emit import), the question is answered where the whole declaration is in hand and there is no list to keep in sync.
Follow-up sized, non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Covered by the readsActor move in 94d6158680 rather than by adding items to a list — lowering walks fill and items both, and it has the whole declaration, so there is no enumeration left here to drift.
The three lowering cases are pinned in operation-lowering-test.ts: a program naming actor(), a fill carrying the marker, and an output projection reading it. Those deepEquals are whole-operation comparisons, so they would have gone red in CI on the new member had it been wrong — which is how I found that three of them lower declarations that read the actor.
| // `lid` is the one member of `data` that is never a param: it is the caller's | ||
| // id for the card the entry mints, which is what other entries link to it by, | ||
| // and an operation declaring a param under that name would have the two | ||
| // meanings arrive in one key. | ||
| export function paramsFor(entry: EnvelopeEntry): Record<string, unknown> { | ||
| if (entry.lid === undefined) { | ||
| return entry.data ?? {}; | ||
| } | ||
| let { lid: _lid, ...params } = entry.data ?? {}; | ||
| return params; | ||
| } |
There was a problem hiding this comment.
[Claude Code 🤖] lid is not the one member of data that is never a param. targetFor reads data.meta.adoptsFrom to scope a class-scoped entry, and the appendContainsMany arm reads data.field / data.items / data.fields — and all four still go through to the operation as params.
Live today on the create path: a class-scoped {"boxel:name": "addActivity", "data": {"meta": {"adoptsFrom": …}, "title": "x"}} hands an operation declaring a param named meta the adoptsFrom wrapper. resourceFromTemplate only checks that declared params are present, so nothing catches the collision — it is the same failure mode the lid strip exists to prevent, one key further along.
Either strip every member the envelope reads for itself, or give params their own member (data.params) and stop overloading data for a named operation — the second also stops the list growing every time the wire learns a new member. Whichever way, the comment wants to state the rule rather than name one key.
Regression, non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 285c4a9525, taking the first option rather than a new data.params member — the wire shape is specified as data being the payload, and moving it is a decision about the extension rather than about this bug.
paramsFor now takes out every member the envelope reads for itself — lid, meta, field, items, fields, content — and the comment states that rule rather than naming one key, with the obligation that anything added to that set lands here in the same commit. Your class-scoped meta collision is the case that made it concrete.
| case 'appendLine': | ||
| return { | ||
| op: 'appendLine', | ||
| definition, | ||
| params: paramsFor(entry), | ||
| href: hrefRequired(entry, 'appendLine'), | ||
| }; | ||
| case 'appendContainsMany': |
There was a problem hiding this comment.
[Claude Code 🤖] This is the first transport to route a write through dispatch, which makes an input-shaped declaration reachable — and nothing on the write side runs one.
assertAppendsALine in packages/base/operations.ts accepts an appendLine that declares no line param as long as it declares an input program ("…or an input program that produces one"). Such an operation resolves, stages, and stageAppendLine refuses it 400 with "an appendLine appends the line its payload carries under line, and this one carries none" — blaming a payload the author's own declaration said would be produced. There is no reader of definition.input or definition.output anywhere in executors.ts or coordinator.ts, so the same holds for a declared transform with an output projection: it runs and answers the plain identity result, silently unprojected.
read.ts already has the shape for this — refuseUnservedStages refuses a specialization carrying an unserved stage with a 501 that names it. The write bases want the same refusal, and batchEntryFor is the cheapest place for it since the definition is already in hand.
Pre-existing in the core, now load-bearing: this endpoint is what makes a declaration relying on those stages reachable at all. Non-blocking if the scope paragraph in the description grows a line saying a write's input/output are not run either; blocking if you'd rather not ship a 400 that misattributes it to the caller. Codex raised the appendLine half.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 285c4a9525 rather than documented — a 400 blaming the caller for a value the author's own declaration said would be produced is the wrong answer to ship, and you are right that it is wider than appendLine.
batchEntryFor refuses any definition carrying input or output with a 501 naming the stage, the same shape as refuseUnservedStages. That covers the declared transform with an unprojected output as well as the appendLine half. The read path already had it, so both are now refused rather than half carried out.
The description's scope paragraph says a batch runs neither stage, since that is now a stated property rather than a gap.
…the realm's graph Whether an operation reads the invoking actor is a property of its declaration, so lowering records it next to `deterministic` and the envelope reads a boolean. That also covers every template a marker can hide in rather than the four members one function remembered, and it keeps the BXL package out of the typecheck program of everything that reaches the realm. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A registered media type now carries a parameter of its own, so comparing whole header values answered for one spelling of it and left the rest unrouted. The lookup compares the type and its extensions, preferring the registered type whose extensions the candidate carries. An entry's href is resolved out of registered-prefix form and canonicalized once, before the definition is resolved, so a declared operation is found on every spelling of a card's URL and the identity an entry answers with is the one the realm serves that card under. A type reference that is not one is refused as the caller's mistake rather than thrown out of the resolver, a staged entry carries the caller's position into the coordinator instead of having one key corrected afterwards, params are what is left when the members the envelope reads for itself are taken out, and a declaration carrying a stage a batch does not run is refused by name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Preview deploymentsHost Test Results 1 files ± 0 1 suites ±0 2h 39m 57s ⏱️ + 7m 32s Results for commit 05aa838. ± Comparison against earlier commit 18df8b1. Realm Server Test Results 1 files ± 0 241 suites +5 1h 27m 38s ⏱️ - 1m 10s Results for commit 05aa838. ± Comparison against earlier commit 18df8b1. |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-boxeloperations-envelope-endpoint-postquery # Conflicts: # packages/runtime-common/card-operations/coordinator.ts
backspace
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This pass went after convergence rather than the endpoint again: the diff since 9a947904, each thread's fix read as a change rather than as a compliance check, a first review of the two commits nothing asked for (6c3aacaa, 18df8b16), and the merge with main checked for a resolution that dropped a hunk from either side. The operation core, the dispatch change and the parts of the envelope no thread touched I did not re-review.
No blocking issues remain — approving. Every thread's fix lands, and each took the route the thread recommended rather than the cheaper one. Two new non-blocking findings came out of the fixes themselves; neither should hold the merge.
Dispositions, in thread order:
bxl-emitimport — resolved, and at lowering rather than by hiding the import.lowering.tsis now the only importer ofbxl-emit.ts, andrealm.tsimports the card-operations modules individually rather than through the barrel that re-exports it, so bxl is off the pathpackages/postgresreaches. Lint is green on the head.- Media-type routing — resolved in the lookup, as the thread asked. The widening it brings has one edge of its own: recommendation 2 below.
- Un-canonicalized and prefix-form
href— both halves resolved, and the canonical spelling now reaches the definition, the read and the staged write from one place. The prefix half rides on#resolveAtomicHrefand has no envelope-level pin; nothing else on the endpoint covers it either, so it stands on that helper's own coverage. - Malformed
adoptsFrom— resolved.isCodeRefis the recursive check:{type:'fieldOf', card:null}passes thetypeofgate and the recursion answers false, so it refuses 400 rather than throwing out of the resolver. - Batch positions — resolved wholesale.
positionsOffeeds every site that reports one, includingmeta.conflictsWithand the prose no post-hoc remap could reach, and it survived the merge with main intact. answeredover a hole — resolved.definition.items— resolved by the lowering move, with no list left to drift.paramsFor— themetacollision is resolved; the strip is now wider than any arm reads. Recommendation 1 below.input/outputon a write — resolved with a 501 naming the stage, matching the read path's status, code and title.
Codex's four findings are the same as 2, 3, 4 and 9.
Recommendations:
paramsForsubtracts four members that no arm reads for the base it is called on, which makes an operation declaring a param under one of those names answer 400 for a value the caller did send — thread onENVELOPE_MEMBERS. Non-blocking.- The second matching pass lets an
Acceptcarrying a parameter select a family whose routes do not cover the path, and the lookup does not then fall back toContent-Type— thread on the second pass inextractSupportedMimeType. Non-blocking. - The comment above
BOXEL_OPERATIONS_EXTstill says the router matches the whole string — inline suggestion.
CI: the tip before the merge (18df8b16) was green across all 63 checks, including the realm-server shards that hold the CI-only actor assertion. On the merge head, Lint has reported green and the test shards are still running — so this approval rests on that run plus the merge resolution, not on a green run of this SHA.
Adjacent, out of scope: the card write verbs now thread a RequestTimings through commitBatch and log the lock / drain / stage / write split, and the envelope's commitBatch call passes none — so a slow batch on the new front door reports no stage split where a slow PATCH does. Worth a follow-up once the endpoint carries traffic.
| const ENVELOPE_MEMBERS = [ | ||
| 'lid', | ||
| 'meta', | ||
| 'field', | ||
| 'items', | ||
| 'fields', | ||
| 'content', | ||
| ] as const; |
There was a problem hiding this comment.
[Claude Code 🤖] The strip is wider than the arms that read these members, so an operation declaring a param under one of these names becomes uninvokable — and the refusal blames the caller for a value they did send.
paramsFor is the payload for transform, appendLine, appendContainsMany and a named create. Of the six names, two are read for every entry: lid in parseInvocation, and meta in targetFor — and that one only when the entry names no href. content is read by the update arm alone, and field / items / fields by the appendContainsMany arm alone.
So a type declaring @operation static addNote = { base: 'transform', params: { content: StringField }, … }: the caller sends data.content, paramsFor drops it, and resolvedParams in executors.ts refuses 400 — operation "addNote" requires a value for params("content") for a value that was in the payload. That is the shape the input/output 501 was introduced to avoid, one key along.
Two ways out. Subtract per base — lid and meta for every entry, content only where the update arm reads it, field/items/fields only in the append arm — which keeps the rule this comment states and narrows it to what is true of the entry in hand. Or keep the flat set and have lowering refuse a declaration whose param name is one of these, so an author finds out when the module is indexed rather than a caller finding out at invocation.
Regression, non-blocking: it needs a card type to name a param one of four words, and nothing can be relying on this endpoint yet.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 7143d37116, taking the per-base route rather than the lowering refusal — it keeps the rule the comment states true of the entry in hand, and it does not spend an author's param namespace to pay for the envelope's.
Your addNote case is what made the choice: refusing the declaration at lowering would tell an author they may not name a param content, which is a real word for a real field, and the envelope's reason for wanting it has nothing to do with them.
What landed is narrower than either option, because working through the arms showed content never needed subtracting at all: the two update arms build their entry from data directly and never call paramsFor, so the only reader of content is not on this path. So it is lid and meta for every entry, plus field/items/fields in the append arm alone — four names freed rather than the two your first option would have.
| for (const candidateMimeType of acceptMimeTypes) { | ||
| let matched = matchParameterized(candidateMimeType, supportedMimeTypes); | ||
| if (matched) { | ||
| return matched; | ||
| } | ||
| } |
There was a problem hiding this comment.
[Claude Code 🤖] This makes Accept win for headers it used to miss, and lookupRouteTable falls back to Content-Type only when the chosen family has no routes for the method — not when it has some and none of them match the path. So a request that routed before can now be answered as no such route.
POST {realm}/_operations with Content-Type: application/vnd.api+json;ext="…/operations" and Accept: application/json; charset=utf-8: the exact pass misses that Accept as it always did, this pass now answers application/json, and that family's POST map holds /_cancel-indexing-job, /_reindex and /_full-reindex. routes is therefore truthy, /_operations matches none of them, and the lookup returns undefined without ever reading the Content-Type that named this endpoint. The same request reached handleOperations before.
The fallback's own comment already describes what would close it — "Fall back to Content-Type when Accept doesn't match a route" — so the fix is to make that true of a route rather than of a route family: resolve the handler from the Accept family first, and fall through to Content-Type when it comes back empty.
Either way, the claim here and in the description — that this only adds a match, so every header that routed before routes to the same place — is true of extractSupportedMimeType and not of the lookup around it, and wants narrowing to the function it holds for.
Regression on top of a pre-existing shape in the lookup, non-blocking: it needs an Accept carrying a parameter on a registered type, and the only parameterized Accept in the tree is the browser-shaped one on the screenshot GET, which lands on */* — a family with no routes at all, so it still falls through.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 7143d37116, and you were right that my claim was scoped to the wrong function.
lookupRouteTable now matches the path inside the Accept family first and falls through to Content-Type when that comes back with no route — not when the family holds none for the method. Path matching moved into a matchRoute helper so both passes use it, and the fall-through skips a redundant second pass when the two headers resolve to the same family.
Your exact header pair is pinned: POST /_operations with Accept: application/json; charset=utf-8 and the operations Content-Type now reaches the handler and answers 200. Before the fix it would have selected the application/json family, matched none of its three POST paths, and returned undefined without ever reading the content type.
Worth saying plainly: this is a pre-existing shape in the lookup that my widening made reachable, so the fix is a strict improvement over both the before and after of this PR — a request whose Accept names a family that does not serve the path now falls through where it never did.
| // is what a plain `application/vnd.api+json` body does not have — so the media | ||
| // type below carries it as the `ext` parameter and the router matches on the | ||
| // whole string. |
There was a problem hiding this comment.
[Claude Code 🤖] Stale as of the lookup change: the router no longer matches this media type by comparing the whole header value, which is the gap the second pass in extractSupportedMimeType exists to close. The next reader of this file gets the old rule.
| // is what a plain `application/vnd.api+json` body does not have — so the media | |
| // type below carries it as the `ext` parameter and the router matches on the | |
| // whole string. | |
| // is what a plain `application/vnd.api+json` body does not have — so the media | |
| // type below carries it as the `ext` parameter, and the router matches a media | |
| // type by its type and the extensions its `ext` names. |
Non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Applied your suggestion verbatim in 7143d37116. The comment was describing the rule the second pass exists to replace, which is the worst kind of stale — it reads as current and is load-bearing for whoever changes that constant next.
A named operation declaring a param under one of the members the envelope reads for itself became uninvokable: the value was taken out of its payload and the executor then refused the operation for carrying none. Only the local id and the type are read for every entry; the rest belong to one arm and are subtracted there. The route lookup falls through to the content type on failing to match a route rather than on the family holding none for the method, so an Accept that selects a family with routes for other paths no longer hides the content type that named the endpoint. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Background
A realm today answers one plain read or one plain write per HTTP request.
GET {realm}/thingreads a card,PATCHmerges a patch into one,DELETEremoves one. That covers a great deal, but there are three things the REST verbs cannot say at all:addComment,escalate,createActivity— as plain data that lowers to a bounded expression program. There is no verb for "run the operation this type callsaddComment".GEThas nowhere to put a payload.The machinery for all three already exists inside the realm. A card type's declarations are lowered when its module is indexed and stored in the type's definition-cache entry.
runOperationresolves a name against that entry and runs the resulting behavior.commitBatchstages a list of writes in memory under the realm's write lock, commits them together, enqueues one index job and broadcasts one event. What has been missing is a way for a client to reach any of it: the only front door was the REST facade, which can only express what its verbs express.This adds the second front door — one endpoint that carries a batch — without adding a second implementation of anything behind it.
What the endpoint is
POST {realm}/_operationsandQUERY {realm}/_operations, under a new media type:application/vnd.api+json;ext="https://boxel.ai/ext/operations". The body is a JSON:API document in the shape of that specification's atomic-operations extension, with one Boxel-specific verb:Every entry uses the same verb,
invoke. The wire never names a base operation.boxel:namenames the operation as an author declared it, and which built-in behavior actually runs — a read, a create, a merge, an expression program over the card's stored JSON — comes from the target's own definition. That is what makes a card type able to redefine whatdeletemeans for its cards without the client knowing.hrefis relative to the endpoint's realm, so/reports/xmeans{realm}reports/x— the realm's root, not the server's origin. An absolute URL is accepted too, and so is a registered-prefix identifier, but whichever form it takes it must resolve inside the same realm; a batch commits under one realm's write lock, so there is no lock that could make a cross-realm write atomic with the rest. The href is folded to the spelling the realm addresses the card by before anything reads it — a trailing slash, a query string and a fragment all name the card they hang off — so a declared operation is found on every spelling of a URL and the identity an entry answers with is the one every other surface uses.lidis the caller's own id for a card the batch is about to mint. A later entry references that samelidwhere a linked card belongs, and the realm resolves it to the URL it is about to give the new card. This is the existing identity chain the cardPOSTpath already threads, not a new concept.What comes back
Results are positional, in
atomic:results, one per entry:meta.version/generation/lastModified— and nothing else. A create also echoes itslid, which is the only thing tying the minted URL back to the name the caller used. The written document is not reprinted: the caller wrote it, and what it usually needs back is the version to reconcile against.{ "data": null }. There is no state left to describe.GETassembles it.Any failure rejects the whole batch with a JSON:API
errorsarray carrying the status the refusal chose, and the index of the entry that caused it. Nothing is written, no index job is enqueued, no event is broadcast.The media type, and why the router changed
The envelope is
application/vnd.api+jsonqualified by anextparameter naming its extension. That is the first registered media type in the realm carrying a parameter of its own, and the router compared whole header values — so only the one byte-exact spelling routed, and every other legitimate way to write the same media type (a space after the semicolon, an unquoted value, acharsetalongside, the extension's URI in a list with another's) fell through to a path nothing serves.So the lookup now compares a media type by its parts: the type and subtype, and the extensions its
extparameter names. It prefers the registered type whose extensions the candidate carries, falling back to the unqualified one — so a body naming this extension reaches this endpoint, and a body naming some other extension reaches the plain JSON:API family and is told what it is missing. This only ever adds a match where comparing whole values found none; every header that routed before routes to the same place.How the method decides what a batch may do
The realm derives the permission it checks from the HTTP method, before any body is read:
POSTneeds realm write,QUERYneeds realm read. So a batch that writes is sent as aPOSTand a read-only batch as aQUERY— the same method/_searchuses, with the existingPOST+X-HTTP-Method-Override: QUERYfallback for clients that cannot send it. AQUERYcarrying an entry that writes is refused with a 400 naming that entry, because that request was authorized to read and the permission check has already passed for the wrong question.Mixed batches, and what "pre-batch state" means
A batch can mix reads and writes. Every entry evaluates against the state the batch started from: a read entry never sees what a write entry in the same batch stages, so there is no read-your-own-writes inside a batch. The endpoint gets this by running every read before handing the writes to the coordinator, which also means a read never waits on the write lock. To fetch a document a batch wrote, read it after the batch commits.
Two things about identity
The caller's Matrix user id — the identity the realm's permission check already verified — travels into the batch as the
actoran operation's program can read. It is passed exactly as the realm authenticated it, and nothing is invented for an anonymous caller: a fabricated id would end up written into cards and compared in filters as though somebody had acted.On a realm that anyone may write, an anonymous request can still arrive. Whether an operation needs an identity is decidable from its stored definition alone — the programs it runs and the template a named create fills are the only places an actor can be read — so the whole batch is refused up front with a 401 rather than part-way through by whichever entry happened to reach the actor first. An operation that reads no actor is carried out for an anonymous caller as usual.
Caching
Envelope responses are never HTTP-cached:
Cache-Control: no-storeon both methods, and noETag. A batch is not a resource with a validator — the same request run twice writes twice — and a read entry's document is served without the index-time validator the card+jsonGETbuilds, so there would be nothing for a conditional request to be answered against. Only the plainGETfacade participates in HTTP caching.Posture (read this one)
Operations are identity-aware but not access-enforced. The realm's own read/write permission is the whole of what this endpoint checks. Any caller who may write the realm may invoke any operation that writes it; any caller who may read it may invoke any read. An operation's program can read the actor and an
assertcan refuse on what it finds, but the realm verifies no claim beyond the permission check and refuses nothing on the strength of who is asking. Every operation's result should be treated as reachable by any permitted caller of the realm until enforcement ships. This is stated on the handler as well as here.Where the code is
packages/runtime-common/card-operations/envelope.ts(new) is the whole of the wire format: reading the body into entries, resolving eachhrefagainst the realm, deciding what an entry'sdatameans for the behavior its name resolved to, and shaping the results. It reads no file, consults no index and resolves no identifier — it takes text and plain data and gives back entries the core already understands — so it runs wherever the operation core runs and is testable without a realm.The entry vocabulary is a discriminated union on
opwith exactly one member today. The extension also defines group verbs (parallel,serial) whose members are batches in their own right, and an entry may eventually name its target by query instead of byhref; both are read here as verbs this endpoint does not carry rather than as malformed entries, so adding one is a new arm rather than a reshaping of the parse.packages/runtime-common/realm.tsregisters the routes and holdshandleOperations, which is the only place that touches the realm. It parses, resolves each entry's name throughresolveOperation, runs the reads throughrunOperationand hands the writes tocommitBatch— the same two entry points the card verbs dispatch into. It reimplements nothing: staging,lidresolution, the size ceilings, the write lock, the single index job and the single event are all the coordinator's, and the duplicate- and dangling-lidrefusals come from there rather than being restated here.The routes are registered under the extended media type and under plain
application/vnd.api+json. Matching only the extended spelling would leave a body sent without theextparameter — the near miss a client actually makes — falling through to a path nothing serves and answering "no such route" to a request that named this one. The handler reads the parameter itself and says what is missing.packages/runtime-common/card-operations/dispatch.tsgains one thing. A target's kind is worked out from its URL, using the table of registered file extensions, and that table does not name every extension a realm stores — a.log, a.css, a.ymlholds bytes and serves them, and each is classified as a card. Areadsurvives this because a card carries a read too and its executor falls back to the file-metadata document. A file-only write has no such overlap, soappendLineon an instance target is now admitted whatever its extension classified it as, and the discrimination happens in the executor, which reads whether the path holds a card's JSON or plain bytes and already has to judge the content type to decide whether a line may be appended at all. This endpoint is the first transport to route a write through dispatch, which is what makes that reachable.Scope
A batch runs no
inputoroutputtransform. Those stages are a declaration's way of reshaping its payload and projecting its result, and no write executor reads them — so rather than carry such an operation out as though the declaration said nothing, an entry whose definition carries one is refused with a 501 naming the stage.queryandreadSourcedo not travel through the envelope, and an entry naming either is told where it belongs: a query is planned and run on the search engine, and stored bytes are served by the card-source and byte routes. A batch is a flat list — groups, and targets named by query, are their own work.input/outputtransforms are not run yet, so a card type that specializesreadis refused rather than served the plain document./_atomicand every card verb are untouched; no existing route changed.Tests
packages/realm-server/tests/realm-endpoints/operations-test.tsruns against two fixture realms — one anyone may read and write, one anyone may only read — with a card type whose module declares real operations, indexed so the declarations are lowered into the definition cache the way an author's would be.It covers the validation matrix (an href outside the realm, a name the target does not carry, a body sent without the extension, an entry naming a query or a stored-bytes read, a
QUERYcarrying a write, a group verb, a body that is not an envelope, a duplicate local id, a local id nothing creates); a named expression program running and answering with the identity it wrote; a class-scoped create and a second entry linking to it by local id, with the local id echoed back; a delete answering with no state; an href relative to the realm; a line appended to a file whose extension the realm does not register; a file reading as its metadata document and a card behavior on a file href being refused; a failing entry leaving the batch unwritten with no index job and no event, and naming the entry the caller sent rather than its position among the entries that write; a mixed batch whose read answers with the pre-batch document while the write in the same batch lands;Cache-Control: no-storewith noETagon both methods; an anonymous request refused with 401 for an operation that reads the actor and carried out for one that does not; the actor recorded being the user the realm verified; and aPOSTrefused 403 where aQUERYof the same batch answers 200.It also covers the transport's edges: every spelling of the media type reaching the endpoint, an equivalent spelling of an href naming the same card and answering with the canonical id, a type reference that is not one being refused rather than thrown out of, an operation declaring a stage a batch does not run being refused by name, and a mixed batch's refusal naming the entry the caller sent in its keys and in its prose rather than the position it took among the entries that write.
One assertion in that suite — the 401 for an anonymous caller invoking an operation that reads the actor — is verified in CI rather than locally. Whether an operation reads the actor is recorded when its module is indexed, and that lowering runs in the prerender host; a local stack serves a host bundle built from whichever checkout started it, so no locally-built definition can carry the flag. CI's realm-server job restores a host built from the branch, which is why it holds there.
The dispatch suite pins the moved boundary from the other side: a file-content write resolves for an instance target whatever its URL classified it as, and the refusal for a card target stays pinned where the executor makes it — with the card's stored file left byte-identical.
🤖 Generated with Claude Code