fix(cli): bundled workspace packaging for release-runtime#128
fix(cli): bundled workspace packaging for release-runtime#128stoicneko wants to merge 2 commits intohappier-dev:devfrom
Conversation
WalkthroughAdds a centralized module exporting canonical bundled workspace package metadata and a factory to compute bundle descriptors; build and bundling scripts now consume these dynamic definitions; tests were extended to verify synchronization between package.json and the new configuration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can generate a title for your PR based on the changes.Add |
Greptile SummaryThis PR fixes the Key changes:
Two minor style observations:
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| apps/cli/scripts/bundledWorkspacePackages.mjs | New canonical shared-list module — defines all four bundled workspace packages (agents, cli-common, protocol, release-runtime) and exports helper factory; clean, no issues. |
| apps/cli/scripts/buildSharedDeps.mjs | Switches to the shared canonical list for tsc compilation loop and syncBundledWorkspaceDist defaults; release-runtime is now compiled in main() but lacks a post-build artifact sanity check (unlike protocol). Minor: bundledWorkspaceDirs is a trivial re-export, which makes the regression test assertion tautological in practice. |
| apps/cli/scripts/bundleWorkspaceDeps.mjs | Replaces inline bundle-list literal with factory call from shared module; all four packages including release-runtime were already present before, behavior is unchanged. |
| apps/cli/scripts/prepack-script.test.mjs | New regression test verifying bundledDependencies, build targets, and bundle targets stay aligned. The assert.deepEqual([...bundledWorkspaceDirs], [...bundledWorkspaceDirNames]) assertion is effectively checking a re-export against itself, making it tautological in the current implementation. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[yarn prepack] --> B[yarn build]
B --> C[prebuild: build:shared\nbuildSharedDeps.mjs main]
C --> D{loop bundledWorkspaceDirs}
D --> E[runTsc packages/agents]
D --> F[runTsc packages/cli-common]
D --> G[runTsc packages/protocol]
D --> H[runTsc packages/release-runtime\n🆕 now included]
E & F & G & H --> I[existsSync protocol/dist check]
I --> J[syncBundledWorkspaceDist\nall 4 packages]
J --> K[pkgroll CLI build]
K --> L[node bundleWorkspaceDeps.mjs]
L --> M[createBundledWorkspaceBundles\n🆕 from shared canonical list]
M --> N[bundleWorkspacePackages\nagents, cli-common, protocol, release-runtime]
N --> O[vendorBundledPackageRuntimeDependencies\nper bundle]
O --> P[npm pack ready]
subgraph bundledWorkspacePackages.mjs [bundledWorkspacePackages.mjs 🆕]
Q[bundledWorkspacePackages array\nagents / cli-common / protocol / release-runtime]
R[bundledWorkspaceDirNames]
S[bundledWorkspacePackageNames]
T[createBundledWorkspaceBundles factory]
Q --> R & S & T
end
M --> bundledWorkspacePackages.mjs
D --> bundledWorkspacePackages.mjs
Comments Outside Diff (1)
-
apps/cli/scripts/buildSharedDeps.mjs, line 140-148 (link)No post-build artifact check for
release-runtimemain()now compiles all four packages in thebundledWorkspaceDirsloop, but the only artifact sanity check after the loop is for@happier-dev/protocol'sdist/index.js.release-runtimeis now equally critical to the prepack pipeline (it was previously missing from the build step entirely, which is exactly the bug being fixed here), yet a silent misconfiguration in itstsconfig.json(outDirpointing somewhere unexpected, orincludematching nothing) could result in an empty dist being bundled without any diagnostic.Consider adding a parallel check:
const releaseRuntimeDist = resolve(repoRoot, 'packages', 'release-runtime', 'dist', 'index.js'); if (!existsSync(releaseRuntimeDist)) { throw new Error(`Expected @happier-dev/release-runtime build output missing: ${releaseRuntimeDist}`); }
This is a non-blocking suggestion since
runTscalready throws on non-zero exit codes — but the explicit check onprotocolsets a precedent that is worth following for the newly added package.
Last reviewed commit: 3919fb0
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| : []; | ||
|
|
||
| assert.deepEqual(bundledDependencies, [...bundledWorkspacePackageNames].sort()); | ||
| assert.deepEqual([...bundledWorkspaceDirs], [...bundledWorkspaceDirNames]); |
There was a problem hiding this comment.
Tautological assertion — re-export compared to itself
bundledWorkspaceDirs in buildSharedDeps.mjs is defined as:
export const bundledWorkspaceDirs = bundledWorkspaceDirNames;…a direct re-export of bundledWorkspaceDirNames. So when the test imports both and spreads them, it is comparing the same underlying frozen array to itself — deepEqual([...x], [...x]) — which will always pass regardless of what value x holds.
The intent of the assertion appears to be guarding against a future divergence where buildSharedDeps.mjs might override bundledWorkspaceDirs with a manually maintained subset. That guard is real and useful, but the assertion can only protect against that if the two symbols are independently defined. Consider adding a comment to the assertion explaining its forward-looking intent, or alternatively move the assertion's value closer to what it actually tests — that the main() loop in buildSharedDeps compiles every entry in the canonical list:
// Verify buildSharedDeps re-exports the canonical list without filtering.
// If buildSharedDeps.mjs ever defines bundledWorkspaceDirs independently,
// this assertion catches any divergence from the source of truth.
assert.deepEqual([...bundledWorkspaceDirs], [...bundledWorkspaceDirNames]);![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/cli/scripts/bundledWorkspacePackages.mjs (1)
3-16: Freeze the definition entries too.
Object.freeze([...])only locks the array shell. The exported{ dirName, packageName }objects are still mutable, so an accidental write would desync this canonical list from the already-derivedbundledWorkspaceDirNamesandbundledWorkspacePackageNames.Optional hardening
export const bundledWorkspacePackages = Object.freeze([ - { dirName: 'agents', packageName: '@happier-dev/agents' }, - { dirName: 'cli-common', packageName: '@happier-dev/cli-common' }, - { dirName: 'protocol', packageName: '@happier-dev/protocol' }, - { dirName: 'release-runtime', packageName: '@happier-dev/release-runtime' }, + Object.freeze({ dirName: 'agents', packageName: '@happier-dev/agents' }), + Object.freeze({ dirName: 'cli-common', packageName: '@happier-dev/cli-common' }), + Object.freeze({ dirName: 'protocol', packageName: '@happier-dev/protocol' }), + Object.freeze({ dirName: 'release-runtime', packageName: '@happier-dev/release-runtime' }), ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/scripts/bundledWorkspacePackages.mjs` around lines 3 - 16, The exported bundledWorkspacePackages array currently freezes only the array shell but leaves each entry mutable; update the initializer for bundledWorkspacePackages so each entry object is frozen as well (e.g., wrap each { dirName, packageName } with Object.freeze or map to Object.freeze for the entries) so bundledWorkspacePackages contains immutable objects; keep bundledWorkspaceDirNames and bundledWorkspacePackageNames as they are but ensure they continue to derive from the now-immutable bundledWorkspacePackages.apps/cli/scripts/buildSharedDeps.mjs (1)
141-143: Validate every bundled workspace emitsdist/.This loop now builds
release-runtime, but the postcondition below still only checkspackages/protocol/dist/index.js. If any other bundled workspace stops emittingdist/,syncBundledWorkspaceDist()just swallows the copy failure and leaves stale artifacts underapps/cli/node_modules.Suggested hardening
export function main() { for (const pkg of bundledWorkspaceDirs) { runTsc(resolve(repoRoot, 'packages', pkg, 'tsconfig.json')); + const distDir = resolve(repoRoot, 'packages', pkg, 'dist'); + if (!existsSync(distDir)) { + throw new Error(`Expected `@happier-dev/`${pkg} build output missing: ${distDir}`); + } } - - const protocolDist = resolve(repoRoot, 'packages', 'protocol', 'dist', 'index.js'); - if (!existsSync(protocolDist)) { - throw new Error(`Expected `@happier-dev/protocol` build output missing: ${protocolDist}`); - } // If the CLI currently has bundled workspace deps under apps/cli/node_modules, // keep their dist outputs in sync so local builds/tests do not consume stale artifacts. syncBundledWorkspaceDist({ repoRoot }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/scripts/buildSharedDeps.mjs` around lines 141 - 143, The loop that runs runTsc over bundledWorkspaceDirs can succeed without emitting a dist/, but the later postcondition only checks packages/protocol/dist/index.js; update the build verification to confirm each package produced a dist directory (e.g. check resolve(repoRoot, 'packages', pkg, 'dist') exists and contains expected artifacts such as index.js or package.json main) immediately after runTsc for each pkg, and if missing throw or exit non‑zero so syncBundledWorkspaceDist cannot continue silently; modify the logic around bundledWorkspaceDirs, runTsc and syncBundledWorkspaceDist to surface copy failures instead of swallowing them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/cli/scripts/buildSharedDeps.mjs`:
- Around line 141-143: The loop that runs runTsc over bundledWorkspaceDirs can
succeed without emitting a dist/, but the later postcondition only checks
packages/protocol/dist/index.js; update the build verification to confirm each
package produced a dist directory (e.g. check resolve(repoRoot, 'packages', pkg,
'dist') exists and contains expected artifacts such as index.js or package.json
main) immediately after runTsc for each pkg, and if missing throw or exit
non‑zero so syncBundledWorkspaceDist cannot continue silently; modify the logic
around bundledWorkspaceDirs, runTsc and syncBundledWorkspaceDist to surface copy
failures instead of swallowing them.
In `@apps/cli/scripts/bundledWorkspacePackages.mjs`:
- Around line 3-16: The exported bundledWorkspacePackages array currently
freezes only the array shell but leaves each entry mutable; update the
initializer for bundledWorkspacePackages so each entry object is frozen as well
(e.g., wrap each { dirName, packageName } with Object.freeze or map to
Object.freeze for the entries) so bundledWorkspacePackages contains immutable
objects; keep bundledWorkspaceDirNames and bundledWorkspacePackageNames as they
are but ensure they continue to derive from the now-immutable
bundledWorkspacePackages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8c141c39-70e3-417c-840b-9749a8d06a96
📒 Files selected for processing (4)
apps/cli/scripts/buildSharedDeps.mjsapps/cli/scripts/bundleWorkspaceDeps.mjsapps/cli/scripts/bundledWorkspacePackages.mjsapps/cli/scripts/prepack-script.test.mjs
- Add JSDoc to bundledWorkspacePackages.mjs (file header + all exports/functions) - Deep-freeze entry objects so mutations cannot desync derived exports - Extend post-build artifact check to cover all bundled workspace packages (previously only checked packages/protocol/dist; now checks dist/ for each package immediately after runTsc so a misconfigured tsconfig is caught early)
stoicneko
changed the title
1 participant
Summary
bundledWorkspacePackages.mjsmodule as single source of truth for all bundled workspace packagesrelease-runtimealongside the other bundled workspace packages (it was previously missing from thebuild:sharedtsc step)protocol)bundledDependencies, build targets, and bundle targets alignedTarget branch
devHow to test
cd apps/clinode --test scripts/prepack-script.test.mjsyarn workspace @happier-dev/cli prepackrelease-runtimedist is present underapps/cli/node_modules/@happier-dev/release-runtime/distChecklist
devbranchSummary by CodeRabbit
Chores
Tests