Skip to content

fix(ci): bootstrap npm from tarball when runner's bundled npm is broken #3010

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
amikofalvy merged 1 commit into from
Apr 4, 2026
+16 −1
Merged

fix(ci): bootstrap npm from tarball when runner's bundled npm is broken #3010

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/frightened-white-guineafowl.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@inkeep/agents-core": patch
---

Fix release workflow npm bootstrap for OIDC publishing
Copy link
Copy Markdown
Contributor

@pullfrog pullfrog bot Apr 4, 2026

Choose a reason for hiding this comment

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

This changeset should be removed. The change is to .github/workflows/release.yml — internal CI tooling, not published code in @inkeep/agents-core. Per repo conventions, internal tooling/scripts changes do not get changesets. Merging this would trigger a spurious patch release of agents-core with zero code changes to the package.

If the goal is to trigger a publish run to verify the fix, a workflow_dispatch on the release workflow achieves the same thing without a no-op version bump.

Comment on lines +1 to +5
Copy link
Copy Markdown
Contributor

@claude claude bot Apr 4, 2026

Choose a reason for hiding this comment

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

🟠 MAJOR: Changeset should not exist for CI-only changes

Issue: This changeset attributes a CI workflow fix to @inkeep/agents-core, but per AGENTS.md, changesets should NOT be created for "Internal tooling/scripts changes".

Why: Creating a changeset for this workflow-only change will trigger an unnecessary patch version bump of @inkeep/agents-core. This creates changelog noise and version churn for consumers who see no actual code change in the package.

Fix: Delete this changeset file. The workflow fix does not require a version bump of any package.

From AGENTS.md:

When NOT to create a changeset:

  • Documentation-only changes
  • Test-only changes
  • Internal tooling/scripts changes
  • Changes to ignored packages

12 changes: 11 additions & 1 deletion .github/workflows/release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,17 @@ jobs:
registry-url: "https://registry.npmjs.org"

- name: Ensure npm 11.5.1+ for OIDC support
run: npm install -g npm@latest
run: |
if npm install -g npm@latest 2>/dev/null; then
Copy link
Copy Markdown
Contributor

@claude claude bot Apr 4, 2026

Choose a reason for hiding this comment

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

🟠 MAJOR: Error suppression hides legitimate failures

Issue: The 2>/dev/null redirection discards all stderr output, hiding legitimate errors unrelated to the broken npm issue (network failures, permission issues, registry auth problems).

Why: When the primary upgrade path fails for any reason other than the specific MODULE_NOT_FOUND bug, the workflow silently falls back to the tarball path. This masks real problems and makes debugging harder.

Fix: Consider checking for the specific error pattern instead of blanket suppression:

if npm install -g npm@latest 2>&1 | tee /tmp/npm-upgrade.log; then
  echo "npm upgraded via bundled npm"
elif grep -qE 'MODULE_NOT_FOUND|Cannot find module' /tmp/npm-upgrade.log; then
  echo "::warning::Bundled npm broken, bootstrapping from tarball"
  # ... fallback logic
else
  echo "::error::npm upgrade failed for unknown reason"
  cat /tmp/npm-upgrade.log
  exit 1
fi

Refs:

echo "npm upgraded via bundled npm"
else
echo "::warning::Bundled npm broken, bootstrapping from tarball"
NPM_VER=$(node -e "fetch('https://registry.npmjs.org/npm/latest').then(r=>r.json()).then(d=>console.log(d.version))")
Copy link
Copy Markdown
Contributor

@pullfrog pullfrog bot Apr 4, 2026

Choose a reason for hiding this comment

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

If fetch fails (network error, registry outage, non-JSON response), NPM_VER will be empty and the subsequent curl + install commands will silently produce garbage URLs or cryptic errors. Add a guard:

Suggested change
NPM_VER=$(node -e "fetch('https://registry.npmjs.org/npm/latest').then(r=>r.json()).then(d=>console.log(d.version))")
NPM_VER=$(node -e "fetch('https://registry.npmjs.org/npm/latest').then(r=>r.json()).then(d=>console.log(d.version))")
if [ -z "$NPM_VER" ]; then echo '::error::Failed to resolve npm version from registry'; exit 1; fi
Suggested change
NPM_VER=$(node -e "fetch('https://registry.npmjs.org/npm/latest').then(r=>r.json()).then(d=>console.log(d.version))")
NPM_VER=$(node -e "fetch('https://registry.npmjs.org/npm/latest').then(r=>r.json()).then(d=>console.log(d.version))")
if [ -z "$NPM_VER" ]; then echo '::error::Failed to resolve npm version from registry'; exit 1; fi

Copy link
Copy Markdown
Contributor

@claude claude bot Apr 4, 2026

Choose a reason for hiding this comment

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

🟡 Minor: Consider using curl for version lookup

Issue: The fallback uses node -e "fetch(...)" which requires Node.js 18+ with native fetch. While the workflow specifies Node 22, using curl would be more robust and consistent with the tarball download.

Why: This avoids any Node.js environment dependency in the fallback path. If npm is broken, other aspects of the Node environment might also be affected.

Fix:

Suggested change
NPM_VER=$(node -e "fetch('https://registry.npmjs.org/npm/latest').then(r=>r.json()).then(d=>console.log(d.version))")
NPM_VER=$(curl -fsSL https://registry.npmjs.org/npm/latest | jq -r .version)

Refs:

  • The runner already has jq available for JSON parsing

curl -fsSL "https://registry.npmjs.org/npm/-/npm-${NPM_VER}.tgz" | tar xz -C /tmp
node /tmp/package/bin/npm-cli.js install -g "npm@${NPM_VER}"
rm -rf /tmp/package
fi
echo "npm $(npm --version)"

- name: Setup Turborepo cache
uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 # v4
Expand Down
Loading