Skip to content

fix(core/build): build test file:// URLs with Url::from_file_path (#3137) - #3138

Merged
trunk-io[bot] merged 1 commit into
mainfrom
claude/dora-issue-3137-i73bik
Aug 13, 2026
Merged

fix(core/build): build test file:// URLs with Url::from_file_path (#3137)#3138
trunk-io[bot] merged 1 commit into
mainfrom
claude/dora-issue-3137-i73bik

Conversation

@phil-opp

Copy link
Copy Markdown
Collaborator

Fixes #3137 — the test-cross-platform nightly failure on the Windows runner.

The failure

Two tests in libraries/core/src/build/git.rs failed on the Windows runner (333 passed, 2 failed):

---- build::git::tests::copy_and_fetch_promotes_into_target stdout ----
thread 'build::git::tests::copy_and_fetch_promotes_into_target' panicked at libraries\core\src\build\git.rs:1259:45:
called `Result::unwrap()` on an `Err` value: Error { code: -1, klass: 2,
  message: "failed to resolve path 'file://C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\.tmp9Db4Gl\\origin':
            The filename, directory name, or volume label syntax is incorrect." }

Line 1259 was the test helper clone_from_origin:

let url = format!("file://{}", origin.display());
git2::Repository::clone(&url, dest).unwrap();

format!("file://{}", path.display()) is only well-formed for a /-rooted Unix path. On Windows it produces file://C:\Users\..., which puts the drive letter in the URL authority and leaves the backslashes as non-separators, so the string doesn't address the repo at all and libgit2 refuses to resolve it.

Introduced by #2809 (merged 2026-08-11), which added the Copy/Rename promotion tests and this helper; the nightly failed the next morning. The other format!("file://…") sites in the module survived only because they funnel the string through Url::parse, and the url crate silently normalizes the malformed spelling to file:///C:/….

Production is not affected. clone_into (libraries/core/src/build/git.rs:632) takes an already-parsed url::Url, and GitManager::choose_clone_dir parses the descriptor's git: field with Url::parse, so a real build never sees the raw format! spelling. This is a test-side bug only — the same run's new_clone_promotes_temp_into_target_and_cleans_up, which clones through a parsed Url, passed on that Windows runner.

The change

Route every test-side file:// construction through one helper:

fn file_url(path: &Path) -> Url {
    Url::from_file_path(path).expect("test repo path must be absolute")
}

Url::from_file_path emits the well-formed file:///C:/Users/… on every platform, so the tests now exercise the same URL shape production does. Six call sites converted; clone_from_origin passes file_url(origin).as_str() straight to git2.

Two Url::parse(&repo_url_str).unwrap() round-trips fall out as dead weight, since the helper hands back a Url the tests can pass to clone_dir_path directly.

No production code changed; no behavior change on Linux/macOS, where the old and new spellings are byte-identical.

Validation

Class A (test-only, no behavior change), per docs/agentic-qa-policy.md:

  • cargo test -p dora-core --all-features339 passed, 0 failed (the 23 build::git::tests::* included)
  • cargo fmt --all -- --check — clean
  • cargo clippy -p dora-core --all-features --all-targets -- -D warnings — clean
  • /review — no findings; /simplify — applied its findings (dropped a self-referential round-trip test that only exercised the url crate, trimmed the duplicated comment, removed the two redundant re-parses)

Not run locally: the full cargo test --all sweep — this container ran out of disk partway through linking the workspace's example binaries (No space left on device), not on any test failure. Since the change lives entirely inside dora-core's #[cfg(test)] module, no other crate compiles it. The Windows half of the fix is verified by the nightly test-cross-platform job, which is what has to go green.

Out of scope

tests/hub-smoke.rs has six instances of the same format!("file://{}", src.display()) idiom (lines 107, 636, 687, 710, 770, 810). Its nightly job is ubuntu-latest only, so they are not currently broken; worth a follow-up if hub-smoke ever runs on Windows.


Generated by Claude Code

@trunk-io

trunk-io Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

😎 Merged successfully - details.

Copy link
Copy Markdown
Collaborator Author

🤖 Automated review by Claude — this review was generated fully automatically, with no human in the loop.

I reviewed this change and don't see any issues. The six converted sites all pass absolute tempfile-derived paths to Url::from_file_path, so the .expect(...) on absoluteness can't trip, and from_file_path doesn't require the path to exist (covering the not-yet-created repo paths and the intentionally-missing path in the clone-failure test). Dropping the two Url::parse(...) round-trips is equivalent now that the helper returns a Url directly, and on Linux/macOS the emitted URL is byte-identical to the old spelling, so there's no behavior change there. The one remaining hand-built file:// literal is the Windows drive-letter parse test, which correctly keeps its literal form.


Generated by Claude Code

@phil-opp
phil-opp marked this pull request as ready for review August 12, 2026 11:14

Copy link
Copy Markdown
Collaborator Author

The Audit (cargo-audit + cargo-deny) failure is not from this diff, and I'm not fixing it here.

Crate:    webbrowser
Version:  1.2.1
Title:    Unix `BROWSER` handling allows browser argument injection
ID:       RUSTSEC-2026-0257
Solution: Upgrade to >=1.2.2
error: 1 vulnerability found!

Evidence it's environmental rather than mine:

  • This PR touches exactly one file (libraries/core/src/build/git.rs, test module only) and does not modify Cargo.lock.
  • The same job passed on this very commit (31d8150) at 10:09 UTC (run 31586137360) and failed at 11:14 UTC (run 31590973049). qa-audit re-fetches the RustSec DB each run, so the advisory landed in the database between those two runs — it will fail on main and on every open PR until the lockfile moves.

#3006 (chore: Update Cargo.lock) already carries the fix: webbrowser 1.2.1 → 1.2.4, plus lru 0.18.1 → 0.18.2 for the RUSTSEC-2026-0253 warning. That's the right vehicle — duplicating a workspace-wide lockfile bump inside a test-only Windows fix would just create a conflict with it. Once #3006 lands, this PR needs a rebase/merge of main to pick it up.

Everything else on this PR is green: Format, Clippy, Check, Typos, Unwrap budget, License check.


Generated by Claude Code

)

`format!("file://{}", path.display())` is not portable. On Windows it
yields `file://C:\Users\...`, which puts the drive letter in the URL
authority and leaves the backslashes as non-separators, so libgit2
refuses to resolve it:

    failed to resolve path 'file://C:\Users\RUNNER~1\...\origin':
    The filename, directory name, or volume label syntax is incorrect.

The tests added in #2809 hand that string straight to
`git2::Repository::clone`, which broke `copy_and_fetch_promotes_into_target`
and `rename_and_fetch_promotes_into_target` on the nightly Windows runner.
The sites that funnelled the same string through `Url::parse` survived
only because the url crate silently normalizes the malformed spelling.

Route every test-side `file://` construction through one `file_url`
helper built on `Url::from_file_path`, which emits the well-formed
`file:///C:/Users/...` on all platforms. Production is unaffected:
`clone_into` already takes an already-parsed `Url`.

Two `Url::parse(&repo_url_str).unwrap()` round-trips fall out as dead
weight now that the helper hands back a `Url` directly.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C74rZYpFMVdg4B9HHt5BYL
@phil-opp
phil-opp force-pushed the claude/dora-issue-3137-i73bik branch from 31d8150 to 4510a72 Compare August 13, 2026 07:37
@trunk-io
trunk-io Bot merged commit f69801f into main Aug 13, 2026
16 checks passed
@trunk-io
trunk-io Bot deleted the claude/dora-issue-3137-i73bik branch August 13, 2026 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nightly regression since 2026-08-12

2 participants