fix(skills): handle reinstall locked folders

[zerob13]

Summary

• Treat same-name skill folders without SKILL.md as stale residues during overwrite reinstall, removing them before copying the new skill instead of creating a .backup-* folder.
• Preserve timestamped backup behavior for valid existing skills and map locked-folder filesystem failures to errorCode: 'target_locked' with targetPath.
• Move uninstall cache/event updates after successful folder removal and avoid using the skill root as the fallback cwd for skill scripts when a session directory can be created.
• Add SDD issue docs and focused presenter/execution-service regression coverage.

UI State

Before:

Import same skill after delete
└─ overwrite
   └─ raw EPERM rename ... .backup-* error

After:

Import same skill after delete
├─ stale folder without SKILL.md -> reinstall normally
└─ locked folder -> target_locked result with targetPath

Validation

• pnpm run format
• pnpm run i18n
• pnpm run lint
• pnpm run typecheck
• pnpm test -- test/main/presenter/skillPresenter/skillPresenter.test.ts test/main/presenter/skillPresenter/skillExecutionService.test.ts

Closes #1803

Summary by CodeRabbit

• New Features

  • Skill installs now recognize and surface a clearer “target locked” state when Windows blocks replacement.
  • Skill scripts now run using a session-based working directory when the conversation workspace is unavailable.

• Bug Fixes

  • Uninstall now reports success only after the skill folder is confirmed removed.
  • Replacement logic preserves the previous valid-backup behavior while correctly handling stale residue and missing SKILL.md.

• Tests

  • Expanded coverage for locked-folder, stale overwrite, cwd fallback, and uninstall success/failure ordering.

• Documentation

  • Added detailed EPERM/EBUSY handling plans and acceptance criteria.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5d2a12f2-a1ce-4cf3-b2bb-a4b84fedc3eb

📥 Commits

Reviewing files that changed from the base of the PR and between 382f514 and c5bf210.

📒 Files selected for processing (5)

• docs/issues/skill-reinstall-windows-eperm/plan.md
• docs/issues/skill-reinstall-windows-eperm/spec.md
• docs/issues/skill-reinstall-windows-eperm/tasks.md
• src/main/presenter/skillPresenter/index.ts
• test/main/presenter/skillPresenter/skillPresenter.test.ts

✅ Files skipped from review due to trivial changes (2)

• docs/issues/skill-reinstall-windows-eperm/spec.md
• docs/issues/skill-reinstall-windows-eperm/tasks.md

🚧 Files skipped from review as they are similar to previous changes (2)

• test/main/presenter/skillPresenter/skillPresenter.test.ts
• src/main/presenter/skillPresenter/index.ts

---

📝 Walkthrough

Walkthrough

Adds structured target_locked handling for skill install and uninstall on Windows, changes overwrite behavior for stale residue, adds session-directory fallback for execution cwd, and updates the install dialog to use typed conflict results.

Changes

Skill Reinstall Windows EPERM

Layer / File(s)	Summary
Spec, plan, and task docs docs/issues/skill-reinstall-windows-eperm/spec.md, docs/issues/skill-reinstall-windows-eperm/plan.md, docs/issues/skill-reinstall-windows-eperm/tasks.md	Adds the spec, implementation plan, and completed task list for the EPERM reinstall work.
SkillInstallResult type extension src/shared/types/skill.ts	Extends SkillInstallResult.errorCode with 'target_locked'.
Install error helpers and stale-residue detection src/main/presenter/skillPresenter/index.ts	Adds overwrite preparation helpers and maps filesystem lock/permission failures to structured install results.
Uninstall error handling and cache ordering src/main/presenter/skillPresenter/index.ts	Updates uninstall to return typed missing-folder and locked-folder results, and to continue sidecar/cache cleanup with the new ordering and error handling.
Execution cwd session-directory fallback src/main/presenter/skillPresenter/skillExecutionService.ts	Routes missing or invalid conversation workdirs through a session-directory fallback before using skillRoot.
SkillInstallDialog typed conflict detection src/renderer/settings/components/skills/SkillInstallDialog.vue	Switches install-result handling to SkillInstallResult and errorCode-based conflict detection.
Presenter and execution-service tests test/main/presenter/skillPresenter/skillPresenter.test.ts, test/main/presenter/skillPresenter/skillExecutionService.test.ts	Adds coverage for stale-residue overwrite, locked-folder failures, uninstall cleanup behavior, session cwd creation, and cwd fallback on mkdir failure.

Sequence Diagram(s)

sequenceDiagram
  participant UI as SkillInstallDialog
  participant Presenter as SkillPresenter
  participant FS as FileSystem

  UI->>Presenter: installFromFolder(..., overwrite=true)
  Presenter->>FS: existsSync(targetDir)
  Presenter->>FS: existsSync(SKILL.md)
  alt SKILL.md present
    Presenter->>FS: renameSync(targetDir, backupDir)
    FS-->>Presenter: ok or EPERM/EBUSY
    Presenter-->>UI: success or target_locked
  else SKILL.md missing
    Presenter->>FS: rmSync(targetDir, { recursive: true })
    Presenter-->>UI: success
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• ThinkInAIXYZ/deepchat#1380: Modifies the same resolveExecutionCwd path and conversation/session working-directory behavior in skillExecutionService.ts.

Poem

🐇 A folder got stuck with a Windows sigh,
But now we can spot when the locks won’t die.
Stale roots get cleared, and backups stay true,
Session dirs bloom when scripts need a cue.
target_locked hops out, neat and clear,
And this rabbit leaves a happy ear.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and matches the main change: handling locked folders during skill reinstall.
Linked Issues check	✅ Passed	The changes address #1803 by clearing stale residues, preserving valid overwrite backups, and returning structured target_locked failures on lock errors.
Out of Scope Changes check	✅ Passed	The diff stays focused on reinstall, uninstall cleanup, cwd fallback, types, tests, and docs needed for #1803.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/investigate-skill-reload-eperm

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 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/main/presenter/skillPresenter/index.ts`:
- Around line 1555-1557: Keep the local uninstall cleanup path in sync with the
not_found branch in `presenter/skillPresenter/index.ts`: when
`fs.existsSync(skillDir)` fails inside the skill uninstall flow, do not return
immediately before running the same cleanup used for a normal uninstall. Make
sure `deleteSkillExtension()` and the related cache eviction still run for this
case, then return the not_found result afterward so removed-on-disk skills don’t
leave stale extension state behind.

🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 187c8b05-535c-457b-9a06-850b82e8cf46

📥 Commits

Reviewing files that changed from the base of the PR and between df64127 and 382f514.

📒 Files selected for processing (9)

• docs/issues/skill-reinstall-windows-eperm/plan.md
• docs/issues/skill-reinstall-windows-eperm/spec.md
• docs/issues/skill-reinstall-windows-eperm/tasks.md
• src/main/presenter/skillPresenter/index.ts
• src/main/presenter/skillPresenter/skillExecutionService.ts
• src/renderer/settings/components/skills/SkillInstallDialog.vue
• src/shared/types/skill.ts
• test/main/presenter/skillPresenter/skillExecutionService.test.ts
• test/main/presenter/skillPresenter/skillPresenter.test.ts