fix(node-pty): patch winpty.gyp paths and harden POSIX spawn errors

[feldsys]

Split out from #962 per @pedramamini's review — this commit is unrelated to the git graph / branch switcher feature and is easier to review and revert independently here.

Summary

Two fixes against the patched node-pty@1.1.0:

• winpty.gyp (Windows build): prefix the GetCommitHash.bat / UpdateGenVersion.bat invocations with .\\ so cmd /c finds them when the working directory isn't on the lookup path. Without this the native build fails on Windows.
• pty_posix_spawn (macOS error reporting): surface the actual posix_spawnp errno via a format_error helper and a std::string err out-param, instead of collapsing every failure into the generic "posix_spawnp failed." string.

CodeRabbit fix folded in

The original PR #962 version of this patch had an OOB / uninitialised-FD bug in the cleanup loop (CodeRabbit flagged it). This branch addresses it:

• Initialise int low_fds[3] = { -1, -1, -1 };
• Replace for (size_t i = 0; i <= count; i++) close(low_fds[i]); with a bounded, -1-aware variant that iterates 0..3 and skips uninitialised entries — no array OOB, no close() on stack garbage.

Why

node-pty ships an upstream patch via patch-package. The Windows build silently breaks for new contributors without the gyp prefix fix, and a bare "posix_spawnp failed." made macOS spawn issues effectively unreproducible.

Test plan

• npm install on Windows applies the patch cleanly via patch-package.
• npm install on macOS applies cleanly.
• On macOS, force a spawn failure (e.g. invalid command) and confirm the thrown Napi::Error carries the formatted "posix_spawn failed: <strerror>" instead of the legacy generic string.

[coderabbitai]

📝 Walkthrough

Walkthrough

This patch updates the node-pty package to improve error reporting in Unix PTY spawning and fix Windows build script invocations. The error handling refactoring replaces integer error codes with formatted error strings propagated from the spawning helper to the N-API layer, enabling richer error messages on failure.

Changes

Windows Build Configuration

Layer / File(s)	Summary
Windows Batch Path Prefixes patches/node-pty+1.1.0.patch	Gyp script variables for GetCommitHash.bat and UpdateGenVersion.bat are invoked with explicit .\ relative-path prefix.

Unix PTY Error Propagation

Layer / File(s)	Summary
Error Interface Contract patches/node-pty+1.1.0.patch	pty_posix_spawn helper signature changes from int* err to std::string* err to propagate formatted error messages.
N-API Caller Refactoring patches/node-pty+1.1.0.patch	PtyFork initializes master to -1, calls pty_posix_spawn with string error buffer, closes master on failure, and throws Napi::Error with the propagated message.
Helper Implementation patches/node-pty+1.1.0.patch	pty_posix_spawn adds format_error helper; replaces early returns with goto done cleanup; captures system call errors via spawn_err and formats them into the string buffer on failure paths including posix_openpt, grantpt/unlockpt, slave open, tcsetattr, and posix_spawn operations.

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested Labels

approved, ready to merge

Poem

🐰 A patch of errors transformed to text,
Where Windows paths find clarity next,
Formatted strings now float on through,
From spawning helpers, detailed and true!
The PTY runs cleaner—hooray, what a view! 🌟

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely captures both main fixes: Windows winpty.gyp path prefixing and POSIX spawn error reporting hardening.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (1)

patches/node-pty+1.1.0.patch (1)

66-70: 💤 Low value

Consider strerror_r for thread safety; truncation is acceptable.

strerror is not thread-safe per POSIX. In practice this helper is invoked on the libuv/N-API main thread for PtyFork, so racing is unlikely, but if node-pty ever calls pty_posix_spawn from worker threads in the future this becomes a latent issue. The 256-byte buffer is fine — snprintf truncates safely and the format string is short. Consider strerror_r (XSI variant on Linux, POSIX on macOS) as a defensive future-proofing measure; not blocking.

♻️ Optional: thread-safe variant

 static std::string format_error(const char* func, int err_code) {
   char buf[256];
-  snprintf(buf, sizeof(buf), "%s: %s", func, strerror(err_code));
+  char errbuf[128];
+#if defined(__APPLE__) || defined(__GLIBC__) && ((_POSIX_C_SOURCE >= 200112L) && !_GNU_SOURCE)
+  strerror_r(err_code, errbuf, sizeof(errbuf));
+  snprintf(buf, sizeof(buf), "%s: %s", func, errbuf);
+#else
+  snprintf(buf, sizeof(buf), "%s: %s", func, strerror(err_code));
+#endif
   return buf;
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@patches/node-pty`+1.1.0.patch around lines 66 - 70, The helper format_error
uses strerror which is not thread-safe; replace its use with the thread-safe
strerror_r API in format_error to avoid races (keeping the existing 256-byte
buffer and snprintf truncation behavior). Update the implementation of
format_error to call strerror_r(buf, sizeof(buf), err_code) (handling both XSI
and GNU variants if needed) and then format the final message with snprintf as
before; ensure you still return a std::string and preserve the current message
layout ("%s: %s").

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@patches/node-pty`+1.1.0.patch:
- Around line 66-70: The helper format_error uses strerror which is not
thread-safe; replace its use with the thread-safe strerror_r API in format_error
to avoid races (keeping the existing 256-byte buffer and snprintf truncation
behavior). Update the implementation of format_error to call strerror_r(buf,
sizeof(buf), err_code) (handling both XSI and GNU variants if needed) and then
format the final message with snprintf as before; ensure you still return a
std::string and preserve the current message layout ("%s: %s").

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f1e24b48-0cd4-47f1-86d9-0df6af31a35e

📥 Commits

Reviewing files that changed from the base of the PR and between f28198c and f98c6ae.

📒 Files selected for processing (1)

• patches/node-pty+1.1.0.patch

[greptile-apps]

Greptile Summary

This PR patches node-pty@1.1.0 via patch-package with two independent fixes: a Windows build correction and a macOS error-reporting improvement in the PTY spawn path.

• winpty.gyp (Windows): Prefixes GetCommitHash.bat and UpdateGenVersion.bat with .\\ so cmd /c resolves the batch files when the working directory is not on PATH, fixing silent native build failures for new contributors.
• pty_posix_spawn (macOS): Replaces the generic \"posix_spawnp failed.\" string with a format_error helper that formats \"<func>: <strerror>\" for each failure site; initialises low_fds[3] to {-1,-1,-1} and replaces the original OOB cleanup loop with a bounded 0..2 sentinel-aware variant; moves acts/attrs init before all goto done jumps so destroy is always called on a properly init'd object; and adds close(slave) in the done: cleanup block to fix a fd leak present in the original.

Confidence Score: 4/5

Safe to merge — the changes are tightly scoped to a patch file applied during npm install via patch-package, so no runtime JS/TS code is touched. The C++ restructuring is well-reasoned and the fd-lifecycle tracing is correct across all success and failure paths.

The logic in the refactored pty_posix_spawn is sound: acts and attrs are always init'd before any goto done and always destroyed in the done block; slave is closed by the callee in done, master is closed by the caller on error — no fd leaks. The low_fds loop now correctly iterates 0..2 with a -1 guard, fixing the original OOB and off-by-one. The only reason to hold back from a perfect score is that this patch touches upstream C++ native code that cannot be exercised by the project's own test suite; the fix must be manually validated on Windows and macOS as the test plan describes.

patches/node-pty+1.1.0.patch — worth a manual npm install smoke-test on both Windows (native build) and macOS (force a spawn failure to verify the formatted error message) before merging to rc.

Important Files Changed

Filename	Overview
patches/node-pty+1.1.0.patch	Adds two fixes: .\ prefix on Windows batch file calls in winpty.gyp, and a refactored pty_posix_spawn on macOS that surfaces per-step errno messages, initialises low_fds to -1 to prevent OOB closes, moves acts/attrs init before any goto done, and closes slave fd in the cleanup block.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["pty_posix_spawn entry\nlow_fds initialized to -1,-1,-1\nacts and attrs init'd early"] --> B["Fill low_fds loop\nposix_openpt x3"]
    B --> C["posix_openpt for master fd"]
    C -- fails --> Z["set *err via format_error"]
    C -- ok --> D["grantpt"]
    D -- fails --> Z
    D -- ok --> E["unlockpt"]
    E -- fails --> Z
    E -- ok --> F["ioctl TIOCPTYGNAME"]
    F -- fails --> Z
    F -- ok --> G["open slave pty"]
    G -- fails --> Z
    G -- ok --> H["tcsetattr + TIOCSWINSZ"]
    H -- fails --> Z
    H -- ok --> I["adddup2+addclose on acts"]
    I --> J["posix_spawnattr_setflags\nsetsigdefault / setsigmask"]
    J -- fails --> Z
    J -- ok --> K["posix_spawn loop\nretry on EINTR"]
    K -- fails --> Z
    K -- ok --> L["spawn succeeded\nerr stays empty"]
    Z --> N
    L --> N["done label:\ndestroy acts+attrs\nclose slave if open\nclose low_fds != -1"]
    N --> O["Return to PtyFork"]
    O --> P{"err empty?"}
    P -- yes --> Q["pty_nonblock master\nreturn pid+master to caller"]
    P -- no --> R["close master if != -1\nthrow Napi::Error with message"]

Loading

_{Reviews (1): Last reviewed commit: "fix(node-pty): patch winpty.gyp paths an..." | Re-trigger Greptile}

[pedramamini]

Thanks for splitting this out, @feldsys — much easier to reason about in isolation. 🙏

Took a careful look at the patch:

• winpty.gyp .\\ prefix: Correct — cmd /c resolves the batch files relative to the cd shared working directory but won't search PATH-style for the unprefixed name in all environments. Straightforward fix.
• pty_posix_spawn rewrite:
  • low_fds[3] = { -1, -1, -1 } and the bounded 0..3 -1-aware cleanup loop properly resolve the OOB / uninitialised-FD issue from feat(git): add graph view + branch switcher dropdown #962 — confirmed.
  • acts / attrs are now declared and *_init()ed before any goto done, so the done: block always destroys initialised objects. ✅
  • slave defaults to -1 and is closed in done: only when set — no double-close, no leak.
  • master is initialised to -1 in PtyFork and closed by the caller on error before throwing. Lifetime is consistent across success and failure paths.
  • Each failure site captures errno (or the posix_spawnattr_* return) into spawn_err before any cleanup syscalls, so the formatted message reflects the original cause. ✅

CodeRabbit's strerror_r nit is fair in principle but the helper is only invoked on the libuv/N-API main thread for PtyFork, so I'm comfortable leaving it as-is for this PR — happy to revisit if node-pty ever calls into this from a worker.

Approving. ✅