Skip to content

Canonicalize workspace roots for session visibility#158

Open
YuMS wants to merge 1 commit into
friuns2:mainfrom
YuMS:feature/canonicalize-workspace-roots
Open

Canonicalize workspace roots for session visibility#158
YuMS wants to merge 1 commit into
friuns2:mainfrom
YuMS:feature/canonicalize-workspace-roots

Conversation

@YuMS
Copy link
Copy Markdown

@YuMS YuMS commented May 11, 2026

Summary

canonicalize saved workspace roots with local realpath
canonicalize thread/list cwd values before sidebar filtering
keep sessions visible when symlink and target paths are mixed
add regression tests and manual test coverage

Verification

npx vitest run src/server/codexAppServerBridge.archive.test.ts
npx tsc -p tsconfig.server.json --noEmit
npm run build:frontend
git diff --check
PROFILE_BASE_URL=http://127.0.0.1:4173 PROFILE_WAIT_MS=7000 node scripts/profile-browser-runtime.cjs

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

Review Summary by Qodo

Canonicalize workspace roots for consistent session visibility

✨ Enhancement 🧪 Tests

Grey Divider

Walkthroughs

Description
• Canonicalize workspace roots and thread cwd values through realpath
• Ensures sessions remain visible with symlinked workspace paths
• Exports canonicalization functions for workspace roots state
• Adds comprehensive test coverage and manual regression tests
Diagram
flowchart LR
  A["Thread/List Response"] -->|canonicalizeThreadListResponseForRead| B["Canonicalized CWD Values"]
  C["Workspace Roots State"] -->|canonicalizeWorkspaceRootsStateForRead| D["Canonicalized Paths"]
  B --> E["Sessions Visible Regardless of Symlink"]
  D --> E
Loading

Grey Divider

File Changes

1. src/server/codexAppServerBridge.ts ✨ Enhancement +76/-5

Add path canonicalization for workspace roots and threads

• Added realpath import from node:fs/promises for path canonicalization
• Exported WorkspaceRootsState type for external use
• Modified callRpcWithArchiveRecovery to canonicalize thread/list responses
• Implemented canonicalizeWorkspaceRootsStateForRead function to realpath workspace roots, labels,
 and project orders
• Implemented canonicalizeThreadCwdRecord and canonicalizeThreadListResponseForRead to
 canonicalize thread cwd values
• Updated readWorkspaceRootsState to apply canonicalization when reading persisted state

src/server/codexAppServerBridge.ts


2. src/server/codexAppServerBridge.archive.test.ts 🧪 Tests +58/-1

Add canonicalization function tests

• Added imports for canonicalizeThreadListResponseForRead and
 canonicalizeWorkspaceRootsStateForRead
• Added test suite for canonicalizeWorkspaceRootsStateForRead verifying symlink paths are
 realpathd
• Added test suite for canonicalizeThreadListResponseForRead verifying thread cwd values are
 canonicalized
• Tests verify that mixed symlink and canonical paths are normalized consistently

src/server/codexAppServerBridge.archive.test.ts


3. tests.md 📝 Documentation +30/-0

Add manual regression test for symlink workspace roots

• Added manual regression test section "Sidebar sessions survive symlinked workspace roots"
• Documents prerequisites, test steps, and expected results for symlink path handling
• Verifies sessions are visible regardless of whether recorded through symlink or canonical path
• Includes verification of API response returning canonical real paths
• Tests both light and dark theme rendering

tests.md


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

qodo-free-for-open-source-projects Bot commented May 11, 2026

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0)

Grey Divider


Action required

1. Label overwrite race 🐞 Bug ≡ Correctness
Description
canonicalizeWorkspaceRootsStateForRead concurrently assigns canonicalized label keys into a shared
object; if multiple original keys (e.g., symlink + target) realpath to the same canonical key, the
winning label becomes dependent on async completion order and can flip between reads. This is
user-visible because the sidebar hydrates display names from rootsState.labels.
Code

src/server/codexAppServerBridge.ts[R3905-3909]

+  const labels: Record<string, string> = { ...state.labels }
+  await Promise.all(Object.entries(state.labels).map(async ([key, label]) => {
+    const canonicalKey = await canonicalizeWorkspaceRootPath(key, pathRealpath)
+    labels[canonicalKey] = label
+  }))
Evidence
The server currently copies labels then concurrently assigns canonical keys, which can collide when
both canonical and symlink forms exist; the state writer can introduce such mixed keys, and the UI
consumes labels to set visible project display names.

src/server/codexAppServerBridge.ts[3896-3909]
src/server/codexAppServerBridge.ts[4006-4022]
src/composables/useDesktopState.ts[3853-3865]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`canonicalizeWorkspaceRootsStateForRead` builds `labels` by copying `state.labels` and then `Promise.all`-writing `labels[canonicalKey] = label` concurrently. If two different keys canonicalize to the same `canonicalKey` (common when both symlink and target were saved), the final label is nondeterministic (depends on which `realpath` resolves last).

## Issue Context
This function is used on every `/codex-api/workspace-roots-state` read, and the UI iterates `rootsState.labels` to hydrate project display names.

## Fix Focus Areas
- src/server/codexAppServerBridge.ts[3896-3917]

## Suggested fix approach
- Build a **new** `labels` map deterministically instead of mutating a shared object from concurrent tasks.
- Make collision policy explicit, e.g.:
 - Prefer an entry whose original key is already canonical (`key === canonicalKey`) over a symlink-derived key.
 - Or process entries sequentially in stable key order.
- Consider dropping non-canonical keys from the returned `labels` to avoid returning both forms.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. No realpath memoization 🐞 Bug ➹ Performance
Description
canonicalizeThreadListResponseForRead calls realpath once per thread item (per absolute cwd) without
memoizing identical cwd strings, so a thread/list page with many sessions from the same project
triggers redundant filesystem syscalls. This runs on every thread/list RPC and thread/list is
paginated (50/100 items per page) and repeatedly requested during background loading.
Code

src/server/codexAppServerBridge.ts[R3935-3940]

+  const record = asRecord(payload)
+  if (!record || !Array.isArray(record.data)) return payload
+  return {
+    ...record,
+    data: await Promise.all(record.data.map((item) => canonicalizeThreadCwdRecord(item, pathRealpath))),
+  }
Evidence
The server now canonicalizes thread/list results and does a per-item async mapping; thread/list is
called with up to 100 items per page and paginated in the UI, so redundant per-item realpath calls
can accumulate across pages.

src/server/codexAppServerBridge.ts[1225-1234]
src/server/codexAppServerBridge.ts[3920-3940]
src/api/codexGateway.ts[688-704]
src/composables/useDesktopState.ts[4080-4091]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`canonicalizeThreadListResponseForRead` maps over every `record.data` entry and calls `canonicalizeWorkspaceRootPath` (which calls `realpath`) per item. If multiple items share the same `cwd` string, we perform redundant `realpath` calls.

## Issue Context
The server applies this transformation for every `thread/list` RPC. The client calls `thread/list` with limits up to 100 and performs background pagination.

## Fix Focus Areas
- src/server/codexAppServerBridge.ts[3920-3941]
- src/server/codexAppServerBridge.ts[1225-1234]

## Suggested fix approach
- Add a per-invocation cache in `canonicalizeThreadListResponseForRead`, e.g. `Map<string, Promise<string>>`, keyed by the original `cwd` string.
- Resolve each unique `cwd` at most once, then reuse the cached promise for subsequent items.
- Keep the existing behavior of skipping non-absolute values and swallowing `realpath` failures.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment on lines +3905 to +3909
const labels: Record<string, string> = { ...state.labels }
await Promise.all(Object.entries(state.labels).map(async ([key, label]) => {
const canonicalKey = await canonicalizeWorkspaceRootPath(key, pathRealpath)
labels[canonicalKey] = label
}))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

1. Label overwrite race 🐞 Bug ≡ Correctness

canonicalizeWorkspaceRootsStateForRead concurrently assigns canonicalized label keys into a shared
object; if multiple original keys (e.g., symlink + target) realpath to the same canonical key, the
winning label becomes dependent on async completion order and can flip between reads. This is
user-visible because the sidebar hydrates display names from rootsState.labels.
Agent Prompt
## Issue description
`canonicalizeWorkspaceRootsStateForRead` builds `labels` by copying `state.labels` and then `Promise.all`-writing `labels[canonicalKey] = label` concurrently. If two different keys canonicalize to the same `canonicalKey` (common when both symlink and target were saved), the final label is nondeterministic (depends on which `realpath` resolves last).

## Issue Context
This function is used on every `/codex-api/workspace-roots-state` read, and the UI iterates `rootsState.labels` to hydrate project display names.

## Fix Focus Areas
- src/server/codexAppServerBridge.ts[3896-3917]

## Suggested fix approach
- Build a **new** `labels` map deterministically instead of mutating a shared object from concurrent tasks.
- Make collision policy explicit, e.g.:
  - Prefer an entry whose original key is already canonical (`key === canonicalKey`) over a symlink-derived key.
  - Or process entries sequentially in stable key order.
- Consider dropping non-canonical keys from the returned `labels` to avoid returning both forms.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

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.

1 participant