Repository navigation
Conversation
ApprovabilityVerdict: Approved at 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 You can add or adjust custom eligibility rules. Learn more. |
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| 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.
|
Note Written by Hi! We are cleaning up open PRs, and this one appears to have been created with an older model ( |
|
Note Written by Reopening, this was closed by mistake. Sorry for the noise! |
|
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. |
|
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 Closing this PR in favor of the linked replacement, which remains open for CI and review. |
Problem
SSH host discovery does not follow valid quoted
Includepaths. WithInclude "/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 andHost=...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.tsand 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.comhostnames. Read-only calls todiscoverSshHosts({})against an existing quoted Include reproduced the missing config alias on the original tokenizer and returned that alias withsource: "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.