fix(filesystem): harden legacy filesystem path normalization and strip Windows drive slashes

[s-chudmunge]

Summary

Hardens legacy filesystem path normalization in normalize_file_path and resolve_path.

Changes

• Gated redundant leading slash stripping before Windows drive letters specifically to Windows (#[cfg(windows)]), preventing unintentional modification of legal POSIX paths like /c:notes.
• Replaced global string .replace("file:/", "") with strip_prefix for file:///, file:/, and file:\ to avoid corrupting POSIX paths that contain colons.
• Added support for standard three-slash file:/// URIs.
• Added tests for POSIX preservation in jan-utils.
• Added unit tests for resolve_path in src-tauri/src/core/filesystem/tests.rs.

[Vect0rM]

Thanks for picking this one up, @s-chudmunge — #271 has been sitting since 4 Sep and a leading slash before a drive letter is exactly the kind of thing that's obvious once someone writes it down and invisible until then. The byte-level check is also the right shape: no regex, no allocation on the common path, and it correctly refuses to fire on a two-character string.

Verified on your branch merged onto current main (64758ea):

• Fast-forwards — no conflicts.
• cargo test in src-tauri/utils — 31 passed, 0 failed, including your test_normalize_file_path_strips_redundant_leading_slash_on_windows_drives.

I can't build the Tauri crate on this machine (no GTK dev libraries), so the helpers.rs half is read rather than compiled.

There's one structural problem, and unfortunately it's the load-bearing one.

1. The fix isn't on the path the bug takes

normalize_file_path has exactly two call sites, both inside core/filesystem/helpers.rs::resolve_path — and that function serves the legacy frontend IPC commands. Its own file header says so:

// WARNING: These APIs will be deprecated soon due to removing FS API access from frontend.

The call in #271 is read(file_path='/C:\Users\TheTh\Documents\Research Vault\fracking-manuscript.md'). Three things say that call never reaches your code:

• No tool in this repo takes file_path. The agent's own reader is os.fs.read and it takes path (core/agent/tools/fs.rs:45). I searched the history back through v2.0.17 — file_path has only ever appeared in agent/eval/dataset.rs, which is unrelated. So that call is coming from an MCP server the reporter has configured.

• MCP arguments are forwarded verbatim. core/mcp/commands.rs:435 hands arguments.clone() straight to CallToolRequestParam and out over stdio. Nothing between the model and the server touches the path. The default filesystem server is @modelcontextprotocol/server-filesystem (core/mcp/constants.rs:32), spawned as a subprocess — and "outside allowed directories" is its error string, not ours.

• Even the agent's own reader wouldn't be helped. os.fs.read resolves through a different resolve_path at core/agent/tools/mod.rs:533:

  pub(super) fn resolve_path(working_dir: &Path, value: &str) -> PathBuf {
      let path = PathBuf::from(value);
      if path.is_absolute() { path } else { working_dir.join(path) }
  }

  No normalization, and this PR doesn't touch it.

So this can't close #271 as written. That's not a reason to drop the work — hardening the frontend fs API is worth having on its own — but the PR should stop claiming the issue, and the real fix needs to land where the path actually flows.

Could you ask the reporter on #271 for their mcp_config.json entry and a log line for the failing call? Once we know which server it is, we can decide whether to normalize at the MCP boundary (only for filesystem-family servers — blanket path-guessing on arbitrary tool arguments would be worse than the bug) or to fix it in the prompt.

2. The strip isn't gated to Windows

The comment says "on Windows or environments where…", but the code runs everywhere. I compiled the function as it stands and ran it:

"/c:notes"  ->  "c:notes"

/c:notes is a legal absolute path to a file named c:notes on Linux and macOS. After this change it becomes a relative path resolved against the process CWD. Rare, but it's a silent behaviour change on the two platforms the bug isn't on.

path.rs already uses #[cfg(windows)] for get_short_path, so gating is the house idiom here — either that or if cfg!(windows). The new test needs the matching gate.

3. replace("file:/", "") is a global substring replace, and the else branch now routes everything through it

This one is pre-existing in the function, but your change is what makes it reachable for every non-file: path handed to resolve_path. Same probe:

"/home/user/profile:/draft.md"  ->  "/home/user/prodraft.md"
"/tmp/makefile:/out"            ->  "/tmp/makeout"

replace hits the substring wherever it occurs, not just at the front. Colons are illegal in Windows filenames so this is POSIX-only — which, with point 2, means both new risks land on the platforms the fix isn't for. strip_prefix for both prefixes instead of replace closes it, and is what the doc comment already claims the function does.

4. The three-slash form doesn't get the new treatment

"file:///C:/Users/x/a.md"  ->  "//C:/Users/x/a.md"
"file:/C:/Users/x/a.md"    ->  "C:/Users/x/a.md"

After "file:/" is removed from file:///… the remainder starts //, so bytes[1] isn't alphabetic and the strip declines. It's harmless inside resolve_path, because that branch trims leading separators anyway — but the function is pub, the doc comment now promises drive-letter handling, and file:/// is the shape a real file URL takes. Worth a case in the test either way, so the next caller knows which it gets.

5. resolve_path itself has no test

The helpers.rs change is the half that alters behaviour for callers, and it's untested — core/filesystem/tests.rs has 6 tests, none of which touch resolve_path. A couple of cases pinning the file:/ branch against the raw branch would stop the next edit from quietly swapping them.

Nits

• cargo fmt reformats your new test (path.rs:301) — it collapses the \D:\data\models assertion onto one line. main has 3 pre-existing fmt diffs in this file at lines 183/210/236; please leave those alone, they'd bury your diff.
• No CI has run on this branch yet — total_count: 0 on the check runs.

Point 1 is the one that decides what this PR is. If the answer is "harden the legacy fs API", I'd take it with 2–4 fixed and the Resolves #271 line dropped. Either way the byte check itself is good, and you found a real gap 🪟

---

Generated by Claude Code

[Vect0rM]

Thanks for the fast turnaround, @s-chudmunge — you took the right fork in the road. Reframing this as "harden the legacy fs path helpers" rather than "closes #271" is exactly what I hoped for, and the title and description now describe what the diff actually does. Points 2, 3 and 4 from last round are all addressed, and 5 has a start.

The strip_prefix change is worth calling out specifically, because it fixes a problem that didn't exist yet. Routing the else branch of resolve_path through normalize_file_path is what makes the old global .replace() dangerous — and you swapped it in the same commit, so the window never opened. I checked the case that would have broken:

"/tmp/a/file:/b.md"   old .replace()  ->  "/tmp/a/b.md"      (would have been silent corruption)
                      your strip_prefix ->  "/tmp/a/file:/b.md"

Verified on your branch merged onto current main (64758ea):

• Fast-forwards — no conflicts.
• cargo test in src-tauri/utils — 31 passed, 0 failed. Same count as last round, but for a different reason: the Windows test is now cfg-gated out on this runner and your new POSIX test takes its slot. More on that in point 2.
• cargo clippy --all-targets on jan-utils — zero warnings in path.rs. (16 elsewhere in the crate; identical on main.)

Still can't build the Tauri crate here — gdk-3.0 isn't installed on this runner — so test_resolve_path is read, not run. I did check the one assumption in it I could isolate: Path::starts_with("https://") really does match PathBuf::from("https://example.com/model.bin"), because the components come out ["https:", "example.com", "model.bin"]. So that assertion should hold.

Two things I'd like before merge, then some smaller ones.

1. file:/// is now broken on POSIX, and the new test pins it

I compiled both versions and ran them side by side:

input	main today	this PR
file:///home/user/file.md	//home/user/file.md	home/user/file.md
file:///	//	`` (empty)

file:///home/user/file.md is the standard URI for an absolute POSIX path. Stripping all three slashes turns it into a relative path — the leading / is the path, not part of the scheme. main's //home/... was accidental but at least still absolute; this is a regression in the one direction that matters.

Your new test asserts the new behaviour as correct:

assert_eq!(
    normalize_file_path("file:///home/user/file.md"),
    "home/user/file.md"
);

Honest scoping: this is not reachable through resolve_path today. That branch does trim_start_matches('/') and joins onto the data folder, so //home/…, /home/… and home/… all land in the same place. I checked all three forms through the merged resolve_path and they're byte-identical. But normalize_file_path is pub in jan_utils, and your new doc comment now advertises file:/// handling — so the next caller gets the wrong answer with a docstring telling them it's right.

The fix is to strip two slashes (the empty authority), not three, and let the drive-letter check you already wrote handle the rest:

let stripped = if let Some(rest) = path.strip_prefix("file://") {
    rest
} else if path.starts_with("file:/") || path.starts_with("file:\\") {
    &path["file:".len()..]
} else {
    path
};

I patched that in and measured it, first as-is and then with the cfg(windows) block forced on to simulate a Windows build:

                                    posix build        windows build
file:///home/user/file.md      ->   /home/user/file.md   /home/user/file.md
file:///C:/Users/x/a.md        ->   /C:/Users/x/a.md     C:/Users/x/a.md
file:/C:/Users/x/a.md          ->   /C:/Users/x/a.md     C:/Users/x/a.md
file:\C:\Users\x\a.md          ->   \C:\Users\x\a.md     C:\Users\x\a.md
/C:\Users\User\Documents\f.md  ->   /C:\Users\...\f.md   C:\Users\User\Documents\f.md
\D:\data\models                ->   \D:\data\models      D:\data\models
/home/user/profile:/draft.md   ->   /home/user/profile:/draft.md  (both)

All four assertions in your Windows test still pass, POSIX absoluteness is preserved, and resolve_path's output is unchanged for every input I tried — the trim in that branch absorbs the difference. It also makes file:/home/user/file.md correct (/home/user/file.md), which main gets wrong too.

file:///home/user/file.md in the POSIX test should then expect /home/user/file.md.

2. Nothing in CI runs either of your new tests

This one isn't your fault, but the cfg(windows) gate changed who can see the fix work, so it's worth knowing.

There's no PR workflow in this repo — release.yml is it, and its Rust step is filtered:

cargo test --manifest-path src-tauri/Cargo.toml --lib --features test-tauri core::agent::

core::agent:: only. The jan-utils suite and core::filesystem::tests run under make test (Makefile:390–398) and nowhere else. So:

• test_normalize_file_path_strips_redundant_leading_slash_on_windows_drives now needs a Windows machine running make test by hand. Before the gate, it at least ran for anyone on Linux or macOS.
• test_resolve_path never runs automatically on any platform.

Gating the behaviour to Windows is right — I asked for it. But the byte check itself is pure string logic with nothing platform-specific about it, so it doesn't have to be gated to be tested. Lifting it into a small always-compiled helper gets you coverage everywhere:

/// Strips one leading separator from `\C:\…` / `/C:/…`. Always compiled so it
/// can be tested off-Windows; only *applied* on Windows.
fn strip_windows_drive_slash(s: &str) -> Option<&str> {
    let b = s.as_bytes();
    (b.len() >= 3
        && (b[0] == b'/' || b[0] == b'\\')
        && b[1].is_ascii_alphabetic()
        && b[2] == b':')
        .then(|| &s[1..])
}

Then normalize_file_path keeps its #[cfg(windows)] block and just calls it, and the test of the byte logic loses its gate. Your call — but as it stands the actual fix has no automated coverage on any runner we have.

3. test_resolve_path doesn't test what this PR changed

Both cases are good to have, but neither exercises the new line:

• file://models/model.gguf goes through the if branch, which this PR doesn't touch.
• https://example.com/model.bin hits the URL early-return before normalization can matter.

The change is in the else branch. A case that would actually fail without it:

// A non-file: path is passed through untouched, including embedded "file:/"
let weird = resolve_path(app.handle().clone(), "/tmp/a/file:/b.md");
assert_eq!(weird, std::path::PathBuf::from("/tmp/a/file:/b.md"));

(That one needs a #[cfg(unix)] gate, and note resolve_path runs canonicalize().unwrap_or(path) on the way out, so it only holds for a path that doesn't exist. A #[cfg(windows)] twin asserting /C:\… → C:\… would cover the other half.)

4. Resolves #271 is still in the branch

The description dropped it — good — but commit ac30484 still carries it:

fix(filesystem): strip leading slash before Windows drive letters in normalize_file_path

Resolves #271

GitHub honours closing keywords in commit messages as well as PR bodies. A squash merge uses the PR title and body, so it'd be fine — but a merge commit would auto-close #271, which we established this doesn't fix. Worth amending so it can't depend on which button gets pressed.

Nits

• cargo fmt — two new diffs. path.rs:310 collapses the \D:\data\models assertion onto one line (same one as last round), and filesystem/tests.rs:105 wants the assert_eq! on the https:// case split across lines. main has 3 pre-existing diffs in path.rs — please leave those.
• /c:notes isn't in the POSIX test. It's the case from my last comment, it works correctly now, and it's the one a reader will wonder about. One line.
• Forward slash in frontof path with 'read' #271 is still waiting. No follow-up on the issue yet asking the reporter for their mcp_config.json and a log line. Not a blocker for this PR, but it's the thread that leads to the actual bug.
• Still no CI on the branch — check runs come back total_count: 0.

Fix 1, decide on 2, and I'll take this. The byte check is correct and the strip_prefix swap was the right instinct — the remaining problem is only about which slash belongs to the scheme 🪟

---

Generated by Claude Code