Skip to content

Fix compound SSH PTY attach startup scripts - #9429

Closed
austinywang wants to merge 2 commits into
mainfrom
issue-9423-ssh-startup-env-prefix-if
Closed

austinywang wants to merge 2 commits into
mainfrom
issue-9423-ssh-startup-env-prefix-if

Conversation

@austinywang

@austinywang austinywang commented Aug 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Export the no-progress retry environment on standalone shell lines before executing attach source.
  • Keep noProgressRetryLoopLines(command:) valid for simple, compound, and multi-line POSIX shell commands.
  • Add a behavior regression that executes a generated compound attach script under /bin/sh.

Closes #9423

Root cause

noProgressRetryLoopLines(command:) accepted arbitrary shell source but interpolated it after NAME=value prefixes. The foreground-auth CLI path passed a command beginning with if, and POSIX shells only permit assignment prefixes before a simple command. The generated startup script therefore failed during parsing before SSH could start.

Moving the two per-attempt values into standalone export commands removes that hidden command-shape restriction. Future callers cannot recreate the same syntax error by supplying compound or multi-line shell source.

Testing

  • Red baseline: https://github.com/manaflow-ai/cmux/actions/runs/30783465527
    • 8 tests executed on test-only commit b0b5270cbe.
    • The new regression failed alone with status 2 and the expected unexpected token then parse error.
  • Local /bin/sh -n syntax check passed with the fixed shape.
  • Local /bin/sh execution verified both retry values are exported to child processes.
  • Swift parser checks passed for the changed source and test files.
  • git diff --check origin/main...HEAD passed.

Demo Video

Not applicable. This changes generated CLI shell source and has no visual UI.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Fixes SSH PTY attach startup scripts so compound or multi-line POSIX shell commands don’t fail to parse. Exports retry env vars on their own lines to avoid if-prefix syntax errors and preserve the retry policy (addresses #9423).

  • Bug Fixes
    • Export CMUX_SSH_PTY_ATTACH_NO_PROGRESS_RETRY and CMUX_SSH_PTY_ATTACH_NO_PROGRESS_LIMIT before running the attach command; no assignment prefixes.
    • Keep noProgressRetryLoopLines(command:) valid for simple, compound, and multi-line commands.
    • Add a regression test that runs a compound if-based attach under /bin/sh and verifies env propagation.

Written for commit 1c4e8ac. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes

    • Improved SSH terminal attachment reliability by ensuring no-progress retry settings are preserved through nested shell commands.
    • Reduced the risk of attachment timeouts and incorrect exit statuses during stalled or slow connections.
  • Tests

    • Added coverage for nested-shell attachment scenarios, including retry configuration propagation and successful completion.

@coderabbitai

coderabbitai Bot commented Aug 3, 2026 •

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: e20167c7-ae3a-4283-a7d1-338058a7ece6

📥 Commits

Reviewing files that changed from the base of the PR and between 84f5755 and 1c4e8ac.

📒 Files selected for processing (2)
  • Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHPTYAttachExitCode.swift
  • cmuxTests/SSHPTYAttachNoProgressRetryTests.swift

📝 Walkthrough

Walkthrough

The SSH PTY retry loop now exports no-progress variables before executing attach commands. An integration test verifies propagation through compound shell commands and successful completion.

Changes

SSH PTY retry handling

Layer / File(s) Summary
Export retry environment before attach attempts
Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHPTYAttachExitCode.swift, cmuxTests/SSHPTYAttachNoProgressRetryTests.swift
noProgressRetryLoopLines uses shell exports instead of command prefixes. The integration test verifies retry-variable propagation, completion, and exit status.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

  • Issue 7695 in manaflow-ai/cmux-dev-artifacts: It addresses exporting SSH PTY retry variables before compound attach commands and adding integration coverage.

Possibly related PRs

🚥 Pre-merge checks | ✅ 25
✅ Passed checks (25 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main fix for compound SSH PTY attach startup scripts.
Description check ✅ Passed The description explains the root cause, fix, testing, and why a demo video is not applicable.
Linked Issues check ✅ Passed The changes directly resolve issue #9423 by removing invalid environment prefixes before compound shell commands and preserving retry behavior.
Out of Scope Changes check ✅ Passed The changes are limited to the SSH PTY attach fix and its regression test, with no unrelated code changes identified.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Cmux Swift Actor Isolation ✅ Passed The Swift 6 production diff only changes shell-source strings and documentation; it adds no actor, Sendable, protocol, UI-store, or shared mutable reference declaration. The other changes are test-...
Cmux Swift Blocking Runtime ✅ Passed The production diff only replaces shell variable prefixes with exports; it adds no blocking or timing primitive. Semaphore waits remain in the pre-existing test-only helper.
Cmux Browser Automation Off-Main ✅ Passed The diff only changes SSH PTY retry generation and its tests; it does not modify browser socket commands, WebKit/AppKit routing, worker policy, or browser policy tests.
Cmux Expensive Synchronous Load ✅ Passed The production diff only changes generated shell exports in SSHPTYAttachExitCode.swift; it adds no agent-history loader, large-file parse, directory scan, syscall loop, or interactive/main-actor path.
Cmux Cache Substitution Correctness ✅ Passed The diff only changes generated POSIX shell exports and command execution; it does not replace an authoritative read with a cache or affect persistence, history, undo, or snapshot paths.
Cmux No Hacky Sleeps ✅ Passed The PR changes only Swift files, adds standalone exports and a test, and introduces no sleep, timer, polling, or wall-clock wait; the existing shell sleep is unchanged.
Cmux Algorithmic Complexity ✅ Passed The production diff only replaces one shell prefix with two export lines; it adds no collection scan, sort, join, or nested traversal. The new 23-line regression is test-only.
Cmux Swift Concurrency ✅ Passed The PR changes shell-generation strings and adds a test call; it introduces no runtime Dispatch, Combine, completion-handler, or fire-and-forget Task pattern. Existing test synchronization is uncha...
Cmux Swift @Concurrent ✅ Passed The diff changes only synchronous Swift functions and adds a synchronous test; it introduces no nonisolated async work, @concurrent annotation, actor-isolated access, or UI-bound heavy async call.
Cmux Swift Package Boundaries ✅ Passed The only production Swift diff is in the existing CmuxFoundation SwiftPM target; the regression test is test code, so no app-target package-boundary violation exists.
Cmux Swiftpm Lockfiles ✅ Passed The PR changes only SSHPTYAttachExitCode.swift and its test; no Package.swift, Package.resolved, .gitignore, workflow, or Xcode package-reference paths changed.
Cmux Swift Logging ✅ Passed The diff adds only standalone shell exports and a regression test; it adds no production print, Logger, NSLog, file, or stdout/stderr diagnostic logging.
Cmux User-Facing Error Privacy ✅ Passed The production diff only moves existing retry environment variables into internal shell exports; the user-visible retry diagnostic remains generic, and added variable references are test-only or in...
Cmux Full Internationalization ✅ Passed The PR changes only shell environment-generation code and a regression test; it adds no user-facing copy, localization keys, catalogs, web messages, metadata, or changelog entries.
Cmux Swiftui State Layout ✅ Passed The diff changes only SSH shell retry logic and its test; no SwiftUI import, state wrapper, layout API, store reference, or render-time mutation was added.
Cmux Architecture Rethink ✅ Passed The full PR diff is a small shell correctness fix: it exports existing retry state before arbitrary command source. No new production timing, polling, locks, observers, flags, or lifecycle owners w...
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed The diff changes shell-generation logic and a test-only /bin/sh Process fixture; it adds no NSWindow, NSPanel, SwiftUI window, identifier, or close-shortcut routing.
Cmux Source Artifacts ✅ Passed Both changed paths are tracked Swift source/test files; scans found no forbidden artifact directories, binary content, or generated-output markers.
Cmux No Test Or Debug Seam In Production Source ✅ Passed The PR production diff only changes shell exports and a comment in SSHPTYAttachExitCode.swift; it adds no DEBUG guard, test/debug seam, visibility widening, or production accessor.
Cmux No Ambient Global State ✅ Passed The PR only changes statements inside existing SSHPTYAttachExitCode.noProgressRetryLoopLines; the public enum and static API predate the PR, with no new global function, mutable var, namespace, or...
✨ Finishing Touches 💡 1
⚔️ Resolve merge conflicts 💡
  • Resolve merge conflict in branch issue-9423-ssh-startup-env-prefix-if
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-9423-ssh-startup-env-prefix-if

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.

@austinywang

Copy link
Copy Markdown
Contributor Author

Superseded by #9425, which merged while this PR's red and green suite runs were in progress. #9425 contains the same generator-level standalone export fix and a stronger behavior test that runs both /bin/sh -n and the generated script. No unique change remains in this PR.

@austinywang austinywang closed this Aug 3, 2026
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.

cmux ssh: shell syntax error in startup script on 0.64.21 (env prefix before if)

1 participant