refactor(storage): drop the barrel and publish narrow entrypoints - #3301
refactor(storage): drop the barrel and publish narrow entrypoints#3301childrentime wants to merge 3 commits into
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Making the SQLite cost visible at the entrypoint is a real improvement, and public-entrypoints.test.ts stating the boundary out loud is the right instinct — a list a reader can check beats a property nobody can see. I also verified the part that is easiest to get wrong and hardest to check: I resolved every one of the 52 published targets against every @maka/storage import in the tree, symbol by symbol, and found no missing subpath and no missing export. That work is sound.
Three things below will fail CI as filed, so I cannot approve this head. Two of them are invisible from this diff, which is what makes them worth flagging rather than leaving to a red run.
The scope question, which is yours to decide rather than mine. The PR argues that dropping the barrel is what fixes #1257. I do not think that premise holds. There are exactly three static value imports of node:sqlite in the tree — operational-target-schema.ts:1, operational-state-backup.ts:16, session-bundle-policy.ts:4 — and all three only construct inside function bodies. This package already owns the seam for that, twice: loadDatabaseSync() in operational-state-store.ts and loadSqliteModule() in sqlite-session-metadata-store.ts, both lazy-require-plus-suppress-warning. A subagent reports restoring the barrel wholesale, converting those three to import type plus the existing loader, rebuilding, and measuring the barrel no longer pulling in node:sqlite. If that holds, #1257 is a three-file fix and this is an 87-file one.
That does not make this PR wrong. The two designs buy different things: dropping the barrel makes cost legible at the import site, lazy-loading keeps the change surface at three files. AGENTS.md's first principle points at the existing seam, and the PR body's claim that "no lazy-load indirection and no warning suppression are involved" is not accurate about this repository either way — both suppressors are on main today and survive this PR. What I am asking for is that the justification match the actual alternative, so whoever approves this is choosing legibility deliberately rather than believing it was forced.
Two contract decisions a maintainer should sign off on rather than a reviewer: removing . is a breaking change to the package's public surface, and eight modules drop from publicly reachable to package-private, which the title does not mention.
AI disclosure: this review was produced with Claude Code (Opus 5) with a subagent covering security, correctness, integration and simplification. I independently re-derived every finding published here: I confirmed the deep dist/index.js import in the smoke script and the bare @maka/storage specifier on main by reading both files, and I reproduced the formatting failure myself by running this repository's pinned Biome over the two files at this head and diffing its output. The subagent additionally reports building the branch and executing the merge; I did not re-run those. Per AGENTS.md this is not independent human review.
`maka --help` printed Node's SQLite ExperimentalWarning to stderr, on a command that never opens a database. The cause was structural, not local: `@maka/storage` published one `export *` barrel, and `cli-core.ts` imported it to reach `resolveMakaDataRoots`. Three modules in that barrel's graph take a static value import of `node:sqlite`, and Node evaluates a builtin the moment it enters a module graph, so every consumer of the barrel loaded SQLite whether or not it wanted a database. `operational-target-schema.ts` was the amplifier: `operational-state-store.ts` imports it, and roughly forty modules import that, which is how three import statements reached 45 of the package's 110 modules and 24 of the barrel's 43 export entries. Remove the barrel instead of working around it. `.` is gone from the exports map, `src/index.ts` is deleted, and the 21 modules that consumers actually reached through it are published as narrow subpaths. This is already the prevailing convention here — `root-authority` has 141 call sites and `execution-stores` 108, against 31 non-test sites on the bare specifier. The three static `node:sqlite` imports stay exactly as they were. They are honest: those modules do need SQLite. What changes is that needing SQLite is now visible in the import path, so `@maka/storage/workspace-root` costs nothing and `@maka/storage/session-store` costs what it should. No lazy-load indirection and no warning suppression are involved. `public-entrypoints.test.ts` pins the boundary: no `.` export, and exactly 29 of the 52 published entrypoints load `node:sqlite`. Widening that set now requires editing the list and saying why. Two tests moved off the barrel's shape rather than its contents: `managed-workspace-baseline` asserted internals were absent from the barrel object and now asserts their modules are absent from the exports map; `provider-request-capture-artifact` reaches its subject directly. Generated-by: Claude Code
… guards - Point the release smoke script's deep import at dist/workspace-root.js, which owns resolveMakaDataRoots now that dist/index.js is not emitted, and guard every such by-path import with a release file-policy test. - Import openStorageWriterComposition through its published subpath; the bare specifier resolved to the removed barrel entrypoint after apache#3295. public-entrypoints.test.ts now rejects bare @maka/storage imports anywhere in the tree, so the next stale-merge of this kind fails a test instead of a build. - Add ./storage-writer-composition to SQLITE_BACKED_ENTRYPOINTS: it statically imports execution-stores and thirteen other SQLite-backed modules. - Detect the SQLite boundary with a module.registerHooks resolve hook instead of matching Node's ExperimentalWarning text, which Node 26 has already reworded. - Drop the dangling main/types manifest fields and assert that every published entrypoint target is emitted by the build. - Assert internals stay private by loading every published entrypoint and checking the union of reachable symbols, not the export map's targets, so a future re-export cannot leak them silently. - Run Biome over the two files format:check rejected. Generated-by: Claude Code Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
0124d97 to
2d5c2cf
Compare
|
@Astro-Han Thanks for the review — every finding was real. All six are addressed at 2d5c2cf, and I've resolved the threads accordingly:
On scope: you're right that a three-file lazy-load fix exists, and the PR body now states it explicitly, along with why I still prefer the structural version — #1257 was fixed the small way once and regressed when its guard was deleted in #2710. The breaking AI disclosure: these follow-ups were implemented with Claude Code (Claude Fable 5); I reviewed the diff and the validation results before pushing. |
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed at 2d5c2cf2d. I dispatched two scoped sub-reviews — integration and simplification/test-quality; I did not run security or resource-bounds dimensions, because an import rewrite has no meaningful surface for either. Everything below I re-derived at this head myself.
All five findings from the previous round are fixed. execution-composition.ts:60 now imports @maka/storage/storage-writer-composition; the smoke script points at dist/workspace-root.js; main/types are gone from the manifest; managed-workspace-baseline.test.ts asserts on the reachable-symbol union rather than a weaker property; and the SQLite probe uses module.registerHooks instead of text matching. I also re-ran the repo-pinned Biome over every changed TS/JS/MJS file outside the apps/desktop/** and packages/ui/** exclusions — byte-identical output, so format:check passes.
I checked the two things that would make a rewrite of this size unsafe, and both are clean. There are zero remaining bare @maka/storage import specifiers anywhere in packages/, apps/ or scripts/ — the four textual hits in scripts/ are package-name list entries, not specifiers. And every subpath any file imports resolves against the new exports map; the set of used subpaths minus the 53 published keys is empty. The one-intent rule holds too: every hunk across the 89 files is an import rewrite, the manifest, the deleted barrel, or a guard for the barrel's absence.
The P1 below is the same mechanism as the smoke-script finding you already accepted and fixed, one file over — a relative reach-in to a dist emit that this PR removes. It is the last one; I grepped the tree to be sure.
Two smaller things that do not warrant their own threads:
apps/desktop/tsconfig.storybook.json:13 still maps @maka/storage to ../../packages/storage/src/index.ts, a file this PR deletes. That config is live in CI — apps/desktop's typecheck runs tsc -p tsconfig.storybook.json --noEmit — but it does not fail today, because nothing in its include set imports @maka/storage. So it is dead config rather than a break. It is worth deleting in this PR anyway: the mapping is non-wildcard, so it would silently shadow the exports map for any future bare import from a story, which is precisely the failure mode this PR exists to remove. The file is not in the diff, hence no inline thread.
The new bare-specifier guard at public-entrypoints.test.ts:120 is, for .ts files, redundant — moduleResolution: "Bundler" already honours exports, so a bare specifier fails npm run typecheck before the guard runs. Its real value is over .mjs/.js, and there it misses the side-effect form import '@maka/storage'; as well as the relative reach-ins the P1 describes. The regression test I suggest on that thread fixes both.
Carrying a P1 → COMMENT. The rest is a clean, well-scoped change and I expect to approve it once the real-model.mjs import is repointed.
AI disclosure: reviewed with Claude Code, including two scoped sub-reviews used as leads. Every finding published here I verified at 2d5c2cf2d myself — I read the deleted barrel's named export lists against the modules they came from, counted external importers per published subpath on the branch, and grepped the tree for surviving packages/storage/dist/ reach-ins. Sub-review findings I could not confirm are not included.
- Repoint the real-model computer-use script at dist/agent-run-store.js; the release file-policy test now walks every script for both the node_modules and the relative packages/*/dist import forms and asserts each target is still emitted, which catches this whole class. - Publish operational-state-store, artifact-store and credential-store through facades that re-export exactly the names the deleted barrel picked. The write authority, the schema-migration internals and the credential file lock return to package-private, and the reachable-symbol union test now names all five withheld symbols. - Drop the five session-bundle entrypoints with no consumer outside the package; public-entrypoints.test.ts now asserts the exact set of consumer-less entrypoints, allowlisting only the five that predate this change, and that every imported subpath is published. - Extend the bare-specifier guard to the side-effect import form and cap the SQLite probe children at four concurrent. - Delete the storybook path mapping to the removed src/index.ts. Generated-by: Claude Code Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@Astro-Han All four findings and both of the no-thread notes are addressed at 049e93f:
Counts in the PR body are updated: 48 entrypoints, 30 SQLite-backed, 18 free, 13 modules package-private. AI disclosure: implemented and self-reviewed with Claude Code (Claude Fable 5); I reviewed the diff and validation before pushing. |
Astro-Han
left a comment
There was a problem hiding this comment.
Approving at 049e93fdf. I reviewed only the delta from 2d5c2cf2d — one commit, nine files — and every finding from the last round is fixed, several of them better than what I asked for.
The P1 is closed at its cause rather than at the symptom. real-model.mjs:13 now points at dist/agent-run-store.js, and release-cli-file-policy.test.mjs no longer reads one file for one literal shape: it walks every script for both the node_modules/@maka/*/dist/*.js and the relative packages/*/dist/*.js forms and asserts each target exists. That closes the whole class, which is what makes it a fix rather than a patch.
The surface-widening P2 got the right treatment. Rather than narrowing by convention, artifact-store-public.ts, operational-state-store-public.ts and credential-store-public.ts re-export exactly the sets the deleted barrel published, and package.json repoints the three subpaths at them — I diffed each against the barrel's old export lists and they match name for name. managed-workspace-baseline.test.ts then asserts the withheld internals stay unreachable, so the narrowing is enforced rather than documented. You also found withCredentialFileLock, which I had missed; that is a fourth internal on the same footing as the three I named.
The consumer-less entrypoints are gone, and the guard that replaces them is stronger than the assertion I suggested: PREEXISTING_UNCONSUMED_ENTRYPOINTS is exact in both directions, so gaining a consumer forces removing the entry and publishing a new consumer-less subpath fails outright, with the comment stating the list may only shrink. That converts a one-time cleanup into an invariant. The dead tsconfig.storybook.json mapping is deleted, and mapWithConcurrency caps the probe children at 4 — the number AGENTS.md actually names.
I re-verified rather than taking the delta on trust: every external @maka/storage/* import in this branch falls inside the narrowed surfaces, ./session-bundle-policy keeps its one real consumer, and the repo-pinned Biome produces byte-identical output for all seven changed TS/JS/MJS files outside the apps/desktop/** and packages/ui/** exclusions.
No findings at this head.
AI disclosure: reviewed with Claude Code. I diffed each new *-public.ts against the deleted barrel's export lists myself, re-scanned the branch for external imports of the repointed subpaths, and ran the read-only Biome check against the head content. The approval is mine and rests on those checks.
Summary
maka --helpprinted Node's SQLiteExperimentalWarningto stderr, on a command that never opens a database. Every other CLI entry did the same. This removes the structure that caused it:@maka/storageno longer publishes a barrel, and every consumer imports the narrow entrypoint that owns what it needs.Fixes the regression first reported in #1257.
Why the warning was there
Node evaluates a builtin the moment it enters a module graph. A static value import is therefore not a declaration of intent — it is the load:
Exactly three modules do this:
operational-target-schema.ts,operational-state-backup.tsandsession-bundle-policy.ts.operational-target-schema.tsis the amplifier.operational-state-store.tsimports it — andoperational-state-store.tsis imported by roughly forty modules, which is how three import statements reached 45 of the package's modules. The last link is the CLI:cli-core.tsimports@maka/storageto reachresolveMakaDataRoots, a pure path helper, and the barrel'sexport *surface hands it the whole storage layer, SQLite included, before--helpprints a single line.The alternative this PR chooses against
Review correctly pointed out that removing the barrel is not the only fix, and the smaller one deserves to be stated so the choice here is deliberate. The three static imports above all construct only inside function bodies, and this package already owns a lazy-load seam twice over (
loadDatabaseSync()inoperational-state-store.ts,loadSqliteModule()insqlite-session-metadata-store.ts). Converting the three imports toimport typeplus that seam is a three-file fix that keeps the barrel and silences the warning.This PR prefers the structural fix because #1257 was already fixed once the small way, and it came back: its guard test asserted an empty stderr, #2710 removed that assertion in a bulk test cleanup, #1994 and #2445 then added static imports, and the warning shipped again. Lazy loading keeps the boundary as a convention each future edit must remember; removing the barrel makes it structural — widening the SQLite surface now requires editing a published entrypoint list in a test and saying why. The two designs buy different things (three files of churn vs. cost made legible at every import site), and a maintainer preferring the minimal fix is a legitimate outcome of this review; I'd then close this and submit the three-file version.
Change
packages/storage/src/index.tsand drop.,mainandtypesfrom the manifest.interaction-store-publicidiom:operational-state-store(schema-migration internals stay private),artifact-store(the lease-gated write authority stays private),credential-store(the file lock stays private). Every other hand-picked module was already surface-neutral, verified by diffing the barrel's picks against each module's runtime exports.main; the touched files inpackages/runtimeare all tests, that package's production code is untouched).managed-workspace-baselinenow loads every published entrypoint and asserts the union of reachable symbols excludes the internals — a claim that survives a future re-export, which an assertion on theexportsmap's targets would not.provider-request-capture-artifactreaches its subject directly.Regression guard
public-entrypoints.test.tsloads all 48 published entrypoints and asserts:.entrypoint and nomain/typesfield;@maka/storagespecifier, side-effect form included — this is the test that would have caught the writer-composition consumer refactor(storage): centralize root writer lifecycle #3295 landed onmainwhile this branch was in review, which merged cleanly and then failed to build;node:sqliteare exactly the 30 listed in the test, leaving 18 that are free of it. Reachingnode:sqliteis observed through amodule.registerHooksresolve hook rather than by matching the warning's text, which Node owns and has already reworded once; probe children are capped at four concurrent.That list is the package's SQLite boundary written down. Widening it later means editing the list and saying why. (
./storage-writer-compositionis entry 30: it statically importsexecution-storesand thirteen other SQLite-backed modules.)Scripts import workspace modules by
dist/file path, outside both the export map and the typechecker — the release smoke script through installednode_modules, the computer-use scripts through relativepackages/*/distpaths.release-cli-file-policy.test.mjswalks every script for both forms and asserts each target is a file the build emits.Validation
Node v24.11.1, macOS arm64, at this head after a clean build.
npm run buildnpm run typechecknpm run lintnpm run format:checkmaka --version/--help/run --helpstderrExperimentalWarningnpm test --workspace maka-agentnpm test --workspace @maka/storagemainon this machinenpx knip --workspace apps/desktop/packages/uinpm test --workspace @maka/runtime-hostauthenticated-websocket) that reproduce identically onmainon this machine;main's CI is green, so these are local-environmentThe real-session case laziness alone cannot cover stays covered by structure: the Runtime Host that owns the database is a child process spawned with
stdio: 'ignore'(packages/runtime-host/src/client/launcher.ts), andpackages/cli/srcopens no database itself.Remaining warning sites
The paths that still emit the warning all genuinely use SQLite, so the warning is expected there:
maka evalandmaka activate— including their--help, since the command module loads before argument handling;@maka/storage;Both existing warning suppressors (
loadDatabaseSync,loadSqliteModule) are onmaintoday and survive this PR unchanged.Compatibility — maintainer sign-off wanted
Two deliberate contract changes, called out for an explicit decision rather than buried in the diff:
.(andmain/types) is a breaking change to the package's public surface.@maka/storageis not published independently and every consumer lives in this repository, so the blast radius today is zero — but if the package is ever published on its own, this belongs in the release notes.artifact-attachments,operational-state-backup,plan-store,provider-request-capture-artifact,session-bundle-canonical-tree,session-bundle-contract,session-bundle-file-service,session-bundle-manifest,session-bundle-ustar,sqlite-artifact-metadata,sqlite-usage-store,task-ledger-store,telemetry-repo. Nothing outside the package imports them; re-publishing any of them later is a one-lineexportsaddition.createSqliteArtifactStoreWriteAuthority,migrateOperationalStateDatabaseInternal,inspectOperationalStateSchema,OperationalStateMigrationBlockedError,withCredentialFileLock. The reachable-symbol union test names each one.Generative tooling disclosure
Claude Code (Claude Fable 5) made a substantive contribution: it traced the regression to the barrel, performed the mechanical rewrite, wrote the regression tests, ran the measurements, applied the review follow-ups and drafted this description. Commits carry the
Generated-by: Claude Codetrailer.A human contributor of record has reviewed the diff, verified the evidence above and decided to submit it.