fix(publish): delete review tasks of any status when replacing a version (#601)

* fix(publish): delete review tasks of any status when replacing a version

Re-uploading a rejected version under the same version number returned
HTTP 500. deleteReplaceableVersionArtifacts only removed a PENDING review
task, but a rejected version owns a REJECTED one; that row kept a foreign
key on the skill_version, so the subsequent delete hit a constraint
violation that surfaced as a 500.

Delete every review task attached to the version instead.

Signed-off-by: FenjuFu <92919259+FenjuFu@users.noreply.github.com>

* test(publish): drop the spring-test dependency from the new test

skillhub-domain has no spring-test on its test classpath, so
ReflectionTestUtils does not resolve there. Use plain JDK reflection for
setting the generated id and invoking the private method.

Signed-off-by: FenjuFu <92919259+FenjuFu@users.noreply.github.com>

* fix(publish): constrain rejected version replacement

Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>

* test(publish): verify replaced review is deleted

Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>

* test(e2e): use generated API response types

Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>

---------

Signed-off-by: FenjuFu <92919259+FenjuFu@users.noreply.github.com>
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
Co-authored-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>

server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java

@@ -62,6 +62,12 @@ public class SkillPublishService {
 
     private static final DateTimeFormatter AUTO_VERSION_FORMATTER =
             DateTimeFormatter.ofPattern("yyyyMMdd.HHmmss").withZone(ZoneId.systemDefault());
+    private static final Set<SkillVersionStatus> REPLACEABLE_VERSION_STATUSES = Set.of(
+            SkillVersionStatus.DRAFT,
+            SkillVersionStatus.SCAN_FAILED,
+            SkillVersionStatus.UPLOADED,
+            SkillVersionStatus.REJECTED
+    );
     private static final Logger log = LoggerFactory.getLogger(SkillPublishService.class);
 
     public record PublishResult(
@@ -566,7 +572,7 @@ public class SkillPublishService {
     }
 
     private void deleteReplaceableVersionArtifacts(Skill skill, SkillVersion version, String namespaceSlug) {
-        if (version.getStatus() == SkillVersionStatus.PUBLISHED) {
+        if (!REPLACEABLE_VERSION_STATUSES.contains(version.getStatus())) {
             throw new DomainBadRequestException("error.skill.version.exists", version.getVersion());
         }
 
@@ -577,8 +583,10 @@ public class SkillPublishService {
             skillRepository.flush();
         }
 
-        reviewTaskRepository.findBySkillVersionIdAndStatus(version.getId(), ReviewTaskStatus.PENDING)
-                .ifPresent(reviewTaskRepository::delete);
+        // Every review task referencing this version has to go, not just a PENDING one:
+        // a rejected version still owns a REJECTED task whose foreign key blocks the
+        // skill_version delete below, which surfaces to the caller as an HTTP 500.
+        reviewTaskRepository.deleteBySkillVersionIdIn(List.of(version.getId()));
 
         List<SkillFile> files = skillFileRepository.findByVersionId(version.getId());
         List<String> storageKeys = new ArrayList<>();

server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java

@@ -260,7 +260,7 @@ class SkillPublishServiceTest {
     }
 
     @Test
-    void testPublishFromEntries_ShouldReplaceDraftVersionWithSameVersion() throws Exception {
+    void testPublishFromEntries_ShouldReplaceRejectedVersionWithSameVersion() throws Exception {
         String namespaceSlug = "test-ns";
         String publisherId = "user-100";
         String skillMdContent = "---\nname: test-skill\ndescription: Test\nversion: 1.0.0\n---\nBody";
@@ -275,9 +275,9 @@ class SkillPublishServiceTest {
 
         Skill skill = new Skill(1L, "test-skill", publisherId, SkillVisibility.PUBLIC);
         setId(skill, 1L);
-        SkillVersion draftVersion = new SkillVersion(1L, "1.0.0", publisherId);
-        draftVersion.setStatus(SkillVersionStatus.DRAFT);
-        setId(draftVersion, 8L);
+        SkillVersion rejectedVersion = new SkillVersion(1L, "1.0.0", publisherId);
+        rejectedVersion.setStatus(SkillVersionStatus.REJECTED);
+        setId(rejectedVersion, 8L);
         SkillFile oldFile = new SkillFile(8L, "SKILL.md", (long) skillMdContent.length(), "text/markdown", "abc", "skills/1/8/SKILL.md");
 
         when(namespaceRepository.findBySlug(namespaceSlug)).thenReturn(Optional.of(namespace));
@@ -288,7 +288,7 @@ class SkillPublishServiceTest {
         when(skillRepository.findByNamespaceIdAndSlug(any(), eq("test-skill"))).thenReturn(List.of(skill));
         when(skillRepository.findByNamespaceIdAndSlugAndOwnerId(any(), eq("test-skill"), eq(publisherId))).thenReturn(Optional.of(skill));
         when(skillVersionRepository.findBySkillIdAndStatus(1L, SkillVersionStatus.PENDING_REVIEW)).thenReturn(List.of());
-        when(skillVersionRepository.findBySkillIdAndVersion(1L, "1.0.0")).thenReturn(Optional.of(draftVersion));
+        when(skillVersionRepository.findBySkillIdAndVersion(1L, "1.0.0")).thenReturn(Optional.of(rejectedVersion));
         when(skillFileRepository.findByVersionId(8L)).thenReturn(List.of(oldFile));
         when(skillVersionRepository.save(any(SkillVersion.class))).thenAnswer(invocation -> {
             SkillVersion saved = invocation.getArgument(0);
@@ -309,10 +309,60 @@ class SkillPublishServiceTest {
 
         assertEquals("1.0.0", result.version().getVersion());
         assertEquals(SkillVersionStatus.PENDING_REVIEW, result.version().getStatus());
+        verify(reviewTaskRepository).deleteBySkillVersionIdIn(List.of(8L));
         verify(skillFileRepository).deleteByVersionId(8L);
-        verify(skillVersionRepository).delete(draftVersion);
+        verify(skillVersionRepository).delete(rejectedVersion);
         verify(skillVersionRepository).flush();
         verify(objectStorageService).deleteObjects(List.of("skills/1/8/SKILL.md", "packages/1/8/bundle.zip"));
+
+        ArgumentCaptor<ReviewTask> reviewTaskCaptor = ArgumentCaptor.forClass(ReviewTask.class);
+        verify(reviewTaskRepository).save(reviewTaskCaptor.capture());
+        assertEquals(result.version().getId(), reviewTaskCaptor.getValue().getSkillVersionId());
+        assertEquals(publisherId, reviewTaskCaptor.getValue().getSubmittedBy());
+    }
+
+    @Test
+    void testPublishFromEntries_ShouldRejectReplacementOfYankedVersion() throws Exception {
+        String namespaceSlug = "test-ns";
+        String publisherId = "user-100";
+        String skillMdContent = "---\nname: test-skill\ndescription: Test\nversion: 1.0.0\n---\nBody";
+
+        PackageEntry skillMd = new PackageEntry("SKILL.md", skillMdContent.getBytes(), skillMdContent.length(), "text/markdown");
+        List<PackageEntry> entries = List.of(skillMd);
+
+        Namespace namespace = new Namespace(namespaceSlug, "Test NS", "user-1");
+        setId(namespace, 1L);
+        NamespaceMember member = mock(NamespaceMember.class);
+        SkillMetadata metadata = new SkillMetadata("test-skill", "Test", "1.0.0", "Body", Map.of());
+
+        Skill skill = new Skill(1L, "test-skill", publisherId, SkillVisibility.PUBLIC);
+        setId(skill, 1L);
+        SkillVersion yankedVersion = new SkillVersion(1L, "1.0.0", publisherId);
+        yankedVersion.setStatus(SkillVersionStatus.YANKED);
+        setId(yankedVersion, 8L);
+
+        when(namespaceRepository.findBySlug(namespaceSlug)).thenReturn(Optional.of(namespace));
+        when(namespaceMemberRepository.findByNamespaceIdAndUserId(any(), eq(publisherId))).thenReturn(Optional.of(member));
+        when(skillPackageValidator.validate(entries)).thenReturn(ValidationResult.pass());
+        when(skillMetadataParser.parse(skillMdContent)).thenReturn(metadata);
+        when(prePublishValidator.validate(any())).thenReturn(ValidationResult.pass());
+        when(skillRepository.findByNamespaceIdAndSlug(any(), eq("test-skill"))).thenReturn(List.of(skill));
+        when(skillRepository.findByNamespaceIdAndSlugAndOwnerId(any(), eq("test-skill"), eq(publisherId))).thenReturn(Optional.of(skill));
+        when(skillVersionRepository.findBySkillIdAndVersion(1L, "1.0.0")).thenReturn(Optional.of(yankedVersion));
+
+        DomainBadRequestException exception = assertThrows(DomainBadRequestException.class, () ->
+                service.publishFromEntries(
+                        namespaceSlug,
+                        entries,
+                        publisherId,
+                        SkillVisibility.PUBLIC,
+                        Set.of()
+                ));
+
+        assertEquals("error.skill.version.exists", exception.messageCode());
+        verify(reviewTaskRepository, never()).deleteBySkillVersionIdIn(anyList());
+        verify(skillVersionRepository, never()).delete(any());
+        verify(skillFileRepository, never()).deleteByVersionId(any());
     }
 
     @Test

web/e2e/helpers/test-data-builder.ts

@@ -3,6 +3,7 @@ import { tmpdir } from 'node:os'
 import { execFileSync } from 'node:child_process'
 import path from 'node:path'
 import type { APIRequestContext, Page, TestInfo } from '@playwright/test'
+import type { components } from '../../src/api/generated/schema'
 import { csrfHeaders } from './csrf'
 
 type CleanupTask = () => Promise<void>
@@ -32,14 +33,9 @@ export interface SeededReviewData {
   skill: SeededSkill
 }
 
-interface ReviewTaskSummary {
-  id: number
-  namespace: string
-  skillSlug: string
-  status: string
-  submittedBy: string
-  version: string
-}
+type ReviewTaskResponse = components['schemas']['ReviewTaskResponse']
+type SkillVersionResponse = components['schemas']['SkillVersionResponse']
+type SkillVersionStatus = NonNullable<SkillVersionResponse['status']>
 
 interface NamespaceCandidate {
   userId: string
@@ -463,19 +459,17 @@ export class E2eTestDataBuilder {
   async waitForPendingReview(namespaceSlug: string, skillSlug: string, version: string): Promise<number> {
     for (let attempt = 0; attempt < 20; attempt += 1) {
       try {
-        const page = await parseEnvelope<{
-          items: ReviewTaskSummary[]
-        }>(
+        const page = await parseEnvelope<components['schemas']['PageResponseReviewTaskResponse']>(
           await this.request.get('/api/web/reviews?status=PENDING&page=0&size=100&sortDirection=DESC'),
         )
 
-        const matched = page.items.find((item) =>
+        const matched = page.items?.find((item) =>
           item.namespace === namespaceSlug &&
           item.skillSlug === skillSlug &&
           item.version === version &&
           item.status === 'PENDING',
         )
-        if (matched) {
+        if (matched?.id != null) {
           return matched.id
         }
       } catch {
@@ -488,6 +482,38 @@ export class E2eTestDataBuilder {
     throw new Error(`Timed out waiting for pending review ${namespaceSlug}/${skillSlug}@${version}`)
   }
 
+  async waitForVersionStatus(
+    namespaceSlug: string,
+    skillSlug: string,
+    version: string,
+    expectedStatus: SkillVersionStatus,
+  ): Promise<number> {
+    for (let attempt = 0; attempt < 60; attempt += 1) {
+      try {
+        const page = await parseEnvelope<components['schemas']['PageResponseSkillVersionResponse']>(
+          await this.request.get(
+            `/api/web/skills/${encodeURIComponent(namespaceSlug)}/${encodeURIComponent(skillSlug)}/versions?page=0&size=100`,
+          ),
+        )
+
+        const matched = page.items?.find((item) =>
+          item.version === version && item.status === expectedStatus,
+        )
+        if (matched?.id != null) {
+          return matched.id
+        }
+      } catch {
+        // Security scanning and version projection can complete asynchronously.
+      }
+
+      await new Promise((resolve) => setTimeout(resolve, 1_000))
+    }
+
+    throw new Error(
+      `Timed out waiting for ${namespaceSlug}/${skillSlug}@${version} to reach ${expectedStatus}`,
+    )
+  }
+
   async approveReview(reviewTaskId: number, comment = 'Approved by Playwright E2E'): Promise<void> {
     let lastError: unknown
     for (let attempt = 0; attempt < 60; attempt += 1) {
@@ -512,6 +538,15 @@ export class E2eTestDataBuilder {
     throw lastError instanceof Error ? lastError : new Error('approveReview timed out')
   }
 
+  async rejectReview(reviewTaskId: number, comment = 'Rejected by Playwright E2E'): Promise<void> {
+    await parseEnvelope<ReviewTaskResponse>(
+      await this.request.post(`/api/web/reviews/${reviewTaskId}/reject`, {
+        data: { comment },
+        headers: await csrfHeaders(this.page),
+      }),
+    )
+  }
+
   async searchNamespaceMemberCandidates(slug: string, search: string): Promise<NamespaceCandidate[]> {
     const query = new URLSearchParams({ search })
     return parseEnvelope<NamespaceCandidate[]>(

web/e2e/rejected-version-republish.spec.ts

@@ -0,0 +1,85 @@
+import { expect, test } from '@playwright/test'
+import { setEnglishLocale } from './helpers/auth-fixtures'
+import { loginWithCredentials, registerSession } from './helpers/session'
+import { E2eTestDataBuilder } from './helpers/test-data-builder'
+
+function getOptionalEnv(name: string): string | undefined {
+  const value = process.env[name]?.trim()
+  return value ? value : undefined
+}
+
+function adminCredentials() {
+  return {
+    username: getOptionalEnv('E2E_ADMIN_USERNAME') ?? getOptionalEnv('BOOTSTRAP_ADMIN_USERNAME') ?? 'admin',
+    password: getOptionalEnv('E2E_ADMIN_PASSWORD') ?? getOptionalEnv('BOOTSTRAP_ADMIN_PASSWORD') ?? 'ChangeMe!2026',
+  }
+}
+
+test.describe('Rejected version replacement (Real API)', () => {
+  test.describe.configure({ timeout: 150_000 })
+
+  test.beforeEach(async ({ page }, testInfo) => {
+    await setEnglishLocale(page)
+    await registerSession(page, testInfo)
+  })
+
+  test('re-publishes the same version after rejection', async ({ page, browser }, testInfo) => {
+    const publisherBuilder = new E2eTestDataBuilder(page, testInfo)
+    await publisherBuilder.init()
+
+    const adminContext = await browser.newContext()
+    const adminPage = await adminContext.newPage()
+    const adminBuilder = new E2eTestDataBuilder(adminPage, testInfo)
+    await loginWithCredentials(adminPage, adminCredentials(), testInfo)
+    await adminBuilder.init()
+
+    try {
+      const namespace = await publisherBuilder.ensureWritableNamespace()
+      const skillName = `replace-rejected-${Date.now().toString(36)}`
+      const firstPublish = await publisherBuilder.publishSkill(namespace.slug, {
+        name: skillName,
+        version: '1.0.0',
+      })
+      const rejectedReviewId = await adminBuilder.waitForPendingReview(
+        namespace.slug,
+        firstPublish.slug,
+        firstPublish.version,
+      )
+      await publisherBuilder.waitForVersionStatus(
+        namespace.slug,
+        firstPublish.slug,
+        firstPublish.version,
+        'PENDING_REVIEW',
+      )
+      await adminBuilder.rejectReview(rejectedReviewId)
+
+      const replacement = await publisherBuilder.publishSkill(namespace.slug, {
+        name: skillName,
+        description: 'Replacement after review rejection',
+        version: '1.0.0',
+      })
+      const replacementReviewId = await adminBuilder.waitForPendingReview(
+        namespace.slug,
+        replacement.slug,
+        replacement.version,
+      )
+      await publisherBuilder.waitForVersionStatus(
+        namespace.slug,
+        replacement.slug,
+        replacement.version,
+        'PENDING_REVIEW',
+      )
+
+      expect(replacement.skillId).toBe(firstPublish.skillId)
+      expect(replacement.version).toBe(firstPublish.version)
+      expect(replacementReviewId).not.toBe(rejectedReviewId)
+
+      const replacedReviewResponse = await adminPage.request.get(`/api/web/reviews/${rejectedReviewId}`)
+      expect(replacedReviewResponse.status()).toBe(404)
+    } finally {
+      await adminBuilder.cleanup()
+      await adminContext.close()
+      await publisherBuilder.cleanup()
+    }
+  })
+})