From 293b72438bc05b443253016afc7177b0ada7966c Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Tue, 1 Sep 2026 18:11:11 +0000 Subject: [PATCH] fix(test-harness): review follow-ups from #151 Bind route loaders to the manifest that produced them, so an explicit manifest cannot silently execute the registered project's module for a colliding route id and report the other manifest's provenance. Separate an absent document value from an emitted null in toHaveValue, record request-scoped progress even when the caller supplies its own reporter, validate the generated registry version where the helpers read it, and correct the README's request-context example. --- .changeset/test-harness-review-follow-ups.md | 11 +++ packages/agent-bundle/README.md | 13 ++- .../agent-bundle/src/rstest/setup-module.ts | 7 ++ packages/agent-bundle/src/test/matchers.ts | 17 +++- packages/agent-bundle/src/test/registry.ts | 56 +++++++++++- packages/agent-bundle/src/test/render.ts | 32 ++++++- .../tests/route-unit/render-route.test.ts | 17 ++++ .../tests/test-harness-manifest.test.ts | 91 ++++++++++++++++++- 8 files changed, 225 insertions(+), 19 deletions(-) create mode 100644 .changeset/test-harness-review-follow-ups.md diff --git a/.changeset/test-harness-review-follow-ups.md b/.changeset/test-harness-review-follow-ups.md new file mode 100644 index 000000000..7b5b5f692 --- /dev/null +++ b/.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`. diff --git a/packages/agent-bundle/README.md b/packages/agent-bundle/README.md index c9d51494e..c13b520a8 100644 --- a/packages/agent-bundle/README.md +++ b/packages/agent-bundle/README.md @@ -186,7 +186,10 @@ 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' }, }); @@ -194,9 +197,11 @@ expectDocument(document).toHaveStatus('success').toContainMarkdown('Dune').toHav ``` `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 diff --git a/packages/agent-bundle/src/rstest/setup-module.ts b/packages/agent-bundle/src/rstest/setup-module.ts index 41d87dee3..f22062c42 100644 --- a/packages/agent-bundle/src/rstest/setup-module.ts +++ b/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))}),`); diff --git a/packages/agent-bundle/src/test/matchers.ts b/packages/agent-bundle/src/test/matchers.ts index f5dd68632..022b2eb05 100644 --- a/packages/agent-bundle/src/test/matchers.ts +++ b/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; diff --git a/packages/agent-bundle/src/test/registry.ts b/packages/agent-bundle/src/test/registry.ts index a266461a9..b7cd4618b 100644 --- a/packages/agent-bundle/src/test/registry.ts +++ b/packages/agent-bundle/src/test/registry.ts @@ -33,7 +33,7 @@ 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', @@ -41,10 +41,24 @@ export const registerTestRoutes = (registry: AgentTestRouteRegistry): void => { { 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; diff --git a/packages/agent-bundle/src/test/render.ts b/packages/agent-bundle/src/test/render.ts index a98c6e02d..e5e6afe0c 100644 --- a/packages/agent-bundle/src/test/render.ts +++ b/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; diff --git a/packages/agent-bundle/tests/route-unit/render-route.test.ts b/packages/agent-bundle/tests/route-unit/render-route.test.ts index 5db7f3c40..078ea4947 100644 --- a/packages/agent-bundle/tests/route-unit/render-route.test.ts +++ b/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'); diff --git a/packages/agent-bundle/tests/test-harness-manifest.test.ts b/packages/agent-bundle/tests/test-harness-manifest.test.ts index ed75ff489..0e450b1c0 100644 --- a/packages/agent-bundle/tests/test-harness-manifest.test.ts +++ b/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; + +/** Installs a registry the way the generated setup module does: straight onto the realm global. */ +const withRealmRegistry = async (registry: unknown, body: () => T | Promise): Promise => { + 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 => { + 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');