Skip to content

feat(uninstall): add oo uninstall command#245

Merged
BlackHole1 merged 3 commits into
mainfrom
support-uninstall
May 29, 2026
Merged

feat(uninstall): add oo uninstall command#245
BlackHole1 merged 3 commits into
mainfrom
support-uninstall

Conversation

@BlackHole1
Copy link
Copy Markdown
Member

Introduce oo uninstall to remove the managed oo runtime and its oo-owned skills, mirroring what oo install puts in place. By default it removes the managed executable, installed versions, self-update staging and locks, bundled skills, and the preset registry skill package, while leaving PATH config, user-authored skills, and unmanaged same-name directories untouched.

Skill removal is gated on .oo-metadata.json ownership so user-authored same-name skills are never deleted. --purge also clears user data and all oo-managed registry skills, --dry-run prints the plan without deleting, and -y/--yes is required in non-interactive terminals. On Windows the running executable is removed via a deferred one-shot PowerShell helper since a process cannot delete itself in place.

The preset skill package list moves into a shared preset-packages module so install and uninstall share a single source of truth, and the command refuses to run while another live oo process holds an active version marker.

Introduce `oo uninstall` to remove the managed oo runtime and its
oo-owned skills, mirroring what `oo install` puts in place. By default
it removes the managed executable, installed versions, self-update
staging and locks, bundled skills, and the preset registry skill
package, while leaving PATH config, user-authored skills, and
unmanaged same-name directories untouched.

Skill removal is gated on `.oo-metadata.json` ownership so user-authored
same-name skills are never deleted. `--purge` also clears user data and
all oo-managed registry skills, `--dry-run` prints the plan without
deleting, and `-y/--yes` is required in non-interactive terminals. On
Windows the running executable is removed via a deferred one-shot
PowerShell helper since a process cannot delete itself in place.

The preset skill package list moves into a shared preset-packages module
so install and uninstall share a single source of truth, and the command
refuses to run while another live oo process holds an active version
marker.

Signed-off-by: Kevin Cui <bh@bugs.cc>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 29, 2026

Review Change Stack

Summary by CodeRabbit

  • New Features

    • Added oo uninstall to remove the managed runtime and built-in skills with --yes, --dry-run, and --purge; shows an uninstall plan, requires confirmation, and refuses when another active runtime is detected.
    • Platform-aware removal: defers Windows binary deletion to a helper and prints package-manager guidance when not natively installed. Secrets are never emitted to stdout/stderr.
  • Documentation

    • English and Chinese command reference added, covering options, safety rules, purge effects, and retained vs removed items.
  • Tests

    • Comprehensive CLI and uninstall behavior tests covering dry-run, purge, platform and concurrency scenarios.

Walkthrough

This PR implements a complete oo uninstall command for the CLI. The change extracts preset package names into a dedicated module, implements self-uninstall planning (categorizing skills and runtime by removal eligibility via metadata inspection) and execution (in-process removal on Unix, spawned PowerShell helper for deferred Windows cleanup), integrates the command into the CLI with user confirmation flow, telemetry recording, and comprehensive testing. Documentation and i18n messages cover command behavior, options (--yes, --dry-run, --purge), platform-specific mechanics, and safety constraints around concurrency and secret leakage.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant CLI as oo uninstall
  participant Plan as buildSelfUninstallPlan
  participant Lock as findAnyActiveVersionOwner
  participant Perform as performSelfUninstall
  participant Helper as Windows Cleanup
  participant Filesystem as File System

  User->>CLI: oo uninstall --yes --purge
  CLI->>Plan: analyze skills/runtime/data
  Plan->>Plan: classify by metadata
  Plan-->>CLI: UninstallPlan

  CLI->>Perform: execute plan
  Perform->>Lock: check active owner
  alt owner found
    Lock-->>Perform: ownerPid
    Perform-->>CLI: error (busy)
  else no owner
    Lock-->>Perform: undefined
    alt Windows + deferred items
      Perform->>Helper: spawn PowerShell script
      Helper->>Filesystem: wait for process
      Helper->>Filesystem: delete deferred paths
      Helper->>Filesystem: self-delete
    end
    Perform->>Filesystem: rm immediate items
    Filesystem-->>Perform: failedPaths[]
    Perform-->>CLI: result
  end

  CLI-->>User: success or error
Loading

Possibly related PRs

  • oomol-lab/oo-cli#48: Changes to uninstallManagedSkill output (adds silent option) that the new uninstall flow may call into.
  • oomol-lab/oo-cli#165: Introduced preset registry skill packages; this PR reuses/refactors that preset package list for uninstall decisions.
  • oomol-lab/oo-cli#238: Related changes to telemetry handling that overlap with the skills.uninstall telemetry entry added here.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat(uninstall): add oo uninstall command' follows the required format and clearly describes the main change—adding a new uninstall command.
Description check ✅ Passed The description comprehensively explains the PR's purpose, detailing what the uninstall command does, its flags, safety features, and refactoring of preset packages.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch support-uninstall

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (5)
docs/commands.md (1)

433-440: ⚡ Quick win

Keep uninstall docs at user-facing behavior level.

This section exposes internal mechanics (“one-shot PowerShell helper” and “active version marker”). Please rephrase to externally observable behavior (deferred Windows cleanup after process exit; refuses when another oo process is running).

As per coding guidelines, “Documentation under docs/commands*.md should describe only user-facing CLI contract ... Do not document internal implementation details.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/commands.md` around lines 433 - 440, Reword the uninstall docs to remove
internal implementation details: replace the Windows sentence that mentions
"`~/.local/bin/oo.exe`" and "one-shot PowerShell helper" with a user-facing
statement like "On Windows the executable is removed after the process exits
(cleanup deferred)," and replace the sentence mentioning "active version marker"
with a user-facing statement that the command "refuses to run while another live
`oo` process is running"; do not mention helper scripts, file paths, markers, or
other internal mechanics and keep the description focused on observable CLI
behavior.
docs/commands.zh-CN.md (1)

371-376: ⚡ Quick win

文档建议改为仅描述用户可见行为。

这里写到了内部实现细节(一次性 PowerShell helper、active version marker)。建议改成用户可观察到的结果描述(Windows 下进程退出后再删除可执行文件;检测到其他 oo 进程运行时拒绝执行)。

As per coding guidelines, “Documentation under docs/commands*.md should describe only user-facing CLI contract ... Do not document internal implementation details.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/commands.zh-CN.md` around lines 371 - 376, 将文档从描述内部实现细节(例如“一次性
PowerShell helper”、“active version marker”)改为只描述用户可见行为:在涉及 `~/.local/bin/oo.exe`
的段落,移除对 PowerShell helper 的说明,改写为“Windows
下可执行文件将在进程退出后被删除;其它路径(versions、staging、locks、skills)会在命令执行期间被删除/清理”;在安全段落,移除对
`active version marker` 的内部名词,改写为“当检测到另一个运行中的 oo 进程时,命令会拒绝执行”;保留关于不将 API key
等机密写入 stdout/stderr 的用户可观察承诺。确保只描述可见行为并删除任何实现细节或内部术语。
src/application/commands/uninstall.cli.test.ts (3)

79-307: ⚡ Quick win

Extract the repeated sandbox setup/cleanup pattern into a local factory helper.

Each test repeats the same sandbox lifecycle; a small local helper will reduce duplication and maintenance churn.

As per coding guidelines, "In test files, extract repeated setup (mock, stub, or setup objects) into a local factory function at the bottom of the file."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/application/commands/uninstall.cli.test.ts` around lines 79 - 307, Tests
repeat the same sandbox lifecycle (createCliSandbox(), try { ... await
sandbox.cleanup(); } finally) — introduce a local factory helper (e.g.,
runWithSandbox or withCliSandbox) at the bottom of this test file that creates
the sandbox, yields it to an async callback, and guarantees cleanup in a finally
block; update each test to call this helper and move usages of createCliSandbox,
sandbox.run, seedRuntime, seedHostSkill, and sandbox.cleanup into the callback
so the helper centralizes setup/teardown and reduces duplication.

21-75: ⚡ Quick win

Move file-local test helpers to the end of the test file.

These helpers appear file-scoped; placing them at the bottom keeps test flow first and aligns with repo test conventions.

As per coding guidelines, "If a test helper function might be called by other test files, place it in the __tests__/helpers.ts file. Otherwise, place the function in the test file at the end of that file."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/application/commands/uninstall.cli.test.ts` around lines 21 - 75, Move
the file-local test helper functions selfUpdatePaths, storePaths, seedRuntime,
seedHostSkill, and nativeExecPath to the bottom of this test file (after all
tests) so tests appear first; if any of these helpers are referenced by other
test files, instead extract them into __tests__/helpers.ts and import them from
there, keeping their signatures unchanged so calls in this file still resolve.

19-19: ⚡ Quick win

Reuse the shared preset package source instead of hardcoding the package string.

Keeping this value local can drift from the shared preset package list used by install/uninstall logic.

As per coding guidelines, "Never duplicate constant values across files. Define once, import or re-export with aliases elsewhere."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/application/commands/uninstall.cli.test.ts` at line 19, Replace the
hardcoded PRESET_PACKAGE constant in uninstall.cli.test.ts with the shared
preset package constant used by the install/uninstall logic: remove the local
const PRESET_PACKAGE = "`@alwaysmavs/gpt-image-2`" and import the canonical preset
identifier exported by the shared presets module (the same symbol used by
install/uninstall code) so the test reuses the single source of truth for preset
package names; update references to use that imported symbol (e.g.,
PRESET_PACKAGE or PRESetsMap key) to avoid duplication and drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/application/self-update/lock.ts`:
- Around line 229-238: The loop over versionEntries should skip non-directory
entries and not abort on a stray file; before calling
findActiveMarkerOwner(join(activeDirectory, versionEntry), options) use fs.lstat
or fs.stat to verify the path is a directory and if not continue, and also guard
the call with a try/catch that logs/ignores errors and continues the loop so a
thrown error from findActiveMarkerOwner doesn't break the scan; update the loop
that references versionEntries, activeDirectory, and findActiveMarkerOwner
accordingly.

In `@src/application/self-update/uninstall.test.ts`:
- Line 5: Replace hardcoded Windows path separators in the test assertions with
path helpers: import and use join(...) (already imported) to construct expected
paths (or use path.relative(...) to compare relationships) instead of asserting
raw strings with backslashes; update the assertions in uninstall.test.ts (the
Windows-path expectations around the self-update/uninstall tests) to build
expected paths via join() so they become OS-neutral and resilient.

---

Nitpick comments:
In `@docs/commands.md`:
- Around line 433-440: Reword the uninstall docs to remove internal
implementation details: replace the Windows sentence that mentions
"`~/.local/bin/oo.exe`" and "one-shot PowerShell helper" with a user-facing
statement like "On Windows the executable is removed after the process exits
(cleanup deferred)," and replace the sentence mentioning "active version marker"
with a user-facing statement that the command "refuses to run while another live
`oo` process is running"; do not mention helper scripts, file paths, markers, or
other internal mechanics and keep the description focused on observable CLI
behavior.

In `@docs/commands.zh-CN.md`:
- Around line 371-376: 将文档从描述内部实现细节(例如“一次性 PowerShell helper”、“active version
marker”)改为只描述用户可见行为:在涉及 `~/.local/bin/oo.exe` 的段落,移除对 PowerShell helper
的说明,改写为“Windows
下可执行文件将在进程退出后被删除;其它路径(versions、staging、locks、skills)会在命令执行期间被删除/清理”;在安全段落,移除对
`active version marker` 的内部名词,改写为“当检测到另一个运行中的 oo 进程时,命令会拒绝执行”;保留关于不将 API key
等机密写入 stdout/stderr 的用户可观察承诺。确保只描述可见行为并删除任何实现细节或内部术语。

In `@src/application/commands/uninstall.cli.test.ts`:
- Around line 79-307: Tests repeat the same sandbox lifecycle
(createCliSandbox(), try { ... await sandbox.cleanup(); } finally) — introduce a
local factory helper (e.g., runWithSandbox or withCliSandbox) at the bottom of
this test file that creates the sandbox, yields it to an async callback, and
guarantees cleanup in a finally block; update each test to call this helper and
move usages of createCliSandbox, sandbox.run, seedRuntime, seedHostSkill, and
sandbox.cleanup into the callback so the helper centralizes setup/teardown and
reduces duplication.
- Around line 21-75: Move the file-local test helper functions selfUpdatePaths,
storePaths, seedRuntime, seedHostSkill, and nativeExecPath to the bottom of this
test file (after all tests) so tests appear first; if any of these helpers are
referenced by other test files, instead extract them into __tests__/helpers.ts
and import them from there, keeping their signatures unchanged so calls in this
file still resolve.
- Line 19: Replace the hardcoded PRESET_PACKAGE constant in
uninstall.cli.test.ts with the shared preset package constant used by the
install/uninstall logic: remove the local const PRESET_PACKAGE =
"`@alwaysmavs/gpt-image-2`" and import the canonical preset identifier exported by
the shared presets module (the same symbol used by install/uninstall code) so
the test reuses the single source of truth for preset package names; update
references to use that imported symbol (e.g., PRESET_PACKAGE or PRESetsMap key)
to avoid duplication and drift.
🪄 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: ea4be4b9-9f3e-414c-81c5-7f6eda521181

📥 Commits

Reviewing files that changed from the base of the PR and between 37e9c4d and 54323d5.

⛔ Files ignored due to path filters (2)
  • src/application/bootstrap/__snapshots__/run-cli.test.ts.snap is excluded by !**/*.snap
  • src/application/commands/config/__snapshots__/index.cli.test.ts.snap is excluded by !**/*.snap
📒 Files selected for processing (13)
  • docs/commands.md
  • docs/commands.zh-CN.md
  • src/application/commands/catalog.ts
  • src/application/commands/skills/index.test.ts
  • src/application/commands/skills/install.ts
  • src/application/commands/skills/preset-packages.ts
  • src/application/commands/telemetry-decisions.test.ts
  • src/application/commands/uninstall.cli.test.ts
  • src/application/commands/uninstall.ts
  • src/application/self-update/lock.ts
  • src/application/self-update/uninstall.test.ts
  • src/application/self-update/uninstall.ts
  • src/i18n/catalog.ts

Comment on lines +229 to +238
for (const versionEntry of versionEntries) {
const owner = await findActiveMarkerOwner(
join(activeDirectory, versionEntry),
options,
);

if (owner !== undefined) {
return owner;
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Ignore non-directory entries while scanning active lock versions.

A stray file under locks/active can make findActiveMarkerOwner throw, which aborts the busy-check instead of continuing scan.

💡 Proposed hardening
 for (const versionEntry of versionEntries) {
-    const owner = await findActiveMarkerOwner(
-        join(activeDirectory, versionEntry),
-        options,
-    );
+    let owner: { ownerPid: number } | undefined;
+    try {
+        owner = await findActiveMarkerOwner(
+            join(activeDirectory, versionEntry),
+            options,
+        );
+    }
+    catch (error) {
+        if (isDirectoryReadError(error) || isPathMissingError(error)) {
+            continue;
+        }
+        throw error;
+    }

     if (owner !== undefined) {
         return owner;
     }
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/application/self-update/lock.ts` around lines 229 - 238, The loop over
versionEntries should skip non-directory entries and not abort on a stray file;
before calling findActiveMarkerOwner(join(activeDirectory, versionEntry),
options) use fs.lstat or fs.stat to verify the path is a directory and if not
continue, and also guard the call with a try/catch that logs/ignores errors and
continues the loop so a thrown error from findActiveMarkerOwner doesn't break
the scan; update the loop that references versionEntries, activeDirectory, and
findActiveMarkerOwner accordingly.

Comment thread src/application/self-update/uninstall.test.ts Outdated
On Windows, `oo uninstall --purge` exited non-zero because the running
process holds the SQLite databases under the data directory open, and
Windows refuses to delete files held open by a live process. The data
directory is now handed to the post-exit PowerShell helper alongside the
running executable, so `--purge` completes cleanly; config files, logs,
and telemetry are still removed in-process. The helper directory is now
resolved for every Windows plan so deferred paths have a post-exit home.

This also fixes the Windows CI test suite:
- the CLI test now expects the deferred "scheduled" message and a
  retained executable on Windows, where the running image cannot
  self-delete;
- buildSelfUninstallPlan tests build the exec path with the simulated
  platform's separators so native detection matches on a Windows runner;
- the failedPaths test uses an embedded-NUL path that fs.rm rejects on
  every platform instead of the ENOTDIR trick Windows tolerates.

Docs updated to describe the broadened Windows deferral.

Signed-off-by: Kevin Cui <bh@bugs.cc>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/application/self-update/uninstall.ts (1)

371-386: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Keep the active-owner guard independent from the deletion plan.

Because addRuntimeItems only runs for native installs on Line 136, non-native plans never contain a "locks" item. That makes locksItem undefined here and silently skips the busy-process check, even though uninstall is supposed to refuse while another live oo process still owns an active-version marker. This can let a package-manager install purge shared skills/user data while another oo process is running.

Suggested fix
 export interface UninstallPlan {
+    activeVersionLocksDirectory: string;
     /**
      * Items whose removal must be deferred to a post-exit helper (the Windows
      * running image cannot unlink itself in-process). Membership in this array,
      * not a per-item flag, is what marks an item as deferred.
      */
     deferred: UninstallPlanItem[];
@@
     return {
+        activeVersionLocksDirectory: selfUpdatePaths.locksDirectory,
         deferred,
         helperDirectory,
         immediate,
         installationMethod,
@@
 export async function performSelfUninstall(
     options: PerformUninstallOptions,
 ): Promise<PerformUninstallResult> {
-    const locksItem = options.plan.immediate.find(item => item.category === "locks");
-
-    if (locksItem !== undefined) {
-        const owner = await findAnyActiveVersionOwner({
-            excludeProcessId: options.processId,
-            locksDirectory: locksItem.path,
-            platform: options.plan.platform,
-        });
-
-        if (owner !== undefined) {
-            return {
-                ownerPid: owner.ownerPid,
-                status: "busy",
-            };
-        }
+    const owner = await findAnyActiveVersionOwner({
+        excludeProcessId: options.processId,
+        locksDirectory: options.plan.activeVersionLocksDirectory,
+        platform: options.plan.platform,
+    });
+
+    if (owner !== undefined) {
+        return {
+            ownerPid: owner.ownerPid,
+            status: "busy",
+        };
     }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/application/self-update/uninstall.ts` around lines 371 - 386, The
busy-process check must not depend on options.plan.immediate containing a
"locks" item; update uninstall.ts so the active-owner guard always runs: call
findAnyActiveVersionOwner (using options.processId and a proper locksDirectory
derived independently of options.plan.immediate — e.g. the runtime locks path
used by addRuntimeItems or a default locks path for the platform) and return {
ownerPid, status: "busy" } if it finds an owner. In short, move/replicate the
findAnyActiveVersionOwner invocation out of the locksItem conditional and ensure
you provide a valid locksDirectory when locksItem is undefined.
🧹 Nitpick comments (1)
src/application/self-update/uninstall.ts (1)

173-176: 💤 Low value

Drop the pass-through paths alias.

const paths = args.paths; does not add meaning here; using args.paths directly keeps this helper aligned with the repo rule.

As per coding guidelines, "Don't create variables that pass through a value unchanged. If no transformation occurs, use the original directly."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/application/self-update/uninstall.ts` around lines 173 - 176, Remove the
unnecessary pass-through alias by deleting the line "const paths = args.paths;"
and update any subsequent references to the local variable "paths" to use
"args.paths" directly (this affects the function that receives the parameter
object and any code inside that uses the "paths" identifier). Ensure no other
transformations are needed and that TypeScript types remain satisfied for the
parameter "paths: SelfUpdatePaths".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/application/self-update/uninstall.ts`:
- Around line 371-386: The busy-process check must not depend on
options.plan.immediate containing a "locks" item; update uninstall.ts so the
active-owner guard always runs: call findAnyActiveVersionOwner (using
options.processId and a proper locksDirectory derived independently of
options.plan.immediate — e.g. the runtime locks path used by addRuntimeItems or
a default locks path for the platform) and return { ownerPid, status: "busy" }
if it finds an owner. In short, move/replicate the findAnyActiveVersionOwner
invocation out of the locksItem conditional and ensure you provide a valid
locksDirectory when locksItem is undefined.

---

Nitpick comments:
In `@src/application/self-update/uninstall.ts`:
- Around line 173-176: Remove the unnecessary pass-through alias by deleting the
line "const paths = args.paths;" and update any subsequent references to the
local variable "paths" to use "args.paths" directly (this affects the function
that receives the parameter object and any code inside that uses the "paths"
identifier). Ensure no other transformations are needed and that TypeScript
types remain satisfied for the parameter "paths: SelfUpdatePaths".

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4589e276-f997-434a-b496-ad3f607fae08

📥 Commits

Reviewing files that changed from the base of the PR and between 54323d5 and f5ad873.

📒 Files selected for processing (5)
  • docs/commands.md
  • docs/commands.zh-CN.md
  • src/application/commands/uninstall.cli.test.ts
  • src/application/self-update/uninstall.test.ts
  • src/application/self-update/uninstall.ts
✅ Files skipped from review due to trivial changes (1)
  • docs/commands.zh-CN.md
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/application/self-update/uninstall.test.ts
  • docs/commands.md
  • src/application/commands/uninstall.cli.test.ts

The active-version owner safety check was previously gated on the
uninstall plan containing a "locks" removal item. When the plan
omitted that item, the guard was silently skipped, letting uninstall
proceed while another live `oo` process was still running.

Store the locks directory on the plan so the owner check always runs
regardless of which paths the plan removes. Docs now describe the
user-facing contract only, and tests no longer hardcode preset skill
package names or non-portable Windows paths.

Signed-off-by: Kevin Cui <bh@bugs.cc>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/application/self-update/uninstall.test.ts (1)

44-59: ⚡ Quick win

Move test helper functions to the end of the file.

The helper functions categoriesOf, paths, and writeSkill should be relocated to the end of the file (after the last test, near writeJsonFile at line 477). As per coding guidelines, test-local helper functions belong at the bottom to keep test logic prominent and implementation details secondary.

♻️ Suggested relocation

Move these three functions to appear just before or after writeJsonFile at line 477. The writeJsonFile helper is already correctly placed at the end.

-function categoriesOf(items: readonly UninstallPlanItem[]): string[] {
-    return items.map(item => item.category);
-}
-
-function paths(items: readonly UninstallPlanItem[]): string[] {
-    return items.map(item => item.path);
-}
-
-async function writeSkill(
-    skillDirectory: string,
-    metadataJson: string,
-): Promise<void> {
-    await mkdir(skillDirectory, { recursive: true });
-    await writeFile(join(skillDirectory, "SKILL.md"), "# x\n");
-    await writeFile(resolveManagedSkillMetadataFilePath(skillDirectory), metadataJson);
-}
-
 describe("shouldRemoveManagedSkill", () => {

Then add them at the end before or after writeJsonFile.

As per coding guidelines, **/*.test.{ts,tsx,js,jsx}: If a test helper function might be called by other test files, place it in the __tests__/helpers.ts file. Otherwise, place the function in the test file at the end.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/application/self-update/uninstall.test.ts` around lines 44 - 59, The
helper functions categoriesOf, paths, and writeSkill are declared mid-file and
should be moved to the end of the test file to keep tests first; cut the
functions named categoriesOf, paths, and writeSkill from their current location
and paste them after the other test-local helpers near writeJsonFile (i.e., at
the bottom of the file, either just before or after writeJsonFile), preserving
their implementations and imports so other tests in this file still call them
correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/application/self-update/uninstall.test.ts`:
- Around line 44-59: The helper functions categoriesOf, paths, and writeSkill
are declared mid-file and should be moved to the end of the test file to keep
tests first; cut the functions named categoriesOf, paths, and writeSkill from
their current location and paste them after the other test-local helpers near
writeJsonFile (i.e., at the bottom of the file, either just before or after
writeJsonFile), preserving their implementations and imports so other tests in
this file still call them correctly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 997de1ff-814d-46ea-a361-6c48c2420647

📥 Commits

Reviewing files that changed from the base of the PR and between f5ad873 and ab2440e.

📒 Files selected for processing (5)
  • docs/commands.md
  • docs/commands.zh-CN.md
  • src/application/commands/uninstall.cli.test.ts
  • src/application/self-update/uninstall.test.ts
  • src/application/self-update/uninstall.ts
✅ Files skipped from review due to trivial changes (2)
  • docs/commands.md
  • docs/commands.zh-CN.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/application/commands/uninstall.cli.test.ts
  • src/application/self-update/uninstall.ts

@BlackHole1 BlackHole1 merged commit 5c00ed6 into main May 29, 2026
6 checks passed
@BlackHole1 BlackHole1 deleted the support-uninstall branch May 29, 2026 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant