Skip to content

fix(compat): prioritize PUBLIC/GLOBAL skills in legacy slug lookup - #750

Merged
XiaoSeS merged 2 commits into
iflytek:mainfrom
myml:fix/compat-public-skills-priority
Aug 25, 2026
Merged

XiaoSeS merged 2 commits into
iflytek:mainfrom
myml:fix/compat-public-skills-priority

Conversation

@myml

@myml myml commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

What

Change findByLegacySlug to prefer PUBLIC visibility and GLOBAL namespace skills when multiple skills share the same slug.

Why

When multiple skills share the same slug across namespaces, the previous implementation just returned the first match, which could be a private or namespace-only skill. For CLI users using plain slugs (without @namespace/ prefix), this could lead to unexpected "skill not found" errors or access to the wrong skill.

The new priority order ensures:

  1. PUBLIC skills are preferred over PRIVATE/NAMESPACE_ONLY
  2. GLOBAL namespace skills are preferred over team namespaces
  3. Skills with published versions are preferred over drafts

How

Add a scoring system to findByLegacySlug:

  • PUBLIC visibility: +100 points
  • GLOBAL namespace: +50 points
  • Has latest version: +10 points

The skill with the highest score is selected.

Testing

  • Create multiple skills with the same slug in different namespaces
  • Verify that PUBLIC/GLOBAL skills are returned first
  • Verify backward compatibility for single-slug scenarios

Impact

  • CLI users will now resolve to the most accessible skill by default
  • No breaking changes - only improves the selection logic when multiple candidates exist

@myml
myml force-pushed the fix/compat-public-skills-priority branch from 98a4505 to d8ebf15 Compare August 24, 2026 09:03
@FenjuFu

FenjuFu commented Aug 24, 2026

Copy link
Copy Markdown
Member

Prioritizing PUBLIC + GLOBAL for bare-slug (no @namespace/) CLI resolution is a sensible disambiguation. Two things:

  1. The score is recomputed inside the comparator, so namespaceRepository.findById runs many times per skill. Comparator.comparingInt(keyExtractor) may invoke the extractor more than once per element during min(), and each call hits the DB for the namespace — an N+1 amplified by the sort. Precompute the score once per skill (e.g. map each Skill to an int in a single pass, or batch-load the namespaces) and then pick the min over the precomputed values.
  2. This changes which skill a bare slug resolves to for existing collisions (previously first-match). That's the intended fix, but worth calling out to the maintainer since it's a behavior change for any CLI user currently relying on the old resolution. A tie among equal-score candidates still falls back to findBySlug order — fine, just noting it's nondeterministic if the query isn't ordered.

@myml
myml force-pushed the fix/compat-public-skills-priority branch from 4767b6d to 701b050 Compare August 25, 2026 02:14
When multiple skills share the same slug across namespaces,
findByLegacySlug now prefers PUBLIC visibility and GLOBAL
namespace over NAMESPACE_ONLY/PRIVATE ones, so plain slug
lookups resolve to the most accessible skill. Namespaces are
batch-fetched via findByIdIn to avoid N+1 database queries.

Signed-off-by: wurongjie <wurongjie@uniontech.com>
@myml
myml force-pushed the fix/compat-public-skills-priority branch from 701b050 to 32d5de7 Compare August 25, 2026 02:27
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
@XiaoSeS
XiaoSeS merged commit 5a95278 into iflytek:main Aug 25, 2026
10 checks passed
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.

3 participants