[otavio]

Summary

Covers Domain 12 (SSH Legacy Mode) of shellhub-io/team#243 with testcontainers tests, plus one Playwright spec for the [both] public key item. It also closes the gaps the 2026-10-02 audit found in the SSH tests that were already ticked.

tests/
├── environment/ssh.go          # SSHHostKey, AwaitDeviceOffline, AgeSessionKeepAlive
├── ssh_helpers_test.go         # runOnDevice, sendSCPTree (scp sink protocol, recursive)
├── ssh_public_key_test.go      # hostname, tag and username rules both ways; the 0.6.0 agent gate
├── ssh_connection_test.go      # ciphers, banners, disconnect cleanup, server close,
│                               #   server restart, keep-alive against the session reaper
├── ssh_test.go                 # builtin, SFTP mkdir/ls, recursive SCP; strengthened cases
└── identity_access_test.go     # handshakeWith: handshake with a whole client config
ui/apps/console/e2e/terminal.spec.ts  # register a key in the console, ssh in with it

Each refusal asserts how the server refused. A filtered key must fail with [none publickey], and the server must log refused the offered public key with the filter or version reason for that SSHID. A wrong password and an unregistered key must fail with unable to authenticate, no longer any error.

Audit gaps closed in the existing tests:

• SCP upload and download compare content.
• Host key verification compares against the key mounted into the server, with a mismatch case.
• Agent reconnects wait for offline before waiting for online, then run a command through the new tunnel.
• Cipher negotiation reads back the cipher the connection picked, one cipher at a time.
• "Keepalive prevents timeout" now has a timeout to beat. A server restart orphans one session, the test ages it and a live idle session past SHELLHUB_SESSION_KEEPALIVE_TIMEOUT, and the reaper retires only the orphan.

Not covered:

• "Session timeout on idle". Neither the gateway nor the agent sets an idle or max timeout on its SSH server, so there is nothing to time out. The old "session timeout behavior" case is renamed to what it shows.
• "Signal forwarding". This is a product bug, filed as shellhub-io/team#285. The gateway forwards the signal request, but the agent never calls Session.Signals, so the forked gliderlabs session buffers the signal and drops it. A probe trapping INT in an exec never saw the trap fire, with or without a PTY. The old case sent signals and read nothing back, so it is removed.
• Firewall Rule Evaluation (10 items), "License device limit blocks connection", and the [both] firewall deny item. These are Enterprise. qa.yml runs the community stack, where EvaluateFirewall and EvaluateLicense have no evaluator, so CI would never run such tests.

Observed, not fixed here:

• Closing a session through POST /api/sessions/:uid/close answers 202 and ends the client's channel, but the gateway keeps the client's connection open and the session active until the client leaves. OpenSSH leaves at once with status 255; a client that keeps its connection holds a dead one. Filed as shellhub-io/team#297.

Evidence

• Before: 18 of the open items had no test that asserts their outcome. The audit found the SCP, host key, reconnect and auth-failure cases assert less than their names, and unticked signals, ciphers and keep-alive.
  After: every new and changed test passes:

  TestSSH (both transports)            new and strengthened cases pass
  TestSSHPublicKeyRules                6/6
  TestSSHPublicKeyRefusedBelowAgent060 2/2
  TestSSHConnectionBehaviour           12/12
  TestSSHServerRestart                 2/2
  Playwright: Public Keys              1 passed
  

  The full tests/ suite ran 568 subtests and 1 failed: the offline-device banner case waited 30s for the stopped agent's device to go offline. When a tunnel's close never reaches the server, the server only finds it at the next yamux ping, every 35 seconds, so AwaitDeviceOffline now waits 90s. That case then passed 4 times in a row.

• Probes against a running stack show what the uncovered items do today. The signal trap never fires. Each default cipher negotiates and reads back as itself, and CBC is refused with no common algorithm.

Merge Danger

Door: two-way

Test-only. Nothing under server/, agent/ or pkg/ changes.

Blast Radius: none

[github-actions]

Claude finished @otavio's task in 6m 42s —— View job

---

Code Review Complete

Reviewed 7 files. 4 inline issues posted. Standards: 0 violations, 4 judgement calls. Spec: 0. Correctness: 0.

Standards

No rule violations. The most useful judgement call: requireKeyRefused rebuilds the client config that handshake (tests/identity_access_test.go:52) already provides (tests/ssh_public_key_test.go:156-160). A one-line suggestion is attached.

Spec

The Spec axis did not run. The PR body and its commits cite no issue (no Fixes: trailer), so there is no spec to review against.

Additional notes

• The Correctness pass found no defects:
  • sendSCPTree follows the scp -t sink protocol: D/C/E records, one ack per step, content followed by \0.
  • The requested modes and the asserted ones agree under the default umask.
  • Each test mutates only its own compose stack.
  • Every expected error string exists in x/crypto or in server/ssh.
  • Nothing under pkg/ or server/api/services changes, so the cloud repo is unaffected.
• One robustness point that is not a defect: requireSCPAck reads stdout with no deadline. A remote sink that stalls without closing would hang the test until go test -timeout.
• Test* doc comments in _test.go files were not flagged. The suite already does this consistently across about 20 files.

---

To request another review round, comment /review.

[otavio]

/review

[github-actions]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.