You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "childrentime (via GitHub)" <gi...@apache.org> on 2026/08/20 09:25:49 UTC
[PR] refactor(storage): drop the barrel and publish narrow entrypoints [maka]
childrentime opened a new pull request, #3301:
URL: https://github.com/apache/maka/pull/3301
## Summary
`maka --help` printed Node's SQLite `ExperimentalWarning` to stderr, on a command that never opens a database. Every other CLI entry did the same. The cause is structural rather than local, so this removes the structure instead of muffling the symptom: `@maka/storage` no longer publishes a barrel.
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:
```ts
import { DatabaseSync } from 'node:sqlite'; // evaluated here
```
Exactly three modules do this: `operational-target-schema.ts`, `operational-state-backup.ts` and `session-bundle-policy.ts`.
`operational-target-schema.ts` is the amplifier. `operational-state-store.ts` imports it — and `operational-state-store.ts` is imported by roughly forty modules, which is how three import statements reached **45 of the package's 110 modules and 24 of the barrel's 43 export entries**. `operational-state-store.ts` already loaded SQLite lazily; it was pulled under anyway by the module it imports.
The last link is the CLI. `cli-core.ts` imports `@maka/storage` to reach `resolveMakaDataRoots`, a pure path helper. The barrel's `export *` surface hands it the whole storage layer, SQLite included, before `--help` prints a single line.
## Change
- Delete `packages/storage/src/index.ts` (159 lines) and drop `.` from the `exports` map.
- Publish the 21 modules consumers actually reached through the barrel as narrow subpaths, for 52 entrypoints total.
- Rewrite 87 files onto those subpaths. The 20 touched files in `packages/runtime` are all tests; that package's production code is untouched.
- Two tests moved off the barrel's *shape* rather than its contents. `managed-workspace-baseline` asserted that internals were absent from the barrel object and now asserts that their modules are absent from the `exports` map — a stronger claim, since reachability is now decided there. `provider-request-capture-artifact` reaches its subject directly.
## Regression guard
`public-entrypoints.test.ts` imports all 52 published entrypoints concurrently (0.6s) and asserts:
- the package publishes no `.` entrypoint;
- the entrypoints that load `node:sqlite` are **exactly** the 29 listed in the test, leaving 23 that are free of it.
That list is the package's SQLite boundary written down. Widening it later means editing the list and saying why.
This matters because of how #1257 came back. Its guard, `package-import.test.ts`, asserted an empty stderr on the package entrypoint; #2710 removed it in a bulk test cleanup, #1994 then added two of the static imports and #2445 a third, and the warning shipped. A list of entrypoints is harder to delete without noticing than an assertion that something is empty.
## Validation
Node v24.11.1, macOS arm64. Before/after measured on the same source tree by checking `main` out, building, running, and switching back.
| Command | `main` | This branch |
| --- | --- | --- |
| `maka --version` | 169 B stderr | **0 B** |
| `maka --help` | 169 B stderr | **0 B** |
| `maka run --help` | 169 B stderr | **0 B** |
| `maka run <prompt>`, isolated profile | 169 B stderr | **0 B** |
The last row is the case laziness alone cannot cover, because a real session does open a database. It stays clean because the Runtime Host that owns the database is a child process spawned with `stdio: 'ignore'` (`packages/runtime-host/src/client/launcher.ts`), and `packages/cli/src` opens no database itself. In the isolated profile used for that row, the host created and initialised a 917 KB `runtime.sqlite` while the client's stderr stayed empty.
| Check | Result |
| --- | --- |
| `npm run build` | pass |
| `npm run typecheck` | pass |
| `npm run lint` | pass, 2452 files |
| `npm test --workspace maka-agent` | 315 tests, **315 pass** |
| `npm test --workspace @maka/runtime-host` | 1019 tests, **1019 pass** |
| `npm test --workspace @maka/storage` | 853 tests, 838 pass, 14 skipped, 1 pre-existing failure (identical on `main`) |
## Remaining warning sites
The paths that still emit the warning all genuinely use SQLite, so the warning is expected there:
- `maka eval` (snapshots result databases) and `maka activate` (validates session bundles against SQLite files) — including their `--help`, since the command module loads before argument handling;
- the 29 SQLite-backed entrypoints of `@maka/storage`;
- the processes that actually open databases (the Runtime Host child, the Electron main process).
## Compatibility
Dropping `.` is a breaking change for anything importing the bare specifier. `@maka/storage` is 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.
## Generative tooling disclosure
Claude Code (Claude Fable 5) made a substantive contribution: it traced the regression to the barrel, performed the mechanical rewrite, wrote `public-entrypoints.test.ts`, ran the before/after measurements, and drafted this description. Commits carry the `Generated-by: Claude Code` trailer.
A human contributor of record has reviewed the diff, verified the evidence above and decided to submit it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(storage): drop the barrel and publish narrow entrypoints [maka]
Posted by "M4n5ter (via GitHub)" <gi...@apache.org>.
M4n5ter commented on code in PR #3301:
URL: https://github.com/apache/maka/pull/3301#discussion_r3841791841
##########
packages/storage/package.json:
##########
@@ -4,43 +4,55 @@
"license": "Apache-2.0",
"private": true,
"type": "module",
- "main": "./dist/index.js",
- "types": "./dist/index.d.ts",
"exports": {
- ".": "./dist/index.js",
- "./credential-store": "./dist/credential-store.js",
- "./file-update-lock": "./dist/file-update-lock.js",
- "./process-lifetime-file-update-lock": "./dist/process-lifetime-file-update-lock.js",
- "./stable-storage": "./dist/stable-storage.js",
+ "./activation-secret-injector": "./dist/activation-secret-injector.js",
+ "./agent-graph-control-store": "./dist/agent-graph-control-store.js",
+ "./agent-run-store": "./dist/agent-run-store.js",
"./artifact-stores": "./dist/artifact-stores.js",
+ "./config-transfer": "./dist/config-transfer.js",
+ "./credential-store": "./dist/credential-store-public.js",
Review Comment:
<details open>
<summary>English</summary>
**[P2] Preserve or explicitly retire `withCredentialFileLock` from this existing entrypoint**
`./credential-store` was already a published subpath before this PR and mapped directly to `credential-store.js`. Importing `@maka/storage/credential-store` on the parent commit exposes `withCredentialFileLock` as a function; at this head the same import resolves through the new facade and the symbol is `undefined`. This contract change is independent of deleting the root barrel.
The new guards do not catch it: the reachable-symbol denylist expects this symbol to be absent, while the unconsumed-subpath guard checks path presence rather than symbol compatibility. Please either preserve the old target/symbol for this pre-existing entrypoint, or explicitly record and obtain agreement for retiring it as a compatibility change.
</details>
<details>
<summary>中文</summary>
**[P2] 保留或明确退役该既有入口中的 `withCredentialFileLock`**
`./credential-store` 在本 PR 之前已经是发布的子路径，并直接指向 `credential-store.js`。在父提交上导入 `@maka/storage/credential-store` 时，`withCredentialFileLock` 是函数；在当前提交上，同一导入经过新 facade 后该符号为 `undefined`。这项契约变化与删除根 barrel 无关。
现有守卫拦不住它：可达符号 denylist 本身要求该符号不存在，而未消费子路径守卫只检查路径是否存在，不检查符号兼容性。请保留这个既有入口原来的目标/符号，或者将其作为兼容性变更明确记录并取得维护者认可。
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(storage): drop the barrel and publish narrow entrypoints [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #3301:
URL: https://github.com/apache/maka/pull/3301#discussion_r3821492071
##########
packages/storage/package.json:
##########
@@ -4,41 +4,59 @@
"license": "Apache-2.0",
"private": true,
"type": "module",
- "main": "./dist/index.js",
- "types": "./dist/index.d.ts",
"exports": {
- ".": "./dist/index.js",
- "./credential-store": "./dist/credential-store.js",
- "./file-update-lock": "./dist/file-update-lock.js",
+ "./activation-secret-injector": "./dist/activation-secret-injector.js",
+ "./agent-graph-control-store": "./dist/agent-graph-control-store.js",
+ "./agent-run-store": "./dist/agent-run-store.js",
+ "./artifact-store": "./dist/artifact-store.js",
"./artifact-stores": "./dist/artifact-stores.js",
+ "./config-transfer": "./dist/config-transfer.js",
+ "./credential-store": "./dist/credential-store.js",
"./daily-review-authority": "./dist/daily-review-authority.js",
"./deep-research-authority": "./dist/deep-research-authority.js",
- "./goal-authority": "./dist/goal-authority.js",
- "./plan-authority": "./dist/plan-authority.js",
+ "./deep-research-store": "./dist/deep-research-store.js",
+ "./encrypted-file-managed-secret-store": "./dist/encrypted-file-managed-secret-store.js",
"./execution-stores": "./dist/execution-stores.js",
"./external-sessions": "./dist/external-sessions.js",
- "./agent-graph-control-store": "./dist/agent-graph-control-store.js",
- "./interaction-store": "./dist/interaction-store-public.js",
+ "./file-update-lock": "./dist/file-update-lock.js",
+ "./foreign-session-store": "./dist/foreign-session-store.js",
"./git-worktree-child-executor": "./dist/git-worktree-child-executor.js",
- "./memory-bundle-store": "./dist/memory-bundle-store.js",
- "./managed-workspace-owner": "./dist/managed-workspace-owner.js",
- "./managed-secret-store": "./dist/managed-secret-store.js",
- "./activation-secret-injector": "./dist/activation-secret-injector.js",
- "./encrypted-file-managed-secret-store": "./dist/encrypted-file-managed-secret-store.js",
+ "./goal-authority": "./dist/goal-authority.js",
+ "./interaction-store": "./dist/interaction-store-public.js",
"./long-term-memory-store": "./dist/long-term-memory-store.js",
+ "./managed-secret-store": "./dist/managed-secret-store.js",
+ "./managed-workspace-owner": "./dist/managed-workspace-owner.js",
+ "./mcp-config-store": "./dist/mcp-config-store.js",
+ "./memory-bundle-store": "./dist/memory-bundle-store.js",
+ "./model-call-ledger": "./dist/model-call-ledger.js",
+ "./operational-state-store": "./dist/operational-state-store.js",
Review Comment:
[P2] Narrow this entrypoint, or move the internals behind their owning module — publishing `operational-state-store.ts` wholesale is the one place where dropping the barrel is not surface-neutral. The deleted barrel re-exported a hand-picked four names plus two types from this module (`index.ts:131-140`: `acquireOperationalStateDatabase`, `OPERATIONAL_STATE_DATABASE_NAME`, `OPERATIONAL_STATE_SCHEMA_VERSION`, `resolveOperationalStateDatabasePath`). A whole-module subpath also publishes `migrateOperationalStateDatabaseInternal` (`:407`), `inspectOperationalStateSchema` (`:279`) and `OperationalStateMigrationBlockedError` (`:128`), none of which any entrypoint reached before — the migration function runs `BEGIN IMMEDIATE` and applies five schema migrations against a caller-supplied `DatabaseSync`, and its only in-repo caller is line 260 of its own module. The same shape with a sharper edge sits at `./artifact-store`: the barrel exported six names and deliberately withheld `create
SqliteArtifactStoreWriteAuthority` (`artifact-store.ts:231`), whose only caller, `artifact-stores.ts:78`, binds it to a storage-root lease via `assertAuthority` and `leaseBoundWriterLockAuthority`. Both of those options are optional, so an external consumer importing `@maka/storage/artifact-store` can now construct an artifact write authority with no lease at all — exactly the gate `artifact-stores.ts` exists to enforce. Evidence: confirmed by reading code at this head; I diffed the barrel's named export lists against the modules' actual exports. Regression test: extend the reachable-symbol union in `managed-workspace-baseline.test.ts:74-85`, which already loads all 53 entrypoints and has the right shape, to include `migrateOperationalStateDatabaseInternal` and `createSqliteArtifactStoreWriteAuthority` — so any future widening has to be declared rather than inherited.
##########
packages/storage/package.json:
##########
@@ -4,41 +4,59 @@
"license": "Apache-2.0",
"private": true,
"type": "module",
- "main": "./dist/index.js",
- "types": "./dist/index.d.ts",
"exports": {
- ".": "./dist/index.js",
- "./credential-store": "./dist/credential-store.js",
- "./file-update-lock": "./dist/file-update-lock.js",
+ "./activation-secret-injector": "./dist/activation-secret-injector.js",
+ "./agent-graph-control-store": "./dist/agent-graph-control-store.js",
+ "./agent-run-store": "./dist/agent-run-store.js",
+ "./artifact-store": "./dist/artifact-store.js",
"./artifact-stores": "./dist/artifact-stores.js",
+ "./config-transfer": "./dist/config-transfer.js",
+ "./credential-store": "./dist/credential-store.js",
"./daily-review-authority": "./dist/daily-review-authority.js",
"./deep-research-authority": "./dist/deep-research-authority.js",
- "./goal-authority": "./dist/goal-authority.js",
- "./plan-authority": "./dist/plan-authority.js",
+ "./deep-research-store": "./dist/deep-research-store.js",
+ "./encrypted-file-managed-secret-store": "./dist/encrypted-file-managed-secret-store.js",
"./execution-stores": "./dist/execution-stores.js",
"./external-sessions": "./dist/external-sessions.js",
- "./agent-graph-control-store": "./dist/agent-graph-control-store.js",
- "./interaction-store": "./dist/interaction-store-public.js",
+ "./file-update-lock": "./dist/file-update-lock.js",
+ "./foreign-session-store": "./dist/foreign-session-store.js",
"./git-worktree-child-executor": "./dist/git-worktree-child-executor.js",
- "./memory-bundle-store": "./dist/memory-bundle-store.js",
- "./managed-workspace-owner": "./dist/managed-workspace-owner.js",
- "./managed-secret-store": "./dist/managed-secret-store.js",
- "./activation-secret-injector": "./dist/activation-secret-injector.js",
- "./encrypted-file-managed-secret-store": "./dist/encrypted-file-managed-secret-store.js",
+ "./goal-authority": "./dist/goal-authority.js",
+ "./interaction-store": "./dist/interaction-store-public.js",
"./long-term-memory-store": "./dist/long-term-memory-store.js",
+ "./managed-secret-store": "./dist/managed-secret-store.js",
+ "./managed-workspace-owner": "./dist/managed-workspace-owner.js",
+ "./mcp-config-store": "./dist/mcp-config-store.js",
+ "./memory-bundle-store": "./dist/memory-bundle-store.js",
+ "./model-call-ledger": "./dist/model-call-ledger.js",
+ "./operational-state-store": "./dist/operational-state-store.js",
"./pet-pack-store": "./dist/pet-pack-store.js",
+ "./plan-authority": "./dist/plan-authority.js",
+ "./project-catalog": "./dist/project-catalog.js",
+ "./project-catalog-authority": "./dist/project-catalog-authority.js",
"./root-authority": "./dist/root-authority.js",
- "./state-root-composition": "./dist/state-root-composition.js",
- "./storage-writer-composition": "./dist/storage-writer-composition.js",
+ "./runtime-event-persistence": "./dist/runtime-event-persistence.js",
"./runtime-policy-stores": "./dist/runtime-policy-stores.js",
"./scheduled-task-store": "./dist/scheduled-task-store.js",
+ "./session-bundle-canonical-tree": "./dist/session-bundle-canonical-tree.js",
+ "./session-bundle-contract": "./dist/session-bundle-contract.js",
+ "./session-bundle-file-service": "./dist/session-bundle-file-service.js",
+ "./session-bundle-manifest": "./dist/session-bundle-manifest.js",
+ "./session-bundle-policy": "./dist/session-bundle-policy.js",
+ "./session-bundle-ustar": "./dist/session-bundle-ustar.js",
Review Comment:
[P2] Drop the five `session-bundle-*` entrypoints that no consumer reaches — they invert the narrowing this PR is executing. `./session-bundle-canonical-tree`, `./session-bundle-contract`, `./session-bundle-file-service`, `./session-bundle-manifest` and `./session-bundle-ustar` are all added here and imported by zero files outside `packages/storage`; the package's own tests reach them by relative specifier, as the rewritten fixtures in this diff show. `./session-bundle-policy` has exactly one external importer and is the only one of the six that is earned. This is the same rule the PR itself applies to demote other modules to package-private, just not applied here, and each published subpath is a compatibility promise from here on. Nothing will ever flag them, since Knip runs only over `apps/desktop` and `packages/ui`. Evidence: confirmed by reading code at this head — I counted external importers per subpath across the branch. Regression test: assert in `public-entrypoints.te
st.ts` that every published subpath has at least one importer outside `packages/storage`. That would also surface the five consumer-less entrypoints that predate this PR (`write-queue`, `work-board-store`, `managed-secret-store`, `activation-secret-injector`, `encrypted-file-managed-secret-store`), which are not this PR's problem but are the same class.
##########
scripts/release-cli-file-policy.test.mjs:
##########
@@ -54,3 +56,37 @@ describe('CLI release file policy', () => {
assert.equal(isThirdPartyDevelopmentArtifact('dist/test-only/index.js'), false);
});
});
+
+describe('CLI release smoke script imports', () => {
+ // importInstalled() loads built files by path, so neither the export maps
+ // nor the typechecker notice when a target module stops being emitted. The
+ // dist trees this asserts against are kept fresh by check:stale, which runs
+ // ahead of this suite in check:release.
+ test('every workspace module the smoke script imports by path still exists', () => {
+ const repoRoot = resolve(import.meta.dirname, '..');
+ const smokeScript = readFileSync(
+ join(repoRoot, 'scripts/smoke-release-cli-package.mjs'),
+ 'utf8',
+ );
+ const workspaceDirByPackageName = new Map(
+ readdirSync(join(repoRoot, 'packages')).flatMap((directory) => {
+ const manifestPath = join(repoRoot, 'packages', directory, 'package.json');
+ if (!existsSync(manifestPath)) return [];
+ return [[JSON.parse(readFileSync(manifestPath, 'utf8')).name, directory]];
+ }),
+ );
+ const literals = [
+ ...smokeScript.matchAll(/'node_modules\/(@maka\/[^/']+)\/(dist\/[^']+\.js)'/gu),
Review Comment:
[P1] Widen this scan beyond the smoke script — one file it does not cover has a reach-in that this PR breaks. `scripts/computer-use/real-model.mjs:13` still reads `import { createSqliteAgentRunStore } from '../../packages/storage/dist/index.js';`. Deleting `packages/storage/src/index.ts` (removed here, -160) means `tsc` stops emitting `dist/index.js`, so that import throws `ERR_MODULE_NOT_FOUND` at module load and `npm run computer-use -- real-model` — the command `docs/computer-use-evidence-classes.md:53` documents for producing real-model provider evidence — no longer starts. Lines 17 and 18 of the same file already use the surviving deep form, so the author of that file understood the pattern; line 13 is the one that was missed. The correct target is `dist/agent-run-store.js` (`packages/storage/src/agent-run-store.ts:265`). Nothing in the tree catches it: it is a relative path so the `exports` map is bypassed, it is `.mjs` so no tsconfig typechecks it, Knip runs only over
`apps/desktop` and `packages/ui`, the new bare-specifier guard in `public-entrypoints.test.ts` matches only the literal `@maka/storage`, and this scan reads exactly one file. Evidence: confirmed by reading code at this head — I grepped the whole repo for `packages/storage/dist/` and this is the only surviving reach-in to a deleted emit. Regression test: replace the single-file read here with a walk over `scripts/**/*.mjs` collecting every `packages/*/dist/*.js` literal as well as the `node_modules/@maka/*/dist/*.js` form, asserting each target exists — that one change covers this and the smoke-script case with the same code.
##########
packages/storage/src/__tests__/public-entrypoints.test.ts:
##########
@@ -0,0 +1,159 @@
+import assert from 'node:assert/strict';
+import { execFile } from 'node:child_process';
+import { existsSync } from 'node:fs';
+import { readdir, readFile } from 'node:fs/promises';
+import { dirname, join, resolve } from 'node:path';
+import { fileURLToPath, pathToFileURL } from 'node:url';
+import { promisify } from 'node:util';
+import { test } from 'node:test';
+
+const run = promisify(execFile);
+const packageRoot = resolve(dirname(fileURLToPath(import.meta.url)), '../..');
+
+/**
+ * Entrypoints whose module graph reaches `node:sqlite` at load time, so
+ * importing one emits Node's SQLite ExperimentalWarning.
+ *
+ * This list is the package's SQLite boundary, stated out loud. `@maka/storage`
+ * publishes no barrel: a consumer that needs a durable store imports the entry
+ * that owns it and accepts the warning, and a consumer that needs
+ * `workspace-root` or `credential-store` pays nothing. Issue #1257 came from
+ * the opposite arrangement, where one `export *` barrel made every consumer —
+ * `maka --help` included — load SQLite.
+ *
+ * Adding an entry here is a deliberate widening of that boundary. Removing one
+ * means an entrypoint became SQLite-free. Either way, update this list in the
+ * same change and say why.
+ */
+const SQLITE_BACKED_ENTRYPOINTS = [
+ './agent-graph-control-store',
+ './agent-run-store',
+ './artifact-store',
+ './artifact-stores',
+ './daily-review-authority',
+ './deep-research-authority',
+ './deep-research-store',
+ './execution-stores',
+ './git-worktree-child-executor',
+ './goal-authority',
+ './interaction-store',
+ './managed-workspace-owner',
+ './model-call-ledger',
+ './operational-state-store',
+ './plan-authority',
+ './project-catalog',
+ './project-catalog-authority',
+ './runtime-event-persistence',
+ './scheduled-task-store',
+ './session-bundle-policy',
+ './session-store',
+ './settings-store',
+ './shell-run-authority',
+ './shell-run-store',
+ './sqlite-runtime-store',
+ './sqlite-session-metadata-store',
+ './storage-writer-composition',
+ './task-ledger-authority',
+ './usage-stores',
+ './work-board-store',
+];
+
+/**
+ * Loads an entrypoint in a child process and reports whether `node:sqlite`
+ * entered its module graph, observed through a `module.registerHooks` resolve
+ * hook. Matching Node's ExperimentalWarning text instead would tie this guard
+ * to a string Node owns and has already reworded once.
+ */
+async function loadsSqlite(target: string): Promise<boolean> {
+ const specifier = pathToFileURL(resolve(packageRoot, target)).href;
+ const probe = [
+ "import { registerHooks } from 'node:module';",
+ 'let sawSqlite = false;',
+ 'registerHooks({',
+ ' resolve(request, context, nextResolve) {',
+ ' const resolved = nextResolve(request, context);',
+ " if (resolved.url === 'node:sqlite') sawSqlite = true;",
+ ' return resolved;',
+ ' },',
+ '});',
+ `await import(${JSON.stringify(specifier)});`,
+ "process.stdout.write(sawSqlite ? '\\nSQLITE_IN_GRAPH=yes' : '\\nSQLITE_IN_GRAPH=no');",
+ ].join('\n');
+ const { stdout } = await run(process.execPath, ['--input-type=module', '--eval', probe], {
+ encoding: 'utf8',
+ });
+ const verdict = /SQLITE_IN_GRAPH=(yes|no)$/u.exec(stdout);
+ assert.ok(verdict, `probe for ${target} produced no verdict; stdout was: ${stdout}`);
+ return verdict[1] === 'yes';
+}
+
+async function publishedEntrypoints(): Promise<Record<string, string>> {
+ const manifest = JSON.parse(await readFile(resolve(packageRoot, 'package.json'), 'utf8')) as {
+ exports: Record<string, string>;
+ };
+ return manifest.exports;
+}
+
+test('the package publishes no barrel entrypoint', async () => {
+ const exports = await publishedEntrypoints();
+ assert.equal('.' in exports, false);
+ const manifest = JSON.parse(await readFile(resolve(packageRoot, 'package.json'), 'utf8')) as {
+ main?: string;
+ types?: string;
+ };
+ assert.equal(manifest.main, undefined, 'a `main` field would re-advertise the barrel');
+ assert.equal(manifest.types, undefined, 'a `types` field would re-advertise the barrel');
+});
+
+test('every published entrypoint target is emitted by the build', async () => {
+ const exports = await publishedEntrypoints();
+ for (const [subpath, target] of Object.entries(exports)) {
+ assert.ok(
+ existsSync(resolve(packageRoot, target)),
+ `"${subpath}" points at ${target}, which the build did not emit`,
+ );
+ }
+});
+
+test('no source file imports the retired bare specifier', { timeout: 60_000 }, async () => {
+ const repoRoot = resolve(packageRoot, '../..');
+ const barePattern = /(?:from\s*|import\s*\(\s*|require\s*\(\s*)['"]@maka\/storage['"]/u;
+ const sourceExtensions = /\.(?:ts|tsx|mts|cts|js|mjs|cjs)$/u;
+ const skipped = new Set(['node_modules', 'dist', '.git']);
+ const offenders: string[] = [];
+ async function walk(directory: string): Promise<void> {
+ const entries = await readdir(directory, { withFileTypes: true });
+ await Promise.all(
+ entries.map(async (entry) => {
+ if (skipped.has(entry.name)) return;
+ const path = join(directory, entry.name);
+ if (entry.isDirectory()) return walk(path);
+ if (!sourceExtensions.test(entry.name)) return;
+ if (barePattern.test(await readFile(path, 'utf8'))) offenders.push(path);
+ }),
+ );
+ }
+ await Promise.all(
+ ['packages', 'apps', 'scripts'].map((directory) => walk(join(repoRoot, directory))),
+ );
+ assert.deepEqual(
+ offenders,
+ [],
+ 'bare `@maka/storage` imports resolve to the removed `.` entrypoint and fail at runtime',
+ );
+});
+
+test('only the declared entrypoints load node:sqlite', { timeout: 120_000 }, async () => {
+ const exports = await publishedEntrypoints();
+ const results = await Promise.all(
Review Comment:
[P3] Batch these probes rather than spawning all 53 at once. `Promise.all` over `Object.entries(exports)` gives one unbounded `execFile` child per entrypoint under a 120s timeout, while `managed-workspace-baseline.test.ts:69` separately imports all 53 into the test process and the outer `node --test "dist/**/*.test.js"` is already running test files concurrently. AGENTS.md calls out this exact shape ("set `--test-concurrency` to at most 4"). This is a flake budget rather than a correctness bug — the assertions are right and the count is fixed — but it is a large instantaneous load on a CI runner for a test whose value does not depend on running in parallel. Evidence: the concurrency is confirmed by reading code at this head; the CI impact is inference, not measured. Regression test: none needed — batch in groups of 4-8 and the existing assertions still cover the same surface.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(storage): drop the barrel and publish narrow entrypoints [maka]
Posted by "childrentime (via GitHub)" <gi...@apache.org>.
childrentime commented on PR #3301:
URL: https://github.com/apache/maka/pull/3301#issuecomment-5355522591
@Astro-Han Thanks for the review — every finding was real. All six are addressed at 2d5c2cf, and I've resolved the threads accordingly:
- Rebased onto `main`; the writer-composition consumer #3295 added now imports through `@maka/storage/storage-writer-composition`, and `public-entrypoints.test.ts` asserts no source file in the repository imports the bare specifier — the test you suggested, and the one that would have caught this without a rebase.
- The smoke script's deep import points at `dist/workspace-root.js`, and `release-cli-file-policy.test.mjs` now walks every `importInstalled(..., 'node_modules/@maka/...')` literal and asserts the build emits the target. (Asserting against `src` twins turned out wrong in one case — `@maka/runtime`'s filesystem worker is emitted by a bundling script, not tsc — so the test checks `dist`, which `check:stale` keeps honest in `check:release`.)
- `main`/`types` are dropped, with a test asserting they stay absent and that every `exports` target is a file the build emits.
- The internal-symbol assertion now loads all 53 published entrypoints and checks the union of reachable symbols against the internals — your re-export scenario fails it.
- The SQLite boundary is observed through a `module.registerHooks` resolve hook; the probe errors loudly if it produces no verdict rather than falling through. `./storage-writer-composition` is entry 30 on the list — you were right that it belongs there, it statically imports `execution-stores` and thirteen other SQLite-backed modules.
- `npm run format` fixed both files; `format:check` is in the validation table now.
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 `.` removal and the eight modules going package-private are called out in the description under "maintainer sign-off wanted". If the maintainers prefer the minimal fix, I'll close this and submit the three-file version.
AI disclosure: these follow-ups were implemented with Claude Code (Claude Fable 5); I reviewed the diff and the validation results before pushing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(storage): drop the barrel and publish narrow entrypoints [maka]
Posted by "M4n5ter (via GitHub)" <gi...@apache.org>.
M4n5ter merged PR #3301:
URL: https://github.com/apache/maka/pull/3301
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(storage): drop the barrel and publish narrow entrypoints [maka]
Posted by "M4n5ter (via GitHub)" <gi...@apache.org>.
M4n5ter commented on code in PR #3301:
URL: https://github.com/apache/maka/pull/3301#discussion_r3841791841
##########
packages/storage/package.json:
##########
@@ -4,43 +4,55 @@
"license": "Apache-2.0",
"private": true,
"type": "module",
- "main": "./dist/index.js",
- "types": "./dist/index.d.ts",
"exports": {
- ".": "./dist/index.js",
- "./credential-store": "./dist/credential-store.js",
- "./file-update-lock": "./dist/file-update-lock.js",
- "./process-lifetime-file-update-lock": "./dist/process-lifetime-file-update-lock.js",
- "./stable-storage": "./dist/stable-storage.js",
+ "./activation-secret-injector": "./dist/activation-secret-injector.js",
+ "./agent-graph-control-store": "./dist/agent-graph-control-store.js",
+ "./agent-run-store": "./dist/agent-run-store.js",
"./artifact-stores": "./dist/artifact-stores.js",
+ "./config-transfer": "./dist/config-transfer.js",
+ "./credential-store": "./dist/credential-store-public.js",
Review Comment:
**[P2] Preserve or explicitly retire `withCredentialFileLock` from this existing entrypoint**
`./credential-store` was already a published subpath before this PR and mapped directly to `credential-store.js`. Importing `@maka/storage/credential-store` on the parent commit exposes `withCredentialFileLock` as a function; at this head the same import resolves through the new facade and the symbol is `undefined`. This contract change is independent of deleting the root barrel.
The new guards do not catch it: the reachable-symbol denylist expects this symbol to be absent, while the unconsumed-subpath guard checks path presence rather than symbol compatibility. Please either preserve the old target/symbol for this pre-existing entrypoint, or explicitly record and obtain agreement for retiring it as a compatibility change.
<details>
<summary>简体中文</summary>
**[P2] 保留或明确退役该既有入口中的 `withCredentialFileLock`**
`./credential-store` 在本 PR 之前已经是发布的子路径，并直接指向 `credential-store.js`。在父提交上导入 `@maka/storage/credential-store` 时，`withCredentialFileLock` 是函数；在当前提交上，同一导入经过新 facade 后该符号为 `undefined`。这项契约变化与删除根 barrel 无关。
现有守卫拦不住它：可达符号 denylist 本身要求该符号不存在，而未消费子路径守卫只检查路径是否存在，不检查符号兼容性。请保留这个既有入口原来的目标/符号，或者将其作为兼容性变更明确记录并取得维护者认可。
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(storage): drop the barrel and publish narrow entrypoints [maka]
Posted by "childrentime (via GitHub)" <gi...@apache.org>.
childrentime commented on PR #3301:
URL: https://github.com/apache/maka/pull/3301#issuecomment-5356052206
@Astro-Han All four findings and both of the no-thread notes are addressed at 049e93f:
- `real-model.mjs:13` points at `dist/agent-run-store.js`, and the file-policy scan now walks every script in `scripts/` for both import forms — installed `node_modules/@maka/*/dist` and relative `packages/*/dist` — asserting each target is still emitted. I verified the guard by reverting the fix locally: it fails on exactly that line.
- The five consumer-less `session-bundle-*` entrypoints are gone; `session-bundle-policy` stays. `public-entrypoints.test.ts` now asserts the exact set of published-but-unconsumed entrypoints, allowlisting only the five that predate this PR (your list matched my recount), and separately that every imported subpath is published. The bare-specifier guard also covers the side-effect form now.
- `operational-state-store`, `artifact-store` — and `credential-store` — publish facades re-exporting exactly the names the barrel picked, following the existing `interaction-store-public` idiom, and the reachable-symbol union test names all five withheld symbols. Credential-store is one you didn't flag: I re-derived the barrel-picks-vs-module-exports diff for every published hand-picked module, and `withCredentialFileLock` was the only other widened symbol — its own doc comment says it is deliberately not on the public surface. Everything else was surface-neutral.
- SQLite probes run at most four concurrent.
- The dead storybook path mapping is deleted (its `tsc -p tsconfig.storybook.json --noEmit` still passes), and knip over `apps/desktop` and `packages/ui` is clean.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(storage): drop the barrel and publish narrow entrypoints [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #3301:
URL: https://github.com/apache/maka/pull/3301#discussion_r3820874150
##########
packages/storage/package.json:
##########
@@ -7,37 +7,57 @@
"main": "./dist/index.js",
"types": "./dist/index.d.ts",
"exports": {
- ".": "./dist/index.js",
- "./credential-store": "./dist/credential-store.js",
- "./file-update-lock": "./dist/file-update-lock.js",
+ "./activation-secret-injector": "./dist/activation-secret-injector.js",
+ "./agent-graph-control-store": "./dist/agent-graph-control-store.js",
+ "./agent-run-store": "./dist/agent-run-store.js",
+ "./artifact-store": "./dist/artifact-store.js",
Review Comment:
[P1] This branch is stale against `main` in a way that merges cleanly and then fails to build. `#3295` landed on `main` about an hour ago and its consumer imports through the **bare** specifier: `packages/runtime-host/src/server/execution-composition.ts:60` is `import { openStorageWriterComposition } from '@maka/storage';`. That line is new on `main`, does not overlap anything this PR edits, and will therefore merge with no conflict marker anywhere near it — while this PR removes the `.` entry it depends on. The result is `@maka/runtime-host` failing with `ERR_PACKAGE_PATH_NOT_EXPORTED`, from a merge that looked clean. Worth knowing that `main`'s export map already carries `./storage-writer-composition`, so the fix on this side is a one-line specifier change, and the visible conflicts in `packages/storage/package.json` and the `src/index.ts` modify/delete are the ones that will draw attention — not this. Confirmed by reading `execution-composition.ts` and `packages/storage/pac
kage.json` on `main` at `a6f33c9`. Also check whether `SQLITE_BACKED_ENTRYPOINTS` needs `storage-writer-composition` added, since it statically imports `./execution-stores.js`. Regression test, and the one that would have caught this without a rebase: assert in `public-entrypoints.test.ts` that no source file in the repository imports or re-exports the bare `@maka/storage` specifier.
##########
packages/storage/package.json:
##########
@@ -7,37 +7,57 @@
"main": "./dist/index.js",
"types": "./dist/index.d.ts",
"exports": {
- ".": "./dist/index.js",
- "./credential-store": "./dist/credential-store.js",
- "./file-update-lock": "./dist/file-update-lock.js",
+ "./activation-secret-injector": "./dist/activation-secret-injector.js",
Review Comment:
[P1] Removing `src/index.ts` breaks `scripts/smoke-release-cli-package.mjs`, which reaches past the export map. Line 706 of that script does `importInstalled(packageRoot, 'node_modules/@maka/storage/dist/index.js')` — a raw `import(pathToFileURL(...))` on a file path, so neither the `exports` map nor typecheck protects it, and deleting `src/index.ts` means `tsc` stops emitting `dist/index.js`. It is called from two scenarios in that script, and the CI path is closed: `scripts/ci-test-plan.mjs` lists `packages/storage` in `CLI_PACKAGE_WORKSPACES`, which sets `cli_package=true`, which runs `npm run release:cli:smoke`. `resolveMakaDataRoots` now lives in `workspace-root.ts`, so the fix is to point that import at `dist/workspace-root.js` — note lines 474, 713 and 780 of the same script already use exactly that deep form. Confirmed by reading the script on `main` and the file removal in this PR; a subagent reports building the workspace and getting `ERR_MODULE_NOT_FOUND` on the res
ulting `dist/index.js`. Regression test: extend `scripts/release-cli-file-policy.test.mjs` to walk every `importInstalled(..., 'node_modules/...')` literal in the smoke script and assert the target exists under `packages/*/dist`, since this class of import is protected by neither the export map nor the typechecker.
##########
packages/storage/src/__tests__/public-entrypoints.test.ts:
##########
@@ -0,0 +1,91 @@
+import assert from 'node:assert/strict';
+import { execFile } from 'node:child_process';
+import { readFile } from 'node:fs/promises';
+import { dirname, resolve } from 'node:path';
+import { fileURLToPath, pathToFileURL } from 'node:url';
+import { promisify } from 'node:util';
+import { test } from 'node:test';
+
+const run = promisify(execFile);
+const packageRoot = resolve(dirname(fileURLToPath(import.meta.url)), '../..');
+
+/**
+ * Entrypoints whose module graph reaches `node:sqlite` at load time, so
+ * importing one emits Node's SQLite ExperimentalWarning.
+ *
+ * This list is the package's SQLite boundary, stated out loud. `@maka/storage`
+ * publishes no barrel: a consumer that needs a durable store imports the entry
+ * that owns it and accepts the warning, and a consumer that needs
+ * `workspace-root` or `credential-store` pays nothing. Issue #1257 came from
+ * the opposite arrangement, where one `export *` barrel made every consumer —
+ * `maka --help` included — load SQLite.
+ *
+ * Adding an entry here is a deliberate widening of that boundary. Removing one
+ * means an entrypoint became SQLite-free. Either way, update this list in the
+ * same change and say why.
+ */
+const SQLITE_BACKED_ENTRYPOINTS = [
+ './agent-graph-control-store',
+ './agent-run-store',
+ './artifact-store',
+ './artifact-stores',
+ './daily-review-authority',
+ './deep-research-authority',
+ './deep-research-store',
+ './execution-stores',
+ './git-worktree-child-executor',
+ './goal-authority',
+ './interaction-store',
+ './managed-workspace-owner',
+ './model-call-ledger',
+ './operational-state-store',
+ './plan-authority',
+ './project-catalog',
+ './project-catalog-authority',
+ './runtime-event-persistence',
+ './scheduled-task-store',
+ './session-bundle-policy',
+ './session-store',
+ './settings-store',
+ './shell-run-authority',
+ './shell-run-store',
+ './sqlite-runtime-store',
+ './sqlite-session-metadata-store',
+ './task-ledger-authority',
+ './usage-stores',
+ './work-board-store',
+];
+
+async function loadsSqlite(target: string): Promise<boolean> {
+ const specifier = pathToFileURL(resolve(packageRoot, target)).href;
+ const { stderr } = await run(
+ process.execPath,
+ ['--input-type=module', '--eval', `await import(${JSON.stringify(specifier)})`],
+ { encoding: 'utf8' },
+ );
+ return stderr.includes('SQLite is an experimental feature');
Review Comment:
[P3] Do not detect the SQLite boundary by matching Node's warning text. `stderr.includes('SQLite is an experimental feature')` ties this guard to a string Node owns and has already changed: a subagent reports running this test on Node 26.7.0 and getting `actual: []` — all 29 entries fall through and the test fails outright. `engines` allows `>=22.19.0` and CI runs 24, so this is green today and red for any contributor on a newer Node, and it will silently stop guarding anything the day CI bumps. A `module.registerHooks` resolve hook that observes whether `node:sqlite` enters the module graph is version-independent and tests the actual property; the subagent reports that recomputing the set that way reproduces your 29 entries exactly, which is a good sign for the list itself. Worth fixing here because this test is the entire enforcement mechanism for the boundary this PR exists to establish.
##########
packages/storage/package.json:
##########
@@ -7,37 +7,57 @@
"main": "./dist/index.js",
"types": "./dist/index.d.ts",
"exports": {
- ".": "./dist/index.js",
- "./credential-store": "./dist/credential-store.js",
- "./file-update-lock": "./dist/file-update-lock.js",
+ "./activation-secret-injector": "./dist/activation-secret-injector.js",
+ "./agent-graph-control-store": "./dist/agent-graph-control-store.js",
+ "./agent-run-store": "./dist/agent-run-store.js",
+ "./artifact-store": "./dist/artifact-store.js",
"./artifact-stores": "./dist/artifact-stores.js",
+ "./config-transfer": "./dist/config-transfer.js",
+ "./credential-store": "./dist/credential-store.js",
"./daily-review-authority": "./dist/daily-review-authority.js",
"./deep-research-authority": "./dist/deep-research-authority.js",
- "./goal-authority": "./dist/goal-authority.js",
- "./plan-authority": "./dist/plan-authority.js",
+ "./deep-research-store": "./dist/deep-research-store.js",
+ "./encrypted-file-managed-secret-store": "./dist/encrypted-file-managed-secret-store.js",
"./execution-stores": "./dist/execution-stores.js",
"./external-sessions": "./dist/external-sessions.js",
- "./agent-graph-control-store": "./dist/agent-graph-control-store.js",
- "./interaction-store": "./dist/interaction-store-public.js",
+ "./file-update-lock": "./dist/file-update-lock.js",
+ "./foreign-session-store": "./dist/foreign-session-store.js",
"./git-worktree-child-executor": "./dist/git-worktree-child-executor.js",
- "./memory-bundle-store": "./dist/memory-bundle-store.js",
- "./managed-workspace-owner": "./dist/managed-workspace-owner.js",
- "./managed-secret-store": "./dist/managed-secret-store.js",
- "./activation-secret-injector": "./dist/activation-secret-injector.js",
- "./encrypted-file-managed-secret-store": "./dist/encrypted-file-managed-secret-store.js",
+ "./goal-authority": "./dist/goal-authority.js",
+ "./interaction-store": "./dist/interaction-store-public.js",
"./long-term-memory-store": "./dist/long-term-memory-store.js",
+ "./managed-secret-store": "./dist/managed-secret-store.js",
+ "./managed-workspace-owner": "./dist/managed-workspace-owner.js",
+ "./mcp-config-store": "./dist/mcp-config-store.js",
+ "./memory-bundle-store": "./dist/memory-bundle-store.js",
+ "./model-call-ledger": "./dist/model-call-ledger.js",
+ "./operational-state-store": "./dist/operational-state-store.js",
"./pet-pack-store": "./dist/pet-pack-store.js",
+ "./plan-authority": "./dist/plan-authority.js",
+ "./project-catalog": "./dist/project-catalog.js",
+ "./project-catalog-authority": "./dist/project-catalog-authority.js",
"./root-authority": "./dist/root-authority.js",
- "./state-root-composition": "./dist/state-root-composition.js",
+ "./runtime-event-persistence": "./dist/runtime-event-persistence.js",
"./runtime-policy-stores": "./dist/runtime-policy-stores.js",
"./scheduled-task-store": "./dist/scheduled-task-store.js",
+ "./session-bundle-canonical-tree": "./dist/session-bundle-canonical-tree.js",
+ "./session-bundle-contract": "./dist/session-bundle-contract.js",
+ "./session-bundle-file-service": "./dist/session-bundle-file-service.js",
+ "./session-bundle-manifest": "./dist/session-bundle-manifest.js",
+ "./session-bundle-policy": "./dist/session-bundle-policy.js",
+ "./session-bundle-ustar": "./dist/session-bundle-ustar.js",
+ "./session-store": "./dist/session-store.js",
"./settings-store": "./dist/settings-store.js",
"./shell-run-authority": "./dist/shell-run-authority.js",
+ "./shell-run-store": "./dist/shell-run-store.js",
+ "./sqlite-runtime-store": "./dist/sqlite-runtime-store.js",
+ "./sqlite-session-metadata-store": "./dist/sqlite-session-metadata-store.js",
+ "./state-root-composition": "./dist/state-root-composition.js",
"./task-ledger-authority": "./dist/task-ledger-authority.js",
- "./work-board-store": "./dist/work-board-store.js",
- "./model-call-ledger": "./dist/model-call-ledger.js",
"./usage-stores": "./dist/usage-stores.js",
+ "./work-board-store": "./dist/work-board-store.js",
"./workspace-identity": "./dist/workspace-identity.js",
+ "./workspace-root": "./dist/workspace-root.js",
Review Comment:
[P2] Drop `main` and `types`, or repoint them — they still name a file this PR stops emitting. `"main": "./dist/index.js"` and `"types": "./dist/index.d.ts"` survive the removal of the `.` export. Nothing in this repository breaks, because Node prefers `exports` and every internal consumer resolves through it. The exposure is at publish time: `scripts/release-cli-package.mjs`'s `copyInternalPackage` lists `main` in `allowedFields` and copies it verbatim into the released manifest, so the published `@maka/storage` advertises an entry point that does not exist, and any consumer that does not honour `exports` — an older bundler, some editor resolvers — fails to resolve the package at all. Since the whole point of this PR is that the package's public surface should be exactly what it says, a dangling `main` is the one field that contradicts it. Confirmed by reading the manifest at this head and the release script on `main`. Regression test, which also covers the finding above: a
ssert in `public-entrypoints.test.ts` that every `main`, `types` and `exports` target named by the manifest exists in `dist` after a build.
##########
packages/storage/src/__tests__/public-entrypoints.test.ts:
##########
@@ -0,0 +1,91 @@
+import assert from 'node:assert/strict';
+import { execFile } from 'node:child_process';
+import { readFile } from 'node:fs/promises';
+import { dirname, resolve } from 'node:path';
+import { fileURLToPath, pathToFileURL } from 'node:url';
+import { promisify } from 'node:util';
+import { test } from 'node:test';
+
+const run = promisify(execFile);
+const packageRoot = resolve(dirname(fileURLToPath(import.meta.url)), '../..');
+
+/**
+ * Entrypoints whose module graph reaches `node:sqlite` at load time, so
+ * importing one emits Node's SQLite ExperimentalWarning.
+ *
+ * This list is the package's SQLite boundary, stated out loud. `@maka/storage`
+ * publishes no barrel: a consumer that needs a durable store imports the entry
+ * that owns it and accepts the warning, and a consumer that needs
+ * `workspace-root` or `credential-store` pays nothing. Issue #1257 came from
+ * the opposite arrangement, where one `export *` barrel made every consumer —
+ * `maka --help` included — load SQLite.
+ *
+ * Adding an entry here is a deliberate widening of that boundary. Removing one
+ * means an entrypoint became SQLite-free. Either way, update this list in the
+ * same change and say why.
+ */
+const SQLITE_BACKED_ENTRYPOINTS = [
+ './agent-graph-control-store',
+ './agent-run-store',
+ './artifact-store',
+ './artifact-stores',
+ './daily-review-authority',
+ './deep-research-authority',
+ './deep-research-store',
+ './execution-stores',
+ './git-worktree-child-executor',
+ './goal-authority',
+ './interaction-store',
+ './managed-workspace-owner',
+ './model-call-ledger',
+ './operational-state-store',
+ './plan-authority',
+ './project-catalog',
+ './project-catalog-authority',
+ './runtime-event-persistence',
+ './scheduled-task-store',
+ './session-bundle-policy',
+ './session-store',
+ './settings-store',
+ './shell-run-authority',
+ './shell-run-store',
+ './sqlite-runtime-store',
+ './sqlite-session-metadata-store',
+ './task-ledger-authority',
+ './usage-stores',
+ './work-board-store',
+];
+
+async function loadsSqlite(target: string): Promise<boolean> {
+ const specifier = pathToFileURL(resolve(packageRoot, target)).href;
+ const { stderr } = await run(
+ process.execPath,
+ ['--input-type=module', '--eval', `await import(${JSON.stringify(specifier)})`],
+ { encoding: 'utf8' },
+ );
+ return stderr.includes('SQLite is an experimental feature');
+}
+
+async function publishedEntrypoints(): Promise<Record<string, string>> {
+ const manifest = JSON.parse(await readFile(resolve(packageRoot, 'package.json'), 'utf8')) as {
+ exports: Record<string, string>;
+ };
+ return manifest.exports;
+}
+
+test('the package publishes no barrel entrypoint', async () => {
+ const exports = await publishedEntrypoints();
+ assert.equal('.' in exports, false);
+});
+
+test('only the declared entrypoints load node:sqlite', { timeout: 120_000 }, async () => {
+ const exports = await publishedEntrypoints();
+ const results = await Promise.all(
+ Object.entries(exports).map(async ([subpath, target]) => ({
+ subpath,
+ sqlite: await loadsSqlite(target),
+ })),
+ );
+ const actual = results.filter((entry) => entry.sqlite).map((entry) => entry.subpath).sort();
Review Comment:
[P1] `npm run format:check` fails on this line, and on `provider-request-capture-artifact.test.ts:13`. Biome wants this chain broken across lines — `const actual = results` then `.filter(...)`, `.map(...)`, `.sort()` each on their own — and wants the `Reflect.get(...)` call in the other file split across three argument lines. `.github/workflows/ci.yml` runs `format:check` whenever `code == 'true'`, which this PR obviously satisfies. Reproduced by execution: I ran this repository's pinned `@biomejs/biome@2.5.6` over both files at `0124d97` via `--stdin-file-path` and diffed its output against the committed bytes; these are the only two differences in either file. The PR's verification table lists `npm run lint` — which I also ran clean — but not `format:check`, and AGENTS.md calls out exactly that gap: workspace typecheck and lint alone do not cover CI's format check. `npm run format` fixes both.
##########
packages/storage/src/__tests__/managed-workspace-baseline.test.ts:
##########
@@ -61,11 +60,18 @@ test('does not expose baseline receipt issuance on the public Git workspace serv
});
test('does not expose artifact-only workspace creation through the public storage API', async () => {
- assert.equal('createGitWorkspaceService' in publicStorage, false);
- assert.equal('issueManagedWorkspaceExecutionHandleInternal' in publicStorage, false);
- assert.equal('inspectManagedWorkspaceExecutionHandleInternal' in publicStorage, false);
- assert.equal('issueManagedWorkspaceExecutionScopeInternal' in publicStorage, false);
- assert.equal('inspectManagedWorkspaceExecutionScopeInternal' in publicStorage, false);
+ // The package has no barrel: reachability is decided by the `exports` map.
+ // These internals live in modules it does not publish, so no consumer can
+ // import them at all.
+ const { readFile } = await import('node:fs/promises');
+ const manifest = JSON.parse(
+ await readFile(new URL('../../package.json', import.meta.url), 'utf8'),
+ ) as { exports: Record<string, string> };
+ const published = new Set(Object.values(manifest.exports));
+ assert.equal(published.has('./dist/git-workspace-service.js'), false);
Review Comment:
[P2] This assertion is weaker than the one it replaces, not stronger. The previous check asked whether the internal symbol was reachable on the barrel object; this one asks whether the module *file* appears as a target in the export map. Those differ precisely where it matters: if any published entrypoint later gains `export * from './git-workspace-service.js'`, the file still never appears as a target, this assertion still passes, and the internal symbol is public again. The PR body describes it as a stronger claim, which holds for direct exposure but not for re-export — and re-export is the failure mode a barrel-removal PR is most likely to reintroduce. No leak exists today; a subagent reports walking the transitive re-export graph of all 52 entrypoints and finding all three internal modules unreachable. The stronger form is to `import()` each published entrypoint and assert the union of their named exports excludes these symbols — asserting on reachable symbols rather than
on module paths.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(storage): drop the barrel and publish narrow entrypoints [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #3301:
URL: https://github.com/apache/maka/pull/3301#issuecomment-5384765129
Hi — this PR **conflicts with current `main`** and cannot be merged as-is.
I tested a rebase onto current `main` locally (in a throwaway worktree — your branch was not touched). It stops on these files:
- `apps/desktop/src/main/client-settings-effects.ts`
- `apps/desktop/src/main/mcp-ipc-main.ts`
- `apps/desktop/src/main/new-session-project.ts`
- `apps/desktop/src/main/runtime-host-boot.ts`
- `…(more)`
These are real source conflicts, so they need your judgement rather than a mechanical rebase — please rebase onto current `main` and resolve them yourself, then push. Once the branch is conflict-free and CI is green on the new head, I will pick it up for review.
```
git fetch upstream && git rebase upstream/main
# resolve, then
git push --force-with-lease
```
Thanks for the contribution — happy to help if any conflict is unclear.
---
*AI-assisted maintenance note, not a review. It does not count as the required human review under `CONTRIBUTING.md` §Review.*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(storage): drop the barrel and publish narrow entrypoints [maka]
Posted by "childrentime (via GitHub)" <gi...@apache.org>.
childrentime commented on code in PR #3301:
URL: https://github.com/apache/maka/pull/3301#discussion_r3841914497
##########
packages/storage/package.json:
##########
@@ -4,43 +4,55 @@
"license": "Apache-2.0",
"private": true,
"type": "module",
- "main": "./dist/index.js",
- "types": "./dist/index.d.ts",
"exports": {
- ".": "./dist/index.js",
- "./credential-store": "./dist/credential-store.js",
- "./file-update-lock": "./dist/file-update-lock.js",
- "./process-lifetime-file-update-lock": "./dist/process-lifetime-file-update-lock.js",
- "./stable-storage": "./dist/stable-storage.js",
+ "./activation-secret-injector": "./dist/activation-secret-injector.js",
+ "./agent-graph-control-store": "./dist/agent-graph-control-store.js",
+ "./agent-run-store": "./dist/agent-run-store.js",
"./artifact-stores": "./dist/artifact-stores.js",
+ "./config-transfer": "./dist/config-transfer.js",
+ "./credential-store": "./dist/credential-store-public.js",
Review Comment:
Fixed at `a2c64e3` — preserved rather than retired.
You're right that this is independent of removing the barrel, and my description had it backwards: I listed `withCredentialFileLock` among "symbols the barrel deliberately withheld", but the barrel is not what made it reachable. `./credential-store` published the whole module and predates this PR, so the symbol was public regardless of what the barrel picked.
The facade was applying the barrel's picks to an entrypoint the barrel never owned. That rule belongs to the subpaths this branch publishes for the first time, where the surface is still being chosen; it is not a reason to narrow one that already shipped. So `./credential-store` points back at `credential-store.js`, the facade is deleted, and `withCredentialFileLock` also leaves the reachable-symbol denylist in `managed-workspace-baseline`, since it is publicly reachable again. Verified at this head: `typeof withCredentialFileLock === 'function'` through the subpath, as on the parent commit.
On the guard gap you named — it is real, and I did not close it. Catching this class properly needs a symbol-level baseline built from the previous commit, which is more machinery than this PR should carry. What changed instead is that the weaker property is now trivially checkable, because the export map only drops `.` and adds subpaths, with no pre-existing entrypoint retargeted:
```
git diff upstream/main...HEAD -- packages/storage/package.json
```
Whether the file lock should become package-private is still open — worth its own change if you want it, and it would then need the compatibility note this one no longer does.
Re-ran after the fix: storage 925 (910 pass, 14 skipped, 1 pre-existing `managed-dependency-environment-crash` failure that reproduces on `main` locally and passes in CI), `lint`, `format:check`, `typecheck` and `check:asf-headers` all pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org