fix(build): isolate re-bundled runtime entries - #636
Conversation
🦋 Changeset detectedLatest commit: 7f784aa 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 |
commit: |
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
P1: the synthetic packed-test build can become the release output
Static review at ce66e4386257fe80b48ea0e9113b65f5f857c620; no release or tests executed by this reviewer.
The structural direction is aligned with #566: a dedicated runtime compilation removes the growing private-sibling entry list. Keep that simplification. The fixture must not replace the canonical publishable build.
In scripts/run-packed-tests.mjs, every invocation now runs the workspace pnpm build with AGENT_BUNDLE_RUNTIME_REBUNDLE_FIXTURE=1, including --release. That rewrites packages/agent-bundle/dist with tests/fixtures/runtime-rebundle/mcp-server-runtime.ts as its runtime entry. The finally block deletes only the temporary tarball directory; it does not restore the production dist.
The root scripts compose as:
pnpm release
-> pnpm check:release
-> pack:dry-run (normal build)
-> lint:release
-> test:packed:release (overwrites workspace dist with fixture build)
-> changeset publish (can consume the fixture-bearing dist)
So the release gate both tests substituted package bytes rather than the ordinary candidate and leaves that substitution in the directory the publisher uses. This is a concrete code-path risk, not evidence that an affected package has already been published.
Required correction
Build the synthetic-private-sibling case into an isolated output/project and pack a separate test tarball. Keep the normal shared packed suite and release candidate built from production entries. Do not rely on the developer remembering to rebuild after tests, or on an environment variable merely being unset in the later publish process—the files have already been changed.
Add a regression that runs the packed gate and confirms the normal publishable runtime entry is still the production build, while the dedicated synthetic fixture demonstrably crossed package and artifact compilation. Preserve the deleted-source stdio/process test and state identity checks. Re-run declaration/public-export validation after the two-lib split.
No emitted-JavaScript dependency scanner is needed to fix this ownership problem. Isolate test artifacts from release artifacts.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Addressed the owner P1 in 0cd885f, 85b43f6, and 7f784aa. The runner now creates the ordinary release build/tarball first, builds the synthetic sibling into an isolated |
…o feat/592-authoritative-manifest
|
Follow-up receipt: this review was against |
|
Follow-up complete in #645 ( |
Closes the remaining runtime re-bundling boundary item from #566.
Summary
runtime-nodeRslib compilation, separated from the public graph that dynamically imports the MCP runtimeTests
pnpm build && pnpm typecheck && pnpm lint && pnpm test:unit && pnpm test:packed(4,123 unit passed; 35 packed passed, 1 skipped)node --input-type=module -e "await import(\"./packages/agent-bundle/dist/mcp-server-runtime.js\"); ..."(normal post-packed dist does not execute the fixture)Deslop
Deslop: GPT-5.6 Sol, 0 edits after reviewing the complete branch diff.
Self-review
Reviewer: Claude Fable 5.1 Thinking High (
change-risk-reviewer).First pass findings and dispositions:
test:packed:releaseinto publish — fixed in 5bcd2b7: the packed runner now builds the fixture into an isolated dist and stages a separate tarball; the normal release dist and tarball are never mutated.splitChunks: falsewas overridden by Rslib and therefore inert — fixed in 5bcd2b7: removed it and documented the actual compilation boundary.@agent-bundle/runtimeremains external and its shared class-identity graph/heavy subpaths are unchanged.app.jsis compiler-rebundled but remained public — fixed in 5bcd2b7: moved it intoruntime-node.Subsequent passes fixed every concrete finding: 0cd885f isolated fixture dist and tarball ownership; 85b43f6 scoped fixture metadata to the stdio proof and made unsupported ad-hoc fixture requests fail clearly; 7f784aa retained the normal release-tarball stdio case alongside the synthetic case and isolated the fixture cache. Final pass: no blocking merge risks. The optional emitted-source scanner was declined because both regressions intentionally prove behavior through real packed processes.