Skip to content

fix: normalize Windows file URIs for DAP source paths - #122

Open
dumanoglu1 wants to merge 2 commits into
0xMiden:nextfrom
dumanoglu1:fix/windows-file-uri-source-paths
Open

fix: normalize Windows file URIs for DAP source paths#122
dumanoglu1 wants to merge 2 commits into
0xMiden:nextfrom
dumanoglu1:fix/windows-file-uri-source-paths

Conversation

@dumanoglu1

Copy link
Copy Markdown

What changed

  • Normalize Windows drive-letter file URIs from file:///C:/... to C:/... in DAP source path matching.
  • Apply the same drive-letter normalization in TUI DAP state path handling and standalone DAP source-file loading.
  • Add regression coverage for Windows file:///C:/... source path matching with and without source path prefixes.

Why

The existing code stripped file:// but kept the extra slash before Windows drive letters. That made file:///C:/workspace/src/main.masm become /C:/workspace/src/main.masm, which does not match the regular Windows path form used by editor/DAP requests or source path prefixes.

Fixes #121

Validation

  • cargo test -p miden-debug-engine --features dap source_paths_match_only_uses_declared_trim_prefixes
  • cargo check --workspace --features dap
  • git diff --check

Note: cargo test --workspace --features dap source_paths_match_only_uses_declared_trim_prefixes was also attempted, but the full workspace compile exceeded the local 180s timeout before completion. The targeted DAP engine test and workspace check both passed.

@djolertrk
djolertrk self-requested a review August 18, 2026 11:16
Comment thread src/ui/state.rs Outdated
let path = path.strip_prefix("file://").unwrap_or(path);
let path = path.replace('\\', "/");
let path = path
.strip_prefix('/')

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, could we use Uri::to_path() here and in other places?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in f64ff66: both the UI loader and DAP path conversion now use Uri::to_path() instead of manually stripping the file URI prefix.

Comment thread crates/engine/src/exec/dap.rs Outdated
let path = path.strip_prefix("file://").unwrap_or(path);
let path = path.replace('\\', "/");
let path = path
.strip_prefix('/')

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we normalize drive-letter casing and add a regression test?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 2958280 (then retained through f64ff66): Windows drive letters are normalized and regression coverage includes uppercase/lowercase drive-letter file URIs.

@dumanoglu1

Copy link
Copy Markdown
Author

Addressed in 2958280:

  • Normalizes Windows drive letters to uppercase after removing the extra slash from file:///C:/... style paths.
  • Covers both /c:/... and c:/... inputs.
  • Added regression tests for Windows file URI paths and non-Windows paths.

Validation:

  • cargo fmt --check
  • cargo test --lib dap_server::tests

@dumanoglu1

dumanoglu1 commented Aug 22, 2026

Copy link
Copy Markdown
Author

Addressed the review points in f64ff66:

  • normalize_source_path now delegates file URI/path handling to miden_debug_types::Uri::to_path() in both the DAP engine and TUI helpers.
  • Windows drive letters are normalized before comparison.
  • Regression coverage now includes drive-letter casing and file://localhost URI handling.

Validation run locally on Windows:

  • cargo test -p miden-debug-engine --features dap source_paths_match_only_uses_declared_trim_prefixes
  • cargo check -p miden-debug --features tui,dap

I also ran cargo test -p miden-debug-engine --features dap; it passed 52/53 and failed only debug::stacktrace::tests::resolves_relative_source_locations_from_filesystem, which is outside this PR's touched code path and appears to be an existing Windows path separator expectation.

Comment thread src/dap_server.rs Outdated
@bitwalker

Copy link
Copy Markdown
Collaborator

Submissions must have signed commits - this cannot be merged without that. GitHub tells you this, so we shouldn't have to ask you to do it.

@dumanoglu1
dumanoglu1 force-pushed the fix/windows-file-uri-source-paths branch from f64ff66 to 40421b9 Compare August 30, 2026 01:17
@dumanoglu1

Copy link
Copy Markdown
Author

Thanks, fixed. I squashed the branch into a single SSH-signed commit and force-pushed it. GitHub now reports the commit as verified.

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.

Windows file:// source URIs keep an extra slash before drive letters

3 participants