Skip to content

fix(client-runtime): Local folder finds an existing UNC project typed with forward slashes - #34760

Open
juliusmarminge wants to merge 2 commits into
mainfrom
factory/issue-34755
Open

juliusmarminge wants to merge 2 commits into
mainfrom
factory/issue-34755

Conversation

@juliusmarminge

Copy link
Copy Markdown
Member

findProjectByPath (packages/client-runtime/src/state/projects.ts), the lookup used by findExistingAddProject and the web lookups, compared //server/share/repo to a stored \\server\share\repo as plain text, because normalizeProjectPathForComparison only treats a leading \\ as UNC. Now, when both paths start with two separators and at least one starts with \\ (which only a Windows environment can hold), both have / turned into \ and go through the existing comparison normaliser, so they match ignoring case. POSIX // paths are never compared that way and still match exactly, literal backslashes included (#29245), with no need to pass the platform in. I left normalizeProjectPathForComparison itself alone because server and file-metadata cache keys share it. No new regex: the change uses the existing isNetworkPath check (/^[\\/]{2}/), which only looks at the first two characters and runs in constant time on any input. I did not test against a real Windows server.

Fixes #34755

Reproduction on main

On origin/main a41b882, after vp i, I added a project with workspaceRoot \\server\share\repo to the existing 'finds existing projects by normalized path in the target environment' test in packages/client-runtime/src/operations/projects.test.ts and asserted that findExistingAddProject({ path: '//server/share/repo' }) returns it. vp test run packages/client-runtime/src/operations/projects.test.ts gives 1 failed, 9 passed: 'expected undefined to be unc-project'. This calls the same production resolver and lookup that mobile's Local folder submit uses. I did not run a native mobile client against a Windows server because no worker has a Windows environment, so the result rests on the shared lookup the UI calls.

Evidence

Being captured: before and after in the real app. This section is filled in before the pull request can merge.

Tests

vp test run packages/client-runtime/src/operations/projects.test.ts: failed 1/10 before the fix (the repro), passed 10/10 after. vp test run packages/client-runtime/src/operations/projects.test.ts packages/client-runtime/src/state: 69 files, 903 tests passed. vp run check:changed: format, lint, knip, typecheck (7 packages) and tests all passed.

Not in this change

  • normalizeProjectPathForComparison is also used for server and file-metadata cache keys (AgentSessionScanner, fileMetadata, projectGrouping keys). A forward-slash UNC spelling there is still a different key from the backslash one, and fixing that would need the destination platform passed in.

AI-assisted; verified with the tests and tools listed above.

juliusmarminge and others added 2 commits November 12, 2026 05:00
…ws project

Fails on main: reproduces #34755.

AI-assisted; verified with the tests and tools listed above.
… with forward slashes

findProjectByPath compared //server/share/repo literally, so it missed a
Windows project at \\server\share\repo. A leading \\ on either side shows the
environment is Windows, so both are then compared as UNC paths; POSIX //
paths keep their literal comparison.

Fixes #34755

AI-assisted; verified with the tests and tools listed above.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Nov 12, 2026
@macroscopeapp

macroscopeapp Bot commented Nov 12, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 7c9a5a9

Macroscope's review found this PR approvable — Existing UNC projects can now be found when their paths are entered with forward slashes. The adjustment is limited to path comparison, preserves environment-scoped lookup, and introduces no network access or product-default changes.

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

@coderabbitai

coderabbitai Bot commented Nov 12, 2026

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

Project lookup now matches an existing UNC project when the candidate path uses forward slashes. The lookup trims paths and skips blank project paths. A regression test covers the forward-slash UNC spelling.

Changes: UNC Project Path Lookup

Layer / File(s) Summary
UNC-aware project lookup
packages/client-runtime/src/state/projects.ts, packages/client-runtime/src/operations/projects.test.ts
findProjectByPath compares UNC paths after converting forward slashes to backslashes. Other paths retain normalized comparison. The test checks that //server/share/repo matches a project rooted at \\server\share\repo.

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

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: t3dotgg, macroscopeapp

Merge Risk: Minimal · up to 7c9a5

Project lookup now reuses an existing project when a UNC path is entered with forward slashes, matching the backslash spelling. POSIX double-slash matching stays case-sensitive in the common flows. No concrete merge-blocking risk was found.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title names the actual change: a fix in client-runtime so a local folder lookup finds an existing UNC project when the path is typed with forward slashes. It matches the raw summary and objectives…
Description check Passed The description covers the problem (forward-slash UNC lookup fails in findProjectByPath), the change (normalize UNC paths to backslashes only when a network path is involved, leaving POSIX // paths un…
Linked Issues check Passed Issue #34755 requires three coding behaviors. (1) A forward-slash UNC input must reuse an existing backslash UNC project. In packages/client-runtime/src/state/projects.ts, findProjectByPath now takes …
Out of Scope Changes check Passed All changes connect to #34755. The projects.ts change adds the UNC branch and its helper normalizeUncForComparison. It also adds an isNetworkPath import, which the branch uses. The test change adds a …
Full details: Title check

Explanation

The title names the actual change: a fix in client-runtime so a local folder lookup finds an existing UNC project when the path is typed with forward slashes. It matches the raw summary and objectives, which describe forward-slash UNC matching in findProjectByPath with a test. It follows the conventional commit format from the template and is specific enough for a reviewer scanning history.

Full details: Description check

Explanation

The description covers the problem (forward-slash UNC lookup fails in findProjectByPath), the change (normalize UNC paths to backslashes only when a network path is involved, leaving POSIX // paths unchanged), a reproduction on main, and tests run before and after. It also notes a known limitation that cache keys are unchanged. The template's Scope and approval section is not filled in: it names no triaged issue or maintainer approval comment, although the description cites Fixes #34755 and the objectives link that issue. The Evidence section is a placeholder that says it will be filled in before merge, and the template asks for before/after evidence for UI changes. The mobile UI path was not run natively. Because the description is mostly complete, the check passes, but the author should confirm the approval reference and complete the evidence section.

Full details: Linked Issues check

Explanation

Issue #34755 requires three coding behaviors. (1) A forward-slash UNC input must reuse an existing backslash UNC project. In packages/client-runtime/src/state/projects.ts, findProjectByPath now takes the UNC branch when both paths are network paths and either is UNC. That branch converts / to \ through normalizeUncForComparison and then applies the existing normalizer, so //server/share/repo matches \\server\share\repo ignoring case. The added test in packages/client-runtime/src/operations/projects.test.ts asserts this through findExistingAddProject. (2) POSIX // paths must keep exact, case-sensitive matching with literal backslashes. Two // paths that are not UNC (no leading \) skip the new branch and use the unchanged normalizeProjectPathForComparison exact comparison. (3) The shared lookup is the path the mobile UI uses, per the PR description. I did not verify the mobile call site directly. The PR description's Evidence section is a placeholder, but it is non-coding and does not affect this verdict. The issue's risk statement and the backslash workaround are context only.

Full details: Out of Scope Changes check

Explanation

All changes connect to #34755. The projects.ts change adds the UNC branch and its helper normalizeUncForComparison. It also adds an isNetworkPath import, which the branch uses. The test change adds a UNC fixture and a regression assertion for the forward-slash lookup. The PR also trims the candidate and each project root before comparison and skips empty roots. This changes behavior for padded inputs on all paths, not only UNC. It is a small addition that is not described in the PR description. It does not conflict with the issue, so it is not treated as out of scope. normalizeProjectPathForComparison is unchanged, so server and file-metadata cache keys are not affected, as the PR states.

Finishing Touches

Generate unit tests (beta)

  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between f991781 and 7c9a5a9.

Files selected for processing: 2

  • packages/client-runtime/src/operations/projects.test.ts
  • packages/client-runtime/src/state/projects.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

@juliusmarminge

Copy link
Copy Markdown
Member Author

The automated pipeline stopped on this pull request and will not merge it:

the proof has no capture anyone can open:

  • The base bug and head fix were not observed in a real client; both verdicts are false because neither was verified.
  • Forward-slash reuse and the backslash/case control require a Windows environment containing the existing UNC project. No such environment was available, and Linux project commands normalize paths using POSIX filesystem semantics.
  • No scenario comparison or UI recording was produced because the required starting state could not be created through the supported commands.

It stays open for a person to finish or close.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Windows Local folder misses an existing UNC project when the input uses forward slashes

1 participant