feat(install): add managed install and update commands#77
Merged
Conversation
Why: The CLI only exposed update checks and bootstrap scripts, so users could not repair or upgrade a managed install in place. What: Add top-level install/update/upgrade commands and the shared managed updater flow for platform resolution, locking, staging, activation, and verification. How: Reuse release metadata fetching, add startup lifetime locks, and update tests, snapshots, and command docs for the new flow. Signed-off-by: Kevin Cui <bh@bugs.cc>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 52 minutes and 28 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
WalkthroughAdds a complete self-update surface: new CLI commands Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Windows resolves the managed executable to oo.exe, while the existing snapshots assumed a Unix-style path. Replacing the resolved executable path with a snapshot placeholder keeps the install tests stable across supported platforms. Signed-off-by: Kevin Cui <bh@bugs.cc>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
src/application/update/release-metadata.ts (1)
73-102: Minor: duplicated warn log string across JSON-parse and invalid-version paths.Both the
response.json()failure branch (line 85) and theparseVersion(payload) === nullbranch (line 99) emit the identical "did not include a valid version" message, which makes the two failure modes indistinguishable in logs. Consider differentiating them (e.g.,"... response was not valid JSON."vs."... response did not include a valid version.") so operators can tell a 200-with-HTML from a 200-with-wrong-shape.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/update/release-metadata.ts` around lines 73 - 102, The two warning branches use identical messages; change the first catch around response.json() to log that the response was not valid JSON (include durationMs, requestUrl cliLatestReleaseMetadataUrl, status, and the parsing error if available) and keep the second branch (where parseVersion(payload) returns null) as the "did not include a valid version" message; update the code around payload/response.json(), the catch block, and the parseVersion/latestVersion check so options.logger.warn calls are distinct and include the same contextual fields (requestStartedAt, cliLatestReleaseMetadataUrl, response.status) and, for the JSON parse failure, include the thrown error information.src/application/self-update/lock.test.ts (1)
14-44: Consider a more robustly-stale PID than999_999_999.On Linux,
pid_maxcan be configured up to2^22 = 4194304, so999_999_999is always out of range and reliably "missing". That's fine, but the intent would read clearer if you asserted the cleanup uses an explicitly invalid PID (e.g.,-1or a comment). Purely a readability nit — no behavior change required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/self-update/lock.test.ts` around lines 14 - 44, In the "removes stale and malformed lock entries" test update the fake stale lock PID to be explicitly invalid (e.g., use pid: -1) or add an inline comment next to the 999_999_999 value to clarify intent so it's clear the test is using an explicitly invalid PID; locate the test block (test name: "removes stale and malformed lock entries") that writes the "stale.lock" entry and change the pid field or add the comment; no changes to cleanupStaleVersionLocks behavior are needed.src/application/update/update-notifier.ts (1)
54-58: Inline the delegated release-version fetch.
fetchLatestReleaseVersiononly forwards options tofetchLatestCliReleaseVersionand isasyncwithout anawait. Calling the shared helper directly at Line 54 removes the trivial wrapper.Proposed refactor
- const latestVersion = await fetchLatestReleaseVersion({ + const latestVersion = await fetchLatestCliReleaseVersion({ currentVersion: context.version, fetcher: context.fetcher, logger: context.logger, + parseVersion: parseLatestCliSemverReleaseVersion, }); @@ -async function fetchLatestReleaseVersion(options: { - currentVersion: string; - fetcher: Fetcher; - logger: CliExecutionContext["logger"]; -}): Promise<string | null> { - return fetchLatestCliReleaseVersion({ - currentVersion: options.currentVersion, - fetcher: options.fetcher, - logger: options.logger, - parseVersion: parseLatestCliSemverReleaseVersion, - }); -}As per coding guidelines, "Never create single-line functions that merely delegate to another function without adding logic, validation, or semantic value" and "Never mark a function
asyncif it contains noawait."Also applies to: 178-189
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/update/update-notifier.ts` around lines 54 - 58, Replace the trivial wrapper fetchLatestReleaseVersion by calling fetchLatestCliReleaseVersion directly where used (e.g., the call that assigns latestVersion and the other occurrence around lines 178-189); remove the unnecessary async wrapper function fetchLatestReleaseVersion from the module (or convert it to a non-async alias if absolutely needed) so you no longer call an extra delegating function, and update imports/usages to reference fetchLatestCliReleaseVersion instead (ensure you pass the same options: currentVersion/context.version, fetcher/context.fetcher, logger/context.logger).src/application/self-update/platform.ts (1)
31-45: Remove the unnecessary async boundary.
detectSelfUpdateReleasePlatformdoes only synchronous work, so returningPromise<string>adds avoidable async overhead and widens the API unnecessarily.Proposed refactor
-export async function detectSelfUpdateReleasePlatform(options: { +export function detectSelfUpdateReleasePlatform(options: { arch: string; platform: NodeJS.Platform; -}): Promise<string> { +}): string {As per coding guidelines, "Never mark a function
asyncif it contains noawait; removeasyncwhen all code paths are synchronous."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/self-update/platform.ts` around lines 31 - 45, The function detectSelfUpdateReleasePlatform is marked async but does only synchronous work; change its signature to return string (not Promise<string>) and remove the async keyword, and return the result of resolveSelfUpdateReleasePlatform directly. Update the parameter usage left intact (arch, platform, linuxLibc via detectLinuxLibcKind(), rosettaTranslated via isRosettaTranslated()) and ensure callers expecting a Promise are updated to handle a synchronous string return if needed; reference detectSelfUpdateReleasePlatform and resolveSelfUpdateReleasePlatform in your changes.src/application/commands/self-update.cli.test.ts (1)
35-105: Extract the repeated self-update fetcher setup.The three tests duplicate release-platform detection and URL branching for
/latest.jsonand the platform binary. A local helper likecreateSelfUpdateFetcher({ latestVersion, binaryBody, failOnLatest })would keep these cases aligned.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: 122-158
🤖 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 35 - 105, The tests duplicate release-platform detection and fetcher logic; extract that into a local helper (e.g., createSelfUpdateFetcher) and use it from the tests (the ones that call createCliSandbox, detectSelfUpdateReleasePlatform, toRequest and createSelfUpdateInstallSnapshot). Implement createSelfUpdateFetcher({ latestVersion, binaryBody, failOnLatest }) to return the async fetcher used in sandbox.run that handles requests ending with "/latest.json" and the platform binary (using detectSelfUpdateReleasePlatform to compute the binary path) and either returns the proper Response or throws when latest.json should not be requested; replace the repeated inline fetcher blocks in the tests with calls to this helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/commands.md`:
- Around line 107-135: The docs include implementation details (e.g.,
latest.json, entrypoint validation, “re-materializes”, string-vs-semver) that
shouldn't be part of the CLI contract; edit the `oo install` and `oo update`
sections to remove those internals and replace them with user-facing behavior
only: for `oo install` keep that [version] is optional and that omitting it
picks the latest published release, keep the `--force` option description and
the observable outputs (prints installed version and final executable path, and
a setup note when the executable dir is not on PATH), and retain the
special-case behavior for `0.0.0-development`; for `oo update` keep that it
takes no arguments, prints an "up-to-date" message when the current version
equals the published latest and prints the version-change result when it
differs, but remove mentions of entrypoint repair wording and string-vs-semver
decision logic—describe only the observable outcome (it ensures the managed
install is current). Ensure the headings `oo install` and `oo update`, option
`--force`, and the `0.0.0-development` note remain, but strip any references to
`latest.json`, "re-materializes", entrypoint validation, and string-equality vs
semver comparisons.
In `@docs/commands.zh-CN.md`:
- Around line 100-124: The current doc sections for the CLI commands `oo
install` and `oo update` expose internal implementation details (e.g.,
`latest.json`, "materialize", entry-file validation, and exact-string vs semver
comparison); update the text to describe only observable CLI behavior: for `oo
install` document the optional `[version]` argument, the `--force` option, the
user-visible outputs (success message with installed version and executable
path, and a setup note if the executable directory is not on PATH), and the
special-case behavior when the current version string is `0.0.0-development`
(prints an unsupported-for-hosted-install/update message and exits
successfully); for `oo update` describe that it updates the hosted `oo` to the
latest release, what messages are printed when there is no update vs when a
version change occurs, that it will repair or recreate the hosted entry as
needed (without exposing "materialize"/`--force`), and the same
`0.0.0-development` special-case — remove references to `latest.json`,
entry-file checks, "materialize", and "string equality vs semver" comparisons so
the contract stays user-facing only.
In `@src/application/self-update/core.ts`:
- Around line 348-355: The binary download call using
options.fetcher(options.url, ...) has no timeout/abort and can hang; wrap this
fetch in an AbortController with a timeout (reusing the release-metadata timeout
pattern) and pass controller.signal into the fetch options, ensure you clear the
timer on success, and handle the abort error path by rejecting/throwing a
timeout-specific error so the version lock/release logic in the surrounding flow
can run; update references in this block (options.fetcher, options.url,
options.currentVersion, response) to accept the signal and to handle the aborted
request cleanly.
- Around line 331-332: Currently the code calls
removePathBestEffort(targetVersionPath) before rename(versionTempPath,
targetVersionPath), which can delete the installed version if rename fails;
instead remove the pre-delete and perform an atomic replacement: attempt
rename(versionTempPath, targetVersionPath) directly (or implement an atomic swap
sequence appropriate for the platform), and only if that rename succeeds then
remove any temp/backup; if rename fails, leave targetVersionPath intact and
clean up versionTempPath; update the code around removePathBestEffort, rename,
versionTempPath and targetVersionPath to reflect this order and add error
handling to avoid deleting the existing version on failure.
- Around line 230-236: renderSelfUpdateLockBusyMessage currently returns
hardcoded English; change it to accept the command rendering context (e.g., a
CommandContext or renderer param) and use the catalog keys 'selfUpdate.lockBusy'
and 'selfUpdate.lockBusyWithPid' to produce the message. Keep the ownerPid
parameter and, when ownerPid is undefined, call the renderer with
'selfUpdate.lockBusy', otherwise call 'selfUpdate.lockBusyWithPid' and pass
ownerPid as the interpolation argument so the localized string can include the
PID.
- Around line 84-110: The performSelfUpdateOperation function currently uses
options.targetVersion directly when constructing filesystem paths (via
resolveSelfUpdateLockFilePath, resolveSelfUpdatePaths, staging/version file
builders); add an explicit validation step at the top of
performSelfUpdateOperation to reject unsafe CLI versions by either enforcing
semver (reuse the existing isSemver refinement logic used for release metadata)
or by rejecting any input that contains path separators or relative segments
("/", "\\", ".", ".."); if validation fails, throw a clear error before calling
resolveSelfUpdateLockFilePath/acquiring locks or touching staging/versions
directories so no path-traversal can occur.
In `@src/application/self-update/lock.ts`:
- Around line 349-360: The current check in the block that calls
readProcessCommandLine(lockData.pid, platform) uses substring matching of the
executableName (basename(lockData.execPath)) which can false-positive on
unrelated commands; update the logic in this area to stop using
normalizedCommandLine.includes(executableName) and instead perform an exact
token match for the executable name or rely solely on the full execPath match:
parse or split normalizedCommandLine into argument tokens (or use a
word-boundary regex) and check if any token equals the executableName (or
compare the full normalizedExecPath first), updating the return expression that
currently references normalizedCommandLine, normalizedExecPath, and
executableName to use the exact-token check.
In `@src/application/self-update/paths.ts`:
- Around line 1-2: resolveSelfUpdatePaths currently builds paths with
node:path.join/dirname which uses the host OS separators; switch to using the
platform-specific path module (path.win32 when platform === "win32", otherwise
path.posix) for all path operations inside resolveSelfUpdatePaths (and any
helper calls) so constructed paths use the target platform separators; replace
uses of join, dirname, and any tmpdir/sep-dependent logic at the referenced
spots (e.g., calls inside resolveSelfUpdatePaths and related helper variables
around lines noted in the review) to call the chosen path module's
join/dirname/etc., and update tests to assert using the same platform-specific
path helper based on the platform under test.
In `@src/application/update/release-metadata.ts`:
- Line 10: The hardcoded cliReleaseRequestTimeoutMs = 2000 is too short for
user-initiated install/update flows because the same value is reused by
functions in release-metadata.ts and causes transient failures; change the API
so the release metadata fetch accepts an optional timeoutMs parameter (default
to 2000 for background notifier callers) and update callers in install/update to
pass a longer timeout or omit it (no timeout) as appropriate; update references
to cliReleaseRequestTimeoutMs, the fetch function(s) in release-metadata.ts, and
the install/update call sites to use the new parameter.
---
Nitpick comments:
In `@src/application/commands/self-update.cli.test.ts`:
- Around line 35-105: The tests duplicate release-platform detection and fetcher
logic; extract that into a local helper (e.g., createSelfUpdateFetcher) and use
it from the tests (the ones that call createCliSandbox,
detectSelfUpdateReleasePlatform, toRequest and createSelfUpdateInstallSnapshot).
Implement createSelfUpdateFetcher({ latestVersion, binaryBody, failOnLatest })
to return the async fetcher used in sandbox.run that handles requests ending
with "/latest.json" and the platform binary (using
detectSelfUpdateReleasePlatform to compute the binary path) and either returns
the proper Response or throws when latest.json should not be requested; replace
the repeated inline fetcher blocks in the tests with calls to this helper.
In `@src/application/self-update/lock.test.ts`:
- Around line 14-44: In the "removes stale and malformed lock entries" test
update the fake stale lock PID to be explicitly invalid (e.g., use pid: -1) or
add an inline comment next to the 999_999_999 value to clarify intent so it's
clear the test is using an explicitly invalid PID; locate the test block (test
name: "removes stale and malformed lock entries") that writes the "stale.lock"
entry and change the pid field or add the comment; no changes to
cleanupStaleVersionLocks behavior are needed.
In `@src/application/self-update/platform.ts`:
- Around line 31-45: The function detectSelfUpdateReleasePlatform is marked
async but does only synchronous work; change its signature to return string (not
Promise<string>) and remove the async keyword, and return the result of
resolveSelfUpdateReleasePlatform directly. Update the parameter usage left
intact (arch, platform, linuxLibc via detectLinuxLibcKind(), rosettaTranslated
via isRosettaTranslated()) and ensure callers expecting a Promise are updated to
handle a synchronous string return if needed; reference
detectSelfUpdateReleasePlatform and resolveSelfUpdateReleasePlatform in your
changes.
In `@src/application/update/release-metadata.ts`:
- Around line 73-102: The two warning branches use identical messages; change
the first catch around response.json() to log that the response was not valid
JSON (include durationMs, requestUrl cliLatestReleaseMetadataUrl, status, and
the parsing error if available) and keep the second branch (where
parseVersion(payload) returns null) as the "did not include a valid version"
message; update the code around payload/response.json(), the catch block, and
the parseVersion/latestVersion check so options.logger.warn calls are distinct
and include the same contextual fields (requestStartedAt,
cliLatestReleaseMetadataUrl, response.status) and, for the JSON parse failure,
include the thrown error information.
In `@src/application/update/update-notifier.ts`:
- Around line 54-58: Replace the trivial wrapper fetchLatestReleaseVersion by
calling fetchLatestCliReleaseVersion directly where used (e.g., the call that
assigns latestVersion and the other occurrence around lines 178-189); remove the
unnecessary async wrapper function fetchLatestReleaseVersion from the module (or
convert it to a non-async alias if absolutely needed) so you no longer call an
extra delegating function, and update imports/usages to reference
fetchLatestCliReleaseVersion instead (ensure you pass the same options:
currentVersion/context.version, fetcher/context.fetcher, logger/context.logger).
🪄 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: c2c415d5-d7eb-474f-9e7e-d3d7be066400
⛔ 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 (23)
docs/commands.mddocs/commands.zh-CN.mdsrc/adapters/commander/commander-cli-adapter.tssrc/adapters/completion/static-completion-renderer.test.tssrc/adapters/store/file-store-utils.tssrc/application/bootstrap/run-cli.tssrc/application/commands/catalog.tssrc/application/commands/connector/schema-cache.tssrc/application/commands/install.tssrc/application/commands/self-update.cli.test.tssrc/application/commands/update.tssrc/application/self-update/core.test.tssrc/application/self-update/core.tssrc/application/self-update/lock.test.tssrc/application/self-update/lock.tssrc/application/self-update/paths.test.tssrc/application/self-update/paths.tssrc/application/self-update/platform.test.tssrc/application/self-update/platform.tssrc/application/shared/fs-errors.tssrc/application/update/release-metadata.tssrc/application/update/update-notifier.tssrc/i18n/catalog.ts
|
|
||
| export const cliReleaseBaseUrl = "https://static.oomol.com/release/apps/oo-cli"; | ||
| export const cliLatestReleaseMetadataUrl = `${cliReleaseBaseUrl}/latest.json`; | ||
| export const cliReleaseRequestTimeoutMs = 2000; |
There was a problem hiding this comment.
2000 ms timeout is likely too aggressive for user-initiated install/update.
cliReleaseRequestTimeoutMs = 2000 made sense for the background update notifier (fail-fast, never block the CLI), but this function is now reused by install/update commands where a transient timeout will surface as errors.selfUpdate.latestVersionUnavailable and block the user's explicit request. On slow/constrained networks this will be user-visible and flaky.
Consider letting callers pass timeoutMs (defaulting to 2000 ms for the notifier), and use a longer value — or omit the timeout entirely — for install/update.
🔧 Proposed API change
export async function fetchLatestCliReleaseVersion(options: {
currentVersion: string;
fetcher: Fetcher;
logger: Logger;
parseVersion?: (payload: unknown) => string | null;
+ timeoutMs?: number;
}): Promise<string | null> {
const parseVersion = options.parseVersion ?? parseLatestCliReleaseVersion;
+ const timeoutMs = options.timeoutMs ?? cliReleaseRequestTimeoutMs;
const requestStartedAt = Date.now();
@@
- cliReleaseRequestTimeoutMs,
+ timeoutMs,
);Also applies to: 20-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/application/update/release-metadata.ts` at line 10, The hardcoded
cliReleaseRequestTimeoutMs = 2000 is too short for user-initiated install/update
flows because the same value is reused by functions in release-metadata.ts and
causes transient failures; change the API so the release metadata fetch accepts
an optional timeoutMs parameter (default to 2000 for background notifier
callers) and update callers in install/update to pass a longer timeout or omit
it (no timeout) as appropriate; update references to cliReleaseRequestTimeoutMs,
the fetch function(s) in release-metadata.ts, and the install/update call sites
to use the new parameter.
Native install and update should clean up older global package manager installs so users do not keep conflicting oo entrypoints. Add best-effort uninstall detection for npm, pnpm, yarn, and bun based installs, wire it into the shared self-update flow, and cover the matching rules with self-update tests and docs. Signed-off-by: Kevin Cui <bh@bugs.cc>
Managed install and update commands felt silent in interactive use, which made long-running downloads and repairs hard to trust. Render staged progress to stderr on TTYs while keeping stable stdout success messages for scripts. Share the terminal progress renderer across commands and cover the new behavior in docs and self-update tests. Signed-off-by: Kevin Cui <bh@bugs.cc>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/application/self-update/core.ts (4)
373-379:⚠️ Potential issue | 🟠 MajorAdd an explicit timeout to the binary download.
This fetch can hang while the version lock is held. Use an
AbortControllertimeout and clear it infinally, similar to the release-metadata request path.Direction
+ const abortController = new AbortController(); + const timeoutId = setTimeout(() => abortController.abort(), binaryDownloadTimeoutMs); + try { response = await options.fetcher(options.url, { headers: { "accept": "application/octet-stream", "user-agent": `${APP_NAME}/${options.currentVersion}`, }, + signal: abortController.signal, }); } + finally { + clearTimeout(timeoutId); + }Verification:
#!/bin/bash # Verify binary-download fetch calls and whether a timeout/signal already exists elsewhere. rg -n -C5 'fetchBinaryResponse|options\.fetcher\(options\.url|AbortController|timeout' --type=ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/self-update/core.ts` around lines 373 - 379, The binary download call using options.fetcher(options.url, ...) can hang; add an AbortController with a timeout (like the release-metadata request path) and pass controller.signal into the fetcher call, store the timeout id, and ensure you clearTimeout and call controller.abort in a finally block; update the code around the options.fetcher(...) call (and the response variable handling) to use the AbortController signal and clear the timer in finally so the request is always cleaned up.
84-115:⚠️ Potential issue | 🟠 MajorValidate
targetVersionbefore using it in filesystem paths.Line 109 and the path builders at Lines 294-328 use
targetVersiondirectly.install [version]and release metadata can provide any non-empty string, so values containing separators or..can escape the managed locks/staging/versions directories when joined into paths.Direction
import { APP_NAME } from "../config/app-config.ts"; import { CliUserError } from "../contracts/cli.ts"; +import { isSemver } from "../semver.ts"; ... export async function performSelfUpdateOperation(options: { @@ }): Promise<SelfUpdateOperationOutcome> { + assertSelfUpdateVersion(options.targetVersion); + const paths = resolveSelfUpdatePaths({ @@ } + +function assertSelfUpdateVersion(version: string): void { + if (isSemver(version)) { + return; + } + + throw new CliUserError("errors.selfUpdate.invalidVersion", 1, { + version, + }); +}Add the corresponding i18n catalog entry, or enforce the same semver rule at both release parsing and command input before calling this function.
Verification:
#!/bin/bash # Verify all version-to-path call sites and current install input validation. rg -n -C3 'resolveSelfUpdate.*Path|targetVersion|installCommandInputSchema|parseLatestCliReleaseVersion' --type=ts node <<'EOF' const path = require('node:path'); console.log(path.join('/tmp/oo/versions', '../../../escape')); console.log(path.join('/tmp/oo/locks', '../../escape.lock')); EOFAs 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: 285-328
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/self-update/core.ts` around lines 84 - 115, Validate and sanitize options.targetVersion before any filesystem use: enforce the same semver/check used when parsing releases (e.g., reuse parseLatestCliReleaseVersion or the installCommandInputSchema) and reject or normalize values containing path separators or path traversal segments (like '/' '\' or '..'); do this at the start of performSelfUpdateOperation and before calling resolveSelfUpdatePaths, resolveSelfUpdateLockFilePath and any path builders (referenced in the diff and lines ~294-328) so only safe, canonical version identifiers are joined into locks/staging/versions paths (alternatively add an i18n error and surface a clear rejection message when validation fails).
246-252:⚠️ Potential issue | 🟡 MinorLocalize the lock-busy message.
This is user-facing output, but it always returns English. Route it through translator keys so
oo install/update --lang zhstays localized.Direction
-export function renderSelfUpdateLockBusyMessage(ownerPid?: number): string { +export function renderSelfUpdateLockBusyMessage( + translator: Pick<CliExecutionContext["translator"], "t">, + ownerPid?: number, +): string { if (ownerPid === undefined) { - return "Another update is already in progress. Please try again later."; + return translator.t("selfUpdate.lockBusy"); } - return `Another update is already in progress (PID ${ownerPid}). Please try again later.`; + return translator.t("selfUpdate.lockBusyWithPid", { ownerPid }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/self-update/core.ts` around lines 246 - 252, The string returned from renderSelfUpdateLockBusyMessage is currently hard-coded in English; change it to use the app's localization/translation utility (e.g., the translator function used elsewhere like t or translate) and return the appropriate translation key(s) instead, passing ownerPid as an interpolation parameter when present; update both the undefined-owner and PID branches to call the translator with keys such as "selfUpdate.lockBusy" and "selfUpdate.lockBusyWithPid" (or the project's existing keys) so oo install/update --lang zh yields localized output.
356-357:⚠️ Potential issue | 🟠 MajorDo not delete the existing version before replacement succeeds.
Line 356 removes the installed target before
rename()succeeds. If the process is interrupted orrename()fails, a forced reinstall can leave that managed version missing.Safer replacement
- await removePathBestEffort(targetVersionPath); await rename(versionTempPath, targetVersionPath);Verification:
#!/bin/bash # Confirm the current replacement order and any other pre-delete-before-rename patterns. rg -n -C4 'removePathBestEffort\(targetVersionPath\)|rename\(versionTempPath, targetVersionPath\)' --type=ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/self-update/core.ts` around lines 356 - 357, The code deletes the existing target (removePathBestEffort(targetVersionPath)) before renaming the temp dir, which can leave the managed version missing if rename() fails; change the logic to attempt the atomic rename first (rename(versionTempPath, targetVersionPath)) and only remove/replace the old target if rename fails due to platform-specific overwrite behavior: try rename(versionTempPath, targetVersionPath) directly, on EEXIST or a rename failure attempt removePathBestEffort(targetVersionPath) and retry rename once, and always ensure the temp path is cleaned up only after a successful rename; update the block around removePathBestEffort, rename, versionTempPath and targetVersionPath to implement this retry-and-cleanup flow.docs/commands.zh-CN.md (1)
100-132:⚠️ Potential issue | 🟡 MinorKeep the command reference at the user-facing contract level.
Lines 100-101, 106-107, 125-126, and 131-132 still expose implementation details (
latest.json, internal validation/materialization, and comparison mechanics). Please describe only observable CLI behavior.Suggested wording
-- 参数：`[version]` 为可选参数。省略时，`oo` 会从发布的 `latest.json` - 元数据解析目标版本。 +- 参数：`[version]` 为可选参数。省略时，`oo` 会安装最新发布版本。 ... -- 说明：install 在报告成功前会校验最终入口文件存在，并且它所指向的是有效的 - 托管版本。 +- 说明：install 在报告成功前会确认安装后的 `oo` 入口可用。 ... -- 输出：当最新发布版本与当前版本字符串完全相等时，CLI 会在必要时修复托管入口， +- 输出：当当前版本已经是最新发布版本时，CLI 会在必要时修复托管入口， 然后打印“已是最新版本”的消息。 -- 输出：当最新发布版本与当前版本字符串不相等时，CLI 会打印版本变更结果。 +- 输出：当存在新的发布版本时，CLI 会打印版本变更结果。 ... -- 说明：`oo update` 总是会修复或重新 materialize 目标版本，不额外暴露 +- 说明：`oo update` 会确保托管入口可用，不额外暴露 `--force`。 -- 说明：update 判断“是否已是最新版本”时使用精确字符串相等，而不是 semver - 大小比较。As per coding guidelines, Documentation under
docs/commands*.mdshould describe only user-facing CLI contract (purpose, arguments, options, output shapes, observable behavior), not internal implementation details.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/commands.zh-CN.md` around lines 100 - 132, The docs expose internal implementation details for the oo install/update commands; update the user-facing text for the "oo install" and "oo update" entries to remove references to internals (e.g. latest.json, materialize/validate wording, semver comparison mechanics) and instead state only observable behavior: for "oo install" keep that [version] is optional, --force reinstalls even if target exists, CLI prints installed version and final executable path, renders colored progress when stderr is a TTY, verifies the final entrypoint exists and is a valid hosted installation (phrase this as an observable check, not as "materialize"/"validate"), prints a PATH setup note if the executable directory is not on PATH, attempts to remove legacy `@oomol-lab/oo-cli` installs after success and reports nothing on cleanup failure, and prints a non-supported message and exits successfully for 0.0.0-development; for "oo update" state it takes no args, prints "already up to date" when the published version string equals the current version (avoid describing comparison algorithm), prints version change when updated, renders colored progress on TTY stderr, always repairs the hosted entrypoint as needed (describe as an observable repair action without "materialize"), attempts legacy cleanup similarly, and prints the 0.0.0-development notice — replace the four offending lines (100-101, 106-107, 125-126, 131-132) accordingly.
🧹 Nitpick comments (1)
src/application/self-update/legacy-installation.test.ts (1)
14-63: Build path fixtures withnode:pathhelpers.These fixtures hardcode POSIX separators. Since the detector normalizes both
/and\, usingjoin()for generic fixtures keeps the tests platform-neutral while still validating segment matching.Example direction
+import { join } from "node:path"; import { describe, expect, test } from "bun:test"; ... - execPath: "/Users/demo/.bun/install/global/node_modules/@oomol-lab/oo-cli/bin/oo", + execPath: join("Users", "demo", ".bun", "install", "global", "node_modules", "@oomol-lab", "oo-cli", "bin", "oo"), ... - resolveCommandPath: commandName => `/mock/bin/${commandName}`, + resolveCommandPath: commandName => join("mock", "bin", commandName), ... - commandPath: "/mock/bin/pnpm", + commandPath: join("mock", "bin", "pnpm"),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: 84-105, 125-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/self-update/legacy-installation.test.ts` around lines 14 - 63, Tests hardcode POSIX separators in execPath fixtures; update them to use node:path helpers so they run on Windows too. Import path (e.g., import { join } from "node:path") at the top of the test file and replace hardcoded strings with join(...) calls for each fixture passed to detectLegacyPackageManager (references: detectLegacyPackageManager tests that assert "bun", "pnpm", "yarn", "npm", and the unrelated-segment case). Apply the same change to the other mentioned test ranges (lines ~84-105 and ~125) so all execPath fixtures use join() instead of literal "/" separators.
🤖 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/core.ts`:
- Around line 337-344: The code writes the downloaded payload from
fetchBinaryResponse directly to stagingBinaryPath via Bun.write without
verifying integrity; update the logic around fetchBinaryResponse and Bun.write
to first obtain and verify a trusted release checksum/signature (from release
metadata or a separate .sig/.sha256 resource) by computing the download's digest
(e.g., SHA-256) or verifying a cryptographic signature against the project's
verification key, and only call Bun.write(stagingBinaryPath, response) if
verification succeeds; on mismatch, log via options.runtime.logger and
throw/return an error so the unverified binary is never written or activated.
---
Duplicate comments:
In `@docs/commands.zh-CN.md`:
- Around line 100-132: The docs expose internal implementation details for the
oo install/update commands; update the user-facing text for the "oo install" and
"oo update" entries to remove references to internals (e.g. latest.json,
materialize/validate wording, semver comparison mechanics) and instead state
only observable behavior: for "oo install" keep that [version] is optional,
--force reinstalls even if target exists, CLI prints installed version and final
executable path, renders colored progress when stderr is a TTY, verifies the
final entrypoint exists and is a valid hosted installation (phrase this as an
observable check, not as "materialize"/"validate"), prints a PATH setup note if
the executable directory is not on PATH, attempts to remove legacy
`@oomol-lab/oo-cli` installs after success and reports nothing on cleanup failure,
and prints a non-supported message and exits successfully for 0.0.0-development;
for "oo update" state it takes no args, prints "already up to date" when the
published version string equals the current version (avoid describing comparison
algorithm), prints version change when updated, renders colored progress on TTY
stderr, always repairs the hosted entrypoint as needed (describe as an
observable repair action without "materialize"), attempts legacy cleanup
similarly, and prints the 0.0.0-development notice — replace the four offending
lines (100-101, 106-107, 125-126, 131-132) accordingly.
In `@src/application/self-update/core.ts`:
- Around line 373-379: The binary download call using
options.fetcher(options.url, ...) can hang; add an AbortController with a
timeout (like the release-metadata request path) and pass controller.signal into
the fetcher call, store the timeout id, and ensure you clearTimeout and call
controller.abort in a finally block; update the code around the
options.fetcher(...) call (and the response variable handling) to use the
AbortController signal and clear the timer in finally so the request is always
cleaned up.
- Around line 84-115: Validate and sanitize options.targetVersion before any
filesystem use: enforce the same semver/check used when parsing releases (e.g.,
reuse parseLatestCliReleaseVersion or the installCommandInputSchema) and reject
or normalize values containing path separators or path traversal segments (like
'/' '\' or '..'); do this at the start of performSelfUpdateOperation and before
calling resolveSelfUpdatePaths, resolveSelfUpdateLockFilePath and any path
builders (referenced in the diff and lines ~294-328) so only safe, canonical
version identifiers are joined into locks/staging/versions paths (alternatively
add an i18n error and surface a clear rejection message when validation fails).
- Around line 246-252: The string returned from renderSelfUpdateLockBusyMessage
is currently hard-coded in English; change it to use the app's
localization/translation utility (e.g., the translator function used elsewhere
like t or translate) and return the appropriate translation key(s) instead,
passing ownerPid as an interpolation parameter when present; update both the
undefined-owner and PID branches to call the translator with keys such as
"selfUpdate.lockBusy" and "selfUpdate.lockBusyWithPid" (or the project's
existing keys) so oo install/update --lang zh yields localized output.
- Around line 356-357: The code deletes the existing target
(removePathBestEffort(targetVersionPath)) before renaming the temp dir, which
can leave the managed version missing if rename() fails; change the logic to
attempt the atomic rename first (rename(versionTempPath, targetVersionPath)) and
only remove/replace the old target if rename fails due to platform-specific
overwrite behavior: try rename(versionTempPath, targetVersionPath) directly, on
EEXIST or a rename failure attempt removePathBestEffort(targetVersionPath) and
retry rename once, and always ensure the temp path is cleaned up only after a
successful rename; update the block around removePathBestEffort, rename,
versionTempPath and targetVersionPath to implement this retry-and-cleanup flow.
---
Nitpick comments:
In `@src/application/self-update/legacy-installation.test.ts`:
- Around line 14-63: Tests hardcode POSIX separators in execPath fixtures;
update them to use node:path helpers so they run on Windows too. Import path
(e.g., import { join } from "node:path") at the top of the test file and replace
hardcoded strings with join(...) calls for each fixture passed to
detectLegacyPackageManager (references: detectLegacyPackageManager tests that
assert "bun", "pnpm", "yarn", "npm", and the unrelated-segment case). Apply the
same change to the other mentioned test ranges (lines ~84-105 and ~125) so all
execPath fixtures use join() instead of literal "/" separators.
🪄 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: b914cabb-c0bd-4cb2-a29d-b19c45cf2283
📒 Files selected for processing (17)
docs/commands.mddocs/commands.zh-CN.mdsrc/application/commands/install.tssrc/application/commands/self-update-progress.test.tssrc/application/commands/self-update-progress.tssrc/application/commands/self-update.cli.test.tssrc/application/commands/shared/terminal-progress-renderer.tssrc/application/commands/skills/install-progress.tssrc/application/commands/skills/progress-renderer.test.tssrc/application/commands/skills/update-progress.tssrc/application/commands/update.tssrc/application/self-update/core.test.tssrc/application/self-update/core.tssrc/application/self-update/legacy-installation.test.tssrc/application/self-update/legacy-installation.tssrc/application/self-update/progress.tssrc/i18n/catalog.ts
✅ Files skipped from review due to trivial changes (6)
- src/application/commands/skills/update-progress.ts
- src/application/commands/skills/progress-renderer.test.ts
- src/i18n/catalog.ts
- src/application/commands/self-update.cli.test.ts
- docs/commands.md
- src/application/commands/update.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/application/self-update/core.test.ts
- src/application/commands/install.ts
Self-update had a few rough edges around stalled downloads, platform-specific paths, and Windows replacement behavior. This change adds bounded retry and timeout handling, shares common file helpers, localizes lock contention, and keeps the documented install/update contract focused on user behavior. It also normalizes invalid-version errors for CLI callers. Signed-off-by: Kevin Cui <bh@bugs.cc>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/core.ts`:
- Line 6: Replace the unsafe startsWith(join(..., "")) containment check with
node:path.relative-based logic: compute const rel =
relative(resolvedVersionsDirectory, targetPath) (after resolving/normalizing
both paths) and treat targetPath as inside the versions directory only if rel
!== '' && !rel.startsWith('..' + path.sep) && rel !== '..' (or simply
!rel.split(path.sep).includes('..')). Update the check where
resolvedVersionsDirectory and the candidate path are compared (the code using
resolvedVersionsDirectory and startsWith) and the similar checks around lines
879-893 to use this relative() pattern and node:path path.sep handling to
prevent sibling-directory escapes.
- Around line 540-566: The function writeBinaryResponseToFile currently returns
early when response.body is missing; instead treat that as a download failure:
when reader === undefined, call options.updateAbortReason with a failure reason
(e.g., "timeout" or "stall"), abort via options.abortController.abort(), ensure
the opened file handle (fileHandle) is properly closed and the partial file
removed (await fileHandle.close() and await unlink(options.outputPath) or
equivalent) to avoid leaving an empty staged binary, then throw an Error
describing the missing response body so callers can handle the failure. Ensure
all cleanup uses the existing fileHandle and options symbols in the function.
- Around line 459-502: The download timeout created when wiring up
abortController is never cleared on early error paths, so locate the timer
variable used for setTimeout when creating the abort (the identifier returned
from setTimeout) and call clearTimeout(timeoutId) before every throw in the
fetch flow: inside the catch block before both the timeout branch (abortReason
=== "timeout") and the generic error branch that logs and throws a CliUserError,
and also before the non-OK response branch that logs and throws; ensure you
reference the existing abortController/abortReason symbols and use the same
timer identifier name used where the timeout is set.
In `@src/application/shared/fs-utils.ts`:
- Around line 30-34: The write loop in fs-utils.ts that uses
fileHandle.write(chunk.subarray(offset)) can hang if writeResult.bytesWritten
=== 0; update the loop in the helper containing variables offset, chunk and the
call fileHandle.write so that after each write you check if
writeResult.bytesWritten === 0 and, if so, throw a descriptive error (e.g.,
"write made no progress: bytesRemaining=X") to fail fast instead of spinning;
keep existing offset increment logic and error message should include
bytesRemaining and the current offset to aid debugging.
In `@src/application/update/release-metadata.test.ts`:
- Around line 13-19: The fake fetcher implementations used in the tests (the
async fetcher that calls Bun.sleep and returns new Response(...)) ignore the
incoming init.signal so the test can pass even if timeout/abort wiring
regresses; update the fetcher(s) to guard at the top by checking if
init?.signal?.aborted and throw an AbortError (or return a rejected promise) and
attach a signal 'abort' event listener to reject when signalled so both fakes
observe init.signal and cannot hang; extract the repeated fake fetcher setup
into a local factory function (e.g., makeFakeFetcher) and use it for both
instances to avoid copy-paste.
🪄 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: cd04d81e-862c-4e7c-897a-e9b876abb355
📒 Files selected for processing (17)
docs/commands.mddocs/commands.zh-CN.mdsrc/application/bootstrap/run-cli.tssrc/application/commands/file/download/file-system.tssrc/application/commands/install.tssrc/application/commands/self-update.cli.test.tssrc/application/commands/update.tssrc/application/self-update/core.test.tssrc/application/self-update/core.tssrc/application/self-update/lock.test.tssrc/application/self-update/lock.tssrc/application/self-update/paths.test.tssrc/application/self-update/paths.tssrc/application/shared/fs-utils.tssrc/application/update/release-metadata.test.tssrc/application/update/release-metadata.tssrc/i18n/catalog.ts
✅ Files skipped from review due to trivial changes (7)
- src/i18n/catalog.ts
- src/application/self-update/lock.test.ts
- src/application/self-update/paths.test.ts
- docs/commands.md
- src/application/commands/self-update.cli.test.ts
- src/application/self-update/lock.ts
- src/application/self-update/core.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/application/bootstrap/run-cli.ts
- src/application/update/release-metadata.ts
- src/application/commands/update.ts
- src/application/self-update/paths.ts
Comment on lines
+540
to
+566
| async function writeBinaryResponseToFile(options: { | ||
| abortController: AbortController; | ||
| outputPath: string; | ||
| response: Response; | ||
| stallTimeoutMs: number; | ||
| updateAbortReason: (reason: "stall" | "timeout") => void; | ||
| }): Promise<void> { | ||
| const reader = options.response.body?.getReader(); | ||
| const fileHandle = await open(options.outputPath, "w"); | ||
| let stallTimeoutId: ReturnType<typeof setTimeout> | undefined; | ||
|
|
||
| const resetStallTimeout = (): void => { | ||
| if (stallTimeoutId !== undefined) { | ||
| clearTimeout(stallTimeoutId); | ||
| } | ||
|
|
||
| stallTimeoutId = setTimeout(() => { | ||
| options.updateAbortReason("stall"); | ||
| options.abortController.abort(); | ||
| }, options.stallTimeoutMs); | ||
| stallTimeoutId.unref?.(); | ||
| }; | ||
|
|
||
| try { | ||
| if (reader === undefined) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Treat a missing binary response body as a download failure.
Line 565 returns success when response.body is unavailable, producing an empty staged binary that can still be copied, chmodded, activated, and pass the current path-only verification.
Proposed failure path
if (reader === undefined) {
- return;
+ throw new Error("Binary download response body is unavailable.");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/application/self-update/core.ts` around lines 540 - 566, The function
writeBinaryResponseToFile currently returns early when response.body is missing;
instead treat that as a download failure: when reader === undefined, call
options.updateAbortReason with a failure reason (e.g., "timeout" or "stall"),
abort via options.abortController.abort(), ensure the opened file handle
(fileHandle) is properly closed and the partial file removed (await
fileHandle.close() and await unlink(options.outputPath) or equivalent) to avoid
leaving an empty staged binary, then throw an Error describing the missing
response body so callers can handle the failure. Ensure all cleanup uses the
existing fileHandle and options symbols in the function.
Keep the self-update timeout and stall timers referenced so in-memory or mocked fetchers cannot wait forever for an abort that never fires. This makes the timeout contract depend on the self-update logic itself instead of incidental runtime handles. Adjust the lock test for Windows, where process command lines are unavailable and a live pid must still be treated as active. Signed-off-by: Kevin Cui <bh@bugs.cc>
The interactive update test could render progress before the tarball gate resolver was assigned, which made the CI run hang until Bun timed the test out. Build the gate before starting the CLI and always release it during cleanup so logger state cannot leak into later CLI tests. Signed-off-by: Kevin Cui <bh@bugs.cc>
Self-update needed tighter failure handling around cleanup and binary downloads. The previous flow could misclassify paths outside the managed versions directory and keep timeout callbacks alive after early failures. This change checks activated versions with relative path boundaries, treats missing bodies and zero-progress writes as hard errors, and adds regression coverage for cleanup, timeout, and partial-write cases. Signed-off-by: Kevin Cui <bh@bugs.cc>
This was referenced Apr 21, 2026
This was referenced Apr 30, 2026
This was referenced May 7, 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.
Why: The CLI only exposed update checks and bootstrap scripts, so users could not repair or upgrade a managed install in place. What: Add top-level install/update/upgrade commands and the shared managed updater flow for platform resolution, locking, staging, activation, and verification.
How: Reuse release metadata fetching, add startup lifetime locks, and update tests, snapshots, and command docs for the new flow.