Skip to content

Let a batch entry describe the card it runs against instead of naming it - #6195

Open
habdelra wants to merge 10 commits into
mainfrom
cs-12928-card-ops-query-defined-targets-boxeltarget-resolves-an
Open

habdelra wants to merge 10 commits into
mainfrom
cs-12928-card-ops-query-defined-targets-boxeltarget-resolves-an

Conversation

@habdelra

@habdelra habdelra commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Background

The realm has a second front door onto card operations besides the card verbs: POST/QUERY {realm}/_operations, which takes a batch — a list of entries, each naming one operation to run — and commits the whole thing or none of it, under one write lock, with one index job and one event. An entry looks like this:

{ "op": "invoke", "boxel:name": "escalate", "href": "/reports/q3" }

boxel:name is the operation, and href is the card it runs against. That href is a URL the caller already has, and for a lot of batches that is the whole story — an editor saving a card knows which card it saved.

It is not the whole story for the batches the operations envelope exists to make possible. "Mark every open report assigned to me as reviewed" knows the shape of its targets, not their URLs, and there is no count to know in advance either. "Create an activity, then update the classroom that activity belongs to" knows a card one link away from a card it can describe. Today both of those mean a round trip: search first, read the ids out of the response, then send a batch naming them — which is two requests, and the state can move between them, so the batch you send is about the realm as it was a moment ago rather than as it is when the lock is taken.

What an entry can say now

In place of href, an entry may carry boxel:target:

{
  "op": "invoke",
  "boxel:name": "escalate",
  "boxel:target": {
    "query": {
      "item.on": { "module": "…/report", "name": "Report" },
      "eq": { "item.status": "open" }
    },
    "field": "owner",
    "expect": "many"
  }
}

query is a filter written in the grammar the realm's own _search endpoint already takes — field paths addressed through item., the card type as item.on, and concrete values throughout. A declared query operation may write actor() or params("x") and have lowering resolve it at invocation; a boxel:target is written by the caller, who holds those values already, so a marker here is refused rather than resolved. That is worth a refusal rather than a pass-through because the search grammar accepts one — {"$ref":"actor"} is a well-formed operand — so it would be compared against a stored value as a literal object, match nothing, and be indistinguishable from a filter that genuinely matched nothing.

The check is narrow, because an operand is a literal JSON value and a card field may store an object: an object reads as a marker only when its $ref names one this runtime resolves and it carries no member beyond the ones such a marker takes, both lists read off resolveMarker. So {"$ref": "#/definitions/Person"} filters for as data. What that leaves unreachable from a query target is a field storing precisely {"$ref": "actor"} and nothing else, where the data is indistinguishable from the marker; an href entry and _search both still reach it.

It is the filter alone, not a whole search request: the members of that grammar which address realms, choose a page and pick a projection are the realm's to fill in, not the caller's. That is what makes a result outside the endpoint realm impossible rather than merely refused — the search runs on this realm's own index engine, and there is no member a caller could set to point it elsewhere.

field follows one link from each matched card, so the entry runs against what the card points at rather than against the card itself. One immediate field name, no dotted paths, and it has to be a field that actually holds a stored link. Five shapes are refused, each saying which it was: a field that does not exist, a contains field, a computed link, a link resolved by a query, and a link that leaves this realm.

The last two are the ones worth naming, because both would otherwise pass for "this card has no link". A field declared with { query } and no computeVia reaches the hop with isComputed false — a definition entry records that from computeVia alone and carries the query beside it — and a query-backed field's members are written onto the served copy of a card, never onto the stored row, so walking the row finds nothing. And a scoped reference whose prefix this process has not registered is stored verbatim rather than relativized, precisely because it is cross-realm by construction; it is not a URL, so joining it against the matched card would have produced a path inside this realm that nothing stores, and the out-of-realm refusal would never have fired for the one spelling that is always out of realm.

A card the query matched can also stop being readable between the search and the hop — its row deleted, or reindexed into an error row. Neither is the caller's request being wrong, so they carry target-not-found and target-errored rather than invalid-params, and the errored one carries the row's own message so the caller learns why.

expect is "one" (the default) or "many". Under one the entry answers with a result, the way an href entry does. Under many the entry answers with an array of results — one per card it found — and no matches is an empty array rather than a refusal, since "I do not know how many there will be" includes none.

Carrying both href and boxel:target is refused, and so is a query target on an operation that creates a card.

How resolution works

Resolution is one pre-step over the whole batch, sitting between reading the envelope off the wire and asking the core which behavior each entry's name resolves to. It answers the same tree it was handed, with every query target replaced by entries carrying an ordinary href.

That replacement is the whole design, and it is worth being explicit about what it buys. Nothing downstream of that pre-step knows a target was found rather than named. So a found target takes the batch's write lock along with everything else, collides with a parallel sibling under the existing conflicting-targets rule, resolves its operation against its own card's type, participates in lid resolution, and rolls back with the rest — and none of those had to be taught about query targets, or restated for them. There is no second code path.

The filter runs against the realm's index as the request found it. That is the same pre-batch state everything else in a batch is evaluated against: a read entry never sees what a write entry in the same batch stages, and a filter never matches a card an entry of the same batch creates, because nothing the batch writes exists yet, let alone is indexed. That is the mistake this shape invites, so the zero-match refusal says it outright when the batch is one that mints cards.

An expect: "many" entry resolves into a serial group of one entry per target. A group is already how the envelope spells "the results for this position go in an array", and serial is what carries the targets out in the order the index returned them. Because the group stands exactly where the caller's entry stood, an expansion inside a parallel group is one branch of that group, and its targets still collide with the siblings beside it — which is how a query-found target and a sibling's href landing on the same file is caught. Zero matches is an empty group: nothing staged, [] in the results, no lock taken and nothing announced.

Each expanded entry is named by the member that produced it and its place in what that member found — [1].boxel:target[0] — in the same path spelling a group's members already use. That one value is what meta.entry carries, what meta.conflictsWith names, and what the prose of a refusal points at, so a caller that sent one entry and got a refusal about the third card it matched can tell which one that was.

The search reads identities, not renderings. A search with no fieldset gets the default, which is the selected prerendered HTML plus a side-load of every match's links — a great deal of work to answer "which cards exist". The query resolution issues pins a sparse fieldset instead, so the projection is the cheap one and no link assembly runs. It also pins the scope to cards: a realm-wide query answers with a card's own row and the dual-indexed .json file row beside it, and counting both would make one card read as two matches. An entry expecting one card pages at two rows, which is enough to tell none from one from several, and takes the number its refusal reports off the result meta — which counts every matching row whatever the page held, so the refusal names the real count even when it is large.

Decisions and trade-offs

The count rule applies to matches, and then again to the hop. expect: "one" requires exactly one match, and then requires that match's link to yield exactly one card; a linksToMany hop under one is refused naming how many it links to. The alternative — letting a collection hop widen the entry to all of them — makes the shape of a slot in the response depend on the arity of a field on the matched type rather than on what the caller wrote. Whether a position holds a result or an array of results is the caller's to choose, and expect is where they choose it.

A create's refusal is decided after resolution rather than at parse. The wire never says that an entry creates a card; the operation's name does, and what that name means is read off the type of a card the query had to find first. So a resolved entry carries a mark saying its href came from a query, and the create arm refuses on that mark. A base create with a query target would have been caught regardless, by the existing refusal for a create that names an href. A named create would not: a named create may legitimately carry an href — the card it reads for context — so without the mark a found target would have been quietly read as that context card and the batch would have succeeded doing the wrong thing.

An expect: "many" expansion is bounded by the realm's own search bounds, not by a number invented here. Resolution reaches the index engine directly, so it has to apply what the search route applies to its item leg: the expansion is paged at SERVER_ABSOLUTE_MAX_PAGE_SIZE — documented as the size no item-leg request may exceed however deliberately it asks, and already an operator's to tune — and the search runs under runWithSearchTimeBudget, with the signal threaded in so an abandoned search stops rather than running on under a request that has already been answered. A budget timeout keeps its own 408 and is labelled with the entry.

Going over the ceiling is refused, not truncated. A caller that asked to run against every card answering a filter and is silently handed the first 2000 of 3000 is told its batch succeeded, so the entries it believes it wrote are exactly the ones it will never go looking for. One row beyond the ceiling is requested, so going over is visible in the rows rather than in a count that can describe a different snapshot.

An entry that matched nothing is gone before the definition gates run. Two gates read an entry's definition: whether a read-only QUERY batch is carrying a write, and whether an anonymous caller invoked something that reads actor(). A definition comes from the type of the card an entry targets, so with no match there is no card and no question to answer — a QUERY batch holding an expect: "many" entry that matched nothing answers 200 with [], where it would have earned a 400 had the query matched. Nothing is written either way and the realm's own permission check runs as it always does; what differs is which answer a caller sees for an entry that did nothing. The code says so where it decides it.

A card reached several ways is one target. A hop converges — every open report may name one owner — and the entry names the cards it runs against, not the routes by which it found them, so the hop's targets are deduplicated in the order they were first reached. Nothing downstream would have caught the duplicate: the expansion is serial, and serial composes two changes to one file rather than refusing them, so conflicting-targets sees nothing wrong. An idempotent update would hide it; three appends would not.

Both fan-outs are held to the same width, for the same reason. Following a link needs the matched card's index row, and resolving an entry costs a search (two statements, since the engine runs the rows and the count together). How many entries a batch carries is as much the caller's number as how many cards one of them matches, so neither axis gets to decide how much of the realm's connection pool one request holds: the hop walks its matches one at a time, and the per-entry resolutions run at most STAGING_WIDTH at once. The sequential hop also settles which refusal is reported without having to sort them — the earliest match that cannot be followed is the one that throws — and the bounded map keeps the same property, since a rejection is carried at its index rather than thrown.

Where the changes live

  • card-operations/envelope.ts — the wire grammar for boxel:target and the create refusal. It still reads no file and consults no index; the shape of a query target is text, and what it resolves to is not.
  • card-operations/find-targets.ts — new. The resolution pass: the query it issues, the count rules, the link hop, and the tree it answers with.
  • card-operations/dispatch.tsOperationIndexQueryEngine gains searchEntries, mirroring the realm index query engine method for method the way its other members do.
  • realm.ts — one await resolveQueryTargets(...) between parsing the envelope and walking its entries, sharing the request's existing index-row memo.

Tests

packages/realm-server/tests/card-operations-find-test.ts is new, stubs the realm's index and definition cache, and runs standalone in seconds. It carries the wire grammar, the count rules, the hop, and the shapes resolution answers with — including assertions on the query the realm actually issues, so "this reads identities and not renderings" is checkable rather than asserted, and a resolveWith helper that binds the realm's identifier map so the registered-prefix arm of a hop is exercised at all.

Every case in it was run against a scratch mutation of the code it names, and each mutation reddens exactly the cases that claim it: dropping the cards scope, dropping the page, scheduling an expansion in parallel, naming expanded positions as group members, following a collection under the field's own name, not recursing into groups when substituting, following a computed link, following a query-backed link, joining an unregistered scoped reference against the card, skipping the identifier map, dropping the hop's dedupe, collapsing the two row-race refusals back into one, letting a hop leave the realm, and removing either half of the create refusal. The concurrency bound is not a clock race — every worker increments the in-flight count synchronously before its first await, so the peak is deterministic: STAGING_WIDTH + 4 entries peak at exactly STAGING_WIDTH, unbounded reads 12 against 8, and a three-entry control peaks at three so the bound is a ceiling rather than a schedule.

realm-endpoints/operations-test.ts carries the end-to-end half against a real realm: one match writes that card and reports its id; zero and several are each refused naming the count; expect: "many" writes every match, answers with an array of lean results and indexes under one job; zero matches under many answers [] and enqueues nothing; a refusal from inside an expansion names which found target produced it; a field hop writes the linked card and leaves the matched one alone; a non-link field is refused naming what it is; a card the batch itself creates is not matched; a found target collides with a parallel sibling that named the same card; a create with a query target is refused; and a read may describe its target too.

Locally: 653/653 across the six standalone card-operations suites, and 97/98 running both changed files through the real harness in one pass — TEST_FILES and TEST_MODULES each naming realm-endpoints/operations-test and card-operations-find-test, which is what puts the new suite's 44 tests through the same load path CI uses rather than only through a standalone driver — the one red being identity > an operation that reads the actor refuses a request that authenticated nobody, which is red on this machine for every branch. @operation lowering runs inside the prerender host's bundle, and the host dist this machine serves has no readsActor in it, so the endpoint's whole-batch actor gate has nothing to read and the executor's own refusal answers instead. CI restores the web assets from the branch and is green on it.

🤖 Generated with Claude Code

habdelra and others added 2 commits September 17, 2026 21:33
An `invoke` entry may carry `boxel:target` — a filter, an optional link to
follow from what it matched, and whether it means one card or many — in place
of the `href` a caller uses when it already knows the card.

Resolution runs once per batch, against the realm's index as the request found
it, and answers the tree it was handed with every query target replaced by
entries carrying an ordinary href. Nothing after it knows a target was found
rather than named, so a found target takes the write lock, collides with a
parallel sibling and rolls back exactly as a named one does.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The endpoint suite gains the half a stub cannot make: one match writes that
card, zero and several are refused naming the count, expecting many writes
every match under one index job and answers with an array, matching nothing
answers with an empty one, a field hop writes the card the match links to, and
a found target collides with a parallel sibling that named the same card.

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:21:21.759597Z ccae2b0 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: 2726d1c98a

ℹ️ 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".

match,
);
}
if (fieldDef.isComputed) {

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 Reject query-backed links before following them

When field names a linksTo or linksToMany declared with a query but no computeVia, getFieldDefinitions records the query in fieldDef.query while leaving fieldDef.isComputed false. The field therefore passes this check even though its target is dynamically computed; the pristine index row can then expose no stored relationship, causing expect: "many" to succeed with [] and silently skip the requested operations instead of refusing the unsupported hop. Check fieldDef.query here as well.

Useful? React with 👍 / 👎.

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. linksTo / linksToMany in card-api.gts pass queryDefinition: query and leave computeVia untouched, and getFieldDefinitions records isComputed: Boolean(field.computeVia) while carrying the query separately as query — so a field declared with { query } and no computeVia reaches this check with isComputed false. card-api.gts's own component path spells the same question as !!this.computeVia || !!this.queryDefinition, which the definition entry does not mirror.

The consequence is the one described: a query-backed collection is stripped from the stored serialization before it is indexed (omitQueryFields in card-serialization.ts, and file-serializer.ts skips computed relationships on write), and the umbrella plus field.N entries a query-backed field carries are written by applyQueryResults onto the served copy, not the row peekInstance reads. So linksOf finds nothing and expect: "many" answers [].

Worth following the shape already used one file away: unwritableReason in card-operations/lowering.ts treats fieldDef.isComputed and fieldDef.query !== undefined as two separate refusals with two distinct codes, on the same reasoning. A single condition here would collapse a distinction the sibling call site keeps, and the two refusals want different prose — one points at computeVia, the other at a query.

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 7b1db3f. fieldDef.query !== undefined is now its own refusal beside the computed one, with its own prose, following unwritableReason's split rather than collapsing the two.

Confirmed the mechanism before fixing: linksTo / linksToMany pass queryDefinition: query and leave computeVia untouched, while getFieldDefinitions records isComputed: Boolean(field.computeVia) and carries the query separately — so the field arrived here with isComputed false. card-api.gts spells the same question as !!computeVia || !!queryDefinition for its own component path, which the definition entry does not mirror.

Pinned by a case that asserts the query wording appears and the computeVia wording does not; removing the new condition makes it answer instead of refusing.

@github-actions

github-actions Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Host Test Results

    1 files      1 suites   2h 37m 0s ⏱️
4 895 tests 4 881 ✅ 14 💤 0 ❌
4 910 runs  4 896 ✅ 14 💤 0 ❌

Results for commit 6886c03.

Realm Server Test Results

    1 files    245 suites   1h 28m 6s ⏱️
3 687 tests 3 686 ✅ 0 💤 1 ❌
7 476 runs  7 475 ✅ 0 💤 1 ❌

Results for commit 6886c03.

For more details on these errors, see this check.

@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 🤖] Read the resolution pass against the machinery it hands its tree to — the tree consumers (invocationsIn, stagedTree, resultsTree, the coordinator's schedule), the query it issues into searchEntries, and the link hop — plus whether the new suite's cases discriminate. Not covered: the realm-server endpoint suite was not run here (shared lane), so the end-to-end half is read and not executed.

No blocking issue in the design; one blocking defect in the new test file, and four non-blocking asks.

  1. basename(__filename) in the new suite — thread on card-operations-find-test.ts. Blocking.
  2. hopTarget turns an unregistered scoped link into an in-realm URL instead of refusing it as cross-realm — thread on new URL(resolved, from).
  3. The unreadable-row refusal is invalid-params where this subsystem already has target-not-indexed / target-errored — thread on the row?.type !== 'instance' branch.
  4. The per-entry search fan-out is unbounded while the hop is deliberately sequential for the same connection-pool reason — thread on Promise.allSettled.
  5. The hop does not dedupe, and a serial expansion composes duplicates rather than refusing them — thread on the hop loop.

Codex's query-backed-link finding holds; the shape the fix should take is in its own thread.

Adjacent, not asked of this PR: the sparse item.id fieldset skips the HTML and the link assembly, but the dataOnly projection still selects i.pristine_doc for every row, so an unbounded expect: "many" ships every match's document across to extract its id. There is no ids-only projection to ask for today, and the rows are read again under the lock, so this is a note for whoever adds one.

CI: every Realm Server Tests shard is red, on finding 1.

return isGroup(node) ? node.op : undefined;
}

module(basename(__filename), function () {

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 🤖] basename(__filename) makes this the only file under packages/realm-server/tests/ that does not use import.meta.filename — 193 others do — and __filename is not bound in ES module scope, so the shard process dies at load and takes every shard whose module list includes this file. Switch to basename(import.meta.filename).

Regression, blocking: every Realm Server Tests shard is red on it.

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 7b1db3fbasename(import.meta.filename), and grep -l 'basename(__filename)' over the test tree now returns nothing.

Worth recording why it survived a local run: my endpoint run narrowed TEST_FILES to realm-endpoints/operations-test, which limits which files are loaded, so the real harness never loaded this file once. The only thing that ever loaded it was a scratch ESM driver — and the first time it failed with exactly this error, which I read as a driver artifact and silenced with a globalThis.__filename shim instead of asking why no neighbour needed one. The shim is gone, and the file now loads there unaided.

Comment on lines +443 to +446
let resolved = opts.resolveIdentifier?.(link) ?? link;
let absolute: URL;
try {
absolute = new URL(resolved, from);

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 turns a cross-realm link into an in-realm one for the one spelling the writer deliberately stores verbatim, so the refusal below never fires for it.

storedRelationshipLink in file-serializer.ts keeps a scoped reference whose prefix this process has not registered exactly as sent — its own comment names joining such a reference against a base as the bug it exists to prevent — and resolveIdentifier here resolves only registered prefixes (Realm#resolveAtomicHref), so the string reaching new URL still starts with @:

new URL('@someorg/somerealm/person', 'https://realm.example/cards/report').href
// 'https://realm.example/cards/@someorg/somerealm/person'

paths.local() accepts that, so instead of "this link leaves the realm" the entry runs against a card URL nothing stores, and the caller gets whatever refusal the missing row produces downstream.

Two ways out: refuse a link that still starts with @ after resolveIdentifier (cheap, and that is exactly the cross-realm case), or resolve the way the read path does — resolvedRelationshipLink(link, from, virtualNetwork) — which means threading the realm's own resolver in rather than resolveIdentifier.

Two smaller things on the same code. The comment above says both forms are resolved "the way an entry's own href is"; hrefIn parses absolute-or-realm-root through paths.fileURL and never joins against a base, so a card-relative link is a genuinely different resolution and the sentence reads as though it were the same one. And nothing exercises this arm: resolve() in card-operations-find-test.ts calls resolveQueryTargets with three arguments, so opts is {} and resolveIdentifier is unbound in every case, while the endpoint fixtures store only ./-relative links. A case with a bound resolver and a prefix-form stored link would pin whichever answer you pick.

Regression. Non-blocking — the outcome is a misdirected refusal, not a wrong write.

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 7b1db3f, taking the cheap branch: a link that still starts with @ after resolveIdentifier is refused as naming a realm this one has no prefix registered for. That is exactly the cross-realm case, and refusing is the outcome either way — threading the realm's resolver in would buy a better URL for a target the batch still cannot commit to.

Both smaller notes taken. The comment no longer claims this resolves "the way an entry's own href is"; it now says a prefix goes through the identifier map and anything else is joined against the card, and that an href is parsed absolute or taken from the realm root and never joined.

On the arm being unexercised — a fair hit, opts was {} in every case. There is now a resolveWith helper that binds resolveIdentifier the way the realm does, and two cases on it: an unregistered @scope/... refused, and a registered prefix resolving through the map to an in-realm URL. Dropping the @ guard makes the first answer instead of refusing; dropping the resolveIdentifier call makes the second refuse instead of answering.

Comment on lines +311 to +320
if (row?.type !== 'instance') {
// The row was there when the filter matched it and is not now, or it is
// an error row the filter's own scope did not exclude. Either way the
// card's type cannot be read, and the field cannot be judged without it.
throw refuse(
`entry ${entry.position} follows "${field}" from ${match}, which this ` +
`realm has no readable index row for`,
entry.position,
match,
);

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 answers 400 invalid-params, and nothing that reaches it is the caller's parameters. The filter excludes error rows on this path — fieldset.html is false, so searchEntries passes sqlOpts with no includeErrors and search adds NOT has_error — so both routes here are races against the search: the row was deleted, or it errored in a reindex that landed in between. A client branching on invalid-params is told to fix its request when retrying is the answer.

This subsystem already codes both conditions: target-not-found (404) and target-not-indexed / target-errored, the last of which carries the row's own error detail so the caller learns why the card is unreadable rather than only that it is. Ask: split the branch on row === undefined vs. row.type === 'error' and use them.

Relatedly, the comment's "an error row the filter's own scope did not exclude" reads as a steady state the scope permits; the scope does exclude error rows, and the only way in is the race above.

Regression (error taxonomy). 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 7b1db3f. The branch is split: row === undefined answers 404 target-not-found, and an error row answers target-errored carrying the row's own error.message in the detail so the caller learns why the card is unreadable rather than only that it is.

target-errored's status is 422 rather than the row's own status mirrored through read.ts's mapping. Deliberate, and I'd take a push-back on it: this is not serving the card, it is reporting that a target cannot be followed to, and duplicating that mapping here is drift I'd rather not seed. Say the word if you'd rather it mirrored.

The comment clause is gone too — it claimed a steady state the scope does not permit. It now says both routes are races against the search that matched the card, which is the only way either is reached.

Two cases pin it, and reverting the split makes the first one throw on row.type instead of refusing.

Comment on lines +76 to +80
let outcomes = await Promise.allSettled(
queried.map((entry) =>
resolveOne(entry, { core, scope, paths, opts, mintsCards }),
),
);

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 fans out one search per query entry with no bound, which is the decision hop a few lines below refuses to let the caller make — "a wide many would otherwise decide for itself how much of the realm's connection pool one request holds — the thing the staging width downstream exists to stop a caller choosing". The axis differs; the caller's control over it does not. Entries per batch is as much the caller's number as matches per entry, and each runFilter costs two statements — search runs the data query and the COUNT(*) query under one Promise.all — so K query entries put 2K concurrent statements on the pool the staging-width comment sizes at 40 per process with search alone observed wanting 20+ at peak.

STAGING_WIDTH is exported from coordinator.ts for exactly this shape of bound and is already env-overridable. Either bound this map by it, or say in the comment why the sibling-entry axis is safe where the hop axis is not — as written the two reasonings contradict each other one function apart.

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 🤖] Bounded in 7b1db3f. You're right that the two reasonings contradicted each other one function apart, and the hop's is the one I'd defend — so this map now runs through settledWithin(STAGING_WIDTH, …), a small bounded-concurrency helper that keeps everything Promise.allSettled gave it: results sit at their item's index and a rejection is carried rather than thrown, so the earliest refusal in request order is still the one reported.

Two cases hold it, and neither is a clock race — every worker increments the in-flight count synchronously before its first await, so the peak is deterministic. With STAGING_WIDTH + 4 entries the peak is exactly STAGING_WIDTH; reverting to Promise.allSettled reads 12 against an expected 8. The control sends three entries and asserts a peak of three, so the bound is a ceiling rather than a schedule.

Comment on lines +180 to +183
let targets: string[] = [];
for (let match of urls) {
targets.push(...(await hop(entry, find, match, context)));
}

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 hop does not dedupe, so two matches linking to the same card put that card in the expansion twice and the batch invokes the operation on it twice. The search cannot produce a duplicate (GROUP BY i.url, i.type), so this is the hop's alone: "follow owner from every open report" over three reports with one owner runs the operation three times on that owner.

It also falls outside the net this design leans on. The expansion is serial, and serial composes two changes to one file in order rather than refusing them, so conflicting-targets catches a found target against a named sibling — which the endpoint suite pins — but not two found targets from one query. For an idempotent update that is invisible; for appendLine or an addComment-shaped operation it is three appends.

Which answer is right is yours: dedupe the hop's targets, or state that a card reachable N ways is invoked N times. Asking because the neighbouring question — a collection hop under expect: "one" — is settled explicitly and at length, and this one is settled silently.

Decision. 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 🤖] Decided: dedupe. Done in 7b1db3f — each card once, in the order it was first reached.

The reasoning, now in the comment so it is settled as explicitly as the neighbouring question: the entry names the cards it runs against, not the routes it found them by, so a card reached three ways is one target. And your point about the net is what decides it rather than taste — the expansion is serial, serial composes rather than refusing, so nothing downstream sees anything wrong. An idempotent update would hide it and an append would land three times, which makes "invoked N times" a silent multiplier on exactly the operations where it matters most.

Pinned by three matches over two distinct classrooms; removing the dedupe gives the shared one twice.

… the pool

A hop refused nothing for four shapes it cannot follow, and two of them
answered as though the card simply had no link — which under `expect: "many"`
is an empty result rather than a refusal.

A field resolved by a `query` reaches the hop with `isComputed` false, since a
definition entry records that from `computeVia` alone and carries the query
beside it; its members are written onto the served copy and never onto the
stored one, so the walk finds nothing. A scoped reference whose prefix this
process has not registered is stored verbatim and is not a URL, so joining it
against the matched card produced a path inside this realm that nothing
stores, and the out-of-realm refusal never fired for the one spelling that is
always out of realm. A row that has gone, and one that errored, are races
against the search rather than a payload the caller can fix, so they carry the
codes this subsystem already has for them.

A hop also converges — every open report may name one owner — so a card
reached several ways is one target rather than one per route. Nothing
downstream would catch the duplicate: the expansion is serial, and serial
composes two changes to one file rather than refusing them.

Resolution's own fan-out is held to the width staging is held to, for the
reason the hop already gave: how many entries a batch carries is as much the
caller's number as how many cards one of them matches.

The suite also names its module after `import.meta.filename` rather than
`__filename`, which is not bound in ES module scope: the runner loads a test
file as ESM, so the shard process died at load and took every shard whose
module list held the file.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@habdelra
habdelra requested a review from a team September 18, 2026 02:11
@habdelra

Copy link
Copy Markdown
Contributor Author

@codex review

[Claude Code 🤖] The previous review was against 2726d1c; HEAD is now 7b1db3f, which changes the link hop and the resolution fan-out. Re-reviewing against the new head.

@habdelra
habdelra changed the base branch from cs-12927-card-ops-parallel-and-serial-groups-in-a-batch-a-batch-is-a to main September 18, 2026 02:17
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Delightful!

Reviewed commit: 7b1db3fa8a

ℹ️ 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 and others added 2 commits September 17, 2026 22:19
A marker is how a declared query stands in for a value only known when
someone invokes the operation, and lowering resolves it against the
invocation. An entry's own query target is not a declaration: the caller
wrote it and holds every value in it, so nothing resolves one.

The search grammar accepts one regardless — `{"$ref": "actor"}` is a
well-formed operand — so it was compared against a stored value as a literal
object, matched nothing, and answered with an empty result under `many` or a
refusal blaming the data under `one`. An author coming from declared query
syntax had no way to tell either from a filter that genuinely matched nothing.

Found structurally, wherever a marker sits, because that is how the resolver
it is mistaken for finds one; a check looking in fewer places than the thing
it guards would pass exactly the payloads worth refusing.

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

Copy link
Copy Markdown
Contributor Author

@codex review

[Claude Code 🤖] One further change since the last clean review: af5a33c refuses a marker (actor(), params("x")) inside a boxel:target filter, which the search grammar otherwise accepts as a literal operand.

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

ℹ️ 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 +317 to +318
let marker = markerIn(find.query);
if (marker) {

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 Preserve literal $ref operands in target filters

Remove this marker rejection because boxel:target.query uses the ordinary SearchEntryWireFilter grammar, whose eq, in, and contains operands are literal JSON values. When a card field legitimately stores an object such as { "$ref": "actor" } or { "$ref": "params", "key": "status" }, the same filter is accepted by _search and can match that value, but this recursive check rejects the batch before searching merely because the object resembles a declaration marker. Marker substitution only applies while lowering declared operation queries; caller-supplied target filters should pass these objects through literally.

Useful? React with 👍 / 👎.

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 🤖] Partly taken — narrowed rather than removed, in ff0f075.

You're right that a filter operand is a literal JSON value and that a card field may legitimately store an object, so refusing every object carrying a $ref made ordinary data unfilterable. {"$ref": "#/definitions/Person"} is the case that convinced me: that is somebody's value and it should filter for as data.

What I did not take is removing the check. The reason it exists is a silent failure rather than a theoretical one: markers pass parseSearchEntryQueryFromPayload untouched — I ran actor(), params("status") and a concrete control through it and watched all three come back accepted with the marker intact in itemQuery.filter — so a marker was compared against a stored value as a literal object, matched nothing, and answered [] under expect: "many" or "matched no card" under "one". An author coming from declared @operation query syntax, where lowering does resolve actor(), had no way to tell either outcome from a filter that genuinely matched nothing. The two authoring surfaces look alike and only one resolves markers.

So an object is now read as a marker only when it is one: a $ref naming a marker this runtime resolves (params, actor, card, instance, realmConfig) and no member beyond the ones such a marker carries (key, value). Both lists are read off resolveMarker in query.ts, so what is refused here is exactly what would have been resolved there.

The residual, stated rather than hidden: a field storing precisely {"$ref": "actor"} and nothing else is unfilterable from a query target, because at that point the data is indistinguishable from the marker. An href entry reaches it, and so does _search.

Pinned both directions — {"$ref": "#/definitions/Person"}, {"$ref": "owner"} and {"$ref": "actor", "label": "mine"} reach the index and arrive in the compiled filter as the values they are, while the three marker shapes still refuse. Widening the condition back to any $ref reddens the pass-through case.

habdelra and others added 2 commits September 17, 2026 23:25
A filter operand is a literal JSON value, and a card field may legitimately
store an object — so refusing every object carrying a `$ref` made ordinary
data unfilterable from a query target. A JSON Schema fragment is the obvious
case: `{"$ref": "#/definitions/Person"}` is somebody's value and filters for
it as data.

An object is read as a marker only when it is one: a `$ref` naming a marker
this runtime resolves, and no member beyond the ones such a marker carries.
Both lists are read off the resolver those markers go through, so what is
refused here is exactly what would have been resolved there.

What that leaves unfilterable from a query target is a field storing precisely
`{"$ref": "actor"}` and nothing else, which is indistinguishable from the
marker. An `href` entry and the search endpoint both still reach it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The pass-through cases type their operands as `Record<string, unknown>`,
which a filter's `eq` does not accept: its operands are `JsonValue`.

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

Copy link
Copy Markdown
Contributor Author

@codex review

[Claude Code 🤖] Head is now 4673432; the last review was against af5a33c. Two changes since: ff0f075 narrows the marker check in response to your comment on it — an operand reads as a marker only when its $ref names one this runtime resolves and it carries no member beyond key/value, so a JSON Schema fragment filters for as data — and 4673432 types a test's filter operands as JsonValue.

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

ℹ️ 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 +4481 to +4486
let tree = await resolveQueryTargets(
this.operationCore,
scope,
parsed,
envelopeOptions,
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Resolve query targets inside write serialization

When another request changes a card between this resolution and the later commitBatch call, the batch still operates on the URLs selected from the old index state even if those cards no longer satisfy the filter (and it omits cards that began satisfying it). Because commitBatch acquires its write locks only after this call, moving search into the request removes the network round trip but does not provide the advertised lock-time selection; synchronize or revalidate target membership before staging writes.

Useful? React with 👍 / 👎.

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 🤖] Not taking this one, because the premise is what I'd dispute rather than the mechanism: nothing advertises lock-time selection. The ticket specifies the opposite — "resolution at batch start, before any staging, against the realm's own index — pre-batch state, like everything else in a batch" — and the module header, the call site here and the PR description all say the filter runs against the index as the request found it. So pre-lock selection is the contract, not a gap in delivering a different one.

Three reasons I think it is also the right contract.

It is the same window a named target has, and a shorter one. An href entry's URL was chosen by the caller in an earlier request, so a card that stopped matching, or was deleted, between selection and staging is exactly as possible today. This feature narrows that window — from a client round trip down to the inside of one request — rather than opening it. Holding query targets to a stricter rule than href entries would make them the only targets in the endpoint that are lock-time, which is a bigger inconsistency than the one being closed.

It is what every other part of a batch does. A read entry answers from pre-batch state and never sees what a write entry beside it stages; lid resolution is path math done before the lock is taken. An entry's target being chosen pre-lock is the same rule, applied to which card rather than to what it contains.

The remedy is not implementable as stated. commitBatch locks the files the batch names (withWriteLocks(lockPaths(entries, …))), so the lock set is a function of the resolved targets. Selecting inside the lock is circular: you cannot lock a filter's result set before you know what it is. Re-running the query after acquiring locks would mean discovering new files to lock and escalating — a lock-ordering problem, in a path where the realm's other writers are already queued behind us — and re-validating membership would still be a snapshot, just a later one, so it narrows the window without closing it.

What is true and worth keeping from this: the window exists, and a caller should know its targets were chosen when the request arrived rather than when the lock was taken. That is stated in the module header, at the call site, and in the PR description under "The filter runs against the realm's index as the request found it".

One thing your sibling comment did find is a real snapshot bug, and I have taken it: the count and the rows of a single search could disagree, and the rule trusted the count. Fixed in b51ca31. That one changed behaviour silently; this one is the documented contract.

Happy to be argued out of it if there is a hazard here that an href entry does not already have — that is the test I applied.

Comment on lines +204 to +206
let { urls, total } = await runFilter(entry, find, context);
if (find.expect === 'one') {
heldToOneMatch(entry, total, context);

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 Decide cardinality from the returned target rows

When an index update lands while an expect: "one" search is running, searchEntries can return rows and meta.page.total from different snapshots because the index engine executes the data and count SQL statements concurrently. Using only total here can therefore accept two returned URLs and silently run against urls[0], or accept a count of one with no URL. Since the data query is already capped at two rows, base the zero/one/many decision on urls.length so selection and cardinality come from the same snapshot.

Useful? React with 👍 / 👎.

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 🤖] Taken in full — fixed in b51ca31, and this was a real silent defect rather than a hardening nit.

Confirmed the mechanism before changing anything: search() ends in let [results, totalResults] = await Promise.all([this.#queryCards(query), this.#queryCards(queryCount)]) — two statements, two connections, two snapshots. So the count and the rows can disagree.

The direction that mattered is the one you named first. A count of one beside two returned rows passed heldToOneMatch, and resolveOne then returned both urls, so nodeFor took urls[0] and the entry ran against whichever row came back first — a silently chosen target, which is the worst outcome available here. The other direction left the entry with no href at all.

The rule now reads urls.length, which is what the entry can act on and comes from the same snapshot as the selection; the page holds two, which is all it needs to tell none from one from several. The count still supplies the number a refusal reports, because two rows cannot say how many there are once the page is full — but as Math.max(urls.length, reported), so the number is never smaller than what was actually seen.

Two cases pin it, with the stub reporting a count that disagrees with its rows on purpose: two rows beside a count of one now refuses instead of answering, and no rows beside a count of one refuses as "matched no card". Reverting to the count makes the first case answer, which is the silent selection going through.

…de them

The engine runs a search's data statement and its `COUNT(*)` concurrently, as
two statements on two connections, so an index update landing between them
leaves the two describing different snapshots. The count rule read the count,
which is wrong in both directions — and the dangerous one is silent: a count
of one beside two returned rows passed the rule, and the entry ran against
whichever row came back first.

The rows are what an entry can act on, and the page holds two, which is all
the rule needs to tell none from one from several. The count still supplies
the number a refusal reports, since two rows cannot say how many there are,
but never as less than the rows themselves.

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

Copy link
Copy Markdown
Contributor Author

@codex review

[Claude Code 🤖] Head is now b51ca31, which takes your snapshot finding: the count rule reads the returned rows rather than the concurrently-executed COUNT(*), so selection and cardinality come from one snapshot. I've replied on the lock-time thread rather than changing it — pre-lock selection is the specified contract there, and the same window applies to an href entry. This is my last planned push.

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

ℹ️ 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".

entry.position,
);
}
let doc = await core.indexQueryEngine.searchEntries(query);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Apply resource bounds before resolving target searches

When an authorized caller sends a broad target filter—especially { "query": {}, "expect": "many" }—this direct engine call bypasses the page ceiling and wall-clock budget applied by the normal _search route. Because the many path deliberately supplies no page, it materializes every card in the realm and then expands them into a single batch, allowing one request to consume unbounded memory and staging/transaction time. Apply an equivalent timeout and reject expansions above a server-side target limit rather than silently truncating them.

Useful? React with 👍 / 👎.

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 🤖] Taken in full — fixed in ccae2b0. This one overturned a position I had stated three times, and you found the reason it was wrong.

I had left the expansion unbounded deliberately, on the grounds that I had no observed magnitude to size a ceiling against and would rather state the gap than invent a number. That reasoning does not survive your comment, because the realm already has the number. search-bounds.ts documents SERVER_ABSOLUTE_MAX_PAGE_SIZE as "the size no item-leg request may exceed, however deliberately it asks… the bound that keeps the server from assembling and serializing an unbounded page", and realm.ts applies it and runWithSearchTimeBudget on the search route. So a ceiling here was never an invention — it was a bound that exists, is already an operator's to tune, and that I was bypassing by reaching the engine directly.

What changed:

  • The expansion is paged at that bound, and the search runs under the same wall-clock budget, with the signal threaded into the engine call so an abandoned search stops rather than running on under a request that has already been answered. A SearchBoundError keeps its own status, so the budget surfaces as a 408 labelled with the entry rather than as a 500.
  • Going over is refused, not truncated — which is your "reject rather than silently truncating", and I want to be explicit that it is the right call and not just the cheaper one. A caller that asked to run against every card answering a filter and is silently handed the first 2000 of 3000 is told its batch succeeded, so the entries it believes it wrote are precisely the ones it will never go looking for.
  • One row beyond the ceiling is requested, so going over is visible in the rows themselves rather than in the COUNT(*), which your sibling comment correctly established can describe a different snapshot.

Four negative controls, each reddening only its own case: unbounding it, truncating instead of refusing, dropping the budget wrapper, and dropping the signal. The truncating variant is the one worth having — it still refuses nothing while passing a naive "is there a ceiling" check.

One of my own tests also went red, correctly: it asserted the many path asks for no page at all, which is exactly the contract this reverses. Removed as obsolete rather than weakened.

Comment on lines +582 to +584
for (let index = 0; ; index++) {
let relationship = relationships[`${field}.${index}`];
if (relationship === undefined) {

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 Normalize array-valued linksToMany relationships

When a matched card stores a supported array-valued relationship such as relationships.attendees = [{ links: { self: a } }, { links: { self: b } }], this loop immediately finds no attendees.0 key and returns no targets. CardResource.relationships permits Relationship[], and the serializer can preserve that form, so expect: "many" silently skips the linked cards while expect: "one" incorrectly reports an empty link. Normalize the relationship map with the existing relationship utility, or explicitly handle the array form, before enumerating members.

Useful? React with 👍 / 👎.

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 🤖] Taken — fixed in ccae2b0. linksOf now reads the field's own key as well as the field.N keys, and one relationship entry yields none, one or many selfs, so the array form is followed.

Worth recording that this contradicted an earlier review pass on this PR, which traced the serialization chain (render → serializeCardcard-indexerpristine_doc) and concluded the Relationship[] form has no producer in the repo and is normalised away before indexing. I did not re-derive that chain, and I am not claiming it is wrong.

I took your finding anyway, because "no producer today" is a weaker guarantee than the one the code needs. CardResource.relationships is typed Relationship | Relationship[], so the array form is in the contract this function reads against; a producer appearing later would not redden anything, it would silently skip links — an empty result under expect: "many" and a wrong "links to no card" under "one". That is the exact failure shape this file exists to eliminate, and the fix is three lines. Paying three lines to close a silent path is a better trade than relying on an absence I would have to re-verify on every future change to the serializer.

One thing the fix had to be careful about, which is why it is not a bare Array.isArray branch: the field's own key is also where an emptied collection's null link lives. Reading that key naively would turn "the author cleared this collection" into a target. Both are pinned — the array form yields both linked cards, and a null link on the field's own key still reads as no link. Reverting to the keyed-only walk reddens the first.

Resolution reached the index engine directly, so the page ceiling and the
wall-clock budget the search route applies to its item leg did not apply to
it. A broad filter under `expect: "many"` deliberately asked for no page at
all, so one authorized request could materialize every card in the realm and
expand them into a single batch.

Both bounds now apply, borrowed rather than invented: the expansion is paged
at the size no item-leg search may exceed however deliberately it asks —
which is the realm's own statement of the most rows one request may
materialize, and already an operator's to tune — and the search runs under
the same budget, with the signal threaded in so an abandoned one stops.

Going over the ceiling is refused, not truncated. A caller that asked to run
against every card answering a filter and is silently handed the first few
hundred is told its batch succeeded, so the entries it believes it wrote are
the ones it will never check. One row beyond the ceiling is requested so that
going over is visible in the rows themselves rather than in a count that can
describe a different snapshot.

A collection stored as one array-valued relationship is also followed now.
`CardResource.relationships` is typed `Relationship | Relationship[]`, so a
collection may arrive as one entry holding a list rather than as the
per-member keys the canonical serialization writes; reading only the keyed
form found nothing and answered as though the card had no link.

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

Copy link
Copy Markdown
Contributor Author

@codex review

[Claude Code 🤖] Head is now ccae2b0, which takes both findings from the last pass: the target search is paged at SERVER_ABSOLUTE_MAX_PAGE_SIZE and runs under runWithSearchTimeBudget with the signal threaded in (going over the ceiling refuses rather than truncates), and linksOf follows an array-valued linksToMany relationship as well as the field.N keys.

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

ℹ️ 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 +91 to +93
let outcomes = await settledWithin(STAGING_WIDTH, queried, (entry) =>
resolveOne(entry, { core, scope, paths, opts, mintsCards }),
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Put target searches through global admission control

When several operations requests containing query targets arrive concurrently, each request may start up to STAGING_WIDTH searches here, but these in-process searches never pass through searchAdmission: that middleware only recognizes /_search and /_federated-search in packages/realm-server/middleware/index.ts. Consequently this path bypasses SERVER_MAX_IN_FLIGHT_SEARCHES, its 429 shedding, and the process-wide telemetry even after the new page and time bounds, allowing a burst of authorized batch requests to saturate the database pool. Acquire an admission slot for these searches or route them through an equivalently process-wide gate.

Useful? React with 👍 / 👎.

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 🤖] Not taking this one in this PR, and I want to be precise about which part I'm disputing: the mechanism is correct, the scope is what I'd push back on.

Verified rather than assumed. searchAdmission is Koa middleware in packages/realm-server/middleware/index.ts, and grep -rn "searchAdmission\|admissionSlot\|acquireSearch" packages/runtime-common/ returns nothing — so SERVER_MAX_IN_FLIGHT_SEARCHES and its 429 shedding are enforced at the HTTP boundary for /_search and /_federated-search and nowhere else. Which means every in-process search already bypasses it: the query-backed field expansion inside loadLinks, and the indexer's own searches. This path is not uniquely delinquent; it joins an existing set.

Why that makes it a follow-up rather than a change here. Acquiring a slot for this one caller would make it the only in-process search in the system that sheds, which is a worse inconsistency than the one it closes — a realm under pressure would start refusing batch targets while loadLinks and the indexer carried on consuming the pool unmetered, and the 429 would point at the wrong subsystem. The gate also lives in realm-server while resolution lives in isomorphic runtime-common, so reaching it means a new bound collaborator on OperationCore; that is the right shape, but it is the shape for all in-process searches at once, not for whichever one a reviewer looked at first.

Where I think you're right and it matters: this path is reachable by an authorized external caller with a tiny payload, which loadLinks and the indexer are not — they are realm-initiated. So the exposure differs in kind even though the bypass is shared, and that is a fair argument for it being the one that finally forces the issue. What this PR does about it is reduce the per-request ceiling to something bounded on every axis I control: searches per request at STAGING_WIDTH, rows per search and targets per entry at SERVER_ABSOLUTE_MAX_PAGE_SIZE, wall-clock per search at the search budget, and entry resolutions at STAGING_WIDTH. What remains unbounded is concurrency across requests, which is what admission is for and what no per-request bound can supply.

Happy to be overruled if you think the cross-request gap is severe enough to block; I'd rather file it as work that covers all three in-process callers than half-solve it here.

Comment on lines +135 to +139
members: targets.map((href, index) => ({
...rest,
position: foundPosition(entry.position, index),
href,
found: true,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Bound resolution of expanded target entries

When an expect: "many" query returns the allowed 2,000 targets, this expansion creates 2,000 invocation entries; #runOperationsBatch subsequently sends every one through Promise.allSettled(entries.map(...)) at realm.ts:4493, and each distinct card normally causes an uncached scope.peekInstance() database query while resolving its operation. Thus a single small envelope can enqueue thousands of point reads simultaneously before the staging-width limiter is reached, monopolizing the pool despite the bounded search. Resolve the generated entries with a bounded worker pool as well.

Useful? React with 👍 / 👎.

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 🤖] Taken — fixed in 6886c03. The resolution pass at realm.ts now runs through the same bounded helper the target searches do, at STAGING_WIDTH.

Your reasoning is the one I had already accepted for the search fan-out and should have followed through to this pass at the same time: each resolution reads its target's index row, scope.peekInstance memoizes per URL so distinct cards do not share, and the number of entries stopped being purely the caller's payload size the moment one entry could expand into a ceiling's worth. So an unbounded pass put that many point reads on the pool at once and reached the staging limiter only after the pool was already spent.

One note on blast radius, stated rather than hidden: this bounds the pre-existing flat path too, so a batch of fifty href entries now resolves eight at a time instead of all at once. That is a latency cost on a shape that was previously unbounded, and I think it is the right trade — the reads are memoized per URL, eight is what staging already spends, and "how many reads one request may have in flight" is not a number a caller should choose. Say so if you would rather the bound applied only to expanded entries; I would argue against it, since it would make the bound depend on how the targets were named rather than on how many there are.

The helper moved from find-targets.ts to coordinator.ts beside STAGING_WIDTH, since the bound and the thing that spends it now have two callers and travel together.

Comment on lines +235 to +238
for (let target of await hop(entry, find, match, context)) {
if (!reached.has(target)) {
reached.add(target);
targets.push(target);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Enforce the target ceiling after following links

When expect: "many" also specifies a field, the ceiling is checked only against the cards returned by runFilter; each call to hop can contribute an arbitrary number of linksToMany members, and this loop appends all deduplicated links without rechecking the limit. The fresh evidence after the prior bound fix is the newly handled array/per-member relationship path: one matched card can already expand beyond the ceiling, or 2,000 matches can multiply into a much larger batch, restoring the unbounded memory, lock-set, and staging cost that the search cap was intended to prevent. Refuse once the post-hop target set exceeds the same endpoint ceiling.

Useful? React with 👍 / 👎.

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 🤖] Taken — fixed in 6886c03, and this is the best finding on the PR, because the bound I added two commits ago did not bound the thing it named.

You're right on the mechanism and on the aggravating detail. The check was urls.length > EXPANSION_CEILING, which is the cards the filter matched; with a field the entry runs against the hop's output instead, and a linksToMany multiplies — one matched card can carry more members than the whole bound, so a single row passes a check on the matches and expands past it. And the array/per-member path I had added in the same commit widened that, which is the "fresh evidence" framing being exactly right.

The ceiling now holds the post-hop target set, checked as the set grows rather than once it is built, so an expansion that cannot be carried out is refused before the memory for it is spent. Dedupe still runs first, so a hop converging on one card is not refused for reaching it repeatedly.

Three cases, and the first is the one that matters: a single match whose collection holds ceiling + 1 members is refused — search returned one row, so a bound on the matches passes it. Plus a control at exactly the ceiling, so it is a ceiling rather than an off-by-one, and fifty matches over one shared classroom resolving to one target, so dedupe-before-bound is pinned. Disabling the post-hop check makes the first case answer.

Worth naming the process failure, since it is the second time this exact shape has been recorded in this subsystem: I added a bound, wrote a test for it, watched the test go red under a mutation, and still shipped a bound that did not hold — because the test only covered the flat path, not the fan-out. A limit on something that multiplies has to be tested through the multiplication.

The expansion ceiling was checked against what the filter matched, which
after a link hop is not what the entry runs against. A `linksToMany`
multiplies: one matched card can carry more members than the whole bound, and
a full page of matches can fan out to many times it — so the bound was not a
bound on the batch at all. It now holds the post-hop target set, checked as
that set grows rather than once it is built, so an expansion that cannot be
carried out is refused before the memory for it is spent. Dedupe still runs
first, so a hop converging on one card is not refused for reaching it often.

Resolving the entries is bounded too, at the same width. Each resolution
reads its target's index row, and how many entries a batch holds became a
number a small envelope can choose once an entry could expand — so an
unbounded pass would put a ceiling's worth of point reads on the pool at once
and reach the staging limiter only after the pool was spent. The helper moves
beside the width it spends, since the two travel together.

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

Copy link
Copy Markdown
Contributor Author

[Claude Code 🤖] Realm Server Test Results is red on a green suite — safe to merge over.

All six realm-server shards pass, as do all 16 host shards, Lint and Software Factory. That aggregate check publishes reports merged across attempts, so it still counts a superseded attempt: server-endpoints/delete-realm-test.ts > DELETE /_delete-realm still deletes a realm when a published copy is no longer mounted failed once on Timeout waiting for condition: published realm from-scratch index did not settle before delete, and its retry passed on this same commit with no code change. Hence 3 687 tests / 3 686 ✅ / 1 ❌ beside six green shards.

That test touches nothing this PR changes — the diff is card-operations/* plus two imports and one pre-step inside realm.ts's #runOperationsBatch — and a pass and a failure on one commit is what timing-sensitivity looks like. It is the same genre as the index-settle timeouts seen elsewhere in this suite: a queued index job not settling inside a waitUntil window.

So: read the shards rather than the aggregate. Nothing here needs a re-run, and the check will stay red unless the whole realm-server workflow is re-run from scratch.

@lukemelia lukemelia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is pretty cool!

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