fix(test-harness): review follow-ups from #151

.changeset/test-harness-review-follow-ups.md

@@ -0,0 +1,11 @@
+---
+"agent-bundle": patch
+---
+
+Bind route-unit test loaders to the manifest that produced them, so rendering
+against an explicit manifest can no longer execute the registered project's
+module for a colliding route id. `expectDocument().toHaveValue()` now separates
+a document that emitted no value from one whose value is `null`, `renderRoute`
+records request-scoped progress even when the caller supplies its own reporter,
+and the generated registry's version is validated where the helpers read it
+rather than only in `registerTestRoutes`.

packages/agent-bundle/README.md

@@ -186,17 +186,22 @@ renderer and the real request store, and resolves to the final Agent Document:
 import { expectDocument, renderRoute, testManifest } from 'agent-bundle/test';
 
 const { document } = await renderRoute('tool:library/summarize', {
-  context: { cwd: '/tmp/library', operationId: 'run-1' },
+  context: {
+    invocation: { id: 'run-1' },
+    workspace: { source: 'native', state: 'available', value: { root: '/tmp/library' } },
+  },
   input: { title: 'Dune' },
 });
 
 expectDocument(document).toHaveStatus('success').toContainMarkdown('Dune').toHaveValue({ chapters: 24 });
 ```
 
 `renderRoute` accepts `input`, `args` (CLI routes), request-`context`
-overrides, a `progress` reporter, render `limits`, and an `abort` signal; it
-returns the document, the ordered render events, the resolved provenance, and
-the route's own `resultSchema`-parsed value. `testManifest()` exposes the
+overrides — including a `context.progress` reporter — render `limits`, and a
+`signal`; it returns the document, the request-scoped progress the route
+reported, the resolved provenance, and the route's own `resultSchema`-parsed
+value. Progress is recorded whether or not the caller supplies a reporter of
+its own. `testManifest()` exposes the
 compiled route inventory, so a suite can iterate every route in process rather
 than paying for a build per route. Every failure — an unknown route, a refused
 route kind, a rejected input, a render error — names the route id, the target

packages/agent-bundle/src/rstest/setup-module.ts

@@ -27,6 +27,13 @@ const renderableRoutes = (manifest: AgentBundleTestManifest): readonly TestableR
 /** Bundler-resolvable module specifier for one route module path. */
 const specifier = (source: string): string => source.replaceAll('\\', '/');
 
+/**
+ * The generated registry is assigned to the realm global directly rather than
+ * through `registerTestRoutes`, so that this file imports nothing but the
+ * project's own route modules. `version` is therefore validated by the helpers
+ * when they read the registry, which is also the only side that knows which
+ * `agent-bundle/test` the test file actually resolved.
+ */
 export const routeTestSetupSource = (manifest: AgentBundleTestManifest): string => {
   const loaders = renderableRoutes(manifest)
     .map((route) => `    ${JSON.stringify(route.id)}: () => import(${JSON.stringify(specifier(route.source))}),`);

packages/agent-bundle/src/test/matchers.ts

@@ -38,7 +38,11 @@ export interface DocumentAssertions {
   /** Asserts the document's node kinds in document order. */
   readonly toHaveNodeKinds: (kinds: readonly AgentDocumentNodeKind[]) => DocumentAssertions;
   readonly toHaveStatus: (status: AgentDocumentStatus) => DocumentAssertions;
-  /** Asserts the document's structured value equals `value` (JSON structural equality). */
+  /**
+   * Asserts the document's structured value equals `value` (JSON structural
+   * equality). `undefined` asserts the document emitted no value at all, which
+   * `null` does not satisfy.
+   */
   readonly toHaveValue: (value: unknown) => DocumentAssertions;
 }
 
@@ -106,10 +110,15 @@ export const expectDocument = (subject: DocumentSubject): DocumentAssertions =>
       return assertions;
     },
     toHaveValue(value) {
-      if (stableJson(document.value ?? null) !== stableJson(value ?? null)) {
+      // `AgentDocument.value` is optional, so an absent value and an emitted
+      // `null` are distinct states. Collapsing them would pass a route that
+      // emitted no structured value at all.
+      const absent = document.value === undefined;
+      const expectAbsent = value === undefined;
+      if (absent !== expectAbsent || (!absent && stableJson(document.value) !== stableJson(value))) {
         fail('The Agent Document value differs from the expected structured value.', [
-          `expected:     ${captured(value)}`,
-          `received:     ${document.value === undefined ? 'no document value' : captured(document.value)}`,
+          `expected:     ${expectAbsent ? 'no document value' : captured(value)}`,
+          `received:     ${absent ? 'no document value' : captured(document.value)}`,
         ]);
       }
       return assertions;

packages/agent-bundle/src/test/registry.ts

@@ -33,18 +33,32 @@ const missingRegistry = (): AgentTestError => new AgentTestError(
   },
 );
 
-export const registerTestRoutes = (registry: AgentTestRouteRegistry): void => {
+const compatible = (registry: AgentTestRouteRegistry): AgentTestRouteRegistry => {
   if (registry.version !== AGENT_TEST_REGISTRY_VERSION) {
     throw new AgentTestError(
       'manifest-unavailable',
       `Incompatible Agent Bundle test registry version: found ${String(registry.version)}, expected ${String(AGENT_TEST_REGISTRY_VERSION)}.`,
       { recovery: 'Install one agent-bundle version for both the Rstest configuration and the test helpers.' },
     );
   }
-  realm[REGISTRY_SYMBOL] = registry;
+  return registry;
 };
 
-const registered = (): AgentTestRouteRegistry | undefined => realm[REGISTRY_SYMBOL];
+export const registerTestRoutes = (registry: AgentTestRouteRegistry): void => {
+  realm[REGISTRY_SYMBOL] = compatible(registry);
+};
+
+/**
+ * The registry this worker may read. The version is checked here rather than
+ * only in `registerTestRoutes`, because the generated setup module assigns the
+ * realm global directly: when the Rstest helper and `agent-bundle/test` resolve
+ * to different package versions, this is the only place that mismatch is seen
+ * before it surfaces as a misleading loader or manifest error.
+ */
+const registered = (): AgentTestRouteRegistry | undefined => {
+  const registry = realm[REGISTRY_SYMBOL];
+  return registry === undefined ? undefined : compatible(registry);
+};
 
 /**
  * The manifest the generated configuration registered for this test process.
@@ -57,7 +71,39 @@ export const testManifest = (): AgentBundleTestManifest => {
   return registry.manifest;
 };
 
-export const registeredRouteLoader = (routeId: string): AgentRouteModuleLoader | undefined =>
-  registered()?.loaders[routeId];
+/**
+ * Whether `manifest` is the compilation whose loaders were registered. Two
+ * projects can name the same route id, so identity is the manifest digest and
+ * project root rather than the route id alone.
+ */
+const producedRegisteredLoaders = (
+  registry: AgentTestRouteRegistry,
+  manifest: AgentBundleTestManifest,
+): boolean =>
+  registry.manifest === manifest
+  || (registry.manifest.digest === manifest.digest && registry.manifest.projectRoot === manifest.projectRoot);
+
+/**
+ * The registered loader for one route of `manifest`. Loaders are bound to the
+ * manifest that produced them: an explicit manifest describing another project
+ * resolves no loader, so a multi-project suite cannot execute the registered
+ * project's module while reporting the other manifest's provenance.
+ */
+export const registeredRouteLoader = (
+  manifest: AgentBundleTestManifest,
+  routeId: string,
+): AgentRouteModuleLoader | undefined => {
+  const registry = registered();
+  if (registry === undefined || !producedRegisteredLoaders(registry, manifest)) return undefined;
+  return registry.loaders[routeId];
+};
+
+/** The registered manifest's identity, so a loader miss can name the mismatch that caused it. */
+export const registeredManifestIdentity = (): { readonly digest: string; readonly projectRoot: string } | undefined => {
+  const registry = registered();
+  return registry === undefined
+    ? undefined
+    : { digest: registry.manifest.digest, projectRoot: registry.manifest.projectRoot };
+};
 
 export const hasRegisteredRoutes = (): boolean => registered() !== undefined;

packages/agent-bundle/src/test/render.ts

@@ -13,7 +13,7 @@ import type * as React from 'react';
 
 import { AgentTestError, captured } from './errors.ts';
 import { ROUTE_UNIT_PROOF_LEVEL, type AgentBundleTestManifest } from './manifest.ts';
-import { registeredRouteLoader, testManifest } from './registry.ts';
+import { registeredManifestIdentity, registeredRouteLoader, testManifest } from './registry.ts';
 import type {
   AgentRouteModule,
   RenderableRouteKind,
@@ -299,14 +299,32 @@ const resolveTarget = async (
     targets: manifest.targets,
   });
   const kind = renderableKind(descriptor, provenance);
-  const loader = registeredRouteLoader(descriptor.id);
+  const loader = registeredRouteLoader(manifest, descriptor.id);
   if (loader === undefined) {
+    const identity = registeredManifestIdentity();
+    // A registered manifest that is not this one means the loaders in this
+    // worker belong to a different compilation. Loading one of them would run
+    // another project's module under this manifest's provenance, so the miss is
+    // reported as the mismatch it is rather than as missing wiring.
+    const mismatched = identity !== undefined && identity.digest !== manifest.digest;
     throw new AgentTestError(
       'manifest-unavailable',
-      `Route ${descriptor.id} is compiled but no test-time module loader is registered for it.`,
+      mismatched
+        ? `Route ${descriptor.id} belongs to a manifest whose route loaders are not the ones registered in this test process.`
+        : `Route ${descriptor.id} is compiled but no test-time module loader is registered for it.`,
       {
+        ...(mismatched
+          ? {
+              details: [
+                `manifest:     ${manifest.digest} (${manifest.projectRoot})`,
+                `registered:   ${identity.digest} (${identity.projectRoot})`,
+              ],
+            }
+          : {}),
         provenance: { ...provenance, kind },
-        recovery: 'Build the Rstest configuration with agentBundleRstest() so the generated setup registers route loaders, or pass the route module to renderRoute() directly.',
+        recovery: mismatched
+          ? 'Render this route through the manifest the generated setup registered, or pass the route module to renderRoute() directly — route loaders are bound to the manifest that produced them.'
+          : 'Build the Rstest configuration with agentBundleRstest() so the generated setup registers route loaders, or pass the route module to renderRoute() directly.',
       },
     );
   }
@@ -378,9 +396,13 @@ export const renderRoute = async (
   const invocation = invocationFor(resolved.kind, resolved.provenance.routeId, options, resolved.provenance);
   const collected: AgentProgressUpdate[] = [];
   const context = options.context ?? {};
-  const progress: AgentProgressReporter = context.progress ?? {
+  // The result contract exposes the route's request-scoped progress, so the
+  // harness always records it and then delegates to a caller's reporter.
+  const reporter = context.progress;
+  const progress: AgentProgressReporter = {
     report: async (update) => {
       collected.push(update);
+      await reporter?.report(update);
     },
   };
   const signal = options.signal ?? new AbortController().signal;

packages/agent-bundle/tests/route-unit/render-route.test.ts

@@ -66,6 +66,23 @@ describe('renderRoute through the real renderer', () => {
     });
   });
 
+  it('records progress even when the caller supplies its own reporter', async () => {
+    const delegated: unknown[] = [];
+    const rendered = await renderRoute('tool:harness/echo', {
+      context: {
+        progress: {
+          report: async (update) => {
+            delegated.push(update);
+          },
+        },
+      },
+      input: { message: 'hello' },
+    });
+
+    expect(rendered.progress).toEqual([{ completed: 1, message: 'echoing', total: 1 }]);
+    expect(delegated).toEqual([...rendered.progress]);
+  });
+
   it('reports a represented error as the document status the runtime decided', async () => {
     const rendered = await renderRoute('tool:harness/unavailable');
 

packages/agent-bundle/tests/test-harness-manifest.test.ts

@@ -6,7 +6,12 @@ import { describe, expect, it } from '@rstest/core';
 import { routeTestSetupSource } from '../src/rstest/setup-module.ts';
 import { AgentTestError } from '../src/test/errors.ts';
 import { compileTestManifest, testManifestFromRouteGraph } from '../src/test/manifest.ts';
-import { AGENT_TEST_REGISTRY_VERSION, registerTestRoutes } from '../src/test/registry.ts';
+import {
+  AGENT_TEST_REGISTRY_SYMBOL_KEY,
+  AGENT_TEST_REGISTRY_VERSION,
+  registerTestRoutes,
+  testManifest,
+} from '../src/test/registry.ts';
 import { renderRoute } from '../src/test/render.ts';
 import { expectDocument } from '../src/test/matchers.ts';
 import { compileRouteGraph } from '../src/routes/graph.ts';
@@ -16,6 +21,21 @@ const fixtureRoot = resolve(import.meta.dirname, '../fixtures/route-harness');
 
 const manifest = await compileTestManifest({ root: fixtureRoot });
 
+const registrySymbol = Symbol.for(AGENT_TEST_REGISTRY_SYMBOL_KEY);
+const realm = globalThis as Record<symbol, unknown>;
+
+/** Installs a registry the way the generated setup module does: straight onto the realm global. */
+const withRealmRegistry = async <T>(registry: unknown, body: () => T | Promise<T>): Promise<T> => {
+  const previous = realm[registrySymbol];
+  realm[registrySymbol] = registry;
+  try {
+    return await body();
+  } finally {
+    if (previous === undefined) delete realm[registrySymbol];
+    else realm[registrySymbol] = previous;
+  }
+};
+
 describe('the compiled test manifest', () => {
   it('names every conventional route the compiler discovered, with its extracted config', () => {
     expect(Object.keys(manifest.routes).sort()).toEqual([
@@ -84,6 +104,60 @@ describe('the generated route registry', () => {
     expect(() => registerTestRoutes({ loaders: {}, manifest, version: 99 }))
       .toThrow('Incompatible Agent Bundle test registry version');
   });
+
+  // The generated module assigns the realm global directly, so registerTestRoutes
+  // never sees it; the version has to be refused where the helpers read it.
+  it('refuses an incompatible registry the generated setup assigned directly', async () => {
+    await withRealmRegistry({ loaders: {}, manifest, version: 99 }, () => {
+      expect(() => testManifest()).toThrow('Incompatible Agent Bundle test registry version');
+    });
+  });
+});
+
+describe('route loaders and the manifest that produced them', () => {
+  const foreign: AgentBundleTestManifest = {
+    ...manifest,
+    digest: 'f0e1d2c3b4a5968778695a4b3c2d1e0ff0e1d2c3b4a5968778695a4b3c2d1e0f',
+    projectRoot: '/tmp/another-project',
+  };
+
+  const registryLoading = (loaded: string[]): unknown => ({
+    loaders: {
+      'tool:harness/echo': (): Promise<unknown> => {
+        loaded.push('tool:harness/echo');
+        return Promise.resolve({ default: () => null });
+      },
+    },
+    manifest,
+    version: AGENT_TEST_REGISTRY_VERSION,
+  });
+
+  it('refuses another manifest\'s route rather than loading the registered module for it', async () => {
+    const loaded: string[] = [];
+    const error = await withRealmRegistry(
+      registryLoading(loaded),
+      async () => renderRoute('tool:harness/echo', { manifest: foreign }).catch((thrown: unknown) => thrown),
+    );
+
+    expect(loaded).toEqual([]);
+    expect((error as AgentTestError).code).toBe('manifest-unavailable');
+    expect((error as AgentTestError).message).toContain('not the ones registered');
+    expect((error as AgentTestError).message).toContain(foreign.digest);
+    expect((error as AgentTestError).message).toContain(manifest.digest);
+    expect((error as AgentTestError).message).toContain('bound to the manifest that produced them');
+  });
+
+  it('still resolves the loader for the manifest the registry was built from', async () => {
+    const loaded: string[] = [];
+    await withRealmRegistry(
+      registryLoading(loaded),
+      // The render itself needs the react-server pool; resolving the loader is
+      // what this asserts, so any later renderer failure is irrelevant here.
+      async () => renderRoute('tool:harness/echo', { manifest }).catch(() => undefined),
+    );
+
+    expect(loaded).toEqual(['tool:harness/echo']);
+  });
 });
 
 describe('route-unit failure diagnostics', () => {
@@ -173,6 +247,21 @@ describe('document matchers', () => {
     expect(error?.message).toContain('{"files":2}');
   });
 
+  it('separates a document that emitted no value from one whose value is null', () => {
+    const documentWith = (value: unknown): never => Object.freeze({
+      root: Object.freeze({ children: Object.freeze([]), kind: 'result' }),
+      status: 'success',
+      version: 1,
+      ...(value === undefined ? {} : { value }),
+    }) as never;
+
+    expectDocument(documentWith(undefined)).toHaveValue(undefined);
+    expectDocument(documentWith(null)).toHaveValue(null);
+    expect(() => expectDocument(documentWith(undefined)).toHaveValue(null)).toThrow('value differs');
+    expect(() => expectDocument(documentWith(null)).toHaveValue(undefined)).toThrow('value differs');
+    expect(() => expectDocument(documentWith(undefined)).toHaveValue({ files: 2 })).toThrow('value differs');
+  });
+
   it('fails an unmet status, Markdown, text, and error assertion', () => {
     expect(() => expectDocument(document).toHaveStatus('failed')).toThrow('unexpected status');
     expect(() => expectDocument(document).toContainMarkdown('missing')).toThrow('no Markdown node');