Skip to content

fix(build): allow esbuild install scripts - #1196

Merged
clay-good merged 5 commits into
Fission-AI:mainfrom
ismet:fix/add-pnpm-workspace-config
Aug 5, 2026
Merged

fix(build): allow esbuild install scripts#1196
clay-good merged 5 commits into
Fission-AI:mainfrom
ismet:fix/add-pnpm-workspace-config

Conversation

@ismet

@ismet ismet commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Status

LGTM. Merge-ready after the current approval is recorded.

What was wrong

Source builds with pnpm 10/11 could reject esbuild's required install script. The original fix only worked with allowBuilds-aware pnpm versions (10.26+), left pnpm 11 unable to reproduce the lockfile's security overrides, and made pnpm --dir website install resolve to the root workspace instead of the website's independent lockfile.

How it was fixed

  • Approves only the locked esbuild@0.28.1 release for pnpm 10.26+/11.
  • Keeps the legacy package-level esbuild approval for pnpm 9 and pnpm 10.0-10.25.
  • Mirrors root and website security overrides into the workspace location required by pnpm 11.
  • Gives website/ its own workspace boundary so its package graph and lockfile remain independent.
  • Includes the root workspace policy in the Nix source and validation paths.
  • Adds regression tests that fail if build approvals, locked esbuild versions, security overrides, or workspace boundaries drift.

Replication / proof

Before hardening:

  • Strict pnpm 10.25 failed with ERR_PNPM_IGNORED_BUILDS.
  • pnpm 11.5.2 frozen root installs failed with ERR_PNPM_LOCKFILE_CONFIG_MISMATCH.
  • pnpm --dir website install installed the 287-package root graph instead of the 373-package website graph.

After hardening:

  • Clean root and website installs pass with pnpm 9.15.9, strict pnpm 10.25, pnpm 10.26+, and pnpm 11.5.2 without changing either lockfile.
  • Local build, type-check, lint, and all 3,665 tests pass.
  • Hosted Linux, macOS, Windows, Nix, audit, dependency review, website lockfile, release tracking, and automated review checks pass.

Notes / nits

This changes source-build policy only. It does not alter OpenSpec's runtime architecture, public API, or npm-installed package contents. The exact esbuild approval intentionally fails closed when the dependency version changes so the next upgrade must be reviewed.

Refs: #1195

pnpm 10+ blocks all dependency build scripts by default unless explicitly
approved via allowBuilds or onlyBuiltDependencies in pnpm-workspace.yaml.

esbuild (transitive dependency of vitest -> vite) has a postinstall script
that downloads a platform-specific native binary. Without this config,
pnpm install exits non-zero with [ERR_PNPM_IGNORED_BUILDS], breaking any
downstream packaging (AUR, Nix, Docker) or local setup using pnpm >=10.

Refs: Fission-AI#1195
@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This PR adds pnpm workspace configuration for the root and website packages. It permits esbuild builds, adds dependency overrides, updates CI and Nix file tracking, and adds configuration tests.

Changes

pnpm workspace policy

Layer / File(s) Summary
Define workspace configuration
pnpm-workspace.yaml, website/pnpm-workspace.yaml, package.json, website/package.json
The root and website configurations define workspace scope, permit esbuild builds, and set dependency overrides.
Track workspace configuration in CI and Nix
.github/workflows/ci.yml, .github/workflows/security.yml, flake.nix
CI, security, and Nix processing now includes pnpm-workspace.yaml changes.
Validate workspace policy
test/pnpm-workspace-config.test.ts
Tests validate workspace scope, build approvals, dependency overrides, versions, and integration references.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers: tabishb, alfred-openspec, clay-good

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the primary change: allowing esbuild install scripts to fix pnpm build installation failures.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

Copilot AI 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.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds a pnpm-workspace.yaml config intended to permit build scripts for esbuild.

Changes:

  • Introduces pnpm-workspace.yaml
  • Configures allowBuilds to allow esbuild builds

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pnpm-workspace.yaml Outdated
Comment on lines +1 to +2
allowBuilds:
esbuild: true

@alfred-openspec alfred-openspec left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verified the clean-install fix in a fresh checkout with pnpm 10.32.1: \ ERR_PNPM_NO_PKG_MANIFEST  No package.json found in /Users/alfred/.openclaw/workspace now allows esbuild's postinstall and \ ERR_PNPM_RECURSIVE_EXEC_NO_PACKAGE  No package found in this workspace passes. The config is scoped to the one dependency that needs a build script and should be included in source tarballs for downstream packagers. Looks good to merge.

@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

alfred-openspec
alfred-openspec previously approved these changes Aug 4, 2026

@alfred-openspec alfred-openspec left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed the current head in a fresh checkout with pnpm 10.32.1. The workspace root is declared, the build permission is scoped to esbuild, its postinstall runs successfully, and the build plus 31 focused tests pass. The full hosted matrix is green.

@clay-good clay-good changed the title Add pnpm-workspace.yaml to allow esbuild build scripts fix(build): allow esbuild install scripts Aug 4, 2026

@clay-good clay-good left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. The hardening closes the pnpm 9/10/11 policy gap, preserves root and website lockfile isolation, retains security overrides across pnpm generations, and includes exact-version drift tests. Verified clean installs across supported pnpm generations, all 3,665 local tests, and the complete hosted Linux/macOS/Windows/Nix/security matrix.

@alfred-openspec alfred-openspec left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed the workspace-policy hardening on the exact current head. Clean installs succeed under both the pinned pnpm 9.15.9 and pnpm 10.32.1, with esbuild 0.28.1 postinstall producing the native binary in both root and website projects. The duplicated override policies match both lockfiles, the focused policy tests and root build pass, both audits are clean, and the full hosted matrix is green.

@clay-good
clay-good added this pull request to the merge queue Aug 4, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 5, 2026
@clay-good
clay-good added this pull request to the merge queue Aug 5, 2026
Merged via the queue into Fission-AI:main with commit 3e50944 Aug 5, 2026
14 checks passed
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.

4 participants