fix(npm-globals): re-run postinstall after native binary repair - #2046
Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe npm globals installer now runs per-package postinstall hooks after successful native optional dependency repairs in both existing-version and fresh-install paths. ChangesNPM native dependency repair
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates install-npm-globals.sh to trigger run_postinstall_if_needed immediately after a native optional dependency is successfully repaired. The reviewer noted that run_postinstall_if_needed might incorrectly skip running the postinstall script if the package uses extensionless shell script wrappers or fallback stubs in its bin/ directory. They suggested adding a force parameter to bypass the native check when a repair has just occurred.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # re-triggers the same bun transitive-optional drop. | ||
| if repair_native_optional_dep "$dep"; then | ||
| echo "$dep native binary repaired in place" | ||
| run_postinstall_if_needed "$dep" |
There was a problem hiding this comment.
If a package uses an extensionless shell script wrapper or fallback stub in its bin/ directory (which is very common), run_postinstall_if_needed will incorrectly identify it as a native binary (since it doesn't match .js, .cjs, .mjs, or .exe) and skip running the postinstall script.
Since we just successfully repaired the native optional dependency, we should force the postinstall script to run to ensure any stubs are correctly replaced and the native binary is properly linked.
We can add a force parameter to run_postinstall_if_needed to bypass the has_native check when we know a repair has just occurred:
run_postinstall_if_needed() {
local dep="$1"
local force="${2:-false}"
...
if [ "$force" != "true" ] && [ -d "$bin_dir" ]; then
...| run_postinstall_if_needed "$dep" | |
| run_postinstall_if_needed "$dep" true |
| if missing_native_optional_dep "$dep"; then | ||
| repair_native_optional_dep "$dep" || echo "Native binary repair failed: $dep" >&2 | ||
| if repair_native_optional_dep "$dep"; then | ||
| run_postinstall_if_needed "$dep" |
There was a problem hiding this comment.
Similarly to the repair path above, we should force the postinstall script to run here to bypass the has_native check, ensuring that any extensionless fallback stubs or wrappers are correctly replaced by the postinstall script after the native binary is repaired.
| run_postinstall_if_needed "$dep" | |
| run_postinstall_if_needed "$dep" true |
![mesa-dot-dev[bot]](https://avatars.githubusercontent.com/in/1050077?s=60&v=4){kind=small}
There was a problem hiding this comment.
Performed full review of b14d903...f0526de
Analysis
• Error handling for postinstall failures is implicit and non-standardized: After repair_native_optional_dep succeeds, run_postinstall_if_needed is called but the consequences of its failure are unclear (silent inconsistency vs. flow abort). Error policy must be explicit with appropriate logging to prevent users from silently ending with repaired natives but broken shims.
• Idempotency of postinstall is assumed but undocumented: The increased invocation frequency of run_postinstall_if_needed requires strict idempotency of wrapper postinstall scripts. This constraint should be documented and verified across all globally managed packages to prevent state corruption from repeated runs.
• Repair+postinstall coupling risk for future drift: Having two separate call sites that must both pair repair_native_optional_dep with run_postinstall_if_needed creates maintenance burden and risk of omission in future code paths. Missing this coupling elsewhere in the codebase would silently reintroduce the lifecycle gap. Consider centralizing this into a single repair_and_reconcile_native_dep primitive.
• Limited scope visibility: The fix covers both repair paths within this file, but other scripts or functions outside this file that call repair_native_optional_dep or mutate optional native deps may lack postinstall reconciliation, creating inconsistency across the broader codebase.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
0 files reviewed | 2 comments | Edit Agent Settings • Read Docs
| if missing_native_optional_dep "$dep"; then | ||
| repair_native_optional_dep "$dep" || echo "Native binary repair failed: $dep" >&2 | ||
| if repair_native_optional_dep "$dep"; then | ||
| run_postinstall_if_needed "$dep" |
There was a problem hiding this comment.
Same concern as the other call site: postinstall rerun failure should be explicitly handled and logged. Without this, a silent failure in run_postinstall_if_needed could leave the wrapper in a stale state despite successful native binary repair. Consider: run_postinstall_if_needed "$dep" || echo "Warning: postinstall reconciliation failed for $dep" >&2
Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#2046
File: home-manager/modules/npm-globals/install-npm-globals.sh#L312
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
Same concern as the other call site: postinstall rerun failure should be explicitly handled and logged. Without this, a silent failure in `run_postinstall_if_needed` could leave the wrapper in a stale state despite successful native binary repair. Consider: `run_postinstall_if_needed "$dep" || echo "Warning: postinstall reconciliation failed for $dep" >&2`
| # re-triggers the same bun transitive-optional drop. | ||
| if repair_native_optional_dep "$dep"; then | ||
| echo "$dep native binary repaired in place" | ||
| run_postinstall_if_needed "$dep" |
There was a problem hiding this comment.
Consider adding explicit error handling for run_postinstall_if_needed failure. If postinstall fails after successful native binary repair, the system could end up in an inconsistent state (native dep present but wrapper shim still broken) with no clear diagnostic signal. Recommendation: capture the exit status and log a warning if postinstall reconciliation fails, e.g., run_postinstall_if_needed "$dep" || echo "Warning: postinstall reconciliation failed for $dep" >&2
Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#2046
File: home-manager/modules/npm-globals/install-npm-globals.sh#L280
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
Consider adding explicit error handling for `run_postinstall_if_needed` failure. If postinstall fails after successful native binary repair, the system could end up in an inconsistent state (native dep present but wrapper shim still broken) with no clear diagnostic signal. Recommendation: capture the exit status and log a warning if postinstall reconciliation fails, e.g., `run_postinstall_if_needed "$dep" || echo "Warning: postinstall reconciliation failed for $dep" >&2`
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| if missing_native_optional_dep "$dep"; then | ||
| repair_native_optional_dep "$dep" || echo "Native binary repair failed: $dep" >&2 | ||
| if repair_native_optional_dep "$dep"; then | ||
| run_postinstall_if_needed "$dep" |
There was a problem hiding this comment.
Postinstall runs twice on fresh installs: run_postinstall_if_needed "$dep" was already invoked at line 307 immediately after bun add. When bun drops the transitive optional dep and we then repair_native_optional_dep, we call it a second time here. For postinstalls that unconditionally hit the network to fetch a platform binary, that's a duplicate download per fresh install.
Consider skipping the earlier call when a native repair is likely to fire, or short-circuit the second call if the first one already populated the wrapper's bin/.
Mesa DescriptionTL;DRRe-runs the wrapper package's postinstall script after successful repair of missing native optional dependencies, ensuring that binaries are correctly linked rather than leaving error-stub shims in place. What changed?
Description generated by Mesa. Update settings |
1 participant
Summary
repair_native_optional_depinstalls a missing native binary (e.g.@ampcode/cli-darwin-arm64), the wrapper's postinstall wasn't re-run, leaving the error-stub shim (~/.bun/bin/amp) in place@ampcode/cliuse apostinstallscript (install.cjs) to hard-link the native binary intobin/amp.exe- this must run after the native package is presentrun_postinstall_if_neededcall after successfulrepair_native_optional_depin both code paths: the "already installed but native missing" path and the "fresh install with native repair" pathRoot cause
~/.bun/bin/ampis a symlink to@ampcode/cli/bin/amp.exe. When bun installs@ampcode/cliwithout its@ampcode/cli-darwin-arm64optional dep, the postinstall writes a fallback error stub tobin/amp.exe. The script then installs the native binary viarepair_native_optional_depbut never re-ran postinstall, so the stub persisted andampkept printing "Amp native binary not installed."Test plan
make switch-ampshould work after activation without manual interventionamp --versionoutputs a version string rather than the error messageSummary by cubic
Re-runs postinstall for npm global wrappers after a native optional dependency is repaired, replacing the error-stub shim with the real binary. Fixes
@ampcode/cliso~/.bun/bin/ampworks right after activation.run_postinstall_if_needed "$dep"after successfulrepair_native_optional_depin both paths (existing wrapper with missing native, and fresh install with immediate repair).@ampcode/clilinks its native package (e.g.,@ampcode/cli-darwin-arm64) intobin/amp.exe, removing the "Amp native binary not installed" stub.Written for commit f0526de. Summary will update on new commits.