fix(cli): build release-runtime before packaging#137
Open
w-gitops wants to merge 2 commits intohappier-dev:devfrom
Open
fix(cli): build release-runtime before packaging#137w-gitops wants to merge 2 commits intohappier-dev:devfrom
w-gitops wants to merge 2 commits intohappier-dev:devfrom
Conversation
- Add workspaceBundleManifest.mjs as single source of truth for bundled packages - Include release-runtime in the shared workspace build list used by apps/cli - Verify build output for every bundled workspace package before packaging - Add contract test that guards the default bundled package sync list @happier-dev/cli imports @happier-dev/release-runtime at runtime and declares it as a bundled dependency, but buildSharedDeps.mjs only built agents, cli-common, and protocol. This made CLI packaging depend on pre-existing local packages/release-runtime/dist state, causing nondeterministic npm release artifacts and ERR_MODULE_NOT_FOUND failures (refs happier-dev#103). Refs happier-dev#127
WalkthroughCentralized the CLI workspace bundle configuration into a new manifest module and updated build/bundle scripts to consume it. Added a contract test verifying the manifest, bundle descriptor generation, build-entry resolution (package.json main fallback), and dist sync behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 suggest fixes for GitHub Check annotations.Configure the |
Contributor
Greptile SummaryThis PR fixes a build-time ordering bug where
Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| apps/cli/scripts/workspaceBundleManifest.mjs | New single source of truth for the bundled workspace package list; clean, minimal file with no issues. |
| apps/cli/scripts/buildSharedDeps.mjs | Adds resolveBundledWorkspaceBuildEntry for per-package build verification and switches default package list to the new manifest; has a minor redundant import+re-export pair (P2). |
| apps/cli/scripts/bundleWorkspaceDeps.mjs | Removes hard-coded bundle list and delegates to createBundledWorkspaceBundles; straightforward cleanup with no issues. |
| apps/cli/scripts/buildSharedDeps.contract.test.mjs | Focused contract tests covering the manifest list, bundle metadata derivation, main-entry resolution logic, and syncBundledWorkspaceDist default inclusion of release-runtime; well-scoped with injected I/O for hermeticity. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[buildSharedDeps.main] --> B[workspaceBundleManifest.mjs\nbundledWorkspacePackages]
B --> C{for each pkg\nin bundledWorkspacePackages}
C --> D[runTsc packages/pkg/tsconfig.json]
D --> E{for each pkg\nverify build output}
E --> F[resolveBundledWorkspaceBuildEntry\nreads pkg/package.json main field]
F --> G{dist entry\nexistsSync?}
G -- No --> H[throw Error\nbuild output missing]
G -- Yes --> I[syncBundledWorkspaceDist\ncopy dist + sanitize package.json]
J[bundleWorkspaceDeps.mjs] --> B
B --> K[createBundledWorkspaceBundles\nrepoRoot + targetRoot]
K --> L[bundleWorkspacePackages\nvendorBundledPackageRuntimeDependencies]
Last reviewed commit: "fix(cli): build rele..."
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
![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/workspaceBundleManifest.mjs (1)
3-3: Consider freezing the exported package list to prevent accidental mutation.Since this is the canonical source of truth, making it immutable avoids subtle cross-script side effects.
Suggested tweak
-export const bundledWorkspacePackages = ['agents', 'cli-common', 'protocol', 'release-runtime']; +export const bundledWorkspacePackages = Object.freeze(['agents', 'cli-common', 'protocol', 'release-runtime']);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/scripts/workspaceBundleManifest.mjs` at line 3, The exported array bundledWorkspacePackages should be made immutable to prevent accidental runtime mutation; update the export of bundledWorkspacePackages so the array is frozen (use Object.freeze on the array value or create a frozen copy) ensuring any attempts to push/pop will fail, and keep the export name bundledWorkspacePackages so other modules consume the same frozen list.apps/cli/scripts/buildSharedDeps.mjs (1)
109-121: Add validation toresolveBundledWorkspaceBuildEntryto guard against non-bundled main paths.The function currently accepts
package.json#mainwithout validating it remains withindist/. While all packages correctly point todist/entries today, a misconfiguredmain(e.g.,../src/index.js,./lib/index.js) would resolve successfully during the existence check but fail once packaged—since bundling only ships thedist/directory.Add a guard to reject or warn when
mainresolves outsidedist/, preventing silent runtime failures in published artifacts.🤖 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 109 - 121, In resolveBundledWorkspaceBuildEntry, validate packageJson.main so it cannot point outside the package's dist/ folder: after computing mainEntry from packageJson.main, resolve it to an absolute path (based on repoRoot/packages/pkg) and check that the resolved path has a path segment starting with the package's dist directory (e.g., ends with or contains "/packages/<pkg>/dist" or is a child of that dist directory); if the check fails, either fallback to the default dist entry (resolve(repoRoot, 'packages', pkg, 'dist', 'index.js')) and log or throw a clear error/warning via the existing logging mechanism — ensure this logic is applied before returning the resolved path so misconfigured mains like "../src/index.js" or "./lib/index.js" are rejected.
🤖 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 109-121: In resolveBundledWorkspaceBuildEntry, validate
packageJson.main so it cannot point outside the package's dist/ folder: after
computing mainEntry from packageJson.main, resolve it to an absolute path (based
on repoRoot/packages/pkg) and check that the resolved path has a path segment
starting with the package's dist directory (e.g., ends with or contains
"/packages/<pkg>/dist" or is a child of that dist directory); if the check
fails, either fallback to the default dist entry (resolve(repoRoot, 'packages',
pkg, 'dist', 'index.js')) and log or throw a clear error/warning via the
existing logging mechanism — ensure this logic is applied before returning the
resolved path so misconfigured mains like "../src/index.js" or "./lib/index.js"
are rejected.
In `@apps/cli/scripts/workspaceBundleManifest.mjs`:
- Line 3: The exported array bundledWorkspacePackages should be made immutable
to prevent accidental runtime mutation; update the export of
bundledWorkspacePackages so the array is frozen (use Object.freeze on the array
value or create a frozen copy) ensuring any attempts to push/pop will fail, and
keep the export name bundledWorkspacePackages so other modules consume the same
frozen list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 57a721ca-f914-4d7d-a14d-5351d2a26288
📒 Files selected for processing (4)
apps/cli/scripts/buildSharedDeps.contract.test.mjsapps/cli/scripts/buildSharedDeps.mjsapps/cli/scripts/bundleWorkspaceDeps.mjsapps/cli/scripts/workspaceBundleManifest.mjs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/cli/scripts/buildSharedDeps.mjs (1)
108-121: LGTM!The function correctly resolves the build entry with appropriate fallback behavior. The dependency injection for
readFileSyncandrepoRootmakes it testable.Optional nit: Line 116 validates that
mainis non-empty after trimming but uses the untrimmed value. For extra robustness, consider using the trimmed value:- const mainEntry = typeof packageJson?.main === 'string' && packageJson.main.trim() ? packageJson.main : './dist/index.js'; + const trimmedMain = typeof packageJson?.main === 'string' ? packageJson.main.trim() : ''; + const mainEntry = trimmedMain || './dist/index.js';🤖 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 108 - 121, In resolveBundledWorkspaceBuildEntry, capture and use the trimmed packageJson.main instead of reusing the untrimmed value: compute a trimmedMain (e.g., const trimmedMain = typeof packageJson?.main === 'string' ? packageJson.main.trim() : '') and then use trimmedMain for the truthy check and as the mainEntry value (falling back to './dist/index.js' when trimmedMain is empty); this ensures the returned path uses the sanitized main string.
🤖 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 108-121: In resolveBundledWorkspaceBuildEntry, capture and use the
trimmed packageJson.main instead of reusing the untrimmed value: compute a
trimmedMain (e.g., const trimmedMain = typeof packageJson?.main === 'string' ?
packageJson.main.trim() : '') and then use trimmedMain for the truthy check and
as the mainEntry value (falling back to './dist/index.js' when trimmedMain is
empty); this ensures the returned path uses the sanitized main string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 20cc027a-f107-42ac-b698-5a63a406cc6d
📒 Files selected for processing (1)
apps/cli/scripts/buildSharedDeps.mjs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
workspaceBundleManifest.mjsas single source of truth for the bundled workspace package listrelease-runtimein the shared workspace build list used byapps/cliWhy
@happier-dev/cliimports@happier-dev/release-runtimeat runtime and declares it as a bundled dependency, butapps/cli/scripts/buildSharedDeps.mjsonly builtagents,cli-common, andprotocol.That made CLI packaging depend on pre-existing local
packages/release-runtime/diststate. In practice this can make npm release artifacts nondeterministic and matches theERR_MODULE_NOT_FOUNDfailure reported in #103.Testing
node --test apps/cli/scripts/buildSharedDeps.contract.test.mjsnode --test apps/cli/scripts/prepack-script.test.mjsRefs #103
Summary by CodeRabbit
Tests
Refactor