fix: address review threads on the legacy-removal PRs

.changeset/legacy-removal-followups.md

@@ -0,0 +1,10 @@
+---
+"agent-bundle": patch
+"@agent-bundle/runtime": patch
+---
+
+Accept a CommonJS `module.exports` server factory as the default export of a
+stdio MCP entry, so `AB4730` no longer rejects a `.cjs` entry the generated
+lifecycle shell can run, and reject a SQLite state store at open with the
+typed `corrupt` error when its journal schema differs in column nullability
+from the one the current kernel writes. (#850)

docs/diagnostics.md

@@ -647,8 +647,12 @@ A local MCP server entry module (explicit `entry:` or the conventional
 in the framework stdio lifecycle shell (console-to-stderr guard,
 SIGINT/SIGTERM, stdin-EOF exit, bounded shutdown, heartbeat), and the shell
 calls the module's default export to build the server, so a module without
-one cannot be built. The detection is the same static default-export scan the
-build uses, so the diagnostic and the build always agree.
+one cannot be built. Validation finds the default export with a static scan of
+the entry's top-level statements. A CommonJS entry's top-level
+`module.exports = <factory>` counts as that default export, because the
+bundler exposes the assigned value as the module's `default`, unless the file
+declares its own `module` binding. A narrower `exports.foo = …` or
+`module.exports.foo = …` does not count.
 
 Recover: default-export the server factory from the entry module, or declare
 a prebuilt server with `command` or `url`, which the framework launches

docs/entry-conventions.md

@@ -713,7 +713,9 @@ real child process's probe (two pipes). A test that wants other values injects
 
 Source validation reports `AB4730` as an **error** when a local stdio MCP
 entry has no default export: the framework lifecycle shell calls that export
-to build the server, so such a module cannot be built. It reports
+to build the server, so such a module cannot be built. A CommonJS entry may
+assign the factory to `module.exports` instead; the bundler exposes that
+value as the module's `default`, and the scan counts it. It reports
 **informational** nudges (never errors) when `src/cli.ts`, `src/index.ts`, or
 `src/mcp/<server-id>.ts` exists but explicit configuration shadows it
 (`AB4731`/`AB4732`/`AB4733`). `bin: false` / `lib: false` opt-outs stay
@@ -1074,7 +1076,10 @@ name).
 
 A module that constructs and connects a transport at top level without a
 default export cannot be built: source validation reports `AB4730` as an
-error. A server the framework should launch as-is instead of compiling is
+error. A CommonJS entry's top-level `module.exports = <factory>` is that
+default export under bundling; `exports.foo = …` and `module.exports.foo = …`
+are named and do not satisfy it. A server the framework should launch as-is
+instead of compiling is
 declared with `command` or `url`, or as a `{ prebuilt: ... }` entry the
 consumer's own build produced. The operator `.env` layer (#469) comes from
 `agent-bundle/launch-env`, which the shell's prelude applies; that module is

packages/agent-bundle/src/build/entry-exports.ts

@@ -17,6 +17,46 @@ export interface EntryExportScan {
 const hasModifier = (statement: ts.Statement, kind: ts.SyntaxKind): boolean =>
   ts.canHaveModifiers(statement) && (ts.getModifiers(statement) ?? []).some((modifier) => modifier.kind === kind);
 
+const assignsModuleExports = (statement: ts.Statement): boolean => {
+  if (!ts.isExpressionStatement(statement)) return false;
+  const assignment = statement.expression;
+  return ts.isBinaryExpression(assignment)
+    && assignment.operatorToken.kind === ts.SyntaxKind.EqualsToken
+    && ts.isPropertyAccessExpression(assignment.left)
+    && ts.isIdentifier(assignment.left.expression)
+    && assignment.left.expression.text === 'module'
+    && assignment.left.name.text === 'exports';
+};
+
+const bindsModule = (statement: ts.Statement): boolean => {
+  if (ts.isVariableStatement(statement)) {
+    return statement.declarationList.declarations.some((declaration) =>
+      ts.isIdentifier(declaration.name) && declaration.name.text === 'module');
+  }
+  if (ts.isFunctionDeclaration(statement) || ts.isClassDeclaration(statement)) return statement.name?.text === 'module';
+  if (ts.isImportDeclaration(statement)) {
+    const clause = statement.importClause;
+    if (clause === undefined) return false;
+    if (clause.name?.text === 'module') return true;
+    const bindings = clause.namedBindings;
+    if (bindings === undefined) return false;
+    return ts.isNamespaceImport(bindings)
+      ? bindings.name.text === 'module'
+      : bindings.elements.some((element) => element.name.text === 'module');
+  }
+  return false;
+};
+
+/**
+ * A CommonJS module's top-level `module.exports = <expr>`, which the bundler
+ * exposes as the namespace's `default`. A file that binds its own `module`
+ * never qualifies.
+ */
+export const assignsModuleExportsSource = (source: string, fileName = 'entry.js'): boolean => {
+  const { statements } = ts.createSourceFile(fileName, source, ts.ScriptTarget.Latest, false);
+  return statements.some(assignsModuleExports) && !statements.some(bindsModule);
+};
+
 const declaresMain = (statement: ts.Statement): boolean => {
   if (ts.isFunctionDeclaration(statement)) return statement.name?.text === 'main';
   if (ts.isVariableStatement(statement)) {

packages/agent-bundle/src/config/validate.ts

@@ -3,7 +3,7 @@ import { basename, extname, isAbsolute, join, posix, relative, resolve, sep } fr
 
 import { capabilityIsSupported, cliBinCapability, webSurfaceCapability } from '../adapters/capability-state.ts';
 import { builtInHostNames, isBuiltInHost } from '../adapters/composite-layout.ts';
-import { type EntryExportScan, scanEntryExportsSource } from '../build/entry-exports.ts';
+import { assignsModuleExportsSource, type EntryExportScan, scanEntryExportsSource } from '../build/entry-exports.ts';
 import { externalizedSpecifiers } from '../build/external-policy.ts';
 import { frameworkOwnedPluginCollisions, frameworkOwnedRsbuildPlugins } from '../build/framework-plugins.ts';
 import type { CapabilityState } from '../core/capabilities.ts';
@@ -616,9 +616,8 @@ const relativePosix = toPosixRelative;
 
 /**
  * AB4730: every local stdio entry is wrapped in the framework stdio lifecycle
- * shell, which imports the entry's default export as its server factory. The
- * detection is the same static export scan the build uses to build the wrap,
- * so the diagnostic and the build always agree.
+ * shell, which imports the entry's default export as its server factory. A
+ * CommonJS entry's top-level `module.exports` is that default under bundling.
  */
 const missingServerFactoryErrors = (
   name: string,
@@ -633,7 +632,8 @@ const missingServerFactoryErrors = (
     : conventionalEntry;
   if (source === undefined || !bundleScriptExtensions.has(extname(source).toLowerCase())) return [];
   try {
-    if (scanEntryExportsSource(readFileSync(source, 'utf8'), source).hasDefaultExport) return [];
+    const text = readFileSync(source, 'utf8');
+    if (scanEntryExportsSource(text, source).hasDefaultExport || assignsModuleExportsSource(text, source)) return [];
   } catch {
     // An unreadable entry is already reported by the existence diagnostics.
     return [];

packages/agent-bundle/tests/entry-shell.test.ts

@@ -8,7 +8,7 @@ import ts from 'typescript-5';
 import { claudeAdapter } from '../src/adapters/claude.ts';
 import { cursorHookWrapperSource, nativeHookWrapperSource, type TargetHookWrapper } from '../src/adapters/hook-contract.ts';
 import type { NoticeDeliveryAdvertisement } from '../src/adapters/notice-delivery.ts';
-import { scanEntryExportsSource } from '../src/build/entry-exports.ts';
+import { assignsModuleExportsSource, scanEntryExportsSource } from '../src/build/entry-exports.ts';
 import * as entryShellModule from '../src/build/entry-shell.ts';
 import { launchEnvLayerSpecifier, operatorEnvLayerImport, operatorEnvLayerModuleSource, operatorEnvLayerVirtualModule } from '../src/build/launch-env-shell.ts';
 import { stableJson } from '../src/core/digest.ts';
@@ -59,6 +59,22 @@ describe('entry export scanning', () => {
     expect(scanEntryExportsSource('export type { main } from "./types.ts";').hasMainExport).toBe(false);
   });
 
+  it('keeps module.exports out of the shared export scan', () => {
+    expect(scanEntryExportsSource('module.exports = () => server;', '/app/entry.cjs')).toEqual({
+      hasDefaultExport: false,
+      hasMainExport: false,
+    });
+  });
+
+  it('recognizes only a top-level module.exports assignment to an unbound module', () => {
+    expect(assignsModuleExportsSource('module.exports = () => server;', '/app/entry.cjs')).toBe(true);
+    expect(assignsModuleExportsSource('exports.foo = () => server;', '/app/entry.cjs')).toBe(false);
+    expect(assignsModuleExportsSource('module.exports.foo = 1;', '/app/entry.cjs')).toBe(false);
+    expect(assignsModuleExportsSource('if (ok) { module.exports = 1; }', '/app/entry.cjs')).toBe(false);
+    expect(assignsModuleExportsSource('const module = { exports: null };\nmodule.exports = factory;', '/app/entry.mts')).toBe(false);
+    expect(assignsModuleExportsSource("import module from './m.ts';\nmodule.exports = factory;", '/app/entry.ts')).toBe(false);
+  });
+
   it('never matches inside comments, strings, or template literals', () => {
     expect(scanEntryExportsSource('// export default nothing\nconst a = 1;').hasDefaultExport).toBe(false);
     expect(scanEntryExportsSource('/* export const main = 1 */ const a = 1;').hasMainExport).toBe(false);

packages/agent-bundle/tests/mcp.test.ts

@@ -1444,6 +1444,52 @@ it('creates session state only after setup succeeds and always inherits the stdi
   }
 }, 30_000);
 
+it('accepts a CommonJS stdio entry whose server factory is module.exports, and runs it', async () => {
+  const root = await mkdtemp(join(tmpdir(), 'agent-bundle-mcp-cjs-entry-'));
+  try {
+    await mkdir(join(root, 'src'), { recursive: true });
+    await mkdir(join(root, 'node_modules'), { recursive: true });
+    await symlink(
+      join(agentBundleNodeModules, '@modelcontextprotocol'),
+      join(root, 'node_modules', '@modelcontextprotocol'),
+      'dir',
+    );
+    await writeFile(join(root, 'agent-bundle.config.ts'), 'export default {};\n');
+    await writeFile(join(root, 'package.json'), '{"type":"module"}\n');
+    await writeFile(join(root, 'src', 'server.cjs'), [
+      "const { McpServer } = require('@modelcontextprotocol/server');",
+      '',
+      'module.exports = () => {',
+      "  const server = new McpServer({ name: 'cjs-server', version: '1.0.0' });",
+      "  server.registerTool('ping', { description: 'Answer a ping.' }, async () => ({",
+      "    content: [{ type: 'text', text: 'pong' }],",
+      '  }));',
+      '  return server;',
+      '};',
+      '',
+    ].join('\n'));
+    const config: AgentBundleConfig = {
+      mcp: { servers: { cjs: { entry: './src/server.cjs' } } },
+      plugin: { name: 'mcp-cjs-fixture' },
+      targets: ['portable'],
+    };
+    // The shell reads the entry's `default`, which is what the bundler makes
+    // of `module.exports`; AB4730 must not refuse an entry it can run.
+    expect(validateSource(loadedProject(root, config), { skills: [] }, registry)).toEqual([]);
+
+    const model = await normalizeProject(loadedProject(root, config), { skills: [] }, registry);
+    const artifact = join(root, 'dist');
+    await build({ model, outputRoot: artifact, projectRoot: root, registry: createDefaultRegistry(), routeGraph: emptyCompiledRouteGraph });
+
+    await expect(new McpService().list({ artifact, server: 'cjs', target: 'portable' })).resolves.toMatchObject({
+      server: { name: 'cjs-server', version: '1.0.0' },
+      tools: [{ name: 'ping' }],
+    });
+  } finally {
+    await removeTree(root);
+  }
+}, 30_000);
+
 it('serves compiler-bundled MCP App resources from a copied artifact without project source', async () => {
   const root = await mkdtemp(join(tmpdir(), 'agent-bundle-mcp-app-resource-'));
   const consumer = await mkdtemp(join(tmpdir(), 'agent-bundle-mcp-app-consumer-'));

packages/rsc-runtime/src/state/sqlite.ts

@@ -100,18 +100,40 @@ const COMPACTED_KERNEL_FORMAT = 2;
 const READABLE_KERNEL_FORMATS: readonly number[] = Object.freeze([KERNEL_FORMAT, COMPACTED_KERNEL_FORMAT]);
 
 /**
- * Column order of every table `initialize` creates. A pre-existing table
- * whose columns differ is not this kernel's schema and fails closed as
- * `corrupt` instead of surfacing a raw SQLite error on the first statement
- * that names a missing column.
+ * Column order and nullability of every table `initialize` creates, written
+ * as the `PRAGMA table_info` signature each `CREATE TABLE` below produces. A
+ * pre-existing table whose columns differ is not this kernel's schema and
+ * fails closed as `corrupt` instead of surfacing a raw SQLite error on the
+ * first statement that names a missing column. Nullability is part of that
+ * identity: a column this kernel reads unconditionally but an earlier layout
+ * left nullable holds rows it cannot decode, and those must fail at open
+ * rather than on the row that happens to be NULL.
  */
 const TABLE_COLUMNS: Readonly<Record<string, readonly string[]>> = Object.freeze({
-  agent_state_head: ['id', 'revision', 'state'],
-  agent_state_journal: ['revision', 'kind', 'name', 'payload', 'state', 'result_state', 'to_version', 'idempotency_key', 'committed_at'],
-  agent_state_meta: ['id', 'definition_id', 'schema_version', 'kernel_format'],
-  agent_state_pruned_keys: ['idempotency_key', 'revision', 'canonical_input'],
+  agent_state_head: ['id', 'revision NOT NULL', 'state NOT NULL'],
+  agent_state_journal: [
+    'revision',
+    'kind NOT NULL',
+    'name',
+    'payload',
+    'state',
+    'result_state NOT NULL',
+    'to_version',
+    'idempotency_key NOT NULL',
+    'committed_at NOT NULL',
+  ],
+  agent_state_meta: ['id', 'definition_id NOT NULL', 'schema_version NOT NULL', 'kernel_format NOT NULL'],
+  agent_state_pruned_keys: ['idempotency_key', 'revision NOT NULL', 'canonical_input NOT NULL'],
 });
 
+interface TableInfoRow {
+  readonly name: string;
+  readonly notnull: number;
+}
+
+const columnSignature = (column: TableInfoRow): string =>
+  column.notnull === 0 ? column.name : `${column.name} NOT NULL`;
+
 export interface SqliteStateDriverOptions {
   /**
    * SQLite lock wait budget per operation in milliseconds (default 5000).
@@ -848,8 +870,8 @@ class SqliteStore<TState, TEvents extends AgentStateEventSchemas> implements Age
       `);
       const definition = this.#definition;
       for (const [table, columns] of Object.entries(TABLE_COLUMNS)) {
-        const actual = (transactionDb.prepare(`PRAGMA table_info(${table})`).all() as unknown as { readonly name: string }[])
-          .map((column) => column.name);
+        const actual = (transactionDb.prepare(`PRAGMA table_info(${table})`).all() as unknown as TableInfoRow[])
+          .map(columnSignature);
         if (actual.join(',') !== columns.join(',')) {
           throw new AgentStateError(
             'corrupt',

packages/rsc-runtime/tests/state-sqlite.test.ts

@@ -189,6 +189,39 @@ describe('sqlite driver storage behavior', () => {
       });
     }));
 
+  it('fails closed with a typed corrupt error when a column that must be NOT NULL is nullable', () =>
+    withRoot(async (root) => {
+      const file = join(root, 'state.sqlite');
+      const store = await createSqliteStateDriver({ file }).open(counterDefinition());
+      await store.dispatch('bumped', { by: 1 }, { idempotencyKey: 'k1' });
+      await store.close();
+      const db = new DatabaseSync(file);
+      // Same column names in the same order, so only the NOT NULL flags tell
+      // this journal apart from the one the current kernel writes.
+      db.exec(`
+        ALTER TABLE agent_state_journal RENAME TO agent_state_journal_old;
+        CREATE TABLE agent_state_journal (
+          revision INTEGER PRIMARY KEY,
+          kind TEXT NOT NULL CHECK (kind IN ('event', 'reset', 'migrate')),
+          name TEXT,
+          payload TEXT,
+          state TEXT,
+          result_state TEXT,
+          to_version INTEGER,
+          idempotency_key TEXT NOT NULL UNIQUE,
+          committed_at TEXT NOT NULL
+        );
+        INSERT INTO agent_state_journal SELECT * FROM agent_state_journal_old;
+        DROP TABLE agent_state_journal_old;
+      `);
+      db.close();
+      await expect(createSqliteStateDriver({ file }).open(counterDefinition())).rejects.toMatchObject({
+        code: 'corrupt',
+        message: expect.stringContaining('table agent_state_journal') as string,
+        name: 'AgentStateError',
+      });
+    }));
+
   it('rejects a pending open when the driver closes before initialization resumes', () =>
     withRoot(async (root) => {
       const driver = createSqliteStateDriver({ root });

website/docs/en/guide/authoring/mcp.mdx

@@ -571,6 +571,19 @@ sixty-second activity throttle, labeled with the server name).
 That guard matters because stdout carries JSON-RPC framing: one stray `console.log` from any
 imported module would corrupt the protocol stream.
 
+A CommonJS entry assigns the same factory to `module.exports`, which the bundler exposes as the
+module's default export:
+
+```js
+// src/mcp/curator.cjs
+const { McpServer } = require('@modelcontextprotocol/server');
+
+module.exports = () => new McpServer({ name: 'curator', version: '1.0.0' });
+```
+
+Only the whole-object assignment counts. `exports.curator = …` and `module.exports.curator = …`
+are named exports, and the shell reads neither.
+
 A module that constructs and connects a transport at top level without a default export cannot be
 built: source validation reports `AB4730` as an **error**. Declare a server you do not want
 compiled with `command` or `url`, or as a `{ prebuilt: ... }` entry your own build produced.

website/docs/en/guide/development/testing.mdx

@@ -361,6 +361,11 @@ export const matrix = async (): Promise<void> => {
 };
 ```
 
+A resource route's fixture, and an MCP App route's fixture under `apps: 'explicit'`, must set
+`kind: 'resource'`. A bare `{}` entry fails that route's `coverage` check with
+`resource/app route fixture must be { kind: "resource" }`; the key alone does not cover the route.
+The same `kind` is refused on a tool, prompt, or CLI route, whose fixtures carry `input` instead.
+
 The packed and installed-host entry points take the same fixture shape plus the session they run
 against: `runPackedContractMatrix` needs the open packed session and the manifest compiled
 *before* source removal, and `runInstalledHostContractMatrix` needs the session

website/docs/en/reference/runtime-environment.mdx

@@ -123,6 +123,25 @@ Default state projection budgets are 5,000 ms per commit, 262,144 bytes per even
 of state, and 100,000 revisions. `agent-bundle inspect --state` reports the resolved driver,
 lifetime, durable location, and budget source for each definition.
 
+### Stores written before the current SQLite layout
+
+The driver reads only the layout it writes today. It neither adopts nor migrates a store written
+before that layout, in either of two ways.
+
+A database under the pre-#201 file name, whose suffix was the state id's own leading bytes in hex
+rather than a SHA-256 digest, is never opened. The driver leaves the old file untouched and
+creates a new, empty store beside it under the current name, so the old data is still on disk and
+simply unread.
+
+A database under the current file name whose `agent_state_journal` table differs from the one the
+driver creates fails at open with a `corrupt` `AgentStateError` naming the table. Column names,
+their order, and their `NOT NULL` flags all count, so a journal whose `result_state` column is
+nullable, as an older runtime's `ALTER TABLE` left it, is rejected before any row is read.
+
+Recover from either by deleting the stale database files, each `*.sqlite` plus its `-wal` and
+`-shm` sidecars, or the whole state root. The next launch creates a fresh store at the definition's
+initial state. Nothing reconstructs the old history.
+
 ## Next
 
 - [Security](./security.mdx): the credential and network boundaries around these processes.