Sign up
View stargazersSign up

fix(shared): keep Windows clone paths out of SSH conversion#24886

closed2
main
GitHub fork: fix/astra-focused-bug
opened20d agobyQQuarks07
2 files changed
View on GitHubTwo-way sync enabledPR last imported 11h ago

A local Windows remote such as C:\repos\project.git is classified as SSH. In Add to another machine, switching it to HTTPS produces an invalid address:

text
1Input: C:\repos\project.git
2Before: protocol = ssh; HTTPS = https://C/\repos\project.git
3After: protocol = null; HTTPS = null

Reuse isWindowsDrivePath before parsing network URLs. The dialog then preserves the original path and offers no protocol switch; the server's clone fallback uses the same helpers.

This is a small, focused obvious-bug fix under CONTRIBUTING: two files, 20 added lines, no dependency or contract changes. Tests cover both drive-letter cases and separators while preserving valid single-letter SSH hosts.

Verification

Rechecked against main at 8b44f42b14 after resolving the conflict with its newer Azure URL tests:

  • vp test run packages/shared/src/cloneUrl.test.ts: with the unpatched parser, the two Windows backslash cases fail (26 pass); with the fix, all 28 pass, including Azure credentials, host/path mapping, and custom-port cases.
  • A direct Node 26.12.1 execution against both parsers produced the transcript above and confirmed single-letter SSH hosts and Azure conversion retain their results.
  • vp run check:changed --concurrency 1: passed format, lint, both knip passes, typechecks for all 34 affected packages, and 29 tests across two selected files. CI owns the full suites, build, and release smoke checks.

Validation ran on Linux with Node 26.12.1. No native Windows clone or browser flow was exercised.

Original code and regression tests: GPT-6 Astra through the Codex harness in T3 Code. Conflict resolution, review, and refreshed verification: GPT-6.1-Sol through the Codex harness in T3 Code.

Checks

23 successful · 12 skipped
All checks passed
128b503

Added 1 commit

  • fix(shared): keep Windows clone paths out of SSH conversion9ceac91
Cchatgpt-codex-connector[bot]20d ago
Synced from GitHub

Codex Review Summary

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

ReviewStatusCommitReview trigger
📝 Code Review🔄 Running since 2026-10-25T13:46:41.236109Z9ceac91PR 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.

MmacroscopeappBot20d ago
Synced from GitHub

Approvability

Verdict: Approved at 9ceac91

Macroscope's review found this PR approvable — This focused bug fix prevents local Windows drive paths from being misclassified as SSH remotes, while preserving valid single-letter SSH hosts. It changes only the shared parser and adds targeted regression coverage, with no schema, default, deployment, or static-analysis configuration impact.

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

CcoderabbitaiBot20d ago
Synced from GitHub

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

parseCloneUrl now returns null for Windows drive-letter paths before matching supported URL formats. Tests cover Windows path formats and SSH forms with single-letter hosts.

Changes: Clone URL parsing

Layer / File(s)Summary
Windows paths and SSH host parsing
packages/shared/src/cloneUrl.ts, packages/shared/src/cloneUrl.test.ts		parseCloneUrl rejects Windows drive paths before URL-pattern matching. Tests verify that these paths have no detected protocol and that single-letter-host SSH forms convert to HTTPS.

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

Change: Bug fix

Suggested reviewers: juliusmarminge, zacharon

Merge Risk: External image Minimal · up to 128b5

The focused parser change and regression coverage support the intended Windows-path behavior without identified merge-blocking risk.

Pre-merge checks: Passed 4
Check nameStatusExplanation
Title checkPassedThe title is concise, uses the repository's conventional format, and clearly identifies the main change: preventing Windows clone paths from SSH conversion.
Description checkPassedThe description clearly explains the problem, implementation, scope, approval rationale, verification results, limitations, and agent details. It does not use separate Problem, Change, or Scope and ap…
Linked Issues checkPassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes checkPassedCheck skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description clearly explains the problem, implementation, scope, approval rationale, verification results, limitations, and agent details. It does not use separate Problem, Change, or Scope and approval headings, but it provides the required information and is substantially complete.

Finishing Touches
Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

External image Recent review info
External image Run configuration
External image Commits

Reviewing files that changed from the base of the PR and between 9ceac91fd1c9f6a62293c6c69b2ba94e9b0b441f and 128b503cca9c6ec5c6135368f8c95cd67d407607.

External image Files selected for processing: 2
  • packages/shared/src/cloneUrl.test.ts
  • packages/shared/src/cloneUrl.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.

Added 1 commit

  • fix(shared): resolve clone URL test conflicts with main128b503
JJulius Marminge16h ago

Thanks for this. Before a bug fix is taken further, the bug is reproduced on current main, and here it did not appear.

We ran your steps on current main (5abf38367b) with a Windows machine as the source environment and a second environment connected and paired in, so a machine without the project was available. A project whose origin is C:\repos\project.git (we also tried C:/repos/project.git) never shows the machine selector or "Add to another machine", so the clone dialog and its SSH/HTTPS switch cannot be opened for it. A control project with origin git@github.com:acme/widget.git on the same setup did offer the option, and its dialog showed the protocol toggle as expected.

The reason is on the server: when it builds a project's repository identity it already discards remotes that look like a Windows drive path. Such a project has no identity, the add-to-machine option is not offered, and the dialog only reads its clone URL from that identity. The clone URL helper would indeed misread a drive path as SSH if it were given one, but the app never passes one in, so we could not show a user-facing bug for this pull request to fix.

Closing this pull request. If it still happens for you, please open an issue with the exact steps, what you see, and the version and platform you are on; a fix is picked up from there.