Skip to content

feat(cli): add generic user-level install target#585

Open
betterlmy wants to merge 2 commits into
iflytek:mainfrom
betterlmy:agent/generic-user-install-target
Open

feat(cli): add generic user-level install target#585
betterlmy wants to merge 2 commits into
iflytek:mainfrom
betterlmy:agent/generic-user-install-target

Conversation

@betterlmy

Copy link
Copy Markdown

Summary

  • Always include a generic target in the interactive user-scope install selector.
  • Map the generic target to the shared user-level Agent Skills directory at ~/.agents/skills.
  • Allow the generic target to be selected independently or together with detected agent-specific directories such as ~/.codex/skills and ~/.claude/skills.
  • Keep project-scope, explicit --agent, custom --dir, JSON, and non-interactive behavior unchanged.
  • Add a unit test covering the generic target alongside an existing detected user-level Agent target.

This builds on the user/project installation flow introduced in #440 and the interactive target selection behavior refined in #534.

Previously, when one or more user-level Agent directories already existed, the interactive selector only displayed those detected directories. Users who wanted the shared ~/.agents/skills directory had to cancel the prompt and run the command again with an explicit --dir argument. Making generic available in the selector keeps that workflow discoverable and avoids requiring a manual path override.

Validation

  • Backend tests passed or were not applicable — no backend changes.
  • Frontend typecheck/build passed or were not applicable — no frontend changes.
  • OpenAPI SDK regenerated or checked when API contracts changed — no API contract changes.
  • Smoke test run when relevant — not applicable to this CLI resolver-only change.

Commands run:

cd cli
bun test
bun run typecheck
bun run lint
bun run build

Results:

  • 337 CLI tests passed with 0 failures.
  • TypeScript type checking passed.
  • ESLint passed.
  • The Node-targeted CLI bundle built successfully.

Risk

  • User-facing impact: Interactive installs using the user scope gain one additional optional target, generic (~/.agents/skills). Existing detected Agent targets remain available and retain their current behavior.
  • Deployment or migration impact: None. The change takes effect only after a new CLI version is published and requires no data, configuration, or storage migration.
  • Rollback approach: Revert this change. Existing installations and inventory records require no cleanup or migration.

Notes

  • Related issue: No directly matching issue was found.
  • Related PRs:
    • #440 introduced the interactive user/project scope selection and the ~/.agents/skills fallback.
    • #534 improved the same interactive install-target selector and added its existing unit test.
  • Follow-up work: Update the CLI guide to state explicitly that the generic user target is always offered in interactive mode.
  • Docs or operator runbooks updated when behavior changed: Not included in this PR. The existing CLI documentation already identifies ~/.agents/skills as the user-level fallback path, but it does not yet describe this selector behavior.

@CLAassistant

CLAassistant commented Jul 17, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Signed-off-by: betterlmy <betterlmy@icloud.com>
@betterlmy
betterlmy force-pushed the agent/generic-user-install-target branch from 61a1155 to 8b84201 Compare July 17, 2026 03:39
@betterlmy

Copy link
Copy Markdown
Author

@FenjuFu @XiaoSeS 麻烦大佬们有空帮我审核一下,提这个PR的原因是我们内部使用了一些非主流的Agent,比如pi.dev,他们都是读取默认~/.agents/skills/目录下的skill,现在通过cli安装需要再增加一步mv的操作 非常不便 ,因此催促一下各位,望见谅~

@dongmucat

Copy link
Copy Markdown
Collaborator

Code review summary

Reviewed head: 8b8420151626b773f33987eb9f092281da396ec5

Verdict: not merge-ready yet. The main path is sound, but there is one reproducible correctness blocker.

Blocking finding — canonical target aliases can partially install and then fail

resolveScopedTargets appends the generic user target, while dedupeByRoot only compares path strings. Two different strings may therefore point to the same physical directory.

Reproduction:

  1. Configure ~/.codex/skills -> ~/.agents/skills as a symlink.
  2. Run an interactive user-scope install and select both codex and generic.
  3. Both candidates resolve to the same canonical directory.
  4. The first target installs successfully and updates inventory; the second reaches installSkill and exits with code 4 (skill already installed).

The command reports failure after mutating the filesystem and inventory, leaving a partial-success state. I reproduced this with real symlinked directories.

Suggested fix:

  • Canonicalize existing target roots with realpath before deduplication.
  • Preflight all resolved install targets before writing, so a later collision cannot leave partial state.
  • Add a symlink-alias regression test.

Non-blocking follow-ups

  • Documentation is stale: cli/README.md, docs/skillhub/guide/cli.md, and docs/skillhub/en/guide/cli.md still say generic is used only when no Agent directory is detected. The new behavior always offers generic for interactive user scope.
  • The new interactive test verifies selecting generic via the highlighted empty-selection fallback, but does not verify selecting codex + generic together. Please add an explicit multi-select assertion.

Verification

  • Resolver-focused tests: 19 passed, 0 failed.
  • Full CLI suite: 337 passed, 0 failed.
  • TypeScript typecheck, ESLint, CLI bundle build, and git diff --check passed.
  • GitHub CLI checks passed on Ubuntu, macOS, and Windows.

Once canonical target deduplication is fixed and covered, this should be ready for re-review.

@dongmucat

Copy link
Copy Markdown
Collaborator

Additional note: the CLI already supports a fully custom installation path through --dir <path>.

For example:

skillhub install <slug> --dir ~/.agents/skills

--dir is mutually exclusive with --scope and --agent. This remains the explicit way to choose a custom or unsupported Agent directory; this PR mainly improves discoverability of the generic user target in the interactive selector. Please include this distinction when updating the CLI documentation.

Signed-off-by: betterlmy <betterlmy@icloud.com>
@betterlmy

Copy link
Copy Markdown
Author

Additional note: the CLI already supports a fully custom installation path through --dir <path>.

For example:

skillhub install <slug> --dir ~/.agents/skills

--dir is mutually exclusive with --scope and --agent. This remains the explicit way to choose a custom or unsupported Agent directory; this PR mainly improves discoverability of the generic user target in the interactive selector. Please include this distinction when updating the CLI documentation.

Yes, my current recommendation internally is to use the --dir flag to install to a specified directory. However, most people don't really follow this advice and still opt for the interactive installation instead.

@betterlmy

Copy link
Copy Markdown
Author

Code review summary

Reviewed head: 8b8420151626b773f33987eb9f092281da396ec5

Verdict: not merge-ready yet. The main path is sound, but there is one reproducible correctness blocker.

Blocking finding — canonical target aliases can partially install and then fail

resolveScopedTargets appends the generic user target, while dedupeByRoot only compares path strings. Two different strings may therefore point to the same physical directory.

Reproduction:

  1. Configure ~/.codex/skills -> ~/.agents/skills as a symlink.
  2. Run an interactive user-scope install and select both codex and generic.
  3. Both candidates resolve to the same canonical directory.
  4. The first target installs successfully and updates inventory; the second reaches installSkill and exits with code 4 (skill already installed).

The command reports failure after mutating the filesystem and inventory, leaving a partial-success state. I reproduced this with real symlinked directories.

Suggested fix:

  • Canonicalize existing target roots with realpath before deduplication.
  • Preflight all resolved install targets before writing, so a later collision cannot leave partial state.
  • Add a symlink-alias regression test.

Non-blocking follow-ups

  • Documentation is stale: cli/README.md, docs/skillhub/guide/cli.md, and docs/skillhub/en/guide/cli.md still say generic is used only when no Agent directory is detected. The new behavior always offers generic for interactive user scope.
  • The new interactive test verifies selecting generic via the highlighted empty-selection fallback, but does not verify selecting codex + generic together. Please add an explicit multi-select assertion.

Verification

  • Resolver-focused tests: 19 passed, 0 failed.
  • Full CLI suite: 337 passed, 0 failed.
  • TypeScript typecheck, ESLint, CLI bundle build, and git diff --check passed.
  • GitHub CLI checks passed on Ubuntu, macOS, and Windows.

Once canonical target deduplication is fixed and covered, this should be ready for re-review.

Thanks for the review. Addressed in f519b08:

  • Canonicalize existing target roots with realpath before deduplication.
  • Preflight all resolved targets before filesystem or inventory writes.
  • Added symlink-alias, later-target collision, and codex + generic multi-select regression tests.
  • Updated all three CLI docs, including the distinction between the interactive generic target and
    explicit --dir usage.

Local verification: 340 tests passed; typecheck, ESLint, build, and git diff --check passed.

@FenjuFu FenjuFu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM — the generic target is correctly gated to interactive user scope, realpath-based dedupe handles symlinked skill dirs, and the resolver/install-service tests cover both the standalone and combined selection paths.

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.

4 participants