Skip to content

Fix transient SSH config scope for ProxyJump aliases - #1321

Closed
sunxiuguo wants to merge 1 commit into
mfat:mainfrom
sunxiuguo:fix/transient-proxyjump-1320
Closed

sunxiuguo wants to merge 1 commit into
mfat:mainfrom
sunxiuguo:fix/transient-proxyjump-1320

Conversation

@sunxiuguo

Copy link
Copy Markdown

Summary

Fixes #1320. An unsaved SSH destination writes a temporary config whose source Include currently remains inside the destination's Host block. OpenSSH's implicit ProxyJump child reuses that -F file for the jump alias, so its Include is inactive and the child misses the jump's HostName/Port.

Insert Host * immediately before the existing source Include, keeping the typed destination block first. Typed user/port/options retain OpenSSH first-value-wins precedence; jump aliases can read the source config without inheriting destination-only options. The existing ConnectionRepository → daemon launch provider → native OpenSSH path is preserved.

Add seven integration-marked native ssh -G cases in tests/core/test_transient_connections.py. They call actual repository creation and daemon launch preparation, evaluate both the generated config and launch argv, and compare destination/jump settings with a synthetic root. Coverage includes typed user/port precedence, no destination keepalive leakage into the jump, paths with spaces, nested Includes, Match originalhost/Match all boundaries, missing roots, source immutability, mode 0600/0700 permissions and cleanup. Baseline: six failed, one passed. Patched: all seven passed.

API and architecture

  • Not applicable: this change does not affect the frontend-neutral API or core/frontend ownership boundary

No public DTO, capability, authentication resolver or alternate connection path is added. No protocol-version or generated API update is needed. Independent Codex review approved the exact patch and final commit; a separate review confirmed the SFTP race attribution below.

Testing

Cloud Linux; Python 3.12.14, OpenSSH 10.0p2 Debian-7+deb13u4 and Ruff 0.15.17. New native cases use temporary configs and .invalid hosts, no real keys or exec predicates, and only ssh -G evaluation. No remote SSH sessions, ProxyCommand execution or authentication were attempted.

Passed checks:

  • PATH=/workspace/sshpilot-1320/.venv/bin:/usr/bin:/bin .venv/bin/pytest -ra tests/core/test_transient_connections.py -m integration -n 0: 7 passed, 14 deselected; independently rerun with the same result. Restricted PATH prevents the generic integration hooks from finding Docker and sweeping containers.
  • .venv/bin/pytest -ra tests/core/test_transient_connections.py -m 'not integration and not gui' -n 0: 14 passed, 7 deselected.
  • .venv/bin/ruff check src/sshpilot/core/connections/transient.py tests/core/test_transient_connections.py: passed.
  • git diff --check: passed.
  • .venv/bin/python scripts/generate_api_artifacts.py --check: passed.

Broader checks were run and are not green:

  • .venv/bin/pytest -ra -m 'not integration and not gui': head 8,312 passed, 13 failed, 95 skipped, 3 warnings; untouched base 8,313 passed, 12 failed, 95 skipped, 3 warnings. The expression also selects property/stress tests and opt-in e2e cases, which skip without their environment.
  • .venv/bin/pytest -ra tests/core tests/architecture tests/daemon/test_connection_launch_provider.py tests/daemon/test_ssh_launch.py tests/daemon/test_ssh_launch_parity.py -m 'not integration and not gui' -n 2: 1,311 passed, 3 failed, 1 skipped. All three failures reproduce on the untouched base.
  • .venv/bin/ruff check src/ tests/ scripts/generate_api_artifacts.py: three unchanged findings: RUF022 in src/sshpilot/api/models/__init__.py:373, UP012 in src/sshpilot/window_tabs.py:85, PIE807 in tests/test_connection_dialog_fixes.py:564. These files are independently verified byte-identical to base.

The twelve shared broad failures, each observed on both head and untouched base, are:

tests/test_effective_config_diff.py::test_default_config_path_excludes_system_defaults
tests/test_protocol_suggestion_messages.py::test_real_catalogue_formats_translated_reason_without_touching_diagnostic
tests/test_transfer_error_messages.py::test_real_french_catalogue_formats_capability
tests/test_macos_menubar.py::test_menubar_installed_at_startup_not_construction
tests/test_macos_menubar.py::test_menubar_never_installed_off_macos
tests/core/test_dependency_boundary.py::test_daemon_imports_only_allowed_edges
tests/daemon/test_sftp_session_death_stuck_tab.py::test_dead_connection_fails_service_and_signals_the_tab
tests/daemon/test_socket_security.py::test_insecure_directory_is_refused
tests/architecture/test_frontend_closure.py::test_active_frontend_has_no_direct_backend_process_or_secret_ownership
tests/architecture/test_frontend_closure.py::test_plugin_gap_identities_are_still_exactly_deferred
tests/sidecar/test_sidecar_stateful.py::test_sidecar_identity_reconciliation_normal
tests/sidecar/test_sidecar_stateful.py::test_sidecar_identity_reconciliation_stress

The thirteenth head failure was tests/daemon/test_transfer_batch_real_sftp.py::test_losing_the_server_mid_batch_stops_short_with_what_landed. It passed in the uninstrumented full baseline run; that result is preserved. Twenty additional ordinary isolated invocations per checkout all pass, as does the original eight-test module on each checkout.

Controlled native scheduling independently reproduces the saved failure on both base and head: when the worker resumes before the reader marks the client closed, real pipe writes produce EPIPE and the unchanged transfer runtime counts all five items handled, failing the original 5 < 5 assertion. Three controls per checkout fail identically; the opposite reader-first order passes three times per checkout and stops at one item with no per-item failures. The temporary diagnostic plugin changes only synchronization, fabricating no packets/errors/summary values, and is excluded from this PR. Relevant SFTP code and fixtures are byte-identical to base and bypass transient creation/launch. This establishes a pre-existing disconnect race; it does not turn the historical broad run green or fix that unrelated defect.

Limits: Python 3.13, real GTK/desktop, packaged deb/Flatpak/macOS, actual SSH/ProxyJump child sessions, authentication/agents/key use, Match exec/canonicalization, packaging/resource builds and remote CI are untested. Broader headless tests use their existing synthetic local files, sockets, pipes and binaries. Optional MCP/pexpect, GTK/Cairo/gettext and other environment-dependent checks skip where unavailable. No remote CI success is claimed.

AI assistance: implementation, regression-test development and separate independent reviews used OpenAI Codex. No CLA acceptance or DCO sign-off is asserted.

Reset to Host * before including the source root, preserving typed destination option precedence while allowing jump aliases to read their own configuration.

AI assistance: implemented and independently reviewed with OpenAI Codex.
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-13T01:24:07.233233Z 46dc453 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@mfat

mfat commented Oct 13, 2026

Copy link
Copy Markdown
Owner

Many thanks for the PR, I've already fixed the issue. Closing.

@mfat mfat closed this Oct 13, 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.

[BUG] ProxyJump configuration not being read in ssh conf

2 participants