Add readSource, the stored-bytes read, as a base operation - #6071
Add readSource, the stored-bytes read, as a base operation#6071habdelra wants to merge 15 commits into
Conversation
A realm serves three reads and the operation core modeled one: the
card+json document, assembled from the search index. The other two —
the `card+source` GET/HEAD that returns a resource's stored text and
the raw byte serve that returns a file's bytes — had no operation to
dispatch through, so a policy layered on operations would have gated
neither.
`readSource` is that operation. It resolves without a definition,
which is the point rather than an optimization: a module has no
`adoptsFrom` and no definition-cache entry, and a `.gts` path takes
the file-def branch of `definitionFor`, so gating its source on a
cache lookup would refuse bytes that are plainly on disk. Dispatch
answers the name before it would reach one, taking the target's kind
from the URL — which is where an instance target's kind comes from
anyway. A type target has no stored bytes, so it refuses as
`operation-not-allowed` without a lookup either, and a FieldDef type
refuses for that reason rather than because a field def carries
nothing.
Nothing may declare one. The authoring decorator refuses `base:
'readSource'` and dispatch refuses a stored definition that carries
it, which is what makes skipping the definition safe: no declaration
can take the name. The two rules are the same rule read from opposite
ends, so they are worth keeping together.
The executor answers `{ contentType, lastModified, created, version,
body }` and, in headers-only mode, everything but `body` — leaving the
adapter's `content` untouched, since it is a lazy getter that opens a
real stream on first touch. `version` is the content hash, resolved by
the realm from its own file-meta row so both modes report the same one
by construction. The redirects, the ETag, the 304 and the source cache
stay at the facade, which takes the resolved path.
No route dispatches here; every existing handler and suite is
untouched. `OperationCore.readSource` is renamed `readFileAsText`,
after the realm method it is bound to, so the one name does not mean
both the text read and the operation.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ShoXa2iVcYebU8yPsxTEvt
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: a7a6f3b6cb
ℹ️ 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".
Preview deploymentsHost Test Results 1 files ±0 1 suites ±0 2h 30m 38s ⏱️ - 2m 3s Results for commit d70f9f6. ± Comparison against earlier commit 9332491. Realm Server Test Results 1 files ±0 214 suites ±0 1h 17m 26s ⏱️ + 4m 58s Results for commit d70f9f6. ± Comparison against earlier commit 9332491. |
…e names Two corrections to the stored-bytes read, both about parity with the byte routes the facade will hand off to. `version` has one job — identifying the bytes it is returned with — and the persisted file-meta row only does that job while it describes the file on disk. `persistFileMeta` is reached from the realm's own write path and nowhere else, so a file overwritten out of band keeps a row describing bytes that are gone; handing that hash back would let a conditional GET answer 304 for content that changed, where `getSourceOrRedirect` hashes the bytes it materialized and does not. The row is now trusted only where the length it recorded matches the handle being read, and the bytes are hashed otherwise. The size travels from the executor with the request for the version, so the check is against the handle those bytes come from rather than a later stat. An out-of-band overwrite preserving the exact byte length is the residual case; closing it needs an unconditional hash per read or an mtime on the row, which is the facade's call. The `_`-prefix refusal described the wrong routes. It is `openFileForMetadata`'s, and the byte routes have no equivalent: the `card+source` GET/HEAD and the raw byte serve are registered on `/.*` and refuse no name, and `upsertCardSource` writes whatever path it is given, so a caller can store `_notes.md` and read it back over HTTP. Only the specific registered `_` endpoints are routed away from the file handlers. Refusing the prefix made this the one read that could not reach such a file, so what is left is the path that names no file at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ShoXa2iVcYebU8yPsxTEvt
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved findings cover declaration-name collisions, root-target handling, snapshot consistency, ETag/range parity, and duplicate metadata queries.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds readSource as a definition-free base operation for reading stored resource bytes and metadata.
Changes:
- Adds executor, types, exports, dispatch, and realm bindings.
- Registers the operation as read-only and non-declarable.
- Adds shared, realm-server, and host test coverage.
File summaries
| File | Summary |
|---|---|
packages/runtime-common/tests/card-operations-dispatch-test.ts |
Tests dispatch, byte reads, metadata, and refusal cases. |
packages/runtime-common/realm.ts |
Binds stored-file and metadata access. |
packages/runtime-common/card-operations/types.ts |
Defines stored-file results and adapter types. |
packages/runtime-common/card-operations/read.ts |
Updates the existing text-read boundary. |
packages/runtime-common/card-operations/read-source.ts |
Executes stored-byte reads. |
packages/runtime-common/card-operations/index.ts |
Exports the new operation APIs. |
packages/runtime-common/card-operations/dispatch.ts |
Adds definition-free dispatch and collaborators. |
packages/realm-server/tests/card-operations-dispatch-test.ts |
Registers shared dispatch tests. |
packages/realm-server/tests/card-operations-core-test.ts |
Tests realm-backed source reads. |
packages/host/tests/integration/operations-test.ts |
Tests operation declarations and restrictions. |
packages/base/operations.ts |
Registers and validates the base operation. |
Review details
Suppressed comments (4)
packages/runtime-common/card-operations/dispatch.ts:306
- [Claude Code 🤖] This early return assumes that no declaration can use the
readSourcename, but the decorator only rejects declarations whose base isreadSource. A valid@operation static readSource = { base: 'read' }(or another allowed base) is therefore skipped here and the built-in byte reader silently runs instead of the declared operation. Reserve thereadSourceoperation name during authoring and keep the persisted-definition path consistent with that invariant.
export async function resolveOperation(
packages/runtime-common/card-operations/read-source.ts:100
- [Claude Code 🤖] The metadata lookup happens after
openStoredFile, whilebodyremains a lazy handle. On the Node adapter,openFilesnapshotslastModifiedimmediately but opens the path only whencontentis touched, so a concurrent write can pair an old mtime with a newversionand body (or the reverse). The facade's ETag can then validate a different representation than it serves; read metadata and bytes from one consistent snapshot or revalidate the pair before returning.
// The size travels with the request for the version so the realm can check
// its recorded hash against the very handle these bytes come from, rather
// than against whatever a later stat would see.
let meta = await core.storedFileMeta(localPath, file.size);
let result: OperationSourceResult = {
contentType,
lastModified: file.lastModified,
packages/runtime-common/card-operations/types.ts:290
- [Claude Code 🤖] This says
versionis what the source-route ETag is built from, but the existing byte handler's non-JSON/non-executablebypassCachepath callsserveLocalFilewithout anetagBase, so images and PDFs uselastModifiedas the ETag base. A facade using this result for conditional requests would not reproduce those validators; either expose the exact ETag inputs or narrow this parity claim to responses that actually use the content hash.
// The content hash of the stored bytes — the same identity the rest of the
// project calls `version`, and what the source route's `ETag` is built
// from. Null only where the realm can neither recall nor compute one.
version: string | null;
packages/runtime-common/card-operations/types.ts:296
- [Claude Code 🤖]
OperationStoredFilepreservessize, but this result drops it along with the adapter's range capability whenreadSourceOperationreturns. For the Node adapterbodyis a lazyReadStream, so a facade consuming only this result cannot reproduce the existingContent-Length/Accept-Ranges/Rangebehavior without reopening or materializing the file. Carry the size/range metadata through the operation result, or keep this operation from claiming parity with those handlers.
// The bytes. Absent in the headers-only mode, which is the whole difference
// between the two: a `HEAD` reports the metadata above and would discard
// this. Whatever form the realm's file adapter produced — a string, a byte
// array, or an unread stream — so a caller hands it to a response body
// rather than materializing it.
body?: OperationSourceBody;
- Files reviewed: 11/11 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…eta query
Three corrections, the first a silent wrong answer.
The name `readSource` is now reserved at the decorator, not just its
base. A name and a base are independent, so `@operation static
readSource = { base: 'read' }` passed validation, replaced the
synthesized entry `getOperations` returns, and was then dispatched
straight past: the realm answers that name without reading a definition,
so the built-in ran and the author's operation was never reached.
Refusing the name is what makes answering it definition-free correct;
refusing the base is what stops the behavior being reached under
another name. Both halves now exist.
A stored-bytes read addresses a path, so the realm root stays the realm
root. `canonicalizeTarget` resolves the root to the realm's index card,
which is what makes it readable as a card, and applied to a byte read it
served whatever file carried the bare name `index` in answer to a
request for a directory. Canonicalization now takes the addressing, and
`runOperation` reads it off the name — sound for the same reason
answering definition-free is, now that no declaration can take the name.
A trailing slash, a query string and a fragment still normalize for
both.
The two file-meta values come from one query rather than one lookup
each, which a byte response routed through here would pay per request.
`OperationSourceResult` also carries `size`, which a facade needs for
`Content-Length` and to decide whether it can offer a `Range` at all,
and two comments that overclaimed are narrowed: the byte routes build a
validator from a content hash only for a `.json` or an executable
extension and from `lastModified` otherwise, and the metadata describes
the handle as it opened while the bytes are read from it afterwards.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ShoXa2iVcYebU8yPsxTEvt
|
[Claude Code 🤖] The four suppressed comments in the Copilot review have no threads of their own, so answering them here. Three are addressed in
Generated by Claude Code |
`getOperations` reports the base operations a def type carries alongside the declared ones, so the expectation for a subclass's merged set names every base operation. It was missing the stored-bytes read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ShoXa2iVcYebU8yPsxTEvt
The body is whatever the file adapter produced, which under Node is a single-use stream: reading it twice yields the bytes and then nothing. Hold the first read and compare it against both the literal and what the source route serves, which is also the one pass a facade putting the body on a response gets. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ShoXa2iVcYebU8yPsxTEvt
A review of the stored-bytes read turned up that its central claims did not describe its behavior, and that resolving `version` cost far more than the comments said. `version` is now populated exactly where the byte routes build a validator from a content hash — a `.json` or an executable extension — and reports the absence everywhere else, which is what those routes do: `getSourceOrRedirect` takes its `bypassCache` path for anything else and bases the `ETag` on `lastModified`, computing no hash at all. The previous fallback hashed unconditionally, and `ensureFileCreatedAt` inserts a row carrying only `created_at`, so that reached a full buffering read of every file the realm had not itself written with a hash — an image or a video included, and in the headers-only mode, whose whole promise is to leave the body alone. That mode now reports no version rather than buying one with a read it declined; the two modes cannot contradict each other, since one answers with a hash where the other answers with nothing. Where the bytes are read they are read from the caller's own handle and handed back with the hash, so one open serves both and `version` describes the body it is returned with. Previously the fallback opened a second handle, so an overwrite between the two opens produced a version for bytes never served — the failure the size check exists to prevent. The definition-free justification was false and is restated. The ordinary path resolves these names for an instance target whether or not a definition resolves, because `defKindFor` takes an instance target's kind from its URL and never from the entry; and a `.gts` does have an entry, the file def its extension names. What skipping the lookup buys is the lookup, on the hottest path the realm has, plus fixing the addressing before anything reads. What makes skipping safe is the name reservation, not the other way round. Lowering now refuses a reserved name too, so no stored definition can carry one. The decorator only governs what it lowers, and a definition-cache row carries no code version and is not re-derived until something invalidates it — so an entry written before the reservation existed would have been dispatched past rather than run. The reserved set moves to `card-operations/types.ts`, which dispatch and lowering can both reach; the authoring decorator enforces the same list from inside a card module. `getOperations` returns `CarriedOperation`, since `OperationDeclaration` is a closed union that cannot express a synthesized entry for a name nothing may declare — a consumer testing for one got "no overlap". `OperationSourceResult.size` reports null rather than being absent, to match the two values beside it. And two comments are corrected: the version claim, which ignored that `computeContentHash` samples above its whole-content limit, and the `_`-prefix rationale, which described the raw byte serve as a registered route and missed that a path under a prefix claimed before the router is reachable through this read and through no byte route. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ShoXa2iVcYebU8yPsxTEvt
…file `version` was bought with a full read of the file whenever the realm had no recorded hash to hand back — more than the byte route it claims parity with pays, since `contentHashFromMaterializedRef` hashes only content the route already holds and falls back to `lastModified` otherwise. The headers-only mode declined that read and so reported no version at all, which left the two modes disagreeing about a validator for the same file. `computeContentHash` already samples above `CONTENT_HASH_WHOLE_LIMIT_BYTES`: the value is the byte length plus a hash of the head and one of the tail. A length comes from a stat, so that value can be assembled from two bounded reads and is byte-identical to hashing the whole content — `computeContentHashFromRanges` does exactly that, and the content-hash suite holds the two forms to the same string at every size boundary and pins which ranges are asked for. The realm reads its fallback fingerprint that way, through the handle's `createRangeStream`, so hashing costs at most the whole-hash limit however large the file, and never touches `content` — which is the single-use body a full read returns and a headers-only read leaves alone. Both modes therefore report the same version at the same cost, `mayReadBytes` and the bytes-back channel are gone, and an adapter offering no bounded read simply has no version to report rather than an unbounded read taken on its behalf. A short range read means the file is no longer the one the stat described, so the fingerprint is abandoned rather than reported. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ShoXa2iVcYebU8yPsxTEvt
…tating it The branch comment claimed a module path's definition lookup cannot succeed. A `.gts` resolves to the file def its extension names, which has a cache entry, so the lookup does succeed — and the constant's own comment says so. Point at that one justification instead of carrying a second, wrong one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ShoXa2iVcYebU8yPsxTEvt
…dsource-base-operation-the-stored-bytes-read-for
The version was withheld for paths outside `.json` and the executable extensions, on the grounds that the source route builds those paths' ETag from `lastModified` and computes no hash — so reading one was a cost the serving route would not pay. Reading a fingerprint is now bounded by the hash's own shape rather than by the file's size, so that cost argument no longer holds, and what the gate withholds is a content identity for exactly the large media a caching facade would most want a strong validator for. Which validator a route builds stays that route's own choice; the result carries both members for either. An absent version now means only that the realm could neither recall one nor read one within a bounded cost — an adapter with no bounded read, or none that knows a size without reading the bytes. The out-of-band overwrite test now covers a module and a `.md` as the two sides of the line the byte routes draw, asserting both report the new bytes' fingerprint, since what a stale row means does not depend on the extension it sits under. The large-file case moves to an extension the source route would validate on `lastModified`, which is the case the gate skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ShoXa2iVcYebU8yPsxTEvt
…dsource-base-operation-the-stored-bytes-read-for
…dsource-base-operation-the-stored-bytes-read-for
backspace
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Convergence pass over the bot findings and the invariants they moved: verified each fix as a change on the head commit rather than against its description, then re-ran the definition-free, addressing and version claims against the code around them. The realm-server and host suites were left to CI, which is green.
No blocking issues — approving. The three items below are comment accuracy and test coverage; none of them changes what the code does, and none needs to hold the merge.
Where each earlier finding landed
- Stale
versionafter an out-of-band overwrite — resolved. The row is trusted only where itscontent_sizematches the handle's stat, andcontentHashFromRangesreads the fingerprint otherwise; pinned for a module and for a.mdina stored-bytes read never reports a version that describes other bytes. - Underscore-prefixed stored files — resolved.
#operationStoredFilekeeps only the empty-path check, and_notes.mdreads in both the shared and realm-server suites. - A declaration taking the name while building on another base — resolved, and this was the one that mattered.
isNotDeclarable(key)runs beforeassertNameAvailable, lowering refuses the key with areserved-nameissue, and the host suite coversreadSourcedeclared onbase: 'read'as well as on its own base. - The realm root serving a bare
indexfile — resolved. The addressing travels onCanonicalizeOptions.rootNamesIndexCardrather than being preserved only inside the executor, and the shared case pins both root spellings refusing whileindexstill reads by name and a card read of the root still serves the index card. - Two queries per stored-bytes read — resolved. One
getFileMetaForPathsread supplies both values. - Metadata and bytes are not one snapshot — accepted as parity rather than closed. Confirmed the byte routes have the same window:
serveLocalFileservesref.lastModifiedfrom the stat taken at open and readsref.contentafterwards. - ETag/range parity —
sizetravels; the bounded-read capability stays on the handle. Theversionhalf of that comment is now out of step with the code — item 1 below.
Recommendations
- The
versiondoc comment onOperationSourceResultstates the opposite of what the code and the suite do — see the thread ontypes.ts. - The executor header's cost claim holds only above the whole-hash limit, and the ranged read is the steady state rather than the exception — see the thread on
read-source.ts. - The reserved-name list has two homes and nothing pins them equal; the lowering branch that enforces it has no test — see the thread on
base/operations.ts.
Adjacent, not asked of this change
assertValidDeclaration builds its "this def type carries only …" message from impliedOperations, and BASE_OPERATIONS fills the "base must name … one of" message, so both now offer an author a base that isNotDeclarable refuses three lines earlier. Filtering NOT_DECLARABLE out of those two messages would keep the guidance pointing somewhere an author can actually go.
| // Two things a facade building a validator from it has to know. It is not by | ||
| // itself the byte routes' `ETag`: the source route builds one from a hash for | ||
| // a `.json` or an executable extension and from `lastModified` for | ||
| // everything else, and `version` is populated on those same terms, so it is | ||
| // null exactly where that route computes no hash. And `computeContentHash` | ||
| // samples above its whole-content limit, so a large file's hash covers its | ||
| // head, tail and length rather than all of it — `isSampledContentHash` tells | ||
| // one from the other, and the realm's own `ETag` joins a sampled hash with | ||
| // `lastModified` rather than trusting it alone. |
There was a problem hiding this comment.
[Claude Code 🤖] This says version is null wherever the source route computes no hash. The code reports one for every path: #operationStoredFileMeta falls through to contentHashFromRanges whenever the row's content_size does not match the handle's stat, extension immaterial — and a stored-bytes read never reports a version that describes other bytes asserts exactly that for notes.md, the case chosen because the source route validates it on lastModified. read-source.ts's own header states the contract the code keeps ("a content identity for every path, whether or not the route serving it asks for one"), so the two disagree.
Worth correcting rather than leaving: this is the type a facade author reads, and as written it tells them to skip version for exactly the large media the bounded fingerprint covers.
Regression (comment only), non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Confirmed and fixed. This is a regression and the diagnosis is exact: the comment is the pre-gate-removal policy left verbatim.
version used to be populated only where the source route builds a validator from a hash, on the reasoning that reading one for an image was a cost the route serving it would not pay. Removing the gate made that reasoning obsolete — the fingerprint is read in bounded ranges, so withholding it saved nothing and denied a strong validator to exactly the large media that most wants one. I updated read-source.ts's header and OperationStoredFileMeta in dispatch.ts and missed this one, which is the worst of the three to miss for the reason you name: it is the type a facade author reads, and read-source.ts's header is not.
Rewritten to say what the code does — which of the two validators to reproduce is the facade's choice, a content identity is reported for every path whether or not the route serving it asks for one, and null means only that the realm could neither recall a fingerprint nor read one within a bounded cost. The sampled-hash half of the comment was accurate and stands.
Verified against the assertion you cite: a stored-bytes read never reports a version that describes other bytes expects computeContentHash('# second, and longer') for notes.md, so the suite does pin the behavior the comment now describes.
Generated by Claude Code
| // index has no row for, and for one it never will. Where the realm has no | ||
| // recorded hash it reads the file to fingerprint it, in ranges bounded by | ||
| // the fingerprint's own shape rather than by the file's size — so this | ||
| // answers for a path of any size at a cost that does not grow with it. |
There was a problem hiding this comment.
[Claude Code 🤖] The bound is min(size, CONTENT_HASH_WHOLE_LIMIT_BYTES), not a constant — below the limit computeContentHashFromRanges asks for readRange(0, size), so the fingerprint read is the whole file. "a cost that does not grow with it" holds only above the limit.
What makes that worth stating is how often the read is reached. ensureFileCreatedAt (index-runner/visit-file.ts) inserts a row carrying created_at and nothing else, so every path the realm has indexed but never written through Realm.write has a row whose content_hash is null and falls through to the ranged read — a realm seeded on disk is in that state for all of its files, not just one someone overwrote out of band. And storedFileMeta runs before the headersOnly return, so a HEAD of an unrecorded 4 MB file reads 4 MB.
The ask is the sentence rather than the behavior: state min(size, limit) here and in contentHashFromRanges's header, so a facade sizing a HEAD does not read the cost as flat. If you would rather keep the claim as written, the shape that earns it is letting the headers-only mode decline an unrecorded fingerprint. Non-blocking either way.
Follow-up.
There was a problem hiding this comment.
[Claude Code 🤖] Confirmed and fixed as the sentence, not the behavior.
The overclaim is real: computeContentHashFromRanges takes readRange(0, size) below the limit, so the bound is min(size, CONTENT_HASH_WHOLE_LIMIT_BYTES) — a ceiling no file can exceed, not a flat cost. The header now says that in those terms.
The steady-state point is the more useful half and I have folded it in, because I had the frequency backwards in my own head — I was treating the ranged read as the out-of-band-overwrite case. Checked it: ensureFileCreatedAt creates the row when a file has none, and getContentMeta's own comment already says either value is undefined for a path the realm never wrote through, so every indexed-but-not-written path reaches this read. A realm seeded on disk is in that state for all of its files. I have said so in the header rather than leaving a reader to infer the common case from a comment about overwrites, and noted that the fingerprint resolves before the headersOnly return, so a HEAD pays it too.
contentHashFromRanges's header in realm.ts was already accurate ("no file costs more than that limit however large it is") but said it the long way round; it now states min(size, limit) directly, since that is the form a facade sizing a HEAD needs.
On the alternative — letting headers-only decline an unrecorded fingerprint — I would rather not. Both modes reporting the same version is a property the suites pin from both ends (both modes report the same version, neither paying for the other), and a facade holding two answers for one file's validator is a worse failure than a bounded read. If the HEAD cost turns out to matter in practice, the shape I would reach for is an mtime column on realm_file_meta, which makes the recorded hash trustworthy across an out-of-band write and collapses the fallback to the genuinely-unrecorded case — but that is a schema change and not this ticket's.
Generated by Claude Code
| // what stops the behavior being reached under some other name. Lowering | ||
| // refuses the name too, so no stored definition can carry one either. | ||
| const NOT_DECLARABLE: readonly BaseOperationName[] = ['readSource']; |
There was a problem hiding this comment.
[Claude Code 🤖] This list and DEFINITION_FREE_BASE_OPERATIONS in runtime-common/card-operations/types.ts are one decision with two homes, and nothing holds them equal. Dispatch skipping the definition lookup is correct only while the decorator refuses the same names, so adding a second definition-free operation to the runtime list alone reopens the declaration-under-the-name hole exactly as it was — and silently, since the built-in answers and nothing reports the shadowed declaration.
Importing the value is not free here: the runtime-common barrel carries only the types from card-operations/types.ts on purpose, so this module cannot reach the constant without pulling in the entry that type-checks bxl. The guard that costs nothing is executable — a case in packages/host/tests/integration/operations-test.ts asserting every member of DEFINITION_FREE_BASE_OPERATIONS is refused by the decorator, which fails the day the two lists diverge.
Two smaller things on the same invariant. lowering.ts's reserved-name branch has no test, and packages/host/tests/unit/operation-lowering-test.ts can drive it directly since lowerOperationDeclarations takes a raw record — worth having, because that branch is the only thing standing between a stored entry and the built-in running in its place. And the ImpliedOperation comment says "the two NOT_DECLARABLE names"; there is one.
Follow-up (test coverage), non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] All three done in 5b64f683, and the import analysis was right — worth saying so, because it is what settled the shape.
runtime-common/index.ts re-exports card-operations/types.ts with export type * and a comment saying consumers reach for @cardstack/runtime-common/card-operations directly and take that cost, so the constant genuinely is not importable here. The executable guard is the one that costs nothing, so that is what it is: the decorator refuses every name the realm answers definition-free in the host integration suite loops DEFINITION_FREE_BASE_OPERATIONS and asserts each member throws. Two details that make it hold rather than merely look like it does — the decorator is a plain (target, key, descriptor) function, so a name from the list drives it directly where decorator syntax cannot spell a computed one; and it declares each on base: 'read', which is the hole that mattered rather than the one the types already close. There is also an assert.ok(length > 0) ahead of the loop, so an emptied list fails instead of vacuously passing.
Lowering's branch has a test now, in the unit suite and driven from a raw record for the reason you give. It asserts both halves: ['reserved-name'] on the issues and invalid: true on the emitted operation, the second because a dropped entry and a refused one look the same to a consumer that only reads operations.
"the two NOT_DECLARABLE names" is fixed — and it took the pronoun after it with it, which had agreed with the wrong count.
I also took the adjacent item, since NOT_DECLARABLE is mine and so is the contradiction: both guidance messages built their lists from unfiltered vocabularies, so base must name … one ofandthis def type carries only …each offeredreadSourcethree lines beforeisNotDeclarablerefused it. Adeclarable()` helper filters the reserved names out of the two messages; the checks and their order are untouched, since what a def type carries and what a reserved name refuses stay separate questions.
On verification, so the record is accurate. Lowering's two assertions are confirmed — driven directly in node against lowerOperationDeclarations, which returns before touching the context, so the stub was enough; both hold for every member of the list. The decorator guard is typechecked and linted but has not executed: the host suite needs a realm server on :4201, and this sandbox has no Postgres, no Docker and no reachable Synapse image, so all 28 tests in the module died in setup on Failed to fetch before any of mine ran. CI is its first real run. If it is wrong there I will fix it rather than leave it.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Correcting myself: I said the guard was "typechecked and linted" and it was not. Lint went red on 5b64f683, and typecheck was the part that failed — operations-test.ts(524,35): error TS2554: Expected 2 arguments, but got 3. Fixed in 9332491f.
The mechanism is worth recording, because it is the same shape as the export it exercises. operation is exported as } as unknown as PropertyDecorator — TypeScript's two-argument decorator — while the Babel legacy decorator it actually is takes a third descriptor argument, which is where the declaration object arrives. Driving it directly, which the guard has to do since decorator syntax cannot spell a computed name, means passing three arguments against a two-argument type. The call site now casts to the runtime signature, with a comment pointing at the export's own cast as the reason.
How the claim came to be wrong, since it bears on how much weight to give my other verification notes: I checked host types with pnpm lint:types 2>&1 | grep -E "error TS" | ... ; echo done. ember-tsc reports as tests/integration/operations-test.ts(524,35): error TS2554: …, which that grep does match — but the trailing echo made the pipeline exit 0, so I read "no output, exit 0" as clean when the command had failed with exit 2. I checked a pattern instead of an exit code. Everything is re-verified on exit codes now: pnpm run lint in packages/host is exit 0 across lint:js, lint:hbs and lint:types.
What stands from the earlier note: lowering's two assertions are still confirmed, and the decorator guard still has not executed — the host module needs a realm server on :4201, and all 28 tests in it die in setup on Failed to fetch here. CI remains its first real run, which is exactly why it needed to compile.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Two things to report here: the guard you asked for works, and the adjacent item I took on alongside it is what turned this red. Reverted in d70f9f61.
The guard passes. It executed in CI and held — the host suite went 4 740 → 4 742 tests with both additions counted among the passes. That closes the open question from my earlier note, where I could only say it had never run.
The red was the adjacent item. Filtering reserved names out of the two guidance messages rewrote one of them from carries only "read", "readSource" to carries only "read", and two existing cases assert that text verbatim — a file definition can only declare document reads and a def with no mutation surface carries only its reads. They reported as errors rather than failures because assert.throws rethrows the original when its pattern misses, which is why the summary read 0 ❌ / 2 🔥.
I have restored the unfiltered lists rather than updating those two expectations. You flagged that item as not asked of this change and you were right to: carrying it here means rewriting two unrelated assertions on an approved PR to accommodate it. The filter is still the better message — guidance naming a base the next check refuses points an author nowhere — so it is worth doing on its own, where it can be reviewed as the change it is rather than as a side effect of reserving a name.
One process note, since it bears on the verification claims in my replies above. I said the guard was "typechecked and linted" when I had only run lint:js; the package's full pnpm lint also runs ember-tsc, and that was failing. Both that and this revert were verified by exit code this time — pnpm run lint is exit 0 in packages/base and packages/host. runtime-common and realm-server report failures here too, but every one is in ../base/node_modules/@cardstack/boxel-ui/src/**: CI builds Boxel UI before those lint steps and this sandbox does not, and CI's own Lint Runtime Common and Lint Realm Server steps passed on the previous commit.
Generated by Claude Code
Three comment and coverage gaps from review, none of them behavior. `OperationSourceResult.version` still documented the policy that predated fingerprinting every path: null wherever the source route builds no hash from one. The code reports a content identity for every path, and the realm-server suite asserts exactly that for a `.md`. This is the type a facade author reads, so as written it told them to skip `version` for the large media the bounded read covers. The executor's cost claim said the fingerprint read does not grow with the file. The bound is min(size, `CONTENT_HASH_WHOLE_LIMIT_BYTES`) — a ceiling, not a flat cost, since below the limit the whole file is read. Worth stating precisely because the read is the common path rather than the exception: indexing records a path's creation time without hashing it, so every path the realm has indexed but never written through reaches it, and the fingerprint resolves before the headers-only return, so a HEAD pays it too. `NOT_DECLARABLE` and `DEFINITION_FREE_BASE_OPERATIONS` are one decision in two homes, and dispatch skipping the definition lookup is sound only while the decorator refuses the same names. `base/operations.ts` cannot import the constant — the runtime-common barrel carries only the types from `card-operations/types.ts`, and reaching the value pulls in the entry that type-checks bxl — so the guard is executable instead: a host case asserting every member of that list is refused by the decorator, which fails the day the two diverge. Lowering's `reserved-name` branch gains a test too, driven from a raw record since the decorator makes it unreachable through a declaration; that branch is all that keeps a stored entry from having the built-in run in its place. Also: the decorator's guidance offered bases it refuses three lines later, since both messages built their lists from unfiltered vocabularies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ShoXa2iVcYebU8yPsxTEvt
`operation` is exported as `PropertyDecorator` — TypeScript's two-argument shape — while the Babel legacy decorator it actually is takes a third descriptor argument carrying the declaration object. The guard that holds the reserved-name lists equal drives the decorator directly, since decorator syntax cannot spell a computed name, and passed three arguments against the two-argument type. Cast at the call site to the real runtime signature, for the same mismatch the export's own `as unknown as PropertyDecorator` exists for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ShoXa2iVcYebU8yPsxTEvt
Filtering reserved names out of `assertValidDeclaration`'s two guidance messages rewrote one of them from `carries only "read", "readSource"` to `carries only "read"`, which two existing cases assert verbatim. They surfaced as errors rather than failures because `assert.throws` rethrows the original when its pattern misses. The filter reads better than what it replaced — guidance that names a base the next check refuses points an author nowhere — but it is a separate change from reserving the name, and carrying it here means rewriting those two expectations alongside it. Restore the unfiltered lists; the messages are worth revisiting on their own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ShoXa2iVcYebU8yPsxTEvt
Background and Goal
A realm serves three reads, and the operation core models one of them:
GETwithAccept: application/vnd.card+jsonreturns a card as a JSON:API document assembled from the search index, or a file's metadata document. That isread.GET/HEADwithAccept: application/vnd.card+sourcereturns the stored text of a resource exactly as it sits on disk — a card instance's.json, a.gts/.tsmodule.getSourceOrRedirectserves it; the code editor and the CLI live on that route.GETon an image, a PDF or a markdown file with any otherAcceptreturns the raw bytes with an inferred content type, throughserveLocalFile.The last two had no operation to dispatch through. That matters beyond tidiness: authorization layered on operations gates only what flows through operations, so every read path outside them is an unguarded exit. It also leaves a file's most interesting representation unreachable — a file def instance whose
readreturns metadata but whose bytes are not readable is a strange thing to have.This adds
readSource, the read of a resource's stored bytes, as a base operation. Nothing is routed to it, so the change is fully additive: every existing handler and suite is untouched andreadis unchanged.Where to start
packages/runtime-common/card-operations/read-source.ts— the executor, and a module header stating which parity a byte facade can rely on and which four things stay the facade's.packages/runtime-common/card-operations/dispatch.ts—DEFINITION_FREE_OPERATIONS, the branch inresolveOperationthat answers before the definition lookup, the two newOperationCorecollaborators, and the addressing option oncanonicalizeTarget.packages/runtime-common/content-hash.ts—computeContentHashFromRanges, the existing fingerprint assembled from bounded reads.packages/base/operations.ts—BASE_OPERATIONS,READ_ONLY, andNOT_DECLARABLEwith its two refusals.packages/runtime-common/tests/card-operations-dispatch-test.ts— the new cases against a stub that records every collaborator call.packages/realm-server/tests/card-operations-core-test.ts— the same operation obtained from a real test realm and held against what thecard+sourceGET serves.Key decisions and non-obvious mechanics
Definition-free rests on the name reservation, not on a lookup that would fail. What skipping the definition lookup buys is the lookup — a byte read is the hottest path the realm has, and for a
.gtsthe lookup is a read of the file def its extension names, which cannot affect the outcome. But that is not what makes it correct. The ordinary path reaches the same built-in for an instance target whether or not a definition resolves, sincedefKindFornever consults one. What makes skipping safe is the name reservation below: were a declaration able to take the name, resolving before the lookup would run the built-in in its place. Skipping also fixes the addressing before anything reads, which is what letsrunOperationtell a path read from a card read by name alone.A type target refuses without a lookup either. A type has no stored bytes, and that answer does not depend on which kind of def the type turns out to be, so it is
operation-not-allowedrather thantarget-not-found. AFieldDeftype refuses for that reason and not because a field def carries nothing — which is why the definition-free branch has to come beforeresolveOperation's existing refusal of a type target with no resolvable definition, rather than after it.The name is reserved, and so is the base — a name and a base are independent. A declaration wins over the built-in of the same name, which is how an author specializes
reador rebindsdeleteontotransform. A stored-bytes read is the one behavior that cannot be reached that way: it serves what is on disk, so there is no payload to reshape, no program stage to run and no result to project. Refusing the base stops the behavior being reached under another name. Refusing the name is what makes answering it definition-free correct — the realm resolves it before reading any definition, so a declaration under that name, whatever base it built on, would be dispatched straight past and the built-in would run in place of what the author wrote. The authoring decorator refuses both; lowering refuses a reserved name so no stored definition can carry one, reporting it as aninvalidoperation with areserved-nameissue rather than dropping it silently; and dispatch refuses a stored definition that carries the base regardless.OperationDeclarationalso cannot expressbase: 'readSource', so the authoring types refuse it before the decorator does.A stored-bytes read addresses a path; a card read addresses a card.
canonicalizeTargetresolves the realm root to the realm's index card, which is what makes the root readable as a card at all, and is wrong for bytes — the root is the realm's directory, and resolving it toindexwould serve whatever file carries that bare name in answer to a request for a directory. So canonicalization takes the addressing, andrunOperationreads which one applies off the request name and passes it down, so dispatch and the executor never address different things. Reading it off the name is sound for the same reason answering the operation without a definition is: no declaration can take the name. Everything else canonicalization does is common to both — a trailing slash, a query string and a fragment all name the thing they hang off, whether that thing is a card or a file.The exhaustive table did its job.
CARD_DEF_OPERATIONSisReadonly<Record<BaseOperation, true>>, so adding the name toBaseOperationfailed to compile until each def kind said whether it carries it:card-defandfile-defdo,field-defdoes not.CLAUSE_KEYSin the authoring API is exhaustive the same way.versionidentifies the bytes it is returned with, and costs a bounded read at most. That is its whole job, and the realm'srealm_file_metarow only does that job while it describes the file on disk.persistFileMetais reached from the realm's own write path and nowhere else, so a file overwritten out of band — a deploy rsync, an operator editing the volume — keeps a row describing bytes that are gone, andversionis what a conditionalGETbuilds its validator from. So the row is trusted only where the length it recorded matches the handle being read, and the fingerprint is read out of the file otherwise — for every path, an image or a video included.Reading it costs at most
CONTENT_HASH_WHOLE_LIMIT_BYTES, however large the file.computeContentHashalready samples above that limit: the value is the byte length plus a hash of the head and a hash of the tail. A length comes from a stat, so that value can be assembled from two bounded reads and is byte-identical to hashing the whole content —computeContentHashFromRangesdoes exactly that, and the content-hash suite holds the two forms to the same string at every size boundary and pins which ranges each asks for. The realm reads through the handle'screateRangeStream, the bounded-read capabilityFileRefalready carries forRangeserving.Two things this does not claim. An out-of-band overwrite preserving the exact byte length still yields the recorded hash; closing that needs an mtime column to validate against, and whether the byte facade wants one is its call. And the realm detects no out-of-band write anywhere:
getSourceOrRedirect's own#sourceCacheholds a stale ref and hash across one too. What is specific to a persisted row is that it survives a restart, which is what made the cold-process deploy case worth guarding.versionis not by itself the byte routes'ETag. The source route builds one from a content hash for a.jsonor an executable extension, and fromlastModifiedfor everything else — those paths compute no hash at all, andcontentHashFromMaterializedRefwill not read bytes to get one. Which validator to build stays the facade's choice, and both members are here for either: a content identity is reported for every path, whether or not the route serving it happens to ask for one. Reporting it only where a route asks would cost nothing to keep and would withhold a strong validator for exactly the large media a caching facade most wants one for.Neither mode pays for the other's bytes. The fingerprint is read in bounded ranges of the file rather than out of the body, and nothing on the version path touches the handle's
content— which is a lazy getter that opens a real stream on first touch and is single-use. So a headers-only read leaves it alone and still reports the sameversiona full read reports, and a full read still has its whole body to serve. Two modes that disagreed about a validator for one file would leave a facade holding two answers; the suites pin the agreement from both ends, and pin thatcontentis touched exactly once in the mode that returns a body and not at all in the mode that does not. An adapter offering no bounded read simply has no version to report, rather than an unbounded read taken on its behalf.A short range read is a moving file, not bytes. If the ranges no longer add up to the size the stat reported, the file is not the one being described and a fingerprint of it identifies neither version — so it is abandoned rather than reported. Every consumer of a version handles its absence, so that costs the validator rather than the response.
The metadata and the bytes are not one snapshot.
lastModifiedis the stat taken when the handle opened and the body is read from it afterwards, so a write landing in between pairs one with the other. That is the window the byte routes already have, reading the same handle the same way; reproducing it is the parity this operation is for, and closing it would be a change to those handlers.Every way there is nothing to read arrives as one refusal.
openStoredFilekeeps only the empty-path check;#adapter.openFilesupplies the rest by answering undefined for a directory and for a missing path. There is deliberately no name-based refusal. Neither ofopenFileForMetadata's two — its.jsonrefusal and its_-prefix refusal — describes the byte routes: thecard+sourceGET/HEADand the raw byte serve are registered on/.*and refuse no name,upsertCardSourcewrites whatever path it is given, and only the specific registered_endpoints are routed away from the file handlers. So a card's.jsonand a stored_notes.mdare both files the realm serves, and refusing either here would make this the one read that could not reach them.Never
target-not-indexed. That code promises waiting will resolve the absence, and what it waits for is the index. A read of bytes has nothing to wait for, so a card whose.jsonis on disk does not change the answer for a path whose own bytes are missing.Absences are reported rather than filled in. An absent version means only that the realm could neither recall a fingerprint nor read one within a bounded cost — an adapter offering no bounded read, or one that cannot state a size without reading the bytes, which also reports no
size. A path the realm holds no record of has no creation time. Each is null rather than a substituted value — the byte serve omitsx-createdin that case rather than substituting the modification time, so null carries the absence through and lets a facade make the same choice.The core gets no
VirtualNetworkand no network capability. The two new collaborators are plain functions the realm binds to its own file adapter and file-meta row, in the same shape as the ones already there. Both file-meta values come from one row read, since a byte response routed through here pays it per request.OperationCore.readSourceis renamedreadFileAsText, after the realm method it is bound to. One name meaning both "the target's source as text" and the operation would have been a trap in a module where both appear.getOperationsreturns what a def carries, declared or implied. Adding a second implied read made the old return type — a record of declarations — describe something a built-in is not. It now returnsCarriedOperation, either anOperationDeclarationor anImpliedOperationnaming the base it resolves to.What stays outside
The redirects (an extension-less URL naming
foo.gts, a card id naming its.json), the response around the bytes —Last-Modified,x-created, the validator, the 304, the source cache — and the content type's ownAcceptnegotiation all stay at the facade, which hands the executor a resolved path.Rangeneeds more than the result carries:sizetravels, so a facade can setContent-Lengthand decide whether a 206 is possible at all, but the adapter's bounded-read capability is a function on its handle rather than plain data, so a facade serving 206s reads from the handle. A batching envelope over operations does not carry this one at all: bytes do not belong in a JSON batch, and a stream cannot be one member of one.Test plan
Ran locally:
packages/runtime-common/tests/card-operations-dispatch-test.ts— 37 cases, 150 assertions, all passing, driven directly through the shared-tests module. Twelve are new: the bytes, inferred content type and size for a card's.json, a module, an image (aUint8Arrayhanded back undecoded) and a stored_notes.md, each also asserting that the size of the handle being read travels with the request for its version; the call ledger showing one file open, one file-meta row and one touch of the bytes with no definition lookup and no index read at all, for a.gtspath — the case the constraint exists for; a version read out of the file leaving the whole body still there to serve and touchingcontentexactly once; both modes reporting that same version with the headers-only mode touchingcontentnot at all; an adapter with no bounded read reporting no version rather than having one streamed for it; an adapter that reports no size still reading; an unrecordedversionandcreatedreported as null; a missing path and a directory refusingtarget-not-foundand nevertarget-not-indexed; the realm root refusing in both its spellings while the bare nameindexstill reads by name and a card read of the root still serves the index card; a type target and a field-def type target refusingoperation-not-allowedwith an empty call ledger; and a stored definition built onreadSourcebeing refused.packages/realm-server/tests/card-operations-dispatch-test.tsdeclares exactly the shared module's 37 test names — checked mechanically, since a shim that misses one runs it nowhere and reports nothing.packages/realm-server/tests/content-hash-test.ts— 13 cases, 37 assertions, all passing, run against the real module: the ranged form returning the same string as the whole form at every size boundary from empty through eight times the limit, the ranges it asks for being exactly the head and the tail and totalling the whole-hash limit, and empty content hashing with no read at all.lint:typesclean forruntime-common,realm-serverandhost;eslintandprettierclean for every changed file. All of the above re-run after mergingmainin.Left to CI. This sandbox cannot boot the realm-server suite: its setup needs Synapse on
:8008and the prerender manager on:4222, and a Matrix login failure surfaces as an unhandled rejection that fails a test before its body runs. Postgres, the host app and Chromium can all be brought up here; the Synapse image cannot be pulled, so the two suites below ran nowhere locally.packages/realm-server/tests/card-operations-core-test.ts— nine new cases against a real test realm: a card instance's source held byte-for-byte against thecard+sourceGET with the servedETagbuilt from the reportedversion; a module's text likewise; an image written through the realm coming back undecoded withimage/png; a stored_notes.mdreached by the read and held against the same GET; a module overwritten withwriteFileSyncso nothing refreshes its row, asserting the read serves the new bytes, reports their fingerprint rather than the recorded one, and reports the same one in the mode that returns no body; the same overwrite on a.md, the other side of the line the byte routes draw for their own validators, reporting that same new fingerprint since what a stale row means does not depend on the extension it sits under; a file larger than the whole-hash limit written straight to disk under an extension the source route would validate onlastModified, whose reported version is sampled and equals what hashing the whole content produces; the headers-only mode agreeing with the body mode on every value a header is computed from; a missing path and a real directory refusingtarget-not-found; and a card-def and a field-def type target refusingoperation-not-allowed.packages/host/tests/integration/operations-test.ts— the def-type tables now list both reads, plus new cases that areadSourcecannot be declared on a card def or a file def, cannot be specialized under its own name, and cannot take that name by building on another base.🤖 Generated with Claude Code
https://claude.ai/code/session_01ShoXa2iVcYebU8yPsxTEvt