Skip to content

fix(ssh): read quoted Include paths in host discovery - #17008

Closed
TurpoFIN wants to merge 3 commits into
pingdotgg:mainfrom
shardfluxdev:fix/ssh-config-quoted-include
Closed

TurpoFIN wants to merge 3 commits into
pingdotgg:mainfrom
shardfluxdev:fix/ssh-config-quoted-include

Conversation

@TurpoFIN

@TurpoFIN TurpoFIN commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Problem

SSH host discovery does not follow valid quoted Include paths. With Include "/path/to/ssh_config", Add environment → SSH omits hosts declared in that file even though OpenSSH resolves them. Paths containing spaces are also split into separate filenames. ssh_config(5) permits quoted arguments containing spaces.

Change

Tokenize directive arguments with OpenSSH-style quotes and escapes before resolving Include paths. Strip quoting syntax while preserving spaces and literal =/# inside the path; consume an optional = only between the directive and its arguments. Handle comments during tokenization and ignore lines with unclosed quotes.

Add config discovery regressions for unquoted and quoted absolute Include paths, spaces, =, #, and escaped quotes. The quote-in-filename case is skipped on Windows because Windows filenames cannot contain that character. Existing relative Include/glob and Host=... tests remain intact.

Scope and approval

This qualifies for CONTRIBUTING.md's very small, focused fix for an obvious bug without a prior issue or discussion: host discovery already supports Include files, but the tokenizer retains quote delimiters from standard OpenSSH configuration. The change is confined to packages/ssh/src/config.ts and its tests and restores the existing SSH suggestion flow. No new feature, setting, bootstrap, authentication, or transport behavior. Searches for existing quoted-Include issues and PRs returned no matches; no prior issue is linked.

Verification

Node v24.13.1, vp 1.0.0, pnpm 11.10.0; focused checks only:

  • vp install --frozen-lockfile — all 16 workspace projects, +1999 packages, completed in 52.8s.
  • vp test run --dir packages/ssh — 5 files passed; 42 tests passed (7.12s).
  • vp test run packages/ssh/src/config.test.ts — final focused regression run: 9 tests passed (301ms).
  • vp run --filter @t3tools/ssh typecheck — exit 0.
  • vp lint packages/ssh/src/config.ts packages/ssh/src/config.test.ts — exit 0.
  • git --no-pager diff --check — clean.

A bounded native ssh -G -F <temporary config> <alias> probe with OpenSSH_10.2p1 resolved all six fixture Include paths to their declared *.example.com hostnames. Read-only calls to discoverSshHosts({}) against an existing quoted Include reproduced the missing config alias on the original tokenizer and returned that alias with source: "ssh-config" after the fix. SSH config files were unchanged. No native Add environment UI run or Windows run was performed for this parser-only fix.

Model: GPT-6. Harness: Codex.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Oct 8, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at eefa257

Macroscope's review found this PR approvable — This is a small, self-contained SSH host-discovery bug fix that adds quoted-argument parsing and regression tests without changing schemas, defaults, deployment, authentication, or transport behavior. Its production impact is confined to correctly resolving existing SSH Include directives.

No code changes detected at 9b9dbf9. Prior analysis still applies.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9222c162-5857-429d-b8c7-1fd3b14c8163
📥 Commits

Reviewing files that changed from the base of the PR and between 9b9dbf9 and a9cade5.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 7e638b05-738e-4139-8757-13c48bd3b39b
📥 Commits

Reviewing files that changed from the base of the PR and between 3143335 and eefa257.

📒 Files selected for processing (2)
  • packages/ssh/src/config.test.ts
  • packages/ssh/src/config.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

SSH config parsing now tokenizes directive arguments with quote and selected escape handling. It treats # as a comment marker only at an argument boundary. Tests cover host discovery through Include paths with quotes and special characters.

Changes: SSH config Include parsing

Layer / File(s) Summary
Tokenize config directive arguments
packages/ssh/src/config.ts, packages/ssh/src/config.test.ts
The parser handles quoted arguments, selected escapes, and comment boundaries. Config lines go directly to the tokenizer. Tests cover Include paths with spaces, =, #, and escaped quotes. The escaped-quote case is skipped on Windows.

Priority: ⬇️ Low

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

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to eefa2

Quoted Include paths can proceed through normal merge checks; no supported host-discovery regression remains identified.

### Architecture Summary

Architecture risk: 🟡 Medium · up to eefa2

The change affects 1 system.

Changed systems: packages/ssh

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — packages/ssh (library) was modified; 2 changed files map to changed impact.

Before / after behavior

  • observed — Modified behavior in packages/ssh/src/config.test.ts: Adds the HostProcessPlatform import used to determine whether to skip a platform-specific test case.
  • observed — Modified behavior in packages/ssh/src/config.test.ts: Adds a table-driven test for discovering hosts through Include paths with several quoting and filename-character cases. It escapes embedded quotes in quoted paths, skips the escaped-quote case on Windows, and asserts the discovered host fields.
  • observed — Modified behavior in packages/ssh/src/config.ts: splitDirectiveArgs now parses the directive name and tokenizes the remainder while honoring quotes and selected escapes. It returns no arguments for an unmatched quote and treats # as a comment marker only at the start of an argument. The prior whitespace-only tokenizer and separate stripInlineComment helper—which truncated at the first # regardless of quoting or token position—are removed.
  • observed — Modified behavior in packages/ssh/src/config.ts: Config lines are passed directly to the tokenizer instead of first being stripped and skipped when empty, so hashes are interpreted according to the tokenizer’s argument-boundary and quote rules.

Reliability and maintainability

  • inferred — Risk-relevant change factors for packages/ssh: blast_radius_1; direct_dependents_1
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: fixing SSH host discovery for quoted Include paths.
Description check ✅ Passed The description covers the problem, change, scope and approval rationale, focused verification results, limitations, and required model and harness details. It is specific and aligned with the pull re…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Approvability ✅ Passed Pass. The pull request changes only packages/ssh/src/config.ts and its regression tests. It fixes SSH config argument parsing for quoted Include paths and adds focused coverage. It does not change…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@maria-rcks

Copy link
Copy Markdown
Collaborator

Note

Written by claude-opus-5-5 on behalf of Maria

Hi! We are cleaning up open PRs, and this one appears to have been created with an older model (gpt-6). If this change is really important, we recommend rebuilding the PR with a newer model if possible.

@maria-rcks maria-rcks closed this Oct 11, 2026
@maria-rcks

Copy link
Copy Markdown
Collaborator

Note

Written by claude-opus-5-5 on behalf of Maria

Reopening, this was closed by mistake. Sorry for the noise!

@maria-rcks maria-rcks reopened this Oct 11, 2026
@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 11, 2026 — with ChatGPT Codex Connector
@juliusmarminge
juliusmarminge enabled auto-merge (squash) October 18, 2026 19:12
@juliusmarminge juliusmarminge self-assigned this Oct 18, 2026
@juliusmarminge

Copy link
Copy Markdown
Member

Taking over this one. I’ll reduce the tests to the useful coverage, update the implementation against current main, and verify it before publishing the changes.

@juliusmarminge

Copy link
Copy Markdown
Member

Thanks @shardfluxdev — the original change correctly identified and handled quoted SSH Include paths. We took this over, kept your commits and the focused parser behavior, simplified the tokenizer state, and consolidated six added test cases into one configuration-level regression.

The maintainer push to your fork was rejected with HTTP 403 despite maintainer edits being enabled, so the reviewed change is now in #21396 on the main repository.

I compared the original diff with the replacement: both cover only packages/ssh/src/config.ts and its existing test file. Quoted/escaped paths, spaces, literal # and =, comments, and existing Include behavior are preserved; no intended production pieces were dropped. The reduced test covers the distinct parser paths, with 53 package tests passing and local OpenSSH ssh -G parity verified.

Closing this PR in favor of the linked replacement, which remains open for CI and review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants