scriptoriumadmin/skillhub
1
0
Fork 0
mirror of https://github.com/iflytek/skillhub.git synced 2026-09-11 22:51:04 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

fix(suite): address final review findings

Browse source 
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
This commit is contained in:
XiaoSeS 2026-09-09 18:20:39 +08:00
parent ea1941e436
commit bf1b293e1f
11 changed files with 302 additions and 2 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
cli/src/index.ts
View file

@ -255,7 +255,7 @@ cli
.option('--scope <scope>', 'Install scope: user or project')
.option('--agent <profile>', 'Agent profile (repeatable)')
.option('--dir <path>', 'Install directory')
.option('--force', 'Replace same-source member versions')
.option('--force', 'Replace same-source member versions or local changes')
.option('--check', 'Show an upgrade plan without writing')
.option('--registry <url>', 'Registry URL')
.option('--token <token>', 'API token')

8
cli/src/services/suite-service.ts
View file

@ -460,6 +460,7 @@ async function removeSuiteTransaction(options: SuiteRemoveOptions): Promise<Suit
current.suites = installedSuites(current).filter(candidate =>
candidate.registry !== options.registry || candidate.namespace !== options.namespace || candidate.slug !== options.slug)
for (const item of current.items) {
if (item.registry !== options.registry) continue
const deletedDirs = new Set(removable
.filter(candidate => candidate.item.registry === item.registry &&
candidate.item.namespace === item.namespace && candidate.item.slug === item.slug)
@ -658,6 +659,13 @@ async function preflightExistingTargets(
}
const currentSuitePrefix = `suite:@${plan.namespace}/${plan.slug}@`
const ownerTarget = owner?.targets.find(existing => resolve(existing.installDir) === installDir)
if (owner && ownerTarget && !force && await pathExists(installDir)
&& (await snapshotSkillDirectory(installDir)).fingerprint !== owner.fingerprint) {
throw new CliError(`local changes detected at ${installDir}`, EXIT.validation, {
path: installDir,
next: 'pass --force only if replacing these local changes is intended'
})
}
if (owner && ownerTarget && owner.version !== member.version && targetInstalledBy(owner, ownerTarget).some(source =>
source.startsWith('suite:') && !source.startsWith(currentSuitePrefix))) {
throw new CliError(`shared Suite member @${member.namespace}/${member.slug} cannot change version in place`, EXIT.validation, {

81
cli/test/unit/services/suite-service.test.ts
View file

@ -677,6 +677,58 @@ describe('Suite local lifecycle', () => {
expect(inventory.items[0].installedBy).toEqual(['suite:@global/editor-pack@1.0.0'])
})
test('removing a Suite does not rewrite the same coordinate from another registry', async () => {
const home = await mkdtemp(join(tmpdir(), 'skillhub-suite-home-'))
const firstRoot = await mkdtemp(join(tmpdir(), 'skillhub-suite-registry-a-'))
const secondRoot = await mkdtemp(join(tmpdir(), 'skillhub-suite-registry-b-'))
const secondRegistry = 'http://registry-b.test'
const { plan, downloads } = makePlan()
plan.members = [plan.members[0]!]
await installSuite({
registry,
namespace: 'global',
slug: 'starter-pack',
targets: [{ agent: 'codex', rootDir: firstRoot, scope: 'project', source: 'explicit' }],
force: false,
home,
client: clientFor(plan, downloads)
})
await installSuite({
registry: secondRegistry,
namespace: 'global',
slug: 'starter-pack',
targets: [{ agent: 'claude', rootDir: secondRoot, scope: 'project', source: 'explicit' }],
force: false,
home,
client: clientFor(plan, downloads)
})
await removeSuite({ registry, namespace: 'global', slug: 'starter-pack', home })
const afterFirstRemoval = await new InventoryStore(home).read()
expect(afterFirstRemoval.suites).toEqual([
expect.objectContaining({ registry: secondRegistry, slug: 'starter-pack' })
])
expect(afterFirstRemoval.items).toEqual([
expect.objectContaining({
registry: secondRegistry,
slug: 'alpha',
installedBy: ['suite:@global/starter-pack@1.0.0']
})
])
expect(await readFile(join(secondRoot, 'alpha', 'SKILL.md'), 'utf8')).toBe('# Alpha')
const secondRemoval = await removeSuite({
registry: secondRegistry,
namespace: 'global',
slug: 'starter-pack',
home
})
expect(secondRemoval.removed).toEqual([join(secondRoot, 'alpha')])
expect(await exists(join(secondRoot, 'alpha'))).toBe(false)
})
test('reuses a matching legacy direct install and preserves it when Suite is removed', async () => {
const home = await mkdtemp(join(tmpdir(), 'skillhub-suite-home-'))
const rootDir = await mkdtemp(join(tmpdir(), 'skillhub-suite-root-'))
@ -716,6 +768,35 @@ describe('Suite local lifecycle', () => {
expect(inventory.items[0].installedBy).toEqual(['direct'])
})
test('requires force before replacing a locally modified same-version member', async () => {
const home = await mkdtemp(join(tmpdir(), 'skillhub-suite-home-'))
const rootDir = await mkdtemp(join(tmpdir(), 'skillhub-suite-root-'))
const { plan, downloads } = makePlan()
plan.members = [plan.members[0]!]
const member = plan.members[0]!
const skillDir = join(rootDir, member.slug)
await mkdir(skillDir, { recursive: true })
await writeFile(join(skillDir, 'SKILL.md'), '# Alpha')
const store = new InventoryStore(home)
await store.upsertTarget(registry, member.namespace, member.slug, member.version, {
agent: 'codex', rootDir, installDir: skillDir, installedAt: new Date().toISOString()
}, member.fingerprint)
await writeFile(join(skillDir, 'SKILL.md'), '# Locally modified Alpha')
await expect(installSuite({
registry,
namespace: 'global',
slug: 'starter-pack',
targets: [{ agent: 'codex', rootDir, scope: 'project', source: 'explicit' }],
force: false,
home,
client: clientFor(plan, downloads)
})).rejects.toThrow('local changes')
expect(await readFile(join(skillDir, 'SKILL.md'), 'utf8')).toBe('# Locally modified Alpha')
expect((await store.read()).suites ?? []).toEqual([])
})
test('tracks direct and Suite ownership independently for each Agent target', async () => {
const home = await mkdtemp(join(tmpdir(), 'skillhub-suite-home-'))
const directRoot = await mkdtemp(join(tmpdir(), 'skillhub-suite-direct-root-'))

22
openspec/changes/add-skill-suites/specs/skill-suites/spec.md
View file

@ -278,6 +278,12 @@ CLI SHALL 在修改目标目录前完成全部成员和全部 Agent 目标的解
- **THEN** CLI 通过 Suite 级本地锁只允许一个操作进入事务
- **AND** 另一个操作明确报告繁忙，不得基于旧 inventory 提交
#### Scenario: Existing Member has local changes
- **WHEN** Suite 安装将复用或替换一个已登记但 fingerprint 已变化的 Member 目录
- **AND** 用户未明确传入 `--force`
- **THEN** CLI 在写入任何目标或 inventory 前拒绝安装
- **AND** 保留本地文件和现有 inventory
### Requirement: Suite installation SHALL preserve Agent Skills compatibility
CLI SHALL 将每个 Member 作为普通 Skill 安装到 Agent 已支持的 Skill 根目录。CLI SHALL NOT 为 Suite 创建同名 `SKILL.md` 或要求 Agent 理解 Suite 协议。
@ -307,6 +313,11 @@ CLI inventory SHALL 记录已安装 SuiteVersion、精确成员快照，以及
- **THEN** CLI 将缺失的 Suite 和来源集合按空值处理
- **AND** 已安装 Skill 记录和目标路径保持不变
#### Scenario: Same Suite coordinate is installed from different registries
- **WHEN** 两个 Registry 各自安装了相同 Namespace 和 slug 的 Suite
- **THEN** inventory 按 Registry 分别记录 Suite 与 Member 来源
- **AND** 移除其中一个 Registry 的 Suite 不得修改另一个 Registry 的来源或文件
### Requirement: Suite removal SHALL be ownership-safe
`skillhub suite remove` SHALL 仅移除该 Suite 的来源记录。CLI SHALL 只自动删除不再被直接安装、未被其他 Suite 引用且未被本地修改的 Member 目录。
@ -437,6 +448,11 @@ Suite 创建、编辑、提交、审核、发布、下架、隐藏、恢复、
- **THEN** 用户可以查看 Suite 公开元数据
- **AND** 只有全部 Member 仍公开且可安装时才能获得完整安装计划
#### Scenario: Anonymous user accesses a Suite in an archived Namespace
- **WHEN** Namespace 已归档且匿名用户访问其中的 PUBLISHED PUBLIC SuiteVersion
- **THEN** 系统拒绝查看和安装
- **AND** Namespace 成员和平台管理员仍按现有归档 Namespace 规则访问
#### Scenario: Namespace member accesses a namespace Suite
- **WHEN** 当前 Namespace MEMBER 访问 PUBLISHED NAMESPACE_ONLY SuiteVersion
- **THEN** 用户可以查看并在全部 Member 校验通过后安装
@ -465,6 +481,12 @@ REJECTED SuiteVersion MAY 由有权限的管理者退回 DRAFT、修改并重新
- **THEN** 系统拒绝修改
- **AND** 提示创建新的 SuiteVersion
#### Scenario: A stale draft edit races with publication
- **WHEN** 一个请求读取 DRAFT 后，另一事务先将同一 SuiteVersion 发布
- **AND** 旧请求随后尝试保存编辑结果
- **THEN** 系统拒绝旧请求的并发更新
- **AND** 已发布状态、发布时间和发布内容保持不变
### Requirement: Suite plan and Member download metrics SHALL remain attributable and idempotent
客户端 SHALL 为一次安装计划生成独立的 idempotency key，并在安全重试时复用；服务端 SHALL 按调用者隔离该 key，并生成 operation ID 关联该计划的审计记录。服务端成功签发完整计划后 SHALL 记录一次 Suite 安装请求，但 SHALL NOT 在此时预增 Member 下载数。每个 Member 继续通过现有 Skill 下载接口按实际下载请求计数，避免计划签发与文件下载对同一 Member 重复计数。这些指标表示服务端计划签发和实际下载请求，不表示 CLI 本地安装成功。

3
server/skillhub-app/src/main/resources/db/migration/V53__skill_suite_version_optimistic_lock.sql Normal file
View file

@ -0,0 +1,3 @@
-- Reject stale draft edits after another transaction publishes or otherwise changes the snapshot.
ALTER TABLE skill_suite_version
ADD COLUMN lock_version BIGINT NOT NULL DEFAULT 0;

146
server/skillhub-app/src/test/java/com/iflytek/skillhub/repository/SkillSuiteVersionOptimisticLockingTest.java Normal file
View file

@ -0,0 +1,146 @@
package com.iflytek.skillhub.repository;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import com.iflytek.skillhub.domain.namespace.Namespace;
import com.iflytek.skillhub.domain.suite.SkillSuite;
import com.iflytek.skillhub.domain.suite.SkillSuiteVersion;
import com.iflytek.skillhub.domain.suite.SkillSuiteVersionStatus;
import com.iflytek.skillhub.domain.skill.SkillVisibility;
import jakarta.persistence.EntityManager;
import jakarta.persistence.EntityManagerFactory;
import jakarta.persistence.OptimisticLockException;
import java.time.Instant;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.autoconfigure.jdbc.AutoConfigureTestDatabase;
import org.springframework.boot.test.autoconfigure.orm.jpa.DataJpaTest;
import org.springframework.test.context.ActiveProfiles;
import org.springframework.test.context.DynamicPropertyRegistry;
import org.springframework.test.context.DynamicPropertySource;
import org.springframework.transaction.annotation.Propagation;
import org.springframework.transaction.annotation.Transactional;
import org.testcontainers.containers.PostgreSQLContainer;
import org.testcontainers.junit.jupiter.Container;
import org.testcontainers.junit.jupiter.Testcontainers;
@DataJpaTest
@AutoConfigureTestDatabase(replace = AutoConfigureTestDatabase.Replace.NONE)
@ActiveProfiles("test")
@Testcontainers
class SkillSuiteVersionOptimisticLockingTest {
@Container
private static final PostgreSQLContainer<?> POSTGRES =
new PostgreSQLContainer<>("postgres:16-alpine");
@DynamicPropertySource
static void configurePostgres(DynamicPropertyRegistry registry) {
registry.add("spring.datasource.url", POSTGRES::getJdbcUrl);
registry.add("spring.datasource.username", POSTGRES::getUsername);
registry.add("spring.datasource.password", POSTGRES::getPassword);
registry.add("spring.datasource.driver-class-name", () -> "org.postgresql.Driver");
registry.add("spring.jpa.database-platform", () -> "org.hibernate.dialect.PostgreSQLDialect");
}
@Autowired
private EntityManagerFactory entityManagerFactory;
@Test
@Transactional(propagation = Propagation.NOT_SUPPORTED)
void publishingRejectsAConcurrentStaleDraftUpdate() {
PersistedSuite persisted = persistDraft();
EntityManager publisher = entityManagerFactory.createEntityManager();
EntityManager staleEditor = entityManagerFactory.createEntityManager();
try {
publisher.getTransaction().begin();
staleEditor.getTransaction().begin();
SkillSuiteVersion published = publisher.find(SkillSuiteVersion.class, persisted.versionId());
SkillSuiteVersion stale = staleEditor.find(SkillSuiteVersion.class, persisted.versionId());
published.setStatus(SkillSuiteVersionStatus.PUBLISHED);
published.setPublishedAt(Instant.parse("2026-09-09T08:00:00Z"));
publisher.getTransaction().commit();
stale.setDisplayName("Edited after publish");
assertThatThrownBy(staleEditor.getTransaction()::commit)
.satisfies(error -> assertThat(hasCause(error, OptimisticLockException.class)).isTrue());
EntityManager verifier = entityManagerFactory.createEntityManager();
try {
SkillSuiteVersion saved = verifier.find(SkillSuiteVersion.class, persisted.versionId());
assertThat(saved.getStatus()).isEqualTo(SkillSuiteVersionStatus.PUBLISHED);
assertThat(saved.getDisplayName()).isEqualTo("Original draft");
assertThat(saved.getPublishedAt()).isNotNull();
} finally {
verifier.close();
}
} finally {
rollbackIfActive(publisher);
rollbackIfActive(staleEditor);
publisher.close();
staleEditor.close();
deleteSuite(persisted);
}
}
private PersistedSuite persistDraft() {
EntityManager entityManager = entityManagerFactory.createEntityManager();
try {
entityManager.getTransaction().begin();
Namespace namespace = new Namespace("suite-locking", "Suite locking", "owner");
entityManager.persist(namespace);
SkillSuite suite = new SkillSuite(namespace.getId(), "starter", "Starter", "owner");
entityManager.persist(suite);
SkillSuiteVersion version = new SkillSuiteVersion(
suite.getId(), "1.0.0", "Original draft", "Summary", SkillVisibility.PUBLIC, "owner");
entityManager.persist(version);
entityManager.getTransaction().commit();
return new PersistedSuite(namespace.getId(), suite.getId(), version.getId());
} finally {
rollbackIfActive(entityManager);
entityManager.close();
}
}
private void deleteSuite(PersistedSuite persisted) {
EntityManager entityManager = entityManagerFactory.createEntityManager();
try {
entityManager.getTransaction().begin();
entityManager.createQuery("DELETE FROM SkillSuiteVersion version WHERE version.id = :id")
.setParameter("id", persisted.versionId())
.executeUpdate();
entityManager.createQuery("DELETE FROM SkillSuite suite WHERE suite.id = :id")
.setParameter("id", persisted.suiteId())
.executeUpdate();
entityManager.createQuery("DELETE FROM Namespace namespace WHERE namespace.id = :id")
.setParameter("id", persisted.namespaceId())
.executeUpdate();
entityManager.getTransaction().commit();
} finally {
rollbackIfActive(entityManager);
entityManager.close();
}
}
private void rollbackIfActive(EntityManager entityManager) {
if (entityManager.getTransaction().isActive()) {
entityManager.getTransaction().rollback();
}
}
private boolean hasCause(Throwable error, Class<? extends Throwable> expectedType) {
Throwable current = error;
while (current != null) {
if (expectedType.isInstance(current)) {
return true;
}
current = current.getCause();
}
return false;
}
private record PersistedSuite(Long namespaceId, Long suiteId, Long versionId) {
}
}

4
server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/suite/SkillSuiteQueryService.java
View file

@ -3,6 +3,7 @@ package com.iflytek.skillhub.domain.suite;
import com.iflytek.skillhub.domain.namespace.Namespace;
import com.iflytek.skillhub.domain.namespace.NamespaceRepository;
import com.iflytek.skillhub.domain.namespace.NamespaceRole;
import com.iflytek.skillhub.domain.namespace.NamespaceStatus;
import com.iflytek.skillhub.domain.review.ReviewPermissionChecker;
import com.iflytek.skillhub.domain.review.ReviewSubjectType;
import com.iflytek.skillhub.domain.review.ReviewTaskRepository;
@ -165,6 +166,9 @@ public class SkillSuiteQueryService {
return true;
}
NamespaceRole role = namespaceRoles.get(suite.getNamespaceId());
if (namespace.getStatus() == NamespaceStatus.ARCHIVED && role == null) {
return false;
}
boolean namespaceAdmin = role == NamespaceRole.OWNER || role == NamespaceRole.ADMIN;
boolean currentCreator = userId != null && userId.equals(suite.getCreatedBy()) && role != null;

5
server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/suite/SkillSuiteVersion.java
View file

@ -11,6 +11,7 @@ import jakarta.persistence.GenerationType;
import jakarta.persistence.Id;
import jakarta.persistence.PrePersist;
import jakarta.persistence.Table;
import jakarta.persistence.Version;
import java.time.Clock;
import java.time.Instant;
@ -26,6 +27,10 @@ public class SkillSuiteVersion {
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;
@Version
@Column(name = "lock_version", nullable = false)
private Long lockVersion;
@Column(name = "suite_id", nullable = false)
private Long suiteId;

28
server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/suite/SkillSuiteQueryServiceTest.java
View file

@ -181,6 +181,34 @@ class SkillSuiteQueryServiceTest {
.isInstanceOf(DomainForbiddenException.class);
}
@Test
void archivedNamespaceHidesPublishedPublicSuiteFromAnonymousUsers() {
Namespace namespace = new Namespace("team", "Team", "owner");
namespace.setStatus(NamespaceStatus.ARCHIVED);
SkillSuite suite = new SkillSuite(1L, "public-suite", "Public Suite", "suite-author");
SkillSuiteVersion version = new SkillSuiteVersion(
10L, "1.0.0", SkillVisibility.PUBLIC, "suite-author");
setId(namespace, 1L);
setId(suite, 10L);
setId(version, 20L);
version.setStatus(SkillSuiteVersionStatus.PUBLISHED);
suite.setLatestVersionId(20L);
when(namespaceRepository.findBySlug("team")).thenReturn(Optional.of(namespace));
when(suiteRepository.findByNamespaceIdAndSlug(1L, "public-suite")).thenReturn(Optional.of(suite));
when(versionRepository.findById(20L)).thenReturn(Optional.of(version));
assertThatThrownBy(() -> service.getDetail(
"team", "public-suite", null, null, Map.of(), Set.of()))
.isInstanceOf(DomainForbiddenException.class);
when(memberRepository.findBySuiteVersionIdOrderByPosition(20L)).thenReturn(List.of());
assertThat(service.getDetail(
"team", "public-suite", null, "member",
Map.of(1L, NamespaceRole.MEMBER), Set.of()).version().getVersion())
.isEqualTo("1.0.0");
}
@Test
void pendingReviewUsesTheActualSubmitterAndReviewPermissions() {
Namespace namespace = new Namespace("team", "Team", "owner");

3
web/src/pages/suite-detail.test.tsx
View file

@ -179,6 +179,9 @@ describe('SuiteDetailPage', () => {
const sidebar = screen.getByRole('complementary', { name: 'suite.detailsSidebar' })
expect(within(sidebar).getByText('v1.0.0')).not.toBeNull()
expect(within(sidebar).getByText('suite.installCommand')).not.toBeNull()
expect(within(sidebar).getByText(
'skillhub suite install @global/care-workflow --version 1.0.0',
)).not.toBeNull()
expect(within(sidebar).getByLabelText('suite.copyInstallCommand')).not.toBeNull()
})
})

2
web/src/pages/suite-detail.tsx
View file

@ -24,7 +24,7 @@ export function SuiteDetailPage() {
const { data: versions } = useSuiteVersions(namespace, slug)
const submitMutation = useSubmitSuite()
const command = useMemo(
() => suite ? `skillhub suite install @${suite.namespace}/${suite.slug}@${suite.version}` : '',
() => suite ? `skillhub suite install @${suite.namespace}/${suite.slug} --version ${suite.version}` : '',
[suite],
)
const suitePath = `/suite/${encodeURIComponent(namespace)}/${encodeURIComponent(slug)}`