Skip to content

feat: detect realpath when open workspace#486

Open
bluelovers wants to merge 3 commits into
NeuralNomadsAI:devfrom
bluelovers:pr/realpath-002
Open

feat: detect realpath when open workspace#486
bluelovers wants to merge 3 commits into
NeuralNomadsAI:devfrom
bluelovers:pr/realpath-002

Conversation

@bluelovers
Copy link
Copy Markdown
Contributor

@bluelovers bluelovers commented May 18, 2026

detect realpath when open workspace

try avoid path issue in windows

  • d:\xxx
  • D:\xxx
  • symbol link

now they are same path when try detect exists workspace

@github-actions
Copy link
Copy Markdown

PR builds are available as GitHub Actions artifacts:

https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/26067322642

Artifacts expire in 7 days.
Artifacts: (none found on this run)

@github-actions
Copy link
Copy Markdown

PR builds are available as GitHub Actions artifacts:

https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/26068392584

Artifacts expire in 7 days.
Artifacts:

  • pr-486-0d6e5b94513f8724e302d2f3876152e158a59a8d-tauri-linux
  • pr-486-0d6e5b94513f8724e302d2f3876152e158a59a8d-tauri-macos
  • pr-486-0d6e5b94513f8724e302d2f3876152e158a59a8d-electron-macos
  • pr-486-0d6e5b94513f8724e302d2f3876152e158a59a8d-tauri-windows
  • pr-486-0d6e5b94513f8724e302d2f3876152e158a59a8d-tauri-macos-arm64
  • pr-486-0d6e5b94513f8724e302d2f3876152e158a59a8d-electron-windows
  • pr-486-0d6e5b94513f8724e302d2f3876152e158a59a8d-electron-linux

@github-actions
Copy link
Copy Markdown

PR builds are available as GitHub Actions artifacts:

https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/26130730350

Artifacts expire in 7 days.
Artifacts:

  • pr-486-0d6e5b94513f8724e302d2f3876152e158a59a8d-tauri-linux
  • pr-486-0d6e5b94513f8724e302d2f3876152e158a59a8d-tauri-windows
  • pr-486-0d6e5b94513f8724e302d2f3876152e158a59a8d-tauri-macos
  • pr-486-0d6e5b94513f8724e302d2f3876152e158a59a8d-electron-macos
  • pr-486-0d6e5b94513f8724e302d2f3876152e158a59a8d-tauri-macos-arm64
  • pr-486-0d6e5b94513f8724e302d2f3876152e158a59a8d-electron-windows
  • pr-486-0d6e5b94513f8724e302d2f3876152e158a59a8d-electron-linux

@shantur
Copy link
Copy Markdown
Collaborator

shantur commented May 20, 2026

@CodeNomadBot

I want you to perform the gatekeeper checks for latest PR state

check for regressions
check for better possible implementations
Be the PR gatekeeper
Be ruthless about code quality

@codenomadbot
Copy link
Copy Markdown
Contributor

codenomadbot Bot commented May 20, 2026

Request understood: perform a ruthless gatekeeper review of the latest PR state for regressions and implementation quality. Assumption: this is review-only, so I did not modify code.

Gatekeeper Result: Request Changes

Findings:

  1. Blocking: new API contract is internally inconsistent and the UI imports a non-exported type.
    packages/server/src/api-types.ts:220 defines DetectPathExistingInRecentRequest with recentPaths, but the route schema expects recentFolders at packages/server/src/server/routes/filesystem.ts:26, and the UI sends recentFolders at packages/ui/src/lib/api-client.ts:489. Any client using the exported request contract would send the wrong field and get a false negative because the server defaults recentFolders to []. Separately, packages/ui/src/lib/api-client.ts:42 imports RecentFolder from ../../../server/src/api-types, but api-types.ts only imports RecentFolder from ./config/schema; it does not export it. Once dependencies are available, this should fail TypeScript with the module declaring RecentFolder locally but not exporting it. Fix by making the API contract match the implementation and either exporting RecentFolder intentionally from api-types.ts or typing the client method from the request interface instead of importing a private schema type.

  2. High: path detection failure now bypasses the existing launch error handling.
    packages/ui/src/App.tsx:273 awaits serverApi.detectPathExistingInRecent(...) before clearLaunchError() and before the try block that wraps createInstance. If fs.realpath(currentPath) throws in packages/server/src/server/routes/filesystem.ts:82 due to permission, missing path, stale selection, or platform edge cases, handleSelectFolder rejects without showing the existing launch error UI and without entering the normal cleanup/error path. The detection step should either be inside the existing try with a clear user-visible error or, preferably, fail open and continue with the originally selected path when canonicalization is unavailable.

  3. High: recent folders are the wrong source of truth for duplicate workspace detection.
    The new route only compares the selected path against recentFolders at packages/server/src/server/routes/filesystem.ts:92, then the UI rewrites folderPath to foundResult.path before calling getExistingInstanceForFolder at packages/ui/src/App.tsx:275-284. This can still miss an already-open workspace when recents are stale, absent, reordered, or contain multiple aliases/symlinks for the same real path. Example: instance is open at symlink A, recents contain symlink B first and symlink A later, and the user selects the real target; the API returns B, then getExistingInstanceForFolder(B) does not match the open instance at A, so a duplicate can still be created. Duplicate detection should compare against the active workspace/instance paths, not mutable recent-folder state.

  4. Medium: the implementation papers over symptoms in the client instead of normalizing at the authoritative boundary.
    Workspace creation in packages/server/src/workspaces/manager.ts:98-113 still stores workspacePath without realpath normalization. The new API adds a separate round trip and only affects this one UI path. Better implementation: normalize/canonicalize paths in the server workspace manager or expose canonical workspace paths in descriptors, then make duplicate detection operate on canonical active workspace paths. That would cover startup restore, future clients, and any route creating workspaces, rather than relying on the folder selection view to remember to call a special endpoint.

  5. Medium: no regression coverage for the actual Windows/symlink cases this PR targets.
    There are no tests covering drive-letter casing, slash normalization, symlink-to-target equivalence, stale recent folders, or failed realpath. Given this change is specifically for path identity edge cases, tests should be added around the canonicalization/detection behavior before merging.

Validation:

  • Inspected the PR diff against origin/dev.
  • Ran npm run typecheck --workspace @codenomad/ui and npm run typecheck --workspace @neuralnomads/codenomad-electron-app; both could not provide useful PR signal because this checkout is missing dependencies/types (solid-js, electron, @opencode-ai/sdk, Node types, etc.).
  • Noted an unrelated pre-existing local modification in .opencode/package-lock.json and ignored it.

Recommendation:

Do not merge as-is. Fix the contract/build issue first, then move the canonical path comparison to the server/workspace layer or compare against active instance paths directly, and add targeted symlink/Windows-case regression tests.

--
Yours,
CodeNomadBot

Change the API to accept and return string paths instead of RecentFolder
objects. This simplifies the interface by:
- Using recentPaths (string[]) instead of recentFolders (RecentFolder[])
- Returning foundResult as string instead of RecentFolder object
- Reducing coupling between API types and UI types
@github-actions
Copy link
Copy Markdown

PR builds are available as GitHub Actions artifacts:

https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/26201835544

Artifacts expire in 7 days.
Artifacts:

  • pr-486-c8a704d7299c51a47b4dd6081c26951c909485fa-tauri-macos
  • pr-486-c8a704d7299c51a47b4dd6081c26951c909485fa-tauri-linux
  • pr-486-c8a704d7299c51a47b4dd6081c26951c909485fa-tauri-windows
  • pr-486-c8a704d7299c51a47b4dd6081c26951c909485fa-electron-macos
  • pr-486-c8a704d7299c51a47b4dd6081c26951c909485fa-tauri-macos-arm64
  • pr-486-c8a704d7299c51a47b4dd6081c26951c909485fa-electron-windows
  • pr-486-c8a704d7299c51a47b4dd6081c26951c909485fa-electron-linux

@shantur
Copy link
Copy Markdown
Collaborator

shantur commented May 23, 2026

@CodeNomadBot

And now?

@codenomadbot
Copy link
Copy Markdown
Contributor

codenomadbot Bot commented May 23, 2026

Request understood: re-check the latest PR state after the follow-up commit and answer whether the prior gatekeeper concerns are resolved. Assumption: review-only, no code changes.

Gatekeeper Result: Still Request Changes

Status versus prior review:

  1. Still blocking: RecentFolder is imported from api-types.ts but is not exported.
    packages/server/src/api-types.ts:1-8 imports RecentFolder from ./config/schema, but does not export it. The latest commit still imports RecentFolder from ../../../server/src/api-types in packages/ui/src/lib/api-client.ts:45, and from ../../server/src/api-types in packages/ui/src/App.tsx:74. That is a TypeScript API-contract/build issue: consumers cannot import a locally imported, non-exported symbol from api-types.ts. Also, api-client.ts:45 no longer uses RecentFolder at all, so that import should simply be removed there. In App.tsx, avoid importing server schema internals through api-types.ts; the recentFolders().map(...) callback can be inferred or typed from the UI store type.

  2. Improved: detection failure no longer blocks launch.
    packages/ui/src/App.tsx:278-285 now catches detectPathExistingInRecent(...) and falls back to null. That resolves the previous concern where a failed realpath would bypass the launch error path entirely.

  3. Improved but still not ideal: active instance paths are now checked before recent folders.
    packages/ui/src/App.tsx:278-285 prepends running instance folders before recent folder paths, which fixes the most serious stale-recents duplicate-instance case from the previous review. This is better than the earlier version.

  4. Still medium risk: canonicalization remains a client-side special case rather than server/workspace-layer behavior.
    Workspace creation still stores the path without canonical normalization in packages/server/src/workspaces/manager.ts:98-113. This endpoint only protects the folder-selection path that remembers to call it. A more durable implementation would canonicalize or expose canonical paths from the workspace manager, then compare active workspace descriptors on that authoritative value.

  5. Still missing: no regression tests for the target behavior.
    I still do not see tests covering Windows drive-letter casing, slash normalization, symlink equivalence, stale/nonexistent paths, or failed realpath. Since the PR exists specifically to avoid path identity regressions, at least targeted unit coverage around the path comparison behavior is warranted before merge.

Additional code-quality notes:

  • packages/server/src/server/routes/filesystem.ts:5 imports RecentFolder and RecentFolderSchema, but the current route no longer uses either. Remove the dead import.
  • The new imports in packages/ui/src/App.tsx:73-75 use single quotes while the surrounding file uses double quotes. Not a blocker, but it is avoidable churn.

Validation:

  • Refreshed the PR comment thread since the previous bot review.
  • Inspected the latest PR head c8a704d and the full diff against origin/dev.
  • Did not rely on uncommitted local state; the worktree is clean.

Recommendation:

Do not merge yet. The previous highest-severity runtime logic issue is mostly addressed, but the non-exported RecentFolder imports are still a blocker. After removing/fixing those imports, I would still want either server-layer canonicalization or focused regression tests before approving.

--
Yours,
CodeNomadBot

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