Skip to content

fix(skills): drop misleading path from update not-installed error#228

Merged
BlackHole1 merged 1 commit into
mainfrom
imporve-skills-update-error
May 23, 2026
Merged

fix(skills): drop misleading path from update not-installed error#228
BlackHole1 merged 1 commit into
mainfrom
imporve-skills-update-error

Conversation

@BlackHole1
Copy link
Copy Markdown
Member

The previous not-installed branch shared a throw with the unmanaged- target branch and filled in targetStates[0].installedSkillDirectoryPath as the displayed path. When the skill was absent from every host, that path was just an arbitrary host pick, making the error look as if update only checked one specific agent.

Split the branch so the unmanaged case keeps its meaningful path, and introduce errors.skills.update.notInstalled without a host path for the truly not-installed case.

The previous not-installed branch shared a throw with the unmanaged-
target branch and filled in targetStates[0].installedSkillDirectoryPath
as the displayed path. When the skill was absent from every host, that
path was just an arbitrary host pick, making the error look as if
update only checked one specific agent.

Split the branch so the unmanaged case keeps its meaningful path, and
introduce errors.skills.update.notInstalled without a host path for
the truly not-installed case.

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

coderabbitai Bot commented May 23, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6fe184a7-f51d-4cc6-88f8-57690b0a7c22

📥 Commits

Reviewing files that changed from the base of the PR and between fce2d2d and eb9c01f.

📒 Files selected for processing (3)
  • src/application/commands/skills/update.test.ts
  • src/application/commands/skills/update.ts
  • src/i18n/catalog.ts

Summary by CodeRabbit

  • Bug Fixes

    • Refined error handling in the skills update command to distinguish between skills that are not installed and those that are not managed, with specific error messages for each scenario.
  • Internationalization

    • Expanded language support by adding Chinese translations for skill update error messages.

Walkthrough

This PR refines error messaging in the skills update command by distinguishing two failure scenarios: when a skill exists locally but lacks managed metadata (notManaged), and when it is not installed anywhere (notInstalled). The implementation adds a new i18n key for the latter case, refactors error branching in resolveSelectedManagedSkills to throw distinct errors with appropriate diagnostic details, and adds a test case verifying that the generic not-installed error hides host directory paths from the user.

Possibly related PRs

  • oomol-lab/oo-cli#110: Also modifies skills update to treat non-managed/missing managed metadata differently in error handling and i18n behavior.
  • oomol-lab/oo-cli#168: Refactors skills/update.ts usage patterns alongside API changes in managed-skill publication modules.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title follows the required format '(): ' with 'fix' as type and 'skills' as scope, clearly describing the main change of removing a misleading path from an error message.
Description check ✅ Passed The description is directly related to the changeset, explaining the rationale for splitting error branches and introducing a new error message without a host path.
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 imporve-skills-update-error

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

@BlackHole1 BlackHole1 merged commit 9419ad1 into main May 23, 2026
6 checks passed
@BlackHole1 BlackHole1 deleted the imporve-skills-update-error branch May 23, 2026 06:44
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