Skip to content

fix(pi-fff): handle Windows cross-volume external paths - #684

Merged
dmtrKovalenko merged 1 commit into
dmtrKovalenko:mainfrom
RunMintOn:fix/pi-fff-cross-volume-external-path
Jul 20, 2026
Merged

dmtrKovalenko merged 1 commit into
dmtrKovalenko:mainfrom
RunMintOn:fix/pi-fff-cross-volume-external-path

Conversation

@RunMintOn

Copy link
Copy Markdown
Contributor

Problem

routePathConstraint() decides whether a path parameter points
outside the workspace and should be served by an aux (secondary)
file index. The existing outside-workspace check only handles
Unix-style ../ and Windows ..\ escapes:

if (rel !== ".." && !rel.startsWith(`..${path.sep}`)) return null;

On Windows, path.relative('D:\workspace', 'C:\target') returns
C:\target — an absolute path, not a ..-prefixed one. This
slips through the check, the path is misclassified as
workspace-local, and downstream code rejects it with:

Path constraint must be relative to the workspace

Fix

  1. Introduce isOutsideWorkspaceRelativePath() — a named, testable
    helper that checks:

    • path.isAbsolute(rel) — catches Windows cross-volume results
    • rel === ".." — parent directory escape
    • rel.startsWith(..${path.sep}) — nested escape (../foo or
      ..\foo)
  2. Use it in routePathConstraint() in place of the inline
    condition.

The change is a single semantic addition (path.isAbsolute()) that
completes the existing "is this path outside the workspace?" test
for a case that was simply overlooked.

Testing

  • npm run typecheck --workspace=@ff-labs/pi-fff: passes
  • bun test test/aux-finders.test.ts with the new Windows test: 1
    pass, 0 fail (20 existing tests unchanged)
  • Gated on process.platform === "win32" — skipped on Linux/macOS CI

Scope

packages/pi-fff/ only. No changes to Rust crates, C header, MCP
server, Neovim plugin, or other packages.

On Windows, `path.relative()` returns an absolute path when the source
and target are on different drives (e.g. `D:\` → `C:\`). The existing
check in `routePathConstraint()` only recognizes `".."` and `"..\..."`
as indicators that a path is outside the workspace — it misses the
cross-volume case entirely. The path is then treated as workspace-local,
and downstream code rejects it with:

    Path constraint must be relative to the workspace

Extract the workspace-outside check into
`isOutsideWorkspaceRelativePath()` so the logic is testable in
isolation, and add `path.isAbsolute()` to the condition. A cross-volume
relative result is by definition outside the workspace.

Add a Windows-specific regression test that verifies the helper
recognizes a cross-volume `path.win32.relative()` result as outside the
workspace. The test is gated on `process.platform === "win32"` and has
no effect on Linux or macOS CI runs.

@dmtrKovalenko dmtrKovalenko left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

thanks

@dmtrKovalenko
dmtrKovalenko merged commit 9dee049 into dmtrKovalenko:main Jul 20, 2026
52 checks passed
abhijit-s pushed a commit to abhijit-s/fff that referenced this pull request Jul 21, 2026
Upstream 0.10.1 batch (5 commits): pi-fff fixes (install fff-bun dmtrKovalenko#689/dmtrKovalenko#694,
Windows cross-volume paths dmtrKovalenko#684), node/bun Android arm64 Termux support (dmtrKovalenko#695),
and the 0.10.1 release/version bumps. No Rust source changes reach the fork —
all JS-SDK/packaging/CI.

Conflicts (config only, no code):
- All crate Cargo.toml + workspace: take ours (fork stays 0.17.1; upstream's
  0.10.1 is its own release line). fff-mcp keeps daemon deps (fff-ipc/dirs/libc).
- install-mcp.sh: ours (fork installs from HEAD/source, not upstream release-pin).
- release.yaml: ours — keep the Android C-FFI matrix entry commented, consistent
  with the fork's policy of disabling non-macOS C builds to keep CI lean.
- Cargo.lock: ours (workspace unchanged vs f5db5c1).

Daemon rust verified intact (server.rs proxy path present); build-daemon green.
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.

2 participants