feat(storage): make application reads backend-agnostic (phase 4.6) - #114
Conversation
Adversarial review — Phase 4.6Reviewed Verdict: the phase does what §12.6 set out to do, with one regression that must not ship, one in-scope item undelivered, and two design costs worth a decision. One Baseline on 1. Plan coverage
Correctly out of scope: no SQLite, no profile branch, no wire change — the diff touches zero files under 2. FindingsF1 —
|
The node adapter asked the Disk store to enumerate every physical name claiming an id on every read, list, stream and write. That enumeration opens and parses every file in the Space, so a whole-Space list became quadratic and synchronous: 2101ms against 25ms for the scan it wraps, on a 300-node Space, blocking the event loop rather than one request. The warm duplicate set already answers the question, so ask it first and enumerate only for an id it flags. Streaming took `strict` and dropped it on the parse while `list` passed it through, so the two could return different records for a malformed node — the equivalence the new contract asserts, missed because its fixtures are well-formed. The delivered snapshots now also back the returned map, since the adapter republishes its duplicate set only once the scan ends. `readWorldTargetCanvasesStrict` lost the check its name is about when it moved onto the port: malformed topology read as empty instead of refusing. It is restored as a record-shape check, so it survives a backend that keeps topology in tables. RFS had started deriving a node's identity from the markdown's own frontmatter, which is this backend's record encoding leaking back into a feature module. The materialization capability owns that mapping, so it answers it: Disk from the sidecar index, an id-addressed projection from the name. Also: put the previous Workspace path back when a mount swap fails (the commit has to lead, because activation refreshes locators that only resolve once the path is committed); cover the World bootstrap branch a new backend meets first — an empty namespace — in the reusable contract rather than only on Disk; and drop two members nothing called. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three gaps the review left open, all of them about the shape of the interface rather than its Disk implementation. Most readers want a handful of named nodes, not a Space: a selection to describe, a neighbourhood to render, one View to serve. Expressing those as a whole-Space list made an unrelated node somewhere else cost the request. `SpaceNodes.readMany` is the shape they actually want and the one every backend serves better than a scan — one `WHERE id IN` rather than every row. Whole-Space reads stay where the work genuinely is whole-Space: executor prestate, the Space GET, the outline, inspection. The neighbourhood runs its pure geometry once to learn which nodes it wants before reading any of them. `SpaceFiles` was a port with one implementation, a `'disk'` literal for a kind, and a composition root that ignored the profile — a seam in the type system only. It is now an axis with two implementations behind one reusable contract: the title-addressed layout Huabu ships, and the id-addressed one a structured backend that keeps Spaces in tables needs. It is derived from the structured backend rather than configured, because a backend that stores each Space as a directory has already chosen where that Space lives and the materialization has to name the same one — pair them wrongly and a Space's blobs and its records land in different directories, neither looking wrong. Deriving it means there is no knob to get that wrong with, and validation catches a profile built by hand. The phase listed a product-level backend harness in scope and did not deliver one. Contracts prove an adapter honours a port in isolation; they cannot answer whether the application still works when the backend changes. `storage/testing.ts` mounts a real profile through the production lifecycle, and `product-boundary.test.ts` exercises the exit criterion against every mounted profile without naming a directory, a filename, or `space.json`. Phase 5 adds one list entry and inherits the coverage. Also: restore the laziness the built-in file tools lost, so a grep that never touches a node file no longer reads every one of them first; widen the Disk-layout guard from the workspace module to all production code, import-level so a local `artifactPath` is not a violation; and record in the proposal what Phase 5 owes the rebased port. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up: the three open items from the review
F2 —
|
The settled architecture is two backend ports, and `ports/files.ts` was a third. A port abstracts a backend family — swap the adapter, the same contract is served by SQLite or Postgres or Azure. Materialization has no family to abstract: it is the local filesystem under every profile, because its whole purpose is handing a real path to something that cannot take a record. §12.5.4 had already placed it correctly as an explicit capability, the Space-level counterpart to `BlobScope.materialize()`, one level below the ports; promoting it to a peer of them was drift, and giving it a `kind`, a profile axis and a contract entrenched the drift rather than catching it. The tell was there and got worked around instead of read: the "axis" had to be derived rather than configured, because a structured backend that stores each Space as a directory has already chosen where that Space lives. A knob with exactly one correct value per deployment is not a knob, and a configuration axis nobody may configure is not an axis. So it moves to `storage/materialization.ts` beside the composition root, `StorageProfile` goes back to two axes, and the composition root derives the placement policy from the structured backend. Names now say which tier they belong to: `SpaceMaterialization` holds `SpaceTree`s, and the two policies are `titled` and `addressed` rather than `disk-*` kinds that read like backend families. Nothing about the capability's behaviour changes, and both policies still pass one shared contract. A boundary test now pins `ports/` to exactly the two ports, so the next interface that needs a home has to be put somewhere deliberately instead of finding `ports/` convenient. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A Space directory is Disk's, and dressing it as portable was the mistake underneath the last two commits. Only two features ever wanted it — a filesystem tree for RFS, the note watcher, the file-tool sandbox, and the agent domain's own files — and none of that is a storage contract. A backend that keeps Spaces in tables has no directory, and the right answer there is that the capability is absent, not that every backend fabricates one. So the interface says what it is. `diskSpaceTree(canvasId)` and `stageDiskSpaceImport()` are the whole surface, named so that Disk is legible at each of the thirteen call sites, and they refuse on a non-Disk profile rather than improvise. The boundary test holds the exported names and that call-site list exactly, and asserts the barrel exposes nothing that reads as a portable path API, so the surface can only shrink. Each entry is already a reason a non-Disk structured profile is unselectable, which is the fence §12.4 put around ZIP import and RFS upload. Gone with the pretence: the second placement policy, the kind, the reusable contract, the derived profile axis, and the port file. The policy pair existed to make materialization portable and proved nothing without it — its contract passed for both while never checking where a file actually lands. The Disk blob adapter also stops borrowing the Space tree and resolves its own `.artifacts/` placement again, so one module answers "where is this Space" for each population instead of two agreeing by luck. The route out of the remaining list is not a portable materialization. It is those features no longer needing a tree — an agent can reach a Space over the HTTP API rather than a projected filesystem — which is now written down as the Phase 5 decision it actually is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…substrate Phase 4.6 landed a single residual "Space-file" capability covering every consumer that needs a real directory, and gave them one route out: stop needing a tree and reach a Space over the HTTP API. That is right for some of them and wrong for the rest — several want a port, not a network hop in front of state that is still unportable. New §6.4 replaces the single capability: - 6.4.1 puts `space(canvasId)` on the `Storage` interface, joining the structured handle with the Space's blob scope. The ports stay independent and are joined above them, never inside one. Disk's tree becomes `space(id).diskTree`, typed by its absence, replacing `diskSpaceTree()`. - 6.4.2 sorts consumers four ways instead of one — Disk-only, portable with a per-backend implementation, structured record, blob — and makes Disk-only the default: it costs a row in a capability matrix, while the others cost a port change, a contract suite, and a migration. - 6.4.3 assigns all fifteen current consumers. - 6.4.4 makes the extension point a connection, not a data API. The Space handle vends an isolated substrate per namespace — a directory, a table prefix, a schema — and the owner brings its own store and its own SQL, as octostaff's bubble extensions already do. Storage owns namespace isolation and lifecycle only, and never sees the data. §12.7 gains Phase 4.7 for this work and keeps it small: the facade, the substrate with a Disk case, and only the moves that pay for themselves on Disk alone. Change notification, RFS's backend-neutral path vocabulary, and the ACP session relocation are deferred to the adapter or phase that first needs them. Also corrects stale text: §12.7 claimed SQLite "does not owe a materialization — `materializationFor` already pairs it", and canvas-storage.md still listed `ports/files.ts` and derived materialization in its module table. Both described code deleted in 80900df and 513da5a. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
superceded by #125 |
1 participant
Summary
CanvasStoreand layout helpersSpaceFilesmaterialization capability plus staged Workspace storage mounts, backend-neutral World bootstrap, and graceful connection shutdownScope
This is the application-neutrality prerequisite for the SQLite preview in #92. It deliberately contains no SQLite adapter, schema, migration, or selectable SQLite profile.
Verification
pnpm format:checkpnpm lint(0 errors; existing warnings only)pnpm typecheckpnpm -r --if-present --no-bail run testpnpm run buildnode scripts/check-headers.mjs