From eb47a7c177029977ff25ddc4f642eb4eaba3c358 Mon Sep 17 00:00:00 2001 From: Patrick Erichsen Date: Tue, 21 Jul 2026 21:49:59 -0700 Subject: [PATCH] fix: remove skill slug alias quotas (#3218) --- convex/skills.ownership.test.ts | 56 +++++++------- convex/skills.ts | 132 +++----------------------------- specs/slug-routing.md | 10 ++- 3 files changed, 45 insertions(+), 153 deletions(-) diff --git a/convex/skills.ownership.test.ts b/convex/skills.ownership.test.ts index e236505b..8576fc44 100644 --- a/convex/skills.ownership.test.ts +++ b/convex/skills.ownership.test.ts @@ -782,8 +782,10 @@ describe("skills ownership", () => { build(chainEq(constraints)); if (name === "by_skill") { return { - collect: async () => - aliases.filter((alias) => alias.skillId === constraints.skillId), + take: async (limit: number) => + aliases + .filter((alias) => alias.skillId === constraints.skillId) + .slice(0, limit), }; } if (name === "by_slug") { @@ -1039,8 +1041,10 @@ describe("skills ownership", () => { build(chainEq(constraints)); if (name === "by_skill") { return { - collect: async () => - aliases.filter((alias) => alias.skillId === constraints.skillId), + take: async (limit: number) => + aliases + .filter((alias) => alias.skillId === constraints.skillId) + .slice(0, limit), }; } if (name === "by_owner_publisher_slug") { @@ -1186,7 +1190,7 @@ describe("skills ownership", () => { ); }); - it("allows publisher admins to rename publisher-owned skills", async () => { + it("allows publisher admins to rename beyond the former historical alias quota", async () => { const patch = vi.fn(async () => {}); const insert = vi.fn(async () => "skillSlugAliases:old"); const skill = { @@ -1205,6 +1209,13 @@ describe("skills ownership", () => { createdAt: 1_700_000_000_000, softDeletedAt: undefined, }; + const aliases = Array.from({ length: 25 }, (_, index) => ({ + _id: `skillSlugAliases:old-${index}`, + slug: `historical-name-${index}`, + skillId: "skills:source", + ownerUserId: "users:creator", + ownerPublisherId: "publishers:org", + })); const result = await renameOwnedSkillInternalHandler( { @@ -1259,8 +1270,8 @@ describe("skills ownership", () => { if (name === "by_owner_publisher_slug") return { unique: async () => null }; if (name === "by_owner_slug") return { unique: async () => null }; if (name === "by_slug") return { take: async () => [], unique: async () => null }; - if (name === "by_skill") return { collect: async () => [] }; - if (name === "by_owner_publisher") return { take: async () => [] }; + if (name === "by_skill") return { collect: async () => aliases }; + if (name === "by_owner_publisher") return { take: async () => aliases }; throw new Error(`unexpected skillSlugAliases index ${name}`); }, }; @@ -2415,7 +2426,7 @@ describe("skills ownership", () => { expect(patch).not.toHaveBeenCalledWith("skills:source", expect.anything()); }); - it("rejects merges that would reserve too many historical slugs for one skill", async () => { + it("bounds aliases rewritten by a single merge transaction", async () => { const patch = vi.fn(async () => {}); const insert = vi.fn(async () => "auditLogs:1"); const skills = [ @@ -2436,7 +2447,7 @@ describe("skills ownership", () => { softDeletedAt: undefined, }, ]; - const aliases = Array.from({ length: 5 }, (_, index) => ({ + const aliases = Array.from({ length: 201 }, (_, index) => ({ _id: `skillSlugAliases:target-${index}`, slug: `target-old-${index}`, skillId: "skills:target", @@ -2469,9 +2480,7 @@ describe("skills ownership", () => { }; } if (name === "by_owner_publisher_slug") { - return { - unique: async () => null, - }; + return { unique: async () => null }; } if (name === "by_owner_slug") { return { @@ -2485,9 +2494,6 @@ describe("skills ownership", () => { .slice(0, 25), }; } - if (name === "by_canonical" || name === "by_fork_of") { - return { collect: async () => [] }; - } throw new Error(`unexpected skills index ${name}`); }, }; @@ -2499,22 +2505,20 @@ describe("skills ownership", () => { build(chainEq(constraints)); if (name === "by_skill") { return { - collect: async () => - aliases.filter((alias) => alias.skillId === constraints.skillId), + take: async (limit: number) => + aliases + .filter((alias) => alias.skillId === constraints.skillId) + .slice(0, limit), }; } if (name === "by_slug") { return { take: async () => aliases.filter((alias) => alias.slug === constraints.slug).slice(0, 2), - unique: async () => - aliases.find((alias) => alias.slug === constraints.slug) ?? null, }; } if (name === "by_owner_publisher_slug") { - return { - unique: async () => null, - }; + return { unique: async () => null }; } if (name === "by_owner_slug") { return { @@ -2528,12 +2532,6 @@ describe("skills ownership", () => { .slice(0, 25), }; } - if (name === "by_owner") { - return { - take: async () => - aliases.filter((alias) => alias.ownerUserId === constraints.ownerUserId), - }; - } throw new Error(`unexpected skillSlugAliases index ${name}`); }, }; @@ -2550,7 +2548,7 @@ describe("skills ownership", () => { targetSlug: "merge-target", }, ), - ).rejects.toThrow(/Too many historical slugs/); + ).rejects.toThrow(/cannot be merged in one transaction/); expect(patch).not.toHaveBeenCalled(); expect(insert).not.toHaveBeenCalled(); diff --git a/convex/skills.ts b/convex/skills.ts index 0d7a0f58..4360bc21 100644 --- a/convex/skills.ts +++ b/convex/skills.ts @@ -217,8 +217,7 @@ const SLUG_RESERVATION_DAYS = 90; const SLUG_RESERVATION_MS = SLUG_RESERVATION_DAYS * RATE_LIMIT_DAY_MS; const UNPUBLISHED_SLUG_RESERVATION_DAYS = 30; const UNPUBLISHED_SLUG_RESERVATION_MS = UNPUBLISHED_SLUG_RESERVATION_DAYS * RATE_LIMIT_DAY_MS; -const MAX_SKILL_SLUG_ALIASES_PER_SKILL = 5; -const MAX_SKILL_SLUG_ALIASES_PER_OWNER = 25; +const MAX_SKILL_SLUG_ALIASES_PER_MERGE = 200; const MAX_MANUAL_OVERRIDE_NOTE_LENGTH = 1200; const DEFAULT_STAFF_AUDIT_LOG_LIMIT = 10; const MAX_STAFF_AUDIT_LOG_LIMIT = 50; @@ -1291,14 +1290,20 @@ async function getSkillSlugAliasBySlug(ctx: Pick, return resolved.alias; } -async function listSkillSlugAliasesForSkill( +async function listSkillSlugAliasesForMerge( ctx: Pick, skillId: Id<"skills">, ) { - return ctx.db + const aliases = await ctx.db .query("skillSlugAliases") .withIndex("by_skill", (q) => q.eq("skillId", skillId)) - .collect(); + .take(MAX_SKILL_SLUG_ALIASES_PER_MERGE + 1); + if (aliases.length > MAX_SKILL_SLUG_ALIASES_PER_MERGE) { + throw new ConvexError( + `A skill with more than ${MAX_SKILL_SLUG_ALIASES_PER_MERGE} historical slugs cannot be merged in one transaction. Contact a ClawHub maintainer for a batched migration.`, + ); + } + return aliases; } function sameSkillSlugAliasOwner( @@ -1312,75 +1317,6 @@ function sameSkillSlugAliasOwner( ); } -async function countSkillSlugAliasesForOwnerQuota( - ctx: Pick, - ownerUserId: Id<"users">, - ownerPublisherId: Id<"publishers"> | undefined, -) { - if (ownerPublisherId) { - const aliases = await ctx.db - .query("skillSlugAliases") - .withIndex("by_owner_publisher", (q) => q.eq("ownerPublisherId", ownerPublisherId)) - .take(MAX_SKILL_SLUG_ALIASES_PER_OWNER + 1); - return aliases.length; - } - - const aliases = await ctx.db - .query("skillSlugAliases") - .withIndex("by_owner", (q) => q.eq("ownerUserId", ownerUserId)) - .take(MAX_SKILL_SLUG_ALIASES_PER_OWNER + 1); - return aliases.length; -} - -async function assertSkillSlugAliasQuota( - ctx: Pick, - params: { - targetSkillId: Id<"skills">; - ownerUserId: Id<"users">; - ownerPublisherId: Id<"publishers"> | undefined; - currentSkillAliasCount?: number; - addedSkillAliases: number; - removedSkillAliases?: number; - addedOwnerAliases: number; - removedOwnerAliases?: number; - }, -) { - const addedSkillAliases = Math.max(0, params.addedSkillAliases); - const removedSkillAliases = Math.max(0, params.removedSkillAliases ?? 0); - const addedOwnerAliases = Math.max(0, params.addedOwnerAliases); - const removedOwnerAliases = Math.max(0, params.removedOwnerAliases ?? 0); - - const currentSkillAliasCount = - params.currentSkillAliasCount ?? - (await listSkillSlugAliasesForSkill(ctx, params.targetSkillId)).length; - const nextSkillAliasCount = - Math.max(0, currentSkillAliasCount - removedSkillAliases) + addedSkillAliases; - if (nextSkillAliasCount > MAX_SKILL_SLUG_ALIASES_PER_SKILL) { - throw new ConvexError( - "Too many historical slugs are already reserved for this skill. " + - `A skill can keep at most ${MAX_SKILL_SLUG_ALIASES_PER_SKILL} old slug redirects. ` + - "Contact support@openclaw.ai if this is a legitimate migration.", - ); - } - - if (addedOwnerAliases === 0 && removedOwnerAliases === 0) return; - - const currentOwnerAliasCount = await countSkillSlugAliasesForOwnerQuota( - ctx, - params.ownerUserId, - params.ownerPublisherId, - ); - const nextOwnerAliasCount = - Math.max(0, currentOwnerAliasCount - removedOwnerAliases) + addedOwnerAliases; - if (nextOwnerAliasCount > MAX_SKILL_SLUG_ALIASES_PER_OWNER) { - throw new ConvexError( - "Too many historical slugs are already reserved by this owner. " + - `An owner can keep at most ${MAX_SKILL_SLUG_ALIASES_PER_OWNER} old slug redirects. ` + - "Contact support@openclaw.ai if this is a legitimate migration.", - ); - } -} - async function releaseExpiredUnpublishedSkillSlug( ctx: MutationCtx, skill: Doc<"skills">, @@ -11163,32 +11099,7 @@ async function renameOwnedSkillByActor( throw new ConvexError(formatReservedSlugCooldownMessage(newSlug, reservation.expiresAt)); } - const aliasesForSkill = await listSkillSlugAliasesForSkill(ctx, skill._id); - const aliasRemovedForNewSlug = - existingAlias && existingAlias.skillId === skill._id ? existingAlias : null; const previousAlias = await getSkillSlugAliasBySlugForPublisher(ctx, skill.slug, skillOwner); - const addedSkillAliases = previousAlias?.skillId === skill._id ? 0 : 1; - const removedSkillAliases = aliasRemovedForNewSlug ? 1 : 0; - const addedOwnerAliases = previousAlias - ? sameSkillSlugAliasOwner(previousAlias, skill.ownerUserId, skill.ownerPublisherId) - ? 0 - : 1 - : 1; - const removedOwnerAliases = - aliasRemovedForNewSlug && - sameSkillSlugAliasOwner(aliasRemovedForNewSlug, skill.ownerUserId, skill.ownerPublisherId) - ? 1 - : 0; - await assertSkillSlugAliasQuota(ctx, { - targetSkillId: skill._id, - ownerUserId: skill.ownerUserId, - ownerPublisherId: skill.ownerPublisherId, - currentSkillAliasCount: aliasesForSkill.length, - addedSkillAliases, - removedSkillAliases, - addedOwnerAliases, - removedOwnerAliases, - }); if (existingAlias && existingAlias.skillId === skill._id) { await ctx.db.delete(existingAlias._id); @@ -11292,9 +11203,9 @@ async function mergeOwnedSkillIntoCanonicalByActor( : null; const targetCanonicalSkillId = target.canonicalSkillId ?? target._id; - const targetAliases = await listSkillSlugAliasesForSkill(ctx, target._id); + const targetAliases = await listSkillSlugAliasesForMerge(ctx, target._id); const targetAliasSlugs = new Set(targetAliases.map((alias) => alias.slug)); - const aliases = await listSkillSlugAliasesForSkill(ctx, source._id); + const aliases = await listSkillSlugAliasesForMerge(ctx, source._id); const targetPublisher = await getOwnerPublisher(ctx, { ownerPublisherId: target.ownerPublisherId, ownerUserId: target.ownerUserId, @@ -11358,27 +11269,8 @@ async function mergeOwnedSkillIntoCanonicalByActor( } } - await assertSkillSlugAliasQuota(ctx, { - targetSkillId: target._id, - ownerUserId: target.ownerUserId, - ownerPublisherId: target.ownerPublisherId, - currentSkillAliasCount: targetAliases.length, - addedSkillAliases: addedSkillAliasSlugs.size, - addedOwnerAliases: sourceOwnerMatchesTargetOwner ? addedOwnerAliasSlugs.size : 0, - }); - const willInsertSourceAlias = !sourceAlias && (!sourceOwnerMatchesTargetOwner || source.slug !== target.slug); - if (!sourceOwnerMatchesTargetOwner && willInsertSourceAlias) { - await assertSkillSlugAliasQuota(ctx, { - targetSkillId: target._id, - ownerUserId: source.ownerUserId, - ownerPublisherId: source.ownerPublisherId, - currentSkillAliasCount: targetAliases.length, - addedSkillAliases: 0, - addedOwnerAliases: 1, - }); - } for (const alias of aliases) { if (sourceOwnerMatchesTargetOwner && alias.slug === target.slug) { diff --git a/specs/slug-routing.md b/specs/slug-routing.md index 555eada8..dc65b927 100644 --- a/specs/slug-routing.md +++ b/specs/slug-routing.md @@ -135,10 +135,12 @@ protected affixes such as `openclaw-`, `-openclaw`, `official-`, or `-official` are blocked unless an internal/admin path explicitly bypasses the reserved list for a controlled migration. -Historical slug redirects are also bounded. Rename and merge may preserve old -slugs as aliases, but a single skill can keep at most five historical slug -redirects and an owner/publisher can keep at most 25. These limits prevent -alias-hoarding while preserving ordinary rename and duplicate-merge redirects. +Rename and merge preserve historical slugs as aliases in the owning publisher's +namespace. Historical aliases are not quota-limited because they do not reserve +names across other publishers. Collision checks still prevent two skills or +aliases from claiming the same slug within one publisher namespace. +The one-shot merge mutation keeps a separate operational ceiling on aliases it +can rewrite in one transaction; larger histories require a batched migration. Owner-initiated unpublishes must not reserve a slug forever. When an owner soft-deletes a skill, the slug remains reserved for 30 days so they can restore