Complete node-pty removal (fixes #204)#227
Closed
khaliqgant wants to merge 12 commits into
Closed
Conversation
Change .nvmrc from exact version v22.0.0 to major version 22, allowing any 22.x.x version to be used. This fixes compatibility issues for users running different minor/patch versions of Node 22. Fixes #204
- Remove node-pty from package.json dependencies - Update spawner.ts to require relay-pty binary (no fallback) - Simplify AgentWrapper type to only use RelayPtyOrchestrator This improves Node.js version compatibility by removing the native node-pty module that requires compilation for each Node version. The relay-pty Rust binary provides PTY functionality instead. Related to #204
BREAKING: Removes node-pty native module dependency and migrates all PTY functionality to use the relay-pty Rust binary. Changes: - Remove node-pty from package.json dependencies - Update spawner.ts to require relay-pty binary (no fallback) - Migrate cli-pty-runner.ts to use relay-pty via child_process.spawn - Migrate cli-auth.ts to use relay-pty for auth flows - Update onboarding.ts to remove IPty type dependency - Create wrapper-types.ts for shared event type definitions - Update imports in agent-manager.ts, persistence.ts to use wrapper-types.ts This improves Node.js version compatibility (including Node 25) by eliminating native module compilation requirements. The relay-pty Rust binary provides equivalent PTY functionality without version constraints. The relay-pty binary must be available for: - Agent spawning via Spawner - CLI authentication flows - CLI PTY runner operations Fixes #204
Add GitHub Actions workflow that tests npm install across multiple Node.js versions (18, 20, 22, 24, 25) using Docker containers. Tests include: - Full install and build in official Node images - npm pack and global installation (npm install -g) - ESM import verification - Minimal install in slim images (no build tools) This ensures the package installs correctly after removing the node-pty native module dependency. Related to #204
Replace references to node-pty with relay-pty in: - spawner.ts file header and spawn method doc - spawner.test.ts file header
The PtyWrapper class is no longer used after migrating to RelayPtyOrchestrator. Remove the file to fix TypeScript compilation errors from the missing node-pty dependency. Event types have been moved to wrapper-types.ts.
- Use local import (./dist/index.js) instead of global package import - Add npm test to verify functionality after build - Remove problematic global install test - Add fresh install test (deletes package-lock.json first) - Verify native modules compile correctly
Update test scripts in scripts/test-cli-auth/ to use relay-pty binary instead of node-pty: - ci-test-runner.ts: Migrated to child_process.spawn with relay-pty - test-oauth-flow.ts: Migrated to child_process.spawn with relay-pty - package.json: Removed node-pty dependency This completes the removal of node-pty from the entire codebase.
Remove scripts/test-pty-input.js and scripts/test-pty-input-auto.js which were manual debugging scripts that used node-pty. These are not part of the test suite and would fail without node-pty.
Add multi-stage Docker build that compiles relay-pty from source: - Stage 1: Build relay-pty using rust:1.75-slim - Stage 2: Copy binary to runtime image This fixes the CLI OAuth tests which now depend on relay-pty instead of node-pty for PTY emulation.
Add overrides for react and react-dom to resolve peer dependency conflict. drizzle-orm@0.45.1 has optional peer dependencies on React Native packages (expo-sqlite, @op-engineering/op-sqlite) that require React 19, but this web project uses React 18.3.1. The overrides tell npm to use React 18 regardless of what the optional React Native packages want. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The claude/fix-issue-204-uNZ6y branch removed node-pty but had two issues: 1. Double-formatting bug: Rust protocol.rs removed pre-formatted message detection, but Node.js buildInjectionString() still formats messages. This caused messages to be wrapped twice with "Relay message from...". Fix: Restored the pre-formatted check in format_for_injection() that skips formatting when body already starts with "Relay message from ". 2. Debug statements: shared.ts had console.log debug statements that should not be in production code. Fix: Removed debug console.log statements from buildInjectionString(). Also restored the removed tests for pre-formatted message handling. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 My Senior Dev — Analysis Complete👤 For @khaliqgant📁 Expert in View your contributor analytics → 📊 22 files reviewed • 7 high risk • 1 need attention 🚨 High Risk:
🚀 Open Interactive Review →The full interface unlocks features not available in GitHub:
💬 Chat here: 📖 View all 12 personas & slash commandsYou can interact with me by mentioning In PR comments or on any line of code:
Slash commands:
AI Personas (mention to get their perspective):
For the best experience, view this PR on myseniordev.com — includes AI chat, file annotations, and interactive reviews. |
Member
Author
|
Closed in favor of #207 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Completes the incomplete work from the
claude/fix-issue-204-uNZ6ybranch that removed the node-pty dependency.Issues fixed:
Double-formatting bug - The branch removed pre-formatted message detection from Rust
format_for_injection(), but Node.jsbuildInjectionString()still formats messages before sending. This caused messages to be wrapped twice ("Relay message from... Relay message from...").Debug statements -
shared.tshad debugconsole.logstatements that should not be in production code.Changes
relay-pty/src/protocol.rssrc/wrapper/shared.tsTest plan
npm run build)npx tsc --noEmit)cd relay-pty && cargo test)Background
Issue #204 reports that
npm installbreaks on Node versions other than 22.0.0 due to node-pty's native compilation issues. The fix removes node-pty and uses the relay-pty Rust binary instead. This PR completes that work by fixing the two issues that made the original branch incomplete.🤖 Generated with Claude Code