[codex] Improve self-update PATH shadowing handling#135
Closed
BlackHole1 wants to merge 3 commits into
Closed
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
Summary by CodeRabbit
WalkthroughThis PR adds command-resolution to self-update: it scans PATH for candidate Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as Install/Update Command
participant Core as Self-Update Core
participant CmdRes as Command Resolution
participant PathScan as PATH Scanner
participant Output as Self-Update Output
User->>CLI: run install/update
activate CLI
CLI->>Core: performSelfUpdateOperation()
activate Core
Core->>CmdRes: resolveSelfUpdateCommandResolution(env, executablePath, platform)
activate CmdRes
CmdRes->>PathScan: resolveCommandPathCandidates(env, executableNames, platform)
activate PathScan
PathScan->>PathScan: read PATH → split entries → probe each dir for executable
PathScan-->>CmdRes: candidate list
deactivate PathScan
alt candidates found
CmdRes->>CmdRes: pick first candidate, compare with executablePath (direct + realpath)
alt match
CmdRes-->>Core: { status: "managed" }
else mismatch
CmdRes-->>Core: { status: "shadowed", path: candidatePath }
end
else no candidates
CmdRes-->>Core: { status: "missing" }
end
deactivate CmdRes
Core->>Output: writeSelfUpdatePathNoteIfNeeded(..., commandResolution)
activate Output
alt commandResolution.status == "shadowed"
Output->>Output: emit selfUpdate.pathShadowedNote { path, directory }
end
Output-->>CLI: note printed
deactivate Output
Core-->>CLI: operation result (includes commandResolution)
deactivate Core
CLI-->>User: finish (may include shadowing warning)
deactivate CLI
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Review rate limit: 3/5 reviews remaining, refill in 15 minutes and 33 seconds. Comment |
alwaysmavs
force-pushed
the
codex/self-update-path-shadowing
branch
from
bb6ce01 to
2d9c512
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
contrib/npm/oo.test.ts (1)
36-40: ⚡ Quick winUse path helpers instead of hard-coded POSIX separators in test input.
Line 38 hardcodes
/separators. Build this input vianode:pathhelpers to keep the test platform-agnostic.♻️ Suggested change
+import { join } from "node:path"; import { createRequire } from "node:module"; // ... test("detects npm from the installed oo path under npm-global", () => { expect(wrapperModule.detectPackageManagerFromOoPath([ - "/Users/demo/Library/Application Support/QClaw/npm-global/bin/oo", + join( + "Users", + "demo", + "Library", + "Application Support", + "QClaw", + "npm-global", + "bin", + "oo", + ), ])).toBe("npm"); });As per coding guidelines, "Never assume POSIX path separators in code, tests, snapshots, or assertions; use
node:pathhelpers (join(),resolve(),relative()) for path construction".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/npm/oo.test.ts` around lines 36 - 40, Replace the hard-coded POSIX path string in the test that calls wrapperModule.detectPackageManagerFromOoPath with a platform-agnostic path built using node:path helpers (e.g., import { join } from "path" and use join("Users","demo","Library","Application Support","QClaw","npm-global","bin","oo") or equivalent) so the input uses path.join rather than "/" separators; update the test expectation unchanged but construct the input array via the join call passed to detectPackageManagerFromOoPath.src/application/self-update/command-path.ts (1)
24-40: ⚡ Quick winParallelize independent PATH existence checks
The checks per directory are independent but executed sequentially. Switching to
Promise.allpreserves order while reducing I/O latency on long PATH values.Suggested patch
- const candidates: CommandPathCandidate[] = []; - - for (const directoryPath of splitPathEntries(pathValue, options.platform)) { - const candidatePath = await resolveFirstPathCandidatePath({ - directoryPath, - executableNames: options.executableNames, - pathExists: options.pathExists ?? defaultPathExists, - pathModule, - }); - - if (candidatePath === undefined) { - continue; - } - - candidates.push({ - directoryPath, - path: candidatePath, - }); - } - - return candidates; + const pathExists = options.pathExists ?? defaultPathExists; + const directories = splitPathEntries(pathValue, options.platform); + const resolvedCandidates = await Promise.all( + directories.map(async (directoryPath) => { + const candidatePath = await resolveFirstPathCandidatePath({ + directoryPath, + executableNames: options.executableNames, + pathExists, + pathModule, + }); + + if (candidatePath === undefined) { + return undefined; + } + + return { + directoryPath, + path: candidatePath, + } satisfies CommandPathCandidate; + }), + ); + + return resolvedCandidates.filter( + (candidate): candidate is CommandPathCandidate => candidate !== undefined, + ); }As per coding guidelines, "Use
Promise.all()for independent async operations instead of sequentialawait."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/self-update/command-path.ts` around lines 24 - 40, The loop over splitPathEntries is performing independent async checks sequentially; change it to run resolveFirstPathCandidatePath for each directory in parallel via Promise.all while preserving order: map splitPathEntries(...) to an array of promises calling resolveFirstPathCandidatePath({ directoryPath, executableNames: options.executableNames, pathExists: options.pathExists ?? defaultPathExists, pathModule }), await Promise.all on that array, then iterate the results in order and for each non-undefined result push { directoryPath, path: candidatePath } into candidates. Ensure you still use the same symbols: splitPathEntries, resolveFirstPathCandidatePath, options.pathExists/defaultPathExists and candidates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/application/self-update/command-path.ts`:
- Around line 58-61: The current split/filter pipeline drops empty PATH segments
(pathValue.split(...).map(...).filter(Boolean)), which loses POSIX-empty entries
that represent the current directory; instead, preserve empty entries by
removing filter(Boolean) and normalize them to '.' on non-Windows platforms.
Update the code around pathValue and readPathModule(platform) so that after
splitting and trimming you map empty strings to '.' when not on Windows (use
platform detection consistent with readPathModule(platform) or an isWindows
flag), and do not filter out falsy entries; this ensures empty segments remain
as valid path entries for the function that computes shadowing/managed/missing.
---
Nitpick comments:
In `@contrib/npm/oo.test.ts`:
- Around line 36-40: Replace the hard-coded POSIX path string in the test that
calls wrapperModule.detectPackageManagerFromOoPath with a platform-agnostic path
built using node:path helpers (e.g., import { join } from "path" and use
join("Users","demo","Library","Application
Support","QClaw","npm-global","bin","oo") or equivalent) so the input uses
path.join rather than "/" separators; update the test expectation unchanged but
construct the input array via the join call passed to
detectPackageManagerFromOoPath.
In `@src/application/self-update/command-path.ts`:
- Around line 24-40: The loop over splitPathEntries is performing independent
async checks sequentially; change it to run resolveFirstPathCandidatePath for
each directory in parallel via Promise.all while preserving order: map
splitPathEntries(...) to an array of promises calling
resolveFirstPathCandidatePath({ directoryPath, executableNames:
options.executableNames, pathExists: options.pathExists ?? defaultPathExists,
pathModule }), await Promise.all on that array, then iterate the results in
order and for each non-undefined result push { directoryPath, path:
candidatePath } into candidates. Ensure you still use the same symbols:
splitPathEntries, resolveFirstPathCandidatePath,
options.pathExists/defaultPathExists and candidates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10f45e02-5fc2-41a3-9502-f6ce1581754d
📒 Files selected for processing (20)
contrib/npm/oo.cjscontrib/npm/oo.test.tsdocs/commands.mddocs/commands.zh-CN.mdsrc/application/commands/install.tssrc/application/commands/self-update-output.test.tssrc/application/commands/self-update-output.tssrc/application/commands/self-update.cli.test.tssrc/application/commands/update.tssrc/application/contracts/self-update.tssrc/application/self-update/command-path.tssrc/application/self-update/command-resolution.test.tssrc/application/self-update/command-resolution.tssrc/application/self-update/core.tssrc/application/self-update/installation.test.tssrc/application/self-update/installation.tssrc/application/self-update/legacy-installation.test.tssrc/application/self-update/legacy-installation.tssrc/application/self-update/path-comparison.tssrc/i18n/catalog.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/application/self-update/command-path.test.ts (2)
6-13: ⚡ Quick winExtract repeated stub/setup patterns into local test helpers.
The
resolveCommandPathCandidatescall shape andpathExistsstub logic are repeated across all three tests. A local factory/helper will reduce duplication and make new PATH cases easier to add.Proposed refactor
import { describe, expect, test } from "bun:test"; import { resolveCommandPathCandidates } from "./command-path.ts"; +function createPathExists(availablePaths: readonly string[]) { + return async (candidatePath: string) => + availablePaths.includes(candidatePath); +} + describe("resolveCommandPathCandidates", () => { @@ env: { PATH: "", }, executableNames: ["oo"], - pathExists: async path => path === "oo", + pathExists: createPathExists(["oo"]), platform: "linux", }); @@ env: { PATH: ":/managed/bin", }, executableNames: ["oo"], - pathExists: async path => path === "oo" || path === "/managed/bin/oo", + pathExists: createPathExists(["oo", "/managed/bin/oo"]), platform: "linux", }); @@ env: { Path: ";C:\\managed\\bin", }, executableNames: ["oo.exe"], - pathExists: async path => ( - path === "oo.exe" || path === "C:\\managed\\bin\\oo.exe" - ), + pathExists: createPathExists(["oo.exe", "C:\\managed\\bin\\oo.exe"]), platform: "win32", });As per coding guidelines, "Extract repeated setup (mocks, stubs, setup objects) into local factory functions in test files; avoid copy-pasting test setup".
Also applies to: 24-31, 46-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/self-update/command-path.test.ts` around lines 6 - 13, The tests repeatedly call resolveCommandPathCandidates with the same pattern of env/PATH, executableNames and a pathExists stub; extract a local test helper (e.g., makeResolveArgs or createResolveStub) that builds the argument object and provides a configurable pathExists implementation so each test can pass PATH and executableNames concisely; update each test to call resolveCommandPathCandidates(helper({ PATH: "", executableNames: ["oo"], pathExistsMatches: p => p === "oo", platform: "linux" })) and adjust other cases to reuse the helper rather than duplicating the stub logic, keeping references to resolveCommandPathCandidates and pathExists for clarity.
29-30: ⚡ Quick winBuild expected paths with
node:pathhelpers instead of hardcoded separators.Lines 29, 40, 52, and 64 hardcode platform separators in assertions/stubs. Prefer
path.posix.join/path.win32.joinfor portable, guideline-compliant tests.Proposed refactor
+import { posix, win32 } from "node:path"; import { describe, expect, test } from "bun:test"; import { resolveCommandPathCandidates } from "./command-path.ts"; describe("resolveCommandPathCandidates", () => { + const posixManagedDirectory = posix.join("/", "managed", "bin"); + const posixManagedExecutable = posix.join(posixManagedDirectory, "oo"); + const windowsManagedDirectory = win32.join("C:\\", "managed", "bin"); + const windowsManagedExecutable = win32.join( + windowsManagedDirectory, + "oo.exe", + ); + test("treats an empty POSIX PATH value as the current-directory entry", async () => { const candidates = await resolveCommandPathCandidates({ @@ test("preserves POSIX empty PATH segments as current-directory entries", async () => { const candidates = await resolveCommandPathCandidates({ env: { - PATH: ":/managed/bin", + PATH: `:${posixManagedDirectory}`, }, executableNames: ["oo"], - pathExists: async path => path === "oo" || path === "/managed/bin/oo", + pathExists: async path => + path === "oo" || path === posixManagedExecutable, platform: "linux", }); @@ { - directoryPath: "/managed/bin", - path: "/managed/bin/oo", + directoryPath: posixManagedDirectory, + path: posixManagedExecutable, }, ]); }); @@ test("preserves Windows empty PATH segments without rewriting them", async () => { const candidates = await resolveCommandPathCandidates({ env: { - Path: ";C:\\managed\\bin", + Path: `;${windowsManagedDirectory}`, }, executableNames: ["oo.exe"], pathExists: async path => ( - path === "oo.exe" || path === "C:\\managed\\bin\\oo.exe" + path === "oo.exe" || path === windowsManagedExecutable ), platform: "win32", }); @@ { - directoryPath: "C:\\managed\\bin", - path: "C:\\managed\\bin\\oo.exe", + directoryPath: windowsManagedDirectory, + path: windowsManagedExecutable, }, ]); }); });As per coding guidelines, "Never assume POSIX path separators in code, tests, snapshots, or assertions; use
node:pathhelpers (join(),resolve(),relative()) for path construction".Also applies to: 40-40, 52-53, 64-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/self-update/command-path.test.ts` around lines 29 - 30, Replace hardcoded path strings in the test stubs/assertions with node:path join helpers: in the pathExists stub (the async path => path === "oo" || path === "/managed/bin/oo") and the other occurrences, build expected paths with path.posix.join(...) when platform is "linux"/POSIX and path.win32.join(...) when platform is "win32"; update the platform values in those test cases accordingly and use the same join calls in any assertions so tests no longer assume literal '/' separators.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/application/self-update/command-path.test.ts`:
- Around line 6-13: The tests repeatedly call resolveCommandPathCandidates with
the same pattern of env/PATH, executableNames and a pathExists stub; extract a
local test helper (e.g., makeResolveArgs or createResolveStub) that builds the
argument object and provides a configurable pathExists implementation so each
test can pass PATH and executableNames concisely; update each test to call
resolveCommandPathCandidates(helper({ PATH: "", executableNames: ["oo"],
pathExistsMatches: p => p === "oo", platform: "linux" })) and adjust other cases
to reuse the helper rather than duplicating the stub logic, keeping references
to resolveCommandPathCandidates and pathExists for clarity.
- Around line 29-30: Replace hardcoded path strings in the test stubs/assertions
with node:path join helpers: in the pathExists stub (the async path => path ===
"oo" || path === "/managed/bin/oo") and the other occurrences, build expected
paths with path.posix.join(...) when platform is "linux"/POSIX and
path.win32.join(...) when platform is "win32"; update the platform values in
those test cases accordingly and use the same join calls in any assertions so
tests no longer assume literal '/' separators.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 556cf8ad-75e7-4108-b86d-90bdf6dcc0d5
📒 Files selected for processing (2)
src/application/self-update/command-path.test.tssrc/application/self-update/command-path.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/application/self-update/command-path.ts
BlackHole1
added a commit
that referenced
this pull request
May 5, 2026
…137) After install or update, the CLI now checks whether PATH still resolves oo to another executable before the managed directory and prints a shadowing note so users notice their shell will keep running the old binary. Set OO_HIDE_PATH_SHADOWING_WARNING to a truthy value to suppress the note when keeping another oo earlier on PATH is intentional. Also harden the legacy npm cleanup: detect installs under npm-global and npm_global path segments, and pass the inferred global prefix to npm uninstall so cleanup works for non-default prefixes. close: #135 --------- Signed-off-by: Kevin Cui <bh@bugs.cc>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
oo installoroo updateleaves anotherooexecutable earlier onPATHand print an explicit shadowing note.npm-globalpaths and passing the detected--prefixwhen uninstalling old@oomol-lab/oo-cliinstalls.Root Cause
oo updatecorrectly installed and activated the managed executable under~/.local/bin, but it did not verify that the current shell would actually resolveooto that managed entrypoint. A prior npm-global installation, such as a customQClaw/npm-global/bin/oo, could remain earlier onPATH, makingoo --versioncontinue to report the old version after a successful update.Validation
bun run lint:fixbun run ts-checkbun run test src/application/self-update/command-resolution.test.ts src/application/self-update/legacy-installation.test.ts src/application/self-update/installation.test.ts src/application/commands/self-update-output.test.ts src/application/commands/self-update.cli.test.ts contrib/npm/oo.test.tsbun run testwith Bun1.3.13:799 pass,9 skip,0 fail