Repository navigation
fix(workspace): canonicalize tmp paths, dynamic config path in serve warning - #1057
Conversation
…path in serve warning Follow-up to #1056 addressing two review comments from gemini-code-assist. 1. isTempPath() now uses realpathSync to canonicalize both tmpdir() and the candidate root before prefix-comparing. On macOS /tmp is a symlink to /private/tmp and tmpdir() returns /var/folders/..., so the previous path.resolve()-only check would miss any literal /tmp/... path and bypass the guard against poisoning the global workspaceRoot config. Fails CLOSED on canonicalization errors (treats as temp → refuses to persist) since the whole purpose of this guard is to be conservative. 2. startAgentSync() now builds its "no workspace found" warning using genieHome() (newly exported) instead of hardcoding ~/.genie/config.json, so users who set GENIE_HOME see the actual path in the fix guidance. Also adds: - A regression test using a real symlink within the test's tmpdir to verify canonicalization works end-to-end (skips gracefully on CI environments that disallow symlinks). - A sanity test that genieHome() honors the GENIE_HOME env override.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request improves workspace detection by using realpathSync to canonicalize paths in isTempPath, which prevents bypassing the temporary path guard on systems like macOS where /tmp is a symlink. It also exports genieHome for consistent configuration path resolution and adds regression tests. Feedback focuses on removing redundant dynamic imports and resolving variable shadowing in src/term-commands/serve.ts.
| async function startAgentSync(): Promise<{ close: () => void } | null> { | ||
| try { | ||
| const { findWorkspace } = require('../lib/workspace.js') as typeof import('../lib/workspace.js'); | ||
| const { findWorkspace, genieHome } = require('../lib/workspace.js') as typeof import('../lib/workspace.js'); |
There was a problem hiding this comment.
Importing genieHome here shadows the local genieHome function defined at line 31. Since both functions have the same implementation, this import is redundant. To improve maintainability, you should consider removing the local definition at line 31 and using the exported one from workspace.ts throughout the file, but for now, removing it from this import will resolve the shadowing. This dynamic import pattern is consistent with the repository's approach to breaking circular dependencies.
| const { findWorkspace, genieHome } = require('../lib/workspace.js') as typeof import('../lib/workspace.js'); | |
| const { findWorkspace } = require('../lib/workspace.js') as typeof import('../lib/workspace.js'); |
References
- Use dynamic imports (require() or import()) to break circular dependency cycles that would otherwise occur at module load time.
| const { join } = require('node:path') as typeof import('node:path'); | ||
| const configPath = join(genieHome(), 'config.json'); |
There was a problem hiding this comment.
The join function is already imported from node:path at the top of the file (line 23). This dynamic require is redundant and can be removed since the module is already loaded at the top level.
| const { join } = require('node:path') as typeof import('node:path'); | |
| const configPath = join(genieHome(), 'config.json'); | |
| const configPath = join(genieHome(), 'config.json'); |
Follow-up to #1056 addressing two review comments from
gemini-code-assistbot.#1056 was already merged before the review comments landed, so these fixes ship as a separate small PR.
Comment 1 —
isTempPathdidn't handle symlinked temp dirs (HIGH)Accepted. Real cross-platform bug.
os.tmpdir()on macOS returns/var/folders/.../T/, while user-facing paths often come through/tmp(symlink).path.resolve()only normalizes..and makes paths absolute — it does not canonicalize symlinks. A literal/tmp/foowould not prefix-match/var/folders/.../T/, so the guard against writing test workspaces into the global~/.genie/config.jsoncould be silently bypassed on macOS.Fix in
src/lib/workspace.ts:realpathSyncto the existingnode:fstop-level import (cleaner than the bot's suggested dynamicrequire).isTempPathnow canonicalizes bothtmpdir()androotviarealpathSyncbefore comparing.Comment 2 — warning message hardcoded
~/.genie/config.json(LOW)Accepted. Cosmetic accuracy. Users who set
GENIE_HOME(CI, test harnesses, multi-workspace setups) would see a misleading fix suggestion.Fix in
src/term-commands/serve.ts:genieHome()fromworkspace.ts(was private).startAgentSync()now resolves the config path dynamically:join(genieHome(), 'config.json').Tests added
workspace.test.ts: creates a real symlink within the test'stmpdir()pointing at a real workspace dir, callsfindWorkspace(symlinkPath), and asserts nothing was persisted to the isolated fakeGENIE_HOMEconfig. Skips gracefully (returns early) on CI environments that disallow symlink creation — we'd rather no coverage than a flaky test.genieHome()env override test: sanity check that the newly-exported helper honorsprocess.env.GENIE_HOMEdynamically, so callers in other modules (e.g. the serve warning) see the same value asworkspace.tsdoes internally.Verification
bun test src/lib/workspace.test.ts→ 14 pass, 0 fail (was 12)bun test src/lib/→ 1573 pass, 0 fail (was 1571, +2 from this PR)bunx tsc --noEmit→ cleanbunx biome checkon changed files → 0 errors, 1 pre-existing warning onstartForegroundcomplexity (unchanged, not introduced here)Test plan
/tmp/test-workspace/.genie/workspace.json, runfindWorkspace('/tmp/test-workspace'), confirm~/.genie/config.jsondoes NOT receive the path (previously would have been persisted on macOS)genie servewithGENIE_HOME=/custom/pathfrom outside a workspace → warning should show/custom/path/config.jsonnot~/.genie/config.json