Skip to content

Test SSH PTY attach retry loop with compound attach commands - #14561

Merged
teamleaderleo merged 1 commit into
mainfrom
salvage/9448-sh-syntax-tests
Sep 25, 2026
Merged

teamleaderleo merged 1 commit into
mainfrom
salvage/9448-sh-syntax-tests

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

Salvaged from #9448 by @lintanghui. The fix itself landed in #9425, so #9448 was closed, but its regression test for the main retry builder never landed. This is a tests-only PR.

What it adds

SSHPTYAttachRetryScriptBuilderTests.retryLoopIsValidPOSIXShellForCompoundAttachCommands:

  • Builds the retry loop around a compound attach command (if ...; then ...; fi), the shape that used to produce "syntax error near unexpected token `then'".
  • Runs /bin/sh -n on the generated script.
  • Runs the script and checks the command still sees the exported CMUX_SSH_PTY_ATTACH_WRAPPER_CAN_RETRY, ..._NO_PROGRESS_RETRY and ..._NO_PROGRESS_LIMIT values (1/0/3).

Not ported

#9448 also had a matching test on SSHPTYAttachExitCode.retryLoopLines. On main that entry point is a deprecated wrapper that delegates to SSHPTYAttachRetryScriptBuilder, and noProgressRetryLoopLines is already covered by the #9425 test in cmuxTests/SSHPTYAttachNoProgressRetryTests.swift, so a second copy would add no coverage.

CI

CmuxFoundation is in the swift-package-tests lane package list in ci-macos.yml, and select_package_tests.py selects it for changes under Packages/macOS/CmuxFoundation/, so this runs on the PR. check-test-determinism.py --strict is clean for the package tests.

Co-authored with @lintanghui (credited in the commit trailer).

🤖 Generated with Claude Code


Summary by cubic

Adds a regression test for the SSH PTY attach retry script builder covering compound attach commands.

Written for commit b763d58. Summary will update on new commits.

Review in cubic

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>
@coderabbitai

coderabbitai Bot commented Sep 25, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 5 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: e8b300fc-85ae-4608-bf8e-e8ed5a96cb58

📥 Commits

Reviewing files that changed from the base of the PR and between 64b150b and b763d58.

📒 Files selected for processing (1)
  • Packages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/SSHPTYAttachRetryScriptBuilderTests.swift

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.

@github-actions

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@teamleaderleo
teamleaderleo merged commit c2a6f79 into main Sep 25, 2026
44 of 45 checks passed
@teamleaderleo
teamleaderleo deleted the salvage/9448-sh-syntax-tests branch September 25, 2026 11:44
@github-actions

Copy link
Copy Markdown
Contributor

Merge receipt for b763d58004, merged 2026-09-25 11:44:52 UTC

  • Not verified at merge: ci-status (not reported), macOS compile admission (in progress), swift-package-tests (in progress)
  • Verified: Web complexity, web-validation, detect-ios-changes, Fast static checks, GhosttyKit release check, guards (6), ios-tests, linux-preflight, macOS admission gate, package-conventions-lint, runner, Testbox broker trust boundary
  • Skipped by policy: browser, Claude wrapper regressions, ios-simulator, ios-simulator-build, mobile-core-package, remote-daemon, suite-coverage, web, web-build, web-database-tests, web-tests
  • Full suite: runs on main after merge.

Labeled merged-unverified: if main breaks near this merge, look here first.

@github-actions github-actions Bot added the merged-unverified A judging check was not green at merge; see the merge receipt comment label Sep 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged-unverified A judging check was not green at merge; see the merge receipt comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant