fix(cli): squad start --tunnel validates node-pty before side effects (#711)#860
fix(cli): squad start --tunnel validates node-pty before side effects (#711)#860
Conversation
🟡 Impact Analysis — PR #860Risk tier: 🟡 MEDIUM 📊 Summary
🎯 Risk Factors
📦 Modules Affectedroot (2 files)
squad-cli (2 files)
tests (2 files)
|
🛫 PR Readiness Check
PR Scope: 📦🔧 Mixed (product + infrastructure)
|
| Status | Check | Details |
|---|---|---|
| ✅ | Single commit | 1 commit — clean history |
| ✅ | Not in draft | Ready for review |
| ✅ | Branch up to date | Up to date with dev |
| ❌ | Copilot review | No Copilot review yet — it may still be processing |
| ✅ | Changeset present | Changeset file found |
| ✅ | Scope clean | No .squad/ or docs/proposals/ files |
| ✅ | No merge conflicts | No merge conflicts |
| ✅ | Copilot threads resolved | 0 active Copilot thread(s) resolved (1 outdated skipped) |
| ❌ | CI passing | 16 check(s) still running |
Files Changed (6 files, +339 −73)
| File | +/− |
|---|---|
.changeset/start-tunnel-node-pty.md |
+5 −0 |
package-lock.json |
+31 −0 |
packages/squad-cli/package.json |
+4 −0 |
packages/squad-cli/src/cli/commands/start.ts |
+32 −5 |
test/cli-packaging-smoke.test.ts |
+125 −67 |
test/cli/start.test.ts |
+142 −1 |
Total: +339 −73
This check runs automatically on every push. Fix any ❌ items and push again.
See CONTRIBUTING.md and PR Requirements for details.
🔒 Security Review🔒 Security review: 1 info.
Automated security review — informational only. |
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #711 by ensuring squad start validates the optional native dependency node-pty before starting any bridge/tunnel work, avoiding side effects (like starting a devtunnel) when node-pty is missing.
Changes:
- Added a
checkNodePty()helper and movednode-ptyvalidation to the very start ofrunStart(). - Declared
node-ptyandqrcode-terminalasoptionalDependenciesfor@bradygaster/squad-cli. - Expanded tests: a regression test for validation ordering + packaging smoke coverage for a forced-missing
node-ptyscenario.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
packages/squad-cli/src/cli/commands/start.ts |
Adds early node-pty validation to prevent tunnel/bridge side effects when the dependency is missing. |
packages/squad-cli/package.json |
Declares node-pty and qrcode-terminal as optionalDependencies. |
package-lock.json |
Updates lockfile to include the new optional dependencies. |
CHANGELOG.md |
Documents the fix and new behavior for squad start --tunnel. |
test/cli-packaging-smoke.test.ts |
Improves packaging isolation and adds a forced-missing node-pty smoke test. |
test/cli/start.test.ts |
Adds regression coverage to ensure node-pty validation happens before bridge/tunnel setup. |
.changeset/start-tunnel-node-pty.md |
Adds a changeset for the CLI patch release. |
7ffe525 to
3e462f5
Compare
Validates node-pty availability before executing tunnel side effects, preventing crashes when the optional dependency is missing. Closes #715 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
3e462f5 to
bcc2d43
Compare
tamirdresher
left a comment
There was a problem hiding this comment.
Approved by @tamirdresher — reviewed diff, CI green, changes are clean and well-tested. cc @diberry
Summary
Validates that
node-ptyis available before attempting to tunnel, preventing unexpected terminal corruption when the package is missing.Changes
checkNodePty()helper insquad start --tunnelnpm install -g node-ptyguidancenode-ptyscenarionode-ptyandqrcode-terminaldeclared as optionalDependenciesFiles Changed
packages/squad-cli/src/cli/commands/start.ts— checkNodePty helper + early validationpackages/squad-cli/package.json— optionalDependenciespackage-lock.json— lockfile updateCHANGELOG.md— changelog entrytest/cli-packaging-smoke.test.ts— refactored + node-pty smoke testtest/cli/start.test.ts— node-pty regression tests.changeset/start-tunnel-node-pty.md— changesetCloses #711
Based on PR #715 by @andikrueger (community contributor)
Co-authored-by: andikrueger
Co-authored-by: Copilot