refactor(storage): Phase 4.5 — move storage-owned layout inside the boundary - #93
Merged
cxxxxxn (cxxxxxn) merged 9 commits intoAug 18, 2026
Conversation
…boundary §12.2.2 moved `paths.ts` and `canvas-dirs.ts` out of `storage/` on the grounds that they serve "non-storage domains". That reason is inverted: a storage detail with consumers outside `storage/` describes a leak rather than earning a home outside the boundary. Record the correction against §12.2.2 and add §12.5 as Phase 4.5, to run before the SQLite phase so consumers are not migrated twice. The census is smaller than the raw import count implies — one production site outside `storage/` reads a storage-owned path. The problem is that `workspace/disk/` is a mixed module whose name asserts the substrate: it holds the Disk record layout, the blob layout, a `space.json`-derived index, pure naming logic, and genuine workspace concerns, all at once. Also records the path families that map to no port and no table — prompt logs, ACP sessions, per-Space memory — which would otherwise split one Space's state across two substrates with nobody deciding it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replace the "place vs storage" split with the operative question: is the symbol still useful, unchanged, once the structured backend is SQLite? Applied per symbol it sorts into four groups rather than three, and forces two corrections. `WORLD_CANVAS_DIR_NAME` is a directory name, so it is Disk's — SQLite encodes World as a column with its own reserved key. `canvasRoot` cannot simply move: its body resolves through an index built from `space.json`, so it is the materialization anchor and has to be re-founded rather than relocated. Reading the implementations also corrects §12.5.3. `chatPromptLogPath` is a debug artifact its own comment says the app never reads, not unowned durable state; `acpSessionsPath`/`canvasAcpNamespace` are Agenetes' own store reached through a namespace root, so they belong to the agent domain. The finding underneath both: `.history/` conflates the Disk structured tier with per-Space state owned by other domains. Records the flat `modules/workspace/` decision and the dependency direction it settles. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`workspace/disk/naming.ts` held no filesystem access — only string rules — while its path asserted a substrate. Every consumer that needed a safe filename or a comparison key had to import through a module named `disk` to get logic that has nothing to do with disks. Move the rules to `utils/naming.ts` and update all 15 call sites. The module is deleted outright rather than left as a forwarding shim: every importer is updated in the same commit, so a shim would only preserve the wrong direction for the next person to follow. Logic is unchanged. The file is byte-identical to the extraction Phase 5 performs as a side effect of needing the rules in a second backend, so that branch rebases onto this with the file already in place. Phase 4.5, step 1 of 6 (proposal §12.5.5). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`workspace/disk/paths.ts` described two unrelated things: where the Disk backend keeps a Space, and where the workspace keeps things that have nothing to do with a backend. Sorting them by the §12.5.2 test — is this still useful once the structured backend is SQLite? — splits cleanly. The Disk record and blob layout moves to `backends/disk/layout.ts`, together with `canvas-dirs.ts` and `name-index.ts`. Those two had zero consumers outside `storage/` already; they were filed under a `disk` segment in the workspace module while being, in fact, Disk structured-backend state built by parsing `space.json`. What stays behind is the residue that survives the test: `setting/`, user skills, user memory, and the per-Space state other domains own. Those anchor on a new `spaceDirectory()` capability exported by the composition root, so they no longer resolve through the Disk name index — the Space counterpart to `BlobScope.materialize()` (§12.5.4). The shim importer list shrinks by fourteen: most consumers turned out to want workspace-owned paths and now say so directly. Five entries join it as relocations of couplings that already existed, and the four remaining storage-owned reads outside the boundary — `chatDir`, `nodesDir`, `changesPath`, `artifactsDir` — are recorded in the boundary test rather than hidden, for step 5 to remove. Behavior is unchanged: every path resolves to the byte-identical location, and no Disk test expectation moved. Phase 4.5, step 2 of 6 (proposal §12.5.5). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two of the storage-owned reads outside the boundary turned out to need
neither a port nor a relocation.
`readChatDigest` scanned `.history/chat/*.json` for a `{ messages: [] }`
shape that two migrations retired: turns moved to `.turns.jsonl` and then
into the Agenetes Tier-2 store under `chat_v2/`. The only live `.json`
writer left in that directory is the change-review sidecar, whose payload
is an array with no `messages` key, so every file failed the guard — the
reader had returned nothing in any migrated workspace. Its test pointed
`chatDir` at a directory that does not exist, so the branch never ran
there either.
Serving it with a `SpaceChats` port would have handed storage authority
over data it does not own — threads and turns belong to the agent runtime,
as the legacy store says outright — and obliged every future backend to
model Agenetes' turn log. Removed instead; whether the memory agent should
see turns at all is filed as a follow-up against `agenetes.history()`, the
call canvas-search already uses. `latestChatTs` and its cursor stay: they
are the resume point a reinstated digest would need.
`import-node-src.ts` asked whether a path was already inside `.artifacts/`
by rebuilding the Disk artifacts directory. `BlobScope.owns()` lets the
scope answer for itself — pure, synchronous, and `false` on a backend that
stores nothing locally. It is covered in the shared blob contract, so both
answers are pinned for every adapter.
Also corrects the §12.5.1 census, which was built by searching for
`workspace/disk/` and so missed every consumer reaching the same symbols
through the `storage/paths.js` shim: six production files outside the
boundary read a storage-owned symbol, not one.
Phase 4.5, step 5 of 6 (proposal §12.5.5, findings in §12.5.7).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replaces `BlobScope.owns(absolutePath)`, added one commit ago and wrong. `materialize()` runs port → filesystem: the port renders a real path as a service, and any backend satisfies it by spooling a temp copy. `owns()` ran the other way, handing the port a path in a vocabulary only a local backend can interpret. The `false` a remote backend would return is not a neutral implementation but an admission that the question is meaningless there — so the fix removed a Disk-layout import from one consumer by moving the Disk assumption into the port, where it would constrain every future backend. That is the mistake this phase exists to correct. The caller never needed storage. `toPhysicalRel` already maps the virtual `artifacts/` prefix onto the hidden directory, so "is this ref already an artifact?" is a question about the ref. `isArtifactsRel` answers it in `fs-sandbox.ts`, which owns that mapping, resolving against a synthetic root so no workspace, canvas directory, or filesystem is involved. Writing its test surfaced a limitation worth pinning rather than fixing: the virtual alias applies as a prefix, so `nodes/../artifacts/x` is not treated as an artifact. The pre-existing check behaved identically, and widening it would change which files the import hook copies. Phase 4.5, step 5 of 6 (proposal §12.5.7). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Completes Phase 4.5 steps 3, 4 and 6. `space-dir-handles.ts` and `world-canvas.ts` join the rest of the Disk layout under `backends/disk/`. Both fail the §12.5.2 test for the same reason: releasing directory handles exists so Windows can rename a Space *folder*, and the World bootstrap writes one — neither has anything to do once Spaces live in tables. `workspace/disk/paths.ts` becomes `workspace/paths.ts`, and the `disk/` segment is gone, because what remains there describes the workspace as a place. The five consumers still resolving `canvasRoot` through the shim now call `spaceDirectory()`. The two Disk capabilities the application genuinely needs are re-exported from the facade, so app code asks for a capability instead of naming a backend. Three new boundary guards pin the outcome: the workspace module has no substrate segment, imports no backend, and names no Disk layout symbol — the last one catching a reintroduction even if the import path looks innocent. The reach-into-backends rule now exempts tests, matching the exemption the composition-root rule already made and for the same reason: a production file naming an adapter has bound the app to a backend, while a test naming one is choosing its subject. Two test mocks needed following rather than rewriting. `canvas.route` mocked the facade for a Disk rename failure, which no longer intercepts the adapter's own import; it now mocks the module under test. `external-watcher` stubbed the facade wholesale, which would have replaced the real handle registry the assertions depend on; its factory now delegates those two members so both sides share one instance. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Yuge Zhang (ultmaster)
force-pushed
the
feat/multi-backend-storage-phase-4-5-storage-boundary
branch
from
August 17, 2026 06:06
b3ec20a to
b8a1e16
Compare
…n hops
A workspace can hold both formats for one thread: the legacy pi-ai
`Context` and a `.turns.jsonl` the live app wrote independently. Hop 1
threw whenever the Context's conversion was not a prefix of that log, the
sweep swallowed the error, and hop 2 then skipped the thread because the
`.json` was still there. Neither copy advanced, `chat_v2` never received
the turns, and the thread rendered empty — silently, on every launch. The
comment promised a later activation would retry, but nothing changed
between activations, so the stall was permanent.
Give the coexistence decision one owner and make every branch terminate:
- Hop 1 resolves divergence instead of throwing. The live log wins,
because it is the one the app has been appending to; the Context is
preserved verbatim as `<thread>.json.unresolved` and the divergence is
logged. Its extra turns stay off the canvas, which is also what
happened before the reconciliation existed — but the bytes remain and
the turn log is free to fold.
- Hop 2 drops its same-thread `.json` gate. A Context still sitting
there is one hop 1 could not read, and folding the turn log anyway is
what keeps the history reachable.
- The admission gate no longer validates every message against today's
`Message` union. These files come from older builds, so an unfamiliar
row is expected; `legacyContextToTurns` already tolerates unknown
roles and now skips non-object rows too. One junk row cost the whole
thread its history.
- The tolerant sweep logs what it could not migrate.
Add an end-to-end suite that activates a legacy workspace the way a launch
does and drives the production routes at the production prefixes: the
pre-rename files migrate, the Space / node body / artifact / events read
back over HTTP, `GET /api/agent/history/:threadId` serves all three seeded
threads, and new writes (node content, an agent execute that imports an
artifact, a new Space) survive a second activation without double-folding.
The Space is titled so its directory name differs from its canvasId, so a
layout that leaked the id into a path fails there.
Reinstating either old behaviour fails that suite with an empty message
list for the divergent thread, which was the user-visible symptom.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Runs before #92 (Phase 5, SQLite). That branch will need a real rebase
onto this, not a replay — see Effect on #92 below.
Why
§12.2.2movedpaths.ts,canvas-dirs.ts, andnaming.tsout ofstorage/into
modules/workspace/disk/, on the grounds that they serve "non-storagedomains". That reason is inverted: a storage detail with consumers outside
storage/is describing a leak, not earning a home outside the boundary.The result was a module named
disk— sitting outside the storage boundary —that held the Disk record layout, the blob layout, an index built by parsing
space.json, and pure string rules, all at once. Six production files outsidestorage/read a storage-owned symbol from it. Fixing that after a secondbackend exists would mean migrating the same consumers twice.
The rule this restores: outside
storage/, nothing knows how Spaces arestored. A domain may know a Space is materialized somewhere — that is a
declared capability with real consumers — but not that its record is
space.json, nor that its events are a JSONL file.The test that decided each move
A symbol belongs outside
storage/only if it is still useful, unchanged,once the structured backend keeps Spaces in tables. Applied per symbol rather
than per file, it sorts cleanly:
SPACE_JSON_FILENAME,canvasJsonPath,nodesDir,nodeFilePath,historyDir,chatDir,changesPath,tasksPath,eventsPath,deltaLogPath,WORLD_CANVAS_DIR_NAMEstorage/backends/disk/layout.tsARTIFACTS_DIR_NAME,artifactsDir,artifactPathcanvasRootspaceDirectory()settingDir,userSkillsDir,workspaceMemoryPathmodules/workspace/paths.tsTwo results worth calling out because a looser reading gets them wrong:
canvas-dirs.tshad zero consumers outsidestorage/already. It buildsits index by reading
space.jsonfrom every directory — Diskstructured-backend state that happened to be filed in the workspace module.
It is also why a SQLite profile would silently fall back to id-named artifact
directories, since
canvasDirName()hits its?? canvasIdfallback when nospace.jsonexists.space-dir-handles.tslooks substrate-generic but is Disk's. It exists soWindows can rename a Space directory; under SQLite there is no such rename.
What changed
utils/naming.ts;workspace/disk/naming.tsdeletedoutright rather than shimmed, since all 15 call sites moved in the same commit.
canvas-dirs.ts,name-index.ts,space-dir-handles.ts, andworld-canvas.ts→storage/backends/disk/.modules/workspace/is now flat —paths.tsandmigrations/, nosubstrate segment.
spaceDirectory()added to the composition root: the Space-levelcounterpart to
BlobScope.materialize(), for consumers that genuinely need areal directory (ACP working dirs, the external watcher, RFS). Declared, not
ambient — a profile that does not materialize Spaces refuses here rather than
handing back a path that does not exist.
release and World bootstrap — are re-exported from the facade, so app code
asks for a capability instead of naming a backend.
Two findings from the consumer sweep
The memory analyzer's chat digest was dead, and is removed.
readChatDigestscanned
.history/chat/*.jsonfor a{ messages: [] }shape that twomigrations retired — turns moved to
.turns.jsonl, then into the AgenetesTier-2 store under
chat_v2/. The only live.jsonwriter left there is thechange-review sidecar, whose payload is an array with no
messageskey, soevery file failed the guard. It had returned nothing in any migrated workspace,
and its test pointed
chatDirat a directory that does not exist, so nothingcaught it.
Serving it with a
SpaceChatsport would have handed storage authority overdata it does not own — threads and turns belong to the agent runtime, as
legacy/canvas-store.tsstates outright — and obliged every future backend tomodel Agenetes' turn log. Whether the memory agent should see turns at all is
filed as a follow-up against
agenetes.history(), the callcanvas-searchalready uses.
latestChatTsand its cursor stay: they are the resume point areinstated digest would need.
A rejected port design, recorded because the reasoning generalizes. The
first fix for
import-node-src.tsaddedBlobScope.owns(absolutePath).That was wrong in the same way this phase exists to correct:
materialize()runs port → filesystem, and any backend satisfies it by spooling a temp copy;
owns()ran the other way, handing the port a path only a local backend caninterpret. The
falsea remote backend would return is not a neutralimplementation but an admission that the question is meaningless there.
The caller never needed storage.
toPhysicalRelalready maps the virtualartifacts/prefix onto its hidden directory, so "is this ref already anartifact?" is a question about the ref — answered by
isArtifactsRelinfs-sandbox.ts, which owns that mapping.Guards
Three new assertions in
module-boundaries.test.tspin the outcome: theworkspace module has no substrate segment, imports no backend, and
names no Disk layout symbol — the last catching a reintroduction even when
the import path looks innocent.
The reach-into-
backends/rule now exempts tests, matching the exemption thecomposition-root rule already made and for its stated reason: a production file
naming an adapter has bound the application to a backend; a test naming one is
choosing its subject.
The
storage/paths.jsshim list shrank by fourteen — most consumers wantedworkspace-owned paths and now say so directly.
Scope
mainapps/server/src/{modules,prompt,utils}and the storage proposalAgenetes, or RFS-contract changes
no Disk test expectation was altered
Deliberately not done. Four storage-owned reads remain outside the boundary
(
nodesDir,SPACE_JSON_FILENAME×2,chatDir), each recorded in §12.5.1 withits resolution rather than hidden.
world-target-access.tsontoSpaceRepository.list()is a sync→async signature change with callers, and twoof the others are not storage leaks at all —
canvas.route.tsjoins thefilename onto an import staging directory (bundle format), and
external-watcher.tswatches a directory tree, which is inherently amaterialization feature.
Effect on #92
Phase 5 needs a rebase, not a replay: it carries its own
utils/naming.tsextraction (now redundant), a
workspace/disk/naming.tsshim pointing at adeleted directory, and import lines in
space-title.ts/space-repository.tsthat will conflict. The
utils/naming.tsfile here is byte-identical to the onethat branch produces, so that half resolves to a no-op.
Validation
Run on
b3ec20a3:pnpm run check— passed