Skip to content

fix(npm-globals): skip .exe bins in postinstall check, remove @sourcegraph/amp - #2044

Merged
shunkakinoki merged 2 commits into
mainfrom
fix-npm-globals-exe-remove-sourcegraph-amp
Jul 11, 2026
Merged

fix(npm-globals): skip .exe bins in postinstall check, remove @sourcegraph/amp#2044
shunkakinoki merged 2 commits into
mainfrom
fix-npm-globals-exe-remove-sourcegraph-amp

Conversation

@shunkakinoki

@shunkakinoki shunkakinoki commented Jul 11, 2026

Copy link
Copy Markdown
Owner

Summary

  • Fix `run_postinstall_if_needed` to skip `.exe` files when detecting native binaries - `.exe` files are Windows-only and should not count as a native macOS binary, which caused `@ampcode/cli`'s postinstall to be skipped
  • Remove `@sourcegraph/amp` (old package, replaced by `@ampcode/cli`)
  • Add `@ampcode/cli` to dependencies and trustedDependencies so `ing` manages it

@indent-zero

indent-zero Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor
PR Summary

Swaps @sourcegraph/amp for the underlying @ampcode/cli package it wraps and hardens the npm-globals installer's postinstall heuristic so Windows .exe bin shims (like the one @ampcode/cli ships) don't get mistaken for the platform-native binary on Darwin/Linux. Result: the amp CLI is still available on macOS/Linux, now fetched directly through @ampcode/cli's platform-specific optional deps via the installer's existing repair_native_optional_dep path.

  • home-manager/modules/npm-globals/install-npm-globals.sh: add *.exe to the case skip list in run_postinstall_if_needed so .exe bin entries don't set has_native=true.
  • package.json: remove @sourcegraph/amp, add @ampcode/cli@^0.0.1783729003-g78d5e5 in both dependencies and trustedDependencies.

Issues

1 potential issue found:

  • bun.lock is stale relative to package.json: still lists @sourcegraph/amp (removed) and pins the transitive @ampcode/cli@0.0.1783629102-g8185a2 while package.json now declares @ampcode/cli@^0.0.1783729003-g78d5e5 as a direct dep. Not a functional break (CI doesn't run bun install; the installer reconciles globals via its STALE path), but bun install should be run to regenerate the lockfile. → Autofix

CI Checks

Waiting for CI checks...


⚡ Autofix All Issues

@coderabbitai

coderabbitai Bot commented Jul 11, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@shunkakinoki, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 27 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f35a3a27-55fa-4d38-8028-fe6cfb63a3f9

📥 Commits

Reviewing files that changed from the base of the PR and between 2fdeefb and 6902173.

📒 Files selected for processing (2)
  • home-manager/modules/npm-globals/install-npm-globals.sh
  • package.json
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-npm-globals-exe-remove-sourcegraph-amp

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request removes the '@sourcegraph/amp' dependency from 'package.json' and updates the global npm installer script to ignore '.exe' files when checking for native binaries. The reviewer suggested expanding this exclusion to other Windows-specific executable and script extensions like '.cmd', '.bat', and '.ps1' to avoid false positives on non-native binaries.

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.

for f in "$bin_dir"/*; do
[ -f "$f" ] || continue
case "$f" in *.js | *.cjs | *.mjs) continue ;; esac
case "$f" in *.js | *.cjs | *.mjs | *.exe) continue ;; esac

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In addition to .exe files, many npm packages also include other Windows-specific wrapper scripts such as .cmd, .bat, and .ps1 in their bin directory. If any of these are present, they will be incorrectly detected as native macOS/Linux binaries, causing the postinstall script to be skipped. It is safer to skip all common Windows executable/script extensions.

Suggested change
case "$f" in *.js | *.cjs | *.mjs | *.exe) continue ;; esac
case "$f" in *.js | *.cjs | *.mjs | *.exe | *.cmd | *.bat | *.ps1) continue ;; esac

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

Re-trigger cubic

@mesa-dot-dev mesa-dot-dev Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performed full review of 2fdeefb...319bbfe

Analysis

• The binary detection heuristic in run_postinstall_if_needed relies on extension-based negative filtering (excluding .js, .exe, etc.), which remains fragile—extensionless scripts, shell wrappers (.sh), or metadata files could still be misclassified as native binaries. A more robust long-term approach would use explicit native binary format detection (magic headers, file command, or known native extensions like .node) instead of exclusion-based logic.

• Removing @sourcegraph/amp assumes no hidden transitive references, config dependencies, or external references (scripts, CI automation, documentation, shell aliases) pointing to the old package or binary name elsewhere in the codebase. If such references exist, users will encounter "command not found" errors post-deployment; verification of complete removal across all reference points is required before merging.

• The fix correctly addresses the cross-platform bug (.exe exclusion for macOS/Linux), but the change assumes this script will not run on Windows hosts. If executed on Windows in the future, ignoring .exe files could cause under-detection of native binaries; clarify platform constraints and document this assumption if the script's execution scope may expand.

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 | 0 comments | Edit Agent SettingsRead Docs

@mesa-dot-dev

mesa-dot-dev Bot commented Jul 11, 2026

Copy link
Copy Markdown

Mesa Description

TL;DR

Fixed npm-globals postinstall detection to ignore .exe files and replaced outdated @sourcegraph/amp package with @ampcode/cli.

What changed?

  • home-manager/modules/npm-globals/install-npm-globals.sh:
    • Updated run_postinstall_if_needed to skip .exe files when checking for native binaries, ensuring macOS postinstall tasks are not incorrectly skipped.
  • package.json:
    • Replaced @sourcegraph/amp with @ampcode/cli (version ^0.0.1783729003-g78d5e5) in dependencies and trustedDependencies.

Description generated by Mesa. Update settings

@shunkakinoki
shunkakinoki merged commit 69e3352 into main Jul 11, 2026
30 of 32 checks passed
@shunkakinoki
shunkakinoki deleted the fix-npm-globals-exe-remove-sourcegraph-amp branch July 11, 2026 00:57
@indent-zero indent-zero Bot mentioned this pull request Jul 11, 2026
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant