chore: migrate node-pty to beta version #1144

zerob13 · 2025-12-01T02:42:41Z

Summary by CodeRabbit

Chores
- Updated PTY library dependency to improve compatibility and maintainability.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-12-01T02:42:51Z

Walkthrough

This pull request replaces the PTY library dependency from @homebridge/node-pty-prebuilt-multiarch to node-pty across the project's package configuration and source imports. No behavioral changes; only package references are updated.

Changes

Cohort / File(s)	Summary
PTY Library Migration `package.json`, `src/main/presenter/configPresenter/acpInitHelper.ts`	Replaced PTY library dependency from `@homebridge/node-pty-prebuilt-multiarch` to `node-pty`. Updated `dependencies` entry, `pnpm.ignoredBuiltDependencies` list, and import statements (both `spawn` function and `IPty` type import).

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Verify that node-pty provides API compatibility with the @homebridge/node-pty-prebuilt-multiarch package, specifically the spawn function and IPty type interface
Confirm no other files in the codebase import from the old package

Poem

A rabbit hops through package trees,
Trading prebuilt for PTY with ease,
One name swapped for another so neat,
The code still runs, no skipped a beat! 🐰✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: migrating from @homebridge/node-pty-prebuilt-multiarch to node-pty, which is characterized as a beta version migration.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch chore/migrate-to-node-pty-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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

package.json (1)

195-210: Remove node-pty from pnpm.ignoredBuiltDependencies—native rebuild is unreliable

Your configuration fails in practice: pnpm install failed with node-gyp failed to rebuild '/node_modules/node-pty' because Python was unavailable during the electron-builder install-app-deps rebuild step. Since node-pty is actively used in the main process for spawning agent PTYs, ignoring its build scripts and relying on electron-builder install-app-deps to rebuild it is fragile and can break in environments where Python or build tools are missing.

Remove node-pty from pnpm.ignoredBuiltDependencies and let its install scripts run normally, or ensure Python and build tools are always available in your CI/dev environments. Removing it is the simpler, more reliable approach.

🧹 Nitpick comments (2)

package.json (2)
60-102: node-pty dependency: API/ABI look compatible, but consider tightening the beta range

Switching from the prebuilt fork to the official node-pty with:
"node-pty": "^1.1.0-beta39"
matches the existing usage pattern (spawn(...), IPty) and is compatible with your current toolchain (Node >= 20.19, Electron ^37.8.0), as node-pty officially supports Node ≥ 16 and Electron ≥ 19. (github.com)

Two things to double‑check:

Using a caret with a beta tag (^1.1.0-beta39) can jump across future beta builds, which may introduce breaking changes; for betas it’s usually safer to pin the exact version.

Confirm that all platforms you ship to (Windows/macOS/Linux, including arm64 variants) are covered by the prebuilt/binary story for this node-pty line, or that your build pipeline reliably recompiles it (see the next comment about ignoredBuiltDependencies).

If stability is a priority, consider changing this to an exact version:
-    "node-pty": "^1.1.0-beta39",
+    "node-pty": "1.1.0-beta39",
8-13: Engines field matches AGENTS/CLAUDE docs but differs from generic “Node ≥ 22” guideline

The current engines/toolchain:
"engines": {
  "node": ">=20.19.0",
  "pnpm": ">=10.11.0"
},
"packageManager": "pnpm@10.13.1+sha512-..."
aligns with the documented setup in AGENTS/CLAUDE (Node ≥ 20.19, pnpm ≥ 10.11). Based on learnings, this is consistent with the project’s current instructions.

However, the higher‑level coding guidelines also mention “Node.js ≥ 22 required” for package.json. To avoid confusion for contributors, it might be worth confirming which requirement is authoritative and updating either the docs or this field accordingly (no need to block this PR on it).

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 80896e9 and 91e7521.

📒 Files selected for processing (2)

package.json (2 hunks)
src/main/presenter/configPresenter/acpInitHelper.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (15)

**/*.{ts,tsx,js,jsx,vue}