Skip to content

Fix ssh attach script syntax error with compound commands - #9448

Closed
lintanghui wants to merge 1 commit into
manaflow-ai:mainfrom
lintanghui:autofix/issue-9443
Closed

lintanghui wants to merge 1 commit into
manaflow-ai:mainfrom
lintanghui:autofix/issue-9443

Conversation

@lintanghui

@lintanghui lintanghui commented Aug 3, 2026 •

Copy link
Copy Markdown
Contributor

The persistent ssh attach loop passes the attach environment as an assignment prefix (VAR=... <command>). A prefix like that is only valid before a simple command; when the caller's command is a compound command the generated script is a POSIX syntax error, which is what broke cmux ssh startup on 0.64.21.

Export the three attach variables on their own lines before invoking the command instead, in both the retry script builder and the shared attach loop. Doc comments on both builders note the constraint.

Fixes #9443

…ith syntax error near `

Automated fix generated by autogit.
Closes manaflow-ai#9443
@coderabbitai

coderabbitai Bot commented Aug 3, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The SSH PTY attach retry scripts now export retry-state variables on separate shell lines. Regression tests validate POSIX syntax and retry-budget propagation for compound attach commands.

Changes

SSH PTY attach shell generation

Layer / File(s) Summary
Separate retry-state exports
Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHPTYAttachExitCode.swift, Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHPTYAttachRetryScriptBuilder.swift
The retry-loop generators assign and export retry-state variables before executing attach commands. Documentation describes the shell requirement.
Compound attach command regression tests
Packages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/SSHPTYAttachExitCodeTests.swift, Packages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/SSHPTYAttachRetryScriptBuilderTests.swift
Tests validate POSIX shell syntax, execute generated scripts, and verify retry-budget values.

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

Possibly related PRs

Suggested reviewers: austinywang

🚥 Pre-merge checks | ✅ 23 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description explains the problem and fix but omits the required Testing, Demo Video, Review Trigger, and Checklist sections. Add the missing template sections and record the tests run, verification results, review trigger, and checklist status.
✅ Passed checks (23 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes fix the invalid environment-assignment syntax, preserve retry behavior, and add shell validation coverage for issue #9443.
Out of Scope Changes check ✅ Passed All changes support issue #9443 by correcting generated shell syntax and adding focused regression tests.
Cmux Swift Actor Isolation ✅ Passed Production changes only update documentation and generated shell strings; the existing enum and Sendable struct declarations are unchanged, and other changes are tests.
Cmux Swift Blocking Runtime ✅ Passed The production diff only exports shell variables and adds no blocking or timing primitive; new Process.waitUntilExit usage is deterministic test-only scaffolding allowed by the rule.
Cmux Browser Automation Off-Main ✅ Passed The commit changes only SSH PTY retry builders and tests; it adds no browser.* command, WebKit/AppKit routing, socket-worker method, or policy-test change covered by this rule.
Cmux Expensive Synchronous Load ✅ Passed The PR production diff only changes SSH shell-script environment exports and documentation; scans found no agent-history loader, synchronous JSON/file load, or interactive-path load.
Cmux Cache Substitution Correctness ✅ Passed The diff only changes generated SSH retry-shell assignments and adds syntax tests; it introduces no cache, authoritative-read, persistence, history, undo, or snapshot substitution.
Cmux No Hacky Sleeps ✅ Passed The diff adds no sleep, timer, polling, or delayed-dispatch logic; it only changes shell exports and adds deterministic POSIX syntax/execution tests. Existing retry backoff sleeps are unchanged.
Cmux Algorithmic Complexity ✅ Passed The production diff only replaces one shell command string with four assignment/export strings; it adds no scalable collection scan or slower algorithm. New collection work is test-only.
Cmux Swift Concurrency ✅ Passed The diff only changes shell-script generation and adds Process-based syntax/execution tests; it introduces no Dispatch, Combine, completion-handler, or fire-and-forget Task patterns.
Cmux Swift @Concurrent ✅ Passed The four-file Swift diff changes only synchronous shell-script builders and tests; it adds no async, nonisolated, @concurrent, actor-isolated, or UI-isolation work.
Cmux Swift Package Boundaries ✅ Passed The production changes are in the existing CmuxFoundation SwiftPM target, with package-local tests; they add no app-target, AppKit, SwiftUI, Ghostty, or singleton-bound logic.
Cmux Swiftpm Lockfiles ✅ Passed The diff only changes CmuxFoundation source and tests; it changes no Package.swift dependencies, .gitignore, workflow, Xcode package references, or Package.resolved lockfile.
Cmux Swift Logging ✅ Passed Production additions only export shell variables and documentation; no print, debugPrint, dump, NSLog, Logger, or sensitive logging was added. Test printf/file I/O is harness output.
Cmux User-Facing Error Privacy ✅ Passed The production diff changes internal shell-script construction and developer comments; it adds no user-facing error, alert, command output, or recovery text exposing sensitive implementation details.
Cmux Full Internationalization ✅ Passed The PR changes shell exports, developer comments, and regression tests only; it adds no user-facing copy or localization/catalog/message changes.
Cmux Swiftui State Layout ✅ Passed The diff changes SSH shell-script builders and tests only; it adds no SwiftUI views, state wrappers, GeometryReader, lazy rows, or render-time state mutation.
Cmux Architecture Rethink ✅ Passed The patch is a small shell-syntax correctness fix: shared builders replace invalid assignment prefixes with exports and add /bin/sh -n tests; it adds no timing, polling, locks, observers, or new...
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed The commit changes only SSH PTY retry-script builders and tests; it introduces no NSWindow, NSPanel, WindowController, SwiftUI Window, or close-shortcut code.
Cmux Source Artifacts ✅ Passed The diff changes only two tracked Swift source files and two Swift test files; no logs, caches, temp directories, build output, or artifact-like paths are added.
Cmux No Test Or Debug Seam In Production Source ✅ Passed The two changed production Swift files only add shell export lines and documentation; no DEBUG/test seam, debug/test-only member, or visibility change appears. Regression tests remain under Tests/.
Cmux No Ambient Global State ✅ Passed The production diff only changes existing methods and documentation in two existing types; it adds no top-level function, mutable global, namespace type, or singleton state.
Title check ✅ Passed The title clearly describes the primary fix for SSH attach scripts with compound commands.
✨ 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

Comment @coderabbitai help to get the list of available commands.

@lintanghui lintanghui changed the title Fix #9443: [Bug] Generated cmux-ssh-startup script fails with syntax error near ` Fix ssh attach script syntax error with compound commands Aug 3, 2026
@teamleaderleo

Copy link
Copy Markdown
Collaborator

Thank you for tracking this down, @lintanghui! The same ssh attach script syntax error got fixed on main in #9425 the same day, so this one's covered. Your /bin/sh -n regression tests are a nice idea too, and we'd happily take them as a small tests-only PR :)

teamleaderleo added a commit that referenced this pull request Sep 25, 2026
)

Adds a /bin/sh -n regression test for SSHPTYAttachRetryScriptBuilder
with a compound attach command, and checks the loop still exports the
retry budget to the command it runs. #9425 fixed the generator and
covered noProgressRetryLoopLines; this covers the main retry builder.

Salvaged from #9448.

Co-authored-by: Tanghui Lin <xmutanghui@gmail.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@teamleaderleo

Copy link
Copy Markdown
Collaborator

Your /bin/sh -n regression test for the ssh attach retry script just landed in #14561 with you as co-author, so a compound attach command can't break that script again. Thanks again @lintanghui :D

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.

[Bug] Generated cmux-ssh-startup script fails with syntax error near then

2 participants