feat(self-update): auto-configure PATH for install and update#103
Merged
Conversation
Self-managed installs could succeed while leaving future shells unable to find `oo` until users edited PATH manually. Persist the managed bin directory across supported shells, print a restart note when that succeeds, and keep an explicit opt-out with `--no-modify-path` and `OO_NO_MODIFY_PATH`. Signed-off-by: Kevin Cui <bh@bugs.cc>
Summary by CodeRabbit
WalkthroughAdds an opt-out for automatic PATH modification via Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "oo install / oo update"
participant Pref as "resolveSelfUpdateModifyPath"
participant SelfUpdate as "performSelfUpdateOperation / ensureSelfUpdateExecutableDirectoryOnPath"
participant PathWriter as "ensureExecutableDirectoryOnPath"
participant Output as "writeSelfUpdatePathNoteIfNeeded"
User->>CLI: run command (with optional --no-modify-path or OO_NO_MODIFY_PATH)
CLI->>Pref: resolve modifyPath from flag + env
Pref-->>CLI: effectiveModifyPath
CLI->>SelfUpdate: performSelfUpdateOperation(modifyPath: effectiveModifyPath)
SelfUpdate->>SelfUpdate: determine executableDirectory
alt modifyPath == false
SelfUpdate-->>CLI: pathConfiguration = skipped | already-configured
else
SelfUpdate->>PathWriter: ensureExecutableDirectoryOnPath(executableDirectory, modifyPath)
PathWriter-->>SelfUpdate: pathConfiguration (already-configured | configured | partial-configured | failed)
end
CLI->>Output: writeSelfUpdatePathNoteIfNeeded(executableDirectory, pathConfiguration)
Output-->>User: prints appropriate message (none / added+restart / partial lists+restart / manual setup)
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/application/commands/self-update.cli.test.ts (1)
142-147: Make the PATH assertions platform-neutral.These expectations hardcode
<HOME>/.local/bin, so they become brittle on Windows and whenever the managed bin path changes. Build the expected path withnode:pathhelpers (or a shared snapshot helper) and interpolate it into the expected stdout instead of embedding POSIX separators.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: 181-186, 222-226, 274-278, 421-425, 470-475, 511-516, 554-558, 610-614, 666-670
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/self-update.cli.test.ts` around lines 142 - 147, The test hardcodes POSIX separators in the expected stdout (e.g. "<HOME>/.local/bin"), which breaks on Windows; update the assertions in createSelfUpdateInstallSnapshot expectations (and the other similar snapshots referenced) to build the managed bin path using node:path helpers and interpolate that constructed path into the expected stdout string instead of embedding "<HOME>/.local/bin". Concretely, import or require node:path (e.g. path.join(home, ".local", "bin") or use the shared snapshot helper if available), compute expectedManagedBinPath once, then assert stdout contains `Add ${expectedManagedBinPath} to PATH...` while leaving the rest of the snapshot unchanged; keep the call to selfUpdateRuntime.configurePathCallCount() as-is.
🤖 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/path-configuration.ts`:
- Around line 128-173: The loop may produce mixed outcomes (some profiles
written, some failed) but current logic returns "configured" whenever
configuredTargets.length > 0; change this so partial successes are not reported
as fully configured: after the loop, compute failedTargets =
configurations.map(c=>c.profilePath) minus configuredTargets and
alreadyConfiguredTargets, and if failedTargets.length > 0 return status "failed"
(or a distinct "partial-failure" if you prefer) with failedTargets; only return
"configured" when configuredTargets.length > 0 and failedTargets.length === 0,
and keep the existing "already-configured" branch for when nothing was changed
but no failures occurred; update references to configurations,
writeShellProfileConfiguration, configuredTargets, alreadyConfiguredTargets, and
options.runtime.logger.* accordingly.
- Around line 493-504: Replace the fragile reference-equality guard that
compares options.env to process.env with an explicit boolean flag on the options
object (e.g., canModifyRealProcess or isProduction) and use that flag in
ensureExecutableDirectoryOnPath/ensureSelfUpdateExecutableDirectoryOnPath to
decide whether to modify the real process PATH; update the option type/interface
and all call sites that pass options (including runCli,
createCliExecutionContext, update.ts) to set the new flag appropriately so the
check becomes if (!options.canModifyRealProcess) { log debug and return {
status: "failed" } } instead of comparing env objects.
---
Nitpick comments:
In `@src/application/commands/self-update.cli.test.ts`:
- Around line 142-147: The test hardcodes POSIX separators in the expected
stdout (e.g. "<HOME>/.local/bin"), which breaks on Windows; update the
assertions in createSelfUpdateInstallSnapshot expectations (and the other
similar snapshots referenced) to build the managed bin path using node:path
helpers and interpolate that constructed path into the expected stdout string
instead of embedding "<HOME>/.local/bin". Concretely, import or require
node:path (e.g. path.join(home, ".local", "bin") or use the shared snapshot
helper if available), compute expectedManagedBinPath once, then assert stdout
contains `Add ${expectedManagedBinPath} to PATH...` while leaving the rest of
the snapshot unchanged; keep the call to
selfUpdateRuntime.configurePathCallCount() as-is.
🪄 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: 44a22f7c-5190-44d5-85aa-983ee6117013
⛔ Files ignored due to path filters (4)
src/application/bootstrap/__snapshots__/run-cli.test.ts.snapis excluded by!**/*.snapsrc/application/commands/__snapshots__/self-update.cli.test.ts.snapis excluded by!**/*.snapsrc/application/commands/config/__snapshots__/index.cli.test.ts.snapis excluded by!**/*.snapsrc/application/commands/skills/__snapshots__/index.cli.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (16)
docs/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/core.tssrc/application/self-update/modify-path-preference.test.tssrc/application/self-update/modify-path-preference.tssrc/application/self-update/path-configuration.test.tssrc/application/self-update/path-configuration.tssrc/application/shared/env-boolean.test.tssrc/application/shared/env-boolean.tssrc/i18n/catalog.ts
PATH setup can touch multiple shell profiles, so a single failed write should not be reported as a full success. Report partial setup results with separate updated and failed profile lists so users know what still needs manual attention. Gate Windows user PATH registry writes behind an explicit runtime flag that is enabled only by the real CLI entry point. This keeps executeCli and test sandboxes safe by default while still allowing production installs and updates to persist PATH changes. Signed-off-by: Kevin Cui <bh@bugs.cc>
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/application/self-update/modify-path-preference.test.ts (1)
8-45: Extract repeated resolver setup into a local test helper.The test intent is clear, but the repeated
{ env, modifyPathFlag }setup is duplicated across cases and can be centralized.♻️ Suggested refactor
describe("resolveSelfUpdateModifyPath", () => { + function resolve(modifyPathFlag: boolean, envValue?: string): boolean { + return resolveSelfUpdateModifyPath({ + env: envValue === undefined + ? {} + : { [selfUpdateNoModifyPathEnvName]: envValue }, + modifyPathFlag, + }); + } + test("modifies PATH by default when neither flag nor env disables it", () => { - expect(resolveSelfUpdateModifyPath({ - env: {}, - modifyPathFlag: true, - })).toBeTrue(); + expect(resolve(true)).toBeTrue(); }); @@ test("skips PATH modification when --no-modify-path is passed", () => { - expect(resolveSelfUpdateModifyPath({ - env: {}, - modifyPathFlag: false, - })).toBeFalse(); + expect(resolve(false)).toBeFalse(); }); @@ expect(resolveSelfUpdateModifyPath({ - env: { [selfUpdateNoModifyPathEnvName]: value }, - modifyPathFlag: true, - })).toBeFalse(); + expect(resolve(true, value)).toBeFalse(); } });As per coding guidelines: "Extract repeated setup (mocks, stubs, setup objects) into local factory functions in test files; avoid copy-pasting test setup".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/self-update/modify-path-preference.test.ts` around lines 8 - 45, The tests repeatedly construct the same input object for resolveSelfUpdateModifyPath with env and modifyPathFlag; extract a small local factory/helper (e.g., makeInput or buildResolveInput) inside the test file to produce { env, modifyPathFlag } and replace each inline object with calls to that helper, referencing resolveSelfUpdateModifyPath and selfUpdateNoModifyPathEnvName to ensure env keys are set consistently and the tests remain identical in behavior.
🤖 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/path-configuration.ts`:
- Around line 291-295: The selection pipeline (selectedConfigurations) can
include the same ".profile" fallback twice when both the candidate filter and
the subsequent fallback-append logic add a ".profile" entry; update the logic
that builds selectedConfigurations and the fallback append routine (referencing
selectedConfigurations, candidates, shouldConfigureShellProfile, and shellName)
to deduplicate by the profile target (e.g., use a Set of target paths or dedupe
by a unique key) so that a ".profile" configuration is only added once; ensure
deduplication runs after combining initial selections and any fallback entries
so the final array contains unique profile targets only.
---
Nitpick comments:
In `@src/application/self-update/modify-path-preference.test.ts`:
- Around line 8-45: The tests repeatedly construct the same input object for
resolveSelfUpdateModifyPath with env and modifyPathFlag; extract a small local
factory/helper (e.g., makeInput or buildResolveInput) inside the test file to
produce { env, modifyPathFlag } and replace each inline object with calls to
that helper, referencing resolveSelfUpdateModifyPath and
selfUpdateNoModifyPathEnvName to ensure env keys are set consistently and the
tests remain identical in behavior.
🪄 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: e8e61954-f559-4f4e-b208-791d3c9e6b61
⛔ Files ignored due to path filters (4)
src/application/bootstrap/__snapshots__/run-cli.test.ts.snapis excluded by!**/*.snapsrc/application/commands/__snapshots__/self-update.cli.test.ts.snapis excluded by!**/*.snapsrc/application/commands/config/__snapshots__/index.cli.test.ts.snapis excluded by!**/*.snapsrc/application/commands/skills/__snapshots__/index.cli.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (17)
docs/commands.mddocs/commands.zh-CN.mdsrc/application/bootstrap/run-cli.tssrc/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/core.tssrc/application/self-update/modify-path-preference.test.tssrc/application/self-update/modify-path-preference.tssrc/application/self-update/path-configuration.test.tssrc/application/self-update/path-configuration.tssrc/application/shared/env-boolean.test.tssrc/application/shared/env-boolean.tssrc/i18n/catalog.ts
Unknown shells can trigger an extra .profile fallback after bash selection has already included the same profile path. Avoiding that duplicate keeps reported targets accurate and prevents redundant profile writes during PATH setup. Signed-off-by: Kevin Cui <bh@bugs.cc>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/path-configuration.ts`:
- Around line 485-492: The PowerShell snippet built in
createPowerShellProfilePathSnippet uses $env:Path which is lowercase-sensitive
and can be $null on PowerShell 7; update the strings that reference the
environment variable (inside the array returned by
createPowerShellProfilePathSnippet and related to pathConfigurationSentinel) to
use $env:PATH and guard against null before calling .Split() (e.g., treat
$env:PATH as empty string when null or check -and $env:PATH). Ensure the two
lines that currently call $env:Path.Split(...) and reference $env:Path are
changed to use $env:PATH with a null-safe expression.
- Around line 322-389: createBashProfileCandidates currently returns only
existing bash profile files and only synthesizes fallbacks when none exist,
which leaves the alternate bash startup path unconfigured; update
createBashProfileCandidates to (1) include ".bash_login" in the profileNames
check on non-darwin platforms so login-chain detection is complete, and (2) when
at least one profile exists, still synthesize the complementary profile(s) so
both login and non-login startup paths are covered (i.e., for each of the
intended fallbackNames ensure an entry is returned with profileExists true/false
based on the readTextFileIfExists results rather than returning early when
existing.length > 0). Use the existing symbols profileNames, fallbackNames,
readTextFileIfExists, and createBashProfileCandidates to implement this
behavior.
🪄 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: 5b385788-e303-4cf5-9be3-c95a5b83088c
📒 Files selected for processing (2)
src/application/self-update/path-configuration.test.tssrc/application/self-update/path-configuration.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/application/self-update/path-configuration.test.ts
Users with only a bash login profile or only .bashrc could still miss oo in the other startup mode. Fill in the missing fallback profile and make the PowerShell snippet handle an empty PATH without relying on case-specific environment variable spelling. Signed-off-by: Kevin Cui <bh@bugs.cc>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/application/self-update/path-configuration.ts (1)
331-335: Consider constructingprofileNamesfrom the declared constants.The
interactiveProfileNameandloginProfileNamesconstants are defined but not used to constructprofileNames. This creates a slight maintenance risk if the constants are updated but the array is forgotten.♻️ Optional: derive profileNames from constants
const interactiveProfileName = ".bashrc"; const loginProfileNames = [".bash_profile", ".bash_login", ".profile"] as const; -const profileNames = options.platform === "darwin" - ? [".bash_profile", ".bash_login", ".bashrc", ".profile"] - : [".bashrc", ".bash_profile", ".bash_login", ".profile"]; +const profileNames = options.platform === "darwin" + ? [...loginProfileNames.slice(0, 2), interactiveProfileName, loginProfileNames[2]] + : [interactiveProfileName, ...loginProfileNames];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/self-update/path-configuration.ts` around lines 331 - 335, Construct profileNames by reusing the existing constants interactiveProfileName and loginProfileNames instead of hardcoding arrays; update the logic that sets profileNames (currently using options.platform === "darwin") to produce the macOS order [ ...loginProfileNames, interactiveProfileName, ".profile" ] and the non-mac order [ interactiveProfileName, ...loginProfileNames, ".profile" ] so changes to interactiveProfileName or loginProfileNames automatically propagate; ensure you reference the symbols interactiveProfileName, loginProfileNames, profileNames and options.platform in the modified code.
🤖 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/path-configuration.ts`:
- Around line 331-335: Construct profileNames by reusing the existing constants
interactiveProfileName and loginProfileNames instead of hardcoding arrays;
update the logic that sets profileNames (currently using options.platform ===
"darwin") to produce the macOS order [ ...loginProfileNames,
interactiveProfileName, ".profile" ] and the non-mac order [
interactiveProfileName, ...loginProfileNames, ".profile" ] so changes to
interactiveProfileName or loginProfileNames automatically propagate; ensure you
reference the symbols interactiveProfileName, loginProfileNames, profileNames
and options.platform in the modified code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c078436-d78b-4f8a-a500-4f93e5e459d4
📒 Files selected for processing (2)
src/application/self-update/path-configuration.test.tssrc/application/self-update/path-configuration.ts
This was referenced Apr 30, 2026
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
1 participant
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.
Self-managed installs could succeed while leaving future shells unable to find
oountil users edited PATH manually. Persist the managed bin directory across supported shells, print a restart note when that succeeds, and keep an explicit opt-out with--no-modify-pathandOO_NO_MODIFY_PATH.fixed: #99