Repository navigation
test: cover scp, tunnels and the Xdebug tunnel with mock SSH forwarding - #241
Merged
Merged
Conversation
`ssh --all` lists the SSH URLs of every app, but it selected a container
first, so it failed in non-interactive mode in a project with several
apps ("Specifying the --app or --worker is required").
Add integration tests for `ssh --pipe`, `--all`, and app and worker
selection, with helpers to serve the layout project over the mock SSH
server.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Test certificate generation and reuse, --new, --new-key, --refresh-only, the SSH configuration files, and ssh-cert:info --no-refresh with missing, expired and mismatched certificates. The mock auth server's certificate key ID and lifetime are now configurable, as the CLI only reuses a certificate whose key ID is the user ID and which expires in more than 2 minutes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Run the commands against the mock SSH server, with fake remote clients (shell scripts on the remote PATH) which print their arguments: - db:dump: default and --file names, --gzip, --stdout, --directory, table and schema options, MariaDB and PostgreSQL, and remote failures - db:sql: queries, --raw, --schema, and piped input - db:size: PostgreSQL and MariaDB usage - mount:size: df and du output parsing - mount:upload and mount:download with rsync: --exclude and --delete - service:redis-cli and service:mongo:* commands Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The mock SSH server handled one connection at a time, so a long-lived
connection such as a tunnel blocked others. It now handles connections
and channels concurrently, and Stop closes open connections and waits
for their handlers.
Add optional support for:
- the SFTP subsystem (SFTPDir), with github.com/pkg/sftp, used by scp
- local port forwarding ("direct-tcpip" channels), via DialTCP
- remote forwarding of Unix sockets ("streamlocal-forward@openssh.com"),
via ListenUnix, as used by environment:xdebug
Add an integration test for environment:scp.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When a tunnel was already open, tunnel:open and tunnel:single reported the port that a new tunnel would have used, instead of the open tunnel's port. Add integration tests for tunnel:open, tunnel:info, tunnel:list, tunnel:close and tunnel:single, forwarding connections through the mock SSH server to local services. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Forward a connection from the remote Xdebug socket to a local listener through the mock SSH server, and check that the command exits when the SSH connection closes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A forwarded connection whose peer stayed open kept its handler running after the SSH connection closed, so Stop could wait indefinitely. Also create the Xdebug test socket in /tmp, as the integration tests' temporary directory may exceed the Unix socket path length limit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A listener registered while the connection was closing could stay open, so Stop waited indefinitely. Each listener now closes itself when the connection closes. Also use a synchronized buffer for the output of tunnel:single, which the test reads while the command runs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![upsun-dispatch[bot]](https://avatars.githubusercontent.com/in/4153575?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Note
Reviewed — No blocking findings · 🔵 1 minor point
🔍 Full review · 11 files reviewed
🔵 Minor point
pkg/mockssh/server.go:42—SFTPDir,DialTCPandListenUnixare plain exported fields that callers set afterNewServerhas already calledstart()and the accept loop is running (e.g.sshServer.SFTPDir = remoteDirafterp.serveSSH,sshServer.DialTCP = ...insetUpTunnelProject).handleSession,handleDirectTCPIPandforwardUnixread them from connection goroutines without any lock or happens-before edge. Today it only works because no client connects before the write. Any test that sets one of these fields while a connection is open races, andgo test -racereports it if the package is exercised that way.
Verification
isOpennow returns the stored tunnel, and both callers (TunnelOpenCommand,TunnelSingleCommand) only pass it togetUrl, so they report the open tunnel's port.Stopsetsconnsto nil under the lock, so a connection accepted during shutdown is closed bytrackConninstead of leaked;untrackConnis safe on the nil map.- In
handleConn,connClosedis closed andwg.Wait()runs before the deferredsshConn.Close; both proxy loops and the Unix listener select onconnClosed, so handlers cannot stallStop. TestTunnelOpenregisters itstunnel:close --allcleanup afterserveSSH'sStopcleanup, so the last-in-first-out order closes the tunnels before the server stops.- Exec payloads are now decoded with
ssh.Unmarshalinstead of slicing off 4 bytes, and a subsystem request is rejected whenSFTPDiris unset.
CI runs the new integration tests (TestEnvironmentScp, TestTunnelOpen, TestTunnelSingle, TestEnvironmentXdebugTunnel) through the ci.yml step cd integration-tests && go test -count=1 ./..., without -race. PHPUnit covers the TunnelManager fix with testIsOpenReturnsTheOpenTunnel. pkg/mockssh/server_test.go does not test the new SFTP or forwarding code, so the race-enabled test-go target never runs it.
Review details
- Commit: 96db1f2
- Model: claude-opus-5-5
Review 1 of 10 for this pull request · View the full run
Skipping the container selection for `ssh --all` also skipped the deployment lookup that reported paused and inactive environments, so the command printed the SSH URLs and exited successfully. Also assert the app-not-found message in TestSSHAppSelection. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…212-mockssh-sftp-forwarding
Contributor
There was a problem hiding this comment.
Note
Reviewed — No new issues found · 1 still open
🔁 Incremental (base moved) · 2 base files checked
Outstanding from earlier reviews:
- 🔵
pkg/mockssh/server.go:42: Configuration that is only safe before the first connection, yet is set after Start, is a latent data race for future tests.. — SFTPDir, DialTCP and ListenUnix in pkg/mockssh/server.go are still plain exported fields with no lock, set by callers after the accept loop has started and read from connection goroutines. (first raised)
Verification
- The base's new TestSSHInactiveEnvironment uses assertExitCode, makeEnv and setUpProject, and each is defined once in the merged tree (tests.go:214, environment_list_test.go:73, project_setup_test.go:38).
- The new paused/inactive check in EnvironmentSshCommand only fires for
ssh --all, which none of the PR's xdebug, tunnel or scp tests run. All of those tests use active environments.
This base movement adds TestSSHInactiveEnvironment. The PR's own integration tests (tunnel_test.go, environment_scp_test.go, environment_xdebug_test.go) are unchanged. Nothing in the moved code overlaps what they test.
Review 2 of 10 for this pull request · View the full run
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…212-mockssh-sftp-forwarding
Contributor
There was a problem hiding this comment.
Note
Reviewed — No new issues found · 1 still open
🔁 Incremental (base moved) · 1 base file checked
Outstanding from earlier reviews:
- 🔵
pkg/mockssh/server.go:42: Configuration that is only safe before the first connection, yet is set after Start, is a latent data race for future tests.. — SFTPDir, DialTCP and ListenUnix (server.go:38-46) are still plain exported fields with no lock, set after start() and read from connection goroutines. (first raised)
Verification
- The base's TestMountSize calls remoteEnv(t, scripts, relationships), whose signature in project_setup_test.go still matches in the merged tree.
- mockssh.ExecHandler(workingDir, env) keeps its signature and still runs commands with bash -c, so the base's quoted heredoc df script runs as intended.
- The base commit only changes a test fixture string and adds a strings import; no symbol the PR adds or relies on was renamed or removed.
This round has no PR diff. The base's TestMountSize in integration-tests/mount_ssh_test.go runs remoteEnv and ExecHandler against the merged tree; no new test covers the interaction beyond that.
Review 3 of 10 for this pull request · View the full run
Base automatically changed from
cli-212-integration-tests-ssh-based-commands
to
main
October 15, 2026 13:32
…-forwarding # Conflicts: # CLAUDE.md
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 1 warning · 1 still open
🔍 Full review · 11 files reviewed
Outstanding from earlier reviews:
- 🔵
pkg/mockssh/server.go:44: Configuration that is only safe before the first connection, yet is set after Start, is a latent data race for future tests.. — A comment now says the fields must be set before clients connect, but the tests still set SFTPDir, DialTCP and ListenUnix after NewServer has started the accept loop, with no lock and no explicit happens-before edge. (first raised)
Verification
handleConnclosesconnClosedbeforewg.Wait(), so Unix-socket listeners andproxygoroutines shut down when the SSH connection ends, andStopdoes not hang on them.trackConnreturns false onceStophas setconnsto nil, so a connection accepted during shutdown is closed and is never left without a handler.- Calling
Stoptwice, asTestEnvironmentXdebugTunneldoes through theserveSSHcleanup, is safe: the second listener-close error is ignored and ranging over or deleting from a nil map does nothing. - The exec payload is now decoded with
ssh.Unmarshalinstead of slicing off 4 bytes, and replying to a request withWantReplyfalse is harmless in x/crypto/ssh.
New integration tests cover the changes: environment_scp_test.go and tunnel_test.go, plus TestEnvironmentXdebugTunnel. A new PHPUnit case, testIsOpenReturnsTheOpenTunnel, covers the isOpen port fix. Nothing tests the mock SSH package itself, and no test covers state entries that lack service metadata.
Review details
- Commit: b1ff16c
- Model: claude-opus-5-5
Review 4 of 10 for this pull request · View the full run
Stored tunnel entries may come from older versions without the service metadata, which getUrl() needs. Use only the open tunnel's port and PID. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #240.
The mock SSH server (
pkg/mockssh) now:Stopcloses open connections and waits for their handlersSFTPDir), withgithub.com/pkg/sftpdirect-tcpip, viaDialTCP) and remote Unix sockets (streamlocal-forward@openssh.com, viaListenUnix), both disabled unless setNew integration tests:
environment:scp,tunnel:open/info/list/close/single, and theenvironment:xdebugtunnel. Background tunnel processes exit when the tests close the tunnels, or when the mock SSH server stops.Fix: when a tunnel was already open,
tunnel:openandtunnel:singlereported the port a new tunnel would have used, instead of the open tunnel's port.🤖 Generated with Claude Code