From 302315f3418d7854d868aa87e18168a387617a6d Mon Sep 17 00:00:00 2001 From: Patrick Erichsen Date: Mon, 20 Jul 2026 18:57:08 -0700 Subject: [PATCH] fix: require restore before republishing deleted entries --- convex/packages.public.test.ts | 5 +- convex/packages.ts | 2 +- convex/skills.backportLatest.test.ts | 420 ++++++++++-------- convex/skills.ts | 147 +++--- e2e/local-auth/helpers.ts | 23 +- .../publish-skill-lifecycle.pw.test.ts | 59 +-- 6 files changed, 327 insertions(+), 329 deletions(-) diff --git a/convex/packages.public.test.ts b/convex/packages.public.test.ts index d10b2385..d026a64e 100644 --- a/convex/packages.public.test.ts +++ b/convex/packages.public.test.ts @@ -7793,7 +7793,10 @@ describe("packages public queries", () => { files: [], integritySha256: "abc123", }), - ).rejects.toThrow("Restore it before publishing another release"); + ).rejects.toThrow( + 'Package "demo-plugin" is hidden/deleted. Run "clawhub package undelete demo-plugin --yes" before publishing another release.', + ); + expect(ctx.insert).not.toHaveBeenCalledWith("packageReleases", expect.anything()); }); it("rejects final package publish inserts when the actor was banned mid-publish", async () => { diff --git a/convex/packages.ts b/convex/packages.ts index 90260023..4b2b70d7 100644 --- a/convex/packages.ts +++ b/convex/packages.ts @@ -9765,7 +9765,7 @@ export const insertReleaseInternal = internalMutation({ const nextNameLabel = typeof args.name === "string" ? args.name : ""; if (existing?.softDeletedAt) { throw new ConvexError( - `Package "${nextNameLabel}" was deleted. Restore it before publishing another release or choose a new package name.`, + `Package "${nextNameLabel}" is hidden/deleted. Run "clawhub package undelete ${nextNameLabel} --yes" before publishing another release.`, ); } const nextChannel = derivePackagePublisherChannel({ diff --git a/convex/skills.backportLatest.test.ts b/convex/skills.backportLatest.test.ts index f5fa4504..5669c5f9 100644 --- a/convex/skills.backportLatest.test.ts +++ b/convex/skills.backportLatest.test.ts @@ -5,7 +5,11 @@ vi.mock("@convex-dev/auth/server", () => ({ authTables: {}, })); -import { insertVersion } from "./skills"; +import { + discardPendingPublicationInternal, + insertVersion, + publishPendingVersionInternal, +} from "./skills"; type WrappedHandler = { _handler: (ctx: unknown, args: TArgs) => Promise; @@ -13,10 +17,15 @@ type WrappedHandler = { const insertVersionHandler = (insertVersion as unknown as WrappedHandler>) ._handler; +const discardPendingPublicationHandler = ( + discardPendingPublicationInternal as unknown as WrappedHandler> +)._handler; +const publishPendingVersionHandler = ( + publishPendingVersionInternal as unknown as WrappedHandler> +)._handler; const OWNER_USER_ID = "users:owner"; const OWNER_PUBLISHER_ID = "publishers:owner"; -const MODERATOR_USER_ID = "users:moderator"; const SKILL_ID = "skills:1"; const PREV_LATEST_VERSION_ID = "skillVersions:prev"; const PREV_EMBEDDING_ID = "skillEmbeddings:prev"; @@ -161,11 +170,6 @@ function buildDb( skill: SkillDoc, captured: Captured, existingVersion?: Record | null, - options: { - activeOwnerPublisher?: boolean; - moderatorUser?: boolean; - pendingVersions?: Record[]; - } = {}, ) { // Trigger-driven code (syncSkillSearchDigestForSkill -> getOwnerPublisher) // will ask for publishers via `db.get(ownerPublisherId)`. Return null so @@ -200,28 +204,14 @@ function buildDb( }; } if (id === OWNER_PUBLISHER_ID) { - if (options.activeOwnerPublisher) { - return { - _id: OWNER_PUBLISHER_ID, - kind: "user", - handle: "alice", - displayName: "Alice", - linkedUserId: OWNER_USER_ID, - deletedAt: undefined, - deactivatedAt: undefined, - }; - } - // Returning null lets getOwnerPublisher fall back to user-based - // resolution, which then hits the synthesize fallback. - return null; - } - if (id === MODERATOR_USER_ID && options.moderatorUser) { return { - _id: MODERATOR_USER_ID, - handle: "moderator", - role: "moderator", - deletedAt: undefined, - deactivatedAt: undefined, + _id: OWNER_PUBLISHER_ID, + kind: "user", + handle: "alice", + displayName: "Alice", + linkedUserId: OWNER_USER_ID, + createdAt: 1, + updatedAt: 1, }; } if (id === SKILL_ID) return skill; @@ -291,9 +281,7 @@ function buildDb( llmAnalysis: { status: "clean" }, staticScan: { status: "clean" }, }; - return [...(options.pendingVersions ?? []), insertedVersion, previousVersion] - .filter(Boolean) - .slice(0, limit); + return [insertedVersion, previousVersion].filter(Boolean).slice(0, limit); }, }), }; @@ -453,15 +441,7 @@ function buildDb( return db; } -function buildCtx( - skill: SkillDoc, - existingVersion?: Record | null, - options: { - activeOwnerPublisher?: boolean; - moderatorUser?: boolean; - pendingVersions?: Record[]; - } = {}, -) { +function buildCtx(skill: SkillDoc, existingVersion?: Record | null) { const captured: Captured = { skillPatches: [], embeddingInserts: [], @@ -469,7 +449,7 @@ function buildCtx( versionInserted: null, allPatches: [], }; - const db = buildDb(skill, captured, existingVersion, options); + const db = buildDb(skill, captured, existingVersion); const ctx = { db, scheduler: { runAfter: vi.fn() }, @@ -495,159 +475,14 @@ describe("skills.insertVersion latest-tag protection", () => { expect(captured.versionInserted).toBeNull(); }); - it("keeps an immutable version reserved when its parent skill is owner-deleted", async () => { - const deletedAt = 1_700_000_000_000; + it("requires owners to restore a deleted skill before publishing another version", async () => { const skill = buildExistingSkill({ moderationStatus: "hidden", moderationReason: undefined, - softDeletedAt: deletedAt, - hiddenAt: deletedAt, + softDeletedAt: 1_700_000_000_000, hiddenBy: OWNER_USER_ID, - unpublishedSlugReservedUntil: deletedAt + 30 * 24 * 60 * 60 * 1000, - }); - const { ctx, captured } = buildCtx( - skill, - { - _id: "skillVersions:existing", - skillId: SKILL_ID, - version: "2.0.0", - }, - { activeOwnerPublisher: true }, - ); - - await expect( - insertVersionHandler( - ctx as never, - buildPublishArgs({ - version: "2.0.0", - publicationStatus: "pending", - }) as never, - ), - ).rejects.toThrow("Version 2.0.0 already exists. Increment the version number and try again."); - - expect(captured.versionInserted).toBeNull(); - expect(captured.skillPatches).not.toContainEqual( - expect.objectContaining({ softDeletedAt: undefined }), - ); - }); - - it("restores an owner-deleted skill when staging a new version", async () => { - const deletedAt = 1_700_000_000_000; - const skill = buildExistingSkill({ - moderationStatus: "hidden", - moderationReason: undefined, - softDeletedAt: deletedAt, - hiddenAt: deletedAt, - hiddenBy: OWNER_USER_ID, - unpublishedSlugReservedUntil: deletedAt + 30 * 24 * 60 * 60 * 1000, - }); - const { ctx, captured } = buildCtx(skill, null, { activeOwnerPublisher: true }); - - await expect( - insertVersionHandler( - ctx as never, - buildPublishArgs({ - version: "2.1.0", - publicationStatus: "pending", - }) as never, - ), - ).resolves.toEqual({ - skillId: SKILL_ID, - versionId: NEW_VERSION_ID, - publicationStatus: "pending", - createdNewParent: false, - }); - - expect(captured.versionInserted).toMatchObject({ - skillId: SKILL_ID, - version: "2.1.0", - publicationStatus: "pending", - pendingPublication: expect.objectContaining({ - ownerDeleteRestoreState: expect.objectContaining({ - softDeletedAt: deletedAt, - moderationStatus: "hidden", - unpublishedSlugReservedUntil: deletedAt + 30 * 24 * 60 * 60 * 1000, - }), - }), - }); - expect(captured.skillPatches).toContainEqual( - expect.objectContaining({ - softDeletedAt: undefined, - unpublishedSlugReservedUntil: undefined, - }), - ); - }); - - it("carries owner-delete rollback state across concurrent pending versions", async () => { - const deletedAt = 1_700_000_000_000; - const restoreState = { - softDeletedAt: deletedAt, - moderationStatus: "hidden", - unpublishedSlugReservedUntil: deletedAt + 30 * 24 * 60 * 60 * 1000, - updatedAt: deletedAt, - }; - const skill = buildExistingSkill({ - moderationStatus: "hidden", - moderationReason: "pending.publication", - softDeletedAt: undefined, - hiddenAt: deletedAt, - hiddenBy: OWNER_USER_ID, - unpublishedSlugReservedUntil: undefined, - }); - const { ctx, captured } = buildCtx(skill, null, { - activeOwnerPublisher: true, - pendingVersions: [ - { - _id: "skillVersions:first-pending", - skillId: SKILL_ID, - version: "2.1.0", - publicationStatus: "pending", - softDeletedAt: undefined, - pendingPublication: { ownerDeleteRestoreState: restoreState }, - }, - ], - }); - - await expect( - insertVersionHandler( - ctx as never, - buildPublishArgs({ - version: "2.2.0", - publicationStatus: "pending", - }) as never, - ), - ).resolves.toEqual({ - skillId: SKILL_ID, - versionId: NEW_VERSION_ID, - publicationStatus: "pending", - createdNewParent: false, - }); - - expect(captured.versionInserted).toMatchObject({ - version: "2.2.0", - pendingPublication: { - ownerDeleteRestoreState: restoreState, - skillInsertArgs: expect.any(Object), - }, - }); - expect(captured.skillPatches).not.toContainEqual( - expect.objectContaining({ softDeletedAt: undefined }), - ); - }); - - it("does not restore a skill hidden by a moderator", async () => { - const hiddenAt = 1_700_000_000_000; - const skill = buildExistingSkill({ - moderationStatus: "hidden", - moderationReason: "manual.report", - softDeletedAt: hiddenAt, - hiddenAt, - hiddenBy: MODERATOR_USER_ID, - }); - const { ctx, captured } = buildCtx(skill, null, { - activeOwnerPublisher: true, - moderatorUser: true, }); + const { ctx, captured } = buildCtx(skill); await expect( insertVersionHandler( @@ -658,13 +493,67 @@ describe("skills.insertVersion latest-tag protection", () => { }) as never, ), ).rejects.toThrow( - "Forbidden: This skill was hidden by moderation and cannot be restored by publishing.", + 'Skill "my-skill" is hidden/deleted. Run "clawhub undelete @alice/my-skill --yes" before publishing another version.', ); expect(captured.versionInserted).toBeNull(); - expect(captured.skillPatches).not.toContainEqual( - expect.objectContaining({ softDeletedAt: undefined }), + }); + + it("does not advertise owner undelete for moderation-hidden skills", async () => { + const skill = buildExistingSkill({ + moderationStatus: "hidden", + moderationReason: "security.redaction", + moderationFlags: ["blocked.malware"], + moderationVerdict: "malicious", + softDeletedAt: 1_700_000_000_000, + hiddenBy: "users:moderator", + }); + const { ctx, captured } = buildCtx(skill); + + await expect( + insertVersionHandler( + ctx as never, + buildPublishArgs({ + version: "2.1.0", + publicationStatus: "pending", + }) as never, + ), + ).rejects.toThrow( + "Forbidden: This skill was hidden by moderation and cannot be restored by the owner. Please contact a moderator.", ); + + expect(captured.versionInserted).toBeNull(); + }); + + it("blocks another version while a legacy deleted-skill republish is pending", async () => { + const deletedAt = 1_700_000_000_000; + const skill = buildExistingSkill({ + moderationStatus: "hidden", + moderationReason: "pending.publication", + softDeletedAt: undefined, + }); + const { ctx, captured } = buildCtx(skill); + captured.versionInserted = { + version: "2.0.1", + publicationStatus: "pending", + pendingPublication: { + ownerDeleteRestoreState: { + softDeletedAt: deletedAt, + moderationStatus: "hidden", + updatedAt: deletedAt, + }, + }, + }; + + await expect( + insertVersionHandler( + ctx as never, + buildPublishArgs({ + version: "2.1.0", + publicationStatus: "pending", + }) as never, + ), + ).rejects.toThrow("deleted-skill republish awaiting security checks"); }); it("promotes latest when publishing a strictly higher version", async () => { @@ -1030,3 +919,150 @@ describe("skills.insertVersion latest-tag protection", () => { expect(finalPatch.latestVersionSummary).toMatchObject({ version: "0.1.0" }); }); }); + +describe("skills.discardPendingPublicationInternal legacy owner-delete rollback", () => { + it("restores a deleted-skill tombstone saved by an already-pending publish", async () => { + const deletedAt = 1_700_000_000_000; + const reservedUntil = deletedAt + 30 * 24 * 60 * 60 * 1000; + const version = { + _id: "skillVersions:pending", + skillId: SKILL_ID, + version: "2.1.0", + publicationStatus: "pending", + files: [], + pendingPublication: { + ownerDeleteRestoreState: { + softDeletedAt: deletedAt, + moderationStatus: "hidden", + unpublishedSlugReservedUntil: reservedUntil, + updatedAt: deletedAt, + }, + }, + }; + const skill = buildExistingSkill({ + moderationStatus: "hidden", + moderationReason: "pending.publication", + softDeletedAt: undefined, + }); + const patch = vi.fn(); + const ctx = { + db: { + get: vi.fn(async (id: string) => { + if (id === version._id) return version; + if (id === SKILL_ID) return skill; + if (id === OWNER_USER_ID) { + return { + _id: OWNER_USER_ID, + publishedSkills: 1, + totalStars: 0, + totalDownloads: 0, + }; + } + return null; + }), + query: vi.fn((table: string) => { + if (table === "skillVersionFingerprints") { + return { + withIndex: () => ({ + take: async () => [], + }), + }; + } + if (table === "skillVersions") { + return { + withIndex: () => ({ + order: () => ({ + take: async () => [], + }), + }), + }; + } + throw new Error(`unexpected table ${table}`); + }), + delete: vi.fn(), + insert: vi.fn(), + patch, + replace: vi.fn(), + normalizeId: vi.fn(), + }, + storage: { + delete: vi.fn(), + }, + }; + + await expect( + discardPendingPublicationHandler(ctx as never, { + skillId: SKILL_ID, + versionId: version._id, + createdNewParent: false, + }), + ).resolves.toEqual({ deleted: true, parentDeleted: false }); + + expect(patch).toHaveBeenCalledWith( + SKILL_ID, + expect.objectContaining({ + softDeletedAt: deletedAt, + moderationStatus: "hidden", + unpublishedSlugReservedUntil: reservedUntil, + updatedAt: deletedAt, + }), + ); + }); +}); + +describe("skills.publishPendingVersionInternal legacy owner-delete coordination", () => { + it("does not finalize after the parent is deleted again", async () => { + const deletedAt = 1_700_000_000_000; + const version = { + _id: "skillVersions:pending", + skillId: SKILL_ID, + version: "2.1.0", + publicationStatus: "pending", + softDeletedAt: undefined, + pendingPublication: { + ownerDeleteRestoreState: { + softDeletedAt: deletedAt, + moderationStatus: "hidden", + updatedAt: deletedAt, + }, + }, + }; + const skill = buildExistingSkill({ + moderationStatus: "hidden", + moderationReason: undefined, + softDeletedAt: deletedAt + 1, + }); + const ctx = { + db: { + get: vi.fn(async (id: string) => { + if (id === version._id) return version; + if (id === SKILL_ID) return skill; + return null; + }), + query: vi.fn((table: string) => { + if (table === "skillEmbeddings") { + return { + withIndex: () => ({ + unique: async () => null, + }), + }; + } + throw new Error(`unexpected table ${table}`); + }), + insert: vi.fn(), + patch: vi.fn(), + delete: vi.fn(), + replace: vi.fn(), + normalizeId: vi.fn(), + }, + scheduler: { runAfter: vi.fn() }, + }; + + await expect( + publishPendingVersionHandler(ctx as never, { + versionId: version._id, + publishArgs: {}, + }), + ).rejects.toThrow("Skill state changed while the deleted-skill republish was pending"); + }); +}); diff --git a/convex/skills.ts b/convex/skills.ts index 4e00ae31..3aad8381 100644 --- a/convex/skills.ts +++ b/convex/skills.ts @@ -12092,7 +12092,7 @@ type PendingOwnerDeleteRestoreState = Pick< | "updatedAt" >; -const OWNER_RESTORE_DENIED_REASONS = new Set([ +const OWNER_UNDELETE_DENIED_REASONS = new Set([ "owner.merged", "user.banned", "security.redaction", @@ -12145,54 +12145,23 @@ function pendingOwnerDeleteRestoreStateFromVersion(version: Doc<"skillVersions"> ); } -async function getOwnerDeleteRestoreStateForPublish( +async function findPendingOwnerDeleteRestoreState( ctx: MutationCtx, - skill: Doc<"skills">, - userId: Id<"users">, + skillId: Id<"skills">, ): Promise { - if (!skill.softDeletedAt) { - if (skill.moderationStatus !== "hidden" || skill.moderationReason !== "pending.publication") { - return null; - } - const pendingVersions = await ctx.db - .query("skillVersions") - .withIndex("by_skill_active_created", (q) => - q.eq("skillId", skill._id).eq("softDeletedAt", undefined), - ) - .order("desc") - .take(25); - for (const version of pendingVersions) { - if (version.publicationStatus !== "pending") continue; - const restoreState = pendingOwnerDeleteRestoreStateFromVersion(version); - if (restoreState) return restoreState; - } - return null; + const pendingVersions = await ctx.db + .query("skillVersions") + .withIndex("by_skill_active_created", (q) => + q.eq("skillId", skillId).eq("softDeletedAt", undefined), + ) + .order("desc") + .take(25); + for (const version of pendingVersions) { + if (version.publicationStatus !== "pending") continue; + const restoreState = pendingOwnerDeleteRestoreStateFromVersion(version); + if (restoreState) return restoreState; } - const moderationFlags = (skill.moderationFlags as string[] | undefined) ?? []; - const reason = skill.moderationReason as string | undefined; - const ownerInitiated = - (await canUserManageSkillOwner(ctx, skill, userId)) && - (await isOwnerInitiatedSkillHideForActor(ctx, skill, userId)) && - !OWNER_RESTORE_DENIED_REASONS.has(reason ?? ""); - if ( - !ownerInitiated || - moderationFlags.includes("blocked.malware") || - skill.moderationVerdict === "malicious" - ) { - throw new ConvexError( - "Forbidden: This skill was hidden by moderation and cannot be restored by publishing. Please contact a moderator.", - ); - } - return { - softDeletedAt: skill.softDeletedAt, - moderationStatus: skill.moderationStatus, - moderationReason: skill.moderationReason, - moderationNotes: skill.moderationNotes, - unpublishedSlugReservedUntil: skill.unpublishedSlugReservedUntil, - unpublishedSlugReleasedAt: skill.unpublishedSlugReleasedAt, - unpublishedOriginalSlug: skill.unpublishedOriginalSlug, - updatedAt: skill.updatedAt, - }; + return null; } async function restoreDiscardedPendingOwnerDelete( @@ -12639,6 +12608,36 @@ export const insertVersion = internalMutation({ skill = { ...skill, ownerPublisherId, lastReviewedAt: now, updatedAt: now }; } + if ( + skill && + !skill.softDeletedAt && + skill.moderationStatus === "hidden" && + skill.moderationReason === "pending.publication" && + (await findPendingOwnerDeleteRestoreState(ctx, skill._id)) + ) { + throw new ConvexError( + `Skill "${normalizedSlug}" has a deleted-skill republish awaiting security checks. Wait for it to finish before publishing another version.`, + ); + } + + if (skill?.softDeletedAt) { + const moderationFlags = (skill.moderationFlags as string[] | undefined) ?? []; + const ownerCanRestore = + (await canUserManageSkillOwner(ctx, skill, userId)) && + (await isOwnerInitiatedSkillHideForActor(ctx, skill, userId)) && + !OWNER_UNDELETE_DENIED_REASONS.has(skill.moderationReason ?? "") && + !moderationFlags.includes("blocked.malware") && + skill.moderationVerdict !== "malicious"; + if (!ownerCanRestore) { + throw new ConvexError( + "Forbidden: This skill was hidden by moderation and cannot be restored by the owner. Please contact a moderator.", + ); + } + throw new ConvexError( + `Skill "${normalizedSlug}" is hidden/deleted. Run "clawhub undelete @${ownerPublisher.handle}/${normalizedSlug} --yes" before publishing another version.`, + ); + } + const qualityAssessment = args.qualityAssessment; const isQualityQuarantine = qualityAssessment?.decision === "quarantine"; @@ -12818,14 +12817,13 @@ export const insertVersion = internalMutation({ `Version ${args.version} already exists. Increment the version number and try again.`, ); } - const ownerDeleteRestoreState = await getOwnerDeleteRestoreStateForPublish(ctx, skill, userId); const versionId = await ctx.db.insert("skillVersions", { skillId: skill._id, version: args.version, publicationStatus: args.publicationStatus ?? "published", pendingPublication: isPendingPublication - ? stripUndefinedForStoredPublication({ skillInsertArgs: args, ownerDeleteRestoreState }) + ? stripUndefinedForStoredPublication({ skillInsertArgs: args }) : undefined, fingerprint: args.fingerprint, sourceProvenance: args.sourceProvenance, @@ -12849,21 +12847,6 @@ export const insertVersion = internalMutation({ kind: "source", createdAt: now, }); - if (ownerDeleteRestoreState && skill.softDeletedAt) { - const patch = { - softDeletedAt: undefined, - moderationStatus: "hidden" as const, - moderationReason: "pending.publication", - moderationNotes: "Pre-publication security checks are pending.", - unpublishedSlugReservedUntil: undefined, - unpublishedSlugReleasedAt: undefined, - unpublishedOriginalSlug: undefined, - updatedAt: now, - }; - const nextSkill = { ...skill, ...patch }; - await ctx.db.patch(skill._id, patch); - await adjustUserSkillStatsForSkillChange(ctx, skill, nextSkill); - } return { skillId: skill._id, versionId, @@ -12981,8 +12964,6 @@ export const insertVersion = internalMutation({ moderationStatus: initialModerationStatus, moderationReason, moderationNotes, - hiddenAt: initialModerationStatus === "hidden" ? now : undefined, - hiddenBy: undefined, moderationVerdict: moderationSnapshot.verdict, moderationReasonCodes: moderationSnapshot.reasonCodes.length ? moderationSnapshot.reasonCodes @@ -13161,6 +13142,17 @@ export const publishPendingVersionInternal = internalMutation({ if (version.publicationStatus !== "pending") { throw new ConvexError(`Skill version is ${version.publicationStatus}, not pending.`); } + const ownerDeleteRestoreState = pendingOwnerDeleteRestoreStateFromVersion(version); + if ( + ownerDeleteRestoreState && + (skill.softDeletedAt || + skill.moderationStatus !== "hidden" || + skill.moderationReason !== "pending.publication") + ) { + throw new ConvexError( + "Skill state changed while the deleted-skill republish was pending. Discard it and restore the skill explicitly before publishing again.", + ); + } const publishArgs = asSkillPendingPublishArgs(args.publishArgs); const user = await ctx.db.get(publishArgs.userId); @@ -13281,8 +13273,6 @@ export const publishPendingVersionInternal = internalMutation({ moderationStatus: initialModerationStatus, moderationReason, moderationNotes, - hiddenAt: initialModerationStatus === "hidden" ? now : undefined, - hiddenBy: undefined, moderationVerdict: moderationSnapshot.verdict, moderationReasonCodes: moderationSnapshot.reasonCodes.length ? moderationSnapshot.reasonCodes @@ -13529,25 +13519,12 @@ async function setSkillSoftDeletedByActor( !isModeratorOrAdmin && !skill.softDeletedAt && skill.moderationStatus === "hidden" && - skill.moderationReason === "pending.publication" + skill.moderationReason === "pending.publication" && + (await findPendingOwnerDeleteRestoreState(ctx, skill._id)) ) { - const pendingVersions = await ctx.db - .query("skillVersions") - .withIndex("by_skill_active_created", (q) => - q.eq("skillId", skill._id).eq("softDeletedAt", undefined), - ) - .order("desc") - .take(25); - const hasPendingOwnerRestore = pendingVersions.some( - (version) => - version.publicationStatus === "pending" && - pendingOwnerDeleteRestoreStateFromVersion(version) !== null, + throw new ConvexError( + "Forbidden: This skill has a republish awaiting security checks and cannot be deleted yet.", ); - if (hasPendingOwnerRestore) { - throw new ConvexError( - "Forbidden: This skill has a republish awaiting security checks and cannot be deleted yet.", - ); - } } // Owner-delete provenance guard: an owner must NOT be able to "re-delete" @@ -13661,7 +13638,7 @@ async function setSkillSoftDeletedByActor( const reason = skill.moderationReason as string | undefined; const ownerInitiatedHide = (await isOwnerInitiatedSkillHideForActor(ctx, skill, args.userId)) && - (reason === undefined || !OWNER_RESTORE_DENIED_REASONS.has(reason)); + (reason === undefined || !OWNER_UNDELETE_DENIED_REASONS.has(reason)); if (!ownerInitiatedHide) { // Prefix with "Forbidden:" so HTTP boundary mappers // (softDeleteErrorToResponse) deterministically return 403 instead of diff --git a/e2e/local-auth/helpers.ts b/e2e/local-auth/helpers.ts index 16bcf7b5..0915774a 100644 --- a/e2e/local-auth/helpers.ts +++ b/e2e/local-auth/helpers.ts @@ -550,6 +550,7 @@ export async function publishSkillVersion( versionExists?: () => Promise; skillMarkdown?: string; completeChecks?: boolean; + expectedError?: string; files?: Array<{ path: string; contents: string | Uint8Array }>; }, ) { @@ -576,9 +577,26 @@ export async function publishSkillVersion( const detailUrlPattern = new RegExp(`/[^/]+/(?:skills/)?${escapeRegExp(args.slug)}$`); const versionExists = async () => args.versionExists ? await args.versionExists() : await publishedSkillVersionExists(page, args); - type PublishState = "duplicate" | "pending" | "private-detail" | "published" | "staged" | ""; + type PublishState = + | "duplicate" + | "pending" + | "private-detail" + | "published" + | "rejected" + | "staged" + | ""; const readPublishState = async (): Promise => { if (await hasDuplicateVersionAlert(page, args.version)) return "duplicate"; + if ( + args.expectedError && + (await page + .getByRole("alert") + .filter({ hasText: args.expectedError }) + .isVisible({ timeout: 500 }) + .catch(() => false)) + ) { + return "rejected"; + } const pathname = new URL(page.url()).pathname; // Staged publishes redirect to the dashboard as soon as Convex accepts the // upload. Treat that navigation as success instead of waiting and retrying. @@ -608,6 +626,9 @@ export async function publishSkillVersion( .poll(readPublishState, { timeout: 60_000, intervals: [500, 1_000, 2_000] }) .not.toBe(""); const observedPublishState = await readPublishState(); + if (observedPublishState === "rejected") { + return args.ownerHandle; + } if (observedPublishState !== "published") { if (args.completeChecks === false) { return args.ownerHandle; diff --git a/e2e/local-auth/publish-skill-lifecycle.pw.test.ts b/e2e/local-auth/publish-skill-lifecycle.pw.test.ts index 78f9101c..5ebd4b8e 100644 --- a/e2e/local-auth/publish-skill-lifecycle.pw.test.ts +++ b/e2e/local-auth/publish-skill-lifecycle.pw.test.ts @@ -432,13 +432,13 @@ test("clean skill publish stays private until TruffleHog and ClawScan pass", asy await expectHealthyPublishPage(page, errors); }); -test("owner republish restores a deleted skill only after a new version passes checks", async ({ +test("publishing another version of a deleted skill requires an explicit restore", async ({ page, request, }, testInfo) => { const errors = trackRuntimeErrors(page); - const slug = `pw-restore-${Date.now().toString(36)}`; - const displayName = "Playwright Restored Skill"; + const slug = `pw-restore-first-${Date.now().toString(36)}`; + const displayName = "Playwright Restore First Skill"; const ownerHandle = await signInAsLocalPublisher(page, "admin"); await publishSkillVersion(page, testInfo, { @@ -447,7 +447,7 @@ test("owner republish restores a deleted skill only after a new version passes c displayName, version: "1.0.0", versionLabel: "published before owner deletion", - changelog: "Initial release before testing owner restore by republish.", + changelog: "Initial release before testing explicit restore.", }); await page.getByRole("link", { name: "Settings" }).click(); @@ -458,39 +458,20 @@ test("owner republish restores a deleted skill only after a new version passes c await deleteDialog.getByRole("button", { name: "Delete skill" }).click(); await expect(page).toHaveURL("/"); - await page.goto("/dashboard", { waitUntil: "domcontentloaded" }); - await waitForHydration(page); - await expect(page.getByText(displayName, { exact: true })).toHaveCount(0); - + const expectedError = `clawhub undelete @${ownerHandle}/${slug} --yes`; await page.goto("/skills/publish", { waitUntil: "domcontentloaded" }); - await publishSkillVersion(page, testInfo, { - ownerHandle, - slug, - displayName, - version: "1.0.0", - versionLabel: "rejected duplicate after owner deletion", - changelog: "An immutable version cannot be reused after deletion.", - completeChecks: false, - }); - await expect(page.getByRole("alert")).toContainText( - "Version 1.0.0 already exists. Increment the version number and try again.", - ); - errors.length = 0; - await publishSkillVersion(page, testInfo, { ownerHandle, slug, displayName, version: "1.0.1", - versionLabel: "pending owner restore", - changelog: "A new version restores the deleted parent through staged publication.", + versionLabel: "rejected until explicit restore", + changelog: "Publishing must not recreate or restore the deleted parent.", completeChecks: false, + expectedError, }); - await expect(page).toHaveURL("/dashboard"); - await expect(page.getByText(displayName, { exact: true }).first()).toBeVisible({ - timeout: 30_000, - }); + await expect(page.getByRole("alert")).toContainText(expectedError); await expect( await publicSkillVersionExists(request, { ownerHandle, @@ -499,27 +480,7 @@ test("owner republish restores a deleted skill only after a new version passes c }), ).toBe(false); - const result = (await completeMockPrePublicationChecks({ - kind: "skill", - slug, - version: "1.0.1", - })) as { status?: string }; - expect(result.status).toBe("finalized"); - - await expect - .poll( - () => - publicSkillVersionExists(request, { - ownerHandle, - slug, - version: "1.0.1", - }), - { timeout: 60_000, intervals: [500, 1_000, 2_000] }, - ) - .toBe(true); - await page.goto(`/${ownerHandle}/${slug}`, { waitUntil: "domcontentloaded" }); - await waitForHydration(page); - await expectCurrentVersion(page, "1.0.1"); + errors.length = 0; await expectHealthyPublishPage(page, errors); });