Skip to content

fix(deps): postinstall smoke test for node-pty native binding - #47

Open
brett817 wants to merge 1 commit into
noogalabs:mainfrom
brett817:fix/postinstall-verify-node-pty
Open

brett817 wants to merge 1 commit into
noogalabs:mainfrom
brett817:fix/postinstall-verify-node-pty

Conversation

@brett817

Copy link
Copy Markdown

Contribution

Type: bug fix

Problem:
An npm install can pick up a node-pty prebuilt binary that is ABI-incompatible with the installed Node version (observed with Node v24.17.0; node-pty 1.1.0 has no matching prebuild). Every agent runtime spawns its AI session through node-pty, so a bad binding silently breaks the entire fleet: pty.spawn() throws posix_spawnp failed for every command while the daemon looks healthy (Telegram polling, cron scheduling, and command registration bypass node-pty). The breakage goes unnoticed until an agent's crash counter trips its daily halt.

Proposed change:
Add a postinstall script (scripts/verify-node-pty.js) that spawns a trivial command through node-pty in a fresh subprocess and rebuilds from source if it fails. Never hard-fails the install — if no compiler is available, it warns and leaves the existing prebuild in place. Benefits any member running on a Node version that outpaces node-pty's prebuild matrix.

Intended Scope

Public base commit: afba07747d304b7cfbf3b22d0265d2c3f9df7eb4

Head commit: 06dc5c70598a5f7de548d3798de48ffee0d9ce20

M	package.json
A	scripts/verify-node-pty.js
  • I inspected the complete upstream/main...HEAD diff, not only HEAD~1 or the latest commit.
  • The declared file list matches the pull request's full Files changed list.
  • This branch contains no unrelated inherited commits, fork catch-up bundle, generated runtime state, or private deployment files.
  • Each changed file is necessary for the stated contribution.

Organization-Specific And Human Data

What organization-specific source material did this work start from?
A production incident where an npm install picked up an ABI-incompatible node-pty prebuild, silently disabling all agent sessions. The fix is generic — no org-specific data was used.

What did you scrub or replace, and in which files?
Nothing to scrub. The script references only generic node-pty behavior and standard npm tooling. The incident date in the comment header is retained as context for the fix.

Persona privacy inventory:
Not an agent/persona contribution.

  • No secrets, tokens, keys, passwords, credential contents, or private credential paths are present.
  • No real names, emails, phone numbers, addresses, calendar events, inbox content, contacts, family details, financial data, or customer/resident/owner/vendor/applicant data are present.
  • Every synthetic US phone fixture uses the NANP-reserved 555-0100 through 555-0199 range; any phone-shaped value outside that range has been removed.
  • No private or organization-specific names, domains, internal paths, agent rosters, chat IDs, label IDs, account/tenant IDs, production URLs, memories, transcripts, incident history, or runtime state are present. Public integration domains disclosed in the contribution's required integration declarations are permitted.
  • Agent/persona fixtures are synthetic and cannot be traced back to a real person or organization.

Do We Want To Own It?

1. Who can this bite?

Any AscendOps member running a Node version newer than what node-pty ships prebuilds for. The triggering condition is: npm install downloads a prebuild that doesn't match the running Node ABI, which happens when Node releases outpace node-pty's prebuild CI matrix. This is not org-specific — it affects every installation on the affected Node version.

2. Why in the next 90 days?

Node v24 was released recently and node-pty 1.1.0 doesn't ship a matching prebuild. Anyone installing on v24 hits this silently. The fix is 75 lines and runs only at install time. Without it, the failure mode is a fleet that looks healthy but has zero working agent sessions.

3. What does it collide with?

The fix/bypass-fresh-evidence branch adds Claude preflight and PTY startup hardening. Those catch problems at spawn time; this catches them at install time. The two are complementary, not conflicting — they cover different failure windows. Searched: all open PRs, scripts/ directory, package.json scripts field, node-pty references in src/.

4. What does ownership cost forever?

Minimal. The script is 75 lines of plain Node.js with no external dependencies. It runs once at npm install and exits. Maintenance cost: if node-pty changes its API (unlikely — pty.spawn is its core interface), the smoke test line needs updating. No security surface (reads nothing, writes nothing, spawns only echo). Failure mode is graceful — warns and exits 0 if rebuild fails.

  • I understand the maintainer outcome may be ACCEPT, REPORT-ONLY, or DECLINE.
  • I understand REPORT-ONLY means the finding may be retained while this pull request is closed and independently reimplemented.
  • I understand this contribution targets only public noogalabs/ascendops; it does not modify or request access to any private/production repository.

Mechanical Verification

npm run typecheck   # clean (tsc --noEmit, 0 errors)
npm run build       # clean (tsup, 92ms)
npm test            # 187 passed | 2 skipped (3459 tests, 24.40s)
node scripts/skill-drift-check.mjs --tier ci  # OK all declared skill mirrors match canonical
git diff --check upstream/main...HEAD  # clean
leak-guard.sh --diff upstream/main HEAD  # clean, scanned 2 changed file(s)
  • The required Leak Guard check passes its configured secret, PII, operator-path, and private-runtime patterns.
  • npm run typecheck passes.
  • npm run build passes.
  • npm test passes.
  • Relevant regression tests were added or updated.
  • node scripts/skill-drift-check.mjs --tier ci passes.
  • git diff --check upstream/main...HEAD passes.
  • If this adds a command, endpoint, hook, or behavior, the relevant agent templates and usage examples are updated.
  • If this changes agent-installed files, existing-agent migration behavior is covered, not only fresh initialization.

Note on regression tests: The script itself is a smoke test — it verifies node-pty works by spawning a real command. A unit test would need to mock node-pty failure at the native binding level, which would test the mock rather than the binding. The postinstall hook's behavior is verified by npm install on any system where node-pty's prebuild is incompatible. If a synthetic test fixture is preferred, happy to add one that exercises the script's exit-code behavior.

External Surface And Rollback

External services, APIs, scopes, environment variables, and permissions:
None. The script uses only Node.js builtins (child_process.spawnSync) and the locally installed node-pty package. No network calls, no environment variable reads, no file writes.

Failure behavior and rollback plan:
The script never hard-fails. If node-pty's binding is broken AND a source rebuild is not possible, it warns to stderr and exits 0 — install completes normally, same as today's behavior. Rollback: remove the postinstall line from package.json and delete scripts/verify-node-pty.js.

Security-sensitive or destructive paths touched:
None. The script spawns /bin/echo ok (or cmd.exe /c ok on Windows) through node-pty and optionally runs npm rebuild node-pty --build-from-source. No auth, tokens, messaging, deploys, deletes, or watchdog logic.

An `npm install` can pick up a node-pty prebuilt binary that is
ABI-incompatible with the installed Node version (observed with
Node v24.17.0; node-pty 1.1.0 has no matching prebuild). Every
agent runtime spawns its AI session through node-pty, so a bad
binding silently breaks the entire fleet — pty.spawn() throws
"posix_spawnp failed" for every command while the daemon looks
healthy (Telegram, crons, and command registration bypass node-pty).

Add a postinstall script that spawns a trivial command through
node-pty in a fresh subprocess and rebuilds from source if it
fails. Never hard-fails the install — if no compiler is available,
it warns and leaves the existing prebuild in place.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@noogalabs

Copy link
Copy Markdown
Owner

Thanks for this. We have got it and it is in front of a human, not a queue.

We will reply here with a real answer, not a status. If you do not hear from us within a couple of days, that is our failure and you should say so loudly in this thread.

@noogalabs

Copy link
Copy Markdown
Owner

Thanks - this addresses a failure class we've hit ourselves (postinstall exit 0 with a broken native module is real), so this is wanted. Verdict: adopt after rebase. Two asks:

  1. It's currently conflicting with main after a large catch-up merge landed Aug 18 - a rebase should be straightforward since your changes are package.json plus one new script.
  2. Heads-up on review depth: a postinstall script is install-time code execution, the highest-trust surface in the repo, so the script (including the rebuild-from-source path) will get a line-by-line security review rather than a skim. Nothing personal - we'd do the same to our own.

Thanks for contributing!

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.

2 participants