Skip to content

chore(pnpm): move pnpm settings to pnpm-workspace.yaml - #11736

Closed
Gldywn wants to merge 1 commit into
stablyai:mainfrom
Gldywn:chore-pnpm-settings-workspace-yaml
Closed

Gldywn wants to merge 1 commit into
stablyai:mainfrom
Gldywn:chore-pnpm-settings-workspace-yaml

Conversation

@Gldywn

@Gldywn Gldywn commented Jul 31, 2026 •

Copy link
Copy Markdown
Contributor

Summary

package.json still carries a pnpm field, but the version this repo pins — "packageManager": "pnpm@10.24.0" — no longer reads it. pnpm says so on every command:

The "pnpm" field in package.json is no longer read by pnpm. The following keys were ignored:
"pnpm.overrides", "pnpm.supportedArchitectures", "pnpm.onlyBuiltDependencies", "pnpm.patchedDependencies".

All four settings are ignored at their source. What is left is a lockfile that happens to remember two of them:

Setting Recorded in pnpm-lock.yaml? Exposure
overrides yes survives only while installs stay --frozen-lockfile
patchedDependencies yes same
onlyBuiltDependencies no no safety net at all
supportedArchitectures no no safety net at all

So the repo is one lockfile regeneration away from silently dropping all five patches in config/patches/ — including the node-pty patch whose .symver pins exist to hold the Ubuntu 20.04 / glibc 2.31 floor documented in AGENTS.md. supportedArchitectures is already unprotected today, and it is what makes a build machine fetch the other platforms' optional binaries rather than only the host's.

This PR moves all four keys into pnpm-workspace.yaml, where pnpm 10 reads them, and deletes the dead package.json field. packages: [] and its explanatory comment are preserved verbatim — that entry exists to stop pnpm -r crawling mobile/ and breaking patches with ERR_PNPM_UNUSED_PATCH.

No dependency specifier changes. pnpm-lock.yaml is untouched.

Screenshots

No visual change — build and dependency configuration only.

Testing

  • pnpm install — full install run and verified, see below
  • pnpm lint / pnpm typecheck / pnpm test / pnpm build — not meaningful signals for this change; it modifies no source. The evidence that matters is the install itself.
  • Added or updated high-quality tests that would catch regressions, or explained why tests were not needed — no test is added: this is configuration relocation, and its correctness is proven by install behavior, not by unit tests

The headline evidence: the lockfile did not move. The verification install deliberately did not use --frozen-lockfile, so pnpm was free to rewrite pnpm-lock.yaml, and it chose not to. The file is byte-identical to its pre-change state (blob 491c536773bea156b9c5a5ccfcd20f86f875cdfd, empty git diff). Relocating the settings changed resolution in no way.

Behaviorally confirmed after the install:

  • patchedDependencies — all 5 patches applied, patch_hash directories matching the lockfile hashes exactly, and the Orca .symver marker present in node_modules/node-pty/binding.gyp.
  • overrides — both pins resolved (@hono/node-server 2.0.10, dompurify 3.4.12), single copy each.
  • supportedArchitectures — full cross-platform matrix materialized: esbuild 6/6, @parcel/watcher 8/8 including Linux glibc and musl, sherpa-onnx 5/5.

One honest non-pass. onlyBuiltDependencies is NOT behaviorally proven. .modules.yaml shows ignoredBuilds: [], which is a good sign that nothing was blocked, but pendingBuilds is non-empty and cpu-features produced no .node. The reason is worth stating because it changes which CI check matters: node-pty ships prebuilds for darwin-arm64, darwin-x64, win32-arm64 and win32-x64, and none for Linux. Linux is therefore the only OS that compiles it from source, and the only place the build allowlist and the .symver pins are ever exercised. A green macOS or Windows run proves nothing about the one path that matters here.

Key names and value shapes were checked against the pnpm v10 documentation (settings.md, cli/patch.md) and the machine-readable SchemaStore pnpm Workspace Specification. This mattered more than it sounds: pnpm's config surface is a passthrough store that echoes back deliberately misspelled keys unchanged and performs no shape validation, so a typo would reproduce the original bug with no warning at all. patchedDependencies is the one key whose recognition could be proven behaviorally rather than by documentation — pnpm resolves its values to absolute paths, while a deliberately misspelled patchDependencies keeps its raw relative string.

AI Review Report

The main risk reviewed was the silently-wrong key name: a misspelling relocates the settings into a file pnpm reads but under names it ignores, reproducing the exact bug this PR fixes while looking correct and emitting no warning. That drove the negative-control test above and the decision to verify against both the official docs and the published JSON schema rather than trusting that the settings appeared to load.

Second risk reviewed: unintended dependency movement hiding inside a settings relocation. Addressed by running a non-frozen install and showing the lockfile is byte-identical, which is stronger evidence than a frozen install could produce.

Third: losing packages: []. Confirmed preserved with its comment; the pnpm-workspace.yaml diff is purely additive.

Cross-platform (macOS, Linux, Windows): central to this change. supportedArchitectures is what makes any build host fetch all three platforms' optional binaries, and it is currently ignored. The full matrix was verified to materialize after the change. Per the note above, Linux is the highest-value runner here because it is the only OS where node-pty compiles from source.

SSH / remote / local: no runtime code touched; this affects install-time dependency resolution only.

Agent / integration / git-provider compatibility: not touched.

Performance: no runtime impact.

UI quality: not applicable.

Security Audit

Low risk, and mildly risk-reducing. No dependency specifier, version or source changes: both overrides pins are preserved exactly, which matters because monaco-editor>dompurify and @modelcontextprotocol/sdk>@hono/node-server are pinned overrides of transitive dependencies. The security-relevant improvement is that patchedDependencies and onlyBuiltDependencies now come from a file pnpm actually reads, rather than depending on the lockfile happening to remember them — a lockfile regeneration currently drops the patches silently.

onlyBuiltDependencies is an allowlist of packages permitted to run install scripts. It being ignored is a live weakness, and it is the one item this PR cannot yet prove is fixed (see Testing). No secrets, auth, IPC or input handling involved.

Notes

This is a CI-validated change: the local install proves it on macOS only, and the finding above means the Linux runner is the check that matters, not one of three.

Follow-up, out of scope here: pnpm v11 removes onlyBuiltDependencies in favour of allowBuilds. The next pnpm major will need that rename, or native build scripts will stop running with no warning — the same silent-failure shape this PR is fixing.

The pnpm warning quoted in Summary is field-dependent and observed to disappear in this repo's worktrees once the field is removed. The emitting layer could not be isolated locally, because this machine's pnpm launcher is itself broken — which is the subject of the related PR below.

Related

Independent of, and mergeable in any order with, the build:native fix in the linked PR below. Same theme — this repo's pnpm integration drifting from the behavior of the pinned pnpm@10.24.0 — but no dependency between them, and no shared files.

See #11738.

pnpm 10.24.0 (our packageManager pin) no longer reads the `pnpm` field in
package.json and warns that overrides, supportedArchitectures,
onlyBuiltDependencies and patchedDependencies were ignored.

overrides and patchedDependencies were still recorded in pnpm-lock.yaml, so
they survived only via --frozen-lockfile installs. onlyBuiltDependencies and
supportedArchitectures had no lockfile backing at all: the lockfile settings
block carries only autoInstallPeers and excludeLinksFromLockfile. Any lockfile
regeneration would have silently dropped the 5 patches in config/patches/ and
narrowed optional-dependency resolution to the host platform, which breaks
cross-platform packaging for a project shipping macOS, Linux and Windows.

Move all four keys verbatim into pnpm-workspace.yaml (pnpm 10 schema) and drop
the dead field. packages: [] and its comment are unchanged.

Verified: pnpm resolved 0 of 4 settings before, 4 of 4 after, with values
round-tripping 1:1. A negative control showed pnpm echoes misspelled keys back
identically, so key names were checked against the pnpm v10 docs and the
published pnpm-workspace JSON schema rather than inferred from that output.

Patch application, override resolution and the cross-platform optional-dep
matrix still need a real install and CI on all 3 OSes.
@Gldywn
Gldywn marked this pull request as ready for review July 31, 2026 08:33
@greptile-apps

greptile-apps Bot commented Jul 31, 2026

Copy link
Copy Markdown

Greptile Summary

Relocates four pnpm configuration keys (overrides, supportedArchitectures, onlyBuiltDependencies, patchedDependencies) from the pnpm field in package.json — where pnpm 10 silently ignores them — into pnpm-workspace.yaml, the file pnpm 10 actually reads for workspace settings.

  • All five patch paths in patchedDependencies are verified to exist under config/patches/, key names match the pnpm 10 schema exactly, and the packages: [] sentinel that prevents ERR_PNPM_UNUSED_PATCH is preserved with its explanatory comment.
  • pnpm-lock.yaml is unchanged, confirming the move is semantically equivalent to the prior (broken) state where overrides and patches were already frozen into the lockfile from a previous install.
  • The PR correctly notes that onlyBuiltDependencies will need renaming to allowBuilds when the repo upgrades to pnpm v11, but that is out of scope here and onlyBuiltDependencies remains the correct key for the pinned pnpm@10.24.0.

Confidence Score: 5/5

Safe to merge — this is a pure configuration relocation with no dependency specifier changes and a byte-identical lockfile.

Every migrated key name is verified against the pnpm 10 schema. All five patch file paths exist on disk and match the new configuration exactly. The packages: [] guard that prevents ERR_PNPM_UNUSED_PATCH is preserved. The change moves settings from a location pnpm 10 actively ignores to the location it actively reads, making the install behavior more reliable rather than less.

Files Needing Attention: No files require special attention. The two-file change is self-contained and the only meaningful follow-up (onlyBuiltDependencies → allowBuilds for pnpm v11) is already flagged in the PR description as a future task.

Important Files Changed

Filename Overview
package.json Removes the pnpm field (overrides, supportedArchitectures, onlyBuiltDependencies, patchedDependencies) that pnpm 10 ignores when read from package.json; no other changes.
pnpm-workspace.yaml Receives all four migrated settings; existing packages: [] and its comment are preserved; all key names, value shapes, and patch paths match the pnpm 10 spec and the actual files under config/patches/.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[pnpm install] --> B{Where does pnpm 10 read settings?}
    B -->|Before this PR| C["package.json pnpm field IGNORED by pnpm 10"]
    B -->|After this PR| D["pnpm-workspace.yaml actively read by pnpm 10"]

    C --> E["overrides, onlyBuiltDependencies,\nsupportedArchitectures, patchedDependencies\nall silently ignored"]

    D --> F["overrides\nhono/node-server and dompurify pins"]
    D --> G["supportedArchitectures\ndarwin, linux, win32 x64 and arm64"]
    D --> H["onlyBuiltDependencies\nnode-pty, esbuild, parcel/watcher, etc"]
    D --> I["patchedDependencies\n5 patches under config/patches"]

    I --> J["node-pty@1.1.0 symver glibc 2.31 floor"]
    I --> K["xterm addon patches x4"]
Loading

Reviews (1): Last reviewed commit: "chore(pnpm): move pnpm settings to pnpm-..." | Re-trigger Greptile

@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6f9da0e8-9240-4025-85dd-35287678559c

📥 Commits

Reviewing files that changed from the base of the PR and between 9d473c8 and 6a80902.

📒 Files selected for processing (2)
  • package.json
  • pnpm-workspace.yaml
💤 Files with no reviewable changes (1)
  • package.json

📝 Walkthrough

Walkthrough

The pnpm configuration was removed from package.json. The configuration was added to pnpm-workspace.yaml, including dependency overrides, supported OS and CPU architectures, approved native build dependencies, and patches for node-pty and xterm.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes moving pnpm settings from package.json to pnpm-workspace.yaml.
Description check ✅ Passed The description covers all required sections, explains testing limitations, and documents cross-platform, security, and follow-up considerations.
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.

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.

@nwparker

Copy link
Copy Markdown
Contributor

Closing as fully superseded by merged #17156 (b17f60d744c05a58ee054260878af1dd8cdd3afb). I checked this PR’s complete two-file diff: its entire scope is relocating the four pnpm settings into the workspace manifest. The merged pnpm 12 upgrade performs that relocation, including the required conversion from onlyBuiltDependencies to allowBuilds; later dependency versions and architecture policy have continued to evolve there.

Current install/package contract suites plus the cancelled-preedit IME suite pass (19 tests). No merge of this older pnpm 10 configuration is needed. Tracking report #15601 is also resolved by the merged upgrade.

@nwparker nwparker closed this Oct 18, 2026
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